| Message ID | 20230721-ti-mdio-pinmux-v2-3-4bb443e09ac0@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl | expand |
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/
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; };
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(-)