Re: [PATCH net-next v3 06/12] net: ethernet: oa_tc6: implement internal PHY initialization

[<Parthiban.Veerasooran@microchip.com>]

Hi Andrew,

On 07/03/24 6:43 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +/* PHY Clause 22 and 29 registers base address and mask */
>> +#define OA_TC6_PHY_STD_REG_ADDR_BASE         0xFF00
>> +#define OA_TC6_PHY_STD_REG_ADDR_MASK         0x3F
> 
> [Goes and looks a 802.3]
> 
> Clause 29 is "System considerations for multisegment 100BASE-T networks"
> 
> I don't see any mention of registers in there.
> 
> TC6 says:
> 
> "Clause 22 standard registers and Clause 22 extended registers (Clause
> 29) are directly mapped into MMS 0 as shown in Table 7."
> 
> Going back to 802.3, we have 22.2.4:
> 
> The MII basic register set consists of two registers referred to as
> the Control register (Register 0) and the Status register (Register
> 1). All PHYs that provide an MII Management Interface shall
> incorporate the basic register set. All PHYs that provide a GMII shall
> incorporate an extended basic register set consisting of the Control
> register (Register 0), Status register (Register 1), and Extended
> Status register (Register 15). The status and control functions
> defined here are considered basic and fundamental to 100 Mb/s and 1000
> Mb/s PHYs. Registers 2 through 14 are part of the extended register
> set. The format of Registers 4 through 10 are defined for the specific
> Auto-Negotiation protocol used (Clause 28 or Clause 37). The format of
> these registers is selected by the bit settings of Registers 1 and 15.
> 
> So clause 29 is not making much sense here. Can anybody explain it?
> 
>> +static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
>> +{
>> +     int ret;
>> +
>> +     tc6->mdiobus = mdiobus_alloc();
>> +     if (!tc6->mdiobus) {
>> +             netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     tc6->mdiobus->priv = tc6;
>> +     tc6->mdiobus->read = oa_tc6_mdiobus_direct_read;
>> +     tc6->mdiobus->write = oa_tc6_mdiobus_direct_write;
> 
> This might get answered in later patches. PLCA registers are in C45
> address space, VEND1 if i remember correctly. You don't provide any
> C45 access methods here. Does TC6 specify that C45 over C22 must be
> implemented?
No the spec doesn't say anything like this. But, as C22 registers are 
mapped in the MMS 0, registers 0xD and 0xE can be used to access C45 
registers indirectly. That's why the driver implemented the above 
functions. I agree that indirect access is slower and requires more 
control commands than direct access. So implementing the direct access 
of C45 registers will overcome this issue.
> 
> The standard does say:
> 
> Vendor specific registers may be mapped into MMS 10 though MMS
> 15. When directly mapped, PHY vendor specific registers in MMD 30 or
> MMD 31 would be mapped into the vendor specific MMS 10 through MMS 15.
> 
> So i'm thinking you might need to provide C45 access, at least MMD 30,
> via MMS 10-15?
Thanks for this detailed comment. If understand you correctly by 
consolidating all your above explanations, the driver should provide C45 
access to the PHY vendor specific and PLCA registers (MMD 31). As per 
the specification, Table 6 describes the Register Memory Map Selector 
(MMS) Assignment. In this, MMS 4 maps the PHY vendor specific and PLCA 
registers. They are in the MMD 31 address space as per spec. They can be 
directly accessed using read_c45 and write_c45 functions in the mdio bus.

In Microchip's MAC-PHY (LAN8650), PHY – Vendor Specific and PLCA 
Registers (MMD 31) mapped in the MMS 4 as per the table 6 in the spec.
There is no other PHY vendor specific registers are mapped in the MMS 10 
through 15. No idea whether any other vendor's MAC-PHY uses MMS 10 
through 15 to map PHY – Vendor Specific and PLCA Registers (MMD 31).

I have given the code below for the C45 access methods. Kindly check is 
this something you expected?

--- Code starts ---

/* PHY – Vendor Specific and PLCA Registers (MMD 31) */ 

#define OA_TC6_PHY_VS_PLCA_REG_ADDR_BASE        0x40000
,,,

static int oa_tc6_mdiobus_read_c45(struct mii_bus *bus, int addr, int 
devnum, int regnum)
{ 

         struct oa_tc6 *tc6 = bus->priv; 

         u32 regval; 

         bool ret; 

 

         ret = oa_tc6_read_register(tc6, 
OA_TC6_PHY_VS_PLCA_REG_ADDR_BASE | regnum, &regval); 

         if (ret) 

                 return -ENODEV; 

 

         return regval; 

} 

 

static int oa_tc6_mdiobus_write_c45(struct mii_bus *bus, int addr, int 
devnum, int regnum, u16 val)
{ 

         struct oa_tc6 *tc6 = bus->priv; 

 

         return oa_tc6_write_register(tc6, 
OA_TC6_PHY_VS_PLCA_REG_ADDR_BASE | regnum, val); 

}
,,,

tc6->mdiobus->read_c45 = oa_tc6_mdiobus_read_c45;
tc6->mdiobus->write_c45 = oa_tc6_mdiobus_write_c45;

--- Code ends ---

Best regrads,
Parthiban V

> 
>      Andrew
>