[PATCH v2 0/3] 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/ https://lore.kernel.org/all/20230720215935.107398-1-rogerq@kernel.org/
It fixes the issue of Linux booting from the DT embedded by U-boot. The main issue there is that U-Boot doesn't handle the MDIO child node that might have resources attached to it.
Thus, any pinctrl configuration that could be attached to the MDIO child node is effectively ignored. Unfortunately, starting with 6.5-rc1, Linux does just that.
This was solved by duplicating the pinctrl configuration onto the MAC device node. Unfortunately, this doesn't work for Linux since now it has two devices competing for the same pins.
Let me know what you think, Maxime
Signed-off-by: Maxime Ripard mripard@kernel.org --- Changes in v2: - Drop the pinctrl API change in favour of a dummy driver - Link to v1: https://lore.kernel.org/r/20230720-ti-mdio-pinmux-v1-0-0bd3bd1cf759@kernel.o...
--- Maxime Ripard (3): 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/Kconfig | 1 + drivers/net/ti/am65-cpsw-nuss.c | 67 ++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 2 deletions(-) --- base-commit: e410677c49839480c8299eed90fe71557e0a37e9 change-id: 20230720-ti-mdio-pinmux-a12525dba973
Best regards,
The binding represents the MDIO controller as a child device tree node of the MAC device tree node.
The U-Boot driver mostly ignores that child device tree node and just hardcodes the resources it uses to support both the MAC and MDIO in a single driver.
However, some resources like pinctrl muxing states are thus ignored. This has been a problem with some device trees that will put some pinctrl states on the MDIO device tree node, like the SK-AM62 Device Tree does.
Let's rework the driver a bit to create a dummy MDIO driver that we will then get during our initialization to force the core to select the right muxing.
Signed-off-by: Maxime Ripard mripard@kernel.org --- drivers/net/ti/Kconfig | 1 + drivers/net/ti/am65-cpsw-nuss.c | 67 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)
diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig index e13dbc940182..08c81f79adf9 100644 --- a/drivers/net/ti/Kconfig +++ b/drivers/net/ti/Kconfig @@ -41,6 +41,7 @@ endchoice config TI_AM65_CPSW_NUSS bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver" depends on ARCH_K3 + imply DM_MDIO imply MISC_INIT_R imply MISC select PHYLIB diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c index 3069550d53c2..ac7907e7ef70 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> @@ -580,14 +581,69 @@ static const struct soc_attr k3_mdio_soc_data[] = { { /* sentinel */ }, };
+static ofnode am65_cpsw_find_mdio(ofnode parent) +{ + ofnode node; + + ofnode_for_each_subnode(node, parent) + if (ofnode_device_is_compatible(node, "ti,cpsw-mdio")) + return node; + + return ofnode_null(); +} + +static int am65_cpsw_mdio_setup(struct udevice *dev) +{ + struct am65_cpsw_priv *priv = dev_get_priv(dev); + struct am65_cpsw_common *cpsw_common = priv->cpsw_common; + struct udevice *mdio_dev; + ofnode mdio; + int ret; + + mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev)); + if (!ofnode_valid(mdio)) + return -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. Fortunately, the core DM + * does that for use when we get a device, so we can work around + * that whole issue by just requesting a dummy MDIO driver to + * probe, and our pins will get muxed. + */ + ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdio_dev); + if (ret) + return ret; + + return 0; +} + static int am65_cpsw_mdio_init(struct udevice *dev) { struct am65_cpsw_priv *priv = dev_get_priv(dev); struct am65_cpsw_common *cpsw_common = priv->cpsw_common; + int ret;
if (!priv->has_phy || cpsw_common->bus) return 0;
+ ret = am65_cpsw_mdio_setup(dev); + if (ret) + return ret; + cpsw_common->bus = cpsw_mdio_init(dev->name, cpsw_common->mdio_base, cpsw_common->bus_freq, @@ -854,3 +910,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = { .plat_auto = sizeof(struct eth_pdata), .flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE, }; + +static const struct udevice_id am65_cpsw_mdio_ids[] = { + { .compatible = "ti,cpsw-mdio" }, + { } +}; + +U_BOOT_DRIVER(am65_cpsw_mdio) = { + .name = "am65_cpsw_mdio", + .id = UCLASS_MDIO, + .of_match = am65_cpsw_mdio_ids, +};
On 21/07/2023 16:07, Maxime Ripard wrote:
The binding represents the MDIO controller as a child device tree node of the MAC device tree node.
The U-Boot driver mostly ignores that child device tree node and just hardcodes the resources it uses to support both the MAC and MDIO in a single driver.
However, some resources like pinctrl muxing states are thus ignored. This has been a problem with some device trees that will put some pinctrl states on the MDIO device tree node, like the SK-AM62 Device Tree does.
You don't clearly explain the problem.
I think you need to mention that there wash a hackish solution in place that was duplicating MDIO pinctrl node in the CPSW node. And this causes problems in Linux (if booted with u-boot's device tree) due to 2 drivers (CPSW and MDIO) racing for the same MDIO pinctrl resource.
Then mention how you are fixing it.
By deleting the duplicate MDIO pinctrl entry from CPSW node.
Let's rework the driver a bit to create a dummy MDIO driver that we will then get during our initialization to force the core to select the right muxing.
Signed-off-by: Maxime Ripard mripard@kernel.org
drivers/net/ti/Kconfig | 1 + drivers/net/ti/am65-cpsw-nuss.c | 67 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)
diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig index e13dbc940182..08c81f79adf9 100644 --- a/drivers/net/ti/Kconfig +++ b/drivers/net/ti/Kconfig @@ -41,6 +41,7 @@ endchoice config TI_AM65_CPSW_NUSS bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver" depends on ARCH_K3
- imply DM_MDIO imply MISC_INIT_R imply MISC select PHYLIB
diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c index 3069550d53c2..ac7907e7ef70 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> @@ -580,14 +581,69 @@ static const struct soc_attr k3_mdio_soc_data[] = { { /* sentinel */ }, };
+static ofnode am65_cpsw_find_mdio(ofnode parent) +{
- ofnode node;
- ofnode_for_each_subnode(node, parent)
if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))return node;- return ofnode_null();
+}
+static int am65_cpsw_mdio_setup(struct udevice *dev) +{
- struct am65_cpsw_priv *priv = dev_get_priv(dev);
- struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
- struct udevice *mdio_dev;
- ofnode mdio;
- int ret;
- mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
- if (!ofnode_valid(mdio))
return -ENODEV;
Do we really want to treat this as an error?
As per Linux/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml mdio node is not a required property/child.
- /*
* 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.
I think you can drop the above 3 paras and instead say "We don't yet have a DM device driver for the MDIO device and so its pinctrl setting will be ignored."
** However, we do need to make sure the pins states tied to the* MDIO node are configured properly. Fortunately, the core DM* does that for use when we get a device, so we can work around* that whole issue by just requesting a dummy MDIO driver to* probe, and our pins will get muxed.*/- ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdio_dev);
- if (ret)
return ret;- return 0;
+}
static int am65_cpsw_mdio_init(struct udevice *dev) { struct am65_cpsw_priv *priv = dev_get_priv(dev); struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
int ret;
if (!priv->has_phy || cpsw_common->bus) return 0;
ret = am65_cpsw_mdio_setup(dev);
if (ret)
return ret;cpsw_common->bus = cpsw_mdio_init(dev->name, cpsw_common->mdio_base, cpsw_common->bus_freq,
@@ -854,3 +910,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = { .plat_auto = sizeof(struct eth_pdata), .flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE, };
+static const struct udevice_id am65_cpsw_mdio_ids[] = {
- { .compatible = "ti,cpsw-mdio" },
- { }
+};
+U_BOOT_DRIVER(am65_cpsw_mdio) = {
- .name = "am65_cpsw_mdio",
- .id = UCLASS_MDIO,
- .of_match = am65_cpsw_mdio_ids,
+};
On Sat, Jul 22, 2023 at 11:25:50AM +0300, Roger Quadros wrote:
On 21/07/2023 16:07, Maxime Ripard wrote:
The binding represents the MDIO controller as a child device tree node of the MAC device tree node.
The U-Boot driver mostly ignores that child device tree node and just hardcodes the resources it uses to support both the MAC and MDIO in a single driver.
However, some resources like pinctrl muxing states are thus ignored. This has been a problem with some device trees that will put some pinctrl states on the MDIO device tree node, like the SK-AM62 Device Tree does.
You don't clearly explain the problem.
I'm sorry to hear that.
I think you need to mention that there wash a hackish solution in place that was duplicating MDIO pinctrl node in the CPSW node. And this causes problems in Linux (if booted with u-boot's device tree) due to 2 drivers (CPSW and MDIO) racing for the same MDIO pinctrl resource.
I mean, ultimately, this hack was due to nothing but a workaround around the fact that U-Boot was ignoring the MDIO child node resources. This is the root cause. And once fixed, that hack can go away.
Then mention how you are fixing it.
I'm fixing it by "rework[ing] the driver a bit to create a dummy MDIO driver that we will then get during our initialization to force the core to select the right muxing."
If that's still not a clear explanation, please provide a better one so that we can move forward.
By deleting the duplicate MDIO pinctrl entry from CPSW node.
Not really, no. If I was only deleting the duplitate pinctrl entry, U-Boot would still be broken.
Let's rework the driver a bit to create a dummy MDIO driver that we will then get during our initialization to force the core to select the right muxing.
Signed-off-by: Maxime Ripard mripard@kernel.org
drivers/net/ti/Kconfig | 1 + drivers/net/ti/am65-cpsw-nuss.c | 67 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)
diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig index e13dbc940182..08c81f79adf9 100644 --- a/drivers/net/ti/Kconfig +++ b/drivers/net/ti/Kconfig @@ -41,6 +41,7 @@ endchoice config TI_AM65_CPSW_NUSS bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver" depends on ARCH_K3
- imply DM_MDIO imply MISC_INIT_R imply MISC select PHYLIB
diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c index 3069550d53c2..ac7907e7ef70 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> @@ -580,14 +581,69 @@ static const struct soc_attr k3_mdio_soc_data[] = { { /* sentinel */ }, };
+static ofnode am65_cpsw_find_mdio(ofnode parent) +{
- ofnode node;
- ofnode_for_each_subnode(node, parent)
if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))return node;- return ofnode_null();
+}
+static int am65_cpsw_mdio_setup(struct udevice *dev) +{
- struct am65_cpsw_priv *priv = dev_get_priv(dev);
- struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
- struct udevice *mdio_dev;
- ofnode mdio;
- int ret;
- mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
- if (!ofnode_valid(mdio))
return -ENODEV;Do we really want to treat this as an error?
As per Linux/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml mdio node is not a required property/child.
Ack, I'll change it.
- /*
* 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.I think you can drop the above 3 paras and instead say "We don't yet have a DM device driver for the MDIO device and so its pinctrl setting will be ignored."
Ok.
Maxime
On 24/07/2023 11:39, Maxime Ripard wrote:
On Sat, Jul 22, 2023 at 11:25:50AM +0300, Roger Quadros wrote:
On 21/07/2023 16:07, Maxime Ripard wrote:
The binding represents the MDIO controller as a child device tree node of the MAC device tree node.
The U-Boot driver mostly ignores that child device tree node and just hardcodes the resources it uses to support both the MAC and MDIO in a single driver.
However, some resources like pinctrl muxing states are thus ignored. This has been a problem with some device trees that will put some pinctrl states on the MDIO device tree node, like the SK-AM62 Device Tree does.
You don't clearly explain the problem.
I'm sorry to hear that.
I think you need to mention that there wash a hackish solution in place that was duplicating MDIO pinctrl node in the CPSW node. And this causes problems in Linux (if booted with u-boot's device tree) due to 2 drivers (CPSW and MDIO) racing for the same MDIO pinctrl resource.
I mean, ultimately, this hack was due to nothing but a workaround around the fact that U-Boot was ignoring the MDIO child node resources. This is the root cause. And once fixed, that hack can go away.
Then mention how you are fixing it.
I'm fixing it by "rework[ing] the driver a bit to create a dummy MDIO driver that we will then get during our initialization to force the core to select the right muxing."
If that's still not a clear explanation, please provide a better one so that we can move forward.
This is fine thanks.
By deleting the duplicate MDIO pinctrl entry from CPSW node.
Not really, no. If I was only deleting the duplitate pinctrl entry, U-Boot would still be broken.
Let's rework the driver a bit to create a dummy MDIO driver that we will then get during our initialization to force the core to select the right muxing.
Signed-off-by: Maxime Ripard mripard@kernel.org
drivers/net/ti/Kconfig | 1 + drivers/net/ti/am65-cpsw-nuss.c | 67 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)
diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig index e13dbc940182..08c81f79adf9 100644 --- a/drivers/net/ti/Kconfig +++ b/drivers/net/ti/Kconfig @@ -41,6 +41,7 @@ endchoice config TI_AM65_CPSW_NUSS bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver" depends on ARCH_K3
- imply DM_MDIO imply MISC_INIT_R imply MISC select PHYLIB
diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c index 3069550d53c2..ac7907e7ef70 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> @@ -580,14 +581,69 @@ static const struct soc_attr k3_mdio_soc_data[] = { { /* sentinel */ }, };
+static ofnode am65_cpsw_find_mdio(ofnode parent) +{
- ofnode node;
- ofnode_for_each_subnode(node, parent)
if (ofnode_device_is_compatible(node, "ti,cpsw-mdio"))return node;- return ofnode_null();
+}
+static int am65_cpsw_mdio_setup(struct udevice *dev) +{
- struct am65_cpsw_priv *priv = dev_get_priv(dev);
- struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
- struct udevice *mdio_dev;
- ofnode mdio;
- int ret;
- mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev));
- if (!ofnode_valid(mdio))
return -ENODEV;Do we really want to treat this as an error?
As per Linux/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml mdio node is not a required property/child.
Ack, I'll change it.
- /*
* 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.I think you can drop the above 3 paras and instead say "We don't yet have a DM device driver for the MDIO device and so its pinctrl setting will be ignored."
Ok.
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 379165373aca..db814ed02a7e 100644 --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi @@ -121,7 +121,6 @@ &cpsw3g { /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";
On 21/07/2023 16:07, Maxime Ripard wrote:
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
Acked-by: Roger Quadros rogerq@kernel.org
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 db814ed02a7e..77c9e4cb87f7 100644 --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi @@ -119,8 +119,8 @@ };
&cpsw3g { - /delete-property/ ranges; bootph-pre-ram; + ranges;
cpsw-phy-sel@04044 { compatible = "ti,am64-phy-gmii-sel"; @@ -129,6 +129,10 @@ }; };
+&cpsw3g_mdio { + reg = <0x0 0x8000f00 0x0 0x100>; +}; + &cpsw_port1 { bootph-pre-ram; };
On 21/07/2023 16:07, 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 db814ed02a7e..77c9e4cb87f7 100644 --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi @@ -119,8 +119,8 @@ };
&cpsw3g {
- /delete-property/ ranges;
cpsw-phy-sel will be broken in u-boot after you remove /delete-property/ ranges. To fix this up we need to teach the am65-cpsw driver to fetch the cpsw-phy-sel address from the phys property instead and drop the cpsw-phy-sel child.
bootph-pre-ram;
- ranges;
You don't have to add ranges here. am62-main.dtsi should have it in the cpsw3g node.
cpsw-phy-sel@04044 { compatible = "ti,am64-phy-gmii-sel"; @@ -129,6 +129,10 @@ }; };
+&cpsw3g_mdio {
- reg = <0x0 0x8000f00 0x0 0x100>;
+};
This should not be required. The u-boot driver is still hard-coding the MDIO address and Linux should get the right address based on address translation of the child cpsw3g_mdio node.
&cpsw_port1 { bootph-pre-ram; };
Hi,
Thanks for your review
On Sat, Jul 22, 2023 at 11:32:37AM +0300, Roger Quadros wrote:
On 21/07/2023 16:07, 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 db814ed02a7e..77c9e4cb87f7 100644 --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi @@ -119,8 +119,8 @@ };
&cpsw3g {
- /delete-property/ ranges;
cpsw-phy-sel will be broken in u-boot after you remove /delete-property/ ranges.
I don't think it would?
If we look at k3-am62-main.dtsi, we have:
ranges = <0x00 0x00 0x00 0x08000000 0x00 0x200000>;
There's an address-cells and size-cells of 2, so it translates any child node address between the range 0x000000-0x200000 to 0x08000000-0x8200000.
cpsw-mdio is thus translated to 0x08000f00.
Nishanth patch was dropping that ranges property, which means that (aside from breaking the address translation if we follow the spec), cpsw-mdio now has the address of its reg property: 0xf00.
...
To fix this up we need to teach the am65-cpsw driver to fetch the cpsw-phy-sel address from the phys property instead and drop the cpsw-phy-sel child.
bootph-pre-ram;
- ranges;
You don't have to add ranges here. am62-main.dtsi should have it in the cpsw3g node.
...
So I'm adding a new 1:1 address translation for spec-compliant code to still work.
...
cpsw-phy-sel@04044 { compatible = "ti,am64-phy-gmii-sel"; @@ -129,6 +129,10 @@ }; };
+&cpsw3g_mdio {
- reg = <0x0 0x8000f00 0x0 0x100>;
+};
... And I'm setting the MDIO reg property to what the address was prior to the ranges property removal. The driver, if it follows ranges properly, will get exactly the same address.
Now, onto your other comments:
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi @@ -119,8 +119,8 @@ };
&cpsw3g {
- /delete-property/ ranges;
cpsw-phy-sel will be broken in u-boot after you remove /delete-property/ ranges.
To fix this up we need to teach the am65-cpsw driver to fetch the cpsw-phy-sel address from the phys property instead and drop the cpsw-phy-sel child.
I don't know the TI platform that well, but my understanding is that the address used to be 0x00104044, which was probably interpreted as such by U-Boot if we were missing ranges. I added back ranges to comply to the spec but with a 1:1 mapping so that address won't change.
How is it broken exactly?
@@ -129,6 +129,10 @@ }; };
+&cpsw3g_mdio {
- reg = <0x0 0x8000f00 0x0 0x100>;
+};
This should not be required. The u-boot driver is still hard-coding the MDIO address and Linux should get the right address based on address translation of the child cpsw3g_mdio node.
As pointed above, Linux will not get the same address anymore (and will actually return -EINVAL when decoding it), so no, it's very much needed.
Maxime
Hi Maxime,
On 24/07/2023 10:44, Maxime Ripard wrote:
Hi,
Thanks for your review
On Sat, Jul 22, 2023 at 11:32:37AM +0300, Roger Quadros wrote:
On 21/07/2023 16:07, 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 db814ed02a7e..77c9e4cb87f7 100644 --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi @@ -119,8 +119,8 @@ };
&cpsw3g {
- /delete-property/ ranges;
cpsw-phy-sel will be broken in u-boot after you remove /delete-property/ ranges.
I don't think it would?
If we look at k3-am62-main.dtsi, we have:
ranges = <0x00 0x00 0x00 0x08000000 0x00 0x200000>;
There's an address-cells and size-cells of 2, so it translates any child node address between the range 0x000000-0x200000 to 0x08000000-0x8200000.
cpsw-mdio is thus translated to 0x08000f00.
Nishanth patch was dropping that ranges property, which means that
This is not right. The ranges property should persist in the am62-main.dtsi. I think you can assume that ranges property will still be there.
(aside from breaking the address translation if we follow the spec), cpsw-mdio now has the address of its reg property: 0xf00.
...
To fix this up we need to teach the am65-cpsw driver to fetch the cpsw-phy-sel address from the phys property instead and drop the cpsw-phy-sel child.
bootph-pre-ram;
- ranges;
You don't have to add ranges here. am62-main.dtsi should have it in the cpsw3g node.
...
So I'm adding a new 1:1 address translation for spec-compliant code to still work.
...
cpsw-phy-sel@04044 { compatible = "ti,am64-phy-gmii-sel"; @@ -129,6 +129,10 @@ }; };
+&cpsw3g_mdio {
- reg = <0x0 0x8000f00 0x0 0x100>;
+};
... And I'm setting the MDIO reg property to what the address was prior to the ranges property removal. The driver, if it follows ranges properly, will get exactly the same address.
Now, onto your other comments:
--- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi @@ -119,8 +119,8 @@ };
&cpsw3g {
- /delete-property/ ranges;
cpsw-phy-sel will be broken in u-boot after you remove /delete-property/ ranges.
To fix this up we need to teach the am65-cpsw driver to fetch the cpsw-phy-sel address from the phys property instead and drop the cpsw-phy-sel child.
I don't know the TI platform that well, but my understanding is that the address used to be 0x00104044, which was probably interpreted as such by U-Boot if we were missing ranges. I added back ranges to comply to the spec but with a 1:1 mapping so that address won't change.
How is it broken exactly?
@@ -129,6 +129,10 @@ }; };
+&cpsw3g_mdio {
- reg = <0x0 0x8000f00 0x0 0x100>;
+};
This should not be required. The u-boot driver is still hard-coding the MDIO address and Linux should get the right address based on address translation of the child cpsw3g_mdio node.
As pointed above, Linux will not get the same address anymore (and will actually return -EINVAL when decoding it), so no, it's very much needed.
Can you please re-base your patches on top of [1]. Hopefully all the issues/questions are resolved after that? Thanks.
[1] - https://lore.kernel.org/all/20230722193151.117345-1-rogerq@kernel.org/
participants (2)
-
Maxime Ripard -
Roger Quadros