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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Aug 02, 2023 at 07:11:27PM +0200, Michael Walle wrote:
> I'm talking about
> 
> 	u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
> 	if (!phydev->is_c45 ||
> 	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
> 		return -ENODEV;
> 
> How should that look like after this series?

In the case of the marvell10g driver, at least the 88x3310 can be
accessed via clause 45 bus cycles _or_ the clause 45 indirect
register access via clause 22. I'm not sure what the clause 22
registers would contain, whether they contain valid values with
the exception of the clause 45 indirect access registers because
there's not enough information in the documentation, and I can't
access the clause 22 registers on real hardware.

Another issue is this PHY has two different ID values. One is
0x002b 0x09aa, the other is 0x0141 0x0daa. One or other of these
values appears in the MMDs - in other words, they are not
consistently used. This means it's impossible to guess what value
may be in the clause 22 ID registers for this PHY.

However, given the way phylib works, the above effectively ensures
that we detected the PHY using clause 45 accesses, and then goes on
to verify that we do indeed have the PMAPMD MMD and the AN MMD.
Effectively, it ensures that get_phy_c45_ids() was used to detect
the device.

So, in effect, this code is ensuring that we discovered the PHY
using clause 45 accesses, and that at a minimum the PHY also
indicates that the PMAPMD and AN MMDs are implemented.

For the bcm84881 driver, it's a similar situation - that's only
ever been used with a bus that supports _only_ clause 45 accesses.
Even less idea whether the PHY could respond to clause 22 accesses
as there is no information available on the PHY.

So, I'd suggest both of these are the same - returns -ENODEV if
the bus doesn't support clause 45 transfers or if the two MMDs
are not indicated.

That said, it _can_ be simplified down to just testing the
devices_in_package member, because that will only be non-zero if
we successfully probed the PHY via some accessible way to the
clause 45 registers. So:

	if ((phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
		return -ENODEV;

is probably entirely sufficient in both cases.

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