[PATCH 1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[PATCH 1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Ahmad Fatoum]

The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
There, of_mdiobus_register was called even for the fixed-link representing
the SPI-connected switch PHY, with the result that the driver attempts to
enumerate PHYs on a non-existent MDIO bus:

	libphy: MACB_mii_bus: probed
	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
        [snip]
	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
	macb f0028000.ethernet: broken fixed-link specification

Cc: <stable@vger.kernel.org>
Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 27 +++++++++++++++---------
 1 file changed, 17 insertions(+), 10 deletions(-)

Fixes since v1:
	Added one more error path for failing to register fixed-link
	Fixed a whitespace issue


diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index dc09f9a8a49b..ef6ce8691443 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -482,11 +482,6 @@ static int macb_mii_probe(struct net_device *dev)
 
 	if (np) {
 		if (of_phy_is_fixed_link(np)) {
-			if (of_phy_register_fixed_link(np) < 0) {
-				dev_err(&bp->pdev->dev,
-					"broken fixed-link specification\n");
-				return -ENODEV;
-			}
 			bp->phy_node = of_node_get(np);
 		} else {
 			bp->phy_node = of_parse_phandle(np, "phy-handle", 0);
@@ -569,7 +564,7 @@ static int macb_mii_init(struct macb *bp)
 {
 	struct macb_platform_data *pdata;
 	struct device_node *np;
-	int err;
+	int err = -ENXIO;
 
 	/* Enable management port */
 	macb_writel(bp, NCR, MACB_BIT(MPE));
@@ -592,12 +587,23 @@ static int macb_mii_init(struct macb *bp)
 	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
 
 	np = bp->pdev->dev.of_node;
-	if (pdata)
-		bp->mii_bus->phy_mask = pdata->phy_mask;
+	if (np && of_phy_is_fixed_link(np)) {
+		if (of_phy_register_fixed_link(np) < 0) {
+			dev_err(&bp->pdev->dev,
+				"broken fixed-link specification\n");
+			goto err_out_free_mdiobus;
+		}
+
+		err = mdiobus_register(bp->mii_bus);
+	} else {
+		if (pdata)
+			bp->mii_bus->phy_mask = pdata->phy_mask;
+
+		err = of_mdiobus_register(bp->mii_bus, np);
+	}
 
-	err = of_mdiobus_register(bp->mii_bus, np);
 	if (err)
-		goto err_out_free_mdiobus;
+		goto err_out_free_fixed_link;
 
 	err = macb_mii_probe(bp->dev);
 	if (err)
@@ -607,6 +613,7 @@ static int macb_mii_init(struct macb *bp)
 
 err_out_unregister_bus:
 	mdiobus_unregister(bp->mii_bus);
+err_out_free_fixed_link:
 	if (np && of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
 err_out_free_mdiobus:
-- 
2.18.0

[PATCH 2/4] of: phy: Warn about unexpected fixed-links in of_mdiobus_register

[Ahmad Fatoum]

fixed-links are currently not handled by of_mdiobus_register,
skip them with a warning instead of trying pointlessly to find their PHY
address:

	libphy: MACB_mii_bus: probed
	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
	[snip]
	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
	macb f0028000.ethernet: broken fixed-link specification

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/of/of_mdio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index e92391d6d1bd..9a7ccd299daf 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -229,6 +229,13 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 
 	/* Loop over the child nodes and register a phy_device for each phy */
 	for_each_available_child_of_node(np, child) {
+		if (of_phy_is_fixed_link(np)) {
+			/* fixed-links are handled in the MAC drivers */
+			dev_warn(&mdio->dev, FW_BUG
+				"Skipping unexpected fixed-link in device tree");
+			continue;
+		}
+
 		addr = of_mdio_parse_addr(&mdio->dev, child);
 		if (addr < 0) {
 			scanphys = true;
-- 
2.18.0

Re: [PATCH 2/4] of: phy: Warn about unexpected fixed-links in of_mdiobus_register

[Ahmad Fatoum]

On 08/20/2018 02:12 PM, Ahmad Fatoum wrote:
>  	/* Loop over the child nodes and register a phy_device for each phy */
>  	for_each_available_child_of_node(np, child) {
> +		if (of_phy_is_fixed_link(np)) {
> +			/* fixed-links are handled in the MAC drivers */
> +			dev_warn(&mdio->dev, FW_BUG
> +				"Skipping unexpected fixed-link in device tree");
> +			continue;
> +		}
> +

This would emit the warning even for normal PHYs,
because of_phy_is_fixed_link looks up a child...

I will correct this in v3 along with any other potential suggestions.

Re: [PATCH 2/4] of: phy: Warn about unexpected fixed-links in of_mdiobus_register

[Andrew Lunn]

On Mon, Aug 20, 2018 at 02:12:36PM +0200, Ahmad Fatoum wrote:
> fixed-links are currently not handled by of_mdiobus_register,
> skip them with a warning instead of trying pointlessly to find their PHY
> address:
> 
> 	libphy: MACB_mii_bus: probed
> 	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
> 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
> 	[snip]
> 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
> 	macb f0028000.ethernet: broken fixed-link specification
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/of/of_mdio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index e92391d6d1bd..9a7ccd299daf 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -229,6 +229,13 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  
>  	/* Loop over the child nodes and register a phy_device for each phy */
>  	for_each_available_child_of_node(np, child) {
> +		if (of_phy_is_fixed_link(np)) {
> +			/* fixed-links are handled in the MAC drivers */
> +			dev_warn(&mdio->dev, FW_BUG
> +				"Skipping unexpected fixed-link in device tree");

Hi Ahmad

We should be more specific than "device tree". It is actually the "mdio bus subtree".

Is this patch on its own sufficient to fix your regression? Ideally we
want one small patch which we can add to stable to fix the regression,
and then all the new functionality goes into net-next.

    Andrew

Re: [PATCH 2/4] of: phy: Warn about unexpected fixed-links in of_mdiobus_register

[Ahmad Fatoum]

On 08/20/2018 03:37 PM, Andrew Lunn wrote:
> We should be more specific than "device tree". It is actually the "mdio bus subtree".

I wasn't sure if there are more drivers that call of_mdiobus_register for the MAC node.
I'll update the message.

> Is this patch on its own sufficient to fix your regression? Ideally we
> want one small patch which we can add to stable to fix the regression,
> and then all the new functionality goes into net-next.

This patch (or at least the fixed version in v3) only replaces the MDIO bus scan for
fixed-links with a warning. Patch 1/4 (the one Cc:ing stable) suffices to fix the regression.

[PATCH 3/4] net: macb: Support specifying PHYs in a mdio container dts node

[Ahmad Fatoum]

To align macb DT entries with those of other MACs.
For backwards compatibility, the old way remains supported.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ef6ce8691443..2ebc5698db9d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -596,10 +596,10 @@ static int macb_mii_init(struct macb *bp)
 
 		err = mdiobus_register(bp->mii_bus);
 	} else {
+		struct device_node *node = of_get_child_by_name(np, "mdio") ?: np;
 		if (pdata)
 			bp->mii_bus->phy_mask = pdata->phy_mask;
-
-		err = of_mdiobus_register(bp->mii_bus, np);
+		err = of_mdiobus_register(bp->mii_bus, node);
 	}
 
 	if (err)
-- 
2.18.0

Re: [PATCH 3/4] net: macb: Support specifying PHYs in a mdio container dts node

[Andrew Lunn]

On Mon, Aug 20, 2018 at 02:12:37PM +0200, Ahmad Fatoum wrote:
> To align macb DT entries with those of other MACs.
> For backwards compatibility, the old way remains supported.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ef6ce8691443..2ebc5698db9d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -596,10 +596,10 @@ static int macb_mii_init(struct macb *bp)
>  
>  		err = mdiobus_register(bp->mii_bus);
>  	} else {
> +		struct device_node *node = of_get_child_by_name(np, "mdio") ?: np;

This is correct. But i would prefer the more readable

		struct device_node *node = of_get_child_by_name(np, "mdio");

		if (!node)
   			/* Allow for the deprecated PHYs in the MAC node. */
   			node = np;

>  		if (pdata)
>  			bp->mii_bus->phy_mask = pdata->phy_mask;
> -
> -		err = of_mdiobus_register(bp->mii_bus, np);
> +		err = of_mdiobus_register(bp->mii_bus, node);
>  	}

Also, the device tree binding documentation needs updating.

Thanks
	Andrew

Re: [PATCH 3/4] net: macb: Support specifying PHYs in a mdio container dts node

[Ahmad Fatoum]

On 08/20/2018 03:42 PM, Andrew Lunn wrote:
> On Mon, Aug 20, 2018 at 02:12:37PM +0200, Ahmad Fatoum wrote:
> This is correct. But i would prefer the more readable
> 
> 		struct device_node *node = of_get_child_by_name(np, "mdio");
> 
> 		if (!node)
>    			/* Allow for the deprecated PHYs in the MAC node. */
>    			node = np;
> 
>>  		if (pdata)
>>  			bp->mii_bus->phy_mask = pdata->phy_mask;
>> -
>> -		err = of_mdiobus_register(bp->mii_bus, np);
>> +		err = of_mdiobus_register(bp->mii_bus, node);
>>  	}

Ok.

> Also, the device tree binding documentation needs updating.

I've done so in part 4/4.

Re: [PATCH 3/4] net: macb: Support specifying PHYs in a mdio container dts node

[Andrew Lunn]

> I've done so in part 4/4.

Yes, i noticed that later. Ideally, it would be here, since this is
the patch which actually changes the binding.

    Andrew

[PATCH 4/4] ARM: dts: macb: wrap macb PHYs in a mdio container

[Ahmad Fatoum]

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 .../devicetree/bindings/net/macb.txt          | 13 +++--
 arch/arm/boot/dts/at91-sam9_l9260.dts         |  6 ++-
 arch/arm/boot/dts/at91-sama5d27_som1.dtsi     | 14 ++---
 arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts     | 10 ++--
 arch/arm/boot/dts/at91-sama5d2_xplained.dts   | 10 ++--
 arch/arm/boot/dts/at91-sama5d3_xplained.dts   | 12 +++--
 arch/arm/boot/dts/at91-sama5d4_ma5d4evk.dts   |  6 ++-
 arch/arm/boot/dts/at91-sama5d4_xplained.dts   | 10 ++--
 arch/arm/boot/dts/at91-sama5d4ek.dts          | 10 ++--
 arch/arm/boot/dts/at91-vinco.dts              | 24 +++++----
 arch/arm/boot/dts/at91rm9200ek.dts            |  8 +--
 arch/arm/boot/dts/sama5d2.dtsi                |  5 ++
 arch/arm/boot/dts/sama5d3_emac.dtsi           |  5 ++
 arch/arm/boot/dts/sama5d3_gmac.dtsi           |  5 ++
 arch/arm/boot/dts/sama5d3xcm_cmp.dtsi         | 52 ++++++++++---------
 arch/arm/boot/dts/sama5d3xmb_gmac.dtsi        | 52 ++++++++++---------
 arch/arm/boot/dts/sama5d4.dtsi                | 10 ++++
 17 files changed, 155 insertions(+), 97 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
index 457d5ae16f23..f39732372538 100644
--- a/Documentation/devicetree/bindings/net/macb.txt
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -25,7 +25,8 @@ Required properties:
 	Optional elements: 'tsu_clk'
 - clocks: Phandles to input clocks.
 
-Optional properties for PHY child node:
+PHY child nodes should be grouped in a mdio container,
+they have following optional properties:
 - reset-gpios : Should specify the gpio for phy reset
 - magic-packet : If present, indicates that the hardware supports waking
   up via magic packet.
@@ -41,8 +42,12 @@ Examples:
 		local-mac-address = [3a 0e 03 04 05 06];
 		clock-names = "pclk", "hclk", "tx_clk";
 		clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
-		ethernet-phy@1 {
-			reg = <0x1>;
-			reset-gpios = <&pioE 6 1>;
+		mdio {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			ethernet-phy@1 {
+				reg = <0x1>;
+				reset-gpios = <&pioE 6 1>;
+			};
 		};
 	};
diff --git a/arch/arm/boot/dts/at91-sam9_l9260.dts b/arch/arm/boot/dts/at91-sam9_l9260.dts
index 70cb36f7a9d7..9d1fbd3afaea 100644
--- a/arch/arm/boot/dts/at91-sam9_l9260.dts
+++ b/arch/arm/boot/dts/at91-sam9_l9260.dts
@@ -67,8 +67,10 @@
 				#size-cells = <0>;
 				status = "okay";
 
-				ethernet-phy@1 {
-					reg = <0x1>;
+				mdio {
+					ethernet-phy@1 {
+						reg = <0x1>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/at91-sama5d27_som1.dtsi b/arch/arm/boot/dts/at91-sama5d27_som1.dtsi
index cf0087b4c9e1..f729f128e68e 100644
--- a/arch/arm/boot/dts/at91-sama5d27_som1.dtsi
+++ b/arch/arm/boot/dts/at91-sama5d27_som1.dtsi
@@ -67,12 +67,14 @@
 				pinctrl-0 = <&pinctrl_macb0_default>;
 				phy-mode = "rmii";
 
-				ethernet-phy@0 {
-					reg = <0x0>;
-					interrupt-parent = <&pioA>;
-					interrupts = <PIN_PD31 IRQ_TYPE_LEVEL_LOW>;
-					pinctrl-names = "default";
-					pinctrl-0 = <&pinctrl_macb0_phy_irq>;
+				mdio {
+					ethernet-phy@0 {
+						reg = <0x0>;
+						interrupt-parent = <&pioA>;
+						interrupts = <PIN_PD31 IRQ_TYPE_LEVEL_LOW>;
+						pinctrl-names = "default";
+						pinctrl-0 = <&pinctrl_macb0_phy_irq>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts b/arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts
index b10dccd0958f..1ba4bad8189b 100644
--- a/arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts
@@ -142,10 +142,12 @@
 				phy-mode = "rmii";
 				status = "okay";
 
-				ethernet-phy@1 {
-					reg = <0x1>;
-					interrupt-parent = <&pioA>;
-					interrupts = <56 IRQ_TYPE_LEVEL_LOW>;
+				mdio {
+					ethernet-phy@1 {
+						reg = <0x1>;
+						interrupt-parent = <&pioA>;
+						interrupts = <56 IRQ_TYPE_LEVEL_LOW>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index fcc85d70f36e..83b435744ffd 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -126,10 +126,12 @@
 				phy-mode = "rmii";
 				status = "okay";
 
-				ethernet-phy@1 {
-					reg = <0x1>;
-					interrupt-parent = <&pioA>;
-					interrupts = <PIN_PC9 IRQ_TYPE_LEVEL_LOW>;
+				mdio {
+					ethernet-phy@1 {
+						reg = <0x1>;
+						interrupt-parent = <&pioA>;
+						interrupts = <PIN_PC9 IRQ_TYPE_LEVEL_LOW>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/at91-sama5d3_xplained.dts b/arch/arm/boot/dts/at91-sama5d3_xplained.dts
index 02c1d2958d78..a84fec83f0a5 100644
--- a/arch/arm/boot/dts/at91-sama5d3_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d3_xplained.dts
@@ -134,8 +134,10 @@
 				#size-cells = <0>;
 				status = "okay";
 
-				ethernet-phy@7 {
-					reg = <0x7>;
+				mdio {
+					ethernet-phy@7 {
+						reg = <0x7>;
+					};
 				};
 			};
 
@@ -201,8 +203,10 @@
 				#size-cells = <0>;
 				status = "okay";
 
-				ethernet-phy@1 {
-					reg = <0x1>;
+				mdio {
+					ethernet-phy@1 {
+						reg = <0x1>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/at91-sama5d4_ma5d4evk.dts b/arch/arm/boot/dts/at91-sama5d4_ma5d4evk.dts
index fe05aaa7ac87..a361423a3969 100644
--- a/arch/arm/boot/dts/at91-sama5d4_ma5d4evk.dts
+++ b/arch/arm/boot/dts/at91-sama5d4_ma5d4evk.dts
@@ -63,8 +63,10 @@
 				phy-mode = "rmii";
 				status = "okay";
 
-				phy0: ethernet-phy@0 {
-					reg = <0>;
+				mdio {
+					phy0: ethernet-phy@0 {
+						reg = <0>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/at91-sama5d4_xplained.dts b/arch/arm/boot/dts/at91-sama5d4_xplained.dts
index 4b7c762d5f22..8b22ff53b40a 100644
--- a/arch/arm/boot/dts/at91-sama5d4_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d4_xplained.dts
@@ -95,10 +95,12 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&pinctrl_macb0_rmii &pinctrl_macb0_phy_irq>;
 
-				phy0: ethernet-phy@1 {
-					interrupt-parent = <&pioE>;
-					interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
-					reg = <1>;
+				mdio {
+					phy0: ethernet-phy@1 {
+						interrupt-parent = <&pioE>;
+						interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
+						reg = <1>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/at91-sama5d4ek.dts b/arch/arm/boot/dts/at91-sama5d4ek.dts
index 0702a2f2b336..35ccba00b8cb 100644
--- a/arch/arm/boot/dts/at91-sama5d4ek.dts
+++ b/arch/arm/boot/dts/at91-sama5d4ek.dts
@@ -144,10 +144,12 @@
 				phy-mode = "rmii";
 				status = "okay";
 
-				ethernet-phy@1 {
-					reg = <0x1>;
-					interrupt-parent = <&pioE>;
-					interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
+				mdio {
+					ethernet-phy@1 {
+						reg = <0x1>;
+						interrupt-parent = <&pioE>;
+						interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/at91-vinco.dts b/arch/arm/boot/dts/at91-vinco.dts
index 1be9889a2b3a..b690031ada28 100644
--- a/arch/arm/boot/dts/at91-vinco.dts
+++ b/arch/arm/boot/dts/at91-vinco.dts
@@ -116,11 +116,13 @@
 				phy-mode = "rmii";
 				status = "okay";
 
-				ethernet-phy@1 {
-					reg = <0x1>;
-					reset-gpios = <&pioE 8 GPIO_ACTIVE_LOW>;
-					interrupt-parent = <&pioB>;
-					interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
+				mdio {
+					ethernet-phy@1 {
+						reg = <0x1>;
+						reset-gpios = <&pioE 8 GPIO_ACTIVE_LOW>;
+						interrupt-parent = <&pioB>;
+						interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
+					};
 				};
 
 			};
@@ -170,11 +172,13 @@
 				#size-cells = <0>;
 				status = "okay";
 
-				ethernet-phy@1 {
-					reg = <0x1>;
-					interrupt-parent = <&pioB>;
-					interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
-					reset-gpios = <&pioE 6 GPIO_ACTIVE_LOW>;
+				mdio {
+					ethernet-phy@1 {
+						reg = <0x1>;
+						interrupt-parent = <&pioB>;
+						interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
+						reset-gpios = <&pioE 6 GPIO_ACTIVE_LOW>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/at91rm9200ek.dts b/arch/arm/boot/dts/at91rm9200ek.dts
index 81aaf8151c76..1b1cc47b2376 100644
--- a/arch/arm/boot/dts/at91rm9200ek.dts
+++ b/arch/arm/boot/dts/at91rm9200ek.dts
@@ -54,9 +54,11 @@
 				phy-mode = "rmii";
 				status = "okay";
 
-				phy0: ethernet-phy {
-					interrupt-parent = <&pioC>;
-					interrupts = <4 IRQ_TYPE_EDGE_BOTH>;
+				mdio {
+					phy0: ethernet-phy {
+						interrupt-parent = <&pioC>;
+						interrupts = <4 IRQ_TYPE_EDGE_BOTH>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index 61f68e5c48e9..424aabd0db68 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -1091,6 +1091,11 @@
 				clocks = <&macb0_clk>, <&macb0_clk>;
 				clock-names = "hclk", "pclk";
 				status = "disabled";
+
+				mdio {
+					#address-cells = <1>;
+					#size-cells = <0>;
+				};
 			};
 
 			tcb0: timer@f800c000 {
diff --git a/arch/arm/boot/dts/sama5d3_emac.dtsi b/arch/arm/boot/dts/sama5d3_emac.dtsi
index 7cb235ef0fb6..d0187d4da1da 100644
--- a/arch/arm/boot/dts/sama5d3_emac.dtsi
+++ b/arch/arm/boot/dts/sama5d3_emac.dtsi
@@ -49,6 +49,11 @@
 				clocks = <&macb1_clk>, <&macb1_clk>;
 				clock-names = "hclk", "pclk";
 				status = "disabled";
+
+				mdio {
+					#address-cells = <1>;
+					#size-cells = <0>;
+				};
 			};
 		};
 	};
diff --git a/arch/arm/boot/dts/sama5d3_gmac.dtsi b/arch/arm/boot/dts/sama5d3_gmac.dtsi
index 23f225fbb756..826d5891a2da 100644
--- a/arch/arm/boot/dts/sama5d3_gmac.dtsi
+++ b/arch/arm/boot/dts/sama5d3_gmac.dtsi
@@ -82,6 +82,11 @@
 				clocks = <&macb0_clk>, <&macb0_clk>;
 				clock-names = "hclk", "pclk";
 				status = "disabled";
+
+				mdio {
+					#address-cells = <1>;
+					#size-cells = <0>;
+				};
 			};
 		};
 	};
diff --git a/arch/arm/boot/dts/sama5d3xcm_cmp.dtsi b/arch/arm/boot/dts/sama5d3xcm_cmp.dtsi
index a02f59021364..948df9aa40ac 100644
--- a/arch/arm/boot/dts/sama5d3xcm_cmp.dtsi
+++ b/arch/arm/boot/dts/sama5d3xcm_cmp.dtsi
@@ -86,32 +86,34 @@
 				#address-cells = <1>;
 				#size-cells = <0>;
 
-				ethernet-phy@1 {
-					reg = <0x1>;
-					interrupt-parent = <&pioB>;
-					interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
-					txen-skew-ps = <800>;
-					txc-skew-ps = <3000>;
-					rxdv-skew-ps = <400>;
-					rxc-skew-ps = <3000>;
-					rxd0-skew-ps = <400>;
-					rxd1-skew-ps = <400>;
-					rxd2-skew-ps = <400>;
-					rxd3-skew-ps = <400>;
-				};
+				mdio {
+					ethernet-phy@1 {
+						reg = <0x1>;
+						interrupt-parent = <&pioB>;
+						interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+						txen-skew-ps = <800>;
+						txc-skew-ps = <3000>;
+						rxdv-skew-ps = <400>;
+						rxc-skew-ps = <3000>;
+						rxd0-skew-ps = <400>;
+						rxd1-skew-ps = <400>;
+						rxd2-skew-ps = <400>;
+						rxd3-skew-ps = <400>;
+					};
 
-				ethernet-phy@7 {
-					reg = <0x7>;
-					interrupt-parent = <&pioB>;
-					interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
-					txen-skew-ps = <800>;
-					txc-skew-ps = <3000>;
-					rxdv-skew-ps = <400>;
-					rxc-skew-ps = <3000>;
-					rxd0-skew-ps = <400>;
-					rxd1-skew-ps = <400>;
-					rxd2-skew-ps = <400>;
-					rxd3-skew-ps = <400>;
+					ethernet-phy@7 {
+						reg = <0x7>;
+						interrupt-parent = <&pioB>;
+						interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+						txen-skew-ps = <800>;
+						txc-skew-ps = <3000>;
+						rxdv-skew-ps = <400>;
+						rxc-skew-ps = <3000>;
+						rxd0-skew-ps = <400>;
+						rxd1-skew-ps = <400>;
+						rxd2-skew-ps = <400>;
+						rxd3-skew-ps = <400>;
+					};
 				};
 			};
 
diff --git a/arch/arm/boot/dts/sama5d3xmb_gmac.dtsi b/arch/arm/boot/dts/sama5d3xmb_gmac.dtsi
index 65aea7a75b1d..af097ff8bee4 100644
--- a/arch/arm/boot/dts/sama5d3xmb_gmac.dtsi
+++ b/arch/arm/boot/dts/sama5d3xmb_gmac.dtsi
@@ -15,32 +15,34 @@
 				#address-cells = <1>;
 				#size-cells = <0>;
 
-				ethernet-phy@1 {
-					reg = <0x1>;
-					interrupt-parent = <&pioB>;
-					interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
-					txen-skew-ps = <800>;
-					txc-skew-ps = <3000>;
-					rxdv-skew-ps = <400>;
-					rxc-skew-ps = <3000>;
-					rxd0-skew-ps = <400>;
-					rxd1-skew-ps = <400>;
-					rxd2-skew-ps = <400>;
-					rxd3-skew-ps = <400>;
-				};
+				mdio {
+					ethernet-phy@1 {
+						reg = <0x1>;
+						interrupt-parent = <&pioB>;
+						interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+						txen-skew-ps = <800>;
+						txc-skew-ps = <3000>;
+						rxdv-skew-ps = <400>;
+						rxc-skew-ps = <3000>;
+						rxd0-skew-ps = <400>;
+						rxd1-skew-ps = <400>;
+						rxd2-skew-ps = <400>;
+						rxd3-skew-ps = <400>;
+					};
 
-				ethernet-phy@7 {
-					reg = <0x7>;
-					interrupt-parent = <&pioB>;
-					interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
-					txen-skew-ps = <800>;
-					txc-skew-ps = <3000>;
-					rxdv-skew-ps = <400>;
-					rxc-skew-ps = <3000>;
-					rxd0-skew-ps = <400>;
-					rxd1-skew-ps = <400>;
-					rxd2-skew-ps = <400>;
-					rxd3-skew-ps = <400>;
+					ethernet-phy@7 {
+						reg = <0x7>;
+						interrupt-parent = <&pioB>;
+						interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+						txen-skew-ps = <800>;
+						txc-skew-ps = <3000>;
+						rxdv-skew-ps = <400>;
+						rxc-skew-ps = <3000>;
+						rxd0-skew-ps = <400>;
+						rxd1-skew-ps = <400>;
+						rxd2-skew-ps = <400>;
+						rxd3-skew-ps = <400>;
+					};
 				};
 			};
 		};
diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index 0cf9beddd556..d623a8dc0116 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -980,6 +980,11 @@
 				clocks = <&macb0_clk>, <&macb0_clk>;
 				clock-names = "hclk", "pclk";
 				status = "disabled";
+
+				mdio {
+					#address-cells = <1>;
+					#size-cells = <0>;
+				};
 			};
 
 			i2c2: i2c@f8024000 {
@@ -1220,6 +1225,11 @@
 				clocks = <&macb1_clk>, <&macb1_clk>;
 				clock-names = "hclk", "pclk";
 				status = "disabled";
+
+				mdio {
+					#address-cells = <1>;
+					#size-cells = <0>;
+				};
 			};
 
 			trng@fc030000 {
-- 
2.18.0

Re: [PATCH 1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Andrew Lunn]

On Mon, Aug 20, 2018 at 02:12:35PM +0200, Ahmad Fatoum wrote:
> The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
> There, of_mdiobus_register was called even for the fixed-link representing
> the SPI-connected switch PHY, with the result that the driver attempts to
> enumerate PHYs on a non-existent MDIO bus:
> 
> 	libphy: MACB_mii_bus: probed

So there are two different things here:

> 	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
> 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
>         [snip]
> 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31

These are the result of the fixed-link being considered a PHY in
of_mdiobus_register(). Patch 2 fixes that, turns it into a single
warning.

> 	macb f0028000.ethernet: broken fixed-link specification

Why is of_phy_register_fixed_link(np) failing?

> 
> Cc: <stable@vger.kernel.org>
> Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 27 +++++++++++++++---------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> Fixes since v1:
> 	Added one more error path for failing to register fixed-link
> 	Fixed a whitespace issue
> 
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index dc09f9a8a49b..ef6ce8691443 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -482,11 +482,6 @@ static int macb_mii_probe(struct net_device *dev)
>  
>  	if (np) {
>  		if (of_phy_is_fixed_link(np)) {
> -			if (of_phy_register_fixed_link(np) < 0) {
> -				dev_err(&bp->pdev->dev,
> -					"broken fixed-link specification\n");
> -				return -ENODEV;
> -			}

As a separate patch, please can you use the error code which
of_phy_register_fixed_link() returns, not -ENODEV. It is possible it
is returning EPROBE_DEFER.

   Andrew

Re: [PATCH 1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Ahmad Fatoum]

On 08/20/2018 03:55 PM, Andrew Lunn wrote:
> Why is of_phy_register_fixed_link(np) failing?

Apparently, the fixed-link's gpio's FLAG_REQUESTED bit remained set
causing gpiod_request_commit to return -EBUSY in (v4.18.0):

[<c0667750>] (gpiod_request_commit) from [<c066890c>] (gpiod_request+0x64/0x88)             
[<c066890c>] (gpiod_request) from [<c066a338>] (gpiod_get_from_of_node+0x48/0x13c)          
[<c066a338>] (gpiod_get_from_of_node) from [<c094b92c>] (mdiobus_register_device+0x70/0x124)
[<c094b92c>] (mdiobus_register_device) from [<c0949d2c>] (phy_device_register+0xc/0xa0)     
[<c0949d2c>] (phy_device_register) from [<c094e824>] (fixed_phy_register+0xe8/0x1f8)        
[<c094e824>] (fixed_phy_register) from [<c0b84260>] (of_phy_register_fixed_link+0x150/0x1e4)
[<c0b84260>] (of_phy_register_fixed_link) from [<c0964908>] (macb_probe+0x548/0xa7c)        
[<c0964908>] (macb_probe) from [<c086ebec>] (platform_drv_probe+0x48/0x98)                  

But that's a separate issue, I'll remove this line from the commit message...

Re: [PATCH 1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Andrew Lunn]

On Mon, Aug 20, 2018 at 05:56:53PM +0200, Ahmad Fatoum wrote:
> On 08/20/2018 03:55 PM, Andrew Lunn wrote:
> > Why is of_phy_register_fixed_link(np) failing?
> 
> Apparently, the fixed-link's gpio's FLAG_REQUESTED bit remained set
> causing gpiod_request_commit to return -EBUSY in (v4.18.0):
> 
> [<c0667750>] (gpiod_request_commit) from [<c066890c>] (gpiod_request+0x64/0x88)             
> [<c066890c>] (gpiod_request) from [<c066a338>] (gpiod_get_from_of_node+0x48/0x13c)          
> [<c066a338>] (gpiod_get_from_of_node) from [<c094b92c>] (mdiobus_register_device+0x70/0x124)
> [<c094b92c>] (mdiobus_register_device) from [<c0949d2c>] (phy_device_register+0xc/0xa0)     
> [<c0949d2c>] (phy_device_register) from [<c094e824>] (fixed_phy_register+0xe8/0x1f8)        
> [<c094e824>] (fixed_phy_register) from [<c0b84260>] (of_phy_register_fixed_link+0x150/0x1e4)
> [<c0b84260>] (of_phy_register_fixed_link) from [<c0964908>] (macb_probe+0x548/0xa7c)        
> [<c0964908>] (macb_probe) from [<c086ebec>] (platform_drv_probe+0x48/0x98)                  
> 
> But that's a separate issue, I'll remove this line from the commit message...

I would actually say, this is your real issue here. The warnings are
annoying, but i don't think they are fatal. This -EBUSY is what is
stopping the driver from loading, causing the real regression.

I'm guessing, but i think you will find the driver is loading once,
but hits a EPROBE_DEFFER condition, after getting the gpio. It does
not release the gpio correctly. Sometime later, it gets loaded again,
but the gpio is now in use, so you get the -EBUSY.

So check the error paths, and make sure cleanup is being done correct.
It could also be a phylib core bug...

   Andrew

Re: [PATCH 1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Ahmad Fatoum]

On 08/20/2018 09:06 PM, Andrew Lunn wrote:
> I would actually say, this is your real issue here. The warnings are
> annoying, but i don't think they are fatal. This -EBUSY is what is
> stopping the driver from loading, causing the real regression.

My real issue is that a specific commit broke the driver and I would like
to partially revert that offending commit.

> I'm guessing, but i think you will find the driver is loading once,
> but hits a EPROBE_DEFFER condition, after getting the gpio. It does
> not release the gpio correctly. Sometime later, it gets loaded again,
> but the gpio is now in use, so you get the -EBUSY.
> 
> So check the error paths, and make sure cleanup is being done correct.
> It could also be a phylib core bug...

I've traced it some more: While mdiobus_register fails to find a PHY,
creation of the "MDIO" bus is still successful and it returns successfully,
having claimed the reset GPIO (These functions should really be called
miibus_register...).

of_phy_fixed_link_register tries to claim the same GPIO and fails.

A fix for that would be something along the lines of da47b45 
("phy: add support for a reset-gpio specification"), which caused a regression
and was unfortunately later reverted.

But regardless, there shouldn't have been an of_mdiobus_register and a MDIO bus probe
before registering the fixed-link in the first place and my patch remedies that.

Reintroducing da47b45 would be out-of-scope for this patch series.


Cheers
Ahmad

Re: [PATCH 1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Andrew Lunn]

> I've traced it some more: While mdiobus_register fails to find a PHY,
> creation of the "MDIO" bus is still successful and it returns successfully,
> having claimed the reset GPIO.
> 
> of_phy_fixed_link_register tries to claim the same GPIO and fails.
 
I don't see where this is happening. It is looking for a gpio called
'link-gpios'.

> But regardless, there shouldn't have been an of_mdiobus_register and a MDIO bus probe
> before registering the fixed-link in the first place and my patch remedies that.

There are cases where you need both, e.g a switch controller over
MDIO. So we cannot make it one or the other.

However, there are currently no such boards. So far "net", lets go
with your partial revert patch. But we also need patches for
"net-next" which put it back again, allows both fixed-link and an
mdiobus inside a subnode, and not break backwards compatibility.

	Andrew

Re: [PATCH 1/4] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Ahmad Fatoum]

On 08/21/2018 03:34 PM, Andrew Lunn wrote:
> I don't see where this is happening. It is looking for a gpio called
> 'link-gpios'.

First while registering the MDIO bus in __mdiobus_register:

	gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW);

and then again when registering the fixed-link in mdiobus_register_gpiod:

	gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
				       "reset-gpios", 0, GPIOD_OUT_LOW,
				       "PHY reset");
 
>> But regardless, there shouldn't have been an of_mdiobus_register and a MDIO bus probe
>> before registering the fixed-link in the first place and my patch remedies that.
> 
> There are cases where you need both, e.g a switch controller over
> MDIO. So we cannot make it one or the other.

I see.

> However, there are currently no such boards. So far "net", lets go
> with your partial revert patch.

I will resend the first patch.

> But we also need patches for
> "net-next" which put it back again, allows both fixed-link and an
> mdiobus inside a subnode, and not break backwards compatibility.

I'll resend the remainder when net-next opens again.


Cheers
Ahmad