All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/9] Consolidate cpuidle functionality
@ 2012-02-27  4:47 Robert Lee
  2012-02-27  4:47 ` [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation Robert Lee
                   ` (9 more replies)
  0 siblings, 10 replies; 27+ messages in thread
From: Robert Lee @ 2012-02-27  4:47 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
@ 2012-02-27  4:47 ` Robert Lee
  2012-02-27 16:19   ` Jean Pihet
                     ` (2 more replies)
  2012-02-27  4:47 ` [PATCH v5 2/9] SH: shmobile: cpuidle consolidation Robert Lee
                   ` (8 subsequent siblings)
  9 siblings, 3 replies; 27+ messages in thread
From: Robert Lee @ 2012-02-27  4:47 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v5 2/9] SH: shmobile: cpuidle consolidation
  2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
  2012-02-27  4:47 ` [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation Robert Lee
@ 2012-02-27  4:47 ` Robert Lee
  2012-02-27 15:13     ` Rob Lee
  2012-02-27  4:47 ` [PATCH v5 3/9] ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable Robert Lee
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 27+ messages in thread
From: Robert Lee @ 2012-02-27  4:47 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v5 3/9] ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable
  2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
  2012-02-27  4:47 ` [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation Robert Lee
  2012-02-27  4:47 ` [PATCH v5 2/9] SH: shmobile: cpuidle consolidation Robert Lee
@ 2012-02-27  4:47 ` Robert Lee
  2012-02-27 16:26   ` Jean Pihet
  2012-02-27  4:47 ` [PATCH v5 4/9] ARM: omap: Consolidate OMAP4 " Robert Lee
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 27+ messages in thread
From: Robert Lee @ 2012-02-27  4:47 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v5 4/9] ARM: omap: Consolidate OMAP4 cpuidle time keeping and irq enable
  2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
                   ` (2 preceding siblings ...)
  2012-02-27  4:47 ` [PATCH v5 3/9] ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable Robert Lee
@ 2012-02-27  4:47 ` Robert Lee
  2012-02-27  4:47 ` [PATCH v5 5/9] ARM: shmobile: Consolidate cpuidle functionality Robert Lee
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 27+ messages in thread
From: Robert Lee @ 2012-02-27  4:47 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v5 5/9] ARM: shmobile: Consolidate cpuidle functionality
  2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
                   ` (3 preceding siblings ...)
  2012-02-27  4:47 ` [PATCH v5 4/9] ARM: omap: Consolidate OMAP4 " Robert Lee
@ 2012-02-27  4:47 ` Robert Lee
  2012-02-27  4:47 ` [PATCH v5 6/9] ARM: davinci: " Robert Lee
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 27+ messages in thread
From: Robert Lee @ 2012-02-27  4:47 UTC (permalink / raw)
  To: linux-arm-kernel

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,
 };
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v5 6/9] ARM: davinci: Consolidate cpuidle functionality
  2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
                   ` (4 preceding siblings ...)
  2012-02-27  4:47 ` [PATCH v5 5/9] ARM: shmobile: Consolidate cpuidle functionality Robert Lee
@ 2012-02-27  4:47 ` Robert Lee
  2012-02-27  4:47 ` [PATCH v5 7/9] ARM: exynos: " Robert Lee
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 27+ messages in thread
From: Robert Lee @ 2012-02-27  4:47 UTC (permalink / raw)
  To: linux-arm-kernel

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) {
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v5 7/9] ARM: exynos: Consolidate cpuidle functionality
  2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
                   ` (5 preceding siblings ...)
  2012-02-27  4:47 ` [PATCH v5 6/9] ARM: davinci: " Robert Lee
@ 2012-02-27  4:47 ` Robert Lee
  2012-02-27  4:47 ` [PATCH v5 8/9] ARM: kirkwood: " Robert Lee
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 27+ messages in thread
From: Robert Lee @ 2012-02-27  4:47 UTC (permalink / raw)
  To: linux-arm-kernel

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);
 }
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v5 8/9] ARM: kirkwood: Consolidate cpuidle functionality
  2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
                   ` (6 preceding siblings ...)
  2012-02-27  4:47 ` [PATCH v5 7/9] ARM: exynos: " Robert Lee
@ 2012-02-27  4:47 ` Robert Lee
  2012-02-27  4:47 ` [PATCH v5 9/9] ARM: at91: " Robert Lee
  2012-02-27 16:32 ` [PATCH v5 0/9] " Amit Kucheria
  9 siblings, 0 replies; 27+ messages in thread
From: Robert Lee @ 2012-02-27  4:47 UTC (permalink / raw)
  To: linux-arm-kernel

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)) {
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v5 9/9] ARM: at91: Consolidate cpuidle functionality
  2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
                   ` (7 preceding siblings ...)
  2012-02-27  4:47 ` [PATCH v5 8/9] ARM: kirkwood: " Robert Lee
@ 2012-02-27  4:47 ` Robert Lee
  2012-02-27 16:32 ` [PATCH v5 0/9] " Amit Kucheria
  9 siblings, 0 replies; 27+ messages in thread
From: Robert Lee @ 2012-02-27  4:47 UTC (permalink / raw)
  To: linux-arm-kernel

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);
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v5 2/9] SH: shmobile: cpuidle consolidation
  2012-02-27  4:47 ` [PATCH v5 2/9] SH: shmobile: cpuidle consolidation Robert Lee
@ 2012-02-27 15:13     ` Rob Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Rob Lee @ 2012-02-27 15:13 UTC (permalink / raw)
  To: linux-arm-kernel

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
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 2/9] SH: shmobile: cpuidle consolidation
@ 2012-02-27 15:13     ` Rob Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Rob Lee @ 2012-02-27 15:13 UTC (permalink / raw)
  To: linux-arm-kernel

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
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-27  4:47 ` [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation Robert Lee
@ 2012-02-27 16:19   ` Jean Pihet
  2012-02-27 19:23     ` Rob Lee
  2012-02-28  0:06   ` Turquette, Mike
  2012-02-28  0:49   ` Turquette, Mike
  2 siblings, 1 reply; 27+ messages in thread
From: Jean Pihet @ 2012-02-27 16:19 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 3/9] ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable
  2012-02-27  4:47 ` [PATCH v5 3/9] ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable Robert Lee
@ 2012-02-27 16:26   ` Jean Pihet
  2012-02-27 19:27     ` Rob Lee
  0 siblings, 1 reply; 27+ messages in thread
From: Jean Pihet @ 2012-02-27 16:26 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 0/9] Consolidate cpuidle functionality
  2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
                   ` (8 preceding siblings ...)
  2012-02-27  4:47 ` [PATCH v5 9/9] ARM: at91: " Robert Lee
@ 2012-02-27 16:32 ` Amit Kucheria
  9 siblings, 0 replies; 27+ messages in thread
From: Amit Kucheria @ 2012-02-27 16:32 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-27 16:19   ` Jean Pihet
@ 2012-02-27 19:23     ` Rob Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Rob Lee @ 2012-02-27 19:23 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 3/9] ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable
  2012-02-27 16:26   ` Jean Pihet
@ 2012-02-27 19:27     ` Rob Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Rob Lee @ 2012-02-27 19:27 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-27  4:47 ` [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation Robert Lee
  2012-02-27 16:19   ` Jean Pihet
@ 2012-02-28  0:06   ` Turquette, Mike
  2012-02-28 15:45     ` Rob Lee
  2012-02-28  0:49   ` Turquette, Mike
  2 siblings, 1 reply; 27+ messages in thread
From: Turquette, Mike @ 2012-02-28  0:06 UTC (permalink / raw)
  To: 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;
> +}

Any reason that this code is in the header?  Why not in cpuidle.c?

Regards,
Mike

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-27  4:47 ` [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation Robert Lee
  2012-02-27 16:19   ` Jean Pihet
  2012-02-28  0:06   ` Turquette, Mike
@ 2012-02-28  0:49   ` Turquette, Mike
  2012-02-28 15:50     ` Rob Lee
  2 siblings, 1 reply; 27+ messages in thread
From: Turquette, Mike @ 2012-02-28  0:49 UTC (permalink / raw)
  To: 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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-28  0:06   ` Turquette, Mike
@ 2012-02-28 15:45     ` Rob Lee
  2012-02-28 15:58       ` Rob Herring
  0 siblings, 1 reply; 27+ messages in thread
From: Rob Lee @ 2012-02-28 15:45 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-28  0:49   ` Turquette, Mike
@ 2012-02-28 15:50     ` Rob Lee
  2012-02-28 21:42       ` Turquette, Mike
  0 siblings, 1 reply; 27+ messages in thread
From: Rob Lee @ 2012-02-28 15:50 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-28 15:45     ` Rob Lee
@ 2012-02-28 15:58       ` Rob Herring
  2012-02-28 20:49         ` Rob Lee
  0 siblings, 1 reply; 27+ messages in thread
From: Rob Herring @ 2012-02-28 15:58 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-28 15:58       ` Rob Herring
@ 2012-02-28 20:49         ` Rob Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Rob Lee @ 2012-02-28 20:49 UTC (permalink / raw)
  To: 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 at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-28 15:50     ` Rob Lee
@ 2012-02-28 21:42       ` Turquette, Mike
  2012-02-28 23:33         ` Rob Lee
  0 siblings, 1 reply; 27+ messages in thread
From: Turquette, Mike @ 2012-02-28 21:42 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-28 21:42       ` Turquette, Mike
@ 2012-02-28 23:33         ` Rob Lee
  2012-02-28 23:47           ` Turquette, Mike
  0 siblings, 1 reply; 27+ messages in thread
From: Rob Lee @ 2012-02-28 23:33 UTC (permalink / raw)
  To: linux-arm-kernel

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

>
> Regards,
> Mike
>
>>
>>>
>>> Regards,
>>> Mike

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-28 23:33         ` Rob Lee
@ 2012-02-28 23:47           ` Turquette, Mike
  2012-02-29  0:04             ` Rob Lee
  0 siblings, 1 reply; 27+ messages in thread
From: Turquette, Mike @ 2012-02-28 23:47 UTC (permalink / raw)
  To: linux-arm-kernel

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

>
>>
>> Regards,
>> Mike
>>
>>>
>>>>
>>>> Regards,
>>>> Mike

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation
  2012-02-28 23:47           ` Turquette, Mike
@ 2012-02-29  0:04             ` Rob Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Rob Lee @ 2012-02-29  0:04 UTC (permalink / raw)
  To: linux-arm-kernel

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

Yes.

>
> Regards,
> Mike
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2012-02-29  0:04 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-27  4:47 [PATCH v5 0/9] Consolidate cpuidle functionality Robert Lee
2012-02-27  4:47 ` [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation Robert Lee
2012-02-27 16:19   ` Jean Pihet
2012-02-27 19:23     ` Rob Lee
2012-02-28  0:06   ` Turquette, Mike
2012-02-28 15:45     ` Rob Lee
2012-02-28 15:58       ` Rob Herring
2012-02-28 20:49         ` Rob Lee
2012-02-28  0:49   ` Turquette, Mike
2012-02-28 15:50     ` Rob Lee
2012-02-28 21:42       ` Turquette, Mike
2012-02-28 23:33         ` Rob Lee
2012-02-28 23:47           ` Turquette, Mike
2012-02-29  0:04             ` Rob Lee
2012-02-27  4:47 ` [PATCH v5 2/9] SH: shmobile: cpuidle consolidation Robert Lee
2012-02-27 15:13   ` Rob Lee
2012-02-27 15:13     ` Rob Lee
2012-02-27  4:47 ` [PATCH v5 3/9] ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable Robert Lee
2012-02-27 16:26   ` Jean Pihet
2012-02-27 19:27     ` Rob Lee
2012-02-27  4:47 ` [PATCH v5 4/9] ARM: omap: Consolidate OMAP4 " Robert Lee
2012-02-27  4:47 ` [PATCH v5 5/9] ARM: shmobile: Consolidate cpuidle functionality Robert Lee
2012-02-27  4:47 ` [PATCH v5 6/9] ARM: davinci: " Robert Lee
2012-02-27  4:47 ` [PATCH v5 7/9] ARM: exynos: " Robert Lee
2012-02-27  4:47 ` [PATCH v5 8/9] ARM: kirkwood: " Robert Lee
2012-02-27  4:47 ` [PATCH v5 9/9] ARM: at91: " Robert Lee
2012-02-27 16:32 ` [PATCH v5 0/9] " Amit Kucheria

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.