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.lin...@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.lin...@arm.com>
> > Signed-off-by: Calvin Johnson <calvin.john...@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
