From mboxrd@z Thu Jan 1 00:00:00 1970 From: linus.walleij@linaro.org (Linus Walleij) Date: Tue, 8 May 2012 13:33:56 +0200 Subject: smp_twd fix for adapting to cpu frequency change In-Reply-To: References: Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, May 3, 2012 at 1:15 PM, shiraz hashim wrote: > In your following patch, > > ? ?commit 4fd7f9b128107034fa925b6877fae3c275f0da86 > ? ?Author: Linus Walleij > ? ?Date: ? Tue Dec 13 12:48:18 2011 +0100 > > ? ? ? ?ARM: 7212/1: smp_twd: reconfigure clockevents after cpufreq change > > ? ? ? ?This break-out from Colin Cross' cpufreq-aware TWD patch > ? ? ? ?will handle the case when our localtimer's clock changes with > ? ? ? ?the cpu clock. A cpufreq transtion notifier will be registered > ? ? ? ?only if the platform has supplied a specified clock to the TWD. > > ? ? ? ?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 necessary changes in the clockevents framework was done by > ? ? ? ?Thomas Gleixner in kernel v3.0. > > > When we handle twd_cpufreq_transition and reprogram the clock event, > the TWD_TIMER_LOAD register still contains the old load value > for CLOCK_EVT_MODE_PERIODIC case. > > This results in wrong number of events generated per second. > > Shouldn't we reprogram the TWD_TIMER_LOAD register to new > twd_timer_rate / HZ on frequency change as well ? Yep the clockevents_update_freq() is explicitly only for the on-shot mode, so you'd either need to write the new period value directly in twd_update_frequency(). I guess we didn't notice because almost noone use the periodic mode on these timers... I guess clockevents_update_freq() could just call the .set_mode() function for periodic mode again, but that seems a bit ugly, since the modeset code might do other things than just reinitialize the timer. And it won't account for the running event. So this solution will try to do what you describe, but I'm still a bit uncertain, since the currently running event will probably use the old load value, then the new value won't get used until the next event. Maybe that's fair enough? I don't know it it's OK for driver code to inspect the internal clockevent mode like this code does though, maybe Thomas has opinions on this... Can you test this snippet? Thomas: does this look sane? >>From d40c3c3302a2f89ab973b41b350153c144f6bded Mon Sep 17 00:00:00 2001 From: Linus Walleij Date: Tue, 8 May 2012 13:26:43 +0200 Subject: [PATCH] ARM: smp_twd: reprogram loadvalue for periodic event The code to handle frequency transitions of the TWD only handle one-shot events. Let's try to account for this by checking the state of the event. Reported-by: Shiraz Hashim Signed-off-by: Linus Walleij --- arch/arm/kernel/smp_twd.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c index fef42b2..98b27e6 100644 --- a/arch/arm/kernel/smp_twd.c +++ b/arch/arm/kernel/smp_twd.c @@ -104,9 +104,17 @@ static void twd_timer_stop(struct clock_event_device *clk) */ static void twd_update_frequency(void *data) { + struct clock_event_device *evt = *__this_cpu_ptr(twd_evt); + twd_timer_rate = clk_get_rate(twd_clk); - clockevents_update_freq(*__this_cpu_ptr(twd_evt), twd_timer_rate); + /* If we're in periodic mode, just put in a new load value */ + if (evt->mode == CLOCK_EVT_MODE_PERIODIC) { + __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD); + return; + } + + clockevents_update_freq(evt, twd_timer_rate); } static int twd_cpufreq_transition(struct notifier_block *nb, -- 1.7.9.2 Yours, Linus Walleij