[RFC/PATCH 0/2] OMAP: PM: add "early" idle notifier chain

[RFC/PATCH 0/2] OMAP: PM: add "early" idle notifier chain

[Kevin Hilman]

Based on previous idle notification series, starting at:

      [PATCH 1/3] OMAP: PM: formalize idle notifications

This series adds an additional "early" idle notifier chain triggered
early in the CPUidle path with interrupts enabled.

This allows users of "early" notifiers to use blocking calls.  While
in general, use of blocking calls in idle notifiers should be avoided,
the current runtime PM API can sleep/schedule so cannot be done from
atomic context.  Use of "early" notifiers allows driver/device code to
use the runtime PM API in their idle notifier callbacks.

RFC: note that patch 2 enables interrupts in the CPUidle path, causing
interrupts to be enabled during the governor state selection and
device idle detection.  What could go wrong here?

Kevin Hilman (2):
  OMAP: PM: add "early" idle notifications
  OMAP3: CPUidle: trigger early idle notification call chain

 arch/arm/mach-omap2/cpuidle34xx.c        |   27 ++++++++++++++++++++++++---
 arch/arm/mach-omap2/pm.c                 |   27 +++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/common.h |    6 ++++++
 3 files changed, 57 insertions(+), 3 deletions(-)

-- 
1.7.2.1

[RFC/PATCH 1/2] OMAP: PM: add "early" idle notifications

[Kevin Hilman]

Add an "early" idle notification chain for device/driver code
that wishes to be notified of idle transitions.

These are called "early" notifiers because they are run early in the
idle path, before interrupts are disabled.  Since interrups are still
enabled, these notifiers can run potentially blocking code.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/pm.c                 |   27 +++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/common.h |    6 ++++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 343e8d6..e927419 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -54,6 +54,33 @@ void omap_idle_notifier_end(void)
 	atomic_notifier_call_chain(&idle_notifier, OMAP_IDLE_END, NULL);
 }
 
+/* idle notifications early in the idle path (interrupts enabled) */
+static BLOCKING_NOTIFIER_HEAD(early_idle_notifier);
+
+void omap_early_idle_notifier_register(struct notifier_block *n)
+{
+	blocking_notifier_chain_register(&early_idle_notifier, n);
+}
+EXPORT_SYMBOL_GPL(omap_early_idle_notifier_register);
+
+void omap_early_idle_notifier_unregister(struct notifier_block *n)
+{
+	blocking_notifier_chain_unregister(&early_idle_notifier, n);
+}
+EXPORT_SYMBOL_GPL(omap_early_idle_notifier_unregister);
+
+void omap_early_idle_notifier_start(void)
+{
+	blocking_notifier_call_chain(&early_idle_notifier,
+				     OMAP_IDLE_START, NULL);
+}
+
+void omap_early_idle_notifier_end(void)
+{
+	blocking_notifier_call_chain(&early_idle_notifier,
+				     OMAP_IDLE_END, NULL);
+}
+
 struct device *omap2_get_mpuss_device(void)
 {
 	WARN_ON_ONCE(!mpu_dev);
diff --git a/arch/arm/plat-omap/include/plat/common.h b/arch/arm/plat-omap/include/plat/common.h
index 1ca32cf..1bd57f1 100644
--- a/arch/arm/plat-omap/include/plat/common.h
+++ b/arch/arm/plat-omap/include/plat/common.h
@@ -106,4 +106,10 @@ extern void omap_idle_notifier_unregister(struct notifier_block *n);
 extern void omap_idle_notifier_start(void);
 extern void omap_idle_notifier_end(void);
 
+/* idle notifications early in the idle path (interrupts enabled) */
+extern void omap_early_idle_notifier_register(struct notifier_block *n);
+extern void omap_early_idle_notifier_unregister(struct notifier_block *n);
+extern void omap_early_idle_notifier_start(void);
+extern void omap_early_idle_notifier_end(void);
+
 #endif /* __ARCH_ARM_MACH_OMAP_COMMON_H */
-- 
1.7.2.1

[RFC/PATCH 2/2] OMAP3: CPUidle: trigger early idle notification call chain

[Kevin Hilman]

During the early part of the CPUidle path (state selection by the
governer, checking for device activity, etc.) there is no (good)
reason to have interrupts disabled.

Therefore, enable interrupts early in the CPUidle path and then
trigger the "early" idle notifier call chain after device activity
checks and the next power states have been programmed.  Interrupts are
then (re)disabled after the final state is selected.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   27 ++++++++++++++++++++++++---
 1 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 0d50b45..8c57360 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -30,6 +30,7 @@
 #include <plat/powerdomain.h>
 #include <plat/clockdomain.h>
 #include <plat/serial.h>
+#include <plat/common.h>
 
 #include "pm.h"
 #include "control.h"
@@ -128,12 +129,14 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
 	/* Used to keep track of the total time in idle */
 	getnstimeofday(&ts_preidle);
 
-	local_irq_disable();
-	local_fiq_disable();
-
 	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
 	pwrdm_set_next_pwrst(core_pd, core_state);
 
+	omap_early_idle_notifier_start();
+
+	local_irq_disable();
+	local_fiq_disable();
+
 	if (omap_irq_pending() || need_resched())
 		goto return_sleep_time;
 
@@ -157,6 +160,8 @@ return_sleep_time:
 	local_irq_enable();
 	local_fiq_enable();
 
+	omap_early_idle_notifier_end();
+
 	return ts_idle.tv_nsec / NSEC_PER_USEC + ts_idle.tv_sec * USEC_PER_SEC;
 }
 
@@ -459,6 +464,21 @@ struct cpuidle_driver omap3_idle_driver = {
 	.owner = 	THIS_MODULE,
 };
 
+static int omap3_idle_prepare(struct cpuidle_device *dev)
+{
+	/*
+	 * Enable interrupts during the early part of the CPUidle path.
+	 * They are later (re)disabled when the final state is selected.
+	 *
+	 * The primary reason for this is to enable non-atomic idle
+	 * notifications to drivers that want to coordinate device
+	 * idle transitions with CPU idle transitions.
+	 */
+	local_irq_enable();
+
+	return 0;
+}
+
 /**
  * omap3_idle_init - Init routine for OMAP3 idle
  *
@@ -482,6 +502,7 @@ int __init omap3_idle_init(void)
 
 	dev = &per_cpu(omap3_idle_dev, smp_processor_id());
 
+	dev->prepare = omap3_idle_prepare;
 	for (i = OMAP3_STATE_C1; i < OMAP3_MAX_STATES; i++) {
 		cx = &omap3_power_states[i];
 		state = &dev->states[count];
-- 
1.7.2.1

[RFC/PATCH 2/2] OMAP3: CPUidle: trigger early idle notification call chain

[Sripathy, Vishwanath]

Kevin,

> -----Original Message-----
> From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-
> owner at vger.kernel.org] On Behalf Of Kevin Hilman
> Sent: Thursday, October 21, 2010 5:09 AM
> To: linux-omap at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Subject: [RFC/PATCH 2/2] OMAP3: CPUidle: trigger early idle notification call chain
> 
> During the early part of the CPUidle path (state selection by the
> governer, checking for device activity, etc.) there is no (good)
> reason to have interrupts disabled.
> 
> Therefore, enable interrupts early in the CPUidle path and then
> trigger the "early" idle notifier call chain after device activity
> checks and the next power states have been programmed.  Interrupts are
> then (re)disabled after the final state is selected.
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/cpuidle34xx.c |   27 ++++++++++++++++++++++++--
> -
>  1 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-
> omap2/cpuidle34xx.c
> index 0d50b45..8c57360 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -30,6 +30,7 @@
>  #include <plat/powerdomain.h>
>  #include <plat/clockdomain.h>
>  #include <plat/serial.h>
> +#include <plat/common.h>
> 
>  #include "pm.h"
>  #include "control.h"
> @@ -128,12 +129,14 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
>  	/* Used to keep track of the total time in idle */
>  	getnstimeofday(&ts_preidle);
> 
> -	local_irq_disable();
> -	local_fiq_disable();
> -
>  	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
>  	pwrdm_set_next_pwrst(core_pd, core_state);
> 
> +	omap_early_idle_notifier_start();
> +
> +	local_irq_disable();
> +	local_fiq_disable();
> +
>  	if (omap_irq_pending() || need_resched())
>  		goto return_sleep_time;
> 
> @@ -157,6 +160,8 @@ return_sleep_time:
>  	local_irq_enable();
>  	local_fiq_enable();
> 
> +	omap_early_idle_notifier_end();
It appears that idle notifiers (for restore path) are called after getnstimeofday is called which means time spent in idle callbacks are not accounted in cpuidle time. 
Will it not impact the cpuidle c state prediction and latency computation?

Regards
Vishwa

> +
>  	return ts_idle.tv_nsec / NSEC_PER_USEC + ts_idle.tv_sec * USEC_PER_SEC;
>  }
> 
> @@ -459,6 +464,21 @@ struct cpuidle_driver omap3_idle_driver = {
>  	.owner = 	THIS_MODULE,
>  };
> 
> +static int omap3_idle_prepare(struct cpuidle_device *dev)
> +{
> +	/*
> +	 * Enable interrupts during the early part of the CPUidle path.
> +	 * They are later (re)disabled when the final state is selected.
> +	 *
> +	 * The primary reason for this is to enable non-atomic idle
> +	 * notifications to drivers that want to coordinate device
> +	 * idle transitions with CPU idle transitions.
> +	 */
> +	local_irq_enable();
> +
> +	return 0;
> +}
> +
>  /**
>   * omap3_idle_init - Init routine for OMAP3 idle
>   *
> @@ -482,6 +502,7 @@ int __init omap3_idle_init(void)
> 
>  	dev = &per_cpu(omap3_idle_dev, smp_processor_id());
> 
> +	dev->prepare = omap3_idle_prepare;
>  	for (i = OMAP3_STATE_C1; i < OMAP3_MAX_STATES; i++) {
>  		cx = &omap3_power_states[i];
>  		state = &dev->states[count];
> --
> 1.7.2.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH 2/2] OMAP3: CPUidle: trigger early idle notification call chain

[Kevin Hilman]

"Sripathy, Vishwanath" <vishwanath.bs@ti.com> writes:

>> 
>> During the early part of the CPUidle path (state selection by the
>> governer, checking for device activity, etc.) there is no (good)
>> reason to have interrupts disabled.
>> 
>> Therefore, enable interrupts early in the CPUidle path and then
>> trigger the "early" idle notifier call chain after device activity
>> checks and the next power states have been programmed.  Interrupts are
>> then (re)disabled after the final state is selected.
>> 
>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>> ---
>>  arch/arm/mach-omap2/cpuidle34xx.c |   27 ++++++++++++++++++++++++--
>> -
>>  1 files changed, 24 insertions(+), 3 deletions(-)
>> 
>>
>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-
>> omap2/cpuidle34xx.c
>> index 0d50b45..8c57360 100644
>> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>> @@ -30,6 +30,7 @@
>>  #include <plat/powerdomain.h>
>>  #include <plat/clockdomain.h>
>>  #include <plat/serial.h>
>> +#include <plat/common.h>
>> 
>>  #include "pm.h"
>>  #include "control.h"
>> @@ -128,12 +129,14 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
>>  	/* Used to keep track of the total time in idle */
>>  	getnstimeofday(&ts_preidle);
>> 
>> -	local_irq_disable();
>> -	local_fiq_disable();
>> -
>>  	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
>>  	pwrdm_set_next_pwrst(core_pd, core_state);
>> 
>> +	omap_early_idle_notifier_start();
>> +
>> +	local_irq_disable();
>> +	local_fiq_disable();
>> +
>>  	if (omap_irq_pending() || need_resched())
>>  		goto return_sleep_time;
>> 
>> @@ -157,6 +160,8 @@ return_sleep_time:
>>  	local_irq_enable();
>>  	local_fiq_enable();
>> 
>> +	omap_early_idle_notifier_end();

> It appears that idle notifiers (for restore path) are called after
> getnstimeofday is called which means time spent in idle callbacks are
> not accounted in cpuidle time.  Will it not impact the cpuidle c state
> prediction and latency computation?

You're right.

The current patch includes the pre-idle notifiers, but not the post-idle
notifiers.  We should either include both or exclude both in the timing
but not do one or the other.

I will post and updated version which includes both in the timing
measurement.

But you've hit on something that I'm not too crazy about, and that
Rajendra raised earlier as well, and also why this series is still RFC.

Not only does the CPUidle timing measurement now measure the idle
callbacks, because interrupts are enabled, it will also measure any
other interrupt processing and/or scheduling that might happend because
of the notifiers.

At LPC this year, we'll hopefully be discussing better ways to decouple
CPU idle states and device idle states, and hopefully come up with some
better options here.  Until then, I think this approach is a good
compromise.

Kevin

[RFC/PATCH 2/2] OMAP3: CPUidle: trigger early idle notification call chain

[Kevin Hilman]

[updated version of the patch, fixing issues raised by Vishwa]