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

[PATCH 0/4] 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/

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>
---
Maxime Ripard (4):
      pinctrl: Create a select_state variant with the ofnode
      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
      fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1

 arch/arm/dts/k3-am625-sk-u-boot.dtsi |  7 ++++--
 drivers/net/ti/am65-cpsw-nuss.c      | 49 ++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-uclass.c     | 15 ++++++-----
 include/dm/pinctrl.h                 | 26 ++++++++++++++-----
 4 files changed, 83 insertions(+), 14 deletions(-)
---
base-commit: acff6e7c553d5a839e885730a4018465a34ba5a7
change-id: 20230720-ti-mdio-pinmux-a12525dba973

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

[PATCH 1/4] pinctrl: Create a select_state variant with the ofnode

[Maxime Ripard]

Some drivers might not follow entirely the driver/device association,
and thus might support what should be multiple devices in a single
driver.

Such a driver is am65-cpsw-nuss, where the MAC and MDIO controllers are
different device tree nodes, with their own resources (including pinctrl
pins) but supported by a single driver tied to the MAC device in U-Boot.

In order to get the proper pinctrl state, we would need to get the
state from the MDIO device tree node, but tie it to the MAC device since
it's the only thing we have access to.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/pinctrl/pinctrl-uclass.c | 15 +++++++++------
 include/dm/pinctrl.h             | 26 ++++++++++++++++++++------
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index 73dd7b1038bb..9e28f1858cbd 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -53,7 +53,8 @@ static int pinctrl_config_one(struct udevice *config)
  * @statename: state name, like "default"
  * @return: 0 on success, or negative error code on failure
  */
-static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
+static int pinctrl_select_state_full(struct udevice *dev, ofnode node,
+				     const char *statename)
 {
 	char propname[32]; /* long enough */
 	const fdt32_t *list;
@@ -61,7 +62,7 @@ static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
 	struct udevice *config;
 	int state, size, i, ret;
 
-	state = dev_read_stringlist_search(dev, "pinctrl-names", statename);
+	state = ofnode_stringlist_search(node, "pinctrl-names", statename);
 	if (state < 0) {
 		char *end;
 		/*
@@ -74,7 +75,7 @@ static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
 	}
 
 	snprintf(propname, sizeof(propname), "pinctrl-%d", state);
-	list = dev_read_prop(dev, propname, &size);
+	list = ofnode_get_property(node, propname, &size);
 	if (!list)
 		return -ENOSYS;
 
@@ -293,20 +294,22 @@ static int pinctrl_select_state_simple(struct udevice *dev)
 	return ops->set_state_simple(pctldev, dev);
 }
 
-int pinctrl_select_state(struct udevice *dev, const char *statename)
+int pinctrl_select_state_by_ofnode(struct udevice *dev, ofnode node,
+				   const char *statename)
 {
 	/*
 	 * Some device which is logical like mmc.blk, do not have
 	 * a valid ofnode.
 	 */
-	if (!dev_has_ofnode(dev))
+	if (!ofnode_valid(node))
 		return 0;
+
 	/*
 	 * Try full-implemented pinctrl first.
 	 * If it fails or is not implemented, try simple one.
 	 */
 	if (CONFIG_IS_ENABLED(PINCTRL_FULL))
-		return pinctrl_select_state_full(dev, statename);
+		return pinctrl_select_state_full(dev, node, statename);
 
 	return pinctrl_select_state_simple(dev);
 }
diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
index e3e50afeaff0..be4679b7f20d 100644
--- a/include/dm/pinctrl.h
+++ b/include/dm/pinctrl.h
@@ -502,6 +502,24 @@ static inline int pinctrl_generic_set_state(struct udevice *pctldev,
 #endif
 
 #if CONFIG_IS_ENABLED(PINCTRL)
+/**
+ * pinctrl_select_state_by_ofnode() - Set a device to a given state using the given ofnode
+ * @dev:	Peripheral device
+ * @ofnode:	Device Tree node to get the state from
+ * @statename:	State name, like "default"
+ *
+ * Return: 0 on success, or negative error code on failure
+ */
+int pinctrl_select_state_by_ofnode(struct udevice *dev, ofnode node, const char *statename);
+#else
+static inline int pinctrl_select_state_by_ofnode(struct udevice *dev,
+						 ofnode node,
+						 const char *statename)
+{
+	return -ENOSYS;
+}
+#endif
+
 /**
  * pinctrl_select_state() - Set a device to a given state
  * @dev:	Peripheral device
@@ -509,14 +527,10 @@ static inline int pinctrl_generic_set_state(struct udevice *pctldev,
  *
  * Return: 0 on success, or negative error code on failure
  */
-int pinctrl_select_state(struct udevice *dev, const char *statename);
-#else
-static inline int pinctrl_select_state(struct udevice *dev,
-				       const char *statename)
+static inline int pinctrl_select_state(struct udevice *dev, const char *statename)
 {
-	return -ENOSYS;
+	return pinctrl_select_state_by_ofnode(dev, dev_ofnode(dev), statename);
 }
-#endif
 
 /**
  * pinctrl_request() - Request a particular pinctrl function

-- 
2.41.0

Re: [PATCH 1/4] pinctrl: Create a select_state variant with the ofnode

[Roger Quadros]

On 20/07/2023 12:55, Maxime Ripard wrote:
> Some drivers might not follow entirely the driver/device association,
> and thus might support what should be multiple devices in a single
> driver.
> 
> Such a driver is am65-cpsw-nuss, where the MAC and MDIO controllers are
> different device tree nodes, with their own resources (including pinctrl
> pins) but supported by a single driver tied to the MAC device in U-Boot.
> 
> In order to get the proper pinctrl state, we would need to get the
> state from the MDIO device tree node, but tie it to the MAC device since
> it's the only thing we have access to.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/pinctrl/pinctrl-uclass.c | 15 +++++++++------
>  include/dm/pinctrl.h             | 26 ++++++++++++++++++++------
>  2 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
> index 73dd7b1038bb..9e28f1858cbd 100644
> --- a/drivers/pinctrl/pinctrl-uclass.c
> +++ b/drivers/pinctrl/pinctrl-uclass.c
> @@ -53,7 +53,8 @@ static int pinctrl_config_one(struct udevice *config)
>   * @statename: state name, like "default"
>   * @return: 0 on success, or negative error code on failure
>   */
> -static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
> +static int pinctrl_select_state_full(struct udevice *dev, ofnode node,
> +				     const char *statename)
>  {
>  	char propname[32]; /* long enough */
>  	const fdt32_t *list;
> @@ -61,7 +62,7 @@ static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
>  	struct udevice *config;
>  	int state, size, i, ret;
>  
> -	state = dev_read_stringlist_search(dev, "pinctrl-names", statename);
> +	state = ofnode_stringlist_search(node, "pinctrl-names", statename);
>  	if (state < 0) {
>  		char *end;
>  		/*
> @@ -74,7 +75,7 @@ static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
>  	}
>  
>  	snprintf(propname, sizeof(propname), "pinctrl-%d", state);
> -	list = dev_read_prop(dev, propname, &size);
> +	list = ofnode_get_property(node, propname, &size);
>  	if (!list)
>  		return -ENOSYS;
>  
> @@ -293,20 +294,22 @@ static int pinctrl_select_state_simple(struct udevice *dev)
>  	return ops->set_state_simple(pctldev, dev);
>  }
>  
> -int pinctrl_select_state(struct udevice *dev, const char *statename)
> +int pinctrl_select_state_by_ofnode(struct udevice *dev, ofnode node,
> +				   const char *statename)
>  {
>  	/*
>  	 * Some device which is logical like mmc.blk, do not have
>  	 * a valid ofnode.
>  	 */
> -	if (!dev_has_ofnode(dev))
> +	if (!ofnode_valid(node))
>  		return 0;
> +
>  	/*
>  	 * Try full-implemented pinctrl first.
>  	 * If it fails or is not implemented, try simple one.
>  	 */
>  	if (CONFIG_IS_ENABLED(PINCTRL_FULL))
> -		return pinctrl_select_state_full(dev, statename);
> +		return pinctrl_select_state_full(dev, node, statename);
>  
>  	return pinctrl_select_state_simple(dev);
>  }
> diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
> index e3e50afeaff0..be4679b7f20d 100644
> --- a/include/dm/pinctrl.h
> +++ b/include/dm/pinctrl.h
> @@ -502,6 +502,24 @@ static inline int pinctrl_generic_set_state(struct udevice *pctldev,
>  #endif
>  
>  #if CONFIG_IS_ENABLED(PINCTRL)
> +/**
> + * pinctrl_select_state_by_ofnode() - Set a device to a given state using the given ofnode
> + * @dev:	Peripheral device
> + * @ofnode:	Device Tree node to get the state from
> + * @statename:	State name, like "default"
> + *
> + * Return: 0 on success, or negative error code on failure
> + */
> +int pinctrl_select_state_by_ofnode(struct udevice *dev, ofnode node, const char *statename);
> +#else
> +static inline int pinctrl_select_state_by_ofnode(struct udevice *dev,
> +						 ofnode node,
> +						 const char *statename)
> +{
> +	return -ENOSYS;
> +}

This allows & encourages one device driver to change another devices pinctrl state
which doesn't look like a good idea to me.

Please see if an alternative solution pointed out in patch2 works
so we don't have to need this patch.

> +#endif
> +
>  /**
>   * pinctrl_select_state() - Set a device to a given state
>   * @dev:	Peripheral device
> @@ -509,14 +527,10 @@ static inline int pinctrl_generic_set_state(struct udevice *pctldev,
>   *
>   * Return: 0 on success, or negative error code on failure
>   */
> -int pinctrl_select_state(struct udevice *dev, const char *statename);
> -#else
> -static inline int pinctrl_select_state(struct udevice *dev,
> -				       const char *statename)
> +static inline int pinctrl_select_state(struct udevice *dev, const char *statename)
>  {
> -	return -ENOSYS;
> +	return pinctrl_select_state_by_ofnode(dev, dev_ofnode(dev), statename);
>  }
> -#endif
>  
>  /**
>   * pinctrl_request() - Request a particular pinctrl function
> 

-- 
cheers,
-roger

[PATCH 2/4] 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 device trees since Linux 6.5-rc1 which will
put some pinctrl states on the MDIO device tree node.

Let's rework the driver a bit to fetch the pinctrl state from the MDIO
node and apply it.

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

diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index f674b0baa359..2d579bf4af2c 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>
@@ -696,6 +697,50 @@ out:
 	return ret;
 }
 
+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_setup_mdio(struct udevice *dev)
+{
+	ofnode mdio;
+	int ret;
+
+	mdio = am65_cpsw_find_mdio(dev_ofnode(dev));
+	if (!ofnode_valid(mdio))
+		return -ENODEV;
+
+	/*
+	 * The MDIO controller is represented in the DT binding by a
+	 * subnode of the MAC controller.
+	 *
+	 * Our driver largely ignores that and supports MDIO by
+	 * hardcoding the register offsets.
+	 *
+	 * However, some resources (clocks, pinctrl) might be tied to
+	 * the MDIO node, and thus ignored.
+	 *
+	 * Clocks are not a concern at the moment since it's shared
+	 * between the MAC and MDIO, so the driver handles both at the
+	 * same time.
+	 *
+	 * However, we do need to make sure the pins states tied to the
+	 * MDIO node are configured properly.
+	 */
+	ret = pinctrl_select_state_by_ofnode(dev, mdio, "default");
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int am65_cpsw_probe_nuss(struct udevice *dev)
 {
 	struct am65_cpsw_common *cpsw_common = dev_get_priv(dev);
@@ -728,6 +773,10 @@ static int am65_cpsw_probe_nuss(struct udevice *dev)
 				AM65_CPSW_CPSW_NU_ALE_BASE;
 	cpsw_common->mdio_base = cpsw_common->ss_base + AM65_CPSW_MDIO_BASE;
 
+	ret = am65_cpsw_setup_mdio(dev);
+	if (ret)
+		goto out;
+
 	ports_np = dev_read_subnode(dev, "ethernet-ports");
 	if (!ofnode_valid(ports_np)) {
 		ret = -ENOENT;

-- 
2.41.0

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

[Roger Quadros]

On 20/07/2023 12:55, 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 device trees since Linux 6.5-rc1 which will
> put some pinctrl states on the MDIO device tree node.
> 
> Let's rework the driver a bit to fetch the pinctrl state from the MDIO
> node and apply it.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/net/ti/am65-cpsw-nuss.c | 49 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
> index f674b0baa359..2d579bf4af2c 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>
> @@ -696,6 +697,50 @@ out:
>  	return ret;
>  }
>  
> +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_setup_mdio(struct udevice *dev)
> +{
> +	ofnode mdio;
> +	int ret;
> +
> +	mdio = am65_cpsw_find_mdio(dev_ofnode(dev));
> +	if (!ofnode_valid(mdio))
> +		return -ENODEV;
> +
> +	/*
> +	 * The MDIO controller is represented in the DT binding by a
> +	 * subnode of the MAC controller.
> +	 *
> +	 * Our driver largely ignores that and supports MDIO by
> +	 * hardcoding the register offsets.
> +	 *
> +	 * However, some resources (clocks, pinctrl) might be tied to
> +	 * the MDIO node, and thus ignored.
> +	 *
> +	 * Clocks are not a concern at the moment since it's shared
> +	 * between the MAC and MDIO, so the driver handles both at the
> +	 * same time.
> +	 *
> +	 * However, we do need to make sure the pins states tied to the
> +	 * MDIO node are configured properly.
> +	 */
Please mention this as a HACK: that needs to go away when davinci_mdio
and this driver properly supports MDIO infrastructure.

> +	ret = pinctrl_select_state_by_ofnode(dev, mdio, "default");

Instead of modifying the API, can we have a dummy driver for the "ti,davinci_mdio"
device that registers as class UCLASS_MDIO but does nothing else?

Then you can drop patch 1 and instead do

	ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &mdio_dev);
        if (!ret)
		pinctrl_select_state(mdio_dev, "default");

	
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int am65_cpsw_probe_nuss(struct udevice *dev)
>  {
>  	struct am65_cpsw_common *cpsw_common = dev_get_priv(dev);
> @@ -728,6 +773,10 @@ static int am65_cpsw_probe_nuss(struct udevice *dev)
>  				AM65_CPSW_CPSW_NU_ALE_BASE;
>  	cpsw_common->mdio_base = cpsw_common->ss_base + AM65_CPSW_MDIO_BASE;
>  
> +	ret = am65_cpsw_setup_mdio(dev);
> +	if (ret)
> +		goto out;
> +
>  	ports_np = dev_read_subnode(dev, "ethernet-ports");
>  	if (!ofnode_valid(ports_np)) {
>  		ret = -ENOENT;
> 

-- 
cheers,
-roger

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

[Maxime Ripard]

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

On Thu, Jul 20, 2023 at 06:47:43PM +0300, Roger Quadros wrote:
> On 20/07/2023 12:55, 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 device trees since Linux 6.5-rc1 which will
> > put some pinctrl states on the MDIO device tree node.
> > 
> > Let's rework the driver a bit to fetch the pinctrl state from the MDIO
> > node and apply it.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/net/ti/am65-cpsw-nuss.c | 49 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> > 
> > diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
> > index f674b0baa359..2d579bf4af2c 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>
> > @@ -696,6 +697,50 @@ out:
> >  	return ret;
> >  }
> >  
> > +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_setup_mdio(struct udevice *dev)
> > +{
> > +	ofnode mdio;
> > +	int ret;
> > +
> > +	mdio = am65_cpsw_find_mdio(dev_ofnode(dev));
> > +	if (!ofnode_valid(mdio))
> > +		return -ENODEV;
> > +
> > +	/*
> > +	 * The MDIO controller is represented in the DT binding by a
> > +	 * subnode of the MAC controller.
> > +	 *
> > +	 * Our driver largely ignores that and supports MDIO by
> > +	 * hardcoding the register offsets.
> > +	 *
> > +	 * However, some resources (clocks, pinctrl) might be tied to
> > +	 * the MDIO node, and thus ignored.
> > +	 *
> > +	 * Clocks are not a concern at the moment since it's shared
> > +	 * between the MAC and MDIO, so the driver handles both at the
> > +	 * same time.
> > +	 *
> > +	 * However, we do need to make sure the pins states tied to the
> > +	 * MDIO node are configured properly.
> > +	 */
>
> Please mention this as a HACK: that needs to go away when davinci_mdio
> and this driver properly supports MDIO infrastructure.

Will do

> > +	ret = pinctrl_select_state_by_ofnode(dev, mdio, "default");
> 
> Instead of modifying the API, can we have a dummy driver for the "ti,davinci_mdio"
> device that registers as class UCLASS_MDIO but does nothing else?
> 
> Then you can drop patch 1 and instead do
> 
> 	ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &mdio_dev);
>         if (!ret)
> 		pinctrl_select_state(mdio_dev, "default");

Good idea, thanks :)

Maxime

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

[PATCH 3/4] 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 76589c7025a0..fe1c7408a89d 100644
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
+++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
@@ -124,7 +124,6 @@
 	reg-names = "cpsw_nuss", "mac_efuse";
 	/delete-property/ ranges;
 	bootph-pre-ram;
-	pinctrl-0 = <&main_mdio1_pins_default &main_rgmii1_pins_default>;
 
 	cpsw-phy-sel@04044 {
 		compatible = "ti,am64-phy-gmii-sel";

-- 
2.41.0

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

[Maxime Ripard]

Dropping ranges entirely doesn't work since the register offset on the
MDIO device node will now be completely off, so we need to adjust it to
the right value without the translation.

We also need to have an empty ranges property for the reg address to be
properly evaluated.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 arch/arm/dts/k3-am625-sk-u-boot.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 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 fe1c7408a89d..2ec3fff99f32 100644
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
+++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
@@ -122,8 +122,8 @@
 	reg = <0x0 0x8000000 0x0 0x200000>,
 	      <0x0 0x43000200 0x0 0x8>;
 	reg-names = "cpsw_nuss", "mac_efuse";
-	/delete-property/ ranges;
 	bootph-pre-ram;
+	ranges;
 
 	cpsw-phy-sel@04044 {
 		compatible = "ti,am64-phy-gmii-sel";
@@ -132,6 +132,10 @@
 	};
 };
 
+&cpsw3g_mdio {
+	reg = <0x0 0x8000f00 0x0 0x100>;
+};
+
 &cpsw_port1 {
 	bootph-pre-ram;
 };

-- 
2.41.0

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

[Roger Quadros]

On 20/07/2023 12:55, Maxime Ripard wrote:
> Dropping ranges entirely doesn't work since the register offset on the
> MDIO device node will now be completely off, so we need to adjust it to
> the right value without the translation.
> 
> We also need to have an empty ranges property for the reg address to be
> properly evaluated.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  arch/arm/dts/k3-am625-sk-u-boot.dtsi | 6 +++++-
>  1 file changed, 5 insertions(+), 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 fe1c7408a89d..2ec3fff99f32 100644
> --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> @@ -122,8 +122,8 @@
>  	reg = <0x0 0x8000000 0x0 0x200000>,
>  	      <0x0 0x43000200 0x0 0x8>;
>  	reg-names = "cpsw_nuss", "mac_efuse";
> -	/delete-property/ ranges;
>  	bootph-pre-ram;
> +	ranges;
>  
>  	cpsw-phy-sel@04044 {
>  		compatible = "ti,am64-phy-gmii-sel";
> @@ -132,6 +132,10 @@
>  	};
>  };
>  
> +&cpsw3g_mdio {
> +	reg = <0x0 0x8000f00 0x0 0x100>;
> +};
> +

Why this change?

Linux DT has
                cpsw3g_mdio: mdio@f00 {
                        compatible = "ti,cpsw-mdio","ti,davinci_mdio";
                        reg = <0x00 0xf00 0x00 0x100>;

And it is a child of cpsw3g node.

>  &cpsw_port1 {
>  	bootph-pre-ram;
>  };
> 

-- 
cheers,
-roger

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

[Maxime Ripard]

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

On Thu, Jul 20, 2023 at 06:27:56PM +0300, Roger Quadros wrote:
> 
> 
> On 20/07/2023 12:55, Maxime Ripard wrote:
> > Dropping ranges entirely doesn't work since the register offset on the
> > MDIO device node will now be completely off, so we need to adjust it to
> > the right value without the translation.
> > 
> > We also need to have an empty ranges property for the reg address to be
> > properly evaluated.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  arch/arm/dts/k3-am625-sk-u-boot.dtsi | 6 +++++-
> >  1 file changed, 5 insertions(+), 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 fe1c7408a89d..2ec3fff99f32 100644
> > --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> > +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> > @@ -122,8 +122,8 @@
> >  	reg = <0x0 0x8000000 0x0 0x200000>,
> >  	      <0x0 0x43000200 0x0 0x8>;
> >  	reg-names = "cpsw_nuss", "mac_efuse";
> > -	/delete-property/ ranges;
> >  	bootph-pre-ram;
> > +	ranges;
> >  
> >  	cpsw-phy-sel@04044 {
> >  		compatible = "ti,am64-phy-gmii-sel";
> > @@ -132,6 +132,10 @@
> >  	};
> >  };
> >  
> > +&cpsw3g_mdio {
> > +	reg = <0x0 0x8000f00 0x0 0x100>;
> > +};
> > +
> 
> Why this change?
> 
> Linux DT has
>                 cpsw3g_mdio: mdio@f00 {
>                         compatible = "ti,cpsw-mdio","ti,davinci_mdio";
>                         reg = <0x00 0xf00 0x00 0x100>;
> 
> And it is a child of cpsw3g node.

Right, but Linux also has on the cpsw3g node:
ranges = <0x00 0x00 0x00 0x08000000 0x00 0x200000>;

Which means that child node don't get a 1:1 mapping but we get a
0x08000000 offset.

Nishanth's patch was removing the ranges property because the
translation is troublesome for the new cpsw-phy-sel@04044 node (I
assume), so we need to add the offset to the mdio node so that it still
expresses the same base address.

Maxime

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

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

[Roger Quadros]

On 21/07/2023 10:46, Maxime Ripard wrote:
> On Thu, Jul 20, 2023 at 06:27:56PM +0300, Roger Quadros wrote:
>>
>>
>> On 20/07/2023 12:55, Maxime Ripard wrote:
>>> Dropping ranges entirely doesn't work since the register offset on the
>>> MDIO device node will now be completely off, so we need to adjust it to
>>> the right value without the translation.
>>>
>>> We also need to have an empty ranges property for the reg address to be
>>> properly evaluated.
>>>
>>> Signed-off-by: Maxime Ripard <mripard@kernel.org>
>>> ---
>>>  arch/arm/dts/k3-am625-sk-u-boot.dtsi | 6 +++++-
>>>  1 file changed, 5 insertions(+), 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 fe1c7408a89d..2ec3fff99f32 100644
>>> --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
>>> +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
>>> @@ -122,8 +122,8 @@
>>>  	reg = <0x0 0x8000000 0x0 0x200000>,
>>>  	      <0x0 0x43000200 0x0 0x8>;
>>>  	reg-names = "cpsw_nuss", "mac_efuse";
>>> -	/delete-property/ ranges;
>>>  	bootph-pre-ram;
>>> +	ranges;
>>>  
>>>  	cpsw-phy-sel@04044 {
>>>  		compatible = "ti,am64-phy-gmii-sel";
>>> @@ -132,6 +132,10 @@
>>>  	};
>>>  };
>>>  
>>> +&cpsw3g_mdio {
>>> +	reg = <0x0 0x8000f00 0x0 0x100>;
>>> +};
>>> +
>>
>> Why this change?
>>
>> Linux DT has
>>                 cpsw3g_mdio: mdio@f00 {
>>                         compatible = "ti,cpsw-mdio","ti,davinci_mdio";
>>                         reg = <0x00 0xf00 0x00 0x100>;
>>
>> And it is a child of cpsw3g node.
> 
> Right, but Linux also has on the cpsw3g node:
> ranges = <0x00 0x00 0x00 0x08000000 0x00 0x200000>;
> 
> Which means that child node don't get a 1:1 mapping but we get a
> 0x08000000 offset.

The child nodes should get 0x08000000 offset because they sit inside
CPSW address range.

The addition of cpsw-phy-sel@04044 node in u-boot.dtsi is wrong
as it sits in the control module and not in CPSW address range.

I'll send out the patch to teach am65-cpsw u-boot driver to correctly
fetch the cpsw-phy-sel via syscon handle phy.

The /delete-property/ ranges can then be removed.

> 
> Nishanth's patch was removing the ranges property because the
> translation is troublesome for the new cpsw-phy-sel@04044 node (I
> assume), so we need to add the offset to the mdio node so that it still
> expresses the same base address.

No, ranges property should be there else address translation will fail
for MDIO node. And that's why you don't need to change the cpsw3g_mdio
address in this patch.

-- 
cheers,
-roger

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

[Maxime Ripard]

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

On Fri, Jul 21, 2023 at 12:14:35PM +0300, Roger Quadros wrote:
> 
> 
> On 21/07/2023 10:46, Maxime Ripard wrote:
> > On Thu, Jul 20, 2023 at 06:27:56PM +0300, Roger Quadros wrote:
> >>
> >>
> >> On 20/07/2023 12:55, Maxime Ripard wrote:
> >>> Dropping ranges entirely doesn't work since the register offset on the
> >>> MDIO device node will now be completely off, so we need to adjust it to
> >>> the right value without the translation.
> >>>
> >>> We also need to have an empty ranges property for the reg address to be
> >>> properly evaluated.
> >>>
> >>> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> >>> ---
> >>>  arch/arm/dts/k3-am625-sk-u-boot.dtsi | 6 +++++-
> >>>  1 file changed, 5 insertions(+), 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 fe1c7408a89d..2ec3fff99f32 100644
> >>> --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> >>> +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
> >>> @@ -122,8 +122,8 @@
> >>>  	reg = <0x0 0x8000000 0x0 0x200000>,
> >>>  	      <0x0 0x43000200 0x0 0x8>;
> >>>  	reg-names = "cpsw_nuss", "mac_efuse";
> >>> -	/delete-property/ ranges;
> >>>  	bootph-pre-ram;
> >>> +	ranges;
> >>>  
> >>>  	cpsw-phy-sel@04044 {
> >>>  		compatible = "ti,am64-phy-gmii-sel";
> >>> @@ -132,6 +132,10 @@
> >>>  	};
> >>>  };
> >>>  
> >>> +&cpsw3g_mdio {
> >>> +	reg = <0x0 0x8000f00 0x0 0x100>;
> >>> +};
> >>> +
> >>
> >> Why this change?
> >>
> >> Linux DT has
> >>                 cpsw3g_mdio: mdio@f00 {
> >>                         compatible = "ti,cpsw-mdio","ti,davinci_mdio";
> >>                         reg = <0x00 0xf00 0x00 0x100>;
> >>
> >> And it is a child of cpsw3g node.
> > 
> > Right, but Linux also has on the cpsw3g node:
> > ranges = <0x00 0x00 0x00 0x08000000 0x00 0x200000>;
> > 
> > Which means that child node don't get a 1:1 mapping but we get a
> > 0x08000000 offset.
> 
> The child nodes should get 0x08000000 offset because they sit inside
> CPSW address range.

Right.

> The addition of cpsw-phy-sel@04044 node in u-boot.dtsi is wrong
> as it sits in the control module and not in CPSW address range.
> 
> I'll send out the patch to teach am65-cpsw u-boot driver to correctly
> fetch the cpsw-phy-sel via syscon handle phy.
> 
> The /delete-property/ ranges can then be removed.

Great.

> > 
> > Nishanth's patch was removing the ranges property because the
> > translation is troublesome for the new cpsw-phy-sel@04044 node (I
> > assume), so we need to add the offset to the mdio node so that it still
> > expresses the same base address.
> 
> No, ranges property should be there else address translation will fail
> for MDIO node. And that's why you don't need to change the cpsw3g_mdio
> address in this patch.

Sure. My point was that, with the /delete-property/ ranges above, that
translation doesn't occur anymore and the MDIO base address is wrong.
This patch is an attempt at fixing that, removing the delete-property is
another.

Maxime

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

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

[Ravi Gunasekaran]

On 7/20/2023 3:25 PM, Maxime Ripard wrote:
> Hi,
>
> This series is based on:
> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/
>
> 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>
> ---
> Maxime Ripard (4):
>       pinctrl: Create a select_state variant with the ofnode
>       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
>       fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1
>
>  arch/arm/dts/k3-am625-sk-u-boot.dtsi |  7 ++++--
>  drivers/net/ti/am65-cpsw-nuss.c      | 49 ++++++++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-uclass.c     | 15 ++++++-----
>  include/dm/pinctrl.h                 | 26 ++++++++++++++-----
>  4 files changed, 83 insertions(+), 14 deletions(-)
> ---
> base-commit: acff6e7c553d5a839e885730a4018465a34ba5a7
> change-id: 20230720-ti-mdio-pinmux-a12525dba973
>
> Best regards,

Adding Roger

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

[Roger Quadros]

Hi,

On 20/07/2023 16:33, Ravi Gunasekaran wrote:
> 
> 
> On 7/20/2023 3:25 PM, Maxime Ripard wrote:
>> Hi,
>>
>> This series is based on:
>> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/
>>
>> 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.

I didn't get this part. Linux does not ignore pinctrl configuration attached
to MDIO child node. What changed in 6.5-rc1?

>>
>> This was solved by duplicating the pinctrl configuration onto the MAC

If I remember right, there is no driver model driver for MDIO in u-boot and
adding the mdio pinctrl into CPSW DT node was a hack in u-boot.

>> device node. Unfortunately, this doesn't work for Linux since now it has
>> two devices competing for the same pins.

What has really changed here is that you are passing u-boot's device tree to Linux.
This has nothing to do with 6.5-rc1 right?

I suppose your solution is still a hack but of lesser evil than
duplicating MDIO pinctrl in CPSW node.

The proper solution would be to implement driver model for the davinci MDIO driver.
Siddharth has been working on this. If that is close to ready we should just use
that instead.

>>
>> Let me know what you think,
>> Maxime
>>
>> Signed-off-by: Maxime Ripard <mripard@kernel.org>
>> ---
>> Maxime Ripard (4):
>>       pinctrl: Create a select_state variant with the ofnode
>>       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
>>       fixup! arm: dts: k3-am62: Bump dtsi from linux v6.5-rc1
>>
>>  arch/arm/dts/k3-am625-sk-u-boot.dtsi |  7 ++++--
>>  drivers/net/ti/am65-cpsw-nuss.c      | 49 ++++++++++++++++++++++++++++++++++++
>>  drivers/pinctrl/pinctrl-uclass.c     | 15 ++++++-----
>>  include/dm/pinctrl.h                 | 26 ++++++++++++++-----
>>  4 files changed, 83 insertions(+), 14 deletions(-)
>> ---
>> base-commit: acff6e7c553d5a839e885730a4018465a34ba5a7
>> change-id: 20230720-ti-mdio-pinmux-a12525dba973
>>
>> Best regards,
> 
> Adding Roger

-- 
cheers,
-roger

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

[Nishanth Menon]

On 16:56-20230720, Roger Quadros wrote:
> Hi,
> 
> On 20/07/2023 16:33, Ravi Gunasekaran wrote:
> > 
> > 
> > On 7/20/2023 3:25 PM, Maxime Ripard wrote:
> >> Hi,
> >>
> >> This series is based on:
> >> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/
> >>
> >> 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.
> 
> I didn't get this part. Linux does not ignore pinctrl configuration attached
> to MDIO child node. What changed in 6.5-rc1?
> 
> >>
> >> This was solved by duplicating the pinctrl configuration onto the MAC
> 
> If I remember right, there is no driver model driver for MDIO in u-boot and
> adding the mdio pinctrl into CPSW DT node was a hack in u-boot.
> 
> >> device node. Unfortunately, this doesn't work for Linux since now it has
> >> two devices competing for the same pins.
> 
> What has really changed here is that you are passing u-boot's device tree to Linux.
> This has nothing to do with 6.5-rc1 right?
> 
> I suppose your solution is still a hack but of lesser evil than
> duplicating MDIO pinctrl in CPSW node.
> 
> The proper solution would be to implement driver model for the davinci MDIO driver.
> Siddharth has been working on this. If that is close to ready we should just use
> that instead.

But this allows for a cleaner device tree while the driver can be fixed
up independently, correct? Unfortunately, Siddharth's driver model work,
from what I understand, is around 2024 timeframe.. which is probably not
something that helps us in the community at this point.

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

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

[Maxime Ripard]

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

Hi

Sorry, I didn't receive Roger mail so I'll reply here

On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote:
> On 16:56-20230720, Roger Quadros wrote:
> > Hi,
> > 
> > On 20/07/2023 16:33, Ravi Gunasekaran wrote:
> > > 
> > > 
> > > On 7/20/2023 3:25 PM, Maxime Ripard wrote:
> > >> Hi,
> > >>
> > >> This series is based on:
> > >> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/
> > >>
> > >> 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.
> > 
> > I didn't get this part. Linux does not ignore pinctrl configuration attached
> > to MDIO child node. What changed in 6.5-rc1?

Linux doesn't ignore it, but Linux started putting pinctrl configuration
on the MDIO node, which is ignored by U-Boot.

> > >> This was solved by duplicating the pinctrl configuration onto the MAC
> > 
> > If I remember right, there is no driver model driver for MDIO in u-boot and
> > adding the mdio pinctrl into CPSW DT node was a hack in u-boot.

Yeah, but we now have in the U-Boot DT two nodes referencing the same
pinctrl configuration: the CPSW and the MDIO node. Linux then tries to
assign that configuration to both devices and it fails.

> > >> device node. Unfortunately, this doesn't work for Linux since now it has
> > >> two devices competing for the same pins.
> > 
> > What has really changed here is that you are passing u-boot's device
> > tree to Linux.

I didn't change anything. This is the default boot process if you don't
provide a DT in the ESP partition.

> > This has nothing to do with 6.5-rc1 right?

The issue started to occur with Nishanth patch to sync with Linux
6.5-rc1 device trees, so there's definitely a connection.

> > I suppose your solution is still a hack but of lesser evil than
> > duplicating MDIO pinctrl in CPSW node.
> > 
> > The proper solution would be to implement driver model for the
> > davinci MDIO driver. Siddharth has been working on this. If that is
> > close to ready we should just use that instead.
> 
> But this allows for a cleaner device tree while the driver can be fixed
> up independently, correct? Unfortunately, Siddharth's driver model work,
> from what I understand, is around 2024 timeframe.. which is probably not
> something that helps us in the community at this point.

Yeah, at the moment I have to choose between getting the MMC to work in
Linux or the Ethernet ports. The former is working thanks to your patch,
the latter is broken because of it. Ideally I'd like both :)

Maxime

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

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

[Roger Quadros]

On 20/07/2023 17:12, Maxime Ripard wrote:
> Hi
> 
> Sorry, I didn't receive Roger mail so I'll reply here
> 
> On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote:
>> On 16:56-20230720, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 20/07/2023 16:33, Ravi Gunasekaran wrote:
>>>>
>>>>
>>>> On 7/20/2023 3:25 PM, Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> This series is based on:
>>>>> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/
>>>>>
>>>>> 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.
>>>
>>> I didn't get this part. Linux does not ignore pinctrl configuration attached
>>> to MDIO child node. What changed in 6.5-rc1?
> 
> Linux doesn't ignore it, but Linux started putting pinctrl configuration
> on the MDIO node, which is ignored by U-Boot.

Linux always had MDIO pinctrl configuration in the MDIO node, way before 6.5-rc1.
Let's not mention 6.5-rc1 here as it gives an indication that something in 6.5-rc1
caused this issue. ;)

Earlier, u-boot didn't enable the MDIO node. With Nishanth's sync it does which
is fine, but duplicating the MDIO pinctrl node in the CPSW node causes problems
on Linux.

I see you reverted that change in patch 3, but probably Nishanth can avoid it if we
got this route.

> 
>>>>> This was solved by duplicating the pinctrl configuration onto the MAC
>>>
>>> If I remember right, there is no driver model driver for MDIO in u-boot and
>>> adding the mdio pinctrl into CPSW DT node was a hack in u-boot.
> 
> Yeah, but we now have in the U-Boot DT two nodes referencing the same
> pinctrl configuration: the CPSW and the MDIO node. Linux then tries to
> assign that configuration to both devices and it fails.

Understood.
> 
>>>>> device node. Unfortunately, this doesn't work for Linux since now it has
>>>>> two devices competing for the same pins.
>>>
>>> What has really changed here is that you are passing u-boot's device
>>> tree to Linux.
> 
> I didn't change anything. This is the default boot process if you don't
> provide a DT in the ESP partition.
> 

OK.
>>> This has nothing to do with 6.5-rc1 right?
> 
> The issue started to occur with Nishanth patch to sync with Linux
> 6.5-rc1 device trees, so there's definitely a connection.
> 
>>> I suppose your solution is still a hack but of lesser evil than
>>> duplicating MDIO pinctrl in CPSW node.
>>>
>>> The proper solution would be to implement driver model for the
>>> davinci MDIO driver. Siddharth has been working on this. If that is
>>> close to ready we should just use that instead.
>>
>> But this allows for a cleaner device tree while the driver can be fixed
>> up independently, correct? Unfortunately, Siddharth's driver model work,

Yes. MDIO pinctrl no longer needs to be duplicated in CPSW node at u-boot.

>> from what I understand, is around 2024 timeframe.. which is probably not
>> something that helps us in the community at this point.

OK, then we need to resort to some temporary solution like this then.
> 
> Yeah, at the moment I have to choose between getting the MMC to work in
> Linux or the Ethernet ports. The former is working thanks to your patch,
> the latter is broken because of it. Ideally I'd like both :)
> 

-- 
cheers,
-roger

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

[Nishanth Menon]

On 17:41-20230720, Roger Quadros wrote:
[...]
> >> from what I understand, is around 2024 timeframe.. which is probably not
> >> something that helps us in the community at this point.
> 
> OK, then we need to resort to some temporary solution like this then.

Thanks. I will squash up the fixup patches, lets try and get the rest
of the patches reviewed? I can put out a new RFC based on this series
(or updates).

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

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

[Ravi Gunasekaran]

Maxime,

On 7/20/23 7:42 PM, Maxime Ripard wrote:
> Hi
> 
> Sorry, I didn't receive Roger mail so I'll reply here
> 
> On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote:
>> On 16:56-20230720, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 20/07/2023 16:33, Ravi Gunasekaran wrote:
>>>>
>>>>
>>>> On 7/20/2023 3:25 PM, Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> This series is based on:
>>>>> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/
>>>>>
>>>>> 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.
>>>
>>> I didn't get this part. Linux does not ignore pinctrl configuration attached
>>> to MDIO child node. What changed in 6.5-rc1?
> 
> Linux doesn't ignore it, but Linux started putting pinctrl configuration
> on the MDIO node, which is ignored by U-Boot.
> 
>>>>> This was solved by duplicating the pinctrl configuration onto the MAC
>>>
>>> If I remember right, there is no driver model driver for MDIO in u-boot and
>>> adding the mdio pinctrl into CPSW DT node was a hack in u-boot.
> 
> Yeah, but we now have in the U-Boot DT two nodes referencing the same
> pinctrl configuration: the CPSW and the MDIO node. Linux then tries to
> assign that configuration to both devices and it fails.

This response might be late, as I know things have moved ahead after this
discussion. But I just wanted to understand the root cause little bit more.
Is the issue mainly because the same mdio pinctrl node(phandle) is referenced in
two other nodes? Or is it because of same pins being updated in two different
places in the kernel?

If it is the former, then would a duplicate mdio node help keep the changes
to minimum?


> 
>>>>> device node. Unfortunately, this doesn't work for Linux since now it has
>>>>> two devices competing for the same pins.
>>>
>>> What has really changed here is that you are passing u-boot's device
>>> tree to Linux.
> 
> I didn't change anything. This is the default boot process if you don't
> provide a DT in the ESP partition.
> 
>>> This has nothing to do with 6.5-rc1 right?
> 
> The issue started to occur with Nishanth patch to sync with Linux
> 6.5-rc1 device trees, so there's definitely a connection.
> 
>>> I suppose your solution is still a hack but of lesser evil than
>>> duplicating MDIO pinctrl in CPSW node.
>>>
>>> The proper solution would be to implement driver model for the
>>> davinci MDIO driver. Siddharth has been working on this. If that is
>>> close to ready we should just use that instead.
>>
>> But this allows for a cleaner device tree while the driver can be fixed
>> up independently, correct? Unfortunately, Siddharth's driver model work,
>> from what I understand, is around 2024 timeframe.. which is probably not
>> something that helps us in the community at this point.
> 
> Yeah, at the moment I have to choose between getting the MMC to work in
> Linux or the Ethernet ports. The former is working thanks to your patch,
> the latter is broken because of it. Ideally I'd like both :)
> 
> Maxime

-- 
Regards,
Ravi

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

[Maxime Ripard]

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

Hi Ravi,

On Wed, Jul 26, 2023 at 02:44:00PM +0530, Ravi Gunasekaran wrote:
> On 7/20/23 7:42 PM, Maxime Ripard wrote:
> > Hi
> > 
> > Sorry, I didn't receive Roger mail so I'll reply here
> > 
> > On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote:
> >> On 16:56-20230720, Roger Quadros wrote:
> >>> Hi,
> >>>
> >>> On 20/07/2023 16:33, Ravi Gunasekaran wrote:
> >>>>
> >>>>
> >>>> On 7/20/2023 3:25 PM, Maxime Ripard wrote:
> >>>>> Hi,
> >>>>>
> >>>>> This series is based on:
> >>>>> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/
> >>>>>
> >>>>> 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.
> >>>
> >>> I didn't get this part. Linux does not ignore pinctrl configuration attached
> >>> to MDIO child node. What changed in 6.5-rc1?
> > 
> > Linux doesn't ignore it, but Linux started putting pinctrl configuration
> > on the MDIO node, which is ignored by U-Boot.
> > 
> >>>>> This was solved by duplicating the pinctrl configuration onto the MAC
> >>>
> >>> If I remember right, there is no driver model driver for MDIO in u-boot and
> >>> adding the mdio pinctrl into CPSW DT node was a hack in u-boot.
> > 
> > Yeah, but we now have in the U-Boot DT two nodes referencing the same
> > pinctrl configuration: the CPSW and the MDIO node. Linux then tries to
> > assign that configuration to both devices and it fails.
> 
> This response might be late, as I know things have moved ahead after this
> discussion. But I just wanted to understand the root cause little bit more.
> Is the issue mainly because the same mdio pinctrl node(phandle) is referenced in
> two other nodes? Or is it because of same pins being updated in two different
> places in the kernel?
> 
> If it is the former, then would a duplicate mdio node help keep the changes
> to minimum?

Neither, actually :/ The issue is that, with the changes made by
Nishanth to bring the 6.5-rc1 DTS in, the same pinctrl node is
referenced in two separate nodes, and Linux sees that as conflicting
users of the same pins.

One quick workaround would be to remove the MDIO pinctrl property, and
add it to the MAC pinctrl property.

That's fairly dangerous if either get extended though, so it might not
be a proper solution long term.

I hope it's clearer

Maxime

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

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

[Ravi Gunasekaran]

Maxime,

On 7/26/23 6:19 PM, Maxime Ripard wrote:
> Hi Ravi,
> 
> On Wed, Jul 26, 2023 at 02:44:00PM +0530, Ravi Gunasekaran wrote:
>> On 7/20/23 7:42 PM, Maxime Ripard wrote:
>>> Hi
>>>
>>> Sorry, I didn't receive Roger mail so I'll reply here
>>>
>>> On Thu, Jul 20, 2023 at 09:00:19AM -0500, Nishanth Menon wrote:
>>>> On 16:56-20230720, Roger Quadros wrote:
>>>>> Hi,
>>>>>
>>>>> On 20/07/2023 16:33, Ravi Gunasekaran wrote:
>>>>>>
>>>>>>
>>>>>> On 7/20/2023 3:25 PM, Maxime Ripard wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> This series is based on:
>>>>>>> https://lore.kernel.org/all/20230713072019.3153871-1-nm@ti.com/
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> I didn't get this part. Linux does not ignore pinctrl configuration attached
>>>>> to MDIO child node. What changed in 6.5-rc1?
>>>
>>> Linux doesn't ignore it, but Linux started putting pinctrl configuration
>>> on the MDIO node, which is ignored by U-Boot.
>>>
>>>>>>> This was solved by duplicating the pinctrl configuration onto the MAC
>>>>>
>>>>> If I remember right, there is no driver model driver for MDIO in u-boot and
>>>>> adding the mdio pinctrl into CPSW DT node was a hack in u-boot.
>>>
>>> Yeah, but we now have in the U-Boot DT two nodes referencing the same
>>> pinctrl configuration: the CPSW and the MDIO node. Linux then tries to
>>> assign that configuration to both devices and it fails.
>>
>> This response might be late, as I know things have moved ahead after this
>> discussion. But I just wanted to understand the root cause little bit more.
>> Is the issue mainly because the same mdio pinctrl node(phandle) is referenced in
>> two other nodes? Or is it because of same pins being updated in two different
>> places in the kernel?
>>
>> If it is the former, then would a duplicate mdio node help keep the changes
>> to minimum?
> 
> Neither, actually :/ The issue is that, with the changes made by
> Nishanth to bring the 6.5-rc1 DTS in, the same pinctrl node is
> referenced in two separate nodes, and Linux sees that as conflicting
> users of the same pins.

So you mean to say, finally it boils down to the users of the same pins.
Even if there are two nodes with the same pinctrl configuration, we would
still hit the issue. 

> 
> One quick workaround would be to remove the MDIO pinctrl property, and
> add it to the MAC pinctrl property.
> 
> That's fairly dangerous if either get extended though, so it might not
> be a proper solution long term.

A proper solution would be to update the MDIO driver. I'm sorry that it 
doesn't follow the standard driver model. We have plans to fix it soon.

> 
> I hope it's clearer
> 
> Maxime

-- 
Regards,
Ravi

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

[Nishanth Menon]

On 14:44-20230726, Ravi Gunasekaran wrote:
[...]
> If it is the former, then would a duplicate mdio node help keep the changes
> to minimum?

That is worse hack. How does it make sense to have two mdio nodes to
represent the same hardware block? we are trying to get closer to kernel
dts support not farther away from it.

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

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

[Ravi Gunasekaran]

On 7/26/23 6:22 PM, Nishanth Menon wrote:
> On 14:44-20230726, Ravi Gunasekaran wrote:
> [...]
>> If it is the former, then would a duplicate mdio node help keep the changes
>> to minimum?
> 
> That is worse hack. How does it make sense to have two mdio nodes to
> represent the same hardware block? we are trying to get closer to kernel
> dts support not farther away from it.
> 

I know it's not a clean hack. In k3-am625-sk-u-boot.dtsi, as a hack,
the mdio pinctrl gets added to cpsw node. So was wondering if a duplicate
mdio pinctrl node could be added in the same file. Just wanted to know if we
could skip the changes in CPSW driver posted by Maxime. 

But Maxime's approach is much cleaner. We can just sync up kernel dts and not
keeping adding the hack to every K3 SoC's DT.


-- 
Regards,
Ravi

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

[Roger Quadros]

Ravi,

On 27/07/2023 08:36, Ravi Gunasekaran wrote:
> 
> 
> On 7/26/23 6:22 PM, Nishanth Menon wrote:
>> On 14:44-20230726, Ravi Gunasekaran wrote:
>> [...]
>>> If it is the former, then would a duplicate mdio node help keep the changes
>>> to minimum?
>>
>> That is worse hack. How does it make sense to have two mdio nodes to
>> represent the same hardware block? we are trying to get closer to kernel
>> dts support not farther away from it.
>>
> 
> I know it's not a clean hack. In k3-am625-sk-u-boot.dtsi, as a hack,
> the mdio pinctrl gets added to cpsw node. So was wondering if a duplicate
> mdio pinctrl node could be added in the same file. Just wanted to know if we
> could skip the changes in CPSW driver posted by Maxime. 
> 
> But Maxime's approach is much cleaner. We can just sync up kernel dts and not
> keeping adding the hack to every K3 SoC's DT.
> 
> 
Everything is sorted now as far as DT sync is concerned.
Please see [1] and its dependencies.

Only thing missing is to have proper MDIO DM support for TI platforms
in u-boot. Then we can get rid of the MDIO hack in am65-cpsw driver.

[1] - https://lore.kernel.org/all/20230726151027.2517151-1-nm@ti.com/

-- 
cheers,
-roger