smp_twd fix for adapting to cpu frequency change

smp_twd fix for adapting to cpu frequency change

[shiraz hashim]

Hi Linus,

In your following patch,

    commit 4fd7f9b128107034fa925b6877fae3c275f0da86
    Author: Linus Walleij <linus.walleij@linaro.org>
    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 ?

-- 
regards
Shiraz Hashim

smp_twd fix for adapting to cpu frequency change

[Linus Walleij]

On Thu, May 3, 2012 at 1:15 PM, shiraz hashim
<shiraz.linux.kernel@gmail.com> wrote:

> In your following patch,
>
> ? ?commit 4fd7f9b128107034fa925b6877fae3c275f0da86
> ? ?Author: Linus Walleij <linus.walleij@linaro.org>
> ? ?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 <linus.walleij@linaro.org>
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 <shiraz.linux.kernel@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 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

smp_twd fix for adapting to cpu frequency change

[Thomas Gleixner]

On Tue, 8 May 2012, Linus Walleij wrote:
> 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?

In principle yes, but this should be really done from the core code
with an extra callback, eg. update_freq(), which is then called from
clockevents_update_freq() in periodic mode.

Thanks,

	tglx

smp_twd fix for adapting to cpu frequency change

[Shiraz Hashim]

Hi Linus,

On Tue, May 08, 2012 at 07:33:56PM +0800, Linus Walleij wrote:
> Can you test this snippet?
> Thomas: does this look sane?
> 
> >From d40c3c3302a2f89ab973b41b350153c144f6bded Mon Sep 17 00:00:00 2001
> From: Linus Walleij <linus.walleij@linaro.org>
> 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 <shiraz.linux.kernel@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  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,
> -- 

Thanks, it works.

--
regards
Shiraz

smp_twd fix for adapting to cpu frequency change

[Linus Walleij]

On Fri, May 11, 2012 at 11:21 AM, Shiraz Hashim <shiraz.hashim@st.com> wrote:

> Thanks, it works.

OK I take it that I can add your Tested-by: tag on this then :-)

Thomas does it look OK to you too? If I get your ACK I can put this
into Russell's patch tracker.

Yours,
Linus Walleij

smp_twd fix for adapting to cpu frequency change

[Viresh Kumar]

On Tue, May 8, 2012 at 5:03 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, May 3, 2012 at 1:15 PM, shiraz hashim
> <shiraz.linux.kernel@gmail.com> wrote:
>
>> In your following patch,
>>
>>    commit 4fd7f9b128107034fa925b6877fae3c275f0da86
>>    Author: Linus Walleij <linus.walleij@linaro.org>
>>    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 <linus.walleij@linaro.org>
> 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 <shiraz.linux.kernel@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  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,

Linus,

Did you ever fixed it up? I think Mason has hit this now:

http://www.spinics.net/lists/arm-kernel/msg417617.html

smp_twd fix for adapting to cpu frequency change

[Russell King - ARM Linux]

On Thu, May 14, 2015 at 08:14:43PM +0530, Viresh Kumar wrote:
> On Tue, May 8, 2012 at 5:03 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Thu, May 3, 2012 at 1:15 PM, shiraz hashim
> > <shiraz.linux.kernel@gmail.com> wrote:
> >
> >> In your following patch,
> >>
> >>    commit 4fd7f9b128107034fa925b6877fae3c275f0da86
> >>    Author: Linus Walleij <linus.walleij@linaro.org>
> >>    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().

This is not correct.

int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
{
        clockevents_config(dev, freq);

        if (dev->state == CLOCK_EVT_STATE_ONESHOT)
                return clockevents_program_event(dev, dev->next_event, false);

        if (dev->state == CLOCK_EVT_STATE_PERIODIC)
                return __clockevents_set_state(dev, CLOCK_EVT_STATE_PERIODIC);

The last two lines re-set the mode to periodic, which cause the ->set_mode
(or its replacement) to be called.  When the driver's ->set_mode method is
called, it is supposed to write the periodic timeout to the hardware.

So, this has the side effect of updating the hardware with the new timeout
value.

The downside to this is that if you keep changing the clock rate before
the TWD expires, you'll never see an interrupt from it, as you'll always
re-start a new period from the beginning on every clock frequency change.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

smp_twd fix for adapting to cpu frequency change

[Viresh Kumar]

On 14-05-15, 15:48, Russell King - ARM Linux wrote:
> This is not correct.
> 
> int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
> {
>         clockevents_config(dev, freq);
> 
>         if (dev->state == CLOCK_EVT_STATE_ONESHOT)
>                 return clockevents_program_event(dev, dev->next_event, false);
> 
>         if (dev->state == CLOCK_EVT_STATE_PERIODIC)
>                 return __clockevents_set_state(dev, CLOCK_EVT_STATE_PERIODIC);
> 
> The last two lines re-set the mode to periodic, which cause the ->set_mode
> (or its replacement) to be called.  When the driver's ->set_mode method is
> called, it is supposed to write the periodic timeout to the hardware.
> 
> So, this has the side effect of updating the hardware with the new timeout
> value.
> 
> The downside to this is that if you keep changing the clock rate before
> the TWD expires, you'll never see an interrupt from it, as you'll always
> re-start a new period from the beginning on every clock frequency change.

Ahh, this was correct when it was discussed earlier.

Commit fe79a9ba1196 ("clockevents: Adjust timer interval when
frequency changes"), fixed this in clockevents core.

-- 
viresh