[PATCH net 0/2] net: macb: fix probing of PHY not described in the dt

[PATCH net 0/2] net: macb: fix probing of PHY not described in the dt

[Antoine Tenart]

Hello,

The macb Ethernet driver supports various ways of referencing its
network PHY. When a device tree is used the PHY can be referenced with
a phy-handle or, if connected to its internal MDIO bus, described in
a child node. Some platforms omitted the PHY description while
connecting the PHY to the internal MDIO bus and in such cases the MDIO
bus has to be scanned "manually" by the macb driver.

Prior to the phylink conversion the driver registered the MDIO bus with
of_mdiobus_register and then in case the PHY couldn't be retrieved
using dt or using phy_find_first (because registering an MDIO bus with
of_mdiobus_register masks all PHYs) the macb driver was "manually"
scanning the MDIO bus (like mdiobus_register does). The phylink
conversion did break this particular case but reimplementing the manual
scan of the bus in the macb driver wouldn't be very clean. The solution
seems to be registering the MDIO bus based on if the PHYs are described
in the device tree or not.

There are multiple ways to do this, none is perfect. I chose to check if
any of the child nodes of the macb node was a network PHY and based on
this to register the MDIO bus with the of_ helper or not. The drawback
is boards referencing the PHY through phy-handle, would scan the entire
MDIO bus of the macb at boot time (as the MDIO bus would be registered
with mdiobus_register). For this solution to work properly
of_mdiobus_child_is_phy has to be exported, which means the patch doing
so has to be backported to -stable as well.

Another possible solution could have been to simply check if the macb
node has a child node by counting its sub-nodes. This isn't techically
perfect, as there could be other sub-nodes (in practice this should be
fine, fixed-link being taken care of in the driver). We could also
simply s/of_mdiobus_register/mdiobus_register/ but that could break
boards using the PHY description in child node as a selector (which
really would be not a proper way to do this...).

The real issue here being having PHYs not described in the dt but we
have dt backward compatibility, so we have to live with that.

Thanks,
Antoine

Antoine Tenart (2):
  of: mdio: export of_mdiobus_child_is_phy
  net: macb: fix probing of PHY not described in the dt

 drivers/net/ethernet/cadence/macb_main.c | 27 ++++++++++++++++++++----
 drivers/of/of_mdio.c                     |  3 ++-
 include/linux/of_mdio.h                  |  6 ++++++
 3 files changed, 31 insertions(+), 5 deletions(-)

-- 
2.23.0

[PATCH net 1/2] of: mdio: export of_mdiobus_child_is_phy

[Antoine Tenart]

This patch exports of_mdiobus_child_is_phy, allowing to check if a child
node is a network PHY.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/of/of_mdio.c    | 3 ++-
 include/linux/of_mdio.h | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index c6b87ce2b0cc..fc757ef6eadc 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -162,7 +162,7 @@ static const struct of_device_id whitelist_phys[] = {
  * A device which is not a phy is expected to have a compatible string
  * indicating what sort of device it is.
  */
-static bool of_mdiobus_child_is_phy(struct device_node *child)
+bool of_mdiobus_child_is_phy(struct device_node *child)
 {
 	u32 phy_id;
 
@@ -187,6 +187,7 @@ static bool of_mdiobus_child_is_phy(struct device_node *child)
 
 	return false;
 }
+EXPORT_SYMBOL(of_mdiobus_child_is_phy);
 
 /**
  * of_mdiobus_register - Register mii_bus and create PHYs from the device tree
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 99cefe6f5edb..79bc82e30c02 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -12,6 +12,7 @@
 #include <linux/of.h>
 
 #if IS_ENABLED(CONFIG_OF_MDIO)
+extern bool of_mdiobus_child_is_phy(struct device_node *child);
 extern int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np);
 extern struct phy_device *of_phy_find_device(struct device_node *phy_np);
 extern struct phy_device *of_phy_connect(struct net_device *dev,
@@ -54,6 +55,11 @@ static inline int of_mdio_parse_addr(struct device *dev,
 }
 
 #else /* CONFIG_OF_MDIO */
+static bool of_mdiobus_child_is_phy(struct device_node *child)
+{
+	return false;
+}
+
 static inline int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 {
 	/*
-- 
2.23.0

[PATCH net 2/2] net: macb: fix probing of PHY not described in the dt

[Antoine Tenart]

This patch fixes the case where the PHY isn't described in the device
tree. This is due to the way the MDIO bus is registered in the driver:
whether the PHY is described in the device tree or not, the bus is
registered through of_mdiobus_register. The function masks all the PHYs
and only allow probing the ones described in the device tree. Prior to
the Phylink conversion this was also done but later on in the driver
the MDIO bus was manually scanned to circumvent the fact that the PHY
wasn't described.

This patch fixes it in a proper way, by registering the MDIO bus based
on if the PHY attached to a given interface is described in the device
tree or not.

Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 27 ++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 9c767ee252ac..c5ee363ca5dc 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -664,9 +664,30 @@ static int macb_mii_probe(struct net_device *dev)
 	return 0;
 }
 
+static int macb_mdiobus_register(struct macb *bp)
+{
+	struct device_node *child, *np = bp->pdev->dev.of_node;
+
+	/* Only create the PHY from the device tree if at least one PHY is
+	 * described. Otherwise scan the entire MDIO bus. We do this to support
+	 * old device tree that did not follow the best practices and did not
+	 * describe their network PHYs.
+	 */
+	for_each_available_child_of_node(np, child)
+		if (of_mdiobus_child_is_phy(child)) {
+			/* The loop increments the child refcount,
+			 * decrement it before returning.
+			 */
+			of_node_put(child);
+
+			return of_mdiobus_register(bp->mii_bus, np);
+		}
+
+	return mdiobus_register(bp->mii_bus);
+}
+
 static int macb_mii_init(struct macb *bp)
 {
-	struct device_node *np;
 	int err = -ENXIO;
 
 	/* Enable management port */
@@ -688,9 +709,7 @@ static int macb_mii_init(struct macb *bp)
 
 	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
 
-	np = bp->pdev->dev.of_node;
-
-	err = of_mdiobus_register(bp->mii_bus, np);
+	err = macb_mdiobus_register(bp);
 	if (err)
 		goto err_out_free_mdiobus;
 
-- 
2.23.0

Re: [PATCH net 0/2] net: macb: fix probing of PHY not described in the dt

[David Miller]

From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Tue, 17 Dec 2019 18:07:40 +0100

> The macb Ethernet driver supports various ways of referencing its
> network PHY. When a device tree is used the PHY can be referenced with
> a phy-handle or, if connected to its internal MDIO bus, described in
> a child node. Some platforms omitted the PHY description while
> connecting the PHY to the internal MDIO bus and in such cases the MDIO
> bus has to be scanned "manually" by the macb driver.
> 
> Prior to the phylink conversion the driver registered the MDIO bus with
> of_mdiobus_register and then in case the PHY couldn't be retrieved
> using dt or using phy_find_first (because registering an MDIO bus with
> of_mdiobus_register masks all PHYs) the macb driver was "manually"
> scanning the MDIO bus (like mdiobus_register does). The phylink
> conversion did break this particular case but reimplementing the manual
> scan of the bus in the macb driver wouldn't be very clean. The solution
> seems to be registering the MDIO bus based on if the PHYs are described
> in the device tree or not.
> 
> There are multiple ways to do this, none is perfect. I chose to check if
> any of the child nodes of the macb node was a network PHY and based on
> this to register the MDIO bus with the of_ helper or not. The drawback
> is boards referencing the PHY through phy-handle, would scan the entire
> MDIO bus of the macb at boot time (as the MDIO bus would be registered
> with mdiobus_register). For this solution to work properly
> of_mdiobus_child_is_phy has to be exported, which means the patch doing
> so has to be backported to -stable as well.
> 
> Another possible solution could have been to simply check if the macb
> node has a child node by counting its sub-nodes. This isn't techically
> perfect, as there could be other sub-nodes (in practice this should be
> fine, fixed-link being taken care of in the driver). We could also
> simply s/of_mdiobus_register/mdiobus_register/ but that could break
> boards using the PHY description in child node as a selector (which
> really would be not a proper way to do this...).
> 
> The real issue here being having PHYs not described in the dt but we
> have dt backward compatibility, so we have to live with that.

Series applied, thanks Antoine.