[PATCH 0/7] net: of_mdio improvements

[PATCH 0/7] net: of_mdio improvements

[Florian Fainelli]

Hi all,

This patchset contains a few improvements to the MDIO device tree parsing
code such as refactoring and parsing the "max-speed" property which is
defined in the ePAPR specification.

Thanks!

Florian Fainelli (7):
  net: of_mdio: factor PHY registration from of_mdiobus_register
  net: of_mdio: use PHY_MAX_ADDR constant
  net: of_mdio: do not overwrite PHY interrupt configuration
  net: phy: breakdown PHY_*_FEATURES defines
  net: of_mdio: parse "max-speed" property to set PHY supported features
  arc_emac: remove custom "max-speed" parsing code
  Documentation: update Ethernet PHY devices binding with 'max-speed'

 Documentation/devicetree/bindings/net/phy.txt |   1 +
 drivers/net/ethernet/arc/emac.h               |   2 -
 drivers/net/ethernet/arc/emac_main.c          |  20 +---
 drivers/of/of_mdio.c                          | 135 ++++++++++++++------------
 include/linux/phy.h                           |  23 +++--
 5 files changed, 89 insertions(+), 92 deletions(-)

-- 
1.8.3.2

[PATCH 1/7] net: of_mdio: factor PHY registration from of_mdiobus_register

[Florian Fainelli]

From: Florian Fainelli <f.fainelli@gmail.com>

Since commit 779d835e ("net: of_mdio: scan mdiobus for PHYs without reg
property") we have two foreach loops which do pretty much the same
thing. Factor the PHY device registration in a function helper:
of_mdiobus_register_phy() which takes care of the details and allows for
future PHY specific extensions.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/of_mdio.c | 109 ++++++++++++++++++++++-----------------------------
 1 file changed, 46 insertions(+), 63 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index d5a57a9..82485d2 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -22,6 +22,47 @@
 MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
 MODULE_LICENSE("GPL");
 
+static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child,
+				   u32 addr)
+{
+	struct phy_device *phy;
+	bool is_c45;
+	int rc;
+
+	is_c45 = of_device_is_compatible(child,
+					 "ethernet-phy-ieee802.3-c45");
+
+	phy = get_phy_device(mdio, addr, is_c45);
+	if (!phy || IS_ERR(phy))
+		return 1;
+
+	if (mdio->irq) {
+		mdio->irq[addr] =
+			irq_of_parse_and_map(child, 0);
+		if (!mdio->irq[addr])
+			mdio->irq[addr] = PHY_POLL;
+	}
+
+	/* Associate the OF node with the device structure so it
+	 * can be looked up later */
+	of_node_get(child);
+	phy->dev.of_node = child;
+
+	/* All data is now stored in the phy struct;
+	 * register it */
+	rc = phy_device_register(phy);
+	if (rc) {
+		phy_device_free(phy);
+		of_node_put(child);
+		return 1;
+	}
+
+	dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
+		 child->name, addr);
+
+	return 0;
+}
+
 /**
  * of_mdiobus_register - Register mii_bus and create PHYs from the device tree
  * @mdio: pointer to mii_bus structure
@@ -32,11 +73,10 @@ MODULE_LICENSE("GPL");
  */
 int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 {
-	struct phy_device *phy;
 	struct device_node *child;
 	const __be32 *paddr;
 	u32 addr;
-	bool is_c45, scanphys = false;
+	bool scanphys = false;
 	int rc, i, len;
 
 	/* Mask out all PHYs from auto probing.  Instead the PHYs listed in
@@ -73,38 +113,9 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 			continue;
 		}
 
-		if (mdio->irq) {
-			mdio->irq[addr] = irq_of_parse_and_map(child, 0);
-			if (!mdio->irq[addr])
-				mdio->irq[addr] = PHY_POLL;
-		}
-
-		is_c45 = of_device_is_compatible(child,
-						 "ethernet-phy-ieee802.3-c45");
-		phy = get_phy_device(mdio, addr, is_c45);
-
-		if (!phy || IS_ERR(phy)) {
-			dev_err(&mdio->dev,
-				"cannot get PHY at address %i\n",
-				addr);
-			continue;
-		}
-
-		/* Associate the OF node with the device structure so it
-		 * can be looked up later */
-		of_node_get(child);
-		phy->dev.of_node = child;
-
-		/* All data is now stored in the phy struct; register it */
-		rc = phy_device_register(phy);
-		if (rc) {
-			phy_device_free(phy);
-			of_node_put(child);
+		rc = of_mdiobus_register_phy(mdio, child, addr);
+		if (rc)
 			continue;
-		}
-
-		dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
-			child->name, addr);
 	}
 
 	if (!scanphys)
@@ -117,9 +128,6 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 		if (paddr)
 			continue;
 
-		is_c45 = of_device_is_compatible(child,
-						 "ethernet-phy-ieee802.3-c45");
-
 		for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
 			/* skip already registered PHYs */
 			if (mdio->phy_map[addr])
@@ -129,34 +137,9 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 			dev_info(&mdio->dev, "scan phy %s at address %i\n",
 				 child->name, addr);
 
-			phy = get_phy_device(mdio, addr, is_c45);
-			if (!phy || IS_ERR(phy))
+			rc = of_mdiobus_register_phy(mdio, child, addr);
+			if (rc)
 				continue;
-
-			if (mdio->irq) {
-				mdio->irq[addr] =
-					irq_of_parse_and_map(child, 0);
-				if (!mdio->irq[addr])
-					mdio->irq[addr] = PHY_POLL;
-			}
-
-			/* Associate the OF node with the device structure so it
-			 * can be looked up later */
-			of_node_get(child);
-			phy->dev.of_node = child;
-
-			/* All data is now stored in the phy struct;
-			 * register it */
-			rc = phy_device_register(phy);
-			if (rc) {
-				phy_device_free(phy);
-				of_node_put(child);
-				continue;
-			}
-
-			dev_info(&mdio->dev, "registered phy %s at address %i\n",
-				 child->name, addr);
-			break;
 		}
 	}
 
-- 
1.8.3.2

[PATCH 2/7] net: of_mdio: use PHY_MAX_ADDR constant

[Florian Fainelli]

From: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Use the PHY_MAX_ADDR constant for checking if a MDIO bus address is
valid instead of using a plain "32".

Signed-off-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/of/of_mdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 82485d2..f93ebca 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -107,7 +107,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 		}
 
 		addr = be32_to_cpup(paddr);
-		if (addr >= 32) {
+		if (addr >= PHY_MAX_ADDR) {
 			dev_err(&mdio->dev, "%s PHY address %i is too large\n",
 				child->full_name, addr);
 			continue;
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/7] net: of_mdio: do not overwrite PHY interrupt configuration

[Florian Fainelli]

From: Florian Fainelli <f.fainelli@gmail.com>

If irq_of_parse_and_map fails to find an interrupt line for a given PHY,
we will force the PHY interrupt to be PHY_POLL, completely overriding
the previous value that the MDIO bus may have set for us (e.g:
PHY_IGNORE_INTERRUPT). In case of failure, just restore the previous
value.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/of_mdio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index f93ebca..4923ab2 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -27,7 +27,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
 {
 	struct phy_device *phy;
 	bool is_c45;
-	int rc;
+	int rc, prev_irq;
 
 	is_c45 = of_device_is_compatible(child,
 					 "ethernet-phy-ieee802.3-c45");
@@ -37,10 +37,11 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
 		return 1;
 
 	if (mdio->irq) {
+		prev_irq= mdio->irq[addr];
 		mdio->irq[addr] =
 			irq_of_parse_and_map(child, 0);
 		if (!mdio->irq[addr])
-			mdio->irq[addr] = PHY_POLL;
+			mdio->irq[addr] = prev_irq;
 	}
 
 	/* Associate the OF node with the device structure so it
-- 
1.8.3.2

[PATCH 4/7] net: phy: breakdown PHY_*_FEATURES defines

[Florian Fainelli]

From: Florian Fainelli <f.fainelli@gmail.com>

Breakdown the PHY_*_FEATURES into per speed defines such that we can
easily re-use them individually.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/phy.h | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 48a4dc3..7ff751a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -27,18 +27,27 @@
 
 #include <linux/atomic.h>
 
-#define PHY_BASIC_FEATURES	(SUPPORTED_10baseT_Half | \
-				 SUPPORTED_10baseT_Full | \
-				 SUPPORTED_100baseT_Half | \
-				 SUPPORTED_100baseT_Full | \
-				 SUPPORTED_Autoneg | \
+#define PHY_DEFAULT_FEATURES	(SUPPORTED_Autoneg | \
 				 SUPPORTED_TP | \
 				 SUPPORTED_MII)
 
-#define PHY_GBIT_FEATURES	(PHY_BASIC_FEATURES | \
-				 SUPPORTED_1000baseT_Half | \
+#define PHY_10BT_FEATURES	(SUPPORTED_10baseT_Half | \
+				 SUPPORTED_10baseT_Full)
+
+#define PHY_100BT_FEATURES	(SUPPORTED_100baseT_Half | \
+				 SUPPORTED_100baseT_Full)
+
+#define PHY_1000BT_FEATURES	(SUPPORTED_1000baseT_Half | \
 				 SUPPORTED_1000baseT_Full)
 
+#define PHY_BASIC_FEATURES	(PHY_10BT_FEATURES | \
+				 PHY_100BT_FEATURES | \
+				 PHY_DEFAULT_FEATURES)
+
+#define PHY_GBIT_FEATURES	(PHY_BASIC_FEATURES | \
+				 PHY_1000BT_FEATURES)
+
+
 /*
  * Set phydev->irq to PHY_POLL if interrupts are not supported,
  * or not desired for this PHY.  Set to PHY_IGNORE_INTERRUPT if
-- 
1.8.3.2

[PATCH 5/7] net: of_mdio: parse "max-speed" property to set PHY supported features

[Florian Fainelli]

From: Florian Fainelli <f.fainelli@gmail.com>

The "max-speed" property is defined per the ePAPR specification to
express the maximum speed a PHY supports. Use that property, if present
to set the phydev->supported features which properly restricts the PHY
within the range of defined speeds.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/of_mdio.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 4923ab2..e1e19e5 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -22,12 +22,30 @@
 MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
 MODULE_LICENSE("GPL");
 
+static void of_set_phy_supported(struct phy_device *phydev, u32 max_speed)
+{
+	phydev->supported |= PHY_DEFAULT_FEATURES;
+
+	switch (max_speed) {
+	default:
+		return;
+
+	case SPEED_1000:
+		phydev->supported |= PHY_1000BT_FEATURES;
+	case SPEED_100:
+		phydev->supported |= PHY_100BT_FEATURES;
+	case SPEED_10:
+		phydev->supported |= PHY_10BT_FEATURES;
+	}
+}
+
 static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child,
 				   u32 addr)
 {
 	struct phy_device *phy;
 	bool is_c45;
 	int rc, prev_irq;
+	u32 max_speed = 0;
 
 	is_c45 = of_device_is_compatible(child,
 					 "ethernet-phy-ieee802.3-c45");
@@ -58,8 +76,13 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
 		return 1;
 	}
 
+	/* Set phydev->supported based on the "max-speed" property
+	 * if present */
+	if (!of_property_read_u32(child, "max-speed", &max_speed))
+		of_set_phy_supported(phy, max_speed);
+
 	dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
-		 child->name, addr);
+		child->name, addr);
 
 	return 0;
 }
-- 
1.8.3.2

[PATCH 6/7] arc_emac: remove custom "max-speed" parsing code

[Florian Fainelli]

From: Florian Fainelli <f.fainelli@gmail.com>

The ARC emac driver was the only in-tree to parse a PHY device
'max-speed' property but yet failed to do it correctly because
'max-speed' is supposed to set a PHY device supported features, not the
advertising features as it was done.

Now that of_mdiobus_register() takes care of doing that, remove the
custom 'max-speed' parsing code.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/arc/emac.h      |  2 --
 drivers/net/ethernet/arc/emac_main.c | 20 +-------------------
 2 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/arc/emac.h b/drivers/net/ethernet/arc/emac.h
index dc08678..928fac6 100644
--- a/drivers/net/ethernet/arc/emac.h
+++ b/drivers/net/ethernet/arc/emac.h
@@ -122,7 +122,6 @@ struct buffer_state {
  * @link:	PHY's last seen link state.
  * @duplex:	PHY's last set duplex mode.
  * @speed:	PHY's last set speed.
- * @max_speed:	Maximum supported by current system network data-rate.
  */
 struct arc_emac_priv {
 	/* Devices */
@@ -152,7 +151,6 @@ struct arc_emac_priv {
 	unsigned int link;
 	unsigned int duplex;
 	unsigned int speed;
-	unsigned int max_speed;
 };
 
 /**
diff --git a/drivers/net/ethernet/arc/emac_main.c b/drivers/net/ethernet/arc/emac_main.c
index b2ffad1..eedf2a5 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -381,17 +381,7 @@ static int arc_emac_open(struct net_device *ndev)
 	phy_dev->autoneg = AUTONEG_ENABLE;
 	phy_dev->speed = 0;
 	phy_dev->duplex = 0;
-	phy_dev->advertising = phy_dev->supported;
-
-	if (priv->max_speed > 100) {
-		phy_dev->advertising &= PHY_GBIT_FEATURES;
-	} else if (priv->max_speed <= 100) {
-		phy_dev->advertising &= PHY_BASIC_FEATURES;
-		if (priv->max_speed <= 10) {
-			phy_dev->advertising &= ~SUPPORTED_100baseT_Half;
-			phy_dev->advertising &= ~SUPPORTED_100baseT_Full;
-		}
-	}
+	phy_dev->advertising &= phy_dev->supported;
 
 	priv->last_rx_bd = 0;
 
@@ -704,14 +694,6 @@ static int arc_emac_probe(struct platform_device *pdev)
 	/* Set poll rate so that it polls every 1 ms */
 	arc_reg_set(priv, R_POLLRATE, clock_frequency / 1000000);
 
-	/* Get max speed of operation from device tree */
-	if (of_property_read_u32(pdev->dev.of_node, "max-speed",
-				 &priv->max_speed)) {
-		dev_err(&pdev->dev, "failed to retrieve <max-speed> from device tree\n");
-		err = -EINVAL;
-		goto out;
-	}
-
 	ndev->irq = irq;
 	dev_info(&pdev->dev, "IRQ is %d\n", ndev->irq);
 
-- 
1.8.3.2

[PATCH 7/7] Documentation: update Ethernet PHY devices binding with 'max-speed'

[Florian Fainelli]

From: Florian Fainelli <f.fainelli@gmail.com>

The 'max-speed' property is optional but defined in the ePAPR
specification and now supported by the Linux Device Tree parsing
infrastructure.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/devicetree/bindings/net/phy.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index 7cd18fb..f648094 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -22,6 +22,7 @@ Optional Properties:
   specifications. If neither of these are specified, the default is to
   assume clause 22. The compatible list may also contain other
   elements.
+- max-speed: Maximum PHY supported speed (10, 100, 1000...)
 
 Example:
 
-- 
1.8.3.2

Re: [PATCH 5/7] net: of_mdio: parse "max-speed" property to set PHY supported features

[Florian Fainelli]

2013/12/5 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
>>
>> +       /* Set phydev->supported based on the "max-speed" property
>> +        * if present */
>
>
>    Preferred multi-line comment style is this:
>
> /*
>  * bla
>  * bla
>  */

All other comments in this file are:

/* bla
 * bla
 */

which is why I used the same style.
-- 
Florian

Re: [PATCH 5/7] net: of_mdio: parse "max-speed" property to set PHY supported features

[Joe Perches]

On Thu, 2013-12-05 at 15:53 -0800, Florian Fainelli wrote:
> 2013/12/5 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> >>
> >> +       /* Set phydev->supported based on the "max-speed" property
> >> +        * if present */
[]
> All other comments in this file are:
> 
> /* bla
>  * bla
>  */
> 
> which is why I used the same style.

Kind of.  This should be:

	/* Set phydev->supported based on the "max-speed" property
	 * if present
	 */

Or if you want to use all 80 columns:

	/* Set phydev->supported based on the "max-speed" property if present */

or maybe

	/* Set phydev->supported using the "max-speed" property (if present) */

Re: [PATCH 3/7] net: of_mdio: do not overwrite PHY interrupt configuration

[Sergei Shtylyov]

Hello.

On 12/06/2013 01:52 AM, Florian Fainelli wrote:

> From: Florian Fainelli <f.fainelli@gmail.com>

> If irq_of_parse_and_map fails to find an interrupt line for a given PHY,
> we will force the PHY interrupt to be PHY_POLL, completely overriding
> the previous value that the MDIO bus may have set for us (e.g:
> PHY_IGNORE_INTERRUPT). In case of failure, just restore the previous
> value.

> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/of/of_mdio.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index f93ebca..4923ab2 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
[...]
> @@ -37,10 +37,11 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>   		return 1;
>
>   	if (mdio->irq) {
> +		prev_irq= mdio->irq[addr];

    You forgot space before =.

WBR, Sergei

Re: [PATCH 5/7] net: of_mdio: parse "max-speed" property to set PHY supported features

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu, 5 Dec 2013 15:53:41 -0800

> 2013/12/5 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
>>>
>>> +       /* Set phydev->supported based on the "max-speed" property
>>> +        * if present */
>>
>>
>>    Preferred multi-line comment style is this:
>>
>> /*
>>  * bla
>>  * bla
>>  */
> 
> All other comments in this file are:
> 
> /* bla
>  * bla
>  */
> 
> which is why I used the same style.

And this is the preferred format for networking, as documented in
Documentation/networking/netdev-FAQ.txt

Re: [PATCH 5/7] net: of_mdio: parse "max-speed" property to set PHY supported features

[Sergei Shtylyov]

Hello.

On 12/06/2013 01:52 AM, Florian Fainelli wrote:

> From: Florian Fainelli <f.fainelli@gmail.com>

> The "max-speed" property is defined per the ePAPR specification to
> express the maximum speed a PHY supports. Use that property, if present
> to set the phydev->supported features which properly restricts the PHY
> within the range of defined speeds.

> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/of/of_mdio.c | 25 ++++++++++++++++++++++++-
>   1 file changed, 24 insertions(+), 1 deletion(-)

> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 4923ab2..e1e19e5 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
[...]
> @@ -58,8 +76,13 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>   		return 1;
>   	}
>
> +	/* Set phydev->supported based on the "max-speed" property
> +	 * if present */

    Preferred multi-line comment style is this:

/*
  * bla
  * bla
  */

WBR, Sergei

Re: [PATCH 0/7] net: of_mdio improvements

[David Miller]

From: Florian Fainelli <florian@openwrt.org>
Date: Thu, 5 Dec 2013 14:52:09 -0800

> This patchset contains a few improvements to the MDIO device tree parsing
> code such as refactoring and parsing the "max-speed" property which is
> defined in the ePAPR specification.

Series applied, thanks.  I fixed up the missing space in the assignment.

Re: [PATCH 5/7] net: of_mdio: parse "max-speed" property to set PHY supported features

[Rob Herring]

On 12/05/2013 04:52 PM, Florian Fainelli wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> 
> The "max-speed" property is defined per the ePAPR specification to
> express the maximum speed a PHY supports. Use that property, if present
> to set the phydev->supported features which properly restricts the PHY
> within the range of defined speeds.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/of/of_mdio.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 4923ab2..e1e19e5 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -22,12 +22,30 @@
>  MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>  MODULE_LICENSE("GPL");
>  
> +static void of_set_phy_supported(struct phy_device *phydev, u32 max_speed)
> +{
> +	phydev->supported |= PHY_DEFAULT_FEATURES;
> +
> +	switch (max_speed) {
> +	default:
> +		return;
> +
> +	case SPEED_1000:
> +		phydev->supported |= PHY_1000BT_FEATURES;

This assumes the speed is not already set. Do you need to first mask
speeds out?

No need to support 10G PHYs?

A fall-thru note would be helpful here.

> +	case SPEED_100:
> +		phydev->supported |= PHY_100BT_FEATURES;
> +	case SPEED_10:
> +		phydev->supported |= PHY_10BT_FEATURES;
> +	}
> +}
> +
>  static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child,
>  				   u32 addr)
>  {
>  	struct phy_device *phy;
>  	bool is_c45;
>  	int rc, prev_irq;
> +	u32 max_speed = 0;
>  
>  	is_c45 = of_device_is_compatible(child,
>  					 "ethernet-phy-ieee802.3-c45");
> @@ -58,8 +76,13 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>  		return 1;
>  	}
>  
> +	/* Set phydev->supported based on the "max-speed" property
> +	 * if present */
> +	if (!of_property_read_u32(child, "max-speed", &max_speed))
> +		of_set_phy_supported(phy, max_speed);
> +
>  	dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> -		 child->name, addr);
> +		child->name, addr);
>  
>  	return 0;
>  }
> 

Re: [PATCH 1/7] net: of_mdio: factor PHY registration from of_mdiobus_register

[Rob Herring]

On 12/05/2013 04:52 PM, Florian Fainelli wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> 
> Since commit 779d835e ("net: of_mdio: scan mdiobus for PHYs without reg
> property") we have two foreach loops which do pretty much the same
> thing. Factor the PHY device registration in a function helper:
> of_mdiobus_register_phy() which takes care of the details and allows for
> future PHY specific extensions.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

A couple of formatting nits, but otherwise:

Acked-by: Rob Herring <rob.herring@calxeda.com>

BTW, unless David has objections, I'd like to see this file moved to
drivers/net. He tends to merge the of_mdio.c changes anyway. We've moved
most other subsystem parsing code out of drivers/of.

> ---
>  drivers/of/of_mdio.c | 109 ++++++++++++++++++++++-----------------------------
>  1 file changed, 46 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index d5a57a9..82485d2 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -22,6 +22,47 @@
>  MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>  MODULE_LICENSE("GPL");
>  
> +static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child,
> +				   u32 addr)
> +{
> +	struct phy_device *phy;
> +	bool is_c45;
> +	int rc;
> +
> +	is_c45 = of_device_is_compatible(child,
> +					 "ethernet-phy-ieee802.3-c45");

This looks like it can be 1 line.

> +
> +	phy = get_phy_device(mdio, addr, is_c45);
> +	if (!phy || IS_ERR(phy))
> +		return 1;
> +
> +	if (mdio->irq) {
> +		mdio->irq[addr] =
> +			irq_of_parse_and_map(child, 0);

ditto

> +		if (!mdio->irq[addr])
> +			mdio->irq[addr] = PHY_POLL;
> +	}
> +
> +	/* Associate the OF node with the device structure so it
> +	 * can be looked up later */
> +	of_node_get(child);
> +	phy->dev.of_node = child;
> +
> +	/* All data is now stored in the phy struct;
> +	 * register it */
> +	rc = phy_device_register(phy);
> +	if (rc) {
> +		phy_device_free(phy);
> +		of_node_put(child);
> +		return 1;
> +	}
> +
> +	dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> +		 child->name, addr);
> +
> +	return 0;
> +}
> +
>  /**
>   * of_mdiobus_register - Register mii_bus and create PHYs from the device tree
>   * @mdio: pointer to mii_bus structure
> @@ -32,11 +73,10 @@ MODULE_LICENSE("GPL");
>   */
>  int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  {
> -	struct phy_device *phy;
>  	struct device_node *child;
>  	const __be32 *paddr;
>  	u32 addr;
> -	bool is_c45, scanphys = false;
> +	bool scanphys = false;
>  	int rc, i, len;
>  
>  	/* Mask out all PHYs from auto probing.  Instead the PHYs listed in
> @@ -73,38 +113,9 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  			continue;
>  		}
>  
> -		if (mdio->irq) {
> -			mdio->irq[addr] = irq_of_parse_and_map(child, 0);
> -			if (!mdio->irq[addr])
> -				mdio->irq[addr] = PHY_POLL;
> -		}
> -
> -		is_c45 = of_device_is_compatible(child,
> -						 "ethernet-phy-ieee802.3-c45");
> -		phy = get_phy_device(mdio, addr, is_c45);
> -
> -		if (!phy || IS_ERR(phy)) {
> -			dev_err(&mdio->dev,
> -				"cannot get PHY at address %i\n",
> -				addr);
> -			continue;
> -		}
> -
> -		/* Associate the OF node with the device structure so it
> -		 * can be looked up later */
> -		of_node_get(child);
> -		phy->dev.of_node = child;
> -
> -		/* All data is now stored in the phy struct; register it */
> -		rc = phy_device_register(phy);
> -		if (rc) {
> -			phy_device_free(phy);
> -			of_node_put(child);
> +		rc = of_mdiobus_register_phy(mdio, child, addr);
> +		if (rc)
>  			continue;
> -		}
> -
> -		dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> -			child->name, addr);
>  	}
>  
>  	if (!scanphys)
> @@ -117,9 +128,6 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  		if (paddr)
>  			continue;
>  
> -		is_c45 = of_device_is_compatible(child,
> -						 "ethernet-phy-ieee802.3-c45");
> -
>  		for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
>  			/* skip already registered PHYs */
>  			if (mdio->phy_map[addr])
> @@ -129,34 +137,9 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  			dev_info(&mdio->dev, "scan phy %s at address %i\n",
>  				 child->name, addr);
>  
> -			phy = get_phy_device(mdio, addr, is_c45);
> -			if (!phy || IS_ERR(phy))
> +			rc = of_mdiobus_register_phy(mdio, child, addr);
> +			if (rc)
>  				continue;
> -
> -			if (mdio->irq) {
> -				mdio->irq[addr] =
> -					irq_of_parse_and_map(child, 0);
> -				if (!mdio->irq[addr])
> -					mdio->irq[addr] = PHY_POLL;
> -			}
> -
> -			/* Associate the OF node with the device structure so it
> -			 * can be looked up later */
> -			of_node_get(child);
> -			phy->dev.of_node = child;
> -
> -			/* All data is now stored in the phy struct;
> -			 * register it */
> -			rc = phy_device_register(phy);
> -			if (rc) {
> -				phy_device_free(phy);
> -				of_node_put(child);
> -				continue;
> -			}
> -
> -			dev_info(&mdio->dev, "registered phy %s at address %i\n",
> -				 child->name, addr);
> -			break;
>  		}
>  	}
>  
> 

Re: [PATCH 2/7] net: of_mdio: use PHY_MAX_ADDR constant

[Grant Likely]

On Thu, 5 Dec 2013 14:52:11 -0800, Florian Fainelli <florian@openwrt.org> wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> 
> Use the PHY_MAX_ADDR constant for checking if a MDIO bus address is
> valid instead of using a plain "32".
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  drivers/of/of_mdio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 82485d2..f93ebca 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -107,7 +107,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  		}
>  
>  		addr = be32_to_cpup(paddr);
> -		if (addr >= 32) {
> +		if (addr >= PHY_MAX_ADDR) {
>  			dev_err(&mdio->dev, "%s PHY address %i is too large\n",
>  				child->full_name, addr);
>  			continue;
> -- 
> 1.8.3.2
> 

Re: [PATCH 3/7] net: of_mdio: do not overwrite PHY interrupt configuration

[Grant Likely]

On Thu, 5 Dec 2013 14:52:12 -0800, Florian Fainelli <florian@openwrt.org> wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> 
> If irq_of_parse_and_map fails to find an interrupt line for a given PHY,
> we will force the PHY interrupt to be PHY_POLL, completely overriding
> the previous value that the MDIO bus may have set for us (e.g:
> PHY_IGNORE_INTERRUPT). In case of failure, just restore the previous
> value.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/of/of_mdio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index f93ebca..4923ab2 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -27,7 +27,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>  {
>  	struct phy_device *phy;
>  	bool is_c45;
> -	int rc;
> +	int rc, prev_irq;
>  
>  	is_c45 = of_device_is_compatible(child,
>  					 "ethernet-phy-ieee802.3-c45");
> @@ -37,10 +37,11 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>  		return 1;
>  
>  	if (mdio->irq) {
> +		prev_irq= mdio->irq[addr];
>  		mdio->irq[addr] =
>  			irq_of_parse_and_map(child, 0);
>  		if (!mdio->irq[addr])
> -			mdio->irq[addr] = PHY_POLL;
> +			mdio->irq[addr] = prev_irq;
>  	}

Nit, the logic is a little weird the way it is written after the change.
I'd rather see:

	if (mdio->irq) {
		irq = irq_of_parse_and_map(child, 0);
		if (irq)
			mdio->irq[addr] = irq;
	}

Or even more concise:

	if (mdio->irq && irq = irq_of_parse_and_map(child, 0))
		mdio->irq[addr] = irq;

g.

Regardless,

Acked-by: Grant Likely <grant.likely@linaro.org>

g.

Re: [PATCH 5/7] net: of_mdio: parse "max-speed" property to set PHY supported features

[Grant Likely]

On Thu, 5 Dec 2013 14:52:14 -0800, Florian Fainelli <florian@openwrt.org> wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> 
> The "max-speed" property is defined per the ePAPR specification to
> express the maximum speed a PHY supports. Use that property, if present
> to set the phydev->supported features which properly restricts the PHY
> within the range of defined speeds.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/of/of_mdio.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 4923ab2..e1e19e5 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -22,12 +22,30 @@
>  MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>  MODULE_LICENSE("GPL");
>  
> +static void of_set_phy_supported(struct phy_device *phydev, u32 max_speed)
> +{
> +	phydev->supported |= PHY_DEFAULT_FEATURES;
> +
> +	switch (max_speed) {
> +	default:
> +		return;

No need for a default clause.

> +
> +	case SPEED_1000:
> +		phydev->supported |= PHY_1000BT_FEATURES;
> +	case SPEED_100:
> +		phydev->supported |= PHY_100BT_FEATURES;
> +	case SPEED_10:
> +		phydev->supported |= PHY_10BT_FEATURES;
> +	}
> +}
> +
>  static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child,
>  				   u32 addr)
>  {
>  	struct phy_device *phy;
>  	bool is_c45;
>  	int rc, prev_irq;
> +	u32 max_speed = 0;
>  
>  	is_c45 = of_device_is_compatible(child,
>  					 "ethernet-phy-ieee802.3-c45");
> @@ -58,8 +76,13 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>  		return 1;
>  	}
>  
> +	/* Set phydev->supported based on the "max-speed" property
> +	 * if present */
> +	if (!of_property_read_u32(child, "max-speed", &max_speed))
> +		of_set_phy_supported(phy, max_speed);
> +

Why the separate function? There is no need since there is only one
caller and the block is very short. Just do this:

	if (!of_property_read_u32(child, "max-speed", &max_speed)) {
		phydev->supported |= PHY_DEFAULT_FEATURES;
		switch (max_speed) {
		case SPEED_1000:
			phydev->supported |= PHY_1000BT_FEATURES;
		case SPEED_100:
			phydev->supported |= PHY_100BT_FEATURES;
		case SPEED_10:
			phydev->supported |= PHY_10BT_FEATURES;
		}
	}

>  	dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> -		 child->name, addr);
> +		child->name, addr);

Unrelated whitespace change. I wouldn't even bother with changing this.
It adds noise.

However, those are style concerns. The content looks fine.

Acked-by: Grant Likely <grant.likely@linaro.org>

g.