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 05:33:20PM +0200, Michael Walle wrote:
> Am 2023-08-01 17:57, schrieb Russell King (Oracle):
> > On Tue, Aug 01, 2023 at 05:20:22PM +0200, Michael Walle wrote:
> > > > In the case of the above (the code in __phy_read_mmd()), I wouldn't
> > > > at least initially change the test there.
> > > >
> > > > phydev->is_c45 will only be true if we probed the PHY using clause
> > > > 45 accesses. Thus, it will be set if "the bus supports clause 45
> > > > accesses" _and_ "the PHY responds to those accesses".
> > > >
> > > > Changing that to only "the bus supports clause 45 accesses" means
> > > > that a PHY supporting only clause 22 access with indirect clause
> > > > 45 access then fails if it's used with a bus that supports both
> > > > clause 22 and clause 45 accesses.
> > > 
> > > Yeah of course. It was more about the naming, but I just realized
> > > that with mdiobus_supports_c45() you can't access the original
> > > "is_c45" property of the PHY. So maybe this patch needs to be split
> > > into two to get rid of .is_c45:
> > > 
> > > First a mechanical one:
> > > phy_has_c45_registers() {
> > >    return phydev->is_c45;
> > > }
> > 
> > Andrew's objection was that "phy_has_c45_registers" is a misnomer, and
> > suggested "_transfers" instead - because a PHY can have C45 registers
> > that are accessible via the indirect registers in C22 space.
> 
> 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()

Honestly, I don't think we can come up with tests that satisfy all of
these. Particularly the question whether a PHY has c45 registers or
not is a difficult one, as there is no sane way to determine that with
a clause 22 PHY.

I'm also not sure what use the c22 transfers one would be, since if a
PHY doesn't have c22 registers, then that's probably all we need to
know.

> Without a functional change. That is, either return phydev->is_c45
> or the inverse.

I think I've already explained why !phydev->is_c45 can't be interpeted
as a PHY having C22 registers, but let me restate. It is _entirely_
possible for a PHY to have C45 registers _and_ C22 registers, and
that is indicated by bit 0 of the devices-in-package field.

> 
> You seem to suggest to use either
> phy_supports_c45_transfers() or
> phy_has_c22_registers()
> 
> I'm not sure how to continue now.
> 
> > I'd go one further:
> > 
> > static bool phy_supports_c45_transfers(struct phy_device *phydev)
> > {
> > 	return phydev->is_c45;
> > }
> > 
> > Since that covers that (a) the bus needs to support C45 transfers and
> > (b) the PHY also needs to respond to C45 transfers.
> > 
> > If we want to truly know whether a clause 22 PHY has clause 45
> > registers, that's difficult to answer, because then you're into the
> > realms of "does this PHY implement the indirect access method" and
> > we haven't been keeping track of that for the PHYs we have drivers
> > for - many will do, but it's optional in clause 22. The problem is
> > that when it's not implemented, the registers could be serving some
> > other function.
> > 
> > > phy_has_c22_registers() {
> > >   return !phydev->is_c45;
> > > }
> > 
> > The reverse is not true, as clause 45 PHYs can also support clause 22
> > registers - from 802.3:
> > 
> >  "For cases where a single entity combines Clause 45 MMDs with  Clause
> > 22
> >  registers, then the Clause 22 registers may be accessed using the
> > Clause
> >  45 electrical interface and the Clause 22 management frame structure."
> > 
> >  "Bit 5.0 is used to indicate that Clause 22 functionality has been
> >  implemented within a Clause 45 electrical interface device."
> > 
> > Therefore, this would more accurately describe when Clause 22 registers
> > are present for a PHY:
> > 
> > static bool phy_has_c22_registers(struct phy_device *phydev)
> > {
> > 	/* If we probed the PHY without clause 45 accesses, then by
> > 	 * definition, clause 22 registers must be present.
> > 	 */
> > 	if (!phydev->is_c45)
> > 		return true;
> > 
> > 	/* If we probed the PHY with clause 45 accesses, clause 22
> > 	 * registers may be present if bit 0 in the Devices-in-pacakge
> > 	 * register pair is set.
> > 	 */
> > 	return phydev->c45_ids.devices_in_package & BIT(0);
> > }
> > 
> > Note that this doesn't take account of whether the bus supports clause
> > 22 register access - there are a number of MDIO buses that do not
> > support such accesses, and they may be coupled with a PHY that does
> > support clause 22 registers.
> > 
> > I'm aware of a SFP with a Realtek PHY on that falls into this exact
> > case, and getting that working is progressing at the moment.
> > 
> > > For all the places Andrew said it's correct. Leave all the
> > > other uses of .is_c45 as is for now and rework them in a
> > > later patch to use mdiobus_supports_{c22,c45}().
> > 
> > For the two cases in marvell10g and bcm84881, the test there for
> > is_c45 is purely to determine "was this ID found on a PHY supporting
> > clause 45 access" - however, in both cases, a check is made for MMDs
> > present in devices_in_package which will fail if the PHY wasn't
> > discovered in clause 45 mode.
> > 
> > Note that 88x3310 does not support clause 22 access. I forget whether
> > bcm84881 does or not.
> 
> So a simple "phydev->is_c45" should be enough? Why do you test
> for the MMD presence bits?

Okay, so if quoting the bits from IEEE 802.3 doesn't provide sufficient
explanation, I'm at a loss what would...

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