[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[Yangbo Lu]

Generally SYSCLK frequency is dependent on on-board switch settings.
It may vary as per requirement, but this doesn't apply to ls1012a.
ls1012a has its SYSCLK frequencies specified in the RM. The fixup
for all 'fixed-clock' compatibles of ls1012a would cause incorrect
SYSCLK frequency values. So remove the SYSCLK frequency fixup for ls1012a.

Fixes: 6f14e25 ("armv8: fsl-lsch3: fixup SYSCLK frequency in device tree")
Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
 arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
index c10ccf9..e59c232 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
+++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
@@ -161,8 +161,10 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 			       "clock-frequency", CONFIG_SYS_NS16550_CLK, 1);
 #endif
 
+#ifndef CONFIG_ARCH_LS1012A
 	do_fixup_by_compat_u32(blob, "fixed-clock",
 			       "clock-frequency", CONFIG_SYS_CLK_FREQ, 1);
+#endif
 
 #ifdef CONFIG_PCI
 	ft_pci_setup(blob, bd);
-- 
2.1.0.27.g96db324

[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[york sun]

On 01/19/2017 07:34 PM, Yangbo Lu wrote:
> Generally SYSCLK frequency is dependent on on-board switch settings.
> It may vary as per requirement, but this doesn't apply to ls1012a.
> ls1012a has its SYSCLK frequencies specified in the RM. The fixup
> for all 'fixed-clock' compatibles of ls1012a would cause incorrect
> SYSCLK frequency values. So remove the SYSCLK frequency fixup for ls1012a.
>
> Fixes: 6f14e25 ("armv8: fsl-lsch3: fixup SYSCLK frequency in device tree")
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
>  arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> index c10ccf9..e59c232 100644
> --- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> +++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> @@ -161,8 +161,10 @@ void ft_cpu_setup(void *blob, bd_t *bd)
>  			       "clock-frequency", CONFIG_SYS_NS16550_CLK, 1);
>  #endif
>
> +#ifndef CONFIG_ARCH_LS1012A
>  	do_fixup_by_compat_u32(blob, "fixed-clock",
>  			       "clock-frequency", CONFIG_SYS_CLK_FREQ, 1);
> +#endif
>
>  #ifdef CONFIG_PCI
>  	ft_pci_setup(blob, bd);
>

Yangbo,

Why fixing up this clock causes incorect frequency value? The macro 
CONFIG_SYS_CLK_FREQ is defined as 125MHz for ls1012a.

York

[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[Scott Wood]

On Fri, 2017-01-20 at 16:28 +0000, york sun wrote:
> On 01/19/2017 07:34 PM, Yangbo Lu wrote:
> > 
> > Generally SYSCLK frequency is dependent on on-board switch settings.
> > It may vary as per requirement, but this doesn't apply to ls1012a.
> > ls1012a has its SYSCLK frequencies specified in the RM. The fixup
> > for all 'fixed-clock' compatibles of ls1012a would cause incorrect
> > SYSCLK frequency values. So remove the SYSCLK frequency fixup for ls1012a.
> > 
> > Fixes: 6f14e25 ("armv8: fsl-lsch3: fixup SYSCLK frequency in device tree")
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > ---
> > ?arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 ++
> > ?1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > index c10ccf9..e59c232 100644
> > --- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > +++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > @@ -161,8 +161,10 @@ void ft_cpu_setup(void *blob, bd_t *bd)
> > ?			???????"clock-frequency", CONFIG_SYS_NS16550_CLK,
> > 1);
> > ?#endif
> > 
> > +#ifndef CONFIG_ARCH_LS1012A
> > ?	do_fixup_by_compat_u32(blob, "fixed-clock",
> > ?			???????"clock-frequency", CONFIG_SYS_CLK_FREQ,
> > 1);
> > +#endif
> > 
> > ?#ifdef CONFIG_PCI
> > ?	ft_pci_setup(blob, bd);
> > 
> Yangbo,
> 
> Why fixing up this clock causes incorect frequency value? The macro?
> CONFIG_SYS_CLK_FREQ is defined as 125MHz for ls1012a.

Because ls1012a has two different input frequencies -- 125 MHz for the
platform PLL and 100 MHz for the core PLLs. ?When we added a second fixed-
clock node for the latter, U-Boot was overwriting it.

While the ifdef solves this immediate problem, it doesn't fix the underlying
problem that this fixup is overly broad. ?It should identify the specific node
it's looking for, and not overwrite every fixed-clock node it finds.

-Scott

[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[york sun]

On 01/20/2017 01:36 PM, Scott Wood wrote:
> On Fri, 2017-01-20 at 16:28 +0000, york sun wrote:
>> On 01/19/2017 07:34 PM, Yangbo Lu wrote:
>>>
>>> Generally SYSCLK frequency is dependent on on-board switch settings.
>>> It may vary as per requirement, but this doesn't apply to ls1012a.
>>> ls1012a has its SYSCLK frequencies specified in the RM. The fixup
>>> for all 'fixed-clock' compatibles of ls1012a would cause incorrect
>>> SYSCLK frequency values. So remove the SYSCLK frequency fixup for ls1012a.
>>>
>>> Fixes: 6f14e25 ("armv8: fsl-lsch3: fixup SYSCLK frequency in device tree")
>>> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
>>> ---
>>>  arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
>>> b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
>>> index c10ccf9..e59c232 100644
>>> --- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
>>> +++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
>>> @@ -161,8 +161,10 @@ void ft_cpu_setup(void *blob, bd_t *bd)
>>>  			       "clock-frequency", CONFIG_SYS_NS16550_CLK,
>>> 1);
>>>  #endif
>>>
>>> +#ifndef CONFIG_ARCH_LS1012A
>>>  	do_fixup_by_compat_u32(blob, "fixed-clock",
>>>  			       "clock-frequency", CONFIG_SYS_CLK_FREQ,
>>> 1);
>>> +#endif
>>>
>>>  #ifdef CONFIG_PCI
>>>  	ft_pci_setup(blob, bd);
>>>
>> Yangbo,
>>
>> Why fixing up this clock causes incorect frequency value? The macro
>> CONFIG_SYS_CLK_FREQ is defined as 125MHz for ls1012a.
>
> Because ls1012a has two different input frequencies -- 125 MHz for the
> platform PLL and 100 MHz for the core PLLs.  When we added a second fixed-
> clock node for the latter, U-Boot was overwriting it.
>
> While the ifdef solves this immediate problem, it doesn't fix the underlying
> problem that this fixup is overly broad.  It should identify the specific node
> it's looking for, and not overwrite every fixed-clock node it finds.
>

So current code tries to fix up any node with "fixed-clock"? That's not 
good. What if we have multiple fixed clocks?

York

[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[Scott Wood]

On Fri, 2017-01-20 at 21:38 +0000, york sun wrote:
> On 01/20/2017 01:36 PM, Scott Wood wrote:
> > 
> > On Fri, 2017-01-20 at 16:28 +0000, york sun wrote:
> > > 
> > > On 01/19/2017 07:34 PM, Yangbo Lu wrote:
> > > > 
> > > > 
> > > > Generally SYSCLK frequency is dependent on on-board switch settings.
> > > > It may vary as per requirement, but this doesn't apply to ls1012a.
> > > > ls1012a has its SYSCLK frequencies specified in the RM. The fixup
> > > > for all 'fixed-clock' compatibles of ls1012a would cause incorrect
> > > > SYSCLK frequency values. So remove the SYSCLK frequency fixup for
> > > > ls1012a.
> > > > 
> > > > Fixes: 6f14e25 ("armv8: fsl-lsch3: fixup SYSCLK frequency in device
> > > > tree")
> > > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > > ---
> > > > ?arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 ++
> > > > ?1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > > > b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > > > index c10ccf9..e59c232 100644
> > > > --- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > > > +++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > > > @@ -161,8 +161,10 @@ void ft_cpu_setup(void *blob, bd_t *bd)
> > > > ?			???????"clock-frequency",
> > > > CONFIG_SYS_NS16550_CLK,
> > > > 1);
> > > > ?#endif
> > > > 
> > > > +#ifndef CONFIG_ARCH_LS1012A
> > > > ?	do_fixup_by_compat_u32(blob, "fixed-clock",
> > > > ?			???????"clock-frequency",
> > > > CONFIG_SYS_CLK_FREQ,
> > > > 1);
> > > > +#endif
> > > > 
> > > > ?#ifdef CONFIG_PCI
> > > > ?	ft_pci_setup(blob, bd);
> > > > 
> > > Yangbo,
> > > 
> > > Why fixing up this clock causes incorect frequency value? The macro
> > > CONFIG_SYS_CLK_FREQ is defined as 125MHz for ls1012a.
> > Because ls1012a has two different input frequencies -- 125 MHz for the
> > platform PLL and 100 MHz for the core PLLs.??When we added a second fixed-
> > clock node for the latter, U-Boot was overwriting it.
> > 
> > While the ifdef solves this immediate problem, it doesn't fix the
> > underlying
> > problem that this fixup is overly broad.??It should identify the specific
> > node
> > it's looking for, and not overwrite every fixed-clock node it finds.
> > 
> So current code tries to fix up any node with "fixed-clock"? That's not?
> good. What if we have multiple fixed clocks?
> 

That is exactly the problem. ?This patch avoids the issue on ls1012a but not
in general.

-Scott

[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[york sun]

On 01/20/2017 02:14 PM, Scott Wood wrote:
> On Fri, 2017-01-20 at 21:38 +0000, york sun wrote:
>> On 01/20/2017 01:36 PM, Scott Wood wrote:
>>>
>>> On Fri, 2017-01-20 at 16:28 +0000, york sun wrote:
>>>>

<snip>

>>>>
>>>> Why fixing up this clock causes incorect frequency value? The macro
>>>> CONFIG_SYS_CLK_FREQ is defined as 125MHz for ls1012a.
>>> Because ls1012a has two different input frequencies -- 125 MHz for the
>>> platform PLL and 100 MHz for the core PLLs.  When we added a second fixed-
>>> clock node for the latter, U-Boot was overwriting it.
>>>
>>> While the ifdef solves this immediate problem, it doesn't fix the
>>> underlying
>>> problem that this fixup is overly broad.  It should identify the specific
>>> node
>>> it's looking for, and not overwrite every fixed-clock node it finds.
>>>
>> So current code tries to fix up any node with "fixed-clock"? That's not
>> good. What if we have multiple fixed clocks?
>>
>
> That is exactly the problem.  This patch avoids the issue on ls1012a but not
> in general.
>

Then a proper fix would be check the clock name or compatible. If none 
of them exists, we should fix the device tree first.

York

[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[york sun]

On 01/20/2017 05:13 PM, york sun wrote:
>
> Then a proper fix would be check the clock name or compatible. If none
> of them exists, we should fix the device tree first.

Yangbo,

Can you fix the code to check clock name or compatible?

York

[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[Y.B. Lu]

> -----Original Message-----
> From: york sun
> Sent: Wednesday, February 08, 2017 1:03 AM
> To: Scott Wood; Y.B. Lu; u-boot at lists.denx.de
> Cc: Albert Aribaud; Z.Q. Hou
> Subject: Re: [U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK
> frequency fixup for ls1012a
> 
> On 01/20/2017 05:13 PM, york sun wrote:
> >
> > Then a proper fix would be check the clock name or compatible. If none
> > of them exists, we should fix the device tree first.
> 
> Yangbo,
> 
> Can you fix the code to check clock name or compatible?
> 
> York

[Lu Yangbo-B47093] Hi York, do you mean check the clock name or compatible to make sure it's sysclk and then fix it?
Scott's patch added coreclock node also with compatible 'fixed-clock'.
https://patchwork.kernel.org/patch/9536515/

If we check clock name, I found most names with 'fixed-clock' compatible are 'sysclk', but some are not.
fsl-ls1012a-frdm.dts:
        sys_mclk: clock-mclk {
                compatible = "fixed-clock";
                #clock-cells = <0>;
                clock-frequency = <25000000>;
        };

fsl-ls1012a-qds.dts:
        sys_mclk: clock-mclk {
                compatible = "fixed-clock";
                #clock-cells = <0>;
                clock-frequency = <24576000>;
        };

Do you have any suggestion?
Thanks.

[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[york sun]

On 02/07/2017 07:30 PM, Y.B. Lu wrote:
>> -----Original Message-----
>> From: york sun
>> Sent: Wednesday, February 08, 2017 1:03 AM
>> To: Scott Wood; Y.B. Lu; u-boot at lists.denx.de
>> Cc: Albert Aribaud; Z.Q. Hou
>> Subject: Re: [U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK
>> frequency fixup for ls1012a
>>
>> On 01/20/2017 05:13 PM, york sun wrote:
>>>
>>> Then a proper fix would be check the clock name or compatible. If none
>>> of them exists, we should fix the device tree first.
>>
>> Yangbo,
>>
>> Can you fix the code to check clock name or compatible?
>>
>> York
>
> [Lu Yangbo-B47093] Hi York, do you mean check the clock name or compatible to make sure it's sysclk and then fix it?
> Scott's patch added coreclock node also with compatible 'fixed-clock'.
> https://patchwork.kernel.org/patch/9536515/
>
> If we check clock name, I found most names with 'fixed-clock' compatible are 'sysclk', but some are not.
> fsl-ls1012a-frdm.dts:
>         sys_mclk: clock-mclk {
>                 compatible = "fixed-clock";
>                 #clock-cells = <0>;
>                 clock-frequency = <25000000>;
>         };
>
> fsl-ls1012a-qds.dts:
>         sys_mclk: clock-mclk {
>                 compatible = "fixed-clock";
>                 #clock-cells = <0>;
>                 clock-frequency = <24576000>;
>         };
>

Clearly "fixed-clock" is not a good compatible string to search for. It 
just tells you this clock is fixed in frequency. It doesn't tell you if 
a clock is system clock.

Can you find this clock by its name? If you need to unify the device 
tree, it may be the time now.

York

[U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

[Y.B. Lu]

Hi York,


> -----Original Message-----
> From: york sun
> Sent: Wednesday, February 08, 2017 12:27 PM
> To: Y.B. Lu; Scott Wood; u-boot at lists.denx.de
> Cc: Albert Aribaud; Z.Q. Hou
> Subject: Re: [U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove SYSCLK
> frequency fixup for ls1012a
> 
> On 02/07/2017 07:30 PM, Y.B. Lu wrote:
> >> -----Original Message-----
> >> From: york sun
> >> Sent: Wednesday, February 08, 2017 1:03 AM
> >> To: Scott Wood; Y.B. Lu; u-boot at lists.denx.de
> >> Cc: Albert Aribaud; Z.Q. Hou
> >> Subject: Re: [U-Boot] [PATCH] armv8/fsl-layerscape: fdt: remove
> >> SYSCLK frequency fixup for ls1012a
> >>
> >> On 01/20/2017 05:13 PM, york sun wrote:
> >>>
> >>> Then a proper fix would be check the clock name or compatible. If
> >>> none of them exists, we should fix the device tree first.
> >>
> >> Yangbo,
> >>
> >> Can you fix the code to check clock name or compatible?
> >>
> >> York
> >
> > [Lu Yangbo-B47093] Hi York, do you mean check the clock name or
> compatible to make sure it's sysclk and then fix it?
> > Scott's patch added coreclock node also with compatible 'fixed-clock'.
> > https://patchwork.kernel.org/patch/9536515/
> >
> > If we check clock name, I found most names with 'fixed-clock'
> compatible are 'sysclk', but some are not.
> > fsl-ls1012a-frdm.dts:
> >         sys_mclk: clock-mclk {
> >                 compatible = "fixed-clock";
> >                 #clock-cells = <0>;
> >                 clock-frequency = <25000000>;
> >         };
> >
> > fsl-ls1012a-qds.dts:
> >         sys_mclk: clock-mclk {
> >                 compatible = "fixed-clock";
> >                 #clock-cells = <0>;
> >                 clock-frequency = <24576000>;
> >         };
> >
> 
> Clearly "fixed-clock" is not a good compatible string to search for. It
> just tells you this clock is fixed in frequency. It doesn't tell you if a
> clock is system clock.
> 
> Can you find this clock by its name? If you need to unify the device tree,
> it may be the time now.

[Lu Yangbo-B47093] Sorry for my late response. I sent out a new version patch fixing sysclock by path '/sysclk'.
This would only fix sysclock. Please help to review.

Thanks.

> 
> York