[PATCH] cpuidle : handle clockevent notify from the cpuidle framework

[PATCH] cpuidle : handle clockevent notify from the cpuidle framework

[Daniel Lezcano]

When a cpu enters a deep idle state, the local timers are stopped and
the time framework falls back to the timer device used as a broadcast
timer.

The different cpuidle drivers are calling clockevents_notify ENTER/EXIT
when the idle state stops the local timer.

The proposed patch introduces a new flag CPUIDLE_FLAG_TIMER_STOP to let
the cpuidle framework to call clockevents_notify instead of duplicating
again and again these lines in all the cpuidle drivers.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle.c |    9 +++++++++
 include/linux/cpuidle.h   |    1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index eba6929..c500370 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -8,6 +8,7 @@
  * This code is licenced under the GPL.
  */
 
+#include <linux/clockchips.h>
 #include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/sched.h>
@@ -146,12 +147,20 @@ int cpuidle_idle_call(void)
 
 	trace_cpu_idle_rcuidle(next_state, dev->cpu);
 
+	if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP)
+		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
+				   &dev->cpu);
+
 	if (cpuidle_state_is_coupled(dev, drv, next_state))
 		entered_state = cpuidle_enter_state_coupled(dev, drv,
 							    next_state);
 	else
 		entered_state = cpuidle_enter_state(dev, drv, next_state);
 
+	if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP)
+		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
+				   &dev->cpu);
+
 	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
 
 	/* give the governor an opportunity to reflect on the outcome */
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 480c14d..a837b33 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -57,6 +57,7 @@ struct cpuidle_state {
 /* Idle State Flags */
 #define CPUIDLE_FLAG_TIME_VALID	(0x01) /* is residency time measurable? */
 #define CPUIDLE_FLAG_COUPLED	(0x02) /* state applies to multiple cpus */
+#define CPUIDLE_FLAG_TIMER_STOP (0x04)  /* timer is stopped on this state */
 
 #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
 
-- 
1.7.9.5

Re: [PATCH] cpuidle : handle clockevent notify from the cpuidle framework

[Thomas Gleixner]

On Wed, 20 Mar 2013, Daniel Lezcano wrote:

> When a cpu enters a deep idle state, the local timers are stopped and
> the time framework falls back to the timer device used as a broadcast
> timer.
> 
> The different cpuidle drivers are calling clockevents_notify ENTER/EXIT
> when the idle state stops the local timer.
> 
> The proposed patch introduces a new flag CPUIDLE_FLAG_TIMER_STOP to let

Please stop using "proposed patch ...." wording in a patch
description. The changelog you submit with your patch should be
applicable w/o rewriting.

> the cpuidle framework to call clockevents_notify instead of duplicating
> again and again these lines in all the cpuidle drivers.

That's a good enough reason, really. So that paragraph should be
something like:

"Add a new flag CPUIDLE_FLAG_TIMER_STOP which can be set by cpuidle
 drivers. If the flag is set the cpuidle core code takes care of the
 notification on behalf of the driver to avoid pointless code
 duplication."

Ideally you would follow up with two or three drivers converted to
that new infrastructure instead of sending that patch standalone. That
way reviewers can really see the benefit in terms of reduced code
duplication.
 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Other than the above rant:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH] cpuidle : handle clockevent notify from the cpuidle framework

[Daniel Lezcano]

On 03/20/2013 10:21 PM, Thomas Gleixner wrote:
> On Wed, 20 Mar 2013, Daniel Lezcano wrote:
> 
>> When a cpu enters a deep idle state, the local timers are stopped and
>> the time framework falls back to the timer device used as a broadcast
>> timer.
>>
>> The different cpuidle drivers are calling clockevents_notify ENTER/EXIT
>> when the idle state stops the local timer.
>>
>> The proposed patch introduces a new flag CPUIDLE_FLAG_TIMER_STOP to let
> 
> Please stop using "proposed patch ...." wording in a patch
> description. The changelog you submit with your patch should be
> applicable w/o rewriting.

Oh, yes. Thanks for pointing this.

>> the cpuidle framework to call clockevents_notify instead of duplicating
>> again and again these lines in all the cpuidle drivers.
> 
> That's a good enough reason, really. So that paragraph should be
> something like:
> 
> "Add a new flag CPUIDLE_FLAG_TIMER_STOP which can be set by cpuidle
>  drivers. If the flag is set the cpuidle core code takes care of the
>  notification on behalf of the driver to avoid pointless code
>  duplication."
> 
> Ideally you would follow up with two or three drivers converted to
> that new infrastructure instead of sending that patch standalone. That
> way reviewers can really see the benefit in terms of reduced code
> duplication.

Thanks for your review. I will resend this patch with a better log and a
couple of patches on top using this flag.

 -- Daniel

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog