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

Parthiban.Veerasooran
2024-03-07T14:42:10Z

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
>