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

[Michael Walle <mwalle@kernel.org>]

Am 2023-08-02 18:06, schrieb Russell King (Oracle):
> 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.

And that is exactly the one which Andrew suggested to use for almost all
cases. It doesn't make sense to me to introduce that function if it 
can't
really be made to return a correct value (in a later patch).

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

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?