[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Alexander Kochetkov]

Hello,

I found what arm-global-timer doesn't update clockevent's frequency
after cpufreq change. So I've backported two patches from
local-timer (smp_twd) to implement clockevent frequency adjustment.

Alexander Kochetkov (1):
  clocksource/arm_global_timer: reconfigure clockevents after cpufreq
    change

 drivers/clocksource/arm_global_timer.c |   93 +++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Alexander Kochetkov]

After a cpufreq transition, update the clockevent's frequency
by fetching the new clock rate from the clock framework and
reprogram the next clock event.

The clock supplying the arm-global-timer on the rk3188 is coming
from the the cpu clock itself and thus changes its rate everytime
cpufreq adjusts the cpu frequency.

Found by code review, real impact not known. Assume what actual
HZ value will be different from expected on platforms using
arm-global-timer as clockevent.

The patch is port of commit 4fd7f9b12810 ("ARM: 7212/1: smp_twd:
reconfigure clockevents after cpufreq change") and
commit 2b25d9f64b54 ("ARM: 7535/1: Reprogram smp_twd based on
new common clk framework notifiers").

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/arm_global_timer.c |   93 +++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index 8da0329..55addeb 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -49,6 +49,7 @@
  * the units for all operations.
  */
 static void __iomem *gt_base;
+static struct clk *gt_clk;
 static unsigned long gt_clk_rate;
 static int gt_ppi;
 static struct clock_event_device __percpu *gt_evt;
@@ -137,6 +138,97 @@ static int gt_clockevent_set_next_event(unsigned long evt,
 	return 0;
 }
 
+#ifdef CONFIG_COMMON_CLK
+
+/*
+ * Updates clockevent frequency when the cpu frequency changes.
+ * Called on the cpu that is changing frequency with interrupts disabled.
+ */
+static void gt_update_frequency(void *new_rate)
+{
+	gt_clk_rate = *((unsigned long *) new_rate);
+
+	clockevents_update_freq(raw_cpu_ptr(gt_evt), gt_clk_rate);
+}
+
+static int gt_rate_change(struct notifier_block *nb,
+	unsigned long flags, void *data)
+{
+	struct clk_notifier_data *cnd = data;
+
+	/*
+	 * The gt clock events must be reprogrammed to account for the new
+	 * frequency.  The timer is local to a cpu, so cross-call to the
+	 * changing cpu.
+	 */
+	if (flags == POST_RATE_CHANGE)
+		on_each_cpu(gt_update_frequency,
+				  (void *)&cnd->new_rate, 1);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block gt_clk_nb = {
+	.notifier_call = gt_rate_change,
+};
+
+static int gt_clk_init(void)
+{
+	if (gt_evt && raw_cpu_ptr(gt_evt) && !IS_ERR(gt_clk))
+		return clk_notifier_register(gt_clk, &gt_clk_nb);
+
+	return 0;
+}
+core_initcall(gt_clk_init);
+
+#elif defined (CONFIG_CPU_FREQ)
+
+#include <linux/cpufreq.h>
+
+/*
+ * Updates clockevent frequency when the cpu frequency changes.
+ * Called on the cpu that is changing frequency with interrupts disabled.
+ */
+static void gt_update_frequency(void *data)
+{
+	gt_clk_rate = clk_get_rate(gt_clk);
+
+	clockevents_update_freq(raw_cpu_ptr(gt_evt), gt_clk_rate);
+}
+
+static int gt_cpufreq_transition(struct notifier_block *nb,
+	unsigned long state, void *data)
+{
+	struct cpufreq_freqs *freqs = data;
+
+	/*
+	 * The gt clock events must be reprogrammed to account for the new
+	 * frequency.  The timer is local to a cpu, so cross-call to the
+	 * changing cpu.
+	 */
+	if (state == CPUFREQ_POSTCHANGE)
+		smp_call_function_single(freqs->cpu, gt_update_frequency,
+			NULL, 1);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block gt_cpufreq_nb = {
+	.notifier_call = gt_cpufreq_transition,
+};
+
+static int gt_cpufreq_init(void)
+{
+	if (gt_evt && raw_cpu_ptr(gt_evt) && !IS_ERR(gt_clk))
+		return cpufreq_register_notifier(&gt_cpufreq_nb,
+			CPUFREQ_TRANSITION_NOTIFIER);
+
+	return 0;
+}
+core_initcall(gt_cpufreq_init);
+
+#endif
+
 static irqreturn_t gt_clockevent_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *evt = dev_id;
@@ -257,7 +349,6 @@ static int __init gt_clocksource_init(void)
 
 static int __init global_timer_of_register(struct device_node *np)
 {
-	struct clk *gt_clk;
 	int err = 0;
 
 	/*
-- 
1.7.9.5

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Thomas Gleixner]

On Tue, 29 Nov 2016, Alexander Kochetkov wrote:

> After a cpufreq transition, update the clockevent's frequency
> by fetching the new clock rate from the clock framework and
> reprogram the next clock event.

The frequency change would not only affect the clockevent device, it also
would affect the clocksource. So the patch is incomplete, but see below.

> The clock supplying the arm-global-timer on the rk3188 is coming
> from the the cpu clock itself and thus changes its rate everytime
> cpufreq adjusts the cpu frequency.

That's broken and the clk framework should keep the CORE_PERI clock at a
constant rate by reprogramming the divider of the CPU clock.

> Found by code review, real impact not known. Assume what actual
> HZ value will be different from expected on platforms using
> arm-global-timer as clockevent.

Assumptions w/o real impact are a perfect reason not to apply that
patch. This want's a proper proof that the global timer really changes and
this hackery is required, which I seriously doubt.

Thanks,

	tglx

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Alexander Kochetkov]

Hello Thomas!

> 29 ????. 2016 ?., ? 16:42, Thomas Gleixner <tglx@linutronix.de> ???????(?):
> 
> The frequency change would not only affect the clockevent device, it also
> would affect the clocksource. So the patch is incomplete, but see below.
Looks like kernel disallow change clocksource and shed_clock rate at
runtime. I haven?t found any function for that.

> 29 ????. 2016 ?., ? 16:42, Thomas Gleixner <tglx@linutronix.de> ???????(?):
> 
> That's broken and the clk framework should keep the CORE_PERI clock at a
> constant rate by reprogramming the divider of the CPU clock.

Thank you for the hint.

> Assumptions w/o real impact are a perfect reason not to apply that
> patch. This want's a proper proof that the global timer really changes and
> this hackery is required, which I seriously doubt.

It really changes. It changes like it changes for smp-twd. I guess they are
driven by same clock CORE_PERI.

Thank you for help.

Alexander.

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Thomas Gleixner]

On Tue, 29 Nov 2016, Alexander Kochetkov wrote:
> > 29 ????. 2016 ?., ? 16:42, Thomas Gleixner <tglx@linutronix.de> ???????(?):
> > 
> > The frequency change would not only affect the clockevent device, it also
> > would affect the clocksource. So the patch is incomplete, but see below.
> Looks like kernel disallow change clocksource and shed_clock rate at
> runtime. I haven?t found any function for that.
> 
> > 29 ????. 2016 ?., ? 16:42, Thomas Gleixner <tglx@linutronix.de> ???????(?):
> > 
> > That's broken and the clk framework should keep the CORE_PERI clock at a
> > constant rate by reprogramming the divider of the CPU clock.
> 
> Thank you for the hint.
> 
> > Assumptions w/o real impact are a perfect reason not to apply that
> > patch. This want's a proper proof that the global timer really changes and
> > this hackery is required, which I seriously doubt.
> 
> It really changes. It changes like it changes for smp-twd. I guess they are
> driven by same clock CORE_PERI.

If that happens then the whole thing wants to be disabled on those
misconfigured systems and not just hacked for nothing.

Thanks,

	tglx

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Marc Zyngier]

On 29/11/16 13:42, Thomas Gleixner wrote:
> On Tue, 29 Nov 2016, Alexander Kochetkov wrote:
> 
>> After a cpufreq transition, update the clockevent's frequency
>> by fetching the new clock rate from the clock framework and
>> reprogram the next clock event.
> 
> The frequency change would not only affect the clockevent device, it also
> would affect the clocksource. So the patch is incomplete, but see below.
> 
>> The clock supplying the arm-global-timer on the rk3188 is coming
>> from the the cpu clock itself and thus changes its rate everytime
>> cpufreq adjusts the cpu frequency.
> 
> That's broken and the clk framework should keep the CORE_PERI clock at a
> constant rate by reprogramming the divider of the CPU clock.
> 
>> Found by code review, real impact not known. Assume what actual
>> HZ value will be different from expected on platforms using
>> arm-global-timer as clockevent.
> 
> Assumptions w/o real impact are a perfect reason not to apply that
> patch. This want's a proper proof that the global timer really changes and
> this hackery is required, which I seriously doubt.

Well, let's not underestimate the "creativity" [1] of A5/A9 when it
comes to the timer clocks, and it is a very sad fact that both the
global timer and the local timers are clocked by PERIPHCLK, which is
ticking at a fixed ratio N (N >= 2) of the main CPU clock (CLK):

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407f/CIHGECHJ.html

I'm not sure how feasible it is to change this ratio (the TRM seems to
be very silent on the subject). So short of being able to reconfigure it
on the fly, this will probably need some surgery similar to what we
already do for the TWD (which this patch mimics).

Thankfully, we don't see that anymore on moderately recent HW (anything
since A15) and the advent of the arch timer, which is guaranteed to have
a fixed frequency.

Thanks,

	M.

[1] I wanted to write something else, but common decency prevents me
from being more explicit...
-- 
Jazz is not dead. It just smells funny...

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Thomas Gleixner]

On Tue, 29 Nov 2016, Marc Zyngier wrote:
> On 29/11/16 13:42, Thomas Gleixner wrote:
> > Assumptions w/o real impact are a perfect reason not to apply that
> > patch. This want's a proper proof that the global timer really changes and
> > this hackery is required, which I seriously doubt.
> 
> Well, let's not underestimate the "creativity" [1] of A5/A9 when it
> comes to the timer clocks, and it is a very sad fact that both the
> global timer and the local timers are clocked by PERIPHCLK, which is
> ticking at a fixed ratio N (N >= 2) of the main CPU clock (CLK):
> 
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407f/CIHGECHJ.html
> 
> I'm not sure how feasible it is to change this ratio (the TRM seems to
> be very silent on the subject).

The CRU documentation of the RK3188 suggests that you can adjust it as it
has a seperate divider, but who knows.

> So short of being able to reconfigure it on the fly, this will probably
> need some surgery similar to what we already do for the TWD (which this
> patch mimics).
>
> Thankfully, we don't see that anymore on moderately recent HW (anything
> since A15) and the advent of the arch timer, which is guaranteed to have
> a fixed frequency.

Can we just disable that global timer on affected SoCs and use something
else instead?

Thanks,

	tglx

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Marc Zyngier]

On 29/11/16 14:32, Thomas Gleixner wrote:
> On Tue, 29 Nov 2016, Marc Zyngier wrote:
>> On 29/11/16 13:42, Thomas Gleixner wrote:
>>> Assumptions w/o real impact are a perfect reason not to apply that
>>> patch. This want's a proper proof that the global timer really changes and
>>> this hackery is required, which I seriously doubt.
>>
>> Well, let's not underestimate the "creativity" [1] of A5/A9 when it
>> comes to the timer clocks, and it is a very sad fact that both the
>> global timer and the local timers are clocked by PERIPHCLK, which is
>> ticking at a fixed ratio N (N >= 2) of the main CPU clock (CLK):
>>
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407f/CIHGECHJ.html
>>
>> I'm not sure how feasible it is to change this ratio (the TRM seems to
>> be very silent on the subject).
> 
> The CRU documentation of the RK3188 suggests that you can adjust it as it
> has a seperate divider, but who knows.
> 
>> So short of being able to reconfigure it on the fly, this will probably
>> need some surgery similar to what we already do for the TWD (which this
>> patch mimics).
>>
>> Thankfully, we don't see that anymore on moderately recent HW (anything
>> since A15) and the advent of the arch timer, which is guaranteed to have
>> a fixed frequency.
> 
> Can we just disable that global timer on affected SoCs and use something
> else instead?

That'd be my preferred course of action. I've located some documentation
over there [1], and page 1126 seems to indicate a profusion of
additional timers, some of which are in an always-on domain. Seems like
a much better use of someone's time...

Thanks,

	M.

[1] http://rockchip.fr/Rockchip%20RK3188%20TRM%20V1.3.pdf
-- 
Jazz is not dead. It just smells funny...

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Alexander Kochetkov]

> 29 ????. 2016 ?., ? 17:51, Marc Zyngier <marc.zyngier@arm.com> ???????(?):
> 
> That'd be my preferred course of action. I've located some documentation
> over there [1], and page 1126 seems to indicate a profusion of
> additional timers, some of which are in an always-on domain. Seems like
> a much better use of someone's time...

Thank you very much :)
And thank you for great explanation of core problem.

In general, I no longer insist on the inclusion the patch in the kernel.
Thank you guys for the help.

Alexander.

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Alexander Kochetkov]

> 29 ????. 2016 ?., ? 17:32, Thomas Gleixner <tglx@linutronix.de> ???????(?):
> 
> Can we just disable that global timer on affected SoCs and use something
> else instead?

I?ve sent patch series for fixing that on rockchip SoC.
http://lists.infradead.org/pipermail/linux-rockchip/2016-November/013217.html

But the series contain fix only for rk3188, because I don?t have another rockchip
SoC. rk3288 and other could be easy fixed with dts files.

There are a lot of other platforms what probably use shed_clock and
clocksource form global-timer.
 
alexander at ubuntu:dts$ grep arm,cortex-a9-global-timer *
am4372.dtsi:		compatible = "arm,cortex-a9-global-timer";
artpec6.dtsi:		compatible = "arm,cortex-a9-global-timer";
bcm5301x.dtsi:			compatible = "arm,cortex-a9-global-timer";
bcm63138.dtsi:			compatible = "arm,cortex-a9-global-timer";
bcm-cygnus.dtsi:			compatible = "arm,cortex-a9-global-timer";
bcm-nsp.dtsi:			compatible = "arm,cortex-a9-global-timer";
hip01.dtsi:			compatible = "arm,cortex-a9-global-timer";
rk3xxx.dtsi:		compatible = "arm,cortex-a9-global-timer";
stih407-family.dtsi:		compatible = "arm,cortex-a9-global-timer";
stih41x.dtsi:		compatible = "arm,cortex-a9-global-timer";
uniphier-common32.dtsi:			compatible = "arm,cortex-a9-global-timer";
uniphier-ph1-sld3.dtsi:			compatible = "arm,cortex-a9-global-timer";
vexpress-v2p-ca5s.dts:		             "arm,cortex-a9-global-timer";
vf500.dtsi:				compatible = "arm,cortex-a9-global-timer";
zx296702.dtsi:			compatible = "arm,cortex-a9-global-timer";
zynq-7000.dtsi:			compatible = "arm,cortex-a9-global-timer?;

Regards,
Alexander.

[PATCH] clocksource/arm_global_timer: reconfigure clockevents after cpufreq change

[Robin Murphy]

On 29/11/16 14:51, Alexander Kochetkov wrote:
> 
>> 29 ????. 2016 ?., ? 17:32, Thomas Gleixner <tglx@linutronix.de> ???????(?):
>>
>> Can we just disable that global timer on affected SoCs and use something
>> else instead?
> 
> I?ve sent patch series for fixing that on rockchip SoC.
> http://lists.infradead.org/pipermail/linux-rockchip/2016-November/013217.html
> 
> But the series contain fix only for rk3188, because I don?t have another rockchip
> SoC. rk3288 and other could be easy fixed with dts files.

3288 (and probably anything newer) is irrelevant to this discussion, as
it has the arch timer interface - that may be busted in other ways (such
as not being correctly set up by firmware and not being always-on as it
should), but frequency is not one of them. This only affects
Cortex-A9/A5 based parts.

> There are a lot of other platforms what probably use shed_clock and
> clocksource form global-timer.

Presumably it's only an issue if they also have cpufreq?

> alexander at ubuntu:dts$ grep arm,cortex-a9-global-timer *
> am4372.dtsi:		compatible = "arm,cortex-a9-global-timer";
> artpec6.dtsi:		compatible = "arm,cortex-a9-global-timer";
> bcm5301x.dtsi:			compatible = "arm,cortex-a9-global-timer";
> bcm63138.dtsi:			compatible = "arm,cortex-a9-global-timer";
> bcm-cygnus.dtsi:			compatible = "arm,cortex-a9-global-timer";
> bcm-nsp.dtsi:			compatible = "arm,cortex-a9-global-timer";
> hip01.dtsi:			compatible = "arm,cortex-a9-global-timer";
> rk3xxx.dtsi:		compatible = "arm,cortex-a9-global-timer";
> stih407-family.dtsi:		compatible = "arm,cortex-a9-global-timer";
> stih41x.dtsi:		compatible = "arm,cortex-a9-global-timer";
> uniphier-common32.dtsi:			compatible = "arm,cortex-a9-global-timer";
> uniphier-ph1-sld3.dtsi:			compatible = "arm,cortex-a9-global-timer";
> vexpress-v2p-ca5s.dts:		             "arm,cortex-a9-global-timer";

I can tell you that one, for one, is never used, because it depends on
an input clock provided by the vexpress-osc driver which cannot be
probed sufficiently early.

Robin.

> vf500.dtsi:				compatible = "arm,cortex-a9-global-timer";
> zx296702.dtsi:			compatible = "arm,cortex-a9-global-timer";
> zynq-7000.dtsi:			compatible = "arm,cortex-a9-global-timer?;
> 
> Regards,
> Alexander.
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>