[PATCH net-next v2] net: stmmac: don't create a MDIO bus if unnecessary
Serge Semin
fancer.lancer at gmail.com
Thu Dec 7 03:56:25 PST 2023
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 at 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 at 8 {
> compatible = "ethernet-phy-id0141.0dd4";
> reg = <0x8>;
> device_type = "ethernet-phy";
> };
>
> sgmii_phy1: phy at 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 at linaro.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski at linaro.org>
> Signed-off-by: Andrew Halaney <ahalaney at 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 at redhat.com>
>
>
More information about the linux-arm-kernel
mailing list