[PATCH 1/2] of: addr: Abort address translation for parent nodes missing 'ranges'
Dario Binacchi
dariobin at libero.it
Tue Apr 6 23:52:42 CEST 2021
> Il 06/04/2021 16:26 Rob Herring <robh at kernel.org> ha scritto:
>
>
> On Tue, Mar 16, 2021 at 8:26 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Dario,
> >
> > On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi <dariobin at libero.it> wrote:
> > >
> > > Hi Bin,
> > >
> > > > Il 16/03/2021 02:28 Bin Meng <bmeng.cn at gmail.com> ha scritto:
> > > >
> > > >
> > > > Hi Dario,
> > > >
> > > > On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi <dariobin at libero.it> wrote:
> > > > >
> > > > >
> > > > > > Il 15/03/2021 19:23 Simon Glass <sjg at chromium.org> ha scritto:
> > > > > >
> > > > > >
> > > > > > +Tom Rini too
> > > > > >
> > > > > >
> > > > > > On Tue, 16 Mar 2021 at 03:48, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > >
> > > > > > > +Dario Binacchi
> > > > > > >
> > > > > > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Bin,
> > > > > > > >
> > > > > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng <bmeng.cn at 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 at gmail.com>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------
> > > > > > > > > > 1 file changed, 6 insertions(+), 18 deletions(-)
> > > > > > > > >
> > > > > > > > > Reviewed-by: Simon Glass <sjg at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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
More information about the U-Boot
mailing list