Re: [PATCH 0/2] fdt: translate address if #size-cells = <0>
From: Tero Kristo
Date: Fri Apr 09 2021 - 06:32:23 EST
On 08/04/2021 23:24, Dario Binacchi wrote:
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/
True, I just said that technically it would be possible to add this data
within the driver itself to avoid modifying DT if that would be preferred.
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.
Fix to above, it was patch #2 I was referring to.
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:
Well, I don't quite like where this is ending at. Basically you are just
modifying the am3/am4 DTs adding a size spec for every clock node. This
leaves the omap3/omap4/omap5/dra7 in the old format. Should someone
convert those also? Has anybody tested what happens with the u-boot
change on those platforms? Or with the kernel change proposed for the TI
clock driver?
Also, none of this changes the fact that imho patch #2 in this series is
wrong and should be fixed. Doing ioremap for every clock node is at
least costly (dra7xx has some 180 clock nodes based on quick grep) and
also potentially breaks things (you get extremely fragmented iomappings,
and some of them might even get rejected because you end up in the same
4K page), and should be avoided.
If things would be fixed properly, we would get rid of the clock nodes
from the DT completely and just leave the clock providers in place;
clocks would be specified via something like 'clocks = <&prcm
AM3_ADC_TSC_FCK>;' similar to what is done with the clkctrl entries, and
rest of the clock data would be built-in to the clock driver. This would
completely get rid of any future compatibility issues and the need to
tweak the DT if some clock driver would need modifications to support
some new feature.
-Tero
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