[PATCH 0/4] net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl
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,
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
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;
- /*
*/ if (CONFIG_IS_ENABLED(PINCTRL_FULL))
- Try full-implemented pinctrl first.
- If it fails or is not implemented, try simple one.
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
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;
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;
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
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";
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; };
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; };
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
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.
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
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
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
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.
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
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 :)
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).
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
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
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
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.
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.
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/
participants (4)
-
Maxime Ripard -
Nishanth Menon -
Ravi Gunasekaran -
Roger Quadros