diff mbox series

[net-next,v2,1/3] net: phy: Allow mdio buses to auto-probe c45 devices

Message ID 20200622081914.2807-2-calvin.johnson@oss.nxp.com
State New
Headers show
Series [net-next,v2,1/3] net: phy: Allow mdio buses to auto-probe c45 devices | expand

Commit Message

Calvin Johnson June 22, 2020, 8:19 a.m. UTC 
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 that 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>


---

Changes in v2:
- Reserve "0" to mean that no mdiobus capabilities have been declared.

 drivers/net/phy/mdio_bus.c | 17 +++++++++++++++--
 include/linux/phy.h        |  8 ++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Madalin Bucur (OSS) June 22, 2020, 9:29 a.m. UTC | #1 
> -----Original Message-----

> From: Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>

> Sent: Monday, June 22, 2020 11:19 AM

> To: Jeremy Linton <jeremy.linton@arm.com>; Russell King - ARM Linux admin

> <linux@armlinux.org.uk>; Jon <jon@solid-run.com>; Cristi Sovaiala

> <cristian.sovaiala@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; Andrew

> Lunn <andrew@lunn.ch>; Andy Shevchenko <andy.shevchenko@gmail.com>;

> Florian Fainelli <f.fainelli@gmail.com>; Madalin Bucur (OSS)

> <madalin.bucur@oss.nxp.com>

> Cc: linux.cj@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)

> <calvin.johnson@oss.nxp.com>

> Subject: [net-next PATCH v2 1/3] net: phy: Allow mdio buses to auto-probe

> c45 devices

> 

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


How is this default working for existing users, the code below does not seem
to do anything for a zeroed struct, as there is no default in the switch?

> 

> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

> 

> ---

> 

> Changes in v2:

> - Reserve "0" to mean that no mdiobus capabilities have been declared.

> 

>  drivers/net/phy/mdio_bus.c | 17 +++++++++++++++--

>  include/linux/phy.h        |  8 ++++++++

>  2 files changed, 23 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..7860d56c6bf5 100644

> --- a/include/linux/phy.h

> +++ b/include/linux/phy.h

> @@ -298,6 +298,14 @@ struct mii_bus {

>  	/* RESET GPIO descriptor pointer */

>  	struct gpio_desc *reset_gpiod;

> 

> +	/* bus capabilities, used for probing */

> +	enum {

> +		MDIOBUS_NO_CAP = 0,

> +		MDIOBUS_C22,

> +		MDIOBUS_C45,

> +		MDIOBUS_C22_C45,

> +	} probe_capabilities;

> +

>  	/* protect access to the shared element */

>  	struct mutex shared_lock;

> 

> --

> 2.17.1
Calvin Johnson June 22, 2020, 1:16 p.m. UTC | #2 
On Mon, Jun 22, 2020 at 09:29:17AM +0000, Madalin Bucur (OSS) wrote:
> > -----Original Message-----

> > From: Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>

> > Sent: Monday, June 22, 2020 11:19 AM

> > To: Jeremy Linton <jeremy.linton@arm.com>; Russell King - ARM Linux admin

> > <linux@armlinux.org.uk>; Jon <jon@solid-run.com>; Cristi Sovaiala

> > <cristian.sovaiala@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; Andrew

> > Lunn <andrew@lunn.ch>; Andy Shevchenko <andy.shevchenko@gmail.com>;

> > Florian Fainelli <f.fainelli@gmail.com>; Madalin Bucur (OSS)

> > <madalin.bucur@oss.nxp.com>

> > Cc: linux.cj@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)

> > <calvin.johnson@oss.nxp.com>

> > Subject: [net-next PATCH v2 1/3] net: phy: Allow mdio buses to auto-probe

> > c45 devices

> > 

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

> 

> How is this default working for existing users, the code below does not seem

> to do anything for a zeroed struct, as there is no default in the switch?


Looking further into this, I think MDIOBUS_C22 = 0, was correct. Prior to
this patch, get_phy_device() was executed for C22 in this path. I'll discuss
with Russell and Andrew on this and get back.

> 

> > 

> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

> > 

> > ---

> > 

> > Changes in v2:

> > - Reserve "0" to mean that no mdiobus capabilities have been declared.

> > 

> >  drivers/net/phy/mdio_bus.c | 17 +++++++++++++++--

> >  include/linux/phy.h        |  8 ++++++++

> >  2 files changed, 23 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..7860d56c6bf5 100644

> > --- a/include/linux/phy.h

> > +++ b/include/linux/phy.h

> > @@ -298,6 +298,14 @@ struct mii_bus {

> >  	/* RESET GPIO descriptor pointer */

> >  	struct gpio_desc *reset_gpiod;

> > 

> > +	/* bus capabilities, used for probing */

> > +	enum {

> > +		MDIOBUS_NO_CAP = 0,

> > +		MDIOBUS_C22,

> > +		MDIOBUS_C45,

> > +		MDIOBUS_C22_C45,

> > +	} probe_capabilities;

> > +

> >  	/* protect access to the shared element */

> >  	struct mutex shared_lock;

> > 

> > --

> > 2.17.1

>
Russell King (Oracle) June 22, 2020, 1:36 p.m. UTC | #3 
On Mon, Jun 22, 2020 at 06:46:52PM +0530, Calvin Johnson wrote:
> On Mon, Jun 22, 2020 at 09:29:17AM +0000, Madalin Bucur (OSS) wrote:

> > > -----Original Message-----

> > > From: Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>

> > > Sent: Monday, June 22, 2020 11:19 AM

> > > To: Jeremy Linton <jeremy.linton@arm.com>; Russell King - ARM Linux admin

> > > <linux@armlinux.org.uk>; Jon <jon@solid-run.com>; Cristi Sovaiala

> > > <cristian.sovaiala@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; Andrew

> > > Lunn <andrew@lunn.ch>; Andy Shevchenko <andy.shevchenko@gmail.com>;

> > > Florian Fainelli <f.fainelli@gmail.com>; Madalin Bucur (OSS)

> > > <madalin.bucur@oss.nxp.com>

> > > Cc: linux.cj@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)

> > > <calvin.johnson@oss.nxp.com>

> > > Subject: [net-next PATCH v2 1/3] net: phy: Allow mdio buses to auto-probe

> > > c45 devices

> > > 

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

> > 

> > How is this default working for existing users, the code below does not seem

> > to do anything for a zeroed struct, as there is no default in the switch?

> 

> Looking further into this, I think MDIOBUS_C22 = 0, was correct. Prior to

> this patch, get_phy_device() was executed for C22 in this path. I'll discuss

> with Russell and Andrew on this and get back.


It is not correct for the reasons I stated when I made the comment.
When you introduce "probe_capabilities", every MDIO bus will have
that field as zero.

In your original patch, that means the bus only supports clause 22.
However, we have buses today that _that_ is factually incorrect.
Therefore, introducing probe_capabilities with zero meaning MDIOBUS_C22
is wrong.  It means we can _never_ assume that bus->probe_capabilities
means the bus does not support Clause 45.

Now, as per your patch below, that is better.  It means we're able to
identify those drivers that have not declared which bus access methods
are supported, while we can positively identify those which have.

All that's needed is for your switch() statement to maintain today's
behaviour where no declared probe_capabilities means that the bus
should be probed for clause 22 PHYs.

This means we can later introduce the ability to prevent clause 45
probing for PHYs that declare themselves as explicitly only supporting
clause 22 if we need to without having been backed into a corner, and
left wondering whether the lack of probe_capabilities is because someone
decided "it's zero, so doesn't need to be initialised" and didn't bother
explicitly stating .probe_capabilities = MDIOBUS_C22.

> > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

> > > 

> > > ---

> > > 

> > > Changes in v2:

> > > - Reserve "0" to mean that no mdiobus capabilities have been declared.

> > > 

> > >  drivers/net/phy/mdio_bus.c | 17 +++++++++++++++--

> > >  include/linux/phy.h        |  8 ++++++++

> > >  2 files changed, 23 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..7860d56c6bf5 100644

> > > --- a/include/linux/phy.h

> > > +++ b/include/linux/phy.h

> > > @@ -298,6 +298,14 @@ struct mii_bus {

> > >  	/* RESET GPIO descriptor pointer */

> > >  	struct gpio_desc *reset_gpiod;

> > > 

> > > +	/* bus capabilities, used for probing */

> > > +	enum {

> > > +		MDIOBUS_NO_CAP = 0,

> > > +		MDIOBUS_C22,

> > > +		MDIOBUS_C45,

> > > +		MDIOBUS_C22_C45,

> > > +	} probe_capabilities;

> > > +

> > >  	/* protect access to the shared element */

> > >  	struct mutex shared_lock;

> > > 

> > > --

> > > 2.17.1

> > 

> 


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Calvin Johnson June 22, 2020, 2:15 p.m. UTC | #4 
On Mon, Jun 22, 2020 at 02:36:19PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Jun 22, 2020 at 06:46:52PM +0530, Calvin Johnson wrote:

> > On Mon, Jun 22, 2020 at 09:29:17AM +0000, Madalin Bucur (OSS) wrote:

> > > > -----Original Message-----

> > > > From: Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>

> > > > Sent: Monday, June 22, 2020 11:19 AM

> > > > To: Jeremy Linton <jeremy.linton@arm.com>; Russell King - ARM Linux admin

> > > > <linux@armlinux.org.uk>; Jon <jon@solid-run.com>; Cristi Sovaiala

> > > > <cristian.sovaiala@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; Andrew

> > > > Lunn <andrew@lunn.ch>; Andy Shevchenko <andy.shevchenko@gmail.com>;

> > > > Florian Fainelli <f.fainelli@gmail.com>; Madalin Bucur (OSS)

> > > > <madalin.bucur@oss.nxp.com>

> > > > Cc: linux.cj@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)

> > > > <calvin.johnson@oss.nxp.com>

> > > > Subject: [net-next PATCH v2 1/3] net: phy: Allow mdio buses to auto-probe

> > > > c45 devices

> > > > 

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

> > > 

> > > How is this default working for existing users, the code below does not seem

> > > to do anything for a zeroed struct, as there is no default in the switch?

> > 

> > Looking further into this, I think MDIOBUS_C22 = 0, was correct. Prior to

> > this patch, get_phy_device() was executed for C22 in this path. I'll discuss

> > with Russell and Andrew on this and get back.

> 

> It is not correct for the reasons I stated when I made the comment.

> When you introduce "probe_capabilities", every MDIO bus will have

> that field as zero.

> 

> In your original patch, that means the bus only supports clause 22.

> However, we have buses today that _that_ is factually incorrect.

> Therefore, introducing probe_capabilities with zero meaning MDIOBUS_C22

> is wrong.  It means we can _never_ assume that bus->probe_capabilities

> means the bus does not support Clause 45.

> 

> Now, as per your patch below, that is better.  It means we're able to

> identify those drivers that have not declared which bus access methods

> are supported, while we can positively identify those which have.

> 

> All that's needed is for your switch() statement to maintain today's

> behaviour where no declared probe_capabilities means that the bus

> should be probed for clause 22 PHYs.

> 

> This means we can later introduce the ability to prevent clause 45

> probing for PHYs that declare themselves as explicitly only supporting

> clause 22 if we need to without having been backed into a corner, and

> left wondering whether the lack of probe_capabilities is because someone

> decided "it's zero, so doesn't need to be initialised" and didn't bother

> explicitly stating .probe_capabilities = MDIOBUS_C22.


Got it. I'll add the MDIOBUS_NO_CAP case also.

Thanks
Calvin
diff mbox series

Patch

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..7860d56c6bf5 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -298,6 +298,14 @@  struct mii_bus {
 	/* RESET GPIO descriptor pointer */
 	struct gpio_desc *reset_gpiod;
 
+	/* bus capabilities, used for probing */
+	enum {
+		MDIOBUS_NO_CAP = 0,
+		MDIOBUS_C22,
+		MDIOBUS_C45,
+		MDIOBUS_C22_C45,
+	} probe_capabilities;
+
 	/* protect access to the shared element */
 	struct mutex shared_lock;