| Message ID | 1484882453-20076-1-git-send-email-yangbo.lu@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | York Sun |
| Headers | show |
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
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
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
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
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
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
> -----Original Message----- > From: york sun > Sent: Wednesday, February 08, 2017 1:03 AM > To: Scott Wood; Y.B. Lu; u-boot@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.
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@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
Hi York, > -----Original Message----- > From: york sun > Sent: Wednesday, February 08, 2017 12:27 PM > To: Y.B. Lu; Scott Wood; u-boot@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@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
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);
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(+)