[PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[PATCH 1/4][resend] 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.

Add a new flag CPUIDLE_FLAG_TIMER_STOP which can be set by the 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.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Len Brown <lenb@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 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

[PATCH 2/4] cpuidle / omap4 : use CPUIDLE_FLAG_TIMER_STOP flag

[Daniel Lezcano]

Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework
to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering
this state.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Len Brown <lenb@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/arm/mach-omap2/cpuidle44xx.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c
index d639aef..fe0e025 100644
--- a/arch/arm/mach-omap2/cpuidle44xx.c
+++ b/arch/arm/mach-omap2/cpuidle44xx.c
@@ -82,7 +82,6 @@ static int omap4_enter_idle_coupled(struct cpuidle_device *dev,
 			int index)
 {
 	struct omap4_idle_statedata *cx = &omap4_idle_data[index];
-	int cpu_id = smp_processor_id();
 
 	local_fiq_disable();
 
@@ -109,8 +108,6 @@ static int omap4_enter_idle_coupled(struct cpuidle_device *dev,
 		}
 	}
 
-	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu_id);
-
 	/*
 	 * Call idle CPU PM enter notifier chain so that
 	 * VFP and per CPU interrupt context is saved.
@@ -152,8 +149,6 @@ static int omap4_enter_idle_coupled(struct cpuidle_device *dev,
 	if (omap4_mpuss_read_prev_context_state())
 		cpu_cluster_pm_exit();
 
-	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu_id);
-
 fail:
 	cpuidle_coupled_parallel_barrier(dev, &abort_barrier);
 	cpu_done[dev->cpu] = false;
@@ -193,7 +188,8 @@ static struct cpuidle_driver omap4_idle_driver = {
 			/* C2 - CPU0 OFF + CPU1 OFF + MPU CSWR */
 			.exit_latency = 328 + 440,
 			.target_residency = 960,
-			.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_COUPLED,
+			.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_COUPLED |
+			         CPUIDLE_FLAG_TIMER_STOP,
 			.enter = omap4_enter_idle_coupled,
 			.name = "C2",
 			.desc = "MPUSS CSWR",
@@ -202,7 +198,8 @@ static struct cpuidle_driver omap4_idle_driver = {
 			/* C3 - CPU0 OFF + CPU1 OFF + MPU OSWR */
 			.exit_latency = 460 + 518,
 			.target_residency = 1100,
-			.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_COUPLED,
+			.flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_COUPLED |
+			         CPUIDLE_FLAG_TIMER_STOP,
 			.enter = omap4_enter_idle_coupled,
 			.name = "C3",
 			.desc = "MPUSS OSWR",
-- 
1.7.9.5

[PATCH 3/4] cpuidle / imx6 : use CPUIDLE_FLAG_TIMER_STOP flag

[Daniel Lezcano]

Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework
to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering
this state.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Len Brown <lenb@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/arm/mach-imx/cpuidle-imx6q.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
index d533e26..5ae22f7 100644
--- a/arch/arm/mach-imx/cpuidle-imx6q.c
+++ b/arch/arm/mach-imx/cpuidle-imx6q.c
@@ -21,10 +21,6 @@ static DEFINE_SPINLOCK(master_lock);
 static int imx6q_enter_wait(struct cpuidle_device *dev,
 			    struct cpuidle_driver *drv, int index)
 {
-	int cpu = dev->cpu;
-
-	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
-
 	if (atomic_inc_return(&master) == num_online_cpus()) {
 		/*
 		 * With this lock, we prevent other cpu to exit and enter
@@ -43,7 +39,6 @@ idle:
 	cpu_do_idle();
 done:
 	atomic_dec(&master);
-	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
 
 	return index;
 }
@@ -70,7 +65,8 @@ static struct cpuidle_driver imx6q_cpuidle_driver = {
 		{
 			.exit_latency = 50,
 			.target_residency = 75,
-			.flags = CPUIDLE_FLAG_TIME_VALID,
+			.flags = CPUIDLE_FLAG_TIME_VALID |
+			         CPUIDLE_FLAG_TIMER_STOP,
 			.enter = imx6q_enter_wait,
 			.name = "WAIT",
 			.desc = "Clock off",
-- 
1.7.9.5

[PATCH 4/4] cpuidle / ux500 : use CPUIDLE_FLAG_TIMER_STOP flag

[Daniel Lezcano]

Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework
to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering
this state.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Len Brown <lenb@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/arm/mach-ux500/cpuidle.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-ux500/cpuidle.c b/arch/arm/mach-ux500/cpuidle.c
index ce91493..6d0c4b6 100644
--- a/arch/arm/mach-ux500/cpuidle.c
+++ b/arch/arm/mach-ux500/cpuidle.c
@@ -30,8 +30,6 @@ static inline int ux500_enter_idle(struct cpuidle_device *dev,
 	int this_cpu = smp_processor_id();
 	bool recouple = false;
 
-	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &this_cpu);
-
 	if (atomic_inc_return(&master) == num_online_cpus()) {
 
 		/* With this lock, we prevent the other cpu to exit and enter
@@ -91,8 +89,6 @@ out:
 		spin_unlock(&master_lock);
 	}
 
-	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &this_cpu);
-
 	return index;
 }
 
@@ -106,7 +102,8 @@ static struct cpuidle_driver ux500_idle_driver = {
 			.enter		  = ux500_enter_idle,
 			.exit_latency	  = 70,
 			.target_residency = 260,
-			.flags		  = CPUIDLE_FLAG_TIME_VALID,
+			.flags		  = CPUIDLE_FLAG_TIME_VALID |
+			                    CPUIDLE_FLAG_TIMER_STOP,
 			.name		  = "ApIdle",
 			.desc		  = "ARM Retention",
 		},
-- 
1.7.9.5

Re: [PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[Santosh Shilimkar]

On Thursday 21 March 2013 05:51 PM, 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.
> 
> Add a new flag CPUIDLE_FLAG_TIMER_STOP which can be set by the 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.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  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);
> +
Seems like good clean-up from drivers.
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

How about taking care of cpu_pm_notifiers() as well with some
additional flag for CPU and cluster power state. That can help
to reduce and consolidate the code. What you say ?

Regards,
Santosh

Regards,
Santosh

Re: [PATCH 2/4] cpuidle / omap4 : use CPUIDLE_FLAG_TIMER_STOP flag

[Santosh Shilimkar]

$subject
s/'cpuidle/omap4' /ARM: CPUidle: OMAP4:

On Thursday 21 March 2013 05:51 PM, Daniel Lezcano wrote:
> Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework
> to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering
> this state.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
Otherwise
Akced-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH 3/4] cpuidle / imx6 : use CPUIDLE_FLAG_TIMER_STOP flag

[Santosh Shilimkar]

On Thursday 21 March 2013 05:51 PM, Daniel Lezcano wrote:
> Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework
> to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering
> this state.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/arm/mach-imx/cpuidle-imx6q.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
Same minor comment about subject o.w for this
patch as well as ux500 one,

Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH 2/4] cpuidle / omap4 : use CPUIDLE_FLAG_TIMER_STOP flag

[Santosh Shilimkar]

On Thursday 21 March 2013 06:31 PM, Santosh Shilimkar wrote:
> $subject
> s/'cpuidle/omap4' /ARM: CPUidle: OMAP4:
> 
> On Thursday 21 March 2013 05:51 PM, Daniel Lezcano wrote:
>> Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework
>> to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering
>> this state.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Cc: Rajendra Nayak <rnayak@ti.com>
>> Cc: Sascha Hauer <kernel@pengutronix.de>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> ---
> Otherwise
> Akced-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> 
One more comment for this patch as well as imx and ux500 one.

Remove "#include <linux/clockchips.h>" since it is not needed
anymore.

Regards,
Santosh

Re: [PATCH 2/4] cpuidle / omap4 : use CPUIDLE_FLAG_TIMER_STOP flag

[Daniel Lezcano]

On 03/21/2013 02:05 PM, Santosh Shilimkar wrote:
> On Thursday 21 March 2013 06:31 PM, Santosh Shilimkar wrote:
>> $subject
>> s/'cpuidle/omap4' /ARM: CPUidle: OMAP4:
>>
>> On Thursday 21 March 2013 05:51 PM, Daniel Lezcano wrote:
>>> Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework
>>> to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering
>>> this state.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: Len Brown <lenb@kernel.org>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>> Cc: Sascha Hauer <kernel@pengutronix.de>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> ---
>> Otherwise
>> Akced-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>
> One more comment for this patch as well as imx and ux500 one.
> 
> Remove "#include <linux/clockchips.h>" since it is not needed
> anymore.

Actually, it is still needed for:

omap_setup_broadcast_timer
imx6q_setup_broadcast_timer
ux500_setup_broadcast_timer

I am preparing another round of patches to move these initialization to
the cpuidle framework but I have to take into account the multiple
drivers support which could be impacted, this is why it is a separated
patchset.


-- 
 <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

Re: [PATCH 2/4] cpuidle / omap4 : use CPUIDLE_FLAG_TIMER_STOP flag

[Santosh Shilimkar]

On Thursday 21 March 2013 06:44 PM, Daniel Lezcano wrote:
> On 03/21/2013 02:05 PM, Santosh Shilimkar wrote:
>> On Thursday 21 March 2013 06:31 PM, Santosh Shilimkar wrote:
>>> $subject
>>> s/'cpuidle/omap4' /ARM: CPUidle: OMAP4:
>>>
>>> On Thursday 21 March 2013 05:51 PM, Daniel Lezcano wrote:
>>>> Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework
>>>> to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering
>>>> this state.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> Cc: Len Brown <lenb@kernel.org>
>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>> Cc: Sascha Hauer <kernel@pengutronix.de>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> ---
>>> Otherwise
>>> Akced-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>
>> One more comment for this patch as well as imx and ux500 one.
>>
>> Remove "#include <linux/clockchips.h>" since it is not needed
>> anymore.
> 
> Actually, it is still needed for:
> 
> omap_setup_broadcast_timer
> imx6q_setup_broadcast_timer
> ux500_setup_broadcast_timer
> 
Indeed !!

> I am preparing another round of patches to move these initialization to
> the cpuidle framework but I have to take into account the multiple
> drivers support which could be impacted, this is why it is a separated
> patchset.
> 
fair enough.

Regards,
Santosh

Re: [PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[Daniel Lezcano]

On 03/21/2013 01:59 PM, Santosh Shilimkar wrote:
> On Thursday 21 March 2013 05:51 PM, 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.
>>
>> Add a new flag CPUIDLE_FLAG_TIMER_STOP which can be set by the 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.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Cc: Rajendra Nayak <rnayak@ti.com>
>> Cc: Sascha Hauer <kernel@pengutronix.de>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>  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);
>> +
> Seems like good clean-up from drivers.
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> 
> How about taking care of cpu_pm_notifiers() as well with some
> additional flag for CPU and cluster power state. That can help
> to reduce and consolidate the code. What you say ?

Do you mean add a flag for different level of idle (idle, suspend, power
off) and then use the cpu_pm_enter/cpu_cluster_pm_enter in all the
drivers as a common framework ?

-- 
 <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

Re: [PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[Daniel Lezcano]

On 03/21/2013 01:59 PM, Santosh Shilimkar wrote:

[ ... ]

> Seems like good clean-up from drivers.
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

... btw, thanks for reviewing the patches :)

  -- 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

Re: [PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[Santosh Shilimkar]

On Thursday 21 March 2013 07:22 PM, Daniel Lezcano wrote:
> On 03/21/2013 01:59 PM, Santosh Shilimkar wrote:
>> On Thursday 21 March 2013 05:51 PM, 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.
>>>
>>> Add a new flag CPUIDLE_FLAG_TIMER_STOP which can be set by the 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.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Len Brown <lenb@kernel.org>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>> Cc: Sascha Hauer <kernel@pengutronix.de>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> ---
>>>  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);
>>> +
>> Seems like good clean-up from drivers.
>> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>
>> How about taking care of cpu_pm_notifiers() as well with some
>> additional flag for CPU and cluster power state. That can help
>> to reduce and consolidate the code. What you say ?
> 
> Do you mean add a flag for different level of idle (idle, suspend, power
> off) and then use the cpu_pm_enter/cpu_cluster_pm_enter in all the
> drivers as a common framework ?
> 
I mean only for CPUidle considering C-state already has the information
about CPU and cluster power state. For suspend, we by-default run the notifiers
so nothing needs to be done there.

You may not even need a framework. Just like we know in a C-state, timer
stops, same lines, we can say CPU state is going to be say off and hence
cpu_pm_enter() notifier needs to be called. And same way for cluster.

I still haven't given complete thought but thought crossed my mind
after looking at your patches.

Regards,
Santosh 

Re: [PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[Daniel Lezcano]

On 03/21/2013 03:04 PM, Santosh Shilimkar wrote:
> On Thursday 21 March 2013 07:22 PM, Daniel Lezcano wrote:
>> On 03/21/2013 01:59 PM, Santosh Shilimkar wrote:
>>> On Thursday 21 March 2013 05:51 PM, 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.
>>>>
>>>> Add a new flag CPUIDLE_FLAG_TIMER_STOP which can be set by the 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.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Len Brown <lenb@kernel.org>
>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>> Cc: Sascha Hauer <kernel@pengutronix.de>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> ---
>>>>  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);
>>>> +
>>> Seems like good clean-up from drivers.
>>> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>
>>> How about taking care of cpu_pm_notifiers() as well with some
>>> additional flag for CPU and cluster power state. That can help
>>> to reduce and consolidate the code. What you say ?
>>
>> Do you mean add a flag for different level of idle (idle, suspend, power
>> off) and then use the cpu_pm_enter/cpu_cluster_pm_enter in all the
>> drivers as a common framework ?
>>
> I mean only for CPUidle considering C-state already has the information
> about CPU and cluster power state. For suspend, we by-default run the notifiers
> so nothing needs to be done there.
> 
> You may not even need a framework. Just like we know in a C-state, timer
> stops, same lines, we can say CPU state is going to be say off and hence
> cpu_pm_enter() notifier needs to be called. And same way for cluster.
> 
> I still haven't given complete thought but thought crossed my mind
> after looking at your patches.

Right, that could be interesting.

I see may be one issue with this approach: when we enter an idle state
with power off, some checking are done before cpu_pm_enter and that
could lead to abort the current idle routine.

By moving this to the cpuidle framework, we will invoke always
cpu_pm_enter/exit even if the idle enter routine failed.

But, IMO, the idea is good.

For example in cpuidle34xx:

static int __omap3_enter_idle(struct cpuidle_device *dev,
                                struct cpuidle_driver *drv,
                                int index)
{
        struct omap3_idle_statedata *cx = &omap3_idle_data[index];

        local_fiq_disable();

        if (omap_irq_pending() || need_resched())
                goto return_sleep_time;

	...

        if (cx->mpu_state == PWRDM_POWER_OFF)
                cpu_pm_enter();

	...
}

The same for omap4 and tegra2/3.

With your knowledge of omap, do you think it is possible to move
cpu_pm_enter before entering the idle routine ?

Thanks
  -- 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

Re: [PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[Santosh Shilimkar]

On Thursday 21 March 2013 08:11 PM, Daniel Lezcano wrote:
> On 03/21/2013 03:04 PM, Santosh Shilimkar wrote:
>> On Thursday 21 March 2013 07:22 PM, Daniel Lezcano wrote:
>>> On 03/21/2013 01:59 PM, Santosh Shilimkar wrote:
>>>> On Thursday 21 March 2013 05:51 PM, 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.
>>>>>
>>>>> Add a new flag CPUIDLE_FLAG_TIMER_STOP which can be set by the 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.
>>>>>
>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
>>>>> Cc: Len Brown <lenb@kernel.org>
>>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>>> Cc: Sascha Hauer <kernel@pengutronix.de>
>>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>>> ---
>>>>>  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);
>>>>> +
>>>> Seems like good clean-up from drivers.
>>>> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>
>>>> How about taking care of cpu_pm_notifiers() as well with some
>>>> additional flag for CPU and cluster power state. That can help
>>>> to reduce and consolidate the code. What you say ?
>>>
>>> Do you mean add a flag for different level of idle (idle, suspend, power
>>> off) and then use the cpu_pm_enter/cpu_cluster_pm_enter in all the
>>> drivers as a common framework ?
>>>
>> I mean only for CPUidle considering C-state already has the information
>> about CPU and cluster power state. For suspend, we by-default run the notifiers
>> so nothing needs to be done there.
>>
>> You may not even need a framework. Just like we know in a C-state, timer
>> stops, same lines, we can say CPU state is going to be say off and hence
>> cpu_pm_enter() notifier needs to be called. And same way for cluster.
>>
>> I still haven't given complete thought but thought crossed my mind
>> after looking at your patches.
> 
> Right, that could be interesting.
> 
> I see may be one issue with this approach: when we enter an idle state
> with power off, some checking are done before cpu_pm_enter and that
> could lead to abort the current idle routine.
> 
I see your point.

> By moving this to the cpuidle framework, we will invoke always
> cpu_pm_enter/exit even if the idle enter routine failed.
> 
If we at all decide to go on this path, we can always get around the
issues. The key is these notifiers have to be run very close to the
low power entry/exit since the saved context for CPU/CPU cluster
at wrong points would lead to many issues.

> But, IMO, the idea is good.
> 
> For example in cpuidle34xx:
> 
> static int __omap3_enter_idle(struct cpuidle_device *dev,
>                                 struct cpuidle_driver *drv,
>                                 int index)
> {
>         struct omap3_idle_statedata *cx = &omap3_idle_data[index];
> 
>         local_fiq_disable();
> 
>         if (omap_irq_pending() || need_resched())
>                 goto return_sleep_time;
> 
> 	...
> 
>         if (cx->mpu_state == PWRDM_POWER_OFF)
>                 cpu_pm_enter();
> 
> 	...
> }
> 
> The same for omap4 and tegra2/3.
> 
> With your knowledge of omap, do you think it is possible to move
> cpu_pm_enter before entering the idle routine ?
> 
I will get back on this topic after some experiments most likely
by next week.

Regards,
Santosh

Re: [PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[Lorenzo Pieralisi]

On Thu, Mar 21, 2013 at 02:52:21PM +0000, Santosh Shilimkar wrote:

[...]

> >>>> How about taking care of cpu_pm_notifiers() as well with some
> >>>> additional flag for CPU and cluster power state. That can help
> >>>> to reduce and consolidate the code. What you say ?
> >>>
> >>> Do you mean add a flag for different level of idle (idle, suspend, power
> >>> off) and then use the cpu_pm_enter/cpu_cluster_pm_enter in all the
> >>> drivers as a common framework ?
> >>>
> >> I mean only for CPUidle considering C-state already has the information
> >> about CPU and cluster power state. For suspend, we by-default run the notifiers
> >> so nothing needs to be done there.
> >>
> >> You may not even need a framework. Just like we know in a C-state, timer
> >> stops, same lines, we can say CPU state is going to be say off and hence
> >> cpu_pm_enter() notifier needs to be called. And same way for cluster.
> >>
> >> I still haven't given complete thought but thought crossed my mind
> >> after looking at your patches.
> > 
> > Right, that could be interesting.
> > 
> > I see may be one issue with this approach: when we enter an idle state
> > with power off, some checking are done before cpu_pm_enter and that
> > could lead to abort the current idle routine.
> > 
> I see your point.
> 
> > By moving this to the cpuidle framework, we will invoke always
> > cpu_pm_enter/exit even if the idle enter routine failed.
> > 
> If we at all decide to go on this path, we can always get around the
> issues. The key is these notifiers have to be run very close to the
> low power entry/exit since the saved context for CPU/CPU cluster
> at wrong points would lead to many issues.
> 
> > But, IMO, the idea is good.
> > 
> > For example in cpuidle34xx:
> > 
> > static int __omap3_enter_idle(struct cpuidle_device *dev,
> >                                 struct cpuidle_driver *drv,
> >                                 int index)
> > {
> >         struct omap3_idle_statedata *cx = &omap3_idle_data[index];
> > 
> >         local_fiq_disable();
> > 
> >         if (omap_irq_pending() || need_resched())
> >                 goto return_sleep_time;
> > 
> > 	...
> > 
> >         if (cx->mpu_state == PWRDM_POWER_OFF)
> >                 cpu_pm_enter();
> > 
> > 	...
> > }
> > 
> > The same for omap4 and tegra2/3.
> > 
> > With your knowledge of omap, do you think it is possible to move
> > cpu_pm_enter before entering the idle routine ?
> > 
> I will get back on this topic after some experiments most likely
> by next week.

Looks like the way to go, we could enhance the notifiers to be able to
save/restore specific subsystems with the C-state flags defining which
ones.

Notifiers actions should not be disruptive so the only drawback in
executing those before entering the idle routine could possibly be
a waste of cycles in case the C-state entry fails but certainly something
to verify on all platforms using them.

Lorenzo

Re: [PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[Rafael J. Wysocki]

On Thursday, March 21, 2013 01:21:31 PM 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.
> 
> Add a new flag CPUIDLE_FLAG_TIMER_STOP which can be set by the 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.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>

All patches in the series applied to linux-pm.git/bleeding-edge and will be
moved to linux-next after build testing.

Thanks,
Rafael


> ---
>  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)
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[Kevin Hilman]

Daniel Lezcano <daniel.lezcano@linaro.org> writes:

> 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.
>
> Add a new flag CPUIDLE_FLAG_TIMER_STOP which can be set by the 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.

Nice cleanup.

Reviewed-by: Kevin Hilman <khilman@linaro.org>

Re: [PATCH 2/4] cpuidle / omap4 : use CPUIDLE_FLAG_TIMER_STOP flag

[Kevin Hilman]

Daniel Lezcano <daniel.lezcano@linaro.org> writes:

> Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework
> to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering
> this state.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>

OMAP changes look good.

Reviewed-by: Kevin Hilman <khilman@linaro.org>

Re: [PATCH 1/4][resend] cpuidle : handle clockevent notify from the cpuidle framework

[Santosh Shilimkar]

Lorenzo, Daniel,

On Thursday 21 March 2013 08:55 PM, Lorenzo Pieralisi wrote:
> On Thu, Mar 21, 2013 at 02:52:21PM +0000, Santosh Shilimkar wrote:
> 
> [...]
> 
>>>>>> How about taking care of cpu_pm_notifiers() as well with some
>>>>>> additional flag for CPU and cluster power state. That can help
>>>>>> to reduce and consolidate the code. What you say ?
>>>>>
>>>>> Do you mean add a flag for different level of idle (idle, suspend, power
>>>>> off) and then use the cpu_pm_enter/cpu_cluster_pm_enter in all the
>>>>> drivers as a common framework ?
>>>>>
>>>> I mean only for CPUidle considering C-state already has the information
>>>> about CPU and cluster power state. For suspend, we by-default run the notifiers
>>>> so nothing needs to be done there.
>>>>
>>>> You may not even need a framework. Just like we know in a C-state, timer
>>>> stops, same lines, we can say CPU state is going to be say off and hence
>>>> cpu_pm_enter() notifier needs to be called. And same way for cluster.
>>>>
>>>> I still haven't given complete thought but thought crossed my mind
>>>> after looking at your patches.
>>>
>>> Right, that could be interesting.
>>>
>>> I see may be one issue with this approach: when we enter an idle state
>>> with power off, some checking are done before cpu_pm_enter and that
>>> could lead to abort the current idle routine.
>>>
>> I see your point.
>>
>>> By moving this to the cpuidle framework, we will invoke always
>>> cpu_pm_enter/exit even if the idle enter routine failed.
>>>
>> If we at all decide to go on this path, we can always get around the
>> issues. The key is these notifiers have to be run very close to the
>> low power entry/exit since the saved context for CPU/CPU cluster
>> at wrong points would lead to many issues.
>>
>>> But, IMO, the idea is good.
>>>
>>> For example in cpuidle34xx:
>>>
>>> static int __omap3_enter_idle(struct cpuidle_device *dev,
>>>                                 struct cpuidle_driver *drv,
>>>                                 int index)
>>> {
>>>         struct omap3_idle_statedata *cx = &omap3_idle_data[index];
>>>
>>>         local_fiq_disable();
>>>
>>>         if (omap_irq_pending() || need_resched())
>>>                 goto return_sleep_time;
>>>
>>> 	...
>>>
>>>         if (cx->mpu_state == PWRDM_POWER_OFF)
>>>                 cpu_pm_enter();
>>>
>>> 	...
>>> }
>>>
>>> The same for omap4 and tegra2/3.
>>>
>>> With your knowledge of omap, do you think it is possible to move
>>> cpu_pm_enter before entering the idle routine ?
>>>
>> I will get back on this topic after some experiments most likely
>> by next week.
> 
> Looks like the way to go, we could enhance the notifiers to be able to
> save/restore specific subsystems with the C-state flags defining which
> ones.
> 
> Notifiers actions should not be disruptive so the only drawback in
> executing those before entering the idle routine could possibly be
> a waste of cycles in case the C-state entry fails but certainly something
> to verify on all platforms using them.
>
After spending few mins on the idea, I made few observations.

1. Since the cpu_pm_enter() involves in saving the context CPU
co-processors like VFP, debug subsystem, it should be called
after irq, preemption is disabled and we are on its way
into idle entry. It can be called from generic idle code
but we just need to take care of above.
 
2. cpu_cluster_pm_enter() actually is bit tricky since its use
is not consistent with couple idle entry and say smp_idle entry.
Clusters notifiers are called when the idle drivers finds that
all the CPUs in that cluster are at sleep and current CPU
can take down the cluster with it. This notifier as well
needs to be called late since the after the notifier call,
there shouldn't be any irq activity which might lead to
incorrect context save for say irq controller.

3. Same goes with exit notifiers which has to be called
before irq, preemption is enabled back again.

4. Couple idle overall needs special considerations to
manage these notifiers from generic code.

Overall it seems to be doable with the direction you
already started for timer broad-cast notifiers. You
can at least add this one in your queue of works :-)
I do not have much cycles left because of other pile
of work to carry out the changes but you can surely
count me on reviewing and testing the patches.

Regards,
Santosh