Consolidate cpuidle functionality

Add functionality that is commonly duplicated by various platforms.

Signed-off-by Robert Lee <[email protected]>

---
?drivers/cpuidle/cpuidle.c | ? 37 ++++++++++++++++++------------ ?include/linux/cpuidle.h ? | ? 55 +++++++++++++++++++++++++++++++++++++++++++++ ?2 files changed, 77 insertions(+), 15 deletions(-)

drivers/cpuidle/cpuidle.c +0 -0

include/linux/cpuidle.h +0 -0

index 712abcc..6563683 100644
@@ -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

include/linux/cpuidle.h +0 -0

@@ -56,6 +57,16 @@ struct cpuidle_state {
 
 ?#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)

include/linux/cpuidle.h +1 -0

+

?/**
?* cpuidle_get_statedata - retrieves private driver state data ?* @st_usage: the state usage statistics

include/linux/cpuidle.h +0 -0

@@ -122,6 +133,14 @@ struct cpuidle_driver {
 ? ? ? ?struct module ? ? ? ? ? *owner;

include/linux/cpuidle.h +3 -0

 ? ? ? ?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);
+

include/linux/cpuidle.h +20 -0

+{
+ ? ? ? 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) { }

Robert,

Quoted 9 lines · Robert Lee

Add functionality that is commonly duplicated by various platforms.

Signed-off-by Robert Lee <[email protected]>

---
?drivers/cpuidle/cpuidle.c | ? 37 ++++++++++++++++++------------ ?include/linux/cpuidle.h ? | ? 55 +++++++++++++++++++++++++++++++++++++++++++++ ?2 files changed, 77 insertions(+), 15 deletions(-)

drivers/cpuidle/cpuidle.c +0 -0

...

Quoted 16 lines

include/linux/cpuidle.h +0 -0

index 712abcc..6563683 100644
@@ -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

include/linux/cpuidle.h +0 -0

@@ -56,6 +57,16 @@ struct cpuidle_state {
 
 ?#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
 

include/linux/cpuidle.h struct cpuidle_state { · line 68

Quoted 9 lines

include/linux/cpuidle.h +9 -0

+/* 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.

Quoted 7 lines

include/linux/cpuidle.h +1 -0

+

?/**
?* cpuidle_get_statedata - retrieves private driver state data ?* @st_usage: the state usage statistics

include/linux/cpuidle.h +0 -0

@@ -122,6 +133,14 @@ struct cpuidle_driver {
 ? ? ? ?struct module ? ? ? ? ? *owner;
 

include/linux/cpuidle.h struct cpuidle_driver { · line 143

Quoted 9 lines

include/linux/cpuidle.h +8 -0

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

Quoted 14 lines

include/linux/cpuidle.h +7 -0

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

include/linux/cpuidle.h extern void cpuidle_pause_and_lock(void);

Quoted 6 lines

include/linux/cpuidle.h +6 -0

+ * @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).

Quoted 21 lines

include/linux/cpuidle.h +20 -0

+{
+ ? ? ? 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;
+}
 

Quoted 2 lines

?#else
?static inline void disable_cpuidle(void) { }

...

Regards,
Jean

include/linux/cpuidle.h +20 -0

+/**
+ * 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();
+

Quoted 24 lines · Robert Lee

include/linux/cpuidle.h +24 -0

+/**
+ * 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)

include/linux/cpuidle.h extern void cpuidle_pause_and_lock(void); · line 194

Quoted 6 lines

+ ? ? ? ? ? ? ? diff = INT_MAX;

include/linux/cpuidle.h +5 -0

+
+ ? ? ? 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,

Quoted 25 lines · Mike

Quoted 24 lines · Robert Lee

include/linux/cpuidle.h +24 -0

+/**
+ * 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)

Quoted 9 lines

include/linux/cpuidle.h extern void cpuidle_pause_and_lock(void); · line 194

Quoted 6 lines

include/linux/cpuidle.h +6 -0

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

Quoted 2 lines

Regards,
Mike

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 14 lines

Quoted 2 lines

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

Quoted 11 lines

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.

Quoted 2 lines

Regards,
Mike

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

include/linux/cpuidle.h +20 -0

+/**
+ * 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();
+

Quoted 20 lines · Robert Lee

include/linux/cpuidle.h +20 -0

+/**
+ * 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();
+

include/linux/cpuidle.h extern void cpuidle_pause_and_lock(void); · line 194

Quoted 10 lines

include/linux/cpuidle.h +10 -0

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

Quoted 39 lines · Mike

Quoted 20 lines · Robert Lee

include/linux/cpuidle.h +20 -0

+/**
+ * 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();
+

include/linux/cpuidle.h extern void cpuidle_pause_and_lock(void); · line 194

Quoted 10 lines

include/linux/cpuidle.h +10 -0

+ ? ? ? 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/

Quoted 8 lines

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.

Quoted 4 lines

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?

Regards,
Mike

Quoted 4 lines

Quoted 4 lines

Regards,
Mike

Quoted 9 lines

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

Quoted 6 lines

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_attempted_and_successful_enter_attempts

Quoted 9 lines

Regards,
Mike

Quoted 4 lines

Quoted 4 lines

Regards,
Mike

Quoted 7 lines

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_attempted_and_successful_enter_attempts

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

Enable core cpuidle timekeeping and irq enabling and remove that handling from this code.

Signed-off-by Robert Lee <[email protected]>

---
?arch/sh/kernel/cpu/shmobile/cpuidle.c | ? 10 +++------- ?1 files changed, 3 insertions(+), 7 deletions(-)

arch/sh/kernel/cpu/shmobile/cpuidle.c +0 -1

index 6d62eb4..1ddc876 100644
@@ -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 <[email protected]>

---
?arch/arm/mach-omap2/cpuidle34xx.c | ? 43 +++++++++++++++--------------------- ?1 files changed, 18 insertions(+), 25 deletions(-)

arch/arm/mach-omap2/cpuidle34xx.c +0 -0

index 464cffd..1f6123f 100644

?/**
+ * 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.
+ */

arch/arm/mach-omap2/cpuidle34xx.c +3 -0

+}
+
+/**

?* next_valid_state - Find next valid C-state
?* @dev: cpuidle device
?* @drv: cpuidle driver

arch/arm/mach-omap2/cpuidle34xx.c +0 -0

@@ -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 */ --
1.7.1

Quoted 12 lines · Robert Lee

Use core cpuidle timekeeping and irqen wrapper and remove that handling from this code.

Signed-off-by Robert Lee <[email protected]>

---
?arch/arm/mach-omap2/cpuidle34xx.c | ? 43 +++++++++++++++--------------------- ?1 files changed, 18 insertions(+), 25 deletions(-)

arch/arm/mach-omap2/cpuidle34xx.c +0 -0

index 464cffd..1f6123f 100644

...

Quoted 10 lines

?/**
+ * 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,

Quoted 4 lines

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

Quoted 9 lines

arch/arm/mach-omap2/cpuidle34xx.c +3 -0

+}
+
+/**

?* next_valid_state - Find next valid C-state
?* @dev: cpuidle device
?* @drv: cpuidle driver

arch/arm/mach-omap2/cpuidle34xx.c +0 -0

@@ -295,6 +287,7 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
 ? ? ? ?new_state_idx = next_valid_state(dev, drv, index);
 

Quoted 2 lines

?select_state:
+

Stray change

Quoted 6 lines

? ? ? ?ret = omap3_enter_idle(dev, drv, new_state_idx);

? ? ? ?/* Restore original PER state if it was modified */ --
1.7.1

Regards,
Jean

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

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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel