Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()

[Russell King (Oracle)]

On Thu, Aug 03, 2023 at 12:21:03AM +0200, Andrew Lunn wrote:
> On Wed, Aug 02, 2023 at 06:10:27PM +0100, Russell King (Oracle) wrote:
> > On Wed, Aug 02, 2023 at 06:15:19PM +0200, Andrew Lunn wrote:
> > > > I'm confused now. Andrew suggested to split it into four different
> > > > functions:
> > > > 
> > > > phy_has_c22_registers()
> > > > phy_has_c45_registers()
> > > > phy_has_c22_transfers()
> > > > phy_has_c45_transfers()
> > > > 
> > > > Without a functional change. That is, either return phydev->is_c45
> > > > or the inverse.
> > > 
> > > Without a functional change at this step of introducing the four
> > > functions. Then later really implement them to do what the name
> > > implies. Doing it in steps helps with bisect when it breaks something.
> > > 
> > > It could also be that not all four are needed, or not all four are
> > > possible. But the four express the full combinations of transfers and
> > > registers.
> > 
> > I'm left wondering how you think that phy_has_c45_registers() can
> > be implemented in a useful way, because I'm at a total loss...
> 
> get_phy_c22_id() looks at the ID registers, and see if its mostly
> Fs. If so, it says -ENODEV. So phy_has_c22_registers() could be
> get_phy_c22_id() != -ENODEV. And you can extend that with the logic
> you quoted from the standard. get_phy_c45_ids() also returns -ENODEV
> if there are no devices in the package. So phy_has_c45_registers()
> could be get_phy_c45_id() != -ENODEV.

No, this is not correct, because get_phy_c45_id() uses mdiobus c45
reads - if the bus doesn't support c45 transfers, then get_phy_c45_id()
fails.

This doesn't tell us whether the PHY has clause 45 registers at all,
it just tells us that we probed the PHY successfully using clause 45
accesses.

The fact of the matter is that there is no way in the current phylib
code (and probably no way to probe it either) to determine whether a
PHY has clause 45 registers or not. To see why, take for example the
AR8035. This clearly has clause 45 registers, because it has MMDs 3
and 7 implemented. However, these are only accessible through the
indirect access method via clause 22 cycles.

So, does it make sense for AR8035 to return false from
phy_has_c45_registers() ? No it doesn't, because it clearly does have
clause 45 registers!

The same can be said about Marvell 88e151x PHYs. And probably many
other clause 22 PHYs.

Hence why I disagree with your suggestion.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!