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 <[email protected]>
Signed-off-by: Bartosz Golaszewski <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
.../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;
+ }
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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> Signed-off-by: Andrew Halaney <[email protected]>
> ---
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 <[email protected]>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> Signed-off-by: Andrew Halaney <[email protected]>
> ---
> .../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 <[email protected]>
>
>
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 <[email protected]>
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > Signed-off-by: Andrew Halaney <[email protected]>
> > ---
> > .../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 <[email protected]>
> >
> >
>
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 <[email protected]>
> > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > > Signed-off-by: Andrew Halaney <[email protected]>
> > > ---
> > > .../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 <[email protected]>
> > >
> > >
> >
>
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