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/[email protected]/).
The second patch of the series aims to demonstrate that the first one, which
enables the translation of addresses also for crossings of DT nodes
with #size-cells = <0>, it really works.
Dario Binacchi (2):
fdt: translate address if #size-cells = <0>
clk: ti: get register address from device tree
drivers/clk/ti/clk.c | 13 ++++++++++++-
drivers/of/Kconfig | 13 +++++++++++++
drivers/of/address.c | 8 +++++++-
drivers/of/fdt_address.c | 6 ++++--
4 files changed, 36 insertions(+), 4 deletions(-)
--
2.17.1
Until now, only the register offset was retrieved from the device tree
to be added, during access, to a common base address for the clocks.
If possible, we try to retrieve the physical address of the register
directly from the device tree.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/clk/ti/clk.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
index 3da33c786d77..938f5a2cb425 100644
--- a/drivers/clk/ti/clk.c
+++ b/drivers/clk/ti/clk.c
@@ -265,9 +265,21 @@ 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)
{
+ const __be32 *addrp;
+ u64 size, addr = OF_BAD_ADDR;
+ unsigned int flags;
u32 val;
int i;
+ addrp = of_get_address(node, index, &size, &flags);
+ if (addrp)
+ addr = of_translate_address(node, addrp);
+
+ if (addr != OF_BAD_ADDR) {
+ reg->ptr = ioremap(addr, sizeof(u32));
+ return 0;
+ }
+
for (i = 0; i < CLK_MAX_MEMMAPS; i++) {
if (clocks_node_ptr[i] == node->parent)
break;
@@ -287,7 +299,6 @@ int ti_clk_get_reg_addr(struct device_node *node, int index,
reg->offset = val;
reg->ptr = NULL;
-
return 0;
}
--
2.17.1
The __of_translate_address() 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 #size-cells = <0> can not
be mapped to a CPU physical address not because it's really specified
that way, but because this is traditionally the way IBM at least do
things. 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>.
The CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol makes translation
possible even in the case of crossing levels with #size-cells = <0>.
The patch has been tested on a beaglebone black board. The addresses
generated for the clock registers are those specified by the AM335x
reference manual.
Signed-off-by: Dario Binacchi <[email protected]>
---
drivers/of/Kconfig | 13 +++++++++++++
drivers/of/address.c | 8 +++++++-
drivers/of/fdt_address.c | 6 ++++--
3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 18450437d5d5..5a76c8c23de6 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -100,4 +100,17 @@ config OF_DMA_DEFAULT_COHERENT
# arches should select this if DMA is coherent by default for OF devices
bool
+
+config OF_TRANSLATE_ZERO_SIZE_CELLS
+ bool "Enable translation for zero size cells"
+ depends on OF_EARLY_FLATTREE || OF_ADDRESS
+ default y if SOC_AM33XX
+ help
+ The routine used to translate an FDT address into a physical CPU
+ address was developed by IBM. It considers that crossing any level
+ with #size-cells = <0> makes translation impossible, even if it is
+ not the way it was specified.
+ Enabling this option makes translation possible even in the case
+ of crossing levels with #size-cells = <0>.
+
endif # OF
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 73ddf2540f3f..7b7ef69fb126 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -20,7 +20,10 @@
/* Max address size we deal with */
#define OF_MAX_ADDR_CELLS 4
#define OF_CHECK_ADDR_COUNT(na) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
-#define OF_CHECK_COUNTS(na, ns) (OF_CHECK_ADDR_COUNT(na) && (ns) > 0)
+#define OF_CHECK_COUNTS(na, ns) \
+ (OF_CHECK_ADDR_COUNT(na) && \
+ ((ns) > 0 || (IS_ENABLED(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS) && \
+ (ns) == 0)))
static struct of_bus *of_match_bus(struct device_node *np);
static int __of_address_to_resource(struct device_node *dev,
@@ -420,6 +423,9 @@ static struct of_bus *of_match_bus(struct device_node *np)
static int of_empty_ranges_quirk(struct device_node *np)
{
+ if (IS_ENABLED(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS))
+ return true;
+
if (IS_ENABLED(CONFIG_PPC)) {
/* To save cycles, we cache the result for global "Mac" setting */
static int quirk_state = -1;
diff --git a/drivers/of/fdt_address.c b/drivers/of/fdt_address.c
index 1dc15ab78b10..09c78c94c1b5 100644
--- a/drivers/of/fdt_address.c
+++ b/drivers/of/fdt_address.c
@@ -19,8 +19,10 @@
/* Max address size we deal with */
#define OF_MAX_ADDR_CELLS 4
-#define OF_CHECK_COUNTS(na, ns) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
- (ns) > 0)
+#define OF_CHECK_COUNTS(na, ns) \
+ ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
+ ((ns) > 0 || (IS_ENABLED(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS) && \
+ (ns) == 0)))
/* Debug utility */
#ifdef DEBUG
--
2.17.1
On 02/04/2021 22:20, Dario Binacchi wrote:
> Until now, only the register offset was retrieved from the device tree
> to be added, during access, to a common base address for the clocks.
> If possible, we try to retrieve the physical address of the register
> directly from the device tree.
The physical address is derived from the base address of the clock
provider, it is not derived from the clock node itself.
Doing what this patch does may actually break things, as you end up
creating an individual ioremap for every single clock register, and they
are typically a word apart from each other. In the TI clock driver case,
the ioremap is done only once for the whole clock register space.
-Tero
>
> Signed-off-by: Dario Binacchi <[email protected]>
>
> ---
>
> drivers/clk/ti/clk.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
> index 3da33c786d77..938f5a2cb425 100644
> --- a/drivers/clk/ti/clk.c
> +++ b/drivers/clk/ti/clk.c
> @@ -265,9 +265,21 @@ 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)
> {
> + const __be32 *addrp;
> + u64 size, addr = OF_BAD_ADDR;
> + unsigned int flags;
> u32 val;
> int i;
>
> + addrp = of_get_address(node, index, &size, &flags);
> + if (addrp)
> + addr = of_translate_address(node, addrp);
> +
> + if (addr != OF_BAD_ADDR) {
> + reg->ptr = ioremap(addr, sizeof(u32));
> + return 0;
> + }
> +
> for (i = 0; i < CLK_MAX_MEMMAPS; i++) {
> if (clocks_node_ptr[i] == node->parent)
> break;
> @@ -287,7 +299,6 @@ int ti_clk_get_reg_addr(struct device_node *node, int index,
>
> reg->offset = val;
> reg->ptr = NULL;
> -
> return 0;
> }
>
>
On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
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.
Does the DT have to be updated anyways for your spread spectrum support?
> The second patch of the series aims to demonstrate that the first one, which
> enables the translation of addresses also for crossings of DT nodes
> with #size-cells = <0>, it really works.
I don't seem to have the 2nd patch... In any case, you should handle
the special case for this platform in code for the platform.
Rob
On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <[email protected]> wrote:
>
>
> > Il 06/04/2021 16:06 Rob Herring <[email protected]> ha scritto:
> >
> >
> > On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
> >
> > 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? If the DT has to be changed anyways (not really
great policy), then you could fix this in the DT at the same time.
Rob
> Il 06/04/2021 16:06 Rob Herring <[email protected]> ha scritto:
>
>
> On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
>
> 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.
>
> > The second patch of the series aims to demonstrate that the first one, which
> > enables the translation of addresses also for crossings of DT nodes
> > with #size-cells = <0>, it really works.
>
> I don't seem to have the 2nd patch... In any case, you should handle
> the special case for this platform in code for the platform.
>
the 2nd patch:
https://lore.kernel.org/patchwork/patch/1407108/
Thanks and regards,
Dario
> Rob
On Wed, Apr 7, 2021 at 2:07 AM Dario Binacchi <[email protected]> wrote:
>
>
> > Il 07/04/2021 03:16 Rob Herring <[email protected]> ha scritto:
> >
> >
> > On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <[email protected]> wrote:
> > >
> > >
> > > > Il 06/04/2021 16:06 Rob Herring <[email protected]> ha scritto:
> > > >
> > > >
> > > > On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
> > > >
> > > > 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.
> > 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.
Rob
On 07/04/2021 15:52, Rob Herring wrote:
> On Wed, Apr 7, 2021 at 2:07 AM Dario Binacchi <[email protected]> wrote:
>>
>>
>>> Il 07/04/2021 03:16 Rob Herring <[email protected]> ha scritto:
>>>
>>>
>>> On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <[email protected]> wrote:
>>>>
>>>>
>>>>> Il 06/04/2021 16:06 Rob Herring <[email protected]> ha scritto:
>>>>>
>>>>>
>>>>> On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
>>>>>
>>>>> 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.
>>> 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.
-Tero
> Il 07/04/2021 03:16 Rob Herring <[email protected]> ha scritto:
>
>
> On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <[email protected]> wrote:
> >
> >
> > > Il 06/04/2021 16:06 Rob Herring <[email protected]> ha scritto:
> > >
> > >
> > > On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
> > >
> > > 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.
> 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?
Thanks and regards,
Dario
>
> Rob
> Il 07/04/2021 15:21 Tero Kristo <[email protected]> ha scritto:
>
>
> On 07/04/2021 15:52, Rob Herring wrote:
> > On Wed, Apr 7, 2021 at 2:07 AM Dario Binacchi <[email protected]> wrote:
> >>
> >>
> >>> Il 07/04/2021 03:16 Rob Herring <[email protected]> ha scritto:
> >>>
> >>>
> >>> On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <[email protected]> wrote:
> >>>>
> >>>>
> >>>>> Il 06/04/2021 16:06 Rob Herring <[email protected]> ha scritto:
> >>>>>
> >>>>>
> >>>>> On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
> >>>>>
> >>>>> 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/[email protected]/)
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
On 08/04/2021 23:24, Dario Binacchi wrote:
>
>> Il 07/04/2021 15:21 Tero Kristo <[email protected]> ha scritto:
>>
>>
>> On 07/04/2021 15:52, Rob Herring wrote:
>>> On Wed, Apr 7, 2021 at 2:07 AM Dario Binacchi <[email protected]> wrote:
>>>>
>>>>
>>>>> Il 07/04/2021 03:16 Rob Herring <[email protected]> ha scritto:
>>>>>
>>>>>
>>>>> On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>>> Il 06/04/2021 16:06 Rob Herring <[email protected]> ha scritto:
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
>>>>>>>
>>>>>>> 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/[email protected]/)
> 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
> Il 09/04/2021 12:32 Tero Kristo <[email protected]> ha scritto:
>
>
> On 08/04/2021 23:24, Dario Binacchi wrote:
> >
> >> Il 07/04/2021 15:21 Tero Kristo <[email protected]> ha scritto:
> >>
> >>
> >> On 07/04/2021 15:52, Rob Herring wrote:
> >>> On Wed, Apr 7, 2021 at 2:07 AM Dario Binacchi <[email protected]> wrote:
> >>>>
> >>>>
> >>>>> Il 07/04/2021 03:16 Rob Herring <[email protected]> ha scritto:
> >>>>>
> >>>>>
> >>>>> On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <[email protected]> wrote:
> >>>>>>
> >>>>>>
> >>>>>>> Il 06/04/2021 16:06 Rob Herring <[email protected]> ha scritto:
> >>>>>>>
> >>>>>>>
> >>>>>>> On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
> >>>>>>>
> >>>>>>> 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.
In the review of the series no one asked not to change the device tree but it is also true
that no review has been made on the patch 'clk: ti: add am33xx / am43xx spread spectrum clock support',
the one to be applied on the drivers that support the SSC.
I take this opportunity to ask you if you can kindly review that patch.
>
> >
> >>
> >>>>> 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/[email protected]/)
> > 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.
You are right, and in fact in my previous email, I proposed only to change the
ti_clk_get_reg_addr() from:
- if (of_property_read_u32_index (node, "reg", index, & val)) {
+ if (of_property_read_u32_index (node, "reg", index * 2, & val)) {
following the change of size-cells from 0 to 1 in the DTS, without ioremap.
> 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>;'
In which node of the device tree should the 'clocks = <&prcm AM3_ADC_TSC_FCK>;'
property be found?
Could you please briefly describe how the device tree would change?
The clock nodes would be removed but I am not clear how the rest of the device
tree would change.
Would this solution only impact the device trees and the code of the am3 / am4
architectures?
Thanks and regards,
Dario
> 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
On 11/04/2021 22:30, Dario Binacchi wrote:
>
>> Il 09/04/2021 12:32 Tero Kristo <[email protected]> ha scritto:
>>
>>
>> On 08/04/2021 23:24, Dario Binacchi wrote:
>>>
>>>> Il 07/04/2021 15:21 Tero Kristo <[email protected]> ha scritto:
>>>>
>>>>
>>>> On 07/04/2021 15:52, Rob Herring wrote:
>>>>> On Wed, Apr 7, 2021 at 2:07 AM Dario Binacchi <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>>> Il 07/04/2021 03:16 Rob Herring <[email protected]> ha scritto:
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <[email protected]> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> Il 06/04/2021 16:06 Rob Herring <[email protected]> ha scritto:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
>>>>>>>>>
>>>>>>>>> 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.
>
> In the review of the series no one asked not to change the device tree but it is also true
> that no review has been made on the patch 'clk: ti: add am33xx / am43xx spread spectrum clock support',
> the one to be applied on the drivers that support the SSC.
> I take this opportunity to ask you if you can kindly review that patch.
The clock driver patch itself seems fine, the devil is on the DT side,
and how we are going to re-arrange the DT data to accommodate it.
>
>>
>>>
>>>>
>>>>>>> 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/[email protected]/)
>>> 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.
>
> You are right, and in fact in my previous email, I proposed only to change the
> ti_clk_get_reg_addr() from:
> - if (of_property_read_u32_index (node, "reg", index, & val)) {
> + if (of_property_read_u32_index (node, "reg", index * 2, & val)) {
> following the change of size-cells from 0 to 1 in the DTS, without ioremap.
Yep that would be ok, assuming we change the DT in the manner proposed.
>
>> 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>;'
>
> In which node of the device tree should the 'clocks = <&prcm AM3_ADC_TSC_FCK>;'
> property be found?
This would be used to replace the device nodes, e.g. currently we have
clocks = <&adc_tsc_fck> under the tscadc node under am4, this would
change to <&prcm AM3_ADC_TSC_FCK>. Similar to any other clock entry
under every device on the platform.
> Could you please briefly describe how the device tree would change?
> The clock nodes would be removed but I am not clear how the rest of the device
> tree would change.
> Would this solution only impact the device trees and the code of the am3 / am4
> architectures?
The change on the DT itself would be pretty large, removing all clock
nodes and modifying any existing handles towards the clock nodes, and
this would impact all OMAP architectures.
Anyways, it is mostly up-to Tony how he wants to see the DT change, as
he is the maintainer for the OMAP family DT data.
I am just raising the opinion here that from kernel point of view,
adding the missing size cells seems unnecessary, and I can't see why
u-boot can't be changed to support the existing broken DT. It is broken
now, and it will be broken with the addition of the size cells in place,
and the actual "neat" end result would be to get rid of the clock nodes
completely.
-Tero
>
> Thanks and regards,
> Dario
>
>> 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
Hi Tero,
> Il 12/04/2021 09:41 Tero Kristo <[email protected]> ha scritto:
>
>
> On 11/04/2021 22:30, Dario Binacchi wrote:
> >
> >> Il 09/04/2021 12:32 Tero Kristo <[email protected]> ha scritto:
> >>
> >>
> >> On 08/04/2021 23:24, Dario Binacchi wrote:
> >>>
> >>>> Il 07/04/2021 15:21 Tero Kristo <[email protected]> ha scritto:
> >>>>
> >>>>
> >>>> On 07/04/2021 15:52, Rob Herring wrote:
> >>>>> On Wed, Apr 7, 2021 at 2:07 AM Dario Binacchi <[email protected]> wrote:
> >>>>>>
> >>>>>>
> >>>>>>> Il 07/04/2021 03:16 Rob Herring <[email protected]> ha scritto:
> >>>>>>>
> >>>>>>>
> >>>>>>> On Tue, Apr 6, 2021 at 5:02 PM Dario Binacchi <[email protected]> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> Il 06/04/2021 16:06 Rob Herring <[email protected]> ha scritto:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Fri, Apr 2, 2021 at 2:21 PM Dario Binacchi <[email protected]> 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/[email protected]/).
> >>>>>>>>>
> >>>>>>>>> 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.
> >
> > In the review of the series no one asked not to change the device tree but it is also true
> > that no review has been made on the patch 'clk: ti: add am33xx / am43xx spread spectrum clock support',
> > the one to be applied on the drivers that support the SSC.
> > I take this opportunity to ask you if you can kindly review that patch.
>
> The clock driver patch itself seems fine, the devil is on the DT side,
> and how we are going to re-arrange the DT data to accommodate it.
>
> >
> >>
> >>>
> >>>>
> >>>>>>> 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/[email protected]/)
> >>> 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.
> >
> > You are right, and in fact in my previous email, I proposed only to change the
> > ti_clk_get_reg_addr() from:
> > - if (of_property_read_u32_index (node, "reg", index, & val)) {
> > + if (of_property_read_u32_index (node, "reg", index * 2, & val)) {
> > following the change of size-cells from 0 to 1 in the DTS, without ioremap.
>
> Yep that would be ok, assuming we change the DT in the manner proposed.
>
> >
> >> 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>;'
> >
> > In which node of the device tree should the 'clocks = <&prcm AM3_ADC_TSC_FCK>;'
> > property be found?
>
> This would be used to replace the device nodes, e.g. currently we have
> clocks = <&adc_tsc_fck> under the tscadc node under am4, this would
> change to <&prcm AM3_ADC_TSC_FCK>. Similar to any other clock entry
> under every device on the platform.
>
> > Could you please briefly describe how the device tree would change?
> > The clock nodes would be removed but I am not clear how the rest of the device
> > tree would change.
> > Would this solution only impact the device trees and the code of the am3 / am4
> > architectures?
>
> The change on the DT itself would be pretty large, removing all clock
> nodes and modifying any existing handles towards the clock nodes, and
> this would impact all OMAP architectures.
>
> Anyways, it is mostly up-to Tony how he wants to see the DT change, as
> he is the maintainer for the OMAP family DT data.
>
> I am just raising the opinion here that from kernel point of view,
> adding the missing size cells seems unnecessary, and I can't see why
> u-boot can't be changed to support the existing broken DT. It is broken
> now, and it will be broken with the addition of the size cells in place,
> and the actual "neat" end result would be to get rid of the clock nodes
> completely.
I'll fix U-boot.
Thanks for your explanations.
Hope for SSC patch review from you and/or some TI MAINTAINER.
Thanks and regards,
Dario
>
> -Tero
>
> >
> > Thanks and regards,
> > Dario
> >
> >> 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
* Dario Binacchi <[email protected]> [210414 20:40]:
> > Il 12/04/2021 09:41 Tero Kristo <[email protected]> ha scritto:
> > The change on the DT itself would be pretty large, removing all clock
> > nodes and modifying any existing handles towards the clock nodes, and
> > this would impact all OMAP architectures.
> >
> > Anyways, it is mostly up-to Tony how he wants to see the DT change, as
> > he is the maintainer for the OMAP family DT data.
While I think all the clocks should use a similar binding to the clkctrl
binding, I don't know if it makes sense to start changing things around
at such a large scale.
Certainly if somebody does the patches and they can be tested to not cause
regressions, sure why not :)
> > I am just raising the opinion here that from kernel point of view,
> > adding the missing size cells seems unnecessary, and I can't see why
> > u-boot can't be changed to support the existing broken DT. It is broken
> > now, and it will be broken with the addition of the size cells in place,
> > and the actual "neat" end result would be to get rid of the clock nodes
> > completely.
>
> I'll fix U-boot.
> Thanks for your explanations.
> Hope for SSC patch review from you and/or some TI MAINTAINER.
Best to fix the issues first, then make any clean-up patches a separate
series.
Regards,
Tony