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

[Andrew Lunn <andrew@lunn.ch>]

> >> +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.

It is not just about performance. It is about compliance to the
standard. The standard does not say anything about C45 over C22. So
there is no reason to expect a PHY device to implement it. It might,
but its optional.

> > 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.

Yes. I think this is required to conform to the standard.

> 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?

The code got mangled by your mail client :-(

> --- 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); 

You appear to ignore devnum. I don't think you can do that. The core
phylib code might try to access other MMDs, e.g. it might try to see
if EEE is supported, by reading MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE.

	Andrew