| Message ID | 20230808120254.11653-1-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: stmmac: don't create the MDIO bus if there's no mdio node on DT | expand |
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
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
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
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; }