Skip to contentSkip to conversation

arm: zynq: Fix system clock with multi_v7_defconfig

PATCH Superseded Ola Jeppsson 4 participants 11 messages Mar 23, 2015 – Apr 10, 2015

Subsystems (3)
Arm (32-bit) Arm sub-architectures Arm/Zynq architecture
lore
Share & tools
lore

Ola Jeppsson

author
#0 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

As mentioned in this commit:
arm: zynq: Don't use arm_global_timer with cpufreq 61f1fc7e9258a169ac8afb5ddf657a181e60d052

arm_global_timer depends on the CPU frequency. With cpufreq altering the CPU frequency arm_global_timer will not maintain a stable time base. So arm_global_timer must not be the clocksource when cpufreq is enabled.

The above commit tries to solve this at build time by only selecting CONFIG_ARM_GLOBAL_TIMER if CONFIG_CPU_FREQ is disabled. This is not always sufficient because other machs can also enable CONFIG_ARM_GLOBAL_TIMER.

Therefore: If built with CONFIG_CPU_FREQ and CONFIG_ARM_GLOBAL_TIMER, disable (on Zynq) the arm_global_timer devicetree node at boot before clock sources are initialized. This ensures that arm_global_timer will not be selected clocksource.

Signed-off-by Ola Jeppsson <[email protected]>
---
 arch/arm/mach-zynq/common.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
arch/arm/mach-zynq/common.c +26 -0
index c887196..a4666d4 100644
@@ -148,10 +148,36 @@ out:
 	platform_device_register_full(&devinfo);
 }
 
+#if defined(CONFIG_CPU_FREQ) && defined(CONFIG_ARM_GLOBAL_TIMER)
+static struct property zynq_disable_arm_global_timer_prop = {
+	.name = "status",
+	.length = sizeof("disabled"),
+	.value = "disabled"
+};
+
+static void __init zynq_disable_arm_global_timer(void)
+{
+	struct device_node *np;
+
+	np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-global-timer");
+	if (!np)
+		return;
+
+	pr_info("%s: disabling arm_global_timer node\n", __func__);
+
+	if (of_update_property(np, &zynq_disable_arm_global_timer_prop))
+		pr_warn("%s: could not disable arm_global_timer node\n",
+			__func__);
+}
+#endif
+
 static void __init zynq_timer_init(void)
 {
 	zynq_early_slcr_init();
 
+#if defined(CONFIG_CPU_FREQ) && defined(CONFIG_ARM_GLOBAL_TIMER)
+	zynq_disable_arm_global_timer();
+#endif
 	zynq_clock_init();
 	of_clk_init(NULL);
 	clocksource_of_init();
Signature · 1 line
2.3.3
#1 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-encrypted
Size: 11 bytes
Desc: PGP/MIME version identification
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150323/aac6b168/attachment-0001.pgp>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: encrypted.asc
Type: application/octet-stream
Size: 2680 bytes
Desc: OpenPGP encrypted message
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150323/aac6b168/attachment-0001.obj>

#2 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

In reply to Ola Jeppsson

Hi,

Quoted 55 lines · Ola Jeppsson

As mentioned in this commit:
arm: zynq: Don't use arm_global_timer with cpufreq 61f1fc7e9258a169ac8afb5ddf657a181e60d052

arm_global_timer depends on the CPU frequency. With cpufreq altering the CPU frequency arm_global_timer will not maintain a stable time base. So arm_global_timer must not be the clocksource when cpufreq is enabled.

The above commit tries to solve this at build time by only selecting CONFIG_ARM_GLOBAL_TIMER if CONFIG_CPU_FREQ is disabled. This is not always sufficient because other machs can also enable CONFIG_ARM_GLOBAL_TIMER.

Therefore: If built with CONFIG_CPU_FREQ and CONFIG_ARM_GLOBAL_TIMER, disable (on Zynq) the arm_global_timer devicetree node at boot before clock sources are initialized. This ensures that arm_global_timer will not be selected clocksource.

Signed-off-by Ola Jeppsson <[email protected]>
---
 arch/arm/mach-zynq/common.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
arch/arm/mach-zynq/common.c +23 -0
index c887196..a4666d4 100644
@@ -148,10 +148,36 @@ out:
 	platform_device_register_full(&devinfo);
 }
 
+#if defined(CONFIG_CPU_FREQ) && defined(CONFIG_ARM_GLOBAL_TIMER)
+static struct property zynq_disable_arm_global_timer_prop = {
+	.name = "status",
+	.length = sizeof("disabled"),
+	.value = "disabled"
+};
+
+static void __init zynq_disable_arm_global_timer(void)
+{
+	struct device_node *np;
+
+	np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-global-timer");
+	if (!np)
+		return;
+
+	pr_info("%s: disabling arm_global_timer node\n", __func__);
+
+	if (of_update_property(np, &zynq_disable_arm_global_timer_prop))
+		pr_warn("%s: could not disable arm_global_timer node\n",
+			__func__);
+}
+#endif
+

arch/arm/mach-zynq/common.c out: · line 183

Quoted 10 lines
arch/arm/mach-zynq/common.c +3 -0
 static void __init zynq_timer_init(void)
 {
 	zynq_early_slcr_init();
 
+#if defined(CONFIG_CPU_FREQ) && defined(CONFIG_ARM_GLOBAL_TIMER)
+	zynq_disable_arm_global_timer();
+#endif
 	zynq_clock_init();
 	of_clk_init(NULL);
 	clocksource_of_init();

Arnd: Waiting for your thoughts on this one?
It is some sort of arch/arm/mach-mvebu/board-v7.c quirk code.

I don't think it can be done via Kconfig because it will affect others.

Thanks,
Michal

#3 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

Quoted 21 lines · On Monday 23 March 2015 08:05:45 Michal Simek wrote:

Hi,

Quoted 18 lines · Ola Jeppsson

As mentioned in this commit:
arm: zynq: Don't use arm_global_timer with cpufreq 61f1fc7e9258a169ac8afb5ddf657a181e60d052

arm_global_timer depends on the CPU frequency. With cpufreq altering the CPU frequency arm_global_timer will not maintain a stable time base. So arm_global_timer must not be the clocksource when cpufreq is enabled.

The above commit tries to solve this at build time by only selecting CONFIG_ARM_GLOBAL_TIMER if CONFIG_CPU_FREQ is disabled. This is not always sufficient because other machs can also enable CONFIG_ARM_GLOBAL_TIMER.

Therefore: If built with CONFIG_CPU_FREQ and CONFIG_ARM_GLOBAL_TIMER, disable (on Zynq) the arm_global_timer devicetree node at boot before clock sources are initialized. This ensures that arm_global_timer will not be selected clocksource.

Quoted 8 lines
Quoted 1 line
Signed-off-by Ola Jeppsson <[email protected]>

Arnd: Waiting for your thoughts on this one?
It is some sort of arch/arm/mach-mvebu/board-v7.c quirk code.

I don't think it can be done via Kconfig because it will affect others.

(catching up with old email)

I think it's ok to work around the problem like this in principle.

However, I have one concern about the way this condition is detected. At the moment, you assume that all zynq variants suffer from this problem and no other chip has it.

How would you handle the situation if a future zynq variant fixes the problem?

What if it turns out to be a more common problem and we actually want to work around it in the arm global timer implementation by dynamically adapting to CPU frequency changes? I believe some other drivers (x86 tsc?) do this. Would we be able to detect this case on zynq?

Arnd

author
#4 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

Quoted 30 lines · Arnd Bergmann
Quoted 21 lines · On Monday 23 March 2015 08:05:45 Michal Simek wrote:

Hi,

Quoted 18 lines · Ola Jeppsson

As mentioned in this commit:
arm: zynq: Don't use arm_global_timer with cpufreq 61f1fc7e9258a169ac8afb5ddf657a181e60d052

arm_global_timer depends on the CPU frequency. With cpufreq altering the CPU frequency arm_global_timer will not maintain a stable time base. So arm_global_timer must not be the clocksource when cpufreq is enabled.

The above commit tries to solve this at build time by only selecting CONFIG_ARM_GLOBAL_TIMER if CONFIG_CPU_FREQ is disabled. This is not always sufficient because other machs can also enable CONFIG_ARM_GLOBAL_TIMER.

Therefore: If built with CONFIG_CPU_FREQ and CONFIG_ARM_GLOBAL_TIMER, disable (on Zynq) the arm_global_timer devicetree node at boot before clock sources are initialized. This ensures that arm_global_timer will not be selected clocksource.

Quoted 6 lines
Quoted 1 line
Signed-off-by Ola Jeppsson <[email protected]>

Arnd: Waiting for your thoughts on this one?
It is some sort of arch/arm/mach-mvebu/board-v7.c quirk code.

I don't think it can be done via Kconfig because it will affect others.

(catching up with old email)

Quoted 8 lines

I think it's ok to work around the problem like this in principle.

However, I have one concern about the way this condition is detected. At the moment, you assume that all zynq variants suffer from this problem and no other chip has it.

How would you handle the situation if a future zynq variant fixes the problem?

The check is only applicable on Cortex A9 zynqs.
Assume for now that it is broken on all those variants. If it gets fixed on some variant add a of_machine_is_compatible() check?

Quoted 7 lines

What if it turns out to be a more common problem and we actually want to work around it in the arm global timer implementation by dynamically adapting to CPU frequency changes? I believe some other drivers (x86 tsc?) do this. Would we be able to detect this case on zynq?

Arnd

If the arm global timer implementation gets cpufreq scaling support, this check isn't needed and should be removed?
The only thing that happens if it is left in is that you end up with an inferior, but correct, clock source (which is what you already had before cpufreq support in arm-global-timer).

Thanks,
Ola

#5 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

Quoted 37 lines · On Friday 10 April 2015 17:09:01 Ola Jeppsson wrote:
Quoted 36 lines · Arnd Bergmann
Quoted 21 lines · On Monday 23 March 2015 08:05:45 Michal Simek wrote:

Hi,

Quoted 18 lines · Ola Jeppsson

As mentioned in this commit:
arm: zynq: Don't use arm_global_timer with cpufreq 61f1fc7e9258a169ac8afb5ddf657a181e60d052

arm_global_timer depends on the CPU frequency. With cpufreq altering the CPU frequency arm_global_timer will not maintain a stable time base. So arm_global_timer must not be the clocksource when cpufreq is enabled.

The above commit tries to solve this at build time by only selecting CONFIG_ARM_GLOBAL_TIMER if CONFIG_CPU_FREQ is disabled. This is not always sufficient because other machs can also enable CONFIG_ARM_GLOBAL_TIMER.

Therefore: If built with CONFIG_CPU_FREQ and CONFIG_ARM_GLOBAL_TIMER, disable (on Zynq) the arm_global_timer devicetree node at boot before clock sources are initialized. This ensures that arm_global_timer will not be selected clocksource.

Quoted 6 lines
Quoted 1 line
Signed-off-by Ola Jeppsson <[email protected]>

Arnd: Waiting for your thoughts on this one?
It is some sort of arch/arm/mach-mvebu/board-v7.c quirk code.

I don't think it can be done via Kconfig because it will affect others.

(catching up with old email)

I think it's ok to work around the problem like this in principle.

However, I have one concern about the way this condition is detected. At the moment, you assume that all zynq variants suffer from this problem and no other chip has it.

Quoted 5 lines
Quoted 2 lines

How would you handle the situation if a future zynq variant fixes the problem?

The check is only applicable on Cortex A9 zynqs.
Assume for now that it is broken on all those variants. If it gets fixed on some variant add a of_machine_is_compatible() check?

I think it would be better to avoid those if we can.

Quoted 8 lines
Quoted 6 lines

What if it turns out to be a more common problem and we actually want to work around it in the arm global timer implementation by dynamically adapting to CPU frequency changes? I believe some other drivers (x86 tsc?) do this. Would we be able to detect this case on zynq?

If the arm global timer implementation gets cpufreq scaling support, this check isn't needed and should be removed?

The timer code however would still need to find out whether to scale the frequence along with the CPU or not.

Quoted 3 lines

The only thing that happens if it is left in is that you end up with an inferior, but correct, clock source (which is what you already had before cpufreq support in arm-global-timer).

I've just had another idea: how about introducing a new compatible string for the global timer that gets used for timers that have their frequency tied to the CPU (alternatively a bool property in that node). This can be checked by the clocksource driver, which will then be able to either skip the device if cpufreq is in use, or implement a more sophisticated workaround.

This would keep the hack outside of platform code and make it immediately available to any platform with the same problem. The only zynq specific hack you need is to modify that property on old dtb files that you want to keep supporting with new kernels (if any).

Arnd

#6 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

Quoted 62 lines · Arnd Bergmann
Quoted 37 lines · On Friday 10 April 2015 17:09:01 Ola Jeppsson wrote:
Quoted 36 lines · Arnd Bergmann
Quoted 21 lines · On Monday 23 March 2015 08:05:45 Michal Simek wrote:

Hi,

Quoted 18 lines · Ola Jeppsson

As mentioned in this commit:
arm: zynq: Don't use arm_global_timer with cpufreq 61f1fc7e9258a169ac8afb5ddf657a181e60d052

arm_global_timer depends on the CPU frequency. With cpufreq altering the CPU frequency arm_global_timer will not maintain a stable time base. So arm_global_timer must not be the clocksource when cpufreq is enabled.

The above commit tries to solve this at build time by only selecting CONFIG_ARM_GLOBAL_TIMER if CONFIG_CPU_FREQ is disabled. This is not always sufficient because other machs can also enable CONFIG_ARM_GLOBAL_TIMER.

Therefore: If built with CONFIG_CPU_FREQ and CONFIG_ARM_GLOBAL_TIMER, disable (on Zynq) the arm_global_timer devicetree node at boot before clock sources are initialized. This ensures that arm_global_timer will not be selected clocksource.

Quoted 6 lines
Quoted 1 line
Signed-off-by Ola Jeppsson <[email protected]>

Arnd: Waiting for your thoughts on this one?
It is some sort of arch/arm/mach-mvebu/board-v7.c quirk code.

I don't think it can be done via Kconfig because it will affect others.

(catching up with old email)

I think it's ok to work around the problem like this in principle.

However, I have one concern about the way this condition is detected. At the moment, you assume that all zynq variants suffer from this problem and no other chip has it.

Quoted 5 lines
Quoted 2 lines

How would you handle the situation if a future zynq variant fixes the problem?

The check is only applicable on Cortex A9 zynqs.
Assume for now that it is broken on all those variants. If it gets fixed on some variant add a of_machine_is_compatible() check?

I think it would be better to avoid those if we can.

Quoted 8 lines
Quoted 6 lines

What if it turns out to be a more common problem and we actually want to work around it in the arm global timer implementation by dynamically adapting to CPU frequency changes? I believe some other drivers (x86 tsc?) do this. Would we be able to detect this case on zynq?

If the arm global timer implementation gets cpufreq scaling support, this check isn't needed and should be removed?

The timer code however would still need to find out whether to scale the frequence along with the CPU or not.

Quoted 3 lines

The only thing that happens if it is left in is that you end up with an inferior, but correct, clock source (which is what you already had before cpufreq support in arm-global-timer).

Quoted 6 lines

I've just had another idea: how about introducing a new compatible string for the global timer that gets used for timers that have their frequency tied to the CPU (alternatively a bool property in that node). This can be checked by the clocksource driver, which will then be able to either skip the device if cpufreq is in use, or implement a more sophisticated workaround.

If this is the only available clocksource, you'd still want to use it though. A bad clocksource is still better than none. But I guess that would just mean to lie in the DT.

S?ren

#7 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

Quoted 11 lines · On Friday 10 April 2015 13:41:34 S?ren Brinkmann wrote:
Quoted 6 lines · Arnd Bergmann

I've just had another idea: how about introducing a new compatible string for the global timer that gets used for timers that have their frequency tied to the CPU (alternatively a bool property in that node). This can be checked by the clocksource driver, which will then be able to either skip the device if cpufreq is in use, or implement a more sophisticated workaround.

If this is the only available clocksource, you'd still want to use it though. A bad clocksource is still better than none. But I guess that would just mean to lie in the DT.

On zynq, you always have a local time, right?

If we get a platform that has no reliable clocksource, it's probably time to implement scaling in the global timer driver.

Arnd

#8 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

Quoted 9 lines · Arnd Bergmann
Quoted 8 lines · On Friday 10 April 2015 13:41:34 S?ren Brinkmann wrote:
Quoted 6 lines · Arnd Bergmann

I've just had another idea: how about introducing a new compatible string for the global timer that gets used for timers that have their frequency tied to the CPU (alternatively a bool property in that node). This can be checked by the clocksource driver, which will then be able to either skip the device if cpufreq is in use, or implement a more sophisticated workaround.

Quoted 5 lines
Quoted 3 lines

If this is the only available clocksource, you'd still want to use it though. A bad clocksource is still better than none. But I guess that would just mean to lie in the DT.

On zynq, you always have a local time, right?

Yes, we have the cadence_ttc as clocksource.

Quoted 2 lines

If we get a platform that has no reliable clocksource, it's probably time to implement scaling in the global timer driver.

IIRC, the person submitting the driver for the arm_gt said something like that those days: https://lkml.org/lkml/2013/5/8/250
No clue what the current status of that is today though.

S?ren

#9 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

Quoted 10 lines · On Friday 10 April 2015 15:15:11 S?ren Brinkmann wrote:
Quoted 9 lines · Arnd Bergmann
Quoted 8 lines · On Friday 10 April 2015 13:41:34 S?ren Brinkmann wrote:
Quoted 6 lines · Arnd Bergmann

I've just had another idea: how about introducing a new compatible string for the global timer that gets used for timers that have their frequency tied to the CPU (alternatively a bool property in that node). This can be checked by the clocksource driver, which will then be able to either skip the device if cpufreq is in use, or implement a more sophisticated workaround.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

If this is the only available clocksource, you'd still want to use it though. A bad clocksource is still better than none. But I guess that would just mean to lie in the DT.

On zynq, you always have a local time, right?

Yes, we have the cadence_ttc as clocksource.

I was thinking of the generic ARM timer (drivers/clocksource/arm_arch_timer.c), but I forgot that this was only introduced with Cortex-A7/A15 while Zynq is based on the older Cortex-A9.

Quoted 6 lines
Quoted 2 lines

If we get a platform that has no reliable clocksource, it's probably time to implement scaling in the global timer driver.

IIRC, the person submitting the driver for the arm_gt said something like that those days: https://lkml.org/lkml/2013/5/8/250
No clue what the current status of that is today though.

I see. Well, I'm not making it your problem, since you have another timer to use, but I guess it will get fixed eventually. This kind of bug tends to get duplicated across chips from multiple vendors.

Arnd

#10 lore raw

PATCH v2 1/1 arm: zynq: Fix system clock with multi_v7_defconfig

In reply to Ola Jeppsson
Quoted 43 lines · Ola Jeppsson

As mentioned in this commit:
arm: zynq: Don't use arm_global_timer with cpufreq 61f1fc7e9258a169ac8afb5ddf657a181e60d052

arm_global_timer depends on the CPU frequency. With cpufreq altering the CPU frequency arm_global_timer will not maintain a stable time base. So arm_global_timer must not be the clocksource when cpufreq is enabled.

The above commit tries to solve this at build time by only selecting CONFIG_ARM_GLOBAL_TIMER if CONFIG_CPU_FREQ is disabled. This is not always sufficient because other machs can also enable CONFIG_ARM_GLOBAL_TIMER.

Therefore: If built with CONFIG_CPU_FREQ and CONFIG_ARM_GLOBAL_TIMER, disable (on Zynq) the arm_global_timer devicetree node at boot before clock sources are initialized. This ensures that arm_global_timer will not be selected clocksource.

Signed-off-by Ola Jeppsson <[email protected]>
---
 arch/arm/mach-zynq/common.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
arch/arm/mach-zynq/common.c +12 -0
index c887196..a4666d4 100644
@@ -148,10 +148,36 @@ out:
 	platform_device_register_full(&devinfo);
 }
 
+#if defined(CONFIG_CPU_FREQ) && defined(CONFIG_ARM_GLOBAL_TIMER)
+static struct property zynq_disable_arm_global_timer_prop = {
+	.name = "status",
+	.length = sizeof("disabled"),
+	.value = "disabled"
+};
+
+static void __init zynq_disable_arm_global_timer(void)
+{
+	struct device_node *np;
+
+	np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-global-timer");

arch/arm/mach-zynq/common.c out: · line 172

Quoted 10 lines
arch/arm/mach-zynq/common.c +10 -0
+	if (!np)
+		return;
+
+	pr_info("%s: disabling arm_global_timer node\n", __func__);
+
+	if (of_update_property(np, &zynq_disable_arm_global_timer_prop))
+		pr_warn("%s: could not disable arm_global_timer node\n",
+			__func__);
+}
+#endif

can we get an empty implementation of the function here and...

arch/arm/mach-zynq/common.c out: · line 180

Quoted 8 lines
arch/arm/mach-zynq/common.c +4 -0
+
 static void __init zynq_timer_init(void)
 {
 	zynq_early_slcr_init();
 
+#if defined(CONFIG_CPU_FREQ) && defined(CONFIG_ARM_GLOBAL_TIMER)
+	zynq_disable_arm_global_timer();
+#endif

remove #ifdefs down here, please?

Quoted 6 lines
arch/arm/mach-zynq/common.c +0 -0
 	zynq_clock_init();
 	of_clk_init(NULL);
 	clocksource_of_init();
Signature · 1 line
2.3.3

S?ren