| Message ID | 20200617171536.12014-2-calvin.johnson@oss.nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ACPI support for xgmac_mdio drivers. | expand |
On Wed, Jun 17, 2020 at 10:45:33PM +0530, Calvin Johnson wrote: > From: Jeremy Linton <jeremy.linton@arm.com> > > The mdiobus_scan logic is currently hardcoded to only > work with c22 devices. This works fairly well in most > cases, but its possible a c45 device doesn't respond > despite being a standard phy. If the parent hardware > is capable, it makes sense to scan for c22 devices before > falling back to c45. > > As we want this to reflect the capabilities of the STA, > lets add a field to the mii_bus structure to represent > the capability. That way devices can opt into the extended > scanning. Existing users should continue to default to c22 > only scanning as long as they are zero'ing the structure > before use. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> I know that we've hashed this out quite a bit already, but I would like to point out that include/linux/mdio.h contains: * struct mdio_if_info - Ethernet controller MDIO interface * @mode_support: MDIO modes supported. If %MDIO_SUPPORTS_C22 is set then * MII register access will be passed through with @devad = * %MDIO_DEVAD_NONE. If %MDIO_EMULATE_C22 is set then access to * commonly used clause 22 registers will be translated into * clause 45 registers. #define MDIO_SUPPORTS_C22 1 #define MDIO_SUPPORTS_C45 2 #define MDIO_EMULATE_C22 4 While this structure is not applicable to phylib or mii_bus, it may be worth considering that there already exist definitions for identifying the properties of the underlying bus. > --- > > drivers/net/phy/mdio_bus.c | 17 +++++++++++++++-- > include/linux/phy.h | 7 +++++++ > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > index 6ceee82b2839..e6c179b89907 100644 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c > @@ -739,10 +739,23 @@ EXPORT_SYMBOL(mdiobus_free); > */ > struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr) > { > - struct phy_device *phydev; > + struct phy_device *phydev = ERR_PTR(-ENODEV); > int err; > > - phydev = get_phy_device(bus, addr, false); > + switch (bus->probe_capabilities) { > + case MDIOBUS_C22: > + phydev = get_phy_device(bus, addr, false); > + break; > + case MDIOBUS_C45: > + phydev = get_phy_device(bus, addr, true); > + break; > + case MDIOBUS_C22_C45: > + phydev = get_phy_device(bus, addr, false); > + if (IS_ERR(phydev)) > + phydev = get_phy_device(bus, addr, true); > + break; > + } > + > if (IS_ERR(phydev)) > return phydev; > > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 9248dd2ce4ca..50e5312b2304 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -298,6 +298,13 @@ struct mii_bus { > /* RESET GPIO descriptor pointer */ > struct gpio_desc *reset_gpiod; > > + /* bus capabilities, used for probing */ > + enum { > + MDIOBUS_C22 = 0, > + MDIOBUS_C45, > + MDIOBUS_C22_C45, > + } probe_capabilities; I think it would be better to reserve "0" to mean that no capabilities have been declared. We hae the situation where we have mii_bus that exist which do support C45, but as they stand, probe_capabilities will be zero, and with your definitions above, that means MDIOBUS_C22. It seems this could lock in some potential issues later down the line if we want to use this elsewhere. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> > + /* bus capabilities, used for probing */ > > + enum { > > + MDIOBUS_C22 = 0, > > + MDIOBUS_C45, > > + MDIOBUS_C22_C45, > > + } probe_capabilities; > > I think it would be better to reserve "0" to mean that no capabilities > have been declared. We hae the situation where we have mii_bus that > exist which do support C45, but as they stand, probe_capabilities will > be zero, and with your definitions above, that means MDIOBUS_C22. > > It seems this could lock in some potential issues later down the line > if we want to use this elsewhere. Hi Russell Actually, this patch already causes issues, i think. drivers/net/ethernet/marvell/mvmdio.c contains two MDIO bus master drivers. "marvell,orion-mdio" is C22 only. "marvell,xmdio" is C45 only. Because the capabilites is not initialized, it will default to 0, aka MDIOBUS_C22, for the C45 only bus master, and i expect bad things will happen. We need the value of 0 to cause existing behaviour. Or all MDIO bus drivers need reviewing, and the correct capabilities set. Andrew
On Wed, Jun 17, 2020 at 08:51:20PM +0200, Andrew Lunn wrote: > > > + /* bus capabilities, used for probing */ > > > + enum { > > > + MDIOBUS_C22 = 0, > > > + MDIOBUS_C45, > > > + MDIOBUS_C22_C45, > > > + } probe_capabilities; > > > > I think it would be better to reserve "0" to mean that no capabilities > > have been declared. We hae the situation where we have mii_bus that > > exist which do support C45, but as they stand, probe_capabilities will > > be zero, and with your definitions above, that means MDIOBUS_C22. > > > > It seems this could lock in some potential issues later down the line > > if we want to use this elsewhere. > > Hi Russell > > Actually, this patch already causes issues, i think. > > drivers/net/ethernet/marvell/mvmdio.c contains two MDIO bus master > drivers. "marvell,orion-mdio" is C22 only. "marvell,xmdio" is C45 > only. Because the capabilites is not initialized, it will default to > 0, aka MDIOBUS_C22, for the C45 only bus master, and i expect bad > things will happen. > > We need the value of 0 to cause existing behaviour. Or all MDIO bus > drivers need reviewing, and the correct capabilities set. Yes, that's basically what I was trying to say, thanks for putting it more clearly. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Jun 17, 2020 at 06:44:51PM +0100, Russell King - ARM Linux admin wrote: > On Wed, Jun 17, 2020 at 10:45:33PM +0530, Calvin Johnson wrote: > > From: Jeremy Linton <jeremy.linton@arm.com> > > > > The mdiobus_scan logic is currently hardcoded to only > > work with c22 devices. This works fairly well in most > > cases, but its possible a c45 device doesn't respond > > despite being a standard phy. If the parent hardware > > is capable, it makes sense to scan for c22 devices before > > falling back to c45. > > > > As we want this to reflect the capabilities of the STA, > > lets add a field to the mii_bus structure to represent > > the capability. That way devices can opt into the extended > > scanning. Existing users should continue to default to c22 > > only scanning as long as they are zero'ing the structure > > before use. > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > I know that we've hashed this out quite a bit already, but I would like > to point out that include/linux/mdio.h contains: > > * struct mdio_if_info - Ethernet controller MDIO interface > * @mode_support: MDIO modes supported. If %MDIO_SUPPORTS_C22 is set then > * MII register access will be passed through with @devad = > * %MDIO_DEVAD_NONE. If %MDIO_EMULATE_C22 is set then access to > * commonly used clause 22 registers will be translated into > * clause 45 registers. > > #define MDIO_SUPPORTS_C22 1 > #define MDIO_SUPPORTS_C45 2 > #define MDIO_EMULATE_C22 4 > > While this structure is not applicable to phylib or mii_bus, it may be > worth considering that there already exist definitions for identifying > the properties of the underlying bus. Can we reuse these or go ahead with the new MDIOBUS_C22? > > > --- > > > > drivers/net/phy/mdio_bus.c | 17 +++++++++++++++-- > > include/linux/phy.h | 7 +++++++ > > 2 files changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > > index 6ceee82b2839..e6c179b89907 100644 > > --- a/drivers/net/phy/mdio_bus.c > > +++ b/drivers/net/phy/mdio_bus.c > > @@ -739,10 +739,23 @@ EXPORT_SYMBOL(mdiobus_free); > > */ > > struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr) > > { > > - struct phy_device *phydev; > > + struct phy_device *phydev = ERR_PTR(-ENODEV); > > int err; > > > > - phydev = get_phy_device(bus, addr, false); > > + switch (bus->probe_capabilities) { > > + case MDIOBUS_C22: > > + phydev = get_phy_device(bus, addr, false); > > + break; > > + case MDIOBUS_C45: > > + phydev = get_phy_device(bus, addr, true); > > + break; > > + case MDIOBUS_C22_C45: > > + phydev = get_phy_device(bus, addr, false); > > + if (IS_ERR(phydev)) > > + phydev = get_phy_device(bus, addr, true); > > + break; > > + } > > + > > if (IS_ERR(phydev)) > > return phydev; > > > > diff --git a/include/linux/phy.h b/include/linux/phy.h > > index 9248dd2ce4ca..50e5312b2304 100644 > > --- a/include/linux/phy.h > > +++ b/include/linux/phy.h > > @@ -298,6 +298,13 @@ struct mii_bus { > > /* RESET GPIO descriptor pointer */ > > struct gpio_desc *reset_gpiod; > > > > + /* bus capabilities, used for probing */ > > + enum { > > + MDIOBUS_C22 = 0, > > + MDIOBUS_C45, > > + MDIOBUS_C22_C45, > > + } probe_capabilities; > > I think it would be better to reserve "0" to mean that no capabilities > have been declared. We hae the situation where we have mii_bus that > exist which do support C45, but as they stand, probe_capabilities will > be zero, and with your definitions above, that means MDIOBUS_C22. > > It seems this could lock in some potential issues later down the line > if we want to use this elsewhere. I'll change it to : enum { MDIOBUS_C22 = 1, MDIOBUS_C45, MDIOBUS_C22_C45, } probe_capabilities; Thanks Calvin
On Wed, Jun 17, 2020 at 07:57:03PM +0100, Russell King - ARM Linux admin wrote: > On Wed, Jun 17, 2020 at 08:51:20PM +0200, Andrew Lunn wrote: > > > > + /* bus capabilities, used for probing */ > > > > + enum { > > > > + MDIOBUS_C22 = 0, > > > > + MDIOBUS_C45, > > > > + MDIOBUS_C22_C45, > > > > + } probe_capabilities; > > > > > > I think it would be better to reserve "0" to mean that no capabilities > > > have been declared. We hae the situation where we have mii_bus that > > > exist which do support C45, but as they stand, probe_capabilities will > > > be zero, and with your definitions above, that means MDIOBUS_C22. > > > > > > It seems this could lock in some potential issues later down the line > > > if we want to use this elsewhere. > > > > Hi Russell > > > > Actually, this patch already causes issues, i think. > > > > drivers/net/ethernet/marvell/mvmdio.c contains two MDIO bus master > > drivers. "marvell,orion-mdio" is C22 only. "marvell,xmdio" is C45 > > only. Because the capabilites is not initialized, it will default to > > 0, aka MDIOBUS_C22, for the C45 only bus master, and i expect bad > > things will happen. > > > > We need the value of 0 to cause existing behaviour. Or all MDIO bus > > drivers need reviewing, and the correct capabilities set. > > Yes, that's basically what I was trying to say, thanks for putting it > more clearly. Prior to this patch, below code which is for C22 was executed in this path. phydev = get_phy_device(bus, addr, false); Doesn't this mean that MDIOBUS_C22 = 0, doesn't break anything? I think for DT cases, there wasn't autoprobing for PHYs and this path wasn't taken. Regards Calvin
On Mon, Jun 22, 2020 at 06:52:00PM +0530, Calvin Johnson wrote: > On Wed, Jun 17, 2020 at 07:57:03PM +0100, Russell King - ARM Linux admin wrote: > > On Wed, Jun 17, 2020 at 08:51:20PM +0200, Andrew Lunn wrote: > > > > > + /* bus capabilities, used for probing */ > > > > > + enum { > > > > > + MDIOBUS_C22 = 0, > > > > > + MDIOBUS_C45, > > > > > + MDIOBUS_C22_C45, > > > > > + } probe_capabilities; > > > > > > > > I think it would be better to reserve "0" to mean that no capabilities > > > > have been declared. We hae the situation where we have mii_bus that > > > > exist which do support C45, but as they stand, probe_capabilities will > > > > be zero, and with your definitions above, that means MDIOBUS_C22. > > > > > > > > It seems this could lock in some potential issues later down the line > > > > if we want to use this elsewhere. > > > > > > Hi Russell > > > > > > Actually, this patch already causes issues, i think. > > > > > > drivers/net/ethernet/marvell/mvmdio.c contains two MDIO bus master > > > drivers. "marvell,orion-mdio" is C22 only. "marvell,xmdio" is C45 > > > only. Because the capabilites is not initialized, it will default to > > > 0, aka MDIOBUS_C22, for the C45 only bus master, and i expect bad > > > things will happen. > > > > > > We need the value of 0 to cause existing behaviour. Or all MDIO bus > > > drivers need reviewing, and the correct capabilities set. > > > > Yes, that's basically what I was trying to say, thanks for putting it > > more clearly. > > Prior to this patch, below code which is for C22 was executed in this path. > phydev = get_phy_device(bus, addr, false); > Doesn't this mean that MDIOBUS_C22 = 0, doesn't break anything? Please re-read Andrew's email that you quoted above, and consider this point: If bus->probe_capabilities == MDIOBUS_C22 for the Marvell XMDIO device, as it _will_ be the case, because bus->probe_capabilities has not been set, is this a sane situation? Are you willing to do a full audit of all MDIO drivers and set their bus->probe_capabilities correctly in every case in order to use MDIOBUS_C22 = 0 ? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 6ceee82b2839..e6c179b89907 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -739,10 +739,23 @@ EXPORT_SYMBOL(mdiobus_free); */ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr) { - struct phy_device *phydev; + struct phy_device *phydev = ERR_PTR(-ENODEV); int err; - phydev = get_phy_device(bus, addr, false); + switch (bus->probe_capabilities) { + case MDIOBUS_C22: + phydev = get_phy_device(bus, addr, false); + break; + case MDIOBUS_C45: + phydev = get_phy_device(bus, addr, true); + break; + case MDIOBUS_C22_C45: + phydev = get_phy_device(bus, addr, false); + if (IS_ERR(phydev)) + phydev = get_phy_device(bus, addr, true); + break; + } + if (IS_ERR(phydev)) return phydev; diff --git a/include/linux/phy.h b/include/linux/phy.h index 9248dd2ce4ca..50e5312b2304 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -298,6 +298,13 @@ struct mii_bus { /* RESET GPIO descriptor pointer */ struct gpio_desc *reset_gpiod; + /* bus capabilities, used for probing */ + enum { + MDIOBUS_C22 = 0, + MDIOBUS_C45, + MDIOBUS_C22_C45, + } probe_capabilities; + /* protect access to the shared element */ struct mutex shared_lock;