[PATCH v4 0/2] net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl

[PATCH v4 0/2] net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl

[Maxime Ripard]

Hi,

This series is based on:
https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/
https://lore.kernel.org/u-boot/20230614222853.574427-1-dannenberg@ti.com/
https://lore.kernel.org/u-boot/20230722193151.117345-1-rogerq@kernel.org/

It fixes the issue of Linux booting from the DT embedded by U-boot. The
main issue there is that U-Boot doesn't handle the MDIO child node that
might have resources attached to it.

Thus, any pinctrl configuration that could be attached to the MDIO
child node is effectively ignored. Unfortunately, starting with 6.5-rc1,
Linux does just that.

This was solved by duplicating the pinctrl configuration onto the MAC
device node. Unfortunately, this doesn't work for Linux since now it has
two devices competing for the same pins.

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
Changes in v4:
- Squashed fixup patches
- Link to v3: https://lore.kernel.org/r/20230724-ti-mdio-pinmux-v3-0-fc8359b0014b@kernel.org

Changes in v3:
- Rebase on top of latest Roger series
- Link to v2: https://lore.kernel.org/r/20230721-ti-mdio-pinmux-v2-0-4bb443e09ac0@kernel.org

Changes in v2:
- Drop the pinctrl API change in favour of a dummy driver
- Link to v1: https://lore.kernel.org/r/20230720-ti-mdio-pinmux-v1-0-0bd3bd1cf759@kernel.org

---
Maxime Ripard (2):
      net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node
      fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

 arch/arm/dts/k3-am625-sk-u-boot.dtsi |  1 -
 drivers/net/ti/Kconfig               |  1 +
 drivers/net/ti/am65-cpsw-nuss.c      | 60 ++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)
---
base-commit: db7126c87c44c45c448c68925d366510f7a09b11
change-id: 20230720-ti-mdio-pinmux-a12525dba973

Best regards,
-- 
Maxime Ripard <mripard@kernel.org>

[PATCH v4 1/2] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Maxime Ripard]

The binding represents the MDIO controller as a child device tree
node of the MAC device tree node.

The U-Boot driver mostly ignores that child device tree node and just
hardcodes the resources it uses to support both the MAC and MDIO in a
single driver.

However, some resources like pinctrl muxing states are thus ignored.
This has been a problem with some device trees that will put some
pinctrl states on the MDIO device tree node, like the SK-AM62 Device
Tree does.

Let's rework the driver a bit to create a dummy MDIO driver that we will
then get during our initialization to force the core to select the right
muxing.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/net/ti/Kconfig          |  1 +
 drivers/net/ti/am65-cpsw-nuss.c | 60 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig
index d9f1c019a885..02660e4fbb44 100644
--- a/drivers/net/ti/Kconfig
+++ b/drivers/net/ti/Kconfig
@@ -41,6 +41,7 @@ endchoice
 config TI_AM65_CPSW_NUSS
 	bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver"
 	depends on ARCH_K3
+	imply DM_MDIO
 	imply MISC_INIT_R
 	imply MISC
 	imply SYSCON
diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index ce52106e5238..51a8167d14a9 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -15,6 +15,7 @@
 #include <dm.h>
 #include <dm/device_compat.h>
 #include <dm/lists.h>
+#include <dm/pinctrl.h>
 #include <dma-uclass.h>
 #include <dm/of_access.h>
 #include <miiphy.h>
@@ -605,14 +606,62 @@ static const struct soc_attr k3_mdio_soc_data[] = {
 	{ /* sentinel */ },
 };
 
+static ofnode am65_cpsw_find_mdio(ofnode parent)
+{
+	ofnode node;
+
+	ofnode_for_each_subnode(node, parent)
+		if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))
+			return node;
+
+	return ofnode_null();
+}
+
+static int am65_cpsw_mdio_setup(struct udevice *dev)
+{
+	struct am65_cpsw_priv *priv = dev_get_priv(dev);
+	struct am65_cpsw_common	*cpsw_common = priv->cpsw_common;
+	struct udevice *mdio_dev;
+	ofnode mdio;
+	int ret;
+
+	mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
+	if (!ofnode_valid(mdio))
+		return 0;
+
+	/*
+	 * The MDIO controller is represented in the DT binding by a
+	 * subnode of the MAC controller.
+	 *
+	 * We don't have a DM driver for the MDIO device yet, and thus any
+	 * pinctrl setting on its node will be ignored.
+	 *
+	 * However, we do need to make sure the pins states tied to the
+	 * MDIO node are configured properly. Fortunately, the core DM
+	 * does that for use when we get a device, so we can work around
+	 * that whole issue by just requesting a dummy MDIO driver to
+	 * probe, and our pins will get muxed.
+	 */
+	ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdio_dev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int am65_cpsw_mdio_init(struct udevice *dev)
 {
 	struct am65_cpsw_priv *priv = dev_get_priv(dev);
 	struct am65_cpsw_common	*cpsw_common = priv->cpsw_common;
+	int ret;
 
 	if (!priv->has_phy || cpsw_common->bus)
 		return 0;
 
+	ret = am65_cpsw_mdio_setup(dev);
+	if (ret)
+		return ret;
+
 	cpsw_common->bus = cpsw_mdio_init(dev->name,
 					  cpsw_common->mdio_base,
 					  cpsw_common->bus_freq,
@@ -868,3 +917,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = {
 	.plat_auto	= sizeof(struct eth_pdata),
 	.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE,
 };
+
+static const struct udevice_id am65_cpsw_mdio_ids[] = {
+	{ .compatible = "ti,cpsw-mdio" },
+	{ }
+};
+
+U_BOOT_DRIVER(am65_cpsw_mdio) = {
+	.name		= "am65_cpsw_mdio",
+	.id		= UCLASS_MDIO,
+	.of_match	= am65_cpsw_mdio_ids,
+};

-- 
2.41.0

[PATCH v4 2/2] fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

[Maxime Ripard]

The MDIO pinctrl nodes can't be duplicated between the child and device,
because if we ever boot Linux with our DT it will try to attach that
pinctrl configuration to both the MAC and MDIO devices, which will
result in failure to probe.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 arch/arm/dts/k3-am625-sk-u-boot.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
index a3f207baa304..c1685bc9ca39 100644
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
+++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
@@ -120,7 +120,6 @@
 
 &cpsw3g {
 	bootph-pre-ram;
-	pinctrl-0 = <&main_mdio1_pins_default &main_rgmii1_pins_default>;
 };
 
 &cpsw_port1 {

-- 
2.41.0

Re: [PATCH v4 1/2] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Siddharth Vadapalli]

Hello Maxime,

Thank you for the patch.

On 24/07/23 19:27, Maxime Ripard wrote:
> The binding represents the MDIO controller as a child device tree
> node of the MAC device tree node.
> 
> The U-Boot driver mostly ignores that child device tree node and just
> hardcodes the resources it uses to support both the MAC and MDIO in a
> single driver.
> 
> However, some resources like pinctrl muxing states are thus ignored.
> This has been a problem with some device trees that will put some
> pinctrl states on the MDIO device tree node, like the SK-AM62 Device
> Tree does.
> 
> Let's rework the driver a bit to create a dummy MDIO driver that we will
> then get during our initialization to force the core to select the right
> muxing.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

LGTM.

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>

> ---
>  drivers/net/ti/Kconfig          |  1 +
>  drivers/net/ti/am65-cpsw-nuss.c | 60 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig
> index d9f1c019a885..02660e4fbb44 100644
> --- a/drivers/net/ti/Kconfig
> +++ b/drivers/net/ti/Kconfig
> @@ -41,6 +41,7 @@ endchoice
>  config TI_AM65_CPSW_NUSS
>  	bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver"
>  	depends on ARCH_K3
> +	imply DM_MDIO
>  	imply MISC_INIT_R
>  	imply MISC
>  	imply SYSCON
> diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
> index ce52106e5238..51a8167d14a9 100644
> --- a/drivers/net/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ti/am65-cpsw-nuss.c
> @@ -15,6 +15,7 @@
>  #include <dm.h>
>  #include <dm/device_compat.h>
>  #include <dm/lists.h>
> +#include <dm/pinctrl.h>
>  #include <dma-uclass.h>
>  #include <dm/of_access.h>
>  #include <miiphy.h>
> @@ -605,14 +606,62 @@ static const struct soc_attr k3_mdio_soc_data[] = {
>  	{ /* sentinel */ },
>  };
>  
> +static ofnode am65_cpsw_find_mdio(ofnode parent)
> +{
> +	ofnode node;
> +
> +	ofnode_for_each_subnode(node, parent)
> +		if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))
> +			return node;
> +
> +	return ofnode_null();
> +}
> +
> +static int am65_cpsw_mdio_setup(struct udevice *dev)
> +{
> +	struct am65_cpsw_priv *priv = dev_get_priv(dev);
> +	struct am65_cpsw_common	*cpsw_common = priv->cpsw_common;
> +	struct udevice *mdio_dev;
> +	ofnode mdio;
> +	int ret;
> +
> +	mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
> +	if (!ofnode_valid(mdio))
> +		return 0;
> +
> +	/*
> +	 * The MDIO controller is represented in the DT binding by a
> +	 * subnode of the MAC controller.
> +	 *
> +	 * We don't have a DM driver for the MDIO device yet, and thus any
> +	 * pinctrl setting on its node will be ignored.
> +	 *
> +	 * However, we do need to make sure the pins states tied to the
> +	 * MDIO node are configured properly. Fortunately, the core DM
> +	 * does that for use when we get a device, so we can work around
> +	 * that whole issue by just requesting a dummy MDIO driver to
> +	 * probe, and our pins will get muxed.
> +	 */
> +	ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int am65_cpsw_mdio_init(struct udevice *dev)
>  {
>  	struct am65_cpsw_priv *priv = dev_get_priv(dev);
>  	struct am65_cpsw_common	*cpsw_common = priv->cpsw_common;
> +	int ret;
>  
>  	if (!priv->has_phy || cpsw_common->bus)
>  		return 0;
>  
> +	ret = am65_cpsw_mdio_setup(dev);
> +	if (ret)
> +		return ret;
> +
>  	cpsw_common->bus = cpsw_mdio_init(dev->name,
>  					  cpsw_common->mdio_base,
>  					  cpsw_common->bus_freq,
> @@ -868,3 +917,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = {
>  	.plat_auto	= sizeof(struct eth_pdata),
>  	.flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE,
>  };
> +
> +static const struct udevice_id am65_cpsw_mdio_ids[] = {
> +	{ .compatible = "ti,cpsw-mdio" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(am65_cpsw_mdio) = {
> +	.name		= "am65_cpsw_mdio",
> +	.id		= UCLASS_MDIO,
> +	.of_match	= am65_cpsw_mdio_ids,
> +};
> 

-- 
Regards,
Siddharth.

Re: [PATCH v4 1/2] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Roger Quadros]

On 24/07/2023 16:57, Maxime Ripard wrote:
> The binding represents the MDIO controller as a child device tree
> node of the MAC device tree node.
> 
> The U-Boot driver mostly ignores that child device tree node and just
> hardcodes the resources it uses to support both the MAC and MDIO in a
> single driver.
> 
> However, some resources like pinctrl muxing states are thus ignored.
> This has been a problem with some device trees that will put some
> pinctrl states on the MDIO device tree node, like the SK-AM62 Device
> Tree does.
> 
> Let's rework the driver a bit to create a dummy MDIO driver that we will
> then get during our initialization to force the core to select the right
> muxing.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Acked-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH v4 1/2] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Nishanth Menon]

On 15:57-20230724, Maxime Ripard wrote:
> The binding represents the MDIO controller as a child device tree
> node of the MAC device tree node.
> 
> The U-Boot driver mostly ignores that child device tree node and just
> hardcodes the resources it uses to support both the MAC and MDIO in a
> single driver.
> 
> However, some resources like pinctrl muxing states are thus ignored.
> This has been a problem with some device trees that will put some
> pinctrl states on the MDIO device tree node, like the SK-AM62 Device
> Tree does.
> 
> Let's rework the driver a bit to create a dummy MDIO driver that we will
> then get during our initialization to force the core to select the right
> muxing.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Acked-by: Nishanth Menon <nm@ti.com>

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v4 1/2] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Marcel Ziswiler]

Hi Maxime

Just a minor nitpick in the comments below.

On Mon, 2023-07-24 at 15:57 +0200, Maxime Ripard wrote:
> The binding represents the MDIO controller as a child device tree
> node of the MAC device tree node.
> 
> The U-Boot driver mostly ignores that child device tree node and just
> hardcodes the resources it uses to support both the MAC and MDIO in a
> single driver.
> 
> However, some resources like pinctrl muxing states are thus ignored.
> This has been a problem with some device trees that will put some
> pinctrl states on the MDIO device tree node, like the SK-AM62 Device
> Tree does.
> 
> Let's rework the driver a bit to create a dummy MDIO driver that we will
> then get during our initialization to force the core to select the right
> muxing.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/net/ti/Kconfig          |  1 +
>  drivers/net/ti/am65-cpsw-nuss.c | 60 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig
> index d9f1c019a885..02660e4fbb44 100644
> --- a/drivers/net/ti/Kconfig
> +++ b/drivers/net/ti/Kconfig
> @@ -41,6 +41,7 @@ endchoice
>  config TI_AM65_CPSW_NUSS
>         bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver"
>         depends on ARCH_K3
> +       imply DM_MDIO
>         imply MISC_INIT_R
>         imply MISC
>         imply SYSCON
> diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
> index ce52106e5238..51a8167d14a9 100644
> --- a/drivers/net/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ti/am65-cpsw-nuss.c
> @@ -15,6 +15,7 @@
>  #include <dm.h>
>  #include <dm/device_compat.h>
>  #include <dm/lists.h>
> +#include <dm/pinctrl.h>
>  #include <dma-uclass.h>
>  #include <dm/of_access.h>
>  #include <miiphy.h>
> @@ -605,14 +606,62 @@ static const struct soc_attr k3_mdio_soc_data[] = {
>         { /* sentinel */ },
>  };
>  
> +static ofnode am65_cpsw_find_mdio(ofnode parent)
> +{
> +       ofnode node;
> +
> +       ofnode_for_each_subnode(node, parent)
> +               if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))
> +                       return node;
> +
> +       return ofnode_null();
> +}
> +
> +static int am65_cpsw_mdio_setup(struct udevice *dev)
> +{
> +       struct am65_cpsw_priv *priv = dev_get_priv(dev);
> +       struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
> +       struct udevice *mdio_dev;
> +       ofnode mdio;
> +       int ret;
> +
> +       mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
> +       if (!ofnode_valid(mdio))
> +               return 0;
> +
> +       /*
> +        * The MDIO controller is represented in the DT binding by a
> +        * subnode of the MAC controller.
> +        *
> +        * We don't have a DM driver for the MDIO device yet, and thus any
> +        * pinctrl setting on its node will be ignored.
> +        *
> +        * However, we do need to make sure the pins states tied to the
> +        * MDIO node are configured properly. Fortunately, the core DM
> +        * does that for use when we get a device, so we can work around

Does that for us?

> +        * that whole issue by just requesting a dummy MDIO driver to
> +        * probe, and our pins will get muxed.
> +        */
> +       ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdio_dev);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
>  static int am65_cpsw_mdio_init(struct udevice *dev)
>  {
>         struct am65_cpsw_priv *priv = dev_get_priv(dev);
>         struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
> +       int ret;
>  
>         if (!priv->has_phy || cpsw_common->bus)
>                 return 0;
>  
> +       ret = am65_cpsw_mdio_setup(dev);
> +       if (ret)
> +               return ret;
> +
>         cpsw_common->bus = cpsw_mdio_init(dev->name,
>                                           cpsw_common->mdio_base,
>                                           cpsw_common->bus_freq,
> @@ -868,3 +917,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = {
>         .plat_auto      = sizeof(struct eth_pdata),
>         .flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE,
>  };
> +
> +static const struct udevice_id am65_cpsw_mdio_ids[] = {
> +       { .compatible = "ti,cpsw-mdio" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(am65_cpsw_mdio) = {
> +       .name           = "am65_cpsw_mdio",
> +       .id             = UCLASS_MDIO,
> +       .of_match       = am65_cpsw_mdio_ids,
> +};

However, so far I could not make this work for our use-case [1]. It just keeps crashing. Any ideas?

[snip]

Model: Toradex 0076 Verdin AM62 Quad 2GB WB IT V1.0A
Serial#: 15037380
Carrier: Toradex Dahlia V1.1A, Serial# 10763237
am65_cpsw_nuss ethernet@8000000: K3 CPSW: nuss_ver: 0x6BA01103 cpsw_ver: 0x6BA81
103 ale_ver: 0x00290105 Ports:2 mdio_freq:1000000
Setting variant to wifi
Net:
Warning: ethernet@8000000port@1 MAC addresses don't match:
Address in ROM is               1c:63:49:22:5f:f9
Address in environment is       00:14:2d:e5:73:c4
eth0: ethernet@8000000port@1 [PRIME]Could not get PHY for ethernet@8000000port@1
: addr 7
am65_cpsw_nuss_port ethernet@8000000port@2: phy_connect() failed

Hit any key to stop autoboot:  0

Verdin AM62 # dhcp
ti-udma dma-controller@485c0000: k3_dmaring Ring probed rings:150, sci-dev-id:30
ti-udma dma-controller@485c0000: dma-ring-reset-quirk: disabled
am65_cpsw_nuss_port ethernet@8000000port@1: K3 CPSW: rflow_id_base: 19
ethernet@8000000port@1 Waiting for PHY auto negotiation to complete......... TIMEOUT !
am65_cpsw_nuss_port ethernet@8000000port@1: phy_startup failed
am65_cpsw_nuss_port ethernet@8000000port@1: am65_cpsw_start end error
"Synchronous Abort" handler, esr 0x96000010, far 0x8000f04
elr: 00000000808503ac lr : 000000008084ce1c (reloc)
elr: 000000009bf533ac lr : 000000009bf4fe1c
x0 : 0000000008000f00 x1 : 0000000000000007
x2 : 00000000ffffffff x3 : 0000000000000002
x4 : 000000009bf53388 x5 : 0000000000011d28
x6 : 0000000000007578 x7 : 0000000099f10570
x8 : 0000000000000002 x9 : 0000000000000007
x10: 0000000000000002 x11: 0000000000007578
x12: 0000000099eeea98 x13: 0000000099eef030
x14: 0000000000000000 x15: 0000000099eee834
x16: 000000009bf5d040 x17: 0000000000000000
x18: 0000000099f00d70 x19: 0000000099eeead4
x20: 0000000099f10590 x21: 0000000000000007
x22: 00000000ffffffff x23: 0000000000000001
x24: 0000000000000080 x25: 0000000000000000
x26: 00000000ffffffff x27: 000000001fffffff
x28: 0000000000000000 x29: 0000000099eeea30

Code: 2a0103e9 2a0303e8 910003fd f94000e0 (b9400400)
Resetting CPU ...

resetting ... 

[1] https://lore.kernel.org/all/20230715074050.941051-1-marcel@ziswiler.com

Cheers

Marcel

Re: [PATCH v4 1/2] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[mripard]

[-- Attachment #1: Type: text/plain, Size: 7476 bytes --]

On Fri, Jul 28, 2023 at 01:34:38PM +0000, Marcel Ziswiler wrote:
> Hi Maxime
> 
> Just a minor nitpick in the comments below.
> 
> On Mon, 2023-07-24 at 15:57 +0200, Maxime Ripard wrote:
> > The binding represents the MDIO controller as a child device tree
> > node of the MAC device tree node.
> > 
> > The U-Boot driver mostly ignores that child device tree node and just
> > hardcodes the resources it uses to support both the MAC and MDIO in a
> > single driver.
> > 
> > However, some resources like pinctrl muxing states are thus ignored.
> > This has been a problem with some device trees that will put some
> > pinctrl states on the MDIO device tree node, like the SK-AM62 Device
> > Tree does.
> > 
> > Let's rework the driver a bit to create a dummy MDIO driver that we will
> > then get during our initialization to force the core to select the right
> > muxing.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/net/ti/Kconfig          |  1 +
> >  drivers/net/ti/am65-cpsw-nuss.c | 60 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 61 insertions(+)
> > 
> > diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig
> > index d9f1c019a885..02660e4fbb44 100644
> > --- a/drivers/net/ti/Kconfig
> > +++ b/drivers/net/ti/Kconfig
> > @@ -41,6 +41,7 @@ endchoice
> >  config TI_AM65_CPSW_NUSS
> >         bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver"
> >         depends on ARCH_K3
> > +       imply DM_MDIO
> >         imply MISC_INIT_R
> >         imply MISC
> >         imply SYSCON
> > diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
> > index ce52106e5238..51a8167d14a9 100644
> > --- a/drivers/net/ti/am65-cpsw-nuss.c
> > +++ b/drivers/net/ti/am65-cpsw-nuss.c
> > @@ -15,6 +15,7 @@
> >  #include <dm.h>
> >  #include <dm/device_compat.h>
> >  #include <dm/lists.h>
> > +#include <dm/pinctrl.h>
> >  #include <dma-uclass.h>
> >  #include <dm/of_access.h>
> >  #include <miiphy.h>
> > @@ -605,14 +606,62 @@ static const struct soc_attr k3_mdio_soc_data[] = {
> >         { /* sentinel */ },
> >  };
> >  
> > +static ofnode am65_cpsw_find_mdio(ofnode parent)
> > +{
> > +       ofnode node;
> > +
> > +       ofnode_for_each_subnode(node, parent)
> > +               if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))
> > +                       return node;
> > +
> > +       return ofnode_null();
> > +}
> > +
> > +static int am65_cpsw_mdio_setup(struct udevice *dev)
> > +{
> > +       struct am65_cpsw_priv *priv = dev_get_priv(dev);
> > +       struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
> > +       struct udevice *mdio_dev;
> > +       ofnode mdio;
> > +       int ret;
> > +
> > +       mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
> > +       if (!ofnode_valid(mdio))
> > +               return 0;
> > +
> > +       /*
> > +        * The MDIO controller is represented in the DT binding by a
> > +        * subnode of the MAC controller.
> > +        *
> > +        * We don't have a DM driver for the MDIO device yet, and thus any
> > +        * pinctrl setting on its node will be ignored.
> > +        *
> > +        * However, we do need to make sure the pins states tied to the
> > +        * MDIO node are configured properly. Fortunately, the core DM
> > +        * does that for use when we get a device, so we can work around
> 
> Does that for us?
> 
> > +        * that whole issue by just requesting a dummy MDIO driver to
> > +        * probe, and our pins will get muxed.
> > +        */
> > +       ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdio_dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> >  static int am65_cpsw_mdio_init(struct udevice *dev)
> >  {
> >         struct am65_cpsw_priv *priv = dev_get_priv(dev);
> >         struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
> > +       int ret;
> >  
> >         if (!priv->has_phy || cpsw_common->bus)
> >                 return 0;
> >  
> > +       ret = am65_cpsw_mdio_setup(dev);
> > +       if (ret)
> > +               return ret;
> > +
> >         cpsw_common->bus = cpsw_mdio_init(dev->name,
> >                                           cpsw_common->mdio_base,
> >                                           cpsw_common->bus_freq,
> > @@ -868,3 +917,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = {
> >         .plat_auto      = sizeof(struct eth_pdata),
> >         .flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE,
> >  };
> > +
> > +static const struct udevice_id am65_cpsw_mdio_ids[] = {
> > +       { .compatible = "ti,cpsw-mdio" },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(am65_cpsw_mdio) = {
> > +       .name           = "am65_cpsw_mdio",
> > +       .id             = UCLASS_MDIO,
> > +       .of_match       = am65_cpsw_mdio_ids,
> > +};
> 
> However, so far I could not make this work for our use-case [1]. It just keeps crashing. Any ideas?
> 
> [snip]
> 
> Model: Toradex 0076 Verdin AM62 Quad 2GB WB IT V1.0A
> Serial#: 15037380
> Carrier: Toradex Dahlia V1.1A, Serial# 10763237
> am65_cpsw_nuss ethernet@8000000: K3 CPSW: nuss_ver: 0x6BA01103 cpsw_ver: 0x6BA81
> 103 ale_ver: 0x00290105 Ports:2 mdio_freq:1000000
> Setting variant to wifi
> Net:
> Warning: ethernet@8000000port@1 MAC addresses don't match:
> Address in ROM is               1c:63:49:22:5f:f9
> Address in environment is       00:14:2d:e5:73:c4
> eth0: ethernet@8000000port@1 [PRIME]Could not get PHY for ethernet@8000000port@1
> : addr 7
> am65_cpsw_nuss_port ethernet@8000000port@2: phy_connect() failed
> 
> Hit any key to stop autoboot:  0
> 
> Verdin AM62 # dhcp
> ti-udma dma-controller@485c0000: k3_dmaring Ring probed rings:150, sci-dev-id:30
> ti-udma dma-controller@485c0000: dma-ring-reset-quirk: disabled
> am65_cpsw_nuss_port ethernet@8000000port@1: K3 CPSW: rflow_id_base: 19
> ethernet@8000000port@1 Waiting for PHY auto negotiation to complete......... TIMEOUT !
> am65_cpsw_nuss_port ethernet@8000000port@1: phy_startup failed
> am65_cpsw_nuss_port ethernet@8000000port@1: am65_cpsw_start end error
> "Synchronous Abort" handler, esr 0x96000010, far 0x8000f04
> elr: 00000000808503ac lr : 000000008084ce1c (reloc)
> elr: 000000009bf533ac lr : 000000009bf4fe1c
> x0 : 0000000008000f00 x1 : 0000000000000007
> x2 : 00000000ffffffff x3 : 0000000000000002
> x4 : 000000009bf53388 x5 : 0000000000011d28
> x6 : 0000000000007578 x7 : 0000000099f10570
> x8 : 0000000000000002 x9 : 0000000000000007
> x10: 0000000000000002 x11: 0000000000007578
> x12: 0000000099eeea98 x13: 0000000099eef030
> x14: 0000000000000000 x15: 0000000099eee834
> x16: 000000009bf5d040 x17: 0000000000000000
> x18: 0000000099f00d70 x19: 0000000099eeead4
> x20: 0000000099f10590 x21: 0000000000000007
> x22: 00000000ffffffff x23: 0000000000000001
> x24: 0000000000000080 x25: 0000000000000000
> x26: 00000000ffffffff x27: 000000001fffffff
> x28: 0000000000000000 x29: 0000000099eeea30
> 
> Code: 2a0103e9 2a0303e8 910003fd f94000e0 (b9400400)
> Resetting CPU ...
> 
> resetting ... 
> 
> [1] https://lore.kernel.org/all/20230715074050.941051-1-marcel@ziswiler.com

It looks like this series is fairly outdated and won't reasonably work
with these patches. Do you have a branch where you pulled our patches?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 1/2] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

On Mon, Jul 24, 2023 at 03:57:30PM +0200, Maxime Ripard wrote:

> The binding represents the MDIO controller as a child device tree
> node of the MAC device tree node.
> 
> The U-Boot driver mostly ignores that child device tree node and just
> hardcodes the resources it uses to support both the MAC and MDIO in a
> single driver.
> 
> However, some resources like pinctrl muxing states are thus ignored.
> This has been a problem with some device trees that will put some
> pinctrl states on the MDIO device tree node, like the SK-AM62 Device
> Tree does.
> 
> Let's rework the driver a bit to create a dummy MDIO driver that we will
> then get during our initialization to force the core to select the right
> muxing.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> Acked-by: Roger Quadros <rogerq@kernel.org>
> Acked-by: Nishanth Menon <nm@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/2] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Marcel Ziswiler]

Hi Maxime

On Fri, 2023-07-28 at 15:52 +0200, mripard@kernel.org wrote:
> On Fri, Jul 28, 2023 at 01:34:38PM +0000, Marcel Ziswiler wrote:

[snip]

> > However, so far I could not make this work for our use-case [1]. It just keeps crashing. Any ideas?
> > 
> > [snip]

[snip]

> > [1] https://lore.kernel.org/all/20230715074050.941051-1-marcel@ziswiler.com
> 
> It looks like this series is fairly outdated and won't reasonably work
> with these patches. Do you have a branch where you pulled our patches?

I figured it out now. I was missing CONFIG_PHY_ETHERNET_ID required for reset-gpios in the PHY node to work in
U-Boot. Sorry for the noise. Cleaned-up v4 of our stuff incoming...


Thanks!

> Maxime

Cheers

Marcel