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

[Dario Binacchi <dariobin@libero.it>]

> Il 07/04/2021 15:21 Tero Kristo <kristo@kernel.org> ha scritto:
> 
>  
> On 07/04/2021 15:52, Rob Herring wrote:
> > On Wed, Apr 7, 2021 at 2:07 AM Dario Binacchi <dariobin@libero.it> wrote:
> >>
> >>
> >>> Il 07/04/2021 03:16 Rob Herring <robh+dt@kernel.org> ha scritto:
> >>>
> >>>
> >>> On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <dariobin@libero.it> wrote:
> >>>>
> >>>>
> >>>>> Il 06/04/2021 16:06 Rob Herring <robh+dt@kernel.org> ha scritto:
> >>>>>
> >>>>>
> >>>>> On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <dariobin@libero.it> 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@gmail.com/).
> >>>>>
> >>>>> 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@gmail.com/)
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