[PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation

On Mon, Feb 27, 2012 at 10:19 AM, Jean Pihet [off-list ref] wrote:

Robert,

On Mon, Feb 27, 2012 at 5:47 AM, Robert Lee [off-list ref] wrote:

quoted

Add functionality that is commonly duplicated by various platforms.

Signed-off-by: Robert Lee <redacted>
---
?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

...

quoted

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.

quoted

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

quoted

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

quoted

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