Messages in this thread | | | Date | Wed, 2 Aug 2023 17:06:31 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers() |
| |
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!
| |