armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

PATCH New Yangbo Lu 3 participants 10 messages Jan 20, 2017 – Apr 10, 2017 Arm (32-bit)

Share & tools

Yangbo Lu

author

2017-01-20 03:20 UTC #0 lore raw

PATCH 1/1 armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

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 <[email protected]>

---
 arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 ++
 1 file changed, 2 insertions(+)

arch/arm/cpu/armv8/fsl-layerscape/fdt.c +2 -0

index c10ccf9..e59c232 100644
@@ -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);

Signature · 1 line

2.1.0.27.g96db324

york sun

2017-01-20 16:28 UTC #1 lore raw

PATCH 1/1 armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

Quoted 18 lines · 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 <[email protected]>
---
 arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 ++
 1 file changed, 2 insertions(+)
arch/arm/cpu/armv8/fsl-layerscape/fdt.c +0 -0
index c10ccf9..e59c232 100644

arch/arm/cpu/armv8/fsl-layerscape/fdt.c void ft_cpu_setup(void *blob, bd_t *bd) · line 170

Quoted 10 lines

arch/arm/cpu/armv8/fsl-layerscape/fdt.c +2 -0

@@ -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

Scott Wood

2017-01-20 21:35 UTC #2 lore raw

PATCH 1/1 armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

Quoted 30 lines · york sun

Quoted 29 lines · 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 <[email protected]>
---
?arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 ++
?1 file changed, 2 insertions(+)
arch/arm/cpu/armv8/fsl-layerscape/fdt.c +0 -0
index c10ccf9..e59c232 100644
@@ -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

Quoted 7 lines

Quoted 3 lines
?#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

york sun

2017-01-20 21:38 UTC #3 lore raw

PATCH 1/1 armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

Quoted 39 lines · Scott Wood

Quoted 30 lines · york sun
Quoted 29 lines · 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 <[email protected]>
---
 arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 ++
 1 file changed, 2 insertions(+)
arch/arm/cpu/armv8/fsl-layerscape/fdt.c +2 -0
index c10ccf9..e59c232 100644
@@ -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
 
Quoted 7 lines
Quoted 3 lines
#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.

Quoted 8 lines

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

Scott Wood

2017-01-20 22:13 UTC #4 lore raw

PATCH 1/1 armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

Quoted 50 lines · york sun

Quoted 49 lines · Scott Wood
Quoted 36 lines · york sun
Quoted 34 lines · 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 <[email protected]>
---
?arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 2 ++
?1 file changed, 2 insertions(+)
arch/arm/cpu/armv8/fsl-layerscape/fdt.c +0 -0
index c10ccf9..e59c232 100644
@@ -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
Quoted 7 lines
Quoted 3 lines
?#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.

Quoted 9 lines

Quoted 6 lines
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

york sun

2017-01-20 22:40 UTC #5 lore raw

PATCH 1/1 armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

Quoted 5 lines · Scott Wood

Quoted 4 lines · york sun
Quoted 3 lines · Scott Wood
Quoted 1 line · york sun

<snip>

Quoted 13 lines

Quoted 13 lines
Quoted 13 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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

york sun

2017-02-07 17:02 UTC #6 lore raw

PATCH 1/1 armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

Quoted 3 lines · york sun

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

Y.B. Lu

author

2017-02-08 03:30 UTC #7 lore raw

PATCH 1/1 armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

Quoted 10 lines

-----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
Quoted 1 line · york sun

Quoted 8 lines

Quoted 2 lines
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.

york sun

2017-02-08 04:26 UTC #8 lore raw

PATCH 1/1 armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

Quoted 31 lines · Y.B. Lu

Quoted 10 lines
-----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
Quoted 1 line · york sun
Quoted 8 lines
Quoted 2 lines
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>;
        };

Quoted 7 lines

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

Y.B. Lu

author

2017-04-10 07:20 UTC #9 lore raw

PATCH 1/1 armv8/fsl-layerscape: fdt: remove SYSCLK frequency fixup for ls1012a

Hi York,

Quoted 50 lines

-----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
Quoted 13 lines · Y.B. Lu
Quoted 13 lines
-----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
Quoted 3 lines · york sun
Then a proper fix would be check the clock name or compatible. If none of them exists, we should fix the device tree first.
Quoted 7 lines
Quoted 5 lines
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?
Quoted 4 lines
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.
Quoted 7 lines
fsl-ls1012a-frdm.dts:
        sys_mclk: clock-mclk {
                compatible = "fixed-clock";
                #clock-cells = <0>;
                clock-frequency = <25000000>;
        };
Quoted 7 lines
fsl-ls1012a-qds.dts:
        sys_mclk: clock-mclk {
                compatible = "fixed-clock";
                #clock-cells = <0>;
                clock-frequency = <24576000>;
        };

Quoted 6 lines

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.

Quoted 2 lines

York