| Message ID | 20231206-stmmac-no-mdio-node-v2-1-333cae49b1ca@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: stmmac: don't create a MDIO bus if unnecessary | expand |
On Wed, Dec 06, 2023 at 05:46:09PM -0600, Andrew Halaney wrote: > The stmmac_dt_phy() function, which parses the devicetree node of the > MAC and ultimately causes MDIO bus allocation, misinterprets what > fixed-link means in relation to the MAC's MDIO bus. This results in > a MDIO bus being created in situations it need not be. > > Currently a MDIO bus is created if the description is either: > > 1. Not fixed-link > 2. fixed-link but contains a MDIO bus as well > > The "1" case above isn't always accurate. If there's a phy-handle, > it could be referencing a phy on another MDIO controller's bus[1]. In > this case currently the MAC will make a MDIO bus and scan it all > anyways unnecessarily. > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to > be created and scanned for a phy. This case can also be inferred from > the platform description by not having a phy-handle && not being > fixed-link. This hits case "1" in the current driver's logic. > > Let's improve the logic to create a MDIO bus if either: > > - Devicetree contains a MDIO bus > - !fixed-link && !phy-handle (legacy handling) > > Below upstream devicetree snippets can be found that explain some of > the cases above more concretely. > > Here's[0] a devicetree example where the MAC is both fixed-link and > driving a switch on MDIO (case "2" above). This needs a MDIO bus to > be created: > > &fec1 { > phy-mode = "rmii"; > > fixed-link { > speed = <100>; > full-duplex; > }; > > mdio1: mdio { > switch0: switch0@0 { > compatible = "marvell,mv88e6190"; > pinctrl-0 = <&pinctrl_gpio_switch0>; > }; > }; > }; > > Here's[1] an example where there is no MDIO bus or fixed-link for > the ethernet1 MAC, so no MDIO bus should be created since ethernet0 > is the MDIO master for ethernet1's phy: > > ðernet0 { > phy-mode = "sgmii"; > phy-handle = <&sgmii_phy0>; > > mdio { > compatible = "snps,dwmac-mdio"; > sgmii_phy0: phy@8 { > compatible = "ethernet-phy-id0141.0dd4"; > reg = <0x8>; > device_type = "ethernet-phy"; > }; > > sgmii_phy1: phy@a { > compatible = "ethernet-phy-id0141.0dd4"; > reg = <0xa>; > device_type = "ethernet-phy"; > }; > }; > }; > > ðernet1 { > phy-mode = "sgmii"; > phy-handle = <&sgmii_phy1>; > }; > > Finally there's descriptions like this[2] which don't describe the > MDIO bus but expect it to be created and the whole address space > scanned for a phy since there's no phy-handle or fixed-link described: > > &gmac { > phy-supply = <&vcc_lan>; > phy-mode = "rmii"; > snps,reset-gpio = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>; > snps,reset-active-low; > snps,reset-delays-us = <0 10000 1000000>; > }; > > [0] https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts > [1] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/qcom/sa8775p-ride.dts > [2] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/rockchip/rk3368-r88.dts#L164 > > Co-developed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- Gah, I failed to describe my changes since Bart's v1 when picking this up with b4 to make v2. Whoops! Changes since v1: - Handle the fixed-link + mdio case (Andrew Lunn) - Reworded commit message - Handle the "legacy" case still mentioned in the commit - Bit further refactoring of the function
Hi Andrew On Wed, Dec 06, 2023 at 05:46:09PM -0600, Andrew Halaney wrote: > The stmmac_dt_phy() function, which parses the devicetree node of the > MAC and ultimately causes MDIO bus allocation, misinterprets what > fixed-link means in relation to the MAC's MDIO bus. This results in > a MDIO bus being created in situations it need not be. > > Currently a MDIO bus is created if the description is either: > > 1. Not fixed-link > 2. fixed-link but contains a MDIO bus as well > > The "1" case above isn't always accurate. If there's a phy-handle, > it could be referencing a phy on another MDIO controller's bus[1]. In > this case currently the MAC will make a MDIO bus and scan it all > anyways unnecessarily. > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to > be created and scanned for a phy. This case can also be inferred from > the platform description by not having a phy-handle && not being > fixed-link. This hits case "1" in the current driver's logic. > > Let's improve the logic to create a MDIO bus if either: > > - Devicetree contains a MDIO bus > - !fixed-link && !phy-handle (legacy handling) > > Below upstream devicetree snippets can be found that explain some of > the cases above more concretely. > > Here's[0] a devicetree example where the MAC is both fixed-link and > driving a switch on MDIO (case "2" above). This needs a MDIO bus to > be created: > > &fec1 { > phy-mode = "rmii"; > > fixed-link { > speed = <100>; > full-duplex; > }; > > mdio1: mdio { > switch0: switch0@0 { > compatible = "marvell,mv88e6190"; > pinctrl-0 = <&pinctrl_gpio_switch0>; > }; > }; > }; > > Here's[1] an example where there is no MDIO bus or fixed-link for > the ethernet1 MAC, so no MDIO bus should be created since ethernet0 > is the MDIO master for ethernet1's phy: > > ðernet0 { > phy-mode = "sgmii"; > phy-handle = <&sgmii_phy0>; > > mdio { > compatible = "snps,dwmac-mdio"; > sgmii_phy0: phy@8 { > compatible = "ethernet-phy-id0141.0dd4"; > reg = <0x8>; > device_type = "ethernet-phy"; > }; > > sgmii_phy1: phy@a { > compatible = "ethernet-phy-id0141.0dd4"; > reg = <0xa>; > device_type = "ethernet-phy"; > }; > }; > }; > > ðernet1 { > phy-mode = "sgmii"; > phy-handle = <&sgmii_phy1>; > }; > > Finally there's descriptions like this[2] which don't describe the > MDIO bus but expect it to be created and the whole address space > scanned for a phy since there's no phy-handle or fixed-link described: > > &gmac { > phy-supply = <&vcc_lan>; > phy-mode = "rmii"; > snps,reset-gpio = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>; > snps,reset-active-low; > snps,reset-delays-us = <0 10000 1000000>; > }; > > [0] https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts > [1] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/qcom/sa8775p-ride.dts > [2] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/rockchip/rk3368-r88.dts#L164 Thank you for the patch. Please find a comment below. > > Co-developed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 85 ++++++++++------------ > 1 file changed, 37 insertions(+), 48 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index 1ffde555da47..7da461fe93f6 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -296,69 +296,39 @@ static int stmmac_mtl_setup(struct platform_device *pdev, > } > > /** > - * stmmac_dt_phy - parse device-tree driver parameters to allocate PHY resources > - * @plat: driver data platform structure > - * @np: device tree node > - * @dev: device pointer > - * Description: > - * The mdio bus will be allocated in case of a phy transceiver is on board; > - * it will be NULL if the fixed-link is configured. > - * If there is the "snps,dwmac-mdio" sub-node the mdio will be allocated > - * in any case (for DSA, mdio must be registered even if fixed-link). > - * The table below sums the supported configurations: > - * ------------------------------- > - * snps,phy-addr | Y > - * ------------------------------- > - * phy-handle | Y > - * ------------------------------- > - * fixed-link | N > - * ------------------------------- > - * snps,dwmac-mdio | > - * even if | Y > - * fixed-link | > - * ------------------------------- > + * stmmac_of_get_mdio() - Gets the MDIO bus from the devicetree > + * @np: devicetree node > * > - * It returns 0 in case of success otherwise -ENODEV. > + * The MDIO bus will be searched for in the following ways: > + * 1. The compatible is "snps,dwc-qos-ethernet-4.10" && a "mdio" named > + * child node exists > + * 2. A child node with the "snps,dwmac-mdio" compatible is present > + * > + * Return: The MDIO node if present otherwise NULL > */ > -static int stmmac_dt_phy(struct plat_stmmacenet_data *plat, > - struct device_node *np, struct device *dev) > +static struct device_node *stmmac_of_get_mdio(struct device_node *np) > { > - bool mdio = !of_phy_is_fixed_link(np); > static const struct of_device_id need_mdio_ids[] = { > { .compatible = "snps,dwc-qos-ethernet-4.10" }, > {}, > }; > + struct device_node *mdio_node = NULL; > > if (of_match_node(need_mdio_ids, np)) { > - plat->mdio_node = of_get_child_by_name(np, "mdio"); > + mdio_node = of_get_child_by_name(np, "mdio"); > } else { > /** > * 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, > + for_each_child_of_node(np, mdio_node) { > + if (of_device_is_compatible(mdio_node, > "snps,dwmac-mdio")) > break; > } > } > > - if (plat->mdio_node) { > - dev_dbg(dev, "Found MDIO subnode\n"); > - mdio = true; > - } > - > - if (mdio) { > - 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; > + return mdio_node; > } > > /** > @@ -417,6 +387,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > struct device_node *np = pdev->dev.of_node; > struct plat_stmmacenet_data *plat; > struct stmmac_dma_cfg *dma_cfg; > + bool legacy_mdio; > int phy_mode; > void *ret; > int rc; > @@ -471,10 +442,28 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > if (of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr) == 0) > dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n"); > > - /* To Configure PHY by using all device-tree supported properties */ > - rc = stmmac_dt_phy(plat, np, &pdev->dev); > - if (rc) > - return ERR_PTR(rc); > + plat->mdio_node = stmmac_of_get_mdio(np); > + if (plat->mdio_node) > + dev_dbg(&pdev->dev, "Found MDIO subnode\n"); > + > + /* Legacy devicetrees allowed for no MDIO bus description and expect > + * the bus to be scanned for devices. If there's no phy or fixed-link > + * described assume this is the case since there must be something > + * connected to the MAC. > + */ > + legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node; > + if (legacy_mdio) > + dev_info(&pdev->dev, "Deprecated MDIO bus assumption used\n"); > + > + if (plat->mdio_node || legacy_mdio) { > + plat->mdio_bus_data = devm_kzalloc(&pdev->dev, > + sizeof(struct stmmac_mdio_bus_data), > + GFP_KERNEL); > + if (!plat->mdio_bus_data) > + return ERR_PTR(-ENOMEM); > + > + plat->mdio_bus_data->needs_reset = true; > + } Why did you decide to move this out of the dedicated function? stmmac_probe_config_dt() is already overwhelmed with various non-coherent actions. The method has already got to being too long to follow the kernel coding style limit (I have got a not submitted yet cleanup patchset which step-by-step fixes that). Could you please get the chunk above back to the respective function and, for instance, just change it's name to something like stmmac_mdio_setup()? (I prefer having "_setup" suffix because some of the locally defined static methods already use it: stmmac_axi_setup(), stmmac_mtl_setup().) -Serge(y) > > of_property_read_u32(np, "tx-fifo-depth", &plat->tx_fifo_size); > > > --- > base-commit: fd8a79b63710acb84321be3ce74be23c876873fb > change-id: 20231127-stmmac-no-mdio-node-1db9da8a25d9 > > Best regards, > -- > Andrew Halaney <ahalaney@redhat.com> > >
On Thu, Dec 07, 2023 at 02:56:25PM +0300, Serge Semin wrote: > Hi Andrew > > On Wed, Dec 06, 2023 at 05:46:09PM -0600, Andrew Halaney wrote: > > The stmmac_dt_phy() function, which parses the devicetree node of the > > MAC and ultimately causes MDIO bus allocation, misinterprets what > > fixed-link means in relation to the MAC's MDIO bus. This results in > > a MDIO bus being created in situations it need not be. > > > > Currently a MDIO bus is created if the description is either: > > > > 1. Not fixed-link > > 2. fixed-link but contains a MDIO bus as well > > > > The "1" case above isn't always accurate. If there's a phy-handle, > > it could be referencing a phy on another MDIO controller's bus[1]. In > > this case currently the MAC will make a MDIO bus and scan it all > > anyways unnecessarily. > > > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to > > be created and scanned for a phy. This case can also be inferred from > > the platform description by not having a phy-handle && not being > > fixed-link. This hits case "1" in the current driver's logic. > > > > Let's improve the logic to create a MDIO bus if either: > > > > - Devicetree contains a MDIO bus > > - !fixed-link && !phy-handle (legacy handling) > > > > Below upstream devicetree snippets can be found that explain some of > > the cases above more concretely. > > > > Here's[0] a devicetree example where the MAC is both fixed-link and > > driving a switch on MDIO (case "2" above). This needs a MDIO bus to > > be created: > > > > &fec1 { > > phy-mode = "rmii"; > > > > fixed-link { > > speed = <100>; > > full-duplex; > > }; > > > > mdio1: mdio { > > switch0: switch0@0 { > > compatible = "marvell,mv88e6190"; > > pinctrl-0 = <&pinctrl_gpio_switch0>; > > }; > > }; > > }; > > > > Here's[1] an example where there is no MDIO bus or fixed-link for > > the ethernet1 MAC, so no MDIO bus should be created since ethernet0 > > is the MDIO master for ethernet1's phy: > > > > ðernet0 { > > phy-mode = "sgmii"; > > phy-handle = <&sgmii_phy0>; > > > > mdio { > > compatible = "snps,dwmac-mdio"; > > sgmii_phy0: phy@8 { > > compatible = "ethernet-phy-id0141.0dd4"; > > reg = <0x8>; > > device_type = "ethernet-phy"; > > }; > > > > sgmii_phy1: phy@a { > > compatible = "ethernet-phy-id0141.0dd4"; > > reg = <0xa>; > > device_type = "ethernet-phy"; > > }; > > }; > > }; > > > > ðernet1 { > > phy-mode = "sgmii"; > > phy-handle = <&sgmii_phy1>; > > }; > > > > Finally there's descriptions like this[2] which don't describe the > > MDIO bus but expect it to be created and the whole address space > > scanned for a phy since there's no phy-handle or fixed-link described: > > > > &gmac { > > phy-supply = <&vcc_lan>; > > phy-mode = "rmii"; > > snps,reset-gpio = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>; > > snps,reset-active-low; > > snps,reset-delays-us = <0 10000 1000000>; > > }; > > > > [0] https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts > > [1] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/qcom/sa8775p-ride.dts > > [2] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/rockchip/rk3368-r88.dts#L164 > > Thank you for the patch. Please find a comment below. > > > > > Co-developed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > --- > > .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 85 ++++++++++------------ > > 1 file changed, 37 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > index 1ffde555da47..7da461fe93f6 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > @@ -296,69 +296,39 @@ static int stmmac_mtl_setup(struct platform_device *pdev, > > } > > > > /** > > - * stmmac_dt_phy - parse device-tree driver parameters to allocate PHY resources > > - * @plat: driver data platform structure > > - * @np: device tree node > > - * @dev: device pointer > > - * Description: > > - * The mdio bus will be allocated in case of a phy transceiver is on board; > > - * it will be NULL if the fixed-link is configured. > > - * If there is the "snps,dwmac-mdio" sub-node the mdio will be allocated > > - * in any case (for DSA, mdio must be registered even if fixed-link). > > - * The table below sums the supported configurations: > > - * ------------------------------- > > - * snps,phy-addr | Y > > - * ------------------------------- > > - * phy-handle | Y > > - * ------------------------------- > > - * fixed-link | N > > - * ------------------------------- > > - * snps,dwmac-mdio | > > - * even if | Y > > - * fixed-link | > > - * ------------------------------- > > + * stmmac_of_get_mdio() - Gets the MDIO bus from the devicetree > > + * @np: devicetree node > > * > > - * It returns 0 in case of success otherwise -ENODEV. > > + * The MDIO bus will be searched for in the following ways: > > + * 1. The compatible is "snps,dwc-qos-ethernet-4.10" && a "mdio" named > > + * child node exists > > + * 2. A child node with the "snps,dwmac-mdio" compatible is present > > + * > > + * Return: The MDIO node if present otherwise NULL > > */ > > -static int stmmac_dt_phy(struct plat_stmmacenet_data *plat, > > - struct device_node *np, struct device *dev) > > +static struct device_node *stmmac_of_get_mdio(struct device_node *np) > > { > > - bool mdio = !of_phy_is_fixed_link(np); > > static const struct of_device_id need_mdio_ids[] = { > > { .compatible = "snps,dwc-qos-ethernet-4.10" }, > > {}, > > }; > > + struct device_node *mdio_node = NULL; > > > > if (of_match_node(need_mdio_ids, np)) { > > - plat->mdio_node = of_get_child_by_name(np, "mdio"); > > + mdio_node = of_get_child_by_name(np, "mdio"); > > } else { > > /** > > * 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, > > + for_each_child_of_node(np, mdio_node) { > > + if (of_device_is_compatible(mdio_node, > > "snps,dwmac-mdio")) > > break; > > } > > } > > > > - if (plat->mdio_node) { > > - dev_dbg(dev, "Found MDIO subnode\n"); > > - mdio = true; > > - } > > - > > - if (mdio) { > > - 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; > > + return mdio_node; > > } > > > > /** > > @@ -417,6 +387,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > > struct device_node *np = pdev->dev.of_node; > > struct plat_stmmacenet_data *plat; > > struct stmmac_dma_cfg *dma_cfg; > > + bool legacy_mdio; > > int phy_mode; > > void *ret; > > int rc; > > @@ -471,10 +442,28 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > > if (of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr) == 0) > > dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n"); > > > > - /* To Configure PHY by using all device-tree supported properties */ > > - rc = stmmac_dt_phy(plat, np, &pdev->dev); > > - if (rc) > > - return ERR_PTR(rc); > > + plat->mdio_node = stmmac_of_get_mdio(np); > > + if (plat->mdio_node) > > + dev_dbg(&pdev->dev, "Found MDIO subnode\n"); > > + > > > + /* Legacy devicetrees allowed for no MDIO bus description and expect > > + * the bus to be scanned for devices. If there's no phy or fixed-link > > + * described assume this is the case since there must be something > > + * connected to the MAC. > > + */ > > + legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node; > > + if (legacy_mdio) > > + dev_info(&pdev->dev, "Deprecated MDIO bus assumption used\n"); > > + > > + if (plat->mdio_node || legacy_mdio) { > > + plat->mdio_bus_data = devm_kzalloc(&pdev->dev, > > + sizeof(struct stmmac_mdio_bus_data), > > + GFP_KERNEL); > > + if (!plat->mdio_bus_data) > > + return ERR_PTR(-ENOMEM); > > + > > + plat->mdio_bus_data->needs_reset = true; > > + } > > Why did you decide to move this out of the dedicated function? > stmmac_probe_config_dt() is already overwhelmed with various > non-coherent actions. The method has already got to being too long to > follow the kernel coding style limit (I have got a not submitted yet > cleanup patchset which step-by-step fixes that). Could you please get > the chunk above back to the respective function and, for instance, > just change it's name to something like stmmac_mdio_setup()? (I prefer > having "_setup" suffix because some of the locally defined static > methods already use it: stmmac_axi_setup(), stmmac_mtl_setup().) Sure, I can put it back in. I'll probably keep stmmac_of_get_mdio() (which is named in the style I see in the current file -- although as you highlight that shouldn't be taken as the best example of clean), and have stmmac_mdio_setup() call that and and mimic the inputs/outputs of the current function (moving the rest of the added code from stmmac_probe_config_dt() back into that function). Thanks for the feedback, let me know if you still think that abstraction isn't ideal (or wait till I post it :P) and I'll go with exactly as you said. I'm not _too_ opinionated on it, but thought stmmac_dt_phy() didn't explain much and stmmac_of_get_mdio() was self-explanatory enough that it helped readability. > > -Serge(y) > > > > > of_property_read_u32(np, "tx-fifo-depth", &plat->tx_fifo_size); > > > > > > --- > > base-commit: fd8a79b63710acb84321be3ce74be23c876873fb > > change-id: 20231127-stmmac-no-mdio-node-1db9da8a25d9 > > > > Best regards, > > -- > > Andrew Halaney <ahalaney@redhat.com> > > > > >
On Thu, Dec 07, 2023 at 07:32:29AM -0600, Andrew Halaney wrote: > On Thu, Dec 07, 2023 at 02:56:25PM +0300, Serge Semin wrote: > > Hi Andrew > > > > On Wed, Dec 06, 2023 at 05:46:09PM -0600, Andrew Halaney wrote: > > > The stmmac_dt_phy() function, which parses the devicetree node of the > > > MAC and ultimately causes MDIO bus allocation, misinterprets what > > > fixed-link means in relation to the MAC's MDIO bus. This results in > > > a MDIO bus being created in situations it need not be. > > > > > > Currently a MDIO bus is created if the description is either: > > > > > > 1. Not fixed-link > > > 2. fixed-link but contains a MDIO bus as well > > > > > > The "1" case above isn't always accurate. If there's a phy-handle, > > > it could be referencing a phy on another MDIO controller's bus[1]. In > > > this case currently the MAC will make a MDIO bus and scan it all > > > anyways unnecessarily. > > > > > > There's also a lot of upstream devicetrees[2] that expect a MDIO bus to > > > be created and scanned for a phy. This case can also be inferred from > > > the platform description by not having a phy-handle && not being > > > fixed-link. This hits case "1" in the current driver's logic. > > > > > > Let's improve the logic to create a MDIO bus if either: > > > > > > - Devicetree contains a MDIO bus > > > - !fixed-link && !phy-handle (legacy handling) > > > > > > Below upstream devicetree snippets can be found that explain some of > > > the cases above more concretely. > > > > > > Here's[0] a devicetree example where the MAC is both fixed-link and > > > driving a switch on MDIO (case "2" above). This needs a MDIO bus to > > > be created: > > > > > > &fec1 { > > > phy-mode = "rmii"; > > > > > > fixed-link { > > > speed = <100>; > > > full-duplex; > > > }; > > > > > > mdio1: mdio { > > > switch0: switch0@0 { > > > compatible = "marvell,mv88e6190"; > > > pinctrl-0 = <&pinctrl_gpio_switch0>; > > > }; > > > }; > > > }; > > > > > > Here's[1] an example where there is no MDIO bus or fixed-link for > > > the ethernet1 MAC, so no MDIO bus should be created since ethernet0 > > > is the MDIO master for ethernet1's phy: > > > > > > ðernet0 { > > > phy-mode = "sgmii"; > > > phy-handle = <&sgmii_phy0>; > > > > > > mdio { > > > compatible = "snps,dwmac-mdio"; > > > sgmii_phy0: phy@8 { > > > compatible = "ethernet-phy-id0141.0dd4"; > > > reg = <0x8>; > > > device_type = "ethernet-phy"; > > > }; > > > > > > sgmii_phy1: phy@a { > > > compatible = "ethernet-phy-id0141.0dd4"; > > > reg = <0xa>; > > > device_type = "ethernet-phy"; > > > }; > > > }; > > > }; > > > > > > ðernet1 { > > > phy-mode = "sgmii"; > > > phy-handle = <&sgmii_phy1>; > > > }; > > > > > > Finally there's descriptions like this[2] which don't describe the > > > MDIO bus but expect it to be created and the whole address space > > > scanned for a phy since there's no phy-handle or fixed-link described: > > > > > > &gmac { > > > phy-supply = <&vcc_lan>; > > > phy-mode = "rmii"; > > > snps,reset-gpio = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>; > > > snps,reset-active-low; > > > snps,reset-delays-us = <0 10000 1000000>; > > > }; > > > > > > [0] https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts > > > [1] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/qcom/sa8775p-ride.dts > > > [2] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/rockchip/rk3368-r88.dts#L164 > > > > Thank you for the patch. Please find a comment below. > > > > > > > > Co-developed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > > --- > > > .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 85 ++++++++++------------ > > > 1 file changed, 37 insertions(+), 48 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > > index 1ffde555da47..7da461fe93f6 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > > @@ -296,69 +296,39 @@ static int stmmac_mtl_setup(struct platform_device *pdev, > > > } > > > > > > /** > > > - * stmmac_dt_phy - parse device-tree driver parameters to allocate PHY resources > > > - * @plat: driver data platform structure > > > - * @np: device tree node > > > - * @dev: device pointer > > > - * Description: > > > - * The mdio bus will be allocated in case of a phy transceiver is on board; > > > - * it will be NULL if the fixed-link is configured. > > > - * If there is the "snps,dwmac-mdio" sub-node the mdio will be allocated > > > - * in any case (for DSA, mdio must be registered even if fixed-link). > > > - * The table below sums the supported configurations: > > > - * ------------------------------- > > > - * snps,phy-addr | Y > > > - * ------------------------------- > > > - * phy-handle | Y > > > - * ------------------------------- > > > - * fixed-link | N > > > - * ------------------------------- > > > - * snps,dwmac-mdio | > > > - * even if | Y > > > - * fixed-link | > > > - * ------------------------------- > > > + * stmmac_of_get_mdio() - Gets the MDIO bus from the devicetree > > > + * @np: devicetree node > > > * > > > - * It returns 0 in case of success otherwise -ENODEV. > > > + * The MDIO bus will be searched for in the following ways: > > > + * 1. The compatible is "snps,dwc-qos-ethernet-4.10" && a "mdio" named > > > + * child node exists > > > + * 2. A child node with the "snps,dwmac-mdio" compatible is present > > > + * > > > + * Return: The MDIO node if present otherwise NULL > > > */ > > > -static int stmmac_dt_phy(struct plat_stmmacenet_data *plat, > > > - struct device_node *np, struct device *dev) > > > +static struct device_node *stmmac_of_get_mdio(struct device_node *np) > > > { > > > - bool mdio = !of_phy_is_fixed_link(np); > > > static const struct of_device_id need_mdio_ids[] = { > > > { .compatible = "snps,dwc-qos-ethernet-4.10" }, > > > {}, > > > }; > > > + struct device_node *mdio_node = NULL; > > > > > > if (of_match_node(need_mdio_ids, np)) { > > > - plat->mdio_node = of_get_child_by_name(np, "mdio"); > > > + mdio_node = of_get_child_by_name(np, "mdio"); > > > } else { > > > /** > > > * 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, > > > + for_each_child_of_node(np, mdio_node) { > > > + if (of_device_is_compatible(mdio_node, > > > "snps,dwmac-mdio")) > > > break; > > > } > > > } > > > > > > - if (plat->mdio_node) { > > > - dev_dbg(dev, "Found MDIO subnode\n"); > > > - mdio = true; > > > - } > > > - > > > - if (mdio) { > > > - 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; > > > + return mdio_node; > > > } > > > > > > /** > > > @@ -417,6 +387,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > > > struct device_node *np = pdev->dev.of_node; > > > struct plat_stmmacenet_data *plat; > > > struct stmmac_dma_cfg *dma_cfg; > > > + bool legacy_mdio; > > > int phy_mode; > > > void *ret; > > > int rc; > > > @@ -471,10 +442,28 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > > > if (of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr) == 0) > > > dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n"); > > > > > > - /* To Configure PHY by using all device-tree supported properties */ > > > - rc = stmmac_dt_phy(plat, np, &pdev->dev); > > > - if (rc) > > > - return ERR_PTR(rc); > > > + plat->mdio_node = stmmac_of_get_mdio(np); > > > + if (plat->mdio_node) > > > + dev_dbg(&pdev->dev, "Found MDIO subnode\n"); > > > + > > > > > + /* Legacy devicetrees allowed for no MDIO bus description and expect > > > + * the bus to be scanned for devices. If there's no phy or fixed-link > > > + * described assume this is the case since there must be something > > > + * connected to the MAC. > > > + */ > > > + legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node; > > > + if (legacy_mdio) > > > + dev_info(&pdev->dev, "Deprecated MDIO bus assumption used\n"); > > > + > > > + if (plat->mdio_node || legacy_mdio) { > > > + plat->mdio_bus_data = devm_kzalloc(&pdev->dev, > > > + sizeof(struct stmmac_mdio_bus_data), > > > + GFP_KERNEL); > > > + if (!plat->mdio_bus_data) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + plat->mdio_bus_data->needs_reset = true; > > > + } > > > > Why did you decide to move this out of the dedicated function? > > stmmac_probe_config_dt() is already overwhelmed with various > > non-coherent actions. The method has already got to being too long to > > follow the kernel coding style limit (I have got a not submitted yet > > cleanup patchset which step-by-step fixes that). Could you please get > > the chunk above back to the respective function and, for instance, > > just change it's name to something like stmmac_mdio_setup()? (I prefer > > having "_setup" suffix because some of the locally defined static > > methods already use it: stmmac_axi_setup(), stmmac_mtl_setup().) > > Sure, I can put it back in. I'll probably keep stmmac_of_get_mdio() > (which is named in the style I see in the current file -- although as > you highlight that shouldn't be taken as the best example of clean), > and have stmmac_mdio_setup() call that and and mimic the inputs/outputs > of the current function (moving the rest of the added code from > stmmac_probe_config_dt() back into that function). > > Thanks for the feedback, let me know if you still think that abstraction > isn't ideal (or wait till I post it :P) and I'll go with exactly as you > said. Sounds good. Thanks for taking my comment into account. > I'm not _too_ opinionated on it, but thought stmmac_dt_phy() > didn't explain much and stmmac_of_get_mdio() was self-explanatory enough > that it helped readability. You were right. stmmac_dt_phy() hasn't been explicitly doing PHY-related things since commit 74371272f97f ("net: stmmac: Convert to phylink and remove phylib logic"), but has been left with the MDIO stuff only. The function name wasn't that well chosen either. It didn't indicate what the function actually did, like "init", "get", "setup", etc. From that perspective your naming is much better - short and self-explanatory indeed. -Serge(y) > > > > > -Serge(y) > > > > > > > > of_property_read_u32(np, "tx-fifo-depth", &plat->tx_fifo_size); > > > > > > > > > --- > > > base-commit: fd8a79b63710acb84321be3ce74be23c876873fb > > > change-id: 20231127-stmmac-no-mdio-node-1db9da8a25d9 > > > > > > Best regards, > > > -- > > > Andrew Halaney <ahalaney@redhat.com> > > > > > > > > >
On Wed, Dec 06, 2023 at 05:46:09PM -0600, Andrew Halaney wrote: > The stmmac_dt_phy() function, which parses the devicetree node of the > MAC and ultimately causes MDIO bus allocation, misinterprets what > fixed-link means in relation to the MAC's MDIO bus. This results in > a MDIO bus being created in situations it need not be. Please extend that with something like.... This is bad, because .... Most 'clean' driver unconditionally create the MDIO bus. But stmmac is not that clean, and has to keep backwards compatibility to some old usage. I'm just wondering what this patch actually brings us, and is it worth it. Is it fixing a real bug, or just an optimisation? Andrew
On Thu, Dec 07, 2023 at 10:30:23PM +0100, Andrew Lunn wrote: > On Wed, Dec 06, 2023 at 05:46:09PM -0600, Andrew Halaney wrote: > > The stmmac_dt_phy() function, which parses the devicetree node of the > > MAC and ultimately causes MDIO bus allocation, misinterprets what > > fixed-link means in relation to the MAC's MDIO bus. This results in > > a MDIO bus being created in situations it need not be. > > Please extend that with something like.... > > This is bad, because .... > > Most 'clean' driver unconditionally create the MDIO bus. But stmmac is > not that clean, and has to keep backwards compatibility to some old > usage. I'm just wondering what this patch actually brings us, and is > it worth it. Is it fixing a real bug, or just an optimisation? > > Andrew > It is an optimization for speeding up time to link up. I already sent out v3 moments before this arrived, I'll be sure to capture that more clearly in v4 (and wait a little longer before respinning it). I'm trying to optimize this device configuration (as shown in the commit): """ Here's[1] an example where there is no MDIO bus or fixed-link for the ethernet1 MAC, so no MDIO bus should be created since ethernet0 is the MDIO master for ethernet1's phy: ðernet0 { phy-mode = "sgmii"; phy-handle = <&sgmii_phy0>; mdio { compatible = "snps,dwmac-mdio"; sgmii_phy0: phy@8 { compatible = "ethernet-phy-id0141.0dd4"; reg = <0x8>; device_type = "ethernet-phy"; }; sgmii_phy1: phy@a { compatible = "ethernet-phy-id0141.0dd4"; reg = <0xa>; device_type = "ethernet-phy"; }; }; }; ðernet1 { phy-mode = "sgmii"; phy-handle = <&sgmii_phy1>; }; """ I'm seeing that ethernet1 scans the whole MDIO bus created for it, and finds nothing, wasting time in the process. Since there's no mdio bus described (since it's vacant) you get something like this call order: stmmac_mdio_register() of_mdiobus_register(new_bus, mdio_node) // mdio_node is NULL in this case __of_mdiobus_register(mdio, np, owner) // this doesn't set phy_mask since np == NULL __mdiobus_register(mdio, owner) mdiobus_scan_bus_c22(bus) mdiobus_scan_c22() // Called PHY_MAX_ADDR times, probing an empty bus Which is causing a good bit of delay in the time to link up, each attempted probe is taking about 5 ms and that's just benchmarking the get_phy_c22_id() calls, if you look at the whole loop it's greater (but I am unsure if that's just scheduling contention or something else going on, once I realized we were doing this work unnecessarily I decided to try and remove it) I know you said the standard is to make the MDIO bus unconditionally, but to me that feels like a waste, and describing say an empty MDIO bus (which would set the phy_mask for us eventually and not scan a bunch of phys, untested but I think that would work) seems like a bad description in the devicetree (I could definitely see describing an empty MDIO bus getting NACKed, but that's a guess). Please let me know if you disagree with that logic and have some alternative solutions for optimizing. In either case I think this code needs some cleaning so I'll carry this through. It also seems that unconditional creation of the MDIO bus is something that's biting some stmmac variants that lack an MDIO controller based on Serge's latest comments on v3: https://lore.kernel.org/netdev/jz6ot44fjkbmwcezi3fkgqd54nurglblbemrchfgxgq6udlhqz@ntepnnzzelta/ Thanks, Andrew
> I know you said the standard is to make the MDIO bus unconditionally, but > to me that feels like a waste, and describing say an empty MDIO bus > (which would set the phy_mask for us eventually and not scan a > bunch of phys, untested but I think that would work) seems like a bad > description in the devicetree (I could definitely see describing an > empty MDIO bus getting NACKed, but that's a guess). DT describes the hardware. The MDIO bus master exists. So typically the SoC .dtsi file would include it in the Ethernet node. At the SoC level it is empty, unless there is an integrated PHY in the SoC. The board .dts file then adds any PHYs to the node which the board actually has. So i doubt adding an empty MDIO node to the SoC .dtsi file will get NACKed, it correctly describes the hardware. Andrew
On Fri, Dec 08, 2023 at 02:14:41PM +0100, Andrew Lunn wrote: > > I know you said the standard is to make the MDIO bus unconditionally, but > > to me that feels like a waste, and describing say an empty MDIO bus > > (which would set the phy_mask for us eventually and not scan a > > bunch of phys, untested but I think that would work) seems like a bad > > description in the devicetree (I could definitely see describing an > > empty MDIO bus getting NACKed, but that's a guess). > > DT describes the hardware. The MDIO bus master exists. So typically > the SoC .dtsi file would include it in the Ethernet node. At the SoC > level it is empty, unless there is an integrated PHY in the SoC. The > board .dts file then adds any PHYs to the node which the board > actually has. > > So i doubt adding an empty MDIO node to the SoC .dtsi file will get > NACKed, it correctly describes the hardware. Agreed, thanks for helping me consider all the cases. In my particular example it would make sense to have SoC dtsi describe the mdio bus, leave it disabled, and boards enable it and describe components as necessary. So you have let's say these 8 abbreviated cases: Case 1 (MDIO bus used with phy on bus connected to MAC): ethernet { status = "okay"; phy-handle = <&phy0>; phy-mode = "rgmii"; mdio { status = "okay"; phy0: phy@0 { }; }; }; Case 2 (MDIO bus used but MAC's connect fixed-link): ethernet { status = "okay"; phy-mode = "rgmii"; fixed-link { }; mdio { status = "okay"; switch/unrelated-phy { }; }; }; Case 3 (MDIO bus used but MAC's connected to another bus's phy): ethernet { status = "okay"; phy-handle = <&phy1>; phy-mode = "rgmii"; mdio { status = "okay"; switch/unrelated-phy { }; }; }; Case 4 (MDIO bus disabled/unused, connected fixed-link): ethernet { status = "okay"; phy-mode = "rgmii"; fixed-link { }; mdio { status = "disabled"; }; }; Case 5 (MDIO bus disabled/unused, connected to another bus's phy): ethernet { status = "okay"; phy-handle = <&phy1>; phy-mode = "rgmii"; mdio { status = "disabled"; }; }; Case 6 (MDIO bus not described, connected fixed-link): ethernet { status = "okay"; phy-mode = "rgmii"; fixed-link { }; }; Case 7 (MDIO bus not described, connected to a different bus's phy): ethernet { status = "okay"; phy-handle = <&phy1>; phy-mode = "rgmii"; }; Case 8 (MDIO bus not described, but phy on MDIO bus is connected to MAC, legacy description[2] in my commit message): ethernet { status = "okay"; }; If we look at the logic in stmmac today about how to handle the MDIO bus, you've got basically: if !fixed-link || mdio_node_present() of_mdiobus_register(np) Applying current stmmac logic to our cases... Case 1 (MDIO bus used with phy on bus connected to MAC): MDIO bus made, no unnecessary scanning Case 2 (MDIO bus used but MAC's fixed-link): MDIO bus made, no unnecessary scanning Case 3 (MDIO bus used but MAC's connected to another bus's phy): MDIO bus made, no unnecessary scanning Case 4 (MDIO bus disabled/unused, connected fixed-link): MDIO bus attempted to be made, fails -ENODEV due to disabled and stmmac returns -ENODEV from probe too Case 5 (MDIO bus disabled/unused, connected to another bus's phy): MDIO bus attempted to be made, fails -ENODEV due to disabled and stmmac returns -ENODEV from probe too Case 6 (MDIO bus not described, connected fixed-link): MDIO bus not created Case 7 (MDIO bus not described, connected to a different bus's phy): MDIO bus created, but the whole bus is scanned Case 8 (MDIO bus not described, but phy on MDIO bus is connected to MAC, legacy description[2] in my commit message): MDIO bus created, the whole bus is scanned and the undescribed but necessary phy is found The things I note of interest are cases 4, 5, 7, 8. Cases 4/5 are a bug in stmmac IMO, which breaks the devicetree description you mentioned as ideal in my case. Case 7 is the one I'm currently working with, and the devicetree can be updated to match case 5, but right now case 7 makes a bus and scans it needlessly which isn't great. It _sounds_ like to me Serge knows of stmmac variants that also *do not* have an MDIO controller, so they'd fall in this case too and really shouldn't create a bus. Case 8 is the legacy one that I wish didn't exist, but it does, and for that reason we should continue to make a bus and scan the whole thing if we can't figure out what the MAC's connected to. So in my opinion there's 3 changes I want to make based on all the use cases I could think of: 1. This patch, which improves the stmmac logic and stops making a bus for case 7 2. A new patch to gracefully handle cases 4/5, where currently if the MDIO bus is disabled in the devicetree, as it should be, stmmac handles -ENODEV from of_mdiobus_register() as a failure instead of gracefully continuing knowing this is fine and dandy. 3. Clean up the sa8775p dts to have the MDIO bus described in the SoC dtsi and left disabled instead of undescribed (a more accurate hardware description). Please let me know if you see any holes in my logic, hopefully the wall of text is useful in explaining how I got here. Thanks, Andrew
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index 1ffde555da47..7da461fe93f6 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c @@ -296,69 +296,39 @@ static int stmmac_mtl_setup(struct platform_device *pdev, } /** - * stmmac_dt_phy - parse device-tree driver parameters to allocate PHY resources - * @plat: driver data platform structure - * @np: device tree node - * @dev: device pointer - * Description: - * The mdio bus will be allocated in case of a phy transceiver is on board; - * it will be NULL if the fixed-link is configured. - * If there is the "snps,dwmac-mdio" sub-node the mdio will be allocated - * in any case (for DSA, mdio must be registered even if fixed-link). - * The table below sums the supported configurations: - * ------------------------------- - * snps,phy-addr | Y - * ------------------------------- - * phy-handle | Y - * ------------------------------- - * fixed-link | N - * ------------------------------- - * snps,dwmac-mdio | - * even if | Y - * fixed-link | - * ------------------------------- + * stmmac_of_get_mdio() - Gets the MDIO bus from the devicetree + * @np: devicetree node * - * It returns 0 in case of success otherwise -ENODEV. + * The MDIO bus will be searched for in the following ways: + * 1. The compatible is "snps,dwc-qos-ethernet-4.10" && a "mdio" named + * child node exists + * 2. A child node with the "snps,dwmac-mdio" compatible is present + * + * Return: The MDIO node if present otherwise NULL */ -static int stmmac_dt_phy(struct plat_stmmacenet_data *plat, - struct device_node *np, struct device *dev) +static struct device_node *stmmac_of_get_mdio(struct device_node *np) { - bool mdio = !of_phy_is_fixed_link(np); static const struct of_device_id need_mdio_ids[] = { { .compatible = "snps,dwc-qos-ethernet-4.10" }, {}, }; + struct device_node *mdio_node = NULL; if (of_match_node(need_mdio_ids, np)) { - plat->mdio_node = of_get_child_by_name(np, "mdio"); + mdio_node = of_get_child_by_name(np, "mdio"); } else { /** * 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, + for_each_child_of_node(np, mdio_node) { + if (of_device_is_compatible(mdio_node, "snps,dwmac-mdio")) break; } } - if (plat->mdio_node) { - dev_dbg(dev, "Found MDIO subnode\n"); - mdio = true; - } - - if (mdio) { - 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; + return mdio_node; } /** @@ -417,6 +387,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) struct device_node *np = pdev->dev.of_node; struct plat_stmmacenet_data *plat; struct stmmac_dma_cfg *dma_cfg; + bool legacy_mdio; int phy_mode; void *ret; int rc; @@ -471,10 +442,28 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) if (of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr) == 0) dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n"); - /* To Configure PHY by using all device-tree supported properties */ - rc = stmmac_dt_phy(plat, np, &pdev->dev); - if (rc) - return ERR_PTR(rc); + plat->mdio_node = stmmac_of_get_mdio(np); + if (plat->mdio_node) + dev_dbg(&pdev->dev, "Found MDIO subnode\n"); + + /* Legacy devicetrees allowed for no MDIO bus description and expect + * the bus to be scanned for devices. If there's no phy or fixed-link + * described assume this is the case since there must be something + * connected to the MAC. + */ + legacy_mdio = !of_phy_is_fixed_link(np) && !plat->phy_node; + if (legacy_mdio) + dev_info(&pdev->dev, "Deprecated MDIO bus assumption used\n"); + + if (plat->mdio_node || legacy_mdio) { + plat->mdio_bus_data = devm_kzalloc(&pdev->dev, + sizeof(struct stmmac_mdio_bus_data), + GFP_KERNEL); + if (!plat->mdio_bus_data) + return ERR_PTR(-ENOMEM); + + plat->mdio_bus_data->needs_reset = true; + } of_property_read_u32(np, "tx-fifo-depth", &plat->tx_fifo_size);
The stmmac_dt_phy() function, which parses the devicetree node of the MAC and ultimately causes MDIO bus allocation, misinterprets what fixed-link means in relation to the MAC's MDIO bus. This results in a MDIO bus being created in situations it need not be. Currently a MDIO bus is created if the description is either: 1. Not fixed-link 2. fixed-link but contains a MDIO bus as well The "1" case above isn't always accurate. If there's a phy-handle, it could be referencing a phy on another MDIO controller's bus[1]. In this case currently the MAC will make a MDIO bus and scan it all anyways unnecessarily. There's also a lot of upstream devicetrees[2] that expect a MDIO bus to be created and scanned for a phy. This case can also be inferred from the platform description by not having a phy-handle && not being fixed-link. This hits case "1" in the current driver's logic. Let's improve the logic to create a MDIO bus if either: - Devicetree contains a MDIO bus - !fixed-link && !phy-handle (legacy handling) Below upstream devicetree snippets can be found that explain some of the cases above more concretely. Here's[0] a devicetree example where the MAC is both fixed-link and driving a switch on MDIO (case "2" above). This needs a MDIO bus to be created: &fec1 { phy-mode = "rmii"; fixed-link { speed = <100>; full-duplex; }; mdio1: mdio { switch0: switch0@0 { compatible = "marvell,mv88e6190"; pinctrl-0 = <&pinctrl_gpio_switch0>; }; }; }; Here's[1] an example where there is no MDIO bus or fixed-link for the ethernet1 MAC, so no MDIO bus should be created since ethernet0 is the MDIO master for ethernet1's phy: ðernet0 { phy-mode = "sgmii"; phy-handle = <&sgmii_phy0>; mdio { compatible = "snps,dwmac-mdio"; sgmii_phy0: phy@8 { compatible = "ethernet-phy-id0141.0dd4"; reg = <0x8>; device_type = "ethernet-phy"; }; sgmii_phy1: phy@a { compatible = "ethernet-phy-id0141.0dd4"; reg = <0xa>; device_type = "ethernet-phy"; }; }; }; ðernet1 { phy-mode = "sgmii"; phy-handle = <&sgmii_phy1>; }; Finally there's descriptions like this[2] which don't describe the MDIO bus but expect it to be created and the whole address space scanned for a phy since there's no phy-handle or fixed-link described: &gmac { phy-supply = <&vcc_lan>; phy-mode = "rmii"; snps,reset-gpio = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>; snps,reset-active-low; snps,reset-delays-us = <0 10000 1000000>; }; [0] https://elixir.bootlin.com/linux/v6.5-rc5/source/arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts [1] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/qcom/sa8775p-ride.dts [2] https://elixir.bootlin.com/linux/v6.6-rc5/source/arch/arm64/boot/dts/rockchip/rk3368-r88.dts#L164 Co-developed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Signed-off-by: Andrew Halaney <ahalaney@redhat.com> --- .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 85 ++++++++++------------ 1 file changed, 37 insertions(+), 48 deletions(-) --- base-commit: fd8a79b63710acb84321be3ce74be23c876873fb change-id: 20231127-stmmac-no-mdio-node-1db9da8a25d9 Best regards,