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