This patch series moves vaious functionality duplicated in platform cpuidle drivers to the core cpuidle driver. Also, the platform irq disabling was removed as it appears that all calls into cpuidle_call_idle will have already called local_irq_disable().
Based on 3.3-rc5 plus recent exynos cpuidle patch: http://www.spinics.net/lists/linux-samsung-soc/msg09467.html
v4 submission can be found here: http://lists.infradead.org/pipermail/linux-arm-kernel/2012-January/082742.ht...
Changes since v4: * Added common cpu_do_idle function to core cpuidle * Added time keep irq en wrapper to core cpuidle * Removed pre/post enter * Re-added platforms that can use new common code.
v3 submission can be found here: http://www.spinics.net/lists/arm-kernel/msg156751.html Changes since v3: * Removed drivers/cpuidle/common.c ** Removed the initialization helper functions ** Removed the wrapper used to consolidate time keeping and irq enable/disable * Add time keeping and local_irq_disable handling in cpuidle_call_idle(). * Made necessary modifications to a few platforms that required the most changes ** Note on omap3: changed structure of omap3_idle_drvdata and added per_next_state and per_saved_state vars to accomodate new framework.
v2 submission can be found here: http://comments.gmane.org/gmane.linux.ports.arm.kernel/144199
Changes since v2: * Made various code organization and style changes as suggested in v1 review. * Removed at91 use of common code. A separate effort is underway to clean at91 code and the author has offered to convert to common interface as part of those changes (if this common interface is accepted in time). * Made platform cpuidle_driver objects __initdata and dynamically added one persistent instance of this object in common code. * Removed imx5 pm usage of gpc_dvfs clock as it is no longer needed after being enabled during clock initialization. * Re-organized patches.
v1 submission can be found here: http://comments.gmane.org/gmane.linux.ports.arm.kernel/142791
Changes since v1: * Common interface moved to drivers/cpuidle and made non arch-specific. * Made various fixes and suggested additions to the common cpuidle code from v1 review. * Added callback for filling in driver_data field as needed. * Modified the various platforms with these changes.
Robert Lee (9): cpuidle: Add commonly used functionality for consolidation SH: shmobile: cpuidle consolidation ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable ARM: omap: Consolidate OMAP4 cpuidle time keeping and irq enable ARM: shmobile: Consolidate cpuidle functionality ARM: davinci: Consolidate cpuidle functionality ARM: exynos: Consolidate cpuidle functionality ARM: kirkwood: Consolidate cpuidle functionality ARM: at91: Consolidate cpuidle functionality
arch/arm/mach-at91/cpuidle.c | 64 +++++++++------------------ arch/arm/mach-davinci/cpuidle.c | 77 +++++++++++++-------------------- arch/arm/mach-exynos/cpuidle.c | 52 ++-------------------- arch/arm/mach-kirkwood/cpuidle.c | 71 +++++++++---------------------- arch/arm/mach-omap2/cpuidle34xx.c | 43 ++++++++----------- arch/arm/mach-omap2/cpuidle44xx.c | 21 +-------- arch/arm/mach-shmobile/cpuidle.c | 22 +--------- arch/sh/kernel/cpu/shmobile/cpuidle.c | 10 +--- drivers/cpuidle/cpuidle.c | 37 +++++++++------ include/linux/cpuidle.h | 55 +++++++++++++++++++++++ 10 files changed, 180 insertions(+), 272 deletions(-)
Add functionality that is commonly duplicated by various platforms.
Signed-off-by: Robert Lee rob.lee@linaro.org --- drivers/cpuidle/cpuidle.c | 37 ++++++++++++++++++------------ include/linux/cpuidle.h | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 15 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 59f4261..f21d58e 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -18,6 +18,7 @@ #include <linux/ktime.h> #include <linux/hrtimer.h> #include <linux/module.h> +#include <asm/proc-fns.h> #include <trace/events/power.h>
#include "cpuidle.h" @@ -97,7 +98,11 @@ int cpuidle_idle_call(void) trace_power_start(POWER_CSTATE, next_state, dev->cpu); trace_cpu_idle(next_state, dev->cpu);
- entered_state = target_state->enter(dev, drv, next_state); + if (drv->en_core_tk_irqen) + entered_state = cpuidle_wrap_enter(dev, drv, next_state, + target_state->enter); + else + entered_state = target_state->enter(dev, drv, next_state);
trace_power_end(dev->cpu); trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu); @@ -164,28 +169,31 @@ void cpuidle_resume_and_unlock(void)
EXPORT_SYMBOL_GPL(cpuidle_resume_and_unlock);
-#ifdef CONFIG_ARCH_HAS_CPU_RELAX -static int poll_idle(struct cpuidle_device *dev, +int cpuidle_simple_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - ktime_t t1, t2; - s64 diff; + cpu_do_idle(); + + return index; +}
- t1 = ktime_get(); - local_irq_enable(); +#ifdef CONFIG_ARCH_HAS_CPU_RELAX +static inline int __poll_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ while (!need_resched()) cpu_relax();
- t2 = ktime_get(); - diff = ktime_to_us(ktime_sub(t2, t1)); - if (diff > INT_MAX) - diff = INT_MAX; - - dev->last_residency = (int) diff; - return index; }
+static int poll_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + return cpuidle_wrap_enter(dev, drv, next_state, + __poll_idle); +} + static void poll_idle_init(struct cpuidle_driver *drv) { struct cpuidle_state *state = &drv->states[0]; @@ -195,7 +203,6 @@ static void poll_idle_init(struct cpuidle_driver *drv) state->exit_latency = 0; state->target_residency = 0; state->power_usage = -1; - state->flags = 0; state->enter = poll_idle; } #else diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 712abcc..6563683 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -15,6 +15,7 @@ #include <linux/list.h> #include <linux/kobject.h> #include <linux/completion.h> +#include <linux/hrtimer.h>
#define CPUIDLE_STATE_MAX 8 #define CPUIDLE_NAME_LEN 16 @@ -56,6 +57,16 @@ struct cpuidle_state {
#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
+/* Common ARM WFI state */ +#define CPUIDLE_ARM_WFI_STATE {\ + .enter = cpuidle_simple_enter,\ + .exit_latency = 1,\ + .target_residency = 1,\ + .flags = CPUIDLE_FLAG_TIME_VALID,\ + .name = "WFI",\ + .desc = "ARM core clock gating (WFI)",\ +} + /** * cpuidle_get_statedata - retrieves private driver state data * @st_usage: the state usage statistics @@ -122,6 +133,14 @@ struct cpuidle_driver { struct module *owner;
unsigned int power_specified:1; + /* + * use the core cpuidle time keeping. This has been implemented for the + * entire driver instead of per state as currently the device enter + * fuction allows the entered state to change which requires handling + * that requires a (subjectively) unnecessary decrease to efficiency + * in this commonly called code. + */ + unsigned int en_core_tk_irqen:1; struct cpuidle_state states[CPUIDLE_STATE_MAX]; int state_count; int safe_state_index; @@ -140,6 +159,39 @@ extern void cpuidle_pause_and_lock(void); extern void cpuidle_resume_and_unlock(void); extern int cpuidle_enable_device(struct cpuidle_device *dev); extern void cpuidle_disable_device(struct cpuidle_device *dev); +extern int cpuidle_simple_enter(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index); + +/** + * cpuidle_enter_wrap - performing timekeeping and irq around enter function + * @dev: pointer to a valid cpuidle_device object + * @drv: pointer to a valid cpuidle_driver object + * @index: index of the target cpuidle state. + */ +static inline int cpuidle_wrap_enter(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index, + int (*enter)(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index)) +{ + ktime_t time_start, time_end; + s64 diff; + + time_start = ktime_get(); + + index = enter(dev, drv, index); + + time_end = ktime_get(); + + local_irq_enable(); + + diff = ktime_to_us(ktime_sub(time_end, time_start)); + if (diff > INT_MAX) + diff = INT_MAX; + + dev->last_residency = (int) diff; + + return index; +}
#else static inline void disable_cpuidle(void) { } @@ -157,6 +209,9 @@ static inline void cpuidle_resume_and_unlock(void) { } static inline int cpuidle_enable_device(struct cpuidle_device *dev) {return -ENODEV; } static inline void cpuidle_disable_device(struct cpuidle_device *dev) { } +static inline int cpuidle_simple_enter(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{return -ENODEV; }
#endif
Robert,
On Mon, Feb 27, 2012 at 5:47 AM, Robert Lee rob.lee@linaro.org wrote:
Add functionality that is commonly duplicated by various platforms.
Signed-off-by: Robert Lee rob.lee@linaro.org
drivers/cpuidle/cpuidle.c | 37 ++++++++++++++++++------------ include/linux/cpuidle.h | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 15 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
...
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 712abcc..6563683 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -15,6 +15,7 @@ #include <linux/list.h> #include <linux/kobject.h> #include <linux/completion.h> +#include <linux/hrtimer.h>
#define CPUIDLE_STATE_MAX 8 #define CPUIDLE_NAME_LEN 16 @@ -56,6 +57,16 @@ struct cpuidle_state {
#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
+/* Common ARM WFI state */ +#define CPUIDLE_ARM_WFI_STATE {\
- .enter = cpuidle_simple_enter,\
- .exit_latency = 1,\
- .target_residency = 1,\
- .flags = CPUIDLE_FLAG_TIME_VALID,\
- .name = "WFI",\
- .desc = "ARM core clock gating (WFI)",\
+}
This macro should belong to an ARM only header.
/** * cpuidle_get_statedata - retrieves private driver state data * @st_usage: the state usage statistics @@ -122,6 +133,14 @@ struct cpuidle_driver { struct module *owner;
unsigned int power_specified:1;
- /*
- * use the core cpuidle time keeping. This has been implemented for the
- * entire driver instead of per state as currently the device enter
- * fuction allows the entered state to change which requires handling
- * that requires a (subjectively) unnecessary decrease to efficiency
- * in this commonly called code.
- */
- unsigned int en_core_tk_irqen:1;
Does the description reads as 'the time accounting is only performed if en_core_tk_irqen is set'? Also it suggests that changing (i.e. demoting) the state is kind of a problem, which is unclear. IIUC the state change is handled correctly in cpuidle_idle_call, is that correct?
struct cpuidle_state states[CPUIDLE_STATE_MAX]; int state_count; int safe_state_index; @@ -140,6 +159,39 @@ extern void cpuidle_pause_and_lock(void); extern void cpuidle_resume_and_unlock(void); extern int cpuidle_enable_device(struct cpuidle_device *dev); extern void cpuidle_disable_device(struct cpuidle_device *dev); +extern int cpuidle_simple_enter(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index);
+/**
- cpuidle_enter_wrap - performing timekeeping and irq around enter function
- @dev: pointer to a valid cpuidle_device object
- @drv: pointer to a valid cpuidle_driver object
- @index: index of the target cpuidle state.
- */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index,
- int (*enter)(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index))
The function name does not match the description (cpuidle_enter_wrap vs cpuidle_wrap_enter).
+{
- ktime_t time_start, time_end;
- s64 diff;
- time_start = ktime_get();
- index = enter(dev, drv, index);
- time_end = ktime_get();
- local_irq_enable();
- diff = ktime_to_us(ktime_sub(time_end, time_start));
- if (diff > INT_MAX)
- diff = INT_MAX;
- dev->last_residency = (int) diff;
- return index;
+}
#else static inline void disable_cpuidle(void) { }
...
Regards, Jean
On Mon, Feb 27, 2012 at 10:19 AM, Jean Pihet jean.pihet@newoldbits.com wrote:
Robert,
On Mon, Feb 27, 2012 at 5:47 AM, Robert Lee rob.lee@linaro.org wrote:
Add functionality that is commonly duplicated by various platforms.
Signed-off-by: Robert Lee rob.lee@linaro.org
drivers/cpuidle/cpuidle.c | 37 ++++++++++++++++++------------ include/linux/cpuidle.h | 55 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 15 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
...
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 712abcc..6563683 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -15,6 +15,7 @@ #include <linux/list.h> #include <linux/kobject.h> #include <linux/completion.h> +#include <linux/hrtimer.h>
#define CPUIDLE_STATE_MAX 8 #define CPUIDLE_NAME_LEN 16 @@ -56,6 +57,16 @@ struct cpuidle_state {
#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
+/* Common ARM WFI state */ +#define CPUIDLE_ARM_WFI_STATE {\
- .enter = cpuidle_simple_enter,\
- .exit_latency = 1,\
- .target_residency = 1,\
- .flags = CPUIDLE_FLAG_TIME_VALID,\
- .name = "WFI",\
- .desc = "ARM core clock gating (WFI)",\
+}
This macro should belong to an ARM only header.
Thanks, I was wondering about that but wasn't which location was better.
/** * cpuidle_get_statedata - retrieves private driver state data * @st_usage: the state usage statistics @@ -122,6 +133,14 @@ struct cpuidle_driver { struct module *owner;
unsigned int power_specified:1;
- /*
- * use the core cpuidle time keeping. This has been implemented for the
- * entire driver instead of per state as currently the device enter
- * fuction allows the entered state to change which requires handling
- * that requires a (subjectively) unnecessary decrease to efficiency
- * in this commonly called code.
- */
- unsigned int en_core_tk_irqen:1;
Does the description reads as 'the time accounting is only performed if en_core_tk_irqen is set'?
Correct. I can make this clearer in the next version's comments.
Also it suggests that changing (i.e. demoting) the state is kind of a problem, which is unclear. IIUC the state change is handled correctly in cpuidle_idle_call, is that correct?
If en_core_tk_irqen was a cpuidle state option instead of a cpuidle driver option, then if the platform enter function changed the state to one that used a different en_core_tk_irqen value, a time keeping problem could occur. Extra handling could be added, but for many SMP systems, this case will be the most common case, and so I didn't think it best to force this extra handling. Anyways, with the current implementation, if a platform cpuidle driver chooses not to use en_core_tk_irqen, they can still use the consolidated time keeping functionality by using the exported inline function (e.g., the OMAP 34XX case).
struct cpuidle_state states[CPUIDLE_STATE_MAX]; int state_count; int safe_state_index; @@ -140,6 +159,39 @@ extern void cpuidle_pause_and_lock(void); extern void cpuidle_resume_and_unlock(void); extern int cpuidle_enable_device(struct cpuidle_device *dev); extern void cpuidle_disable_device(struct cpuidle_device *dev); +extern int cpuidle_simple_enter(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index);
+/**
- cpuidle_enter_wrap - performing timekeeping and irq around enter function
- @dev: pointer to a valid cpuidle_device object
- @drv: pointer to a valid cpuidle_driver object
- @index: index of the target cpuidle state.
- */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index,
- int (*enter)(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index))
The function name does not match the description (cpuidle_enter_wrap vs cpuidle_wrap_enter).
Oops, thanks.
+{
- ktime_t time_start, time_end;
- s64 diff;
- time_start = ktime_get();
- index = enter(dev, drv, index);
- time_end = ktime_get();
- local_irq_enable();
- diff = ktime_to_us(ktime_sub(time_end, time_start));
- if (diff > INT_MAX)
- diff = INT_MAX;
- dev->last_residency = (int) diff;
- return index;
+}
#else static inline void disable_cpuidle(void) { }
...
Regards, Jean
On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee rob.lee@linaro.org wrote:
+/**
- cpuidle_enter_wrap - performing timekeeping and irq around enter function
- @dev: pointer to a valid cpuidle_device object
- @drv: pointer to a valid cpuidle_driver object
- @index: index of the target cpuidle state.
- */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index,
- int (*enter)(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index))
+{
- ktime_t time_start, time_end;
- s64 diff;
- time_start = ktime_get();
- index = enter(dev, drv, index);
- time_end = ktime_get();
- local_irq_enable();
- diff = ktime_to_us(ktime_sub(time_end, time_start));
- if (diff > INT_MAX)
- diff = INT_MAX;
- dev->last_residency = (int) diff;
- return index;
+}
Any reason that this code is in the header? Why not in cpuidle.c?
Regards, Mike
Hey Mike,
On Mon, Feb 27, 2012 at 6:06 PM, Turquette, Mike mturquette@ti.com wrote:
On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee rob.lee@linaro.org wrote:
+/**
- cpuidle_enter_wrap - performing timekeeping and irq around enter function
- @dev: pointer to a valid cpuidle_device object
- @drv: pointer to a valid cpuidle_driver object
- @index: index of the target cpuidle state.
- */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index,
- int (*enter)(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index))
+{
- ktime_t time_start, time_end;
- s64 diff;
- time_start = ktime_get();
- index = enter(dev, drv, index);
- time_end = ktime_get();
- local_irq_enable();
- diff = ktime_to_us(ktime_sub(time_end, time_start));
- if (diff > INT_MAX)
- diff = INT_MAX;
- dev->last_residency = (int) diff;
- return index;
+}
Any reason that this code is in the header? Why not in cpuidle.c?
Not a strong reason. I thought making it an inline would introduce slightly less new execution when adding this code (realizing that there are function calls immediately after, so the only benefit is the reduce popping and pushing). But it does require an extra copy of this code for any platform driver that does not enable en_core_tk_irqen and instead makes calls to it directly (like omap3). For this case, I don't think the inline implementation should add extra code from what exists today as it should simply replace the existing platform time keeping calls to a standard one defined by the core cpuidle.
I don't have a strong preference with using the inline so if you or others can give your opinion on which method to use and why, I'd be glad to read it.
Regards, Mike
On 02/28/2012 09:45 AM, Rob Lee wrote:
Hey Mike,
On Mon, Feb 27, 2012 at 6:06 PM, Turquette, Mike mturquette@ti.com wrote:
On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee rob.lee@linaro.org wrote:
+/**
- cpuidle_enter_wrap - performing timekeeping and irq around enter function
- @dev: pointer to a valid cpuidle_device object
- @drv: pointer to a valid cpuidle_driver object
- @index: index of the target cpuidle state.
- */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int index,int (*enter)(struct cpuidle_device *dev,struct cpuidle_driver *drv, int index))+{
ktime_t time_start, time_end;s64 diff;time_start = ktime_get();index = enter(dev, drv, index);time_end = ktime_get();local_irq_enable();diff = ktime_to_us(ktime_sub(time_end, time_start));if (diff > INT_MAX)diff = INT_MAX;dev->last_residency = (int) diff;return index;+}
Any reason that this code is in the header? Why not in cpuidle.c?
Not a strong reason. I thought making it an inline would introduce slightly less new execution when adding this code (realizing that there are function calls immediately after, so the only benefit is the reduce popping and pushing). But it does require an extra copy of this code for any platform driver that does not enable en_core_tk_irqen and instead makes calls to it directly (like omap3). For this case, I don't think the inline implementation should add extra code from what exists today as it should simply replace the existing platform time keeping calls to a standard one defined by the core cpuidle.
But you will have multiple copies of the inlined code if platforms do use it. Or is it used only by the core cpuidle code? In that case, gcc can automatically inline static functions.
It seems a bit long to inline and this isn't performance critical (at least for the enter side).
Rob
I don't have a strong preference with using the inline so if you or others can give your opinion on which method to use and why, I'd be glad to read it.
Regards, Mike
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Any reason that this code is in the header? Why not in cpuidle.c?
Not a strong reason. I thought making it an inline would introduce slightly less new execution when adding this code (realizing that there are function calls immediately after, so the only benefit is the reduce popping and pushing). But it does require an extra copy of this code for any platform driver that does not enable en_core_tk_irqen and instead makes calls to it directly (like omap3). For this case, I don't think the inline implementation should add extra code from what exists today as it should simply replace the existing platform time keeping calls to a standard one defined by the core cpuidle.
But you will have multiple copies of the inlined code if platforms do use it. Or is it used only by the core cpuidle code? In that case, gcc can automatically inline static functions.
Used by some platforms as well.
It seems a bit long to inline and this isn't performance critical (at least for the enter side).
Ok. Unless there are further comments supporting the inline method, I'll switch to non-inline for next version. Thanks Mike and Rob for the feedback.
Rob
I don't have a strong preference with using the inline so if you or others can give your opinion on which method to use and why, I'd be glad to read it.
Regards, Mike
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee rob.lee@linaro.org wrote:
+/**
- cpuidle_enter_wrap - performing timekeeping and irq around enter function
- @dev: pointer to a valid cpuidle_device object
- @drv: pointer to a valid cpuidle_driver object
- @index: index of the target cpuidle state.
- */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index,
- int (*enter)(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index))
+{
- ktime_t time_start, time_end;
- s64 diff;
- time_start = ktime_get();
- index = enter(dev, drv, index);
- time_end = ktime_get();
- local_irq_enable();
- diff = ktime_to_us(ktime_sub(time_end, time_start));
- if (diff > INT_MAX)
- diff = INT_MAX;
- dev->last_residency = (int) diff;
- return index;
+}
Hi Rob,
In a previous series I brought up the idea of not accounting for time if a C-state transition fails. My post on that thread can be found here: http://article.gmane.org/gmane.linux.ports.arm.kernel/149293/
How do you feel about adding something like the following?
if (IS_ERR(index)) dev->last_residency = 0; return index;
Obviously it will up to the platforms to figure out how to propagate that error up from their respective low power code.
Regards, Mike
On Mon, Feb 27, 2012 at 6:49 PM, Turquette, Mike mturquette@ti.com wrote:
On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee rob.lee@linaro.org wrote:
+/**
- cpuidle_enter_wrap - performing timekeeping and irq around enter function
- @dev: pointer to a valid cpuidle_device object
- @drv: pointer to a valid cpuidle_driver object
- @index: index of the target cpuidle state.
- */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index,
- int (*enter)(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index))
+{
- ktime_t time_start, time_end;
- s64 diff;
- time_start = ktime_get();
- index = enter(dev, drv, index);
- time_end = ktime_get();
- local_irq_enable();
- diff = ktime_to_us(ktime_sub(time_end, time_start));
- if (diff > INT_MAX)
- diff = INT_MAX;
- dev->last_residency = (int) diff;
- return index;
+}
Hi Rob,
In a previous series I brought up the idea of not accounting for time if a C-state transition fails. My post on that thread can be found here: http://article.gmane.org/gmane.linux.ports.arm.kernel/149293/
How do you feel about adding something like the following?
if (IS_ERR(index)) dev->last_residency = 0; return index;
Obviously it will up to the platforms to figure out how to propagate that error up from their respective low power code.
To be completely clear on what you're asking for, from cpuidle_idle_call in drivers/cpuidle/cpuidle.c:
... target_state = &drv->states[next_state];
trace_power_start(POWER_CSTATE, next_state, dev->cpu); trace_cpu_idle(next_state, dev->cpu);
entered_state = target_state->enter(dev, drv, next_state);
trace_power_end(dev->cpu); trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
if (entered_state >= 0) { /* Update cpuidle counters */ /* This can be moved to within driver enter routine * but that results in multiple copies of same code. */ dev->states_usage[entered_state].time += (unsigned long long)dev->last_residency; dev->states_usage[entered_state].usage++; } ...
Note the "if (entered_state >= 0)". This ultimately prevents the cpuidle device time accounting upon an negative value being returned. So are you asking for the if(IS_ERR(index)) check to prevent the unnecessary last_residency time calculation in the wrapper, or to make sure a last_residency is zero upon failure? (or both?)
It seems like a bug (or lack or documentation at best) in the code that exists today to not zero out dev->last_residency upon a negative return value as this value is used by the governors upon the next idle. So to ensure last_residency is 0 upon failure, I think it'd be best to add that to an new else statement immediately following the "if (entered_state >=))" so that any platform cpuidle driver that returns a negative will have the last_residency zeroed out, not just those that use en_core_tk_irqen.
Regards, Mike
On Tue, Feb 28, 2012 at 7:50 AM, Rob Lee rob.lee@linaro.org wrote:
On Mon, Feb 27, 2012 at 6:49 PM, Turquette, Mike mturquette@ti.com wrote:
On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee rob.lee@linaro.org wrote:
+/**
- cpuidle_enter_wrap - performing timekeeping and irq around enter function
- @dev: pointer to a valid cpuidle_device object
- @drv: pointer to a valid cpuidle_driver object
- @index: index of the target cpuidle state.
- */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index,
- int (*enter)(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int index))
+{
- ktime_t time_start, time_end;
- s64 diff;
- time_start = ktime_get();
- index = enter(dev, drv, index);
- time_end = ktime_get();
- local_irq_enable();
- diff = ktime_to_us(ktime_sub(time_end, time_start));
- if (diff > INT_MAX)
- diff = INT_MAX;
- dev->last_residency = (int) diff;
- return index;
+}
Hi Rob,
In a previous series I brought up the idea of not accounting for time if a C-state transition fails. My post on that thread can be found here: http://article.gmane.org/gmane.linux.ports.arm.kernel/149293/
How do you feel about adding something like the following?
if (IS_ERR(index)) dev->last_residency = 0; return index;
Obviously it will up to the platforms to figure out how to propagate that error up from their respective low power code.
To be completely clear on what you're asking for, from cpuidle_idle_call in drivers/cpuidle/cpuidle.c:
... target_state = &drv->states[next_state];
trace_power_start(POWER_CSTATE, next_state, dev->cpu); trace_cpu_idle(next_state, dev->cpu);
entered_state = target_state->enter(dev, drv, next_state);
trace_power_end(dev->cpu); trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
if (entered_state >= 0) { /* Update cpuidle counters */ /* This can be moved to within driver enter routine * but that results in multiple copies of same code. */ dev->states_usage[entered_state].time += (unsigned long long)dev->last_residency; dev->states_usage[entered_state].usage++; } ...
Note the "if (entered_state >= 0)". This ultimately prevents the cpuidle device time accounting upon an negative value being returned. So are you asking for the if(IS_ERR(index)) check to prevent the unnecessary last_residency time calculation in the wrapper, or to make sure a last_residency is zero upon failure? (or both?)
It seems like a bug (or lack or documentation at best) in the code that exists today to not zero out dev->last_residency upon a negative return value as this value is used by the governors upon the next idle. So to ensure last_residency is 0 upon failure, I think it'd be best to add that to an new else statement immediately following the "if (entered_state >=))" so that any platform cpuidle driver that returns a negative will have the last_residency zeroed out, not just those that use en_core_tk_irqen.
+ Cc: Jon Hunter
Hi Rob,
I didn't review the code carefully enough to catch the 'if (entered_state >= 0)' part, but that seems like a graceful way to solve this problem by appending the 'else' statement on there and seeting last_residency to zero.
I brought this topic up internally and Jon suggested that the 'usage' statistics that are reported in sysfs should also reflect failed versus successful C-state transitions, which is a great idea. This could simply be achieved by renaming the current 'usage' count to something like 'transitions_attempted' and then conditionally increment a new counter within the 'if (entered_state >= 0)' block, perhaps named, 'transition_succeeded'.
This way the old 'usage' count paradigm is as accurate as the new time-keeping code. Being able to easily observe which C-state tend to fail the most would be invaluable in tuning idle policy for maximum effectiveness.
Thoughts?
Regards, Mike
Regards, Mike
I brought this topic up internally and Jon suggested that the 'usage' statistics that are reported in sysfs should also reflect failed versus successful C-state transitions, which is a great idea. This could simply be achieved by renaming the current 'usage' count to something like 'transitions_attempted' and then conditionally increment a new counter within the 'if (entered_state >= 0)' block, perhaps named, 'transition_succeeded'.
This way the old 'usage' count paradigm is as accurate as the new time-keeping code. Being able to easily observe which C-state tend to fail the most would be invaluable in tuning idle policy for maximum effectiveness.
Thoughts?
Sounds reasonable. In some cases it may be helpful to track state demotion as well. Since I'm still a noob and wearing my submission training wheels, I'm trying to minimize things that fall outside of this basic consolidation effort for this patch series. But I added Jon's suggestion to this cpuidle page which contains future cpuidle items to consider adding: https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/CPUIdle#Track_both...
Regards, Mike
Regards, Mike
On Tue, Feb 28, 2012 at 3:33 PM, Rob Lee rob.lee@linaro.org wrote:
I brought this topic up internally and Jon suggested that the 'usage' statistics that are reported in sysfs should also reflect failed versus successful C-state transitions, which is a great idea. This could simply be achieved by renaming the current 'usage' count to something like 'transitions_attempted' and then conditionally increment a new counter within the 'if (entered_state >= 0)' block, perhaps named, 'transition_succeeded'.
This way the old 'usage' count paradigm is as accurate as the new time-keeping code. Being able to easily observe which C-state tend to fail the most would be invaluable in tuning idle policy for maximum effectiveness.
Thoughts?
Sounds reasonable. In some cases it may be helpful to track state demotion as well. Since I'm still a noob and wearing my submission training wheels, I'm trying to minimize things that fall outside of this basic consolidation effort for this patch series. But I added Jon's suggestion to this cpuidle page which contains future cpuidle items to consider adding: https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/CPUIdle#Track_both...
Yeah, I don't want to feature-bloat your submission more than necessary. I'm happy for the usage counter stuff to get tackled at a later date, but you're still on board for setting last_residency to zero in this series, right?
Regards, Mike
Regards, Mike
Regards, Mike
Sounds reasonable. In some cases it may be helpful to track state demotion as well. Since I'm still a noob and wearing my submission training wheels, I'm trying to minimize things that fall outside of this basic consolidation effort for this patch series. But I added Jon's suggestion to this cpuidle page which contains future cpuidle items to consider adding: https://wiki.linaro.org/WorkingGroups/PowerManagement/Doc/CPUIdle#Track_both...
Yeah, I don't want to feature-bloat your submission more than necessary. I'm happy for the usage counter stuff to get tackled at a later date, but you're still on board for setting last_residency to zero in this series, right?
Yes.
Regards, Mike
Enable core cpuidle timekeeping and irq enabling and remove that handling from this code.
Signed-off-by: Robert Lee rob.lee@linaro.org --- arch/sh/kernel/cpu/shmobile/cpuidle.c | 10 +++------- 1 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/arch/sh/kernel/cpu/shmobile/cpuidle.c b/arch/sh/kernel/cpu/shmobile/cpuidle.c index 6d62eb4..1ddc876 100644 --- a/arch/sh/kernel/cpu/shmobile/cpuidle.c +++ b/arch/sh/kernel/cpu/shmobile/cpuidle.c @@ -29,7 +29,6 @@ static int cpuidle_sleep_enter(struct cpuidle_device *dev, int index) { unsigned long allowed_mode = SUSP_SH_SLEEP; - ktime_t before, after; int requested_state = index; int allowed_state; int k; @@ -47,19 +46,16 @@ static int cpuidle_sleep_enter(struct cpuidle_device *dev, */ k = min_t(int, allowed_state, requested_state);
- before = ktime_get(); sh_mobile_call_standby(cpuidle_mode[k]); - after = ktime_get(); - - dev->last_residency = (int)ktime_to_ns(ktime_sub(after, before)) >> 10;
return k; }
static struct cpuidle_device cpuidle_dev; static struct cpuidle_driver cpuidle_driver = { - .name = "sh_idle", - .owner = THIS_MODULE, + .name = "sh_idle", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, };
void sh_mobile_setup_cpuidle(void)
Adding sh mailing list and sh contributors I missed on the original submission. SH folks, full patchset submission can be found here: http://www.spinics.net/lists/arm-kernel/msg161596.html
Best Regards, Rob
On Sun, Feb 26, 2012 at 10:47 PM, Robert Lee rob.lee@linaro.org wrote:
Enable core cpuidle timekeeping and irq enabling and remove that handling from this code.
Signed-off-by: Robert Lee rob.lee@linaro.org
arch/sh/kernel/cpu/shmobile/cpuidle.c | 10 +++------- 1 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/arch/sh/kernel/cpu/shmobile/cpuidle.c b/arch/sh/kernel/cpu/shmobile/cpuidle.c index 6d62eb4..1ddc876 100644 --- a/arch/sh/kernel/cpu/shmobile/cpuidle.c +++ b/arch/sh/kernel/cpu/shmobile/cpuidle.c @@ -29,7 +29,6 @@ static int cpuidle_sleep_enter(struct cpuidle_device *dev, int index) { unsigned long allowed_mode = SUSP_SH_SLEEP;
- ktime_t before, after;
int requested_state = index; int allowed_state; int k; @@ -47,19 +46,16 @@ static int cpuidle_sleep_enter(struct cpuidle_device *dev, */ k = min_t(int, allowed_state, requested_state);
- before = ktime_get();
sh_mobile_call_standby(cpuidle_mode[k]);
- after = ktime_get();
- dev->last_residency = (int)ktime_to_ns(ktime_sub(after, before)) >> 10;
return k; }
static struct cpuidle_device cpuidle_dev; static struct cpuidle_driver cpuidle_driver = {
- .name = "sh_idle",
- .owner = THIS_MODULE,
- .name = "sh_idle",
- .owner = THIS_MODULE,
- .en_core_tk_irqen = 1,
};
void sh_mobile_setup_cpuidle(void)
1.7.1
Use core cpuidle timekeeping and irqen wrapper and remove that handling from this code.
Signed-off-by: Robert Lee rob.lee@linaro.org --- arch/arm/mach-omap2/cpuidle34xx.c | 43 +++++++++++++++--------------------- 1 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c index 464cffd..1f6123f 100644 --- a/arch/arm/mach-omap2/cpuidle34xx.c +++ b/arch/arm/mach-omap2/cpuidle34xx.c @@ -87,29 +87,14 @@ static int _cpuidle_deny_idle(struct powerdomain *pwrdm, return 0; }
-/** - * omap3_enter_idle - Programs OMAP3 to enter the specified state - * @dev: cpuidle device - * @drv: cpuidle driver - * @index: the index of state to be entered - * - * Called from the CPUidle framework to program the device to the - * specified target state selected by the governor. - */ -static int omap3_enter_idle(struct cpuidle_device *dev, +static inline int __omap3_enter_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { struct omap3_idle_statedata *cx = cpuidle_get_statedata(&dev->states_usage[index]); - struct timespec ts_preidle, ts_postidle, ts_idle; u32 mpu_state = cx->mpu_state, core_state = cx->core_state; - int idle_time;
- /* 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); @@ -148,22 +133,29 @@ static int omap3_enter_idle(struct cpuidle_device *dev, }
return_sleep_time: - getnstimeofday(&ts_postidle); - ts_idle = timespec_sub(ts_postidle, ts_preidle);
- local_irq_enable(); local_fiq_enable();
- idle_time = ts_idle.tv_nsec / NSEC_PER_USEC + ts_idle.tv_sec * \ - USEC_PER_SEC; - - /* Update cpuidle counters */ - dev->last_residency = idle_time; - return index; }
/** + * omap3_enter_idle - Programs OMAP3 to enter the specified state + * @dev: cpuidle device + * @drv: cpuidle driver + * @index: the index of state to be entered + * + * Called from the CPUidle framework to program the device to the + * specified target state selected by the governor. + */ +static int omap3_enter_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, + int index) +{ + return cpuidle_wrap_enter(dev, drv, index, __omap3_enter_idle); +} + +/** * next_valid_state - Find next valid C-state * @dev: cpuidle device * @drv: cpuidle driver @@ -295,6 +287,7 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev, new_state_idx = next_valid_state(dev, drv, index);
select_state: + ret = omap3_enter_idle(dev, drv, new_state_idx);
/* Restore original PER state if it was modified */
On Mon, Feb 27, 2012 at 5:47 AM, Robert Lee rob.lee@linaro.org wrote:
Use core cpuidle timekeeping and irqen wrapper and remove that handling from this code.
Signed-off-by: Robert Lee rob.lee@linaro.org
arch/arm/mach-omap2/cpuidle34xx.c | 43 +++++++++++++++--------------------- 1 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c index 464cffd..1f6123f 100644 --- a/arch/arm/mach-omap2/cpuidle34xx.c +++ b/arch/arm/mach-omap2/cpuidle34xx.c
...
/**
- omap3_enter_idle - Programs OMAP3 to enter the specified state
- @dev: cpuidle device
- @drv: cpuidle driver
- @index: the index of state to be entered
- Called from the CPUidle framework to program the device to the
- specified target state selected by the governor.
- */
+static int omap3_enter_idle(struct cpuidle_device *dev,
- struct cpuidle_driver *drv,
- int index)
+{
- return cpuidle_wrap_enter(dev, drv, index, __omap3_enter_idle);
Is it the intention to call cpuidle_wrap_enter from the non-common code? Could the driver set en_core_tk_irqen to 1 and so let the core call the idle function? Is it to have the time measurement code closer to the low level idle code in __omap3_enter_idle?
+}
+/** * next_valid_state - Find next valid C-state * @dev: cpuidle device * @drv: cpuidle driver @@ -295,6 +287,7 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev, new_state_idx = next_valid_state(dev, drv, index);
select_state:
Stray change
ret = omap3_enter_idle(dev, drv, new_state_idx);
/* Restore original PER state if it was modified */
1.7.1
Regards, Jean
On Mon, Feb 27, 2012 at 10:26 AM, Jean Pihet jean.pihet@newoldbits.com wrote:
On Mon, Feb 27, 2012 at 5:47 AM, Robert Lee rob.lee@linaro.org wrote:
Use core cpuidle timekeeping and irqen wrapper and remove that handling from this code.
Signed-off-by: Robert Lee rob.lee@linaro.org
arch/arm/mach-omap2/cpuidle34xx.c | 43 +++++++++++++++--------------------- 1 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c index 464cffd..1f6123f 100644 --- a/arch/arm/mach-omap2/cpuidle34xx.c +++ b/arch/arm/mach-omap2/cpuidle34xx.c
...
/**
- omap3_enter_idle - Programs OMAP3 to enter the specified state
- @dev: cpuidle device
- @drv: cpuidle driver
- @index: the index of state to be entered
- Called from the CPUidle framework to program the device to the
- specified target state selected by the governor.
- */
+static int omap3_enter_idle(struct cpuidle_device *dev,
- struct cpuidle_driver *drv,
- int index)
+{
- return cpuidle_wrap_enter(dev, drv, index, __omap3_enter_idle);
Is it the intention to call cpuidle_wrap_enter from the non-common code? Could the driver set en_core_tk_irqen to 1 and so let the core call the idle function? Is it to have the time measurement code closer to the low level idle code in __omap3_enter_idle?
Yes, Yes, and Yes.
+}
+/** * next_valid_state - Find next valid C-state * @dev: cpuidle device * @drv: cpuidle driver @@ -295,6 +287,7 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev, new_state_idx = next_valid_state(dev, drv, index);
select_state:
Stray change
Oops, thanks.
ret = omap3_enter_idle(dev, drv, new_state_idx);
/* Restore original PER state if it was modified */
1.7.1
Regards, Jean
Use core cpuidle timekeeping and irqen wrapper and remove that handling from this code.
Signed-off-by: Robert Lee rob.lee@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 21 +++------------------ 1 files changed, 3 insertions(+), 18 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index cfdbb86..9729d2e 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -62,16 +62,10 @@ static int omap4_enter_idle(struct cpuidle_device *dev, { struct omap4_idle_statedata *cx = cpuidle_get_statedata(&dev->states_usage[index]); - struct timespec ts_preidle, ts_postidle, ts_idle; u32 cpu1_state; - int idle_time; int new_state_idx; int cpu_id = smp_processor_id();
- /* Used to keep track of the total time in idle */ - getnstimeofday(&ts_preidle); - - local_irq_disable(); local_fiq_disable();
/* @@ -129,26 +123,17 @@ static int omap4_enter_idle(struct cpuidle_device *dev, if (index > 0) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu_id);
- getnstimeofday(&ts_postidle); - ts_idle = timespec_sub(ts_postidle, ts_preidle); - - local_irq_enable(); local_fiq_enable();
- idle_time = ts_idle.tv_nsec / NSEC_PER_USEC + ts_idle.tv_sec * \ - USEC_PER_SEC; - - /* Update cpuidle counters */ - dev->last_residency = idle_time; - return index; }
DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev);
struct cpuidle_driver omap4_idle_driver = { - .name = "omap4_idle", - .owner = THIS_MODULE, + .name = "omap4_idle", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, };
static inline void _fill_cstate(struct cpuidle_driver *drv,
Use newly added core cpuidle functionality and remove this duplicated code from the platform cpuidle.
Signed-off-by: Robert Lee rob.lee@linaro.org --- arch/arm/mach-shmobile/cpuidle.c | 22 ++-------------------- 1 files changed, 2 insertions(+), 20 deletions(-)
diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c index 1b23342..e01a73f 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -29,21 +29,8 @@ static int shmobile_cpuidle_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - ktime_t before, after; - - before = ktime_get(); - - local_irq_disable(); - local_fiq_disable(); - shmobile_cpuidle_modes[index]();
- local_irq_enable(); - local_fiq_enable(); - - after = ktime_get(); - dev->last_residency = ktime_to_ns(ktime_sub(after, before)) >> 10; - return index; }
@@ -51,13 +38,8 @@ static struct cpuidle_device shmobile_cpuidle_dev; static struct cpuidle_driver shmobile_cpuidle_driver = { .name = "shmobile_cpuidle", .owner = THIS_MODULE, - .states[0] = { - .name = "C1", - .desc = "WFI", - .exit_latency = 1, - .target_residency = 1 * 2, - .flags = CPUIDLE_FLAG_TIME_VALID, - }, + .en_core_tk_irqen = 1, + .states[0] = CPUIDLE_ARM_WFI_STATE, .safe_state_index = 0, /* C1 */ .state_count = 1, };
Use newly added core cpuidle functionality and remove this duplicated code from the platform cpuidle.
Signed-off-by: Robert Lee rob.lee@linaro.org --- arch/arm/mach-davinci/cpuidle.c | 77 +++++++++++++++------------------------ 1 files changed, 30 insertions(+), 47 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c index a30c7c5..c3f19d9 100644 --- a/arch/arm/mach-davinci/cpuidle.c +++ b/arch/arm/mach-davinci/cpuidle.c @@ -30,12 +30,42 @@ struct davinci_ops { u32 flags; };
+/* Actual code that puts the SoC in different idle states */ +static int davinci_enter_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, + int index) +{ + struct cpuidle_state_usage *state_usage = &dev->states_usage[index]; + struct davinci_ops *ops = cpuidle_get_statedata(state_usage); + + if (ops && ops->enter) + ops->enter(ops->flags); + + return cpuidle_wrap_enter(dev, drv, index, + cpuidle_simple_enter); + + if (ops && ops->exit) + ops->exit(ops->flags); + + return index; +} + /* fields in davinci_ops.flags */ #define DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN BIT(0)
static struct cpuidle_driver davinci_idle_driver = { .name = "cpuidle-davinci", .owner = THIS_MODULE, + .states[0] = CPUIDLE_ARM_WFI_STATE, + .states[1] = { + .enter = davinci_enter_idle, + .exit_latency = 10, + .target_residency = 100000, + .flags = CPUIDLE_FLAG_TIME_VALID, + .name = "DDR SR", + .desc = "WFI and DDR Self Refresh", + }, + .state_count = DAVINCI_CPUIDLE_MAX_STATES, };
static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device); @@ -77,41 +107,10 @@ static struct davinci_ops davinci_states[DAVINCI_CPUIDLE_MAX_STATES] = { }, };
-/* Actual code that puts the SoC in different idle states */ -static int davinci_enter_idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, - int index) -{ - struct cpuidle_state_usage *state_usage = &dev->states_usage[index]; - struct davinci_ops *ops = cpuidle_get_statedata(state_usage); - struct timeval before, after; - int idle_time; - - local_irq_disable(); - do_gettimeofday(&before); - - if (ops && ops->enter) - ops->enter(ops->flags); - /* Wait for interrupt state */ - cpu_do_idle(); - if (ops && ops->exit) - ops->exit(ops->flags); - - do_gettimeofday(&after); - local_irq_enable(); - idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC + - (after.tv_usec - before.tv_usec); - - dev->last_residency = idle_time; - - return index; -} - static int __init davinci_cpuidle_probe(struct platform_device *pdev) { int ret; struct cpuidle_device *device; - struct cpuidle_driver *driver = &davinci_idle_driver; struct davinci_cpuidle_config *pdata = pdev->dev.platform_data;
device = &per_cpu(davinci_cpuidle_device, smp_processor_id()); @@ -123,27 +122,11 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev)
ddr2_reg_base = pdata->ddr2_ctlr_base;
- /* Wait for interrupt state */ - driver->states[0].enter = davinci_enter_idle; - driver->states[0].exit_latency = 1; - driver->states[0].target_residency = 10000; - driver->states[0].flags = CPUIDLE_FLAG_TIME_VALID; - strcpy(driver->states[0].name, "WFI"); - strcpy(driver->states[0].desc, "Wait for interrupt"); - - /* Wait for interrupt and DDR self refresh state */ - driver->states[1].enter = davinci_enter_idle; - driver->states[1].exit_latency = 10; - driver->states[1].target_residency = 10000; - driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID; - strcpy(driver->states[1].name, "DDR SR"); - strcpy(driver->states[1].desc, "WFI and DDR Self Refresh"); if (pdata->ddr2_pdown) davinci_states[1].flags |= DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN; cpuidle_set_statedata(&device->states_usage[1], &davinci_states[1]);
device->state_count = DAVINCI_CPUIDLE_MAX_STATES; - driver->state_count = DAVINCI_CPUIDLE_MAX_STATES;
ret = cpuidle_register_driver(&davinci_idle_driver); if (ret) {
Use newly added core cpuidle functionality and remove this duplicated code from the platform cpuidle.
Signed-off-by: Robert Lee rob.lee@linaro.org --- arch/arm/mach-exynos/cpuidle.c | 52 ++++------------------------------------ 1 files changed, 5 insertions(+), 47 deletions(-)
diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c index 9bf6743..87f8e3f 100644 --- a/arch/arm/mach-exynos/cpuidle.c +++ b/arch/arm/mach-exynos/cpuidle.c @@ -34,22 +34,12 @@
#define S5P_CHECK_AFTR 0xFCBA0D10
-static int exynos4_enter_idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, - int index); static int exynos4_enter_lowpower(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index);
static struct cpuidle_state exynos4_cpuidle_set[] = { - [0] = { - .enter = exynos4_enter_idle, - .exit_latency = 1, - .target_residency = 100000, - .flags = CPUIDLE_FLAG_TIME_VALID, - .name = "C0", - .desc = "ARM clock gating(WFI)", - }, + [0] = CPUIDLE_ARM_WFI_STATE, [1] = { .enter = exynos4_enter_lowpower, .exit_latency = 300, @@ -63,8 +53,9 @@ static struct cpuidle_state exynos4_cpuidle_set[] = { static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
static struct cpuidle_driver exynos4_idle_driver = { - .name = "exynos4_idle", - .owner = THIS_MODULE, + .name = "exynos4_idle", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, };
/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */ @@ -103,13 +94,8 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - struct timeval before, after; - int idle_time; unsigned long tmp;
- local_irq_disable(); - do_gettimeofday(&before); - exynos4_set_wakeupmask();
/* Set value of power down register for aftr mode */ @@ -148,34 +134,6 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device *dev, /* Clear wakeup state register */ __raw_writel(0x0, S5P_WAKEUP_STAT);
- do_gettimeofday(&after); - - local_irq_enable(); - idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC + - (after.tv_usec - before.tv_usec); - - dev->last_residency = idle_time; - return index; -} - -static int exynos4_enter_idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, - int index) -{ - struct timeval before, after; - int idle_time; - - local_irq_disable(); - do_gettimeofday(&before); - - cpu_do_idle(); - - do_gettimeofday(&after); - local_irq_enable(); - idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC + - (after.tv_usec - before.tv_usec); - - dev->last_residency = idle_time; return index; }
@@ -190,7 +148,7 @@ static int exynos4_enter_lowpower(struct cpuidle_device *dev, new_index = drv->safe_state_index;
if (new_index == 0) - return exynos4_enter_idle(dev, drv, new_index); + return cpuidle_simple_enter(dev, drv, new_index); else return exynos4_enter_core0_aftr(dev, drv, new_index); }
Use newly added core cpuidle functionality and remove this duplicated code from the platform cpuidle.
Signed-off-by: Robert Lee rob.lee@linaro.org --- arch/arm/mach-kirkwood/cpuidle.c | 71 +++++++++++--------------------------- 1 files changed, 20 insertions(+), 51 deletions(-)
diff --git a/arch/arm/mach-kirkwood/cpuidle.c b/arch/arm/mach-kirkwood/cpuidle.c index 7088180..261a2a8 100644 --- a/arch/arm/mach-kirkwood/cpuidle.c +++ b/arch/arm/mach-kirkwood/cpuidle.c @@ -24,73 +24,42 @@
#define KIRKWOOD_MAX_STATES 2
-static struct cpuidle_driver kirkwood_idle_driver = { - .name = "kirkwood_idle", - .owner = THIS_MODULE, -}; - -static DEFINE_PER_CPU(struct cpuidle_device, kirkwood_cpuidle_device); - /* Actual code that puts the SoC in different idle states */ static int kirkwood_enter_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - struct timeval before, after; - int idle_time; - - local_irq_disable(); - do_gettimeofday(&before); - if (index == 0) - /* Wait for interrupt state */ - cpu_do_idle(); - else if (index == 1) { - /* - * Following write will put DDR in self refresh. - * Note that we have 256 cycles before DDR puts it - * self in self-refresh, so the wait-for-interrupt - * call afterwards won't get the DDR from self refresh - * mode. - */ - writel(0x7, DDR_OPERATION_BASE); - cpu_do_idle(); - } - do_gettimeofday(&after); - local_irq_enable(); - idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC + - (after.tv_usec - before.tv_usec); - - /* Update last residency */ - dev->last_residency = idle_time; + writel(0x7, DDR_OPERATION_BASE); + cpu_do_idle();
return index; }
+static struct cpuidle_driver kirkwood_idle_driver = { + .name = "kirkwood_idle", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, + .states[0] = CPUIDLE_ARM_WFI_STATE, + .states[1] = { + .enter = kirkwood_enter_idle, + .exit_latency = 10, + .target_residency = 100000, + .flags = CPUIDLE_FLAG_TIME_VALID, + .name = "DDR SR", + .desc = "WFI and DDR Self Refresh", + }, + .state_count = KIRKWOOD_MAX_STATES, +}; + +static DEFINE_PER_CPU(struct cpuidle_device, kirkwood_cpuidle_device); + /* Initialize CPU idle by registering the idle states */ static int kirkwood_init_cpuidle(void) { struct cpuidle_device *device; - struct cpuidle_driver *driver = &kirkwood_idle_driver;
device = &per_cpu(kirkwood_cpuidle_device, smp_processor_id()); device->state_count = KIRKWOOD_MAX_STATES; - driver->state_count = KIRKWOOD_MAX_STATES; - - /* Wait for interrupt state */ - driver->states[0].enter = kirkwood_enter_idle; - driver->states[0].exit_latency = 1; - driver->states[0].target_residency = 10000; - driver->states[0].flags = CPUIDLE_FLAG_TIME_VALID; - strcpy(driver->states[0].name, "WFI"); - strcpy(driver->states[0].desc, "Wait for interrupt"); - - /* Wait for interrupt and DDR self refresh state */ - driver->states[1].enter = kirkwood_enter_idle; - driver->states[1].exit_latency = 10; - driver->states[1].target_residency = 10000; - driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID; - strcpy(driver->states[1].name, "DDR SR"); - strcpy(driver->states[1].desc, "WFI and DDR Self Refresh");
cpuidle_register_driver(&kirkwood_idle_driver); if (cpuidle_register_device(device)) {
Use newly added core cpuidle functionality and remove this duplicated code from the platform cpuidle.
Signed-off-by: Robert Lee rob.lee@linaro.org --- arch/arm/mach-at91/cpuidle.c | 64 ++++++++++++++--------------------------- 1 files changed, 22 insertions(+), 42 deletions(-)
diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c index a851e6c..10c1564 100644 --- a/arch/arm/mach-at91/cpuidle.c +++ b/arch/arm/mach-at91/cpuidle.c @@ -27,66 +27,46 @@
static DEFINE_PER_CPU(struct cpuidle_device, at91_cpuidle_device);
-static struct cpuidle_driver at91_idle_driver = { - .name = "at91_idle", - .owner = THIS_MODULE, -}; - /* Actual code that puts the SoC in different idle states */ static int at91_enter_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - struct timeval before, after; - int idle_time; u32 saved_lpr;
- local_irq_disable(); - do_gettimeofday(&before); - if (index == 0) - /* Wait for interrupt state */ - cpu_do_idle(); - else if (index == 1) { - asm("b 1f; .align 5; 1:"); - asm("mcr p15, 0, r0, c7, c10, 4"); /* drain write buffer */ - saved_lpr = sdram_selfrefresh_enable(); - cpu_do_idle(); - sdram_selfrefresh_disable(saved_lpr); - } - do_gettimeofday(&after); - local_irq_enable(); - idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC + - (after.tv_usec - before.tv_usec); + __asm__("b 1f; .align 5; 1:\n" + " mcr p15, 0, r0, c7, c10, 4"); /* drain write buffer */ + + saved_lpr = sdram_selfrefresh_enable(); + cpu_do_idle(); + sdram_selfrefresh_disable(saved_lpr);
- dev->last_residency = idle_time; return index; }
+static struct cpuidle_driver at91_idle_driver = { + .name = "at91_idle", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, + .states[0] = CPUIDLE_ARM_WFI_STATE, + .states[1] = { + .enter = at91_enter_idle, + .exit_latency = 10, + .target_residency = 100000, + .flags = CPUIDLE_FLAG_TIME_VALID, + .name = "RAM_SR", + .desc = "WFI and DDR Self Refresh", + }, + .state_count = AT91_MAX_STATES, +}; + /* Initialize CPU idle by registering the idle states */ static int at91_init_cpuidle(void) { struct cpuidle_device *device; - struct cpuidle_driver *driver = &at91_idle_driver;
device = &per_cpu(at91_cpuidle_device, smp_processor_id()); device->state_count = AT91_MAX_STATES; - driver->state_count = AT91_MAX_STATES; - - /* Wait for interrupt state */ - driver->states[0].enter = at91_enter_idle; - driver->states[0].exit_latency = 1; - driver->states[0].target_residency = 10000; - driver->states[0].flags = CPUIDLE_FLAG_TIME_VALID; - strcpy(driver->states[0].name, "WFI"); - strcpy(driver->states[0].desc, "Wait for interrupt"); - - /* Wait for interrupt and RAM self refresh state */ - driver->states[1].enter = at91_enter_idle; - driver->states[1].exit_latency = 10; - driver->states[1].target_residency = 10000; - driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID; - strcpy(driver->states[1].name, "RAM_SR"); - strcpy(driver->states[1].desc, "WFI and RAM Self Refresh");
cpuidle_register_driver(&at91_idle_driver);
CC'ing Venki on this too.
The entire thread can be found at: http://www.spinics.net/lists/arm-kernel/msg161596.html
On Mon, Feb 27, 2012 at 6:47 AM, Robert Lee rob.lee@linaro.org wrote:
This patch series moves vaious functionality duplicated in platform cpuidle drivers to the core cpuidle driver. Also, the platform irq disabling was removed as it appears that all calls into cpuidle_call_idle will have already called local_irq_disable().
Based on 3.3-rc5 plus recent exynos cpuidle patch: http://www.spinics.net/lists/linux-samsung-soc/msg09467.html
v4 submission can be found here: http://lists.infradead.org/pipermail/linux-arm-kernel/2012-January/082742.ht...
Changes since v4:
- Added common cpu_do_idle function to core cpuidle
- Added time keep irq en wrapper to core cpuidle
- Removed pre/post enter
- Re-added platforms that can use new common code.
v3 submission can be found here: http://www.spinics.net/lists/arm-kernel/msg156751.html Changes since v3:
- Removed drivers/cpuidle/common.c
** Removed the initialization helper functions ** Removed the wrapper used to consolidate time keeping and irq enable/disable
- Add time keeping and local_irq_disable handling in cpuidle_call_idle().
- Made necessary modifications to a few platforms that required the most changes
** Note on omap3: changed structure of omap3_idle_drvdata and added per_next_state and per_saved_state vars to accomodate new framework.
v2 submission can be found here: http://comments.gmane.org/gmane.linux.ports.arm.kernel/144199
Changes since v2:
- Made various code organization and style changes as suggested in v1 review.
- Removed at91 use of common code. A separate effort is underway to clean
at91 code and the author has offered to convert to common interface as part of those changes (if this common interface is accepted in time).
- Made platform cpuidle_driver objects __initdata and dynamically added one
persistent instance of this object in common code.
- Removed imx5 pm usage of gpc_dvfs clock as it is no longer needed after
being enabled during clock initialization.
- Re-organized patches.
v1 submission can be found here: http://comments.gmane.org/gmane.linux.ports.arm.kernel/142791
Changes since v1:
- Common interface moved to drivers/cpuidle and made non arch-specific.
- Made various fixes and suggested additions to the common cpuidle
code from v1 review.
- Added callback for filling in driver_data field as needed.
- Modified the various platforms with these changes.
Robert Lee (9): cpuidle: Add commonly used functionality for consolidation SH: shmobile: cpuidle consolidation ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable ARM: omap: Consolidate OMAP4 cpuidle time keeping and irq enable ARM: shmobile: Consolidate cpuidle functionality ARM: davinci: Consolidate cpuidle functionality ARM: exynos: Consolidate cpuidle functionality ARM: kirkwood: Consolidate cpuidle functionality ARM: at91: Consolidate cpuidle functionality
arch/arm/mach-at91/cpuidle.c | 64 +++++++++------------------ arch/arm/mach-davinci/cpuidle.c | 77 +++++++++++++-------------------- arch/arm/mach-exynos/cpuidle.c | 52 ++-------------------- arch/arm/mach-kirkwood/cpuidle.c | 71 +++++++++---------------------- arch/arm/mach-omap2/cpuidle34xx.c | 43 ++++++++----------- arch/arm/mach-omap2/cpuidle44xx.c | 21 +-------- arch/arm/mach-shmobile/cpuidle.c | 22 +--------- arch/sh/kernel/cpu/shmobile/cpuidle.c | 10 +--- drivers/cpuidle/cpuidle.c | 37 +++++++++------ include/linux/cpuidle.h | 55 +++++++++++++++++++++++ 10 files changed, 180 insertions(+), 272 deletions(-)
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-
Amit Kucheria -
Jean Pihet -
Rob Herring -
Rob Lee -
Robert Lee
-
Turquette, Mike