Il 06/04/2021 16:26 Rob Herring robh@kernel.org ha scritto:
On Tue, Mar 16, 2021 at 8:26 PM Bin Meng bmeng.cn@gmail.com wrote:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote:
Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto:
+Tom Rini too
On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote: > > +Dario Binacchi > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > > > Hi Bin, > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > The implementation of of_translate_one() was taken from the one in > > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > > for Apple Macs that don't have the <ranges> property in the parent > > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > > block and adhere to the spec to abort the translation. > > > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > > --- > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > Reviewed-by: Simon Glass sjg@chromium.org > > > > Unfortunately this seems to cause a test failure for > > ut_dm_fdt_translation. Can you please take a look? > > It seems the no "ranges" property was intentionally removed by the > following commit: > > commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc > Author: Dario Binacchi dariobin@libero.it > Date: Wed Dec 30 00:16:21 2020 +0100 > > fdt: translate address if #size-cells = <0> > > The __of_translate_address routine translates an address from the > device tree into a CPU physical address. A note in the description of > the routine explains that the crossing of any level with > since inherited from IBM. This does not happen for Texas Instruments, or > at least for the beaglebone device tree. Without this patch, in fact, > the translation into physical addresses of the registers contained in the > am33xx-clocks.dtsi nodes would not be possible. They all have a parent > with #size-cells = <0>. > > It looks the commit was needed for beaglebone board. > > Dario, could you please comment on why U-Boot needs to done like this, > while Linux kernel has this check? Is the beaglebone board not working > in Linux? >
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
I've replied on that patch...
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
IMO, yes it is. Simply put, 'ranges' must be present to be translatable. Using '#size-cells == 0' is at least questionable, but could maybe be supported (depends what happens with non-empty 'ranges').
The 'fix' makes every 'reg' translatable. Give the function an I2C address 'reg' and you'll get back 'I2C controller address + I2C address'.
IMHO this could mean that using size-cells = <0> in the prcm_clocks and scm_clocks nodes is perhaps not quite correct. We need an address translation and if possible, better without having to develop code to handle this special case.
With the following changes in the am33xx-l4.dtsi and am33xx-clocks.dtsi files we get an address translation that does not require special case handling code and the device tree would not be an exception to what we usually see. I have tested these changes in the kernel and they seem to work.
diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi index 1fb22088caeb..59b0a0cf211e 100644 --- a/arch/arm/boot/dts/am33xx-l4.dtsi +++ b/arch/arm/boot/dts/am33xx-l4.dtsi @@ -110,7 +110,8 @@
prcm_clocks: clocks { #address-cells = <1>; - #size-cells = <0>; + #size-cells = <1>; + ranges = <0 0 0x2000>; };
prcm_clockdomains: clockdomains { @@ -320,7 +321,8 @@
scm_clocks: clocks { #address-cells = <1>; - #size-cells = <0>; + #size-cells = <1>; + ranges = <0 0 0x800>; }; };
--- a/arch/arm/boot/dts/am33xx-clocks.dtsi +++ b/arch/arm/boot/dts/am33xx-clocks.dtsi @@ -10,7 +10,7 @@ compatible = "ti,mux-clock"; clocks = <&virt_19200000_ck>, <&virt_24000000_ck>, <&virt_25000000_ck>, <&virt_26000000_ck>; ti,bit-shift = <22>; - reg = <0x0040>; + reg = <0x0040 0x4>; };
adc_tsc_fck: adc_tsc_fck { @@ -98,7 +98,7 @@ compatible = "ti,gate-clock"; clocks = <&l4ls_gclk>; ti,bit-shift = <0>; - reg = <0x0664>; + reg = <0x0664 0x04>; };
ehrpwm1_tbclk: ehrpwm1_tbclk@44e10664 { @@ -106,7 +106,7 @@ compatible = "ti,gate-clock"; clocks = <&l4ls_gclk>; ti,bit-shift = <1>; - reg = <0x0664>; + reg = <0x0664 0x04>; };
ehrpwm2_tbclk: ehrpwm2_tbclk@44e10664 { @@ -114,7 +114,7 @@ compatible = "ti,gate-clock"; clocks = <&l4ls_gclk>; ti,bit-shift = <2>; - reg = <0x0664>; + reg = <0x0664 0x04>; }; }; &prcm_clocks { @@ -164,7 +164,7 @@ #clock-cells = <0>; compatible = "ti,am3-dpll-core-clock"; clocks = <&sys_clkin_ck>, <&sys_clkin_ck>; - reg = <0x0490>, <0x045c>, <0x0468>; + reg = <0x0490 0x04>, <0x045c 0x04>, <0x0468 0x04>; };
dpll_core_x2_ck: dpll_core_x2_ck { @@ -178,7 +178,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_core_x2_ck>; ti,max-div = <31>; - reg = <0x0480>; + reg = <0x0480 0x04>; ti,index-starts-at-one; };
@@ -187,7 +187,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_core_x2_ck>; ti,max-div = <31>; - reg = <0x0484>; + reg = <0x0484 0x04>; ti,index-starts-at-one; };
@@ -196,7 +196,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_core_x2_ck>; ti,max-div = <31>; - reg = <0x04d8>; + reg = <0x04d8 0x04>; ti,index-starts-at-one; };
@@ -204,7 +204,7 @@ #clock-cells = <0>; compatible = "ti,am3-dpll-clock"; clocks = <&sys_clkin_ck>, <&sys_clkin_ck>; - reg = <0x0488>, <0x0420>, <0x042c>; + reg = <0x0488 0x04>, <0x0420 0x04>, <0x042c 0x04>; };
dpll_mpu_m2_ck: dpll_mpu_m2_ck@4a8 { @@ -212,7 +212,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_mpu_ck>; ti,max-div = <31>; - reg = <0x04a8>; + reg = <0x04a8 0x04>; ti,index-starts-at-one; };
@@ -220,7 +220,7 @@ #clock-cells = <0>; compatible = "ti,am3-dpll-no-gate-clock"; clocks = <&sys_clkin_ck>, <&sys_clkin_ck>; - reg = <0x0494>, <0x0434>, <0x0440>; + reg = <0x0494 0x04>, <0x0434 0x04>, <0x0440 0x04>; };
dpll_ddr_m2_ck: dpll_ddr_m2_ck@4a0 { @@ -244,7 +244,7 @@ #clock-cells = <0>; compatible = "ti,am3-dpll-no-gate-clock"; clocks = <&sys_clkin_ck>, <&sys_clkin_ck>; - reg = <0x0498>, <0x0448>, <0x0454>; + reg = <0x0498 0x04>, <0x0448 0x04>, <0x0454 0x04>; };
dpll_disp_m2_ck: dpll_disp_m2_ck@4a4 { @@ -252,7 +252,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_disp_ck>; ti,max-div = <31>; - reg = <0x04a4>; + reg = <0x04a4 0x04>; ti,index-starts-at-one; ti,set-rate-parent; }; @@ -261,7 +261,7 @@ #clock-cells = <0>; compatible = "ti,am3-dpll-no-gate-j-type-clock"; clocks = <&sys_clkin_ck>, <&sys_clkin_ck>; - reg = <0x048c>, <0x0470>, <0x049c>; + reg = <0x048c 0x04>, <0x0470 0x04>, <0x049c 0x04>; };
dpll_per_m2_ck: dpll_per_m2_ck@4ac { @@ -269,7 +269,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_per_ck>; ti,max-div = <31>; - reg = <0x04ac>; + reg = <0x04ac 0x04>; ti,index-starts-at-one; };
@@ -317,7 +317,7 @@ #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&l3_gclk>, <&dpll_disp_m2_ck>; - reg = <0x0530>; + reg = <0x0530 0x04>; };
mmu_fck: mmu_fck@914 { @@ -325,56 +325,56 @@ compatible = "ti,gate-clock"; clocks = <&dpll_core_m4_ck>; ti,bit-shift = <1>; - reg = <0x0914>; + reg = <0x0914 0x04>; };
timer1_fck: timer1_fck@528 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>, <&tclkin_ck>, <&clk_rc32k_ck>, <&clk_32768_ck>; - reg = <0x0528>; + reg = <0x0528 0x04>; };
timer2_fck: timer2_fck@508 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x0508>; + reg = <0x0508 0x04>; };
timer3_fck: timer3_fck@50c { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x050c>; + reg = <0x050c 0x04>; };
timer4_fck: timer4_fck@510 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x0510>; + reg = <0x0510 0x04>; };
timer5_fck: timer5_fck@518 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x0518>; + reg = <0x0518 0x04>; };
timer6_fck: timer6_fck@51c { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x051c>; + reg = <0x051c 0x04>; };
timer7_fck: timer7_fck@504 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x0504>; + reg = <0x0504 0x04>; };
usbotg_fck: usbotg_fck@47c { @@ -382,7 +382,7 @@ compatible = "ti,gate-clock"; clocks = <&dpll_per_ck>; ti,bit-shift = <8>; - reg = <0x047c>; + reg = <0x047c 0x04>; };
dpll_core_m4_div2_ck: dpll_core_m4_div2_ck { @@ -398,14 +398,14 @@ compatible = "ti,gate-clock"; clocks = <&dpll_core_m4_div2_ck>; ti,bit-shift = <1>; - reg = <0x00e4>; + reg = <0x00e4 0x04>; };
wdt1_fck: wdt1_fck@538 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&clk_rc32k_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x0538>; + reg = <0x0538 0x04>; };
l4_rtc_gclk: l4_rtc_gclk { @@ -468,21 +468,21 @@ #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&dpll_core_m5_ck>, <&dpll_core_m4_ck>; - reg = <0x0520>; + reg = <0x0520 4>; };
gpio0_dbclk_mux_ck: gpio0_dbclk_mux_ck@53c { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&clk_rc32k_ck>, <&clk_32768_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x053c>; + reg = <0x053c 0x04>; };
lcd_gclk: lcd_gclk@534 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&dpll_disp_m2_ck>, <&dpll_core_m5_ck>, <&dpll_per_m2_ck>; - reg = <0x0534>; + reg = <0x0534 0x04>; ti,set-rate-parent; };
@@ -499,14 +499,14 @@ compatible = "ti,mux-clock"; clocks = <&dpll_core_m4_ck>, <&dpll_per_m2_ck>; ti,bit-shift = <1>; - reg = <0x052c>; + reg = <0x052c 0x04>; };
gfx_fck_div_ck: gfx_fck_div_ck@52c { #clock-cells = <0>; compatible = "ti,divider-clock"; clocks = <&gfx_fclk_clksel_ck>; - reg = <0x052c>; + reg = <0x052c 0x04>; ti,max-div = <2>; };
@@ -514,7 +514,7 @@ #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&clk_32768_ck>, <&l3_gclk>, <&dpll_ddr_m2_ck>, <&dpll_per_m2_ck>, <&lcd_gclk>; - reg = <0x0700>; + reg = <0x0700 0x04>; };
clkout2_div_ck: clkout2_div_ck@700 { @@ -523,7 +523,7 @@ clocks = <&sysclkout_pre_ck>; ti,bit-shift = <3>; ti,max-div = <8>; - reg = <0x0700>; + reg = <0x0700 0x04>; };
clkout2_ck: clkout2_ck@700 { @@ -531,7 +531,7 @@ compatible = "ti,gate-clock"; clocks = <&clkout2_div_ck>; ti,bit-shift = <7>; - reg = <0x0700>; + reg = <0x0700 0x04>; }; };
Thanks and regards, Dario
Maybe the beagleboard needs to use another API to decode the address? What API is used in Linux?
The quirk for the platform should live in the code for the platform.
Rob