[PATCH net-next] net: stmmac: don't create the MDIO bus if there's no mdio node on DT

[PATCH net-next] net: stmmac: don't create the MDIO bus if there's no mdio node on DT

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The stmmac_dt_phy() function that parses the device-tree node of the MAC
and allocates the MDIO and PHY resources misses one use-case: when the
MAC doesn't have a fixed link but also doesn't define its own mdio bus
on the device tree and instead shares the MDIO lines with a different
MAC with its PHY phandle reaching over into a different node.

As this function could also use some more readability, rework it to
handle this use-case and simplify the code.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 .../ethernet/stmicro/stmmac/stmmac_platform.c | 26 +++++++++----------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index be8e79c7aa34..91844673df43 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -320,12 +320,14 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
 static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
 			 struct device_node *np, struct device *dev)
 {
-	bool mdio = !of_phy_is_fixed_link(np);
 	static const struct of_device_id need_mdio_ids[] = {
 		{ .compatible = "snps,dwc-qos-ethernet-4.10" },
 		{},
 	};
 
+	if (of_phy_is_fixed_link(np))
+		return 0;
+
 	if (of_match_node(need_mdio_ids, np)) {
 		plat->mdio_node = of_get_child_by_name(np, "mdio");
 	} else {
@@ -340,20 +342,18 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
 		}
 	}
 
-	if (plat->mdio_node) {
-		dev_dbg(dev, "Found MDIO subnode\n");
-		mdio = true;
-	}
+	if (!plat->mdio_node)
+		return 0;
 
-	if (mdio) {
-		plat->mdio_bus_data =
-			devm_kzalloc(dev, sizeof(struct stmmac_mdio_bus_data),
-				     GFP_KERNEL);
-		if (!plat->mdio_bus_data)
-			return -ENOMEM;
+	dev_dbg(dev, "Found MDIO subnode\n");
 
-		plat->mdio_bus_data->needs_reset = true;
-	}
+	plat->mdio_bus_data = devm_kzalloc(dev,
+					   sizeof(struct stmmac_mdio_bus_data),
+					   GFP_KERNEL);
+	if (!plat->mdio_bus_data)
+		return -ENOMEM;
+
+	plat->mdio_bus_data->needs_reset = true;
 
 	return 0;
 }
-- 
2.39.2

Re: [PATCH net-next] net: stmmac: don't create the MDIO bus if there's no mdio node on DT

[Andrew Lunn]

On Tue, Aug 08, 2023 at 02:02:54PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> The stmmac_dt_phy() function that parses the device-tree node of the MAC
> and allocates the MDIO and PHY resources misses one use-case: when the
> MAC doesn't have a fixed link but also doesn't define its own mdio bus
> on the device tree and instead shares the MDIO lines with a different
> MAC with its PHY phandle reaching over into a different node.

It does not share the MDIO lines. The other MDIO bus master happens to
have two PHYs and there are no PHYs on this MDIO bus, so no point
instantiating it.

>  static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
>  			 struct device_node *np, struct device *dev)
>  {
> -	bool mdio = !of_phy_is_fixed_link(np);
>  	static const struct of_device_id need_mdio_ids[] = {
>  		{ .compatible = "snps,dwc-qos-ethernet-4.10" },
>  		{},
>  	};
>  
> +	if (of_phy_is_fixed_link(np))
> +		return 0;
> +

		/**
		 * If snps,dwmac-mdio is passed from DT, always register
		 * the MDIO
		 */
		for_each_child_of_node(np, plat->mdio_node) {
			if (of_device_is_compatible(plat->mdio_node,
						    "snps,dwmac-mdio"))
				break;
		}

The comment suggests it should always be registered. This MAC might
have a fixed-phy, but that does not mean there is not an Ethernet
switch on the bus, or a PHY for some other MAC etc. MDIO busses
masters should be considered fully independent devices.

https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts

&fec1 {
	phy-mode = "rmii";
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_fec1>;
	status = "okay";

	fixed-link {
		speed = <100>;
		full-duplex;
	};

	mdio1: mdio {
		#address-cells = <1>;
		#size-cells = <0>;
		clock-frequency = <12500000>;
		suppress-preamble;
		status = "okay";

		switch0: switch0@0 {
			compatible = "marvell,mv88e6190";
			pinctrl-0 = <&pinctrl_gpio_switch0>;
			pinctrl-names = "default";
			reg = <0>;
			eeprom-length = <65536>;
			interrupt-parent = <&gpio3>;
			interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
			interrupt-controller;
			#interrupt-cells = <2>;

Both a fixed link, and something on the MDIO bus....

     Andrew

Re: [PATCH net-next] net: stmmac: don't create the MDIO bus if there's no mdio node on DT

[Bartosz Golaszewski]

On Tue, Aug 8, 2023 at 8:46 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Aug 08, 2023 at 02:02:54PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > The stmmac_dt_phy() function that parses the device-tree node of the MAC
> > and allocates the MDIO and PHY resources misses one use-case: when the
> > MAC doesn't have a fixed link but also doesn't define its own mdio bus
> > on the device tree and instead shares the MDIO lines with a different
> > MAC with its PHY phandle reaching over into a different node.
>
> It does not share the MDIO lines. The other MDIO bus master happens to
> have two PHYs and there are no PHYs on this MDIO bus, so no point
> instantiating it.

Yes, I sent it before we established that thanks to Andrew's input.

>
> >  static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
> >                        struct device_node *np, struct device *dev)
> >  {
> > -     bool mdio = !of_phy_is_fixed_link(np);
> >       static const struct of_device_id need_mdio_ids[] = {
> >               { .compatible = "snps,dwc-qos-ethernet-4.10" },
> >               {},
> >       };
> >
> > +     if (of_phy_is_fixed_link(np))
> > +             return 0;
> > +
>
>                 /**
>                  * If snps,dwmac-mdio is passed from DT, always register
>                  * the MDIO
>                  */
>                 for_each_child_of_node(np, plat->mdio_node) {
>                         if (of_device_is_compatible(plat->mdio_node,
>                                                     "snps,dwmac-mdio"))
>                                 break;
>                 }
>
> The comment suggests it should always be registered. This MAC might
> have a fixed-phy, but that does not mean there is not an Ethernet
> switch on the bus, or a PHY for some other MAC etc. MDIO busses
> masters should be considered fully independent devices.
>
> https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts
>
> &fec1 {
>         phy-mode = "rmii";
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_fec1>;
>         status = "okay";
>
>         fixed-link {
>                 speed = <100>;
>                 full-duplex;
>         };
>
>         mdio1: mdio {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 clock-frequency = <12500000>;
>                 suppress-preamble;
>                 status = "okay";
>
>                 switch0: switch0@0 {
>                         compatible = "marvell,mv88e6190";
>                         pinctrl-0 = <&pinctrl_gpio_switch0>;
>                         pinctrl-names = "default";
>                         reg = <0>;
>                         eeprom-length = <65536>;
>                         interrupt-parent = <&gpio3>;
>                         interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
>                         interrupt-controller;
>                         #interrupt-cells = <2>;
>
> Both a fixed link, and something on the MDIO bus....
>
>      Andrew

Makes sense, we can drop all my stmmac patches from today, I need to
rethink it in detail.

Bart

Re: [PATCH net-next] net: stmmac: don't create the MDIO bus if there's no mdio node on DT

[Alexandre TORGUE]

On 8/8/23 14:02, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> The stmmac_dt_phy() function that parses the device-tree node of the MAC
> and allocates the MDIO and PHY resources misses one use-case: when the
> MAC doesn't have a fixed link but also doesn't define its own mdio bus
> on the device tree and instead shares the MDIO lines with a different
> MAC with its PHY phandle reaching over into a different node.
> 
> As this function could also use some more readability, rework it to
> handle this use-case and simplify the code.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   .../ethernet/stmicro/stmmac/stmmac_platform.c | 26 +++++++++----------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index be8e79c7aa34..91844673df43 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -320,12 +320,14 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
>   static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
>   			 struct device_node *np, struct device *dev)
>   {
> -	bool mdio = !of_phy_is_fixed_link(np);
>   	static const struct of_device_id need_mdio_ids[] = {
>   		{ .compatible = "snps,dwc-qos-ethernet-4.10" },
>   		{},
>   	};
>   
> +	if (of_phy_is_fixed_link(np))
> +		return 0;
> +
>   	if (of_match_node(need_mdio_ids, np)) {
>   		plat->mdio_node = of_get_child_by_name(np, "mdio");
>   	} else {
> @@ -340,20 +342,18 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
>   		}
>   	}
>   
> -	if (plat->mdio_node) {
> -		dev_dbg(dev, "Found MDIO subnode\n");
> -		mdio = true;
> -	}
> +	if (!plat->mdio_node)
> +		return 0;
>   
> -	if (mdio) {
> -		plat->mdio_bus_data =
> -			devm_kzalloc(dev, sizeof(struct stmmac_mdio_bus_data),
> -				     GFP_KERNEL);
> -		if (!plat->mdio_bus_data)
> -			return -ENOMEM;
> +	dev_dbg(dev, "Found MDIO subnode\n");
>   
> -		plat->mdio_bus_data->needs_reset = true;
> -	}
> +	plat->mdio_bus_data = devm_kzalloc(dev,
> +					   sizeof(struct stmmac_mdio_bus_data),
> +					   GFP_KERNEL);
> +	if (!plat->mdio_bus_data)
> +		return -ENOMEM;
> +
> +	plat->mdio_bus_data->needs_reset = true;
>   
>   	return 0;
>   }

Acked-by: Alexandre TORGUE <alexandre.torgue@foss.st.com>

Regards
Alex