[PATCH v1 0/3] ACPI support for xgmac_mdio drivers.

[PATCH v1 0/3] ACPI support for xgmac_mdio drivers.

[Calvin Johnson]

This patch series provides ACPI support for xgmac_mdio driver.
This patchset depends on the following patchset:
https://www.spinics.net/lists/netdev/msg656492.html



Jeremy Linton (3):
  net: phy: Allow mdio buses to auto-probe c45 devices
  net/fsl: acpize xgmac_mdio
  net/fsl: enable extended scanning in xgmac_mdio

 drivers/net/ethernet/freescale/xgmac_mdio.c | 28 +++++++++++++--------
 drivers/net/phy/mdio_bus.c                  | 17 +++++++++++--
 include/linux/phy.h                         |  7 ++++++
 3 files changed, 40 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH v1 1/3] net: phy: Allow mdio buses to auto-probe c45 devices

[Calvin Johnson]

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

 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;
+
 	/* protect access to the shared element */
 	struct mutex shared_lock;
 
-- 
2.17.1

Re: [PATCH v1 1/3] net: phy: Allow mdio buses to auto-probe c45 devices

[Russell King - ARM Linux admin]

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!

Re: [PATCH v1 1/3] net: phy: Allow mdio buses to auto-probe c45 devices

[Andrew Lunn]

> > +	/* 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

Re: [PATCH v1 1/3] net: phy: Allow mdio buses to auto-probe c45 devices

[Russell King - ARM Linux admin]

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!

Re: [PATCH v1 1/3] net: phy: Allow mdio buses to auto-probe c45 devices

[Calvin Johnson]

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

Re: [PATCH v1 1/3] net: phy: Allow mdio buses to auto-probe c45 devices

[Russell King - ARM Linux admin]

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!

Re: [PATCH v1 1/3] net: phy: Allow mdio buses to auto-probe c45 devices

[Calvin Johnson]

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

[PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Calvin Johnson]

From: Jeremy Linton <jeremy.linton@arm.com>

Add ACPI support for xgmac MDIO bus registration while maintaining
the existing DT support.

The function mdiobus_register() inside of_mdiobus_register(), brings
up all the PHYs on the mdio bus and attach them to the bus.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

 drivers/net/ethernet/freescale/xgmac_mdio.c | 27 +++++++++++++--------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index c82c85ef5fb3..fb7f8caff643 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -245,14 +245,14 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct mii_bus *bus;
-	struct resource res;
+	struct resource *res;
 	struct mdio_fsl_priv *priv;
 	int ret;
 
-	ret = of_address_to_resource(np, 0, &res);
-	if (ret) {
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
 		dev_err(&pdev->dev, "could not obtain address\n");
-		return ret;
+		return -EINVAL;
 	}
 
 	bus = mdiobus_alloc_size(sizeof(struct mdio_fsl_priv));
@@ -263,21 +263,21 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 	bus->read = xgmac_mdio_read;
 	bus->write = xgmac_mdio_write;
 	bus->parent = &pdev->dev;
-	snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res.start);
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res->start);
 
 	/* Set the PHY base address */
 	priv = bus->priv;
-	priv->mdio_base = of_iomap(np, 0);
+	priv->mdio_base = ioremap(res->start, resource_size(res));
 	if (!priv->mdio_base) {
 		ret = -ENOMEM;
 		goto err_ioremap;
 	}
 
-	priv->is_little_endian = of_property_read_bool(pdev->dev.of_node,
-						       "little-endian");
+	priv->is_little_endian = device_property_read_bool(&pdev->dev,
+							   "little-endian");
 
-	priv->has_a011043 = of_property_read_bool(pdev->dev.of_node,
-						  "fsl,erratum-a011043");
+	priv->has_a011043 = device_property_read_bool(&pdev->dev,
+						      "fsl,erratum-a011043");
 
 	ret = of_mdiobus_register(bus, np);
 	if (ret) {
@@ -320,10 +320,17 @@ static const struct of_device_id xgmac_mdio_match[] = {
 };
 MODULE_DEVICE_TABLE(of, xgmac_mdio_match);
 
+static const struct acpi_device_id xgmac_acpi_match[] = {
+	{ "NXP0006", (kernel_ulong_t)NULL },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, xgmac_acpi_match);
+
 static struct platform_driver xgmac_mdio_driver = {
 	.driver = {
 		.name = "fsl-fman_xmdio",
 		.of_match_table = xgmac_mdio_match,
+		.acpi_match_table = xgmac_acpi_match,
 	},
 	.probe = xgmac_mdio_probe,
 	.remove = xgmac_mdio_remove,
-- 
2.17.1

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Andy Shevchenko]

On Wed, Jun 17, 2020 at 8:16 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> From: Jeremy Linton <jeremy.linton@arm.com>
>
> Add ACPI support for xgmac MDIO bus registration while maintaining
> the existing DT support.
>
> The function mdiobus_register() inside of_mdiobus_register(), brings
> up all the PHYs on the mdio bus and attach them to the bus.

...

> -       snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res.start);
> +       snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res->start);

Since you are here, better to change the specifier to the dedicated
one, i.e. "%pa".
But I don't remember if it adds 0x to it...

...

> +static const struct acpi_device_id xgmac_acpi_match[] = {
> +       { "NXP0006", (kernel_ulong_t)NULL },

       { "NXP0006" },

is enough. And I suppose this is the official ID.

> +       { },

No need to have comma in terminator line.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Andrew Lunn]

On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:
> From: Jeremy Linton <jeremy.linton@arm.com>

> +static const struct acpi_device_id xgmac_acpi_match[] = {
> +	{ "NXP0006", (kernel_ulong_t)NULL },

Hi Jeremy

What exactly does NXP0006 represent? An XGMAC MDIO bus master? Some
NXP MDIO bus master? An XGMAC Ethernet controller which has an NXP
MDIO bus master? A cluster of Ethernet controllers?

Is this documented somewhere? In the DT world we have a clear
documentation for all the compatible strings. Is there anything
similar in the ACPI world for these magic numbers?

Thanks
     Andrew

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Jeremy Linton]

Hi,

On 6/17/20 12:34 PM, Andrew Lunn wrote:
> On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:
>> From: Jeremy Linton <jeremy.linton@arm.com>
> 
>> +static const struct acpi_device_id xgmac_acpi_match[] = {
>> +	{ "NXP0006", (kernel_ulong_t)NULL },
> 
> Hi Jeremy
> 
> What exactly does NXP0006 represent? An XGMAC MDIO bus master? Some
> NXP MDIO bus master? An XGMAC Ethernet controller which has an NXP
> MDIO bus master? A cluster of Ethernet controllers?

Strictly speaking its a NXP defined (they own the "NXP" prefix per 
https://uefi.org/pnp_id_list) id. So, they have tied it to a specific 
bit of hardware. In this case it appears to be a shared MDIO master 
which isn't directly contained in an Ethernet controller. Its somewhat 
similar to a  "nxp,xxxxx" compatible id, depending on how they are using 
it to identify an ACPI device object (_HID()/_CID()).

So AFAIK, this is all valid ACPI usage as long as the ID maps to a 
unique device/object.

> 
> Is this documented somewhere? In the DT world we have a clear
> documentation for all the compatible strings. Is there anything
> similar in the ACPI world for these magic numbers?

Sadly not fully. The mentioned PNP and ACPI 
(https://uefi.org/acpi_id_list) ids lists are requested and registered 
to a given organization. But, once the prefix is owned, it becomes the 
responsibility of that organization to assign & manage the ID's with 
their prefix. There are various individuals/etc which have collected 
lists, though like PCI ids, there aren't any formal publishing requirements.

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Andy Shevchenko]

On Thu, Jun 18, 2020 at 6:46 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
> On 6/17/20 12:34 PM, Andrew Lunn wrote:
> > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:
> >> From: Jeremy Linton <jeremy.linton@arm.com>
> >
> >> +static const struct acpi_device_id xgmac_acpi_match[] = {
> >> +    { "NXP0006", (kernel_ulong_t)NULL },
> >
> > Hi Jeremy
> >
> > What exactly does NXP0006 represent? An XGMAC MDIO bus master? Some
> > NXP MDIO bus master? An XGMAC Ethernet controller which has an NXP
> > MDIO bus master? A cluster of Ethernet controllers?
>
> Strictly speaking its a NXP defined (they own the "NXP" prefix per
> https://uefi.org/pnp_id_list) id. So, they have tied it to a specific
> bit of hardware. In this case it appears to be a shared MDIO master
> which isn't directly contained in an Ethernet controller. Its somewhat
> similar to a  "nxp,xxxxx" compatible id, depending on how they are using
> it to identify an ACPI device object (_HID()/_CID()).
>
> So AFAIK, this is all valid ACPI usage as long as the ID maps to a
> unique device/object.
>
> >
> > Is this documented somewhere? In the DT world we have a clear
> > documentation for all the compatible strings. Is there anything
> > similar in the ACPI world for these magic numbers?
>
> Sadly not fully. The mentioned PNP and ACPI
> (https://uefi.org/acpi_id_list) ids lists are requested and registered
> to a given organization. But, once the prefix is owned, it becomes the
> responsibility of that organization to assign & manage the ID's with
> their prefix. There are various individuals/etc which have collected
> lists, though like PCI ids, there aren't any formal publishing requirements.

And here is the question, do we have (in form of email or other means)
an official response from NXP about above mentioned ID?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Calvin Johnson]

Hi,

On Thu, Jun 18, 2020 at 07:00:20PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 18, 2020 at 6:46 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
> > On 6/17/20 12:34 PM, Andrew Lunn wrote:
> > > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:
> > >> From: Jeremy Linton <jeremy.linton@arm.com>
> > >
> > >> +static const struct acpi_device_id xgmac_acpi_match[] = {
> > >> +    { "NXP0006", (kernel_ulong_t)NULL },
> > >
> > > Hi Jeremy
> > >
> > > What exactly does NXP0006 represent? An XGMAC MDIO bus master? Some
> > > NXP MDIO bus master? An XGMAC Ethernet controller which has an NXP
> > > MDIO bus master? A cluster of Ethernet controllers?
> >
> > Strictly speaking its a NXP defined (they own the "NXP" prefix per
> > https://uefi.org/pnp_id_list) id. So, they have tied it to a specific
> > bit of hardware. In this case it appears to be a shared MDIO master
> > which isn't directly contained in an Ethernet controller. Its somewhat
> > similar to a  "nxp,xxxxx" compatible id, depending on how they are using
> > it to identify an ACPI device object (_HID()/_CID()).
> >
> > So AFAIK, this is all valid ACPI usage as long as the ID maps to a
> > unique device/object.
> >
> > >
> > > Is this documented somewhere? In the DT world we have a clear
> > > documentation for all the compatible strings. Is there anything
> > > similar in the ACPI world for these magic numbers?
> >
> > Sadly not fully. The mentioned PNP and ACPI
> > (https://uefi.org/acpi_id_list) ids lists are requested and registered
> > to a given organization. But, once the prefix is owned, it becomes the
> > responsibility of that organization to assign & manage the ID's with
> > their prefix. There are various individuals/etc which have collected
> > lists, though like PCI ids, there aren't any formal publishing requirements.
> 
> And here is the question, do we have (in form of email or other means)
> an official response from NXP about above mentioned ID?

At NXP, we've assgined NXP ids to various controllers and "NXP0006" is
assigned to the xgmac_mdio controller.

Regards
Calvin

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Andy Shevchenko]

On Thu, Jun 18, 2020 at 8:43 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
> On Thu, Jun 18, 2020 at 07:00:20PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 18, 2020 at 6:46 PM Jeremy Linton <jeremy.linton@arm.com> wrote:

...

> > And here is the question, do we have (in form of email or other means)
> > an official response from NXP about above mentioned ID?
>
> At NXP, we've assgined NXP ids to various controllers and "NXP0006" is
> assigned to the xgmac_mdio controller.

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Russell King - ARM Linux admin]

On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:
> From: Jeremy Linton <jeremy.linton@arm.com>
> 
> Add ACPI support for xgmac MDIO bus registration while maintaining
> the existing DT support.
> 
> The function mdiobus_register() inside of_mdiobus_register(), brings
> up all the PHYs on the mdio bus and attach them to the bus.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
>  drivers/net/ethernet/freescale/xgmac_mdio.c | 27 +++++++++++++--------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
> index c82c85ef5fb3..fb7f8caff643 100644
> --- a/drivers/net/ethernet/freescale/xgmac_mdio.c
> +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
> @@ -245,14 +245,14 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
>  	struct mii_bus *bus;
> -	struct resource res;
> +	struct resource *res;
>  	struct mdio_fsl_priv *priv;
>  	int ret;
>  
> -	ret = of_address_to_resource(np, 0, &res);
> -	if (ret) {
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
>  		dev_err(&pdev->dev, "could not obtain address\n");
> -		return ret;
> +		return -EINVAL;
>  	}

I think, as you're completely rewriting the resource handling, it would
be a good idea to switch over to using devm_* stuff here.

	void __iomem *regs;

	regs = devm_platform_ioremap_resource(pdev, 0);
	if (IS_ERR(regs)) {
		dev_err(&pdev->dev, "could not map resource: %pe\n",
			regs);
		return PTR_ERR(regs);
	}

This also gets rid of the error checking on priv->mdio_base below, and
of course the unmap/resource handling for priv->mdio_base in the error
paths and device removal paths.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Andy Shevchenko]

On Wed, Jun 17, 2020 at 8:49 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:

...

> > -     ret = of_address_to_resource(np, 0, &res);
> > -     if (ret) {
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     if (!res) {
> >               dev_err(&pdev->dev, "could not obtain address\n");
> > -             return ret;
> > +             return -EINVAL;
> >       }
>
> I think, as you're completely rewriting the resource handling, it would
> be a good idea to switch over to using devm_* stuff here.
>
>         void __iomem *regs;
>
>         regs = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(regs))
{
>                 dev_err(&pdev->dev, "could not map resource: %pe\n",
>                         regs);

And just in case, this message is dup. The API has few of them
depending on the error conditions.

>                 return PTR_ERR(regs);
>         }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Russell King - ARM Linux admin]

On Wed, Jun 17, 2020 at 08:54:23PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 17, 2020 at 8:49 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:
> 
> ...
> 
> > > -     ret = of_address_to_resource(np, 0, &res);
> > > -     if (ret) {
> > > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +     if (!res) {
> > >               dev_err(&pdev->dev, "could not obtain address\n");
> > > -             return ret;
> > > +             return -EINVAL;
> > >       }
> >
> > I think, as you're completely rewriting the resource handling, it would
> > be a good idea to switch over to using devm_* stuff here.
> >
> >         void __iomem *regs;
> >
> >         regs = devm_platform_ioremap_resource(pdev, 0);
> >         if (IS_ERR(regs))
> {
> >                 dev_err(&pdev->dev, "could not map resource: %pe\n",
> >                         regs);
> 
> And just in case, this message is dup. The API has few of them
> depending on the error conditions.

I did try to check for that, but it seems it was rather buried.  This
seems to have been a common mistake, as I've seen patches removing
such things from various drivers, but I'm never sure which require that
treatment.

Maybe adding such details to the kerneldoc for the functions (maybe
actually _writing_ some kernel doc to describe what the functions are
doing) would be a good start to prevent this kind of thing...

There's a difference between the lazy kerneldoc style of:

/**
 * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
 *                                  device
 *
 * @pdev: platform device to use both for memory resource lookup as well as
 *        resource management
 * @index: resource index
 */

and the "lets try to be informative" style:

/**
 * phy_lookup_setting - lookup a PHY setting
 * @speed: speed to match
 * @duplex: duplex to match
 * @mask: allowed link modes
 * @exact: an exact match is required
 *
 * Search the settings array for a setting that matches the speed and
 * duplex, and which is supported.
 *
 * If @exact is unset, either an exact match or %NULL for no match will
 * be returned.
 *
 * If @exact is set, an exact match, the fastest supported setting at
 * or below the specified speed, the slowest supported setting, or if
 * they all fail, %NULL will be returned.
 */

;)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Calvin Johnson]

On Wed, Jun 17, 2020 at 06:49:31PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:
> > From: Jeremy Linton <jeremy.linton@arm.com>
> > 
> > Add ACPI support for xgmac MDIO bus registration while maintaining
> > the existing DT support.
> > 
> > The function mdiobus_register() inside of_mdiobus_register(), brings
> > up all the PHYs on the mdio bus and attach them to the bus.
> > 
> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > ---
> > 
> >  drivers/net/ethernet/freescale/xgmac_mdio.c | 27 +++++++++++++--------
> >  1 file changed, 17 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
> > index c82c85ef5fb3..fb7f8caff643 100644
> > --- a/drivers/net/ethernet/freescale/xgmac_mdio.c
> > +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
> > @@ -245,14 +245,14 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node *np = pdev->dev.of_node;
> >  	struct mii_bus *bus;
> > -	struct resource res;
> > +	struct resource *res;
> >  	struct mdio_fsl_priv *priv;
> >  	int ret;
> >  
> > -	ret = of_address_to_resource(np, 0, &res);
> > -	if (ret) {
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> >  		dev_err(&pdev->dev, "could not obtain address\n");
> > -		return ret;
> > +		return -EINVAL;
> >  	}
> 
> I think, as you're completely rewriting the resource handling, it would
> be a good idea to switch over to using devm_* stuff here.
> 
> 	void __iomem *regs;
> 
> 	regs = devm_platform_ioremap_resource(pdev, 0);

I had used devm_ API earlier in this place and ran into a regression.
This mdio driver is used by both DPAA-1 and DPAA-2. In DPAA2 case, this
works fine.

But in DPAA-1 case, the existing device tree describes the memory map in a
hierarchical manner. The FMan of DPAA-1 area include the MDIO, Port, MAC areas.
Therefore, we may have to continue with existing method.

Thanks
Calvin

Re: [PATCH v1 2/3] net/fsl: acpize xgmac_mdio

[Russell King - ARM Linux admin]

On Fri, Jun 19, 2020 at 08:29:27PM +0530, Calvin Johnson wrote:
> On Wed, Jun 17, 2020 at 06:49:31PM +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Jun 17, 2020 at 10:45:34PM +0530, Calvin Johnson wrote:
> > > From: Jeremy Linton <jeremy.linton@arm.com>
> > > 
> > > Add ACPI support for xgmac MDIO bus registration while maintaining
> > > the existing DT support.
> > > 
> > > The function mdiobus_register() inside of_mdiobus_register(), brings
> > > up all the PHYs on the mdio bus and attach them to the bus.
> > > 
> > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > > ---
> > > 
> > >  drivers/net/ethernet/freescale/xgmac_mdio.c | 27 +++++++++++++--------
> > >  1 file changed, 17 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
> > > index c82c85ef5fb3..fb7f8caff643 100644
> > > --- a/drivers/net/ethernet/freescale/xgmac_mdio.c
> > > +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
> > > @@ -245,14 +245,14 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
> > >  {
> > >  	struct device_node *np = pdev->dev.of_node;
> > >  	struct mii_bus *bus;
> > > -	struct resource res;
> > > +	struct resource *res;
> > >  	struct mdio_fsl_priv *priv;
> > >  	int ret;
> > >  
> > > -	ret = of_address_to_resource(np, 0, &res);
> > > -	if (ret) {
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (!res) {
> > >  		dev_err(&pdev->dev, "could not obtain address\n");
> > > -		return ret;
> > > +		return -EINVAL;
> > >  	}
> > 
> > I think, as you're completely rewriting the resource handling, it would
> > be a good idea to switch over to using devm_* stuff here.
> > 
> > 	void __iomem *regs;
> > 
> > 	regs = devm_platform_ioremap_resource(pdev, 0);
> 
> I had used devm_ API earlier in this place and ran into a regression.
> This mdio driver is used by both DPAA-1 and DPAA-2. In DPAA2 case, this
> works fine.
> 
> But in DPAA-1 case, the existing device tree describes the memory map in a
> hierarchical manner. The FMan of DPAA-1 area include the MDIO, Port, MAC areas.
> Therefore, we may have to continue with existing method.

And you need to document that in comments in the driver, otherwise you
will have people come along and try to "clean up" the driver.

You can still use devm_ioremap() though.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH v1 3/3] net/fsl: enable extended scanning in xgmac_mdio

[Calvin Johnson]

From: Jeremy Linton <jeremy.linton@arm.com>

Since we know the xgmac hardware always has a c45
complaint bus, lets try scanning for c22 capable
phys first. If we fail to find any, then it with
fall back to c45 automatically.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>

---

 drivers/net/ethernet/freescale/xgmac_mdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index fb7f8caff643..5732ca13b821 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -263,6 +263,7 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 	bus->read = xgmac_mdio_read;
 	bus->write = xgmac_mdio_write;
 	bus->parent = &pdev->dev;
+	bus->probe_capabilities = MDIOBUS_C22_C45;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res->start);
 
 	/* Set the PHY base address */
-- 
2.17.1

Re: [PATCH v1 3/3] net/fsl: enable extended scanning in xgmac_mdio

[Florian Fainelli]

On 6/17/2020 10:15 AM, Calvin Johnson wrote:
> From: Jeremy Linton <jeremy.linton@arm.com>
> 
> Since we know the xgmac hardware always has a c45
> complaint bus, lets try scanning for c22 capable
> phys first. If we fail to find any, then it with
> fall back to c45 automatically.

s/complaint/compliant/
s/lets/let's/
s/phys/PHYs/
s/with/will/

> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> 
> ---
> 
>  drivers/net/ethernet/freescale/xgmac_mdio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
> index fb7f8caff643..5732ca13b821 100644
> --- a/drivers/net/ethernet/freescale/xgmac_mdio.c
> +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
> @@ -263,6 +263,7 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
>  	bus->read = xgmac_mdio_read;
>  	bus->write = xgmac_mdio_write;
>  	bus->parent = &pdev->dev;
> +	bus->probe_capabilities = MDIOBUS_C22_C45;
>  	snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res->start);
>  
>  	/* Set the PHY base address */
> 

-- 
Florian