Re: [PATCH 0/2] fdt: translate address if #size-cells = <0>

From: Dario Binacchi
Date: Thu Apr 08 2021 - 16:24:06 EST


> Il 07/04/2021 15:21 Tero Kristo <kristo@xxxxxxxxxx> ha scritto:
>
>
> On 07/04/2021 15:52, Rob Herring wrote:
> > On Wed, Apr 7, 2021 at 2:07 AM Dario Binacchi <dariobin@xxxxxxxxx> wrote:
> >>
> >>
> >>> Il 07/04/2021 03:16 Rob Herring <robh+dt@xxxxxxxxxx> ha scritto:
> >>>
> >>>
> >>> On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <dariobin@xxxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>> Il 06/04/2021 16:06 Rob Herring <robh+dt@xxxxxxxxxx> ha scritto:
> >>>>>
> >>>>>
> >>>>> On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <dariobin@xxxxxxxxx> wrote:
> >>>>>>
> >>>>>>
> >>>>>> The series comes from my commit in U-boot
> >>>>>> d64b9cdcd4 ("fdt: translate address if #size-cells = <0>")
> >>>>>> and from the subsequent exchange of emails at the end of which I was
> >>>>>> suggested to send the patch to the linux kernel
> >>>>>> (https://patchwork.ozlabs.org/project/uboot/patch/1614324949-61314-1-git-send-email-bmeng.cn@xxxxxxxxx/).
> >>>>>
> >>>>> It's 'ranges' that determines translatable which is missing from the
> >>>>> DT. This should have not had a 0 size either though maybe we could
> >>>>> support that.
> >>>>
> >>>> I have replied to the email you sent to the u-boot mailing list
> >>>>
> >>>>>
> >>>>> Does the DT have to be updated anyways for your spread spectrum support?
> >>>>
> >>>> The spread spectrum support patch does not need this patch to work. They belong
> >>>> to two different series.
> >>>
> >>> That's not what I asked. Is the spread spectrum support forcing a DT
> >>> update for users?
> >>
> >> Yes, the deltam and modfreq registers must be added to the DPLL clocks.
> >
> > That's a shame given this dts has been mostly untouched since 2013.
> >
>
> I think technically it would be possible to map these registers within
> the driver also, seeing there are like a handful of the DPLLs for both
> am3/am4 which are impacted. Just add a new compatible or something, or
> alternatively parse the register addresses and populate the
> deltam/modfreq registers based on that.

I have not added new compatibles, but I have added the offset of the delta and modfreq
registers to the data structures used by the DPLL drivers and I have set them in the
related setup functions.
https://lore.kernel.org/patchwork/patch/1406590/

>
> >>> If the DT has to be changed anyways (not really
> >>> great policy), then you could fix this in the DT at the same time.
> >>
> >> I could put the fix to the device tree in that series, although I wouldn't
> >> create a single patch to fix and add the SSC registers. First the size-cells = <0>
> >> fix patch and then the SSC patch.
> >> Do you agree?
> >
> > By at the same time, I really just meant within 1 release.
> >
> > But I'd like to hear TI maintainers' thoughts on this.
>
> I did post a comment on patch #1 questioning the approach from TI clock
> driver perspective, imho I can't see why these two patches would be
> needed right now.

Because U-boot maintainers asked me after I sent them my patch on this issue.
I believe that the email exchange that took place in the U-boot (https://patchwork.ozlabs.org/project/uboot/patch/1614324949-61314-1-git-send-email-bmeng.cn@xxxxxxxxx/)
and Linux kernel mailing lists showed that:
- The patch 'fdt: translate address if # size-cells = <0>' is wrong (U-boot has accepted
it, and it will have to be reverted).
- However, the same patch highlighted that it is wrong to use the size-cells = <0> property
in the prcm_clocks and scm_clocks nodes of device tree.
- Rob agrees that in the case of the am3xx this is the right choice:
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>;
};
[...]

- U-boot rightly wants to use the same device tree as the Kernel.
- IMHO, if I'm not missing something, I think using a #size-cells = <1>; for clocks
it requires only one code change in the ti_clk_get_reg_addr():

--- a/drivers/clk/ti/clk.c
+++ b/drivers/clk/ti/clk.c
@@ -265,9 +265,27 @@ int __init ti_clk_retry_init(struct device_node *node, void *user,
int ti_clk_get_reg_addr(struct device_node *node, int index,
struct clk_omap_reg *reg)

- if (of_property_read_u32_index(node, "reg", index, &val)) {
+ if (of_property_read_u32_index(node, "reg", index * 2, &val)) {

The other changes to develop affect device trees of architectures which, like am3, currently
use #size-cells = <0>.

IMHO, all this would lead to an improvement of the device trees with minimal impact on the code.
It would benefit U-boot, which would not have to develop special platform code and any new
architectures that would inherit from these DTs.

If you think it might be worth it, I am available to develop this patch.

Thanks and regards,
Dario

>
> -Tero