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,

+};

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