[PATCH v3 0/7] Add common cpuidle code for consolidation.

[PATCH v3 0/7] Add common cpuidle code for consolidation.

[Robert Lee]

This patch series adds a new common cpuidle interface to consolidate code
commonly duplicated by various platforms.  A patch was then made for each
platform that could immediately take advantage of this code.  The platform
specific changes are not required by the common code and are only made for
consoldation.

Maintainers and cpuidle idle developers of these platforms, please check to make
sure that you agree with the changes.  Besides just code consolidation, a
default "WFI" state is now used with default parameters that different from your
original paramenters. The assumption is that if you have a WFI only idle state,
the parameters in the new default WFI are more realistic as a true WFI only
hardware state incurs minimal latency(<1us) or power penalty to enter and exit.
If your platform actually performs other platform specific functionality upon
entering WFI and the default parameters do not accurately reflect the 
exit_latency and target_residency given in the common default state, please
say so.  Also, the default state uses a common name and description value
which may differ from the previous platform specific values you used.

Lastly, a imx5 cpuidle implementation is added which uses the common cpuidle
interface.

Based on 3.3-rc1

Tested on i.MX51 Babbage Board

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 (7):
  cpuidle: Add common init interface and idle functionality
  ARM: exynos: Modify to use new common cpuidle code.
  ARM: shmobile: Modify to use new common cpuidle code.
  ARM: kirkwood: Modify to use new common cpuidle code.
  ARM: davinci: Modify to use new common cpuidle code.
  ARM: imx: Init imx5 gpc_dvfs clock for global use
  ARM: imx: Add imx5 cpuidle implementation

 arch/arm/mach-davinci/cpuidle.c     |  135 ++++++++-----------------------
 arch/arm/mach-exynos/cpuidle.c      |   73 ++---------------
 arch/arm/mach-kirkwood/cpuidle.c    |   89 +++++---------------
 arch/arm/mach-mx5/Makefile          |    3 +-
 arch/arm/mach-mx5/clock-mx51-mx53.c |    3 +
 arch/arm/mach-mx5/cpuidle.c         |   64 +++++++++++++++
 arch/arm/mach-mx5/pm-imx5.c         |   24 +-----
 arch/arm/mach-shmobile/cpuidle.c    |   51 ++----------
 drivers/cpuidle/Makefile            |    2 +-
 drivers/cpuidle/common.c            |  152 +++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h             |   24 ++++++
 11 files changed, 319 insertions(+), 301 deletions(-)
 create mode 100644 arch/arm/mach-mx5/cpuidle.c
 create mode 100644 drivers/cpuidle/common.c

[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

[Robert Lee]

The patch adds some cpuidle initialization functionality commonly
duplicated by many platforms.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 drivers/cpuidle/Makefile |    2 +-
 drivers/cpuidle/common.c |  152 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h  |   24 +++++++
 3 files changed, 177 insertions(+), 1 deletions(-)
 create mode 100644 drivers/cpuidle/common.c

diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 5634f88..2928d93 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -2,4 +2,4 @@
 # Makefile for cpuidle.
 #
 
-obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
+obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/
diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
new file mode 100644
index 0000000..dafa758
--- /dev/null
+++ b/drivers/cpuidle/common.c
@@ -0,0 +1,152 @@
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc.
+ * Copyright 2011 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/*
+ * This code performs provides some commonly used cpuidle setup functionality
+ * used by many ARM SoC platforms.  Providing this functionality here
+ * reduces the duplication of this code for each ARM platform that uses it.
+ */
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/cpuidle.h>
+#include <linux/hrtimer.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <asm/proc-fns.h>
+
+static struct cpuidle_device __percpu * common_cpuidle_devices;
+
+static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv, int index);
+
+int cpuidle_def_idle(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv, int index)
+{
+	cpu_do_idle();
+	return index;
+}
+
+static int simple_enter(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv, int index)
+{
+	ktime_t time_start, time_end;
+
+	local_irq_disable();
+
+	time_start = ktime_get();
+
+	index = do_idle[index](dev, drv, index);
+
+	time_end = ktime_get();
+
+	local_irq_enable();
+
+	dev->last_residency =
+		(int)ktime_to_us(ktime_sub(time_end, time_start));
+
+	return index;
+}
+
+void common_cpuidle_devices_uninit(void)
+{
+	int cpu_id;
+	struct cpuidle_device *dev;
+
+	for_each_possible_cpu(cpu_id) {
+		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
+		cpuidle_unregister_device(dev);
+	}
+
+	free_percpu(common_cpuidle_devices);
+}
+
+/**
+ * common_cpuidle_init() - Provides some commonly used init functionality.
+ * @pdrv		Pointer to your cpuidle_driver object.
+ * @simple		Use the common simple_enter wrapper?
+ * @driver_data_init	Pointer to your platform function to initialize your
+ *			platform specific driver data.  Use NULL if platform
+ *			specific data is not needed.
+ *
+ * Common cpuidle init interface to provide common cpuidle functionality
+ * used by many platforms.
+ */
+int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
+			 void (*driver_data_init)(struct cpuidle_device *dev))
+{
+	struct cpuidle_device *dev;
+	struct cpuidle_driver *drv;
+	int i, cpu_id, ret;
+
+	if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) {
+		pr_err("%s: Invalid Input\n", __func__);
+		return -EINVAL;
+	}
+
+	drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
+	if (!drv) {
+		pr_err("%s: no memory for cpuidle driver\n", __func__);
+		return -ENOMEM;
+	}
+
+	*drv = *pdrv;
+
+	for (i = 0; simple && (i < drv->state_count); i++) {
+		do_idle[i] = drv->states[i].enter;
+		drv->states[i].enter = simple_enter;
+	}
+
+	ret = cpuidle_register_driver(drv);
+	if (ret) {
+		pr_err("%s: Failed to register cpuidle driver\n", __func__);
+		goto free_drv;
+	}
+
+	common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+	if (common_cpuidle_devices == NULL) {
+		ret = -ENOMEM;
+		goto unregister_drv;
+	}
+
+	/* initialize state data for each cpuidle_device */
+	for_each_possible_cpu(cpu_id) {
+		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
+		dev->cpu = cpu_id;
+		dev->state_count = drv->state_count;
+
+		if (driver_data_init)
+			driver_data_init(dev);
+
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("%s: Failed to register cpu %u\n",
+				__func__, cpu_id);
+			goto uninit;
+		}
+	}
+
+	return 0;
+uninit:
+
+	common_cpuidle_devices_uninit();
+
+unregister_drv:
+
+	cpuidle_unregister_driver(drv);
+
+free_drv:
+
+	kfree(drv);
+
+	return ret;
+}
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 712abcc..2aa89db 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -56,6 +56,16 @@ struct cpuidle_state {
 
 #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
 
+/* Common ARM WFI state */
+#define CPUIDLE_ARM_WFI_STATE {\
+	.enter                  = cpuidle_def_idle,\
+	.exit_latency           = 2,\
+	.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
@@ -141,6 +151,13 @@ 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);
 
+/* provide a default idle function */
+extern int cpuidle_def_idle(struct cpuidle_device *dev,
+		       struct cpuidle_driver *drv, int index);
+extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple,
+			 void (*driver_data_init)(struct cpuidle_device *dev));
+extern void common_cpuidle_devices_uninit(void);
+
 #else
 static inline void disable_cpuidle(void) { }
 static inline int cpuidle_idle_call(void) { return -ENODEV; }
@@ -157,6 +174,13 @@ 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_def_idle(struct cpuidle_device *dev,
+		       struct cpuidle_driver *drv, int index)
+{return -ENODEV; }
+static inline int common_cpuidle_init(struct cpuidle_driver *pdrv,
+	bool simple, void (*driver_data_init)(struct cpuidle_device *dev))
+{return -ENODEV; }
+static inline void common_cpuidle_devices_uninit(void) { }
 
 #endif
 
-- 
1.7.1

[PATCH v3 2/7] ARM: exynos: Modify to use new common cpuidle code.

[Robert Lee]

Make necessary changes for consolidation with new common cpuidle code.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-exynos/cpuidle.c |   73 +++------------------------------------
 1 files changed, 6 insertions(+), 67 deletions(-)

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index 4ebb382..baa1178 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -13,80 +13,19 @@
 #include <linux/cpuidle.h>
 #include <linux/io.h>
 #include <linux/export.h>
-#include <linux/time.h>
-
 #include <asm/proc-fns.h>
 
-static int exynos4_enter_idle(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			= "IDLE",
-		.desc			= "ARM clock gating(WFI)",
-	},
-};
-
-static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
-
-static struct cpuidle_driver exynos4_idle_driver = {
+static struct cpuidle_driver exynos4_idle_driver __initdata = {
 	.name		= "exynos4_idle",
 	.owner		= THIS_MODULE,
+	.states[0]	= CPUIDLE_ARM_WFI_STATE,
+	.state_count	= 1,
 };
 
-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;
-}
-
 static int __init exynos4_init_cpuidle(void)
 {
-	int i, max_cpuidle_state, cpu_id;
-	struct cpuidle_device *device;
-	struct cpuidle_driver *drv = &exynos4_idle_driver;
-
-	/* Setup cpuidle driver */
-	drv->state_count = (sizeof(exynos4_cpuidle_set) /
-				       sizeof(struct cpuidle_state));
-	max_cpuidle_state = drv->state_count;
-	for (i = 0; i < max_cpuidle_state; i++) {
-		memcpy(&drv->states[i], &exynos4_cpuidle_set[i],
-				sizeof(struct cpuidle_state));
-	}
-	cpuidle_register_driver(&exynos4_idle_driver);
-
-	for_each_cpu(cpu_id, cpu_online_mask) {
-		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
-		device->cpu = cpu_id;
-
-		device->state_count = drv->state_count;
-
-		if (cpuidle_register_device(device)) {
-			printk(KERN_ERR "CPUidle register device failed\n,");
-			return -EIO;
-		}
-	}
-	return 0;
+	return common_cpuidle_init(&exynos4_idle_driver,
+					true,
+					NULL);
 }
 device_initcall(exynos4_init_cpuidle);
-- 
1.7.1

[PATCH v3 3/7] ARM: shmobile: Modify to use new common cpuidle code.

[Robert Lee]

Make necessary changes for consolidation with new common cpuidle code.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-shmobile/cpuidle.c |   51 ++++++-------------------------------
 1 files changed, 9 insertions(+), 42 deletions(-)

diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c
index 1b23342..2a4eaef 100644
--- a/arch/arm/mach-shmobile/cpuidle.c
+++ b/arch/arm/mach-shmobile/cpuidle.c
@@ -16,71 +16,38 @@
 #include <asm/system.h>
 #include <asm/io.h>
 
-static void shmobile_enter_wfi(void)
-{
-	cpu_do_idle();
-}
-
-void (*shmobile_cpuidle_modes[CPUIDLE_STATE_MAX])(void) = {
-	shmobile_enter_wfi, /* regular sleep mode */
-};
+void (*shmobile_cpuidle_modes[CPUIDLE_STATE_MAX])(void);
 
 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;
 }
 
-static struct cpuidle_device shmobile_cpuidle_dev;
-static struct cpuidle_driver shmobile_cpuidle_driver = {
+static struct cpuidle_driver shmobile_cpuidle_driver __initdata = {
 	.name =		"shmobile_cpuidle",
 	.owner =	THIS_MODULE,
-	.states[0] = {
-		.name = "C1",
-		.desc = "WFI",
-		.exit_latency = 1,
-		.target_residency = 1 * 2,
-		.flags = CPUIDLE_FLAG_TIME_VALID,
-	},
-	.safe_state_index = 0, /* C1 */
+	.states[0] = CPUIDLE_ARM_WFI_STATE,
+	.safe_state_index = 0,
 	.state_count = 1,
 };
 
 void (*shmobile_cpuidle_setup)(struct cpuidle_driver *drv);
 
-static int shmobile_cpuidle_init(void)
+static int __init shmobile_cpuidle_init(void)
 {
-	struct cpuidle_device *dev = &shmobile_cpuidle_dev;
-	struct cpuidle_driver *drv = &shmobile_cpuidle_driver;
 	int i;
+	struct cpuidle_driver *drv = &shmobile_cpuidle_driver;
 
-	for (i = 0; i < CPUIDLE_STATE_MAX; i++)
+	for (i = drv->state_count; i < CPUIDLE_STATE_MAX; i++)
 		drv->states[i].enter = shmobile_cpuidle_enter;
 
 	if (shmobile_cpuidle_setup)
-		shmobile_cpuidle_setup(drv);
-
-	cpuidle_register_driver(drv);
-
-	dev->state_count = drv->state_count;
-	cpuidle_register_device(dev);
+		shmobile_cpuidle_setup(&shmobile_cpuidle_driver);
 
-	return 0;
+	return common_cpuidle_init(&shmobile_cpuidle_driver, true, NULL);
 }
 late_initcall(shmobile_cpuidle_init);
-- 
1.7.1

[PATCH v3 4/7] ARM: kirkwood: Modify to use new common cpuidle code.

[Robert Lee]

Make necessary changes for consolidation with new common cpuidle code.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-kirkwood/cpuidle.c |   89 ++++++++++----------------------------
 1 files changed, 23 insertions(+), 66 deletions(-)

diff --git a/arch/arm/mach-kirkwood/cpuidle.c b/arch/arm/mach-kirkwood/cpuidle.c
index 7088180..8b2d1ab 100644
--- a/arch/arm/mach-kirkwood/cpuidle.c
+++ b/arch/arm/mach-kirkwood/cpuidle.c
@@ -24,80 +24,37 @@
 
 #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,
+static int kirkwood_idle_ddr(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
-			       int index)
+				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 __initdata = {
+	.name =         "kirkwood_idle",
+	.owner =        THIS_MODULE,
+	.states[0] = CPUIDLE_ARM_WFI_STATE,
+	.states[1] = {
+		.enter			= kirkwood_idle_ddr,
+		.exit_latency		= 10,
+		.target_residency	= 10000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "DDR SR",
+		.desc			= "WFI and DDR Self Refresh",
+	},
+	.state_count = KIRKWOOD_MAX_STATES,
+};
+
 /* Initialize CPU idle by registering the idle states */
-static int kirkwood_init_cpuidle(void)
+static int __init 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)) {
-		printk(KERN_ERR "kirkwood_init_cpuidle: Failed registering\n");
-		return -EIO;
-	}
-	return 0;
+	return common_cpuidle_init(&kirkwood_idle_driver,
+					true,
+					NULL);
 }
-
 device_initcall(kirkwood_init_cpuidle);
-- 
1.7.1

[PATCH v3 5/7] ARM: davinci: Modify to use new common cpuidle code.

[Robert Lee]

Make necessary changes for consolidation with new common cpuidle code.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-davinci/cpuidle.c |  135 ++++++++++-----------------------------
 1 files changed, 33 insertions(+), 102 deletions(-)

diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
index a30c7c5..8cfc0be 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
@@ -24,98 +24,57 @@
 
 #define DAVINCI_CPUIDLE_MAX_STATES	2
 
-struct davinci_ops {
-	void (*enter) (u32 flags);
-	void (*exit) (u32 flags);
-	u32 flags;
-};
-
-/* 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,
-};
-
-static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device);
+u32 __initdata ddr_reg_mask;
 static void __iomem *ddr2_reg_base;
 
-static void davinci_save_ddr_power(int enter, bool pdown)
+/* idle that includes ddr low power */
+static int davinci_idle_ddr(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv,
+				int index)
 {
 	u32 val;
 
 	val = __raw_readl(ddr2_reg_base + DDR2_SDRCR_OFFSET);
 
-	if (enter) {
-		if (pdown)
-			val |= DDR2_SRPD_BIT;
-		else
-			val &= ~DDR2_SRPD_BIT;
-		val |= DDR2_LPMODEN_BIT;
-	} else {
-		val &= ~(DDR2_SRPD_BIT | DDR2_LPMODEN_BIT);
-	}
+	val |= (u32)dev->states_usage[index].driver_data;
 
 	__raw_writel(val, ddr2_reg_base + DDR2_SDRCR_OFFSET);
-}
 
-static void davinci_c2state_enter(u32 flags)
-{
-	davinci_save_ddr_power(1, !!(flags & DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN));
-}
+	/* Wait for interrupt state */
+	cpu_do_idle();
 
-static void davinci_c2state_exit(u32 flags)
-{
-	davinci_save_ddr_power(0, !!(flags & DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN));
+	val &= ~(DDR2_SRPD_BIT | DDR2_LPMODEN_BIT);
+	__raw_writel(val, ddr2_reg_base + DDR2_SDRCR_OFFSET);
+
+	return index;
 }
 
-static struct davinci_ops davinci_states[DAVINCI_CPUIDLE_MAX_STATES] = {
-	[1] = {
-		.enter	= davinci_c2state_enter,
-		.exit	= davinci_c2state_exit,
+static struct cpuidle_driver davinci_idle_driver __initdata = {
+	.name	= "cpuidle-davinci",
+	.owner	= THIS_MODULE,
+	.states[0] = CPUIDLE_ARM_WFI_STATE,
+	.states[1] = {
+		.enter			= davinci_idle_ddr,
+		.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,
 };
 
-/* 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)
+/* use drive_data field to hold the configured ddr low power bitmask */
+static void __init davinci_cpuidle_dd_init(struct cpuidle_device * dev)
 {
-	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;
+	dev->states_usage[1].driver_data = (void *)ddr_reg_mask;
 }
 
 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());
-
 	if (!pdata) {
 		dev_err(&pdev->dev, "cannot get platform data\n");
 		return -ENOENT;
@@ -123,42 +82,15 @@ 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]);
+		ddr_reg_mask = (DDR2_SRPD_BIT | DDR2_LPMODEN_BIT);
+	else
+		ddr_reg_mask = (DDR2_LPMODEN_BIT);
 
-	device->state_count = DAVINCI_CPUIDLE_MAX_STATES;
-	driver->state_count = DAVINCI_CPUIDLE_MAX_STATES;
+	ret = common_cpuidle_init(&davinci_idle_driver, true,
+					davinci_cpuidle_dd_init);
 
-	ret = cpuidle_register_driver(&davinci_idle_driver);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register driver\n");
-		return ret;
-	}
-
-	ret = cpuidle_register_device(device);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register device\n");
-		cpuidle_unregister_driver(&davinci_idle_driver);
-		return ret;
-	}
-
-	return 0;
+	return ret;
 }
 
 static struct platform_driver davinci_cpuidle_driver = {
@@ -174,4 +106,3 @@ static int __init davinci_cpuidle_init(void)
 						davinci_cpuidle_probe);
 }
 device_initcall(davinci_cpuidle_init);
-
-- 
1.7.1

[PATCH v3 6/7] ARM: imx: Init imx5 gpc_dvfs clock for global use

[Robert Lee]

The gpc_dvfs clock consumes practically zero power and must be enabled
for various low power funcitonality.  Now that a second user of this
clock is being added (cpuidle) for mx5, it is cleanest to just enable
this clock during clock initialization and leave it enabled.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-mx5/clock-mx51-mx53.c |    3 +++
 arch/arm/mach-mx5/pm-imx5.c         |   24 ++----------------------
 2 files changed, 5 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
index 4cb2769..12c8a2b 100644
--- a/arch/arm/mach-mx5/clock-mx51-mx53.c
+++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
@@ -1533,6 +1533,7 @@ static struct clk_lookup mx53_lookups[] = {
 	_REGISTER_CLOCK("imx53-ahci.0", "ahci", sata_clk)
 	_REGISTER_CLOCK("imx53-ahci.0", "ahci_phy", ahci_phy_clk)
 	_REGISTER_CLOCK("imx53-ahci.0", "ahci_dma", ahci_dma_clk)
+	_REGISTER_CLOCK(NULL, "gpc_dvfs", gpc_dvfs_clk)
 };
 
 static void clk_tree_init(void)
@@ -1572,6 +1573,7 @@ int __init mx51_clocks_init(unsigned long ckil, unsigned long osc,
 
 	clk_enable(&cpu_clk);
 	clk_enable(&main_bus_clk);
+	clk_enable(&gpc_dvfs_clk);
 
 	clk_enable(&iim_clk);
 	imx_print_silicon_rev("i.MX51", mx51_revision());
@@ -1615,6 +1617,7 @@ int __init mx53_clocks_init(unsigned long ckil, unsigned long osc,
 	clk_set_parent(&uart_root_clk, &pll3_sw_clk);
 	clk_enable(&cpu_clk);
 	clk_enable(&main_bus_clk);
+	clk_enable(&gpc_dvfs_clk);
 
 	clk_enable(&iim_clk);
 	imx_print_silicon_rev("i.MX53", mx53_revision());
diff --git a/arch/arm/mach-mx5/pm-imx5.c b/arch/arm/mach-mx5/pm-imx5.c
index 98052fc..36c5b57 100644
--- a/arch/arm/mach-mx5/pm-imx5.c
+++ b/arch/arm/mach-mx5/pm-imx5.c
@@ -18,13 +18,6 @@
 #include <mach/hardware.h>
 #include "crm_regs.h"
 
-static struct clk *gpc_dvfs_clk;
-
-static int mx5_suspend_prepare(void)
-{
-	return clk_enable(gpc_dvfs_clk);
-}
-
 static int mx5_suspend_enter(suspend_state_t state)
 {
 	switch (state) {
@@ -50,11 +43,6 @@ static int mx5_suspend_enter(suspend_state_t state)
 	return 0;
 }
 
-static void mx5_suspend_finish(void)
-{
-	clk_disable(gpc_dvfs_clk);
-}
-
 static int mx5_pm_valid(suspend_state_t state)
 {
 	return (state > PM_SUSPEND_ON && state <= PM_SUSPEND_MAX);
@@ -62,21 +50,13 @@ static int mx5_pm_valid(suspend_state_t state)
 
 static const struct platform_suspend_ops mx5_suspend_ops = {
 	.valid = mx5_pm_valid,
-	.prepare = mx5_suspend_prepare,
 	.enter = mx5_suspend_enter,
-	.finish = mx5_suspend_finish,
 };
 
 static int __init mx5_pm_init(void)
 {
-	if (gpc_dvfs_clk == NULL)
-		gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs");
-
-	if (!IS_ERR(gpc_dvfs_clk)) {
-		if (cpu_is_mx51())
-			suspend_set_ops(&mx5_suspend_ops);
-	} else
-		return -EPERM;
+	if (cpu_is_mx51())
+		suspend_set_ops(&mx5_suspend_ops);
 
 	return 0;
 }
-- 
1.7.1

[PATCH v3 7/7] ARM: imx: Add imx5 cpuidle implementation

[Robert Lee]

Add imx5 cpuidle implementation using common cpuidle functionality.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-mx5/Makefile  |    3 +-
 arch/arm/mach-mx5/cpuidle.c |   64 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-mx5/cpuidle.c

diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index 0fc6080..2c348b4 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
@@ -3,8 +3,9 @@
 #
 
 # Object file lists.
-obj-y   := cpu.o mm.o clock-mx51-mx53.o ehci.o system.o
 
+obj-y   := cpu.o mm.o clock-mx51-mx53.o ehci.o system.o
+obj-$(CONFIG_CPU_IDLE) += cpuidle.o
 obj-$(CONFIG_PM) += pm-imx5.o
 obj-$(CONFIG_CPU_FREQ_IMX)    += cpu_op-mx51.o
 obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
diff --git a/arch/arm/mach-mx5/cpuidle.c b/arch/arm/mach-mx5/cpuidle.c
new file mode 100644
index 0000000..230d335
--- /dev/null
+++ b/arch/arm/mach-mx5/cpuidle.c
@@ -0,0 +1,64 @@
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc.
+ * Copyright 2011 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/export.h>
+#include <linux/cpuidle.h>
+#include <asm/proc-fns.h>
+#include <mach/common.h>
+
+int imx5_enter(struct cpuidle_device *dev,
+			struct cpuidle_driver *drv,
+			int index)
+{
+	mx5_cpu_lp_set((unsigned int)dev->states_usage[index].driver_data);
+	cpu_do_idle();
+	return index;
+}
+
+static struct cpuidle_driver imx5_cpuidle_driver __initdata = {
+	.name	= "imx5_cpuidle",
+	.owner	= THIS_MODULE,
+	.states[0] = {
+		.enter			= imx5_enter,
+		.exit_latency		= 12, /* max latency at 160MHz */
+		.target_residency	= 1,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "IMX WFI",
+		.desc			= "CPU and related clocks gated",
+	},
+	.states[1] = {
+		.enter			= imx5_enter,
+		.exit_latency		= 20, /* max latency at 160MHz */
+		.target_residency	= 1,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "IMX SRPG",
+		.desc			= "CPU state retained,powered off",
+	},
+	.state_count = 2,
+};
+
+/* use driver_data field to hold the mx5 idle parameter */
+static void __init imx5_dd_init(struct cpuidle_device * dev)
+{
+	dev->states_usage[0].driver_data = (void *)WAIT_UNCLOCKED;
+	dev->states_usage[1].driver_data = (void *)WAIT_UNCLOCKED_POWER_OFF;
+}
+
+static int __init imx5_init_cpuidle(void)
+{
+	return common_cpuidle_init(&imx5_cpuidle_driver,
+					true,
+					imx5_dd_init);
+}
+late_initcall(imx5_init_cpuidle);
-- 
1.7.1

[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

[Rob Herring]

On 01/23/2012 10:37 PM, Robert Lee wrote:
> The patch adds some cpuidle initialization functionality commonly
> duplicated by many platforms.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>

A few cosmetic comments below, but otherwise looks good.

Reviewed-by: Rob Herring <rob.herring@calxeda.com>

> ---
>  drivers/cpuidle/Makefile |    2 +-
>  drivers/cpuidle/common.c |  152 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/cpuidle.h  |   24 +++++++
>  3 files changed, 177 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/cpuidle/common.c
> 
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 5634f88..2928d93 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -2,4 +2,4 @@
>  # Makefile for cpuidle.
>  #
>  
> -obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
> +obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/
> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
> new file mode 100644
> index 0000000..dafa758
> --- /dev/null
> +++ b/drivers/cpuidle/common.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + * Copyright 2011 Linaro Ltd.

2012 now...

> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/*
> + * This code performs provides some commonly used cpuidle setup functionality
> + * used by many ARM SoC platforms.  Providing this functionality here
> + * reduces the duplication of this code for each ARM platform that uses it.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/cpuidle.h>
> +#include <linux/hrtimer.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <asm/proc-fns.h>
> +
> +static struct cpuidle_device __percpu * common_cpuidle_devices;
> +
> +static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index);
> +
> +int cpuidle_def_idle(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	cpu_do_idle();
> +	return index;
> +}
> +
> +static int simple_enter(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	ktime_t time_start, time_end;
> +
> +	local_irq_disable();
> +
> +	time_start = ktime_get();
> +
> +	index = do_idle[index](dev, drv, index);
> +
> +	time_end = ktime_get();
> +
> +	local_irq_enable();
> +
> +	dev->last_residency =
> +		(int)ktime_to_us(ktime_sub(time_end, time_start));
> +
> +	return index;
> +}
> +
> +void common_cpuidle_devices_uninit(void)
> +{
> +	int cpu_id;
> +	struct cpuidle_device *dev;
> +
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	free_percpu(common_cpuidle_devices);
> +}
> +
> +/**
> + * common_cpuidle_init() - Provides some commonly used init functionality.
> + * @pdrv		Pointer to your cpuidle_driver object.
> + * @simple		Use the common simple_enter wrapper?

remove the ?

> + * @driver_data_init	Pointer to your platform function to initialize your
> + *			platform specific driver data.  Use NULL if platform
> + *			specific data is not needed.
> + *
> + * Common cpuidle init interface to provide common cpuidle functionality
> + * used by many platforms.
> + */
> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
> +			 void (*driver_data_init)(struct cpuidle_device *dev))
> +{
> +	struct cpuidle_device *dev;
> +	struct cpuidle_driver *drv;
> +	int i, cpu_id, ret;
> +
> +	if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) {
> +		pr_err("%s: Invalid Input\n", __func__);

Using pr_fmt rather than function name is preferred.

> +		return -EINVAL;
> +	}
> +
> +	drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
> +	if (!drv) {
> +		pr_err("%s: no memory for cpuidle driver\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	*drv = *pdrv;

Perhaps a comment so it is clear you intend this to be struct copy.

> +
> +	for (i = 0; simple && (i < drv->state_count); i++) {
> +		do_idle[i] = drv->states[i].enter;
> +		drv->states[i].enter = simple_enter;
> +	}
> +
> +	ret = cpuidle_register_driver(drv);
> +	if (ret) {
> +		pr_err("%s: Failed to register cpuidle driver\n", __func__);
> +		goto free_drv;
> +	}
> +
> +	common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (common_cpuidle_devices == NULL) {
> +		ret = -ENOMEM;
> +		goto unregister_drv;
> +	}
> +
> +	/* initialize state data for each cpuidle_device */
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
> +		dev->cpu = cpu_id;
> +		dev->state_count = drv->state_count;
> +
> +		if (driver_data_init)
> +			driver_data_init(dev);
> +
> +		ret = cpuidle_register_device(dev);
> +		if (ret) {
> +			pr_err("%s: Failed to register cpu %u\n",
> +				__func__, cpu_id);
> +			goto uninit;
> +		}
> +	}
> +
> +	return 0;
> +uninit:
> +
> +	common_cpuidle_devices_uninit();
> +
> +unregister_drv:
> +
> +	cpuidle_unregister_driver(drv);
> +
> +free_drv:
> +
> +	kfree(drv);
> +
> +	return ret;

Remove all the blank lines in the error handling.

> +}
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 712abcc..2aa89db 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -56,6 +56,16 @@ struct cpuidle_state {
>  
>  #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
>  
> +/* Common ARM WFI state */
> +#define CPUIDLE_ARM_WFI_STATE {\
> +	.enter                  = cpuidle_def_idle,\
> +	.exit_latency           = 2,\
> +	.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
> @@ -141,6 +151,13 @@ 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);
>  
> +/* provide a default idle function */
> +extern int cpuidle_def_idle(struct cpuidle_device *dev,
> +		       struct cpuidle_driver *drv, int index);
> +extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple,
> +			 void (*driver_data_init)(struct cpuidle_device *dev));
> +extern void common_cpuidle_devices_uninit(void);
> +
>  #else
>  static inline void disable_cpuidle(void) { }
>  static inline int cpuidle_idle_call(void) { return -ENODEV; }
> @@ -157,6 +174,13 @@ 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_def_idle(struct cpuidle_device *dev,
> +		       struct cpuidle_driver *drv, int index)
> +{return -ENODEV; }
> +static inline int common_cpuidle_init(struct cpuidle_driver *pdrv,
> +	bool simple, void (*driver_data_init)(struct cpuidle_device *dev))
> +{return -ENODEV; }
> +static inline void common_cpuidle_devices_uninit(void) { }
>  
>  #endif
>  

[PATCH v3 0/7] Add common cpuidle code for consolidation.

[Kevin Hilman]

Robert Lee <rob.lee@linaro.org> writes:

> This patch series adds a new common cpuidle interface to consolidate code
> commonly duplicated by various platforms.  A patch was then made for each
> platform that could immediately take advantage of this code.  The platform
> specific changes are not required by the common code and are only made for
> consoldation.

I noticed that the generic code uses ktime_get() for measuring time.  On
OMAP, we use getnstimeofday() because I while back I remember having
problems with the interaction of CPUidle state measurements and system
suspend.  Any idle activity during system suspend/resume ktime_get()
will WARN() because the timekeeping system has been suspended.

Off the top of my head, I don't remember the interactions that triggered
this, but I guess all it would require the idle loop to be entered after
the syscore_ops->suspend for the timekeeping subsystem (and before the
syscore_ops ->resume.)

Depending on what syscore_ops are registered, this could be rather
platforms specific.

> Maintainers and cpuidle idle developers of these platforms, please check to make
> sure that you agree with the changes.  

In earlier version you mentioned that OMAP didn't quite fit the
pattern.  Do you have any suggestions for how to make OMAP fit.

> Besides just code consolidation, a
> default "WFI" state is now used with default parameters that different from your
> original paramenters. The assumption is that if you have a WFI only idle state,
> the parameters in the new default WFI are more realistic as a true WFI only
> hardware state incurs minimal latency(<1us) or power penalty to enter and exit.
> If your platform actually performs other platform specific functionality upon
> entering WFI and the default parameters do not accurately reflect the 
> exit_latency and target_residency given in the common default state, please
> say so.  

I'm not sure what you mean by "WFI only".  On OMAP, WFI is the entry
point for all the idle states, with varying latencies.  The latencies
are then dependent on how the states are programmed for the various
power domains.  Upon WFI, the hardware then takes over puts the
powerdomains to their programmed states.   

Kevin

[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

[Kevin Hilman]

Robert Lee <rob.lee@linaro.org> writes:

> The patch adds some cpuidle initialization functionality commonly
> duplicated by many platforms.
>
> Signed-off-by: Robert Lee <rob.lee@linaro.org>

[...]

> +static int simple_enter(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	ktime_t time_start, time_end;
> +
> +	local_irq_disable();

Is this needed?   arch/arm/kernel/process.c:cpu_idle() already disables IRQs

> +	time_start = ktime_get();
> +
> +	index = do_idle[index](dev, drv, index);
> +
> +	time_end = ktime_get();
> +
> +	local_irq_enable();
> +
> +	dev->last_residency =
> +		(int)ktime_to_us(ktime_sub(time_end, time_start));

I don't think this cast is quite right here, since last_residency is an
int, it needs to be capped, or you'll end up with negative last_residency.

I think you need to do something like is done in poll_idle() in
drivers/cpuidle/cpuidle.c:

	diff = ktime_to_us(ktime_sub(t2, t1));
	if (diff > INT_MAX)
		diff = INT_MAX;


Speaking of poll_idle(), simple_idle() looks quite similar to it.   Maybe
they can be combined?

Kevin

[PATCH v3 0/7] Add common cpuidle code for consolidation.

[Mark Brown]

On Tue, Jan 24, 2012 at 12:08:08PM -0800, Kevin Hilman wrote:
> Robert Lee <rob.lee@linaro.org> writes:

> > Besides just code consolidation, a
> > default "WFI" state is now used with default parameters that different from your
> > original paramenters. The assumption is that if you have a WFI only idle state,
> > the parameters in the new default WFI are more realistic as a true WFI only
> > hardware state incurs minimal latency(<1us) or power penalty to enter and exit.

> > If your platform actually performs other platform specific functionality upon
> > entering WFI and the default parameters do not accurately reflect the 
> > exit_latency and target_residency given in the common default state, please
> > say so.  

> I'm not sure what you mean by "WFI only".  On OMAP, WFI is the entry
> point for all the idle states, with varying latencies.  The latencies
> are then dependent on how the states are programmed for the various
> power domains.  Upon WFI, the hardware then takes over puts the
> powerdomains to their programmed states.   

The default state in the patches is set up with parameters for a state
that literally only does the WFI and has no other hardware actions taken
adding latencies.  I asked for this because the existing drivers for
most of the SoCs out there currently only support that basic idle state
and when doing something more complex (which most of the SoCs actually
can do in hardware) it's still likely to get used during bringup of the
feature.

If there's varying levels of idle state then the SoC specific code would
need to enumerate them and their varying latencies so that the core can
figure out which one to enter.  This isn't a problem, it's a good thing,
but most SoCs haven't got so far as to need it yet.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120124/b1295bed/attachment.sig>

[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

[Rob Lee]

>
> A few cosmetic comments below, but otherwise looks good.
>
> Reviewed-by: Rob Herring <rob.herring@calxeda.com>
>
Rob, thanks for the review.  Agree with your comments and will fix in
the next version.

[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

[Rob Lee]

On Tue, Jan 24, 2012 at 2:16 PM, Kevin Hilman <khilman@ti.com> wrote:
> Robert Lee <rob.lee@linaro.org> writes:
>
>> The patch adds some cpuidle initialization functionality commonly
>> duplicated by many platforms.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>
> [...]
>
>> +static int simple_enter(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index)
>> +{
>> + ? ? ktime_t time_start, time_end;
>> +
>> + ? ? local_irq_disable();
>
> Is this needed? ? arch/arm/kernel/process.c:cpu_idle() already disables IRQs
>

As far as I can tell, all calls into cpuidle_idle_call have already
called this.  I had left it just in case someone wanted to call
cpuidle_idle_call directly, but that probably doesn't make sense.
Perhaps in the comments of cpuidle_idle_call I just list this as a
requirement and remove it from simple_enter.

>> + ? ? time_start = ktime_get();
>> +
>> + ? ? index = do_idle[index](dev, drv, index);
>> +
>> + ? ? time_end = ktime_get();
>> +
>> + ? ? local_irq_enable();
>> +
>> + ? ? dev->last_residency =
>> + ? ? ? ? ? ? (int)ktime_to_us(ktime_sub(time_end, time_start));
>
> I don't think this cast is quite right here, since last_residency is an
> int, it needs to be capped, or you'll end up with negative last_residency.
>
> I think you need to do something like is done in poll_idle() in
> drivers/cpuidle/cpuidle.c:
>
> ? ? ? ?diff = ktime_to_us(ktime_sub(t2, t1));
> ? ? ? ?if (diff > INT_MAX)
> ? ? ? ? ? ? ? ?diff = INT_MAX;
>
>

Thanks, I missed that.  Will fix in v4.

> Speaking of poll_idle(), simple_idle() looks quite similar to it. ? Maybe
> they can be combined?

Good point.  I'll look into that.

>
> Kevin
>

[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

[Turquette, Mike]

On Mon, Jan 23, 2012 at 8:37 PM, Robert Lee <rob.lee@linaro.org> wrote:
> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
> new file mode 100644
> index 0000000..dafa758
> --- /dev/null
> +++ b/drivers/cpuidle/common.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + * Copyright 2011 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/*
> + * This code performs provides some commonly used cpuidle setup functionality

s/performs //

> +static struct cpuidle_device __percpu * common_cpuidle_devices;

You can use DECLARE_PER_CPU here.

Is there any particular reason to allocate these dynamically?  You can
replace the code above with,

static DEFINE_PER_CPU(struct cpuidle_device, common_cpuidle_devices);

I might change the variable name to "cpu_cpuidle_device" in that case
since you are addressing a single CPU when using the per cpu accessor
functions and "common_cpuidle_devices" sounds like an array or a list
or something.  No big deal to keep the current name though.

> +static int simple_enter(struct cpuidle_device *dev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index)
> +{
> + ? ? ? ktime_t time_start, time_end;
> +
> + ? ? ? local_irq_disable();
> +
> + ? ? ? time_start = ktime_get();
> +
> + ? ? ? index = do_idle[index](dev, drv, index);
> +
> + ? ? ? time_end = ktime_get();
> +
> + ? ? ? local_irq_enable();
> +
> + ? ? ? dev->last_residency =
> + ? ? ? ? ? ? ? (int)ktime_to_us(ktime_sub(time_end, time_start));

Sometimes an attempt to enter some C-state fails and the do_idle will
return immediately.  What do you think about having do_idle return
-EERROR in this case and the conditionally setting last_residency to
zero in those cases?  The point is that a C-state's total residency
time should not increase in the case where the hardware did not
successfully transition into that C-state.  I've observed many times
where a specific low power state was actually achieved in the hardware
but /sys/devices/system/cpu/cpuN/cpuidle/stateM/time keeps
incrementing (albeit in very tiny increments).  Something like,

if (IS_ERR(index))
        dev->last_residency = 0;
else
        ...

Note: I haven't been through the CPUidle core in a while so maybe the
above suggestion violates some other requirements/assumptions...

> +
> + ? ? ? return index;
> +}
> +
> +void common_cpuidle_devices_uninit(void)
> +{
> + ? ? ? int cpu_id;
> + ? ? ? struct cpuidle_device *dev;
> +
> + ? ? ? for_each_possible_cpu(cpu_id) {
> + ? ? ? ? ? ? ? dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
> + ? ? ? ? ? ? ? cpuidle_unregister_device(dev);
> + ? ? ? }
> +
> + ? ? ? free_percpu(common_cpuidle_devices);

See note above about statically allocating the per-cpu variables.

> +}
> +
> +/**
> + * common_cpuidle_init() - Provides some commonly used init functionality.
> + * @pdrv ? ? ? ? ? ? ? Pointer to your cpuidle_driver object.
> + * @simple ? ? ? ? ? ? Use the common simple_enter wrapper?
> + * @driver_data_init ? Pointer to your platform function to initialize your
> + * ? ? ? ? ? ? ? ? ? ? platform specific driver data. ?Use NULL if platform
> + * ? ? ? ? ? ? ? ? ? ? specific data is not needed.
> + *
> + * Common cpuidle init interface to provide common cpuidle functionality
> + * used by many platforms.
> + */
> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
> + ? ? ? ? ? ? ? ? ? ? ? ?void (*driver_data_init)(struct cpuidle_device *dev))
> +{
> + ? ? ? struct cpuidle_device *dev;
> + ? ? ? struct cpuidle_driver *drv;
> + ? ? ? int i, cpu_id, ret;
> +
> + ? ? ? if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) {
> + ? ? ? ? ? ? ? pr_err("%s: Invalid Input\n", __func__);
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
> + ? ? ? if (!drv) {
> + ? ? ? ? ? ? ? pr_err("%s: no memory for cpuidle driver\n", __func__);
> + ? ? ? ? ? ? ? return -ENOMEM;
> + ? ? ? }
> +
> + ? ? ? *drv = *pdrv;
> +
> + ? ? ? for (i = 0; simple && (i < drv->state_count); i++) {
> + ? ? ? ? ? ? ? do_idle[i] = drv->states[i].enter;
> + ? ? ? ? ? ? ? drv->states[i].enter = simple_enter;
> + ? ? ? }
> +
> + ? ? ? ret = cpuidle_register_driver(drv);
> + ? ? ? if (ret) {
> + ? ? ? ? ? ? ? pr_err("%s: Failed to register cpuidle driver\n", __func__);
> + ? ? ? ? ? ? ? goto free_drv;
> + ? ? ? }
> +
> + ? ? ? common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> + ? ? ? if (common_cpuidle_devices == NULL) {
> + ? ? ? ? ? ? ? ret = -ENOMEM;
> + ? ? ? ? ? ? ? goto unregister_drv;
> + ? ? ? }

See note above about statically allocating these.

Regards,
Mike

[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

[Daniel Lezcano]

On 01/24/2012 05:37 AM, Robert Lee wrote:
> The patch adds some cpuidle initialization functionality commonly
> duplicated by many platforms.
>
> Signed-off-by: Robert Lee<rob.lee@linaro.org>
> ---

Hi Rob,

nice work. The result is interesting. I have a few comments below.


>   drivers/cpuidle/Makefile |    2 +-
>   drivers/cpuidle/common.c |  152 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/cpuidle.h  |   24 +++++++
>   3 files changed, 177 insertions(+), 1 deletions(-)
>   create mode 100644 drivers/cpuidle/common.c
>
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 5634f88..2928d93 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -2,4 +2,4 @@
>   # Makefile for cpuidle.
>   #
>
> -obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
> +obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/
> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
> new file mode 100644
> index 0000000..dafa758
> --- /dev/null
> +++ b/drivers/cpuidle/common.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + * Copyright 2011 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/*
> + * This code performs provides some commonly used cpuidle setup functionality
> + * used by many ARM SoC platforms.  Providing this functionality here
> + * reduces the duplication of this code for each ARM platform that uses it.
> + */
> +
> +#include<linux/kernel.h>
> +#include<linux/io.h>
> +#include<linux/cpuidle.h>
> +#include<linux/hrtimer.h>
> +#include<linux/err.h>
> +#include<linux/slab.h>
> +#include<asm/proc-fns.h>
> +
> +static struct cpuidle_device __percpu * common_cpuidle_devices;
> +
> +static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index);
> +
> +int cpuidle_def_idle(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	cpu_do_idle();
> +	return index;
> +}
> +
> +static int simple_enter(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
> +{
> +	ktime_t time_start, time_end;
> +
> +	local_irq_disable();
> +
> +	time_start = ktime_get();
> +
> +	index = do_idle[index](dev, drv, index);
> +
> +	time_end = ktime_get();
> +
> +	local_irq_enable();
> +
> +	dev->last_residency =
> +		(int)ktime_to_us(ktime_sub(time_end, time_start));
> +
> +	return index;
> +}
> +
> +void common_cpuidle_devices_uninit(void)
> +{
> +	int cpu_id;
> +	struct cpuidle_device *dev;
> +
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	free_percpu(common_cpuidle_devices);
> +}

If the registering sequence aborts, won't cpuidle_unregister_device 
leads to a kernel warning as it could be specified with a cpu which has 
*not* been registered yet ?

Perhaps we should pass the cpuid from where the cpu has failed an do a 
reverse unregister sequence.

void common_cpuidle_devices_uninit(int cpu)
{
         for (cpu--; cpu >= 0; cpu--) {
                 device = &per_cpu(common_cpuidle_devices, cpu);
                 cpuidle_unregister_device(device);
         }
...


> +
> +/**
> + * common_cpuidle_init() - Provides some commonly used init functionality.
> + * @pdrv		Pointer to your cpuidle_driver object.
> + * @simple		Use the common simple_enter wrapper?
> + * @driver_data_init	Pointer to your platform function to initialize your
> + *			platform specific driver data.  Use NULL if platform
> + *			specific data is not needed.
> + *
> + * Common cpuidle init interface to provide common cpuidle functionality
> + * used by many platforms.
> + */
> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
> +			 void (*driver_data_init)(struct cpuidle_device *dev))
> +{
> +	struct cpuidle_device *dev;
> +	struct cpuidle_driver *drv;
> +	int i, cpu_id, ret;
> +
> +	if (!pdrv || pdrv->state_count>  CPUIDLE_STATE_MAX) {
> +		pr_err("%s: Invalid Input\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
> +	if (!drv) {
> +		pr_err("%s: no memory for cpuidle driver\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	*drv = *pdrv;

Rob can you explain why is needed to copy this structure ?

Maybe kmemdup is more adequate here.

drv = kmemdup(pdrv, sizeof(*drv), GFP_KERNEL);

> +
> +	for (i = 0; simple&&  (i<  drv->state_count); i++) {
> +		do_idle[i] = drv->states[i].enter;
> +		drv->states[i].enter = simple_enter;
> +	}

Do we really need a 'simple' parameter ? Is there an idle enter function 
which does not correspond to the 'simple' scheme except omap3/4 ?

Maybe I am wrong but that looks a bit hacky because we are trying to 
override the functions the driver had previously defined and in order to 
prevent to modify the cpuidle.c core and more code.

I am wondering if it is possible to move the usual:

[ local_irq_disable(), getnstimeofday(before), myidle, 
getnstimeofday(after), local_irq_enable(), dev->last_residency = 
after-before, return index ]

to cpuidle.c/cpuidle_idle_call and wrap the
	entered_state = target_state->enter(dev, drv, next_state)
with these simple scheme.

Also I am not sure local_irq_disable is needed because AFAICT the idle 
function is called with the local_irq_disable. For example, the 
intel_idle driver does not do that and assume the enter_idle function is 
called with the local irq disabled.

Looking at the code :

arch/arm/kernel/process.c : pm_idle is wrapped with local_irq_disable / 
local_irq_enable.

arch/x86/kernel/process_32/64.c : pm_idle is called with 
local_irq_disable but assumes the function will enable local irq

arch/ia64/kernel/process.c : the code assumes the idle function will 
disable/enable the local irq.

etc ...

It seems the code with the different arch is non consistent except there 
is a technical reason I don't know. May be making them consistent will 
help to factor out this part of the code and make the common framework 
more simple.

It is just a suggestion and IMO that could be done later on top of this 
patchset.


> +	ret = cpuidle_register_driver(drv);
> +	if (ret) {
> +		pr_err("%s: Failed to register cpuidle driver\n", __func__);
> +		goto free_drv;
> +	}
> +
> +	common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (common_cpuidle_devices == NULL) {
> +		ret = -ENOMEM;
> +		goto unregister_drv;
> +	}
> +
> +	/* initialize state data for each cpuidle_device */
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
> +		dev->cpu = cpu_id;
> +		dev->state_count = drv->state_count;
> +
> +		if (driver_data_init)
> +			driver_data_init(dev);
> +
> +		ret = cpuidle_register_device(dev);
> +		if (ret) {
> +			pr_err("%s: Failed to register cpu %u\n",
> +				__func__, cpu_id);
> +			goto uninit;
> +		}
> +	}
> +
> +	return 0;
> +uninit:
> +
> +	common_cpuidle_devices_uninit();
> +
> +unregister_drv:
> +
> +	cpuidle_unregister_driver(drv);
> +
> +free_drv:
> +
> +	kfree(drv);
> +
> +	return ret;
> +}
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 712abcc..2aa89db 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -56,6 +56,16 @@ struct cpuidle_state {
>
>   #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
>
> +/* Common ARM WFI state */
> +#define CPUIDLE_ARM_WFI_STATE {\
> +	.enter                  = cpuidle_def_idle,\
> +	.exit_latency           = 2,\
> +	.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
> @@ -141,6 +151,13 @@ 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);
>
> +/* provide a default idle function */
> +extern int cpuidle_def_idle(struct cpuidle_device *dev,
> +		       struct cpuidle_driver *drv, int index);
> +extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple,
> +			 void (*driver_data_init)(struct cpuidle_device *dev));
> +extern void common_cpuidle_devices_uninit(void);
> +
>   #else
>   static inline void disable_cpuidle(void) { }
>   static inline int cpuidle_idle_call(void) { return -ENODEV; }
> @@ -157,6 +174,13 @@ 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_def_idle(struct cpuidle_device *dev,
> +		       struct cpuidle_driver *drv, int index)
> +{return -ENODEV; }
> +static inline int common_cpuidle_init(struct cpuidle_driver *pdrv,
> +	bool simple, void (*driver_data_init)(struct cpuidle_device *dev))
> +{return -ENODEV; }
> +static inline void common_cpuidle_devices_uninit(void) { }
>
>   #endif
>


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH v3 0/7] Add common cpuidle code for consolidation.

[Kevin Hilman]

Mark Brown <broonie@opensource.wolfsonmicro.com> writes:

> On Tue, Jan 24, 2012 at 12:08:08PM -0800, Kevin Hilman wrote:
>> Robert Lee <rob.lee@linaro.org> writes:
>
>> > Besides just code consolidation, a
>> > default "WFI" state is now used with default parameters that different from your
>> > original paramenters. The assumption is that if you have a WFI only idle state,
>> > the parameters in the new default WFI are more realistic as a true WFI only
>> > hardware state incurs minimal latency(<1us) or power penalty to enter and exit.
>
>> > If your platform actually performs other platform specific functionality upon
>> > entering WFI and the default parameters do not accurately reflect the 
>> > exit_latency and target_residency given in the common default state, please
>> > say so.  
>
>> I'm not sure what you mean by "WFI only".  On OMAP, WFI is the entry
>> point for all the idle states, with varying latencies.  The latencies
>> are then dependent on how the states are programmed for the various
>> power domains.  Upon WFI, the hardware then takes over puts the
>> powerdomains to their programmed states.   
>
> The default state in the patches is set up with parameters for a state
> that literally only does the WFI and has no other hardware actions taken
> adding latencies.  I asked for this because the existing drivers for
> most of the SoCs out there currently only support that basic idle state
> and when doing something more complex (which most of the SoCs actually
> can do in hardware) it's still likely to get used during bringup of the
> feature.
>
> If there's varying levels of idle state then the SoC specific code would
> need to enumerate them and their varying latencies so that the core can
> figure out which one to enter.  This isn't a problem, it's a good thing,
> but most SoCs haven't got so far as to need it yet.

OK, makes sense now.

I see now that that default is easily overridden by platform-specific
drivers, so I don't have any problem with it.

Kevin

[PATCH v3 0/7] Add common cpuidle code for consolidation.

[Rob Lee]

On Tue, Jan 24, 2012 at 2:08 PM, Kevin Hilman <khilman@ti.com> wrote:
> Robert Lee <rob.lee@linaro.org> writes:
>
>> This patch series adds a new common cpuidle interface to consolidate code
>> commonly duplicated by various platforms. ?A patch was then made for each
>> platform that could immediately take advantage of this code. ?The platform
>> specific changes are not required by the common code and are only made for
>> consoldation.
>
> I noticed that the generic code uses ktime_get() for measuring time. ?On
> OMAP, we use getnstimeofday() because I while back I remember having
> problems with the interaction of CPUidle state measurements and system
> suspend. ?Any idle activity during system suspend/resume ktime_get()
> will WARN() because the timekeeping system has been suspended.
>

When I originally looked at which time mechanism to use, I convinced
myself that use getnstimeofday() on an SMP system could be susceptible
to error (or require extra handling) in the case whee a call was made
to do_settimeofday by another CPU that wasn't idle.  That seemed to
leave get_monotonic_bootime() or ktime_get().  Off the top of my head,
I don't remember how I settled on ktime_get, but get_monotonic_bootime
will try to account for "suspend" time. I'll look into this further.

> Off the top of my head, I don't remember the interactions that triggered
> this, but I guess all it would require the idle loop to be entered after
> the syscore_ops->suspend for the timekeeping subsystem (and before the
> syscore_ops ->resume.)
>
> Depending on what syscore_ops are registered, this could be rather
> platforms specific.
>
>> Maintainers and cpuidle idle developers of these platforms, please check to make
>> sure that you agree with the changes.
>
> In earlier version you mentioned that OMAP didn't quite fit the
> pattern. ?Do you have any suggestions for how to make OMAP fit.

Many platforms are only performing very basic idle operations and the
time keeping and interrupt disabling/enabling could be made into a
common wrapper.  But OMAP3 isn't that simple and so benefits by
performing its own time accounting to accurately account for only the
idle time (not the idle preparation time).  I think that's OK as the
current cpuidle functionality allows for this flexibility.  The less
flexible common code is just to remove unnecessary code duplication
that exists on several simpler idle implementations.  That said, I
could still look at using the common_cpuidle_init for OMAP3 to make
dynamically allocated driver and device objects and just not use
simple_enter.

I either missed OMAP4 implementation or it wasn't in the kernel
version I was using at the time.  It appears that the simple_enter
could be used for OMAP4 in its current form, though I think the
current implementation accounts for some "idle preparation" time that
perhaps it doesn't need to? (and thus shouldn't use the common
simple_enter).  Perhaps this amount of time is small enough that you
don't care about it or perhaps this is done to avoid time keeping
issues causes by the timer being disabled.  But it seems the current
OMAP4 implementation is only enabling cpuidle for one CPU?  The
current common_cpuidle_init expects o will create and register a
cpuidle_device for each cpu.  Perhaps that is not what you want?

>
>> Besides just code consolidation, a
>> default "WFI" state is now used with default parameters that different from your
>> original paramenters. The assumption is that if you have a WFI only idle state,
>> the parameters in the new default WFI are more realistic as a true WFI only
>> hardware state incurs minimal latency(<1us) or power penalty to enter and exit.
>> If your platform actually performs other platform specific functionality upon
>> entering WFI and the default parameters do not accurately reflect the
>> exit_latency and target_residency given in the common default state, please
>> say so.
>
> I'm not sure what you mean by "WFI only". ?On OMAP, WFI is the entry
> point for all the idle states, with varying latencies. ?The latencies
> are then dependent on how the states are programmed for the various
> power domains. ?Upon WFI, the hardware then takes over puts the
> powerdomains to their programmed states.
>

This text was just to describe the extra macro I threw in for use by
systems with a state that only performs the very basic ARM WFI
available to all (or most modern) ARM archs, sans any additional
platform specific functionality that may cause additional exit latency
or target residency time beyond the defaults I used for that macro.

> Kevin

[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

[Rob Lee]

Mike, thanks for your review.  Comments below.

On Tue, Jan 24, 2012 at 5:46 PM, Turquette, Mike <mturquette@ti.com> wrote:
> On Mon, Jan 23, 2012 at 8:37 PM, Robert Lee <rob.lee@linaro.org> wrote:
>> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
>> new file mode 100644
>> index 0000000..dafa758
>> --- /dev/null
>> +++ b/drivers/cpuidle/common.c
>> @@ -0,0 +1,152 @@
>> +/*
>> + * Copyright 2011 Freescale Semiconductor, Inc.
>> + * Copyright 2011 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +/*
>> + * This code performs provides some commonly used cpuidle setup functionality
>
> s/performs //
>
>> +static struct cpuidle_device __percpu * common_cpuidle_devices;
>
> You can use DECLARE_PER_CPU here.

Ok.  Do you mean that DECLARE_PER_CPU is preferred in this case?

>
> Is there any particular reason to allocate these dynamically? ?You can
> replace the code above with,
>
> static DEFINE_PER_CPU(struct cpuidle_device, common_cpuidle_devices);
>

I was thinking of the single kernel with multiple platform support
case.  In that particular case, it seems better to create the number
of device objects you need at run time.

> I might change the variable name to "cpu_cpuidle_device" in that case
> since you are addressing a single CPU when using the per cpu accessor
> functions and "common_cpuidle_devices" sounds like an array or a list
> or something. ?No big deal to keep the current name though.
>
>> +static int simple_enter(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index)
>> +{
>> + ? ? ? ktime_t time_start, time_end;
>> +
>> + ? ? ? local_irq_disable();
>> +
>> + ? ? ? time_start = ktime_get();
>> +
>> + ? ? ? index = do_idle[index](dev, drv, index);
>> +
>> + ? ? ? time_end = ktime_get();
>> +
>> + ? ? ? local_irq_enable();
>> +
>> + ? ? ? dev->last_residency =
>> + ? ? ? ? ? ? ? (int)ktime_to_us(ktime_sub(time_end, time_start));
>
> Sometimes an attempt to enter some C-state fails and the do_idle will
> return immediately. ?What do you think about having do_idle return
> -EERROR in this case and the conditionally setting last_residency to
> zero in those cases? ?The point is that a C-state's total residency
> time should not increase in the case where the hardware did not
> successfully transition into that C-state. ?I've observed many times
> where a specific low power state was actually achieved in the hardware
> but /sys/devices/system/cpu/cpuN/cpuidle/stateM/time keeps
> incrementing (albeit in very tiny increments). ?Something like,
>
> if (IS_ERR(index))
> ? ? ? ?dev->last_residency = 0;
> else
> ? ? ? ?...
>
> Note: I haven't been through the CPUidle core in a while so maybe the
> above suggestion violates some other requirements/assumptions...
>

Good suggestion.  I'll look into adding this to v4.

>> +
>> + ? ? ? return index;
>> +}
>> +
>> +void common_cpuidle_devices_uninit(void)
>> +{
>> + ? ? ? int cpu_id;
>> + ? ? ? struct cpuidle_device *dev;
>> +
>> + ? ? ? for_each_possible_cpu(cpu_id) {
>> + ? ? ? ? ? ? ? dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
>> + ? ? ? ? ? ? ? cpuidle_unregister_device(dev);
>> + ? ? ? }
>> +
>> + ? ? ? free_percpu(common_cpuidle_devices);
>
> See note above about statically allocating the per-cpu variables.
>
>> +}
>> +
>> +/**
>> + * common_cpuidle_init() - Provides some commonly used init functionality.
>> + * @pdrv ? ? ? ? ? ? ? Pointer to your cpuidle_driver object.
>> + * @simple ? ? ? ? ? ? Use the common simple_enter wrapper?
>> + * @driver_data_init ? Pointer to your platform function to initialize your
>> + * ? ? ? ? ? ? ? ? ? ? platform specific driver data. ?Use NULL if platform
>> + * ? ? ? ? ? ? ? ? ? ? specific data is not needed.
>> + *
>> + * Common cpuidle init interface to provide common cpuidle functionality
>> + * used by many platforms.
>> + */
>> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
>> + ? ? ? ? ? ? ? ? ? ? ? ?void (*driver_data_init)(struct cpuidle_device *dev))
>> +{
>> + ? ? ? struct cpuidle_device *dev;
>> + ? ? ? struct cpuidle_driver *drv;
>> + ? ? ? int i, cpu_id, ret;
>> +
>> + ? ? ? if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) {
>> + ? ? ? ? ? ? ? pr_err("%s: Invalid Input\n", __func__);
>> + ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? }
>> +
>> + ? ? ? drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
>> + ? ? ? if (!drv) {
>> + ? ? ? ? ? ? ? pr_err("%s: no memory for cpuidle driver\n", __func__);
>> + ? ? ? ? ? ? ? return -ENOMEM;
>> + ? ? ? }
>> +
>> + ? ? ? *drv = *pdrv;
>> +
>> + ? ? ? for (i = 0; simple && (i < drv->state_count); i++) {
>> + ? ? ? ? ? ? ? do_idle[i] = drv->states[i].enter;
>> + ? ? ? ? ? ? ? drv->states[i].enter = simple_enter;
>> + ? ? ? }
>> +
>> + ? ? ? ret = cpuidle_register_driver(drv);
>> + ? ? ? if (ret) {
>> + ? ? ? ? ? ? ? pr_err("%s: Failed to register cpuidle driver\n", __func__);
>> + ? ? ? ? ? ? ? goto free_drv;
>> + ? ? ? }
>> +
>> + ? ? ? common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>> + ? ? ? if (common_cpuidle_devices == NULL) {
>> + ? ? ? ? ? ? ? ret = -ENOMEM;
>> + ? ? ? ? ? ? ? goto unregister_drv;
>> + ? ? ? }
>
> See note above about statically allocating these.
>
> Regards,
> Mike
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

[Rob Lee]

Daniel, thanks for your review.  I think you and Mike timed sending
your responses :)  Comments below.

On Tue, Jan 24, 2012 at 5:49 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 01/24/2012 05:37 AM, Robert Lee wrote:
>>
>> The patch adds some cpuidle initialization functionality commonly
>> duplicated by many platforms.
>>
>> Signed-off-by: Robert Lee<rob.lee@linaro.org>
>> ---
>
>
> Hi Rob,
>
> nice work. The result is interesting. I have a few comments below.
>
>
>
>> ?drivers/cpuidle/Makefile | ? ?2 +-
>> ?drivers/cpuidle/common.c | ?152
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> ?include/linux/cpuidle.h ?| ? 24 +++++++
>> ?3 files changed, 177 insertions(+), 1 deletions(-)
>> ?create mode 100644 drivers/cpuidle/common.c
>>
>> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
>> index 5634f88..2928d93 100644
>> --- a/drivers/cpuidle/Makefile
>> +++ b/drivers/cpuidle/Makefile
>> @@ -2,4 +2,4 @@
>> ?# Makefile for cpuidle.
>> ?#
>>
>> -obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
>> +obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/
>> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c
>> new file mode 100644
>> index 0000000..dafa758
>> --- /dev/null
>> +++ b/drivers/cpuidle/common.c
>> @@ -0,0 +1,152 @@
>> +/*
>> + * Copyright 2011 Freescale Semiconductor, Inc.
>> + * Copyright 2011 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +/*
>> + * This code performs provides some commonly used cpuidle setup
>> functionality
>> + * used by many ARM SoC platforms. ?Providing this functionality here
>> + * reduces the duplication of this code for each ARM platform that uses
>> it.
>> + */
>> +
>> +#include<linux/kernel.h>
>> +#include<linux/io.h>
>> +#include<linux/cpuidle.h>
>> +#include<linux/hrtimer.h>
>> +#include<linux/err.h>
>> +#include<linux/slab.h>
>> +#include<asm/proc-fns.h>
>> +
>> +static struct cpuidle_device __percpu * common_cpuidle_devices;
>> +
>> +static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index);
>> +
>> +int cpuidle_def_idle(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index)
>> +{
>> + ? ? ? cpu_do_idle();
>> + ? ? ? return index;
>> +}
>> +
>> +static int simple_enter(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index)
>> +{
>> + ? ? ? ktime_t time_start, time_end;
>> +
>> + ? ? ? local_irq_disable();
>> +
>> + ? ? ? time_start = ktime_get();
>> +
>> + ? ? ? index = do_idle[index](dev, drv, index);
>> +
>> + ? ? ? time_end = ktime_get();
>> +
>> + ? ? ? local_irq_enable();
>> +
>> + ? ? ? dev->last_residency =
>> + ? ? ? ? ? ? ? (int)ktime_to_us(ktime_sub(time_end, time_start));
>> +
>> + ? ? ? return index;
>> +}
>> +
>> +void common_cpuidle_devices_uninit(void)
>> +{
>> + ? ? ? int cpu_id;
>> + ? ? ? struct cpuidle_device *dev;
>> +
>> + ? ? ? for_each_possible_cpu(cpu_id) {
>> + ? ? ? ? ? ? ? dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
>> + ? ? ? ? ? ? ? cpuidle_unregister_device(dev);
>> + ? ? ? }
>> +
>> + ? ? ? free_percpu(common_cpuidle_devices);
>> +}
>
>
> If the registering sequence aborts, won't cpuidle_unregister_device leads to
> a kernel warning as it could be specified with a cpu which has *not* been
> registered yet ?
>

I think you may have been looking at cpuidle_unregister_driver.  Here
is cpuidle_unregister_device which seems to handle a device not yet
registered ok:

void cpuidle_unregister_device(struct cpuidle_device *dev)
{
	struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu);
	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();

	if (dev->registered == 0)
		return;
...

> Perhaps we should pass the cpuid from where the cpu has failed an do a
> reverse unregister sequence.
>
> void common_cpuidle_devices_uninit(int cpu)
> {
> ? ? ? ?for (cpu--; cpu >= 0; cpu--) {
> ? ? ? ? ? ? ? ?device = &per_cpu(common_cpuidle_devices, cpu);
> ? ? ? ? ? ? ? ?cpuidle_unregister_device(device);
> ? ? ? ?}
> ...
>
>
>
>> +
>> +/**
>> + * common_cpuidle_init() - Provides some commonly used init
>> functionality.
>> + * @pdrv ? ? ? ? ? ? ? Pointer to your cpuidle_driver object.
>> + * @simple ? ? ? ? ? ? Use the common simple_enter wrapper?
>> + * @driver_data_init ? Pointer to your platform function to initialize
>> your
>> + * ? ? ? ? ? ? ? ? ? ? platform specific driver data. ?Use NULL if
>> platform
>> + * ? ? ? ? ? ? ? ? ? ? specific data is not needed.
>> + *
>> + * Common cpuidle init interface to provide common cpuidle functionality
>> + * used by many platforms.
>> + */
>> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple,
>> + ? ? ? ? ? ? ? ? ? ? ? ?void (*driver_data_init)(struct cpuidle_device
>> *dev))
>> +{
>> + ? ? ? struct cpuidle_device *dev;
>> + ? ? ? struct cpuidle_driver *drv;
>> + ? ? ? int i, cpu_id, ret;
>> +
>> + ? ? ? if (!pdrv || pdrv->state_count> ?CPUIDLE_STATE_MAX) {
>> + ? ? ? ? ? ? ? pr_err("%s: Invalid Input\n", __func__);
>> + ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? }
>> +
>> + ? ? ? drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
>> + ? ? ? if (!drv) {
>> + ? ? ? ? ? ? ? pr_err("%s: no memory for cpuidle driver\n", __func__);
>> + ? ? ? ? ? ? ? return -ENOMEM;
>> + ? ? ? }
>> +
>> + ? ? ? *drv = *pdrv;
>
>
> Rob can you explain why is needed to copy this structure ?
>

I made the original platform cpuidle_driver objects __initdata so I
need to copy over to the dynamically allocated structure.

> Maybe kmemdup is more adequate here.
>
> drv = kmemdup(pdrv, sizeof(*drv), GFP_KERNEL);
>

Is this preferred by the community over direct structure copies?  Or
is there some other advantage?

>> +
>> + ? ? ? for (i = 0; simple&& ?(i< ?drv->state_count); i++) {
>>
>> + ? ? ? ? ? ? ? do_idle[i] = drv->states[i].enter;
>> + ? ? ? ? ? ? ? drv->states[i].enter = simple_enter;
>> + ? ? ? }
>
>
> Do we really need a 'simple' parameter ? Is there an idle enter function
> which does not correspond to the 'simple' scheme except omap3/4 ?
>
> Maybe I am wrong but that looks a bit hacky because we are trying to
> override the functions the driver had previously defined and in order to
> prevent to modify the cpuidle.c core and more code.
>
> I am wondering if it is possible to move the usual:
>
> [ local_irq_disable(), getnstimeofday(before), myidle,
> getnstimeofday(after), local_irq_enable(), dev->last_residency =
> after-before, return index ]
>
> to cpuidle.c/cpuidle_idle_call and wrap the
> ? ? ? ?entered_state = target_state->enter(dev, drv, next_state)
> with these simple scheme.
>

Yes, I considered the same thing and originally made a version of this
patch with direct changes to cpuidle_idle_call.  But I concluded that
since this common code's main purpose is just to consolidate code
duplication on *some* (but not all) cpuidle implementations, it was
better to create the extra simple_enter wrapper than to add additional
code in cpuidle_idle_call that other platforms don't need.  I'd be
happy to submit a version of this patch with cpuidle_idle_call changes
though and let the community decide.  If anyone else thinks this is a
good or bad idea, please give your input.

> Also I am not sure local_irq_disable is needed because AFAICT the idle
> function is called with the local_irq_disable. For example, the intel_idle
> driver does not do that and assume the enter_idle function is called with
> the local irq disabled.
>
> Looking at the code :
>
> arch/arm/kernel/process.c : pm_idle is wrapped with local_irq_disable /
> local_irq_enable.
>
> arch/x86/kernel/process_32/64.c : pm_idle is called with local_irq_disable
> but assumes the function will enable local irq
>
> arch/ia64/kernel/process.c : the code assumes the idle function will
> disable/enable the local irq.
>
> etc ...
>

Agree.  I considered this as well but ultimately decided to leave it
in.  I can remove it for the next patch version though.

> It seems the code with the different arch is non consistent except there is
> a technical reason I don't know. May be making them consistent will help to
> factor out this part of the code and make the common framework more simple.
>
> It is just a suggestion and IMO that could be done later on top of this
> patchset.
>
>
>
>> + ? ? ? ret = cpuidle_register_driver(drv);
>> + ? ? ? if (ret) {
>> + ? ? ? ? ? ? ? pr_err("%s: Failed to register cpuidle driver\n",
>> __func__);
>> + ? ? ? ? ? ? ? goto free_drv;
>> + ? ? ? }
>> +
>> + ? ? ? common_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>> + ? ? ? if (common_cpuidle_devices == NULL) {
>> + ? ? ? ? ? ? ? ret = -ENOMEM;
>> + ? ? ? ? ? ? ? goto unregister_drv;
>> + ? ? ? }
>> +
>> + ? ? ? /* initialize state data for each cpuidle_device */
>> + ? ? ? for_each_possible_cpu(cpu_id) {
>> + ? ? ? ? ? ? ? dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
>> + ? ? ? ? ? ? ? dev->cpu = cpu_id;
>> + ? ? ? ? ? ? ? dev->state_count = drv->state_count;
>> +
>> + ? ? ? ? ? ? ? if (driver_data_init)
>> + ? ? ? ? ? ? ? ? ? ? ? driver_data_init(dev);
>> +
>> + ? ? ? ? ? ? ? ret = cpuidle_register_device(dev);
>> + ? ? ? ? ? ? ? if (ret) {
>> + ? ? ? ? ? ? ? ? ? ? ? pr_err("%s: Failed to register cpu %u\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, cpu_id);
>> + ? ? ? ? ? ? ? ? ? ? ? goto uninit;
>> + ? ? ? ? ? ? ? }
>> + ? ? ? }
>> +
>> + ? ? ? return 0;
>> +uninit:
>> +
>> + ? ? ? common_cpuidle_devices_uninit();
>> +
>> +unregister_drv:
>> +
>> + ? ? ? cpuidle_unregister_driver(drv);
>> +
>> +free_drv:
>> +
>> + ? ? ? kfree(drv);
>> +
>> + ? ? ? return ret;
>> +}
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 712abcc..2aa89db 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -56,6 +56,16 @@ struct cpuidle_state {
>>
>> ?#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
>>
>> +/* Common ARM WFI state */
>> +#define CPUIDLE_ARM_WFI_STATE {\
>> + ? ? ? .enter ? ? ? ? ? ? ? ? ?= cpuidle_def_idle,\
>> + ? ? ? .exit_latency ? ? ? ? ? = 2,\
>> + ? ? ? .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
>> @@ -141,6 +151,13 @@ 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);
>>
>> +/* provide a default idle function */
>> +extern int cpuidle_def_idle(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index);
>> +extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple,
>> + ? ? ? ? ? ? ? ? ? ? ? ?void (*driver_data_init)(struct cpuidle_device
>> *dev));
>> +extern void common_cpuidle_devices_uninit(void);
>> +
>> ?#else
>> ?static inline void disable_cpuidle(void) { }
>> ?static inline int cpuidle_idle_call(void) { return -ENODEV; }
>> @@ -157,6 +174,13 @@ 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_def_idle(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index)
>> +{return -ENODEV; }
>> +static inline int common_cpuidle_init(struct cpuidle_driver *pdrv,
>> + ? ? ? bool simple, void (*driver_data_init)(struct cpuidle_device *dev))
>> +{return -ENODEV; }
>> +static inline void common_cpuidle_devices_uninit(void) { }
>>
>> ?#endif
>>
>
>
> --
> ?<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
>
> Follow Linaro: ?<http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

[PATCH v3 1/7] cpuidle: Add common init interface and idle functionality

[Daniel Lezcano]

On 01/25/2012 03:38 AM, Rob Lee wrote:
> Daniel, thanks for your review.  I think you and Mike timed sending
> your responses :)  Comments below.

[ ... ]

>>> +       for_each_possible_cpu(cpu_id) {
>>> +               dev = per_cpu_ptr(common_cpuidle_devices, cpu_id);
>>> +               cpuidle_unregister_device(dev);
>>> +       }
>>> +
>>> +       free_percpu(common_cpuidle_devices);
>>> +}
>>
>>
>> If the registering sequence aborts, won't cpuidle_unregister_device leads to
>> a kernel warning as it could be specified with a cpu which has *not* been
>> registered yet ?
>>
>
> I think you may have been looking at cpuidle_unregister_driver.  Here
> is cpuidle_unregister_device which seems to handle a device not yet
> registered ok:
>
> void cpuidle_unregister_device(struct cpuidle_device *dev)
> {
> 	struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu);
> 	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
>
> 	if (dev->registered == 0)
> 		return;
> ...


Ok, it is harmless. I could have looked at that ... :)

>>> +
>>> +       drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL);
>>> +       if (!drv) {
>>> +               pr_err("%s: no memory for cpuidle driver\n", __func__);
>>> +               return -ENOMEM;
>>> +       }
>>> +
>>> +       *drv = *pdrv;

[ ... ]

>> Rob can you explain why is needed to copy this structure ?
>>
>
> I made the original platform cpuidle_driver objects __initdata so I
> need to copy over to the dynamically allocated structure.

Yes, but why declare a static object to be freed and allocate a new one 
and copy it ? Why don't just use the pdrv parameter of the function ?

>> Maybe kmemdup is more adequate here.
>>
>> drv = kmemdup(pdrv, sizeof(*drv), GFP_KERNEL);
>>
>
> Is this preferred by the community over direct structure copies?  Or
> is there some other advantage?

It does kmalloc + memcpy. And *drv = *pdrv is converted to a memcpy by 
the compiler. So using kmemdup generates the same code as kmalloc + 
memcpy, or kmalloc + *drv = *pdrv

>>> +
>>> +       for (i = 0; simple&&    (i<    drv->state_count); i++) {
>>>
>>> +               do_idle[i] = drv->states[i].enter;
>>> +               drv->states[i].enter = simple_enter;
>>> +       }
>>
>>
>> Do we really need a 'simple' parameter ? Is there an idle enter function
>> which does not correspond to the 'simple' scheme except omap3/4 ?
>>
>> Maybe I am wrong but that looks a bit hacky because we are trying to
>> override the functions the driver had previously defined and in order to
>> prevent to modify the cpuidle.c core and more code.
>>
>> I am wondering if it is possible to move the usual:
>>
>> [ local_irq_disable(), getnstimeofday(before), myidle,
>> getnstimeofday(after), local_irq_enable(), dev->last_residency =
>> after-before, return index ]
>>
>> to cpuidle.c/cpuidle_idle_call and wrap the
>>         entered_state = target_state->enter(dev, drv, next_state)
>> with these simple scheme.
>>
>
> Yes, I considered the same thing and originally made a version of this
> patch with direct changes to cpuidle_idle_call.  But I concluded that
> since this common code's main purpose is just to consolidate code
> duplication on *some* (but not all) cpuidle implementations, it was
> better to create the extra simple_enter wrapper than to add additional
> code in cpuidle_idle_call that other platforms don't need.  I'd be
> happy to submit a version of this patch with cpuidle_idle_call changes
> though and let the community decide.  If anyone else thinks this is a
> good or bad idea, please give your input.

[1]

>> Also I am not sure local_irq_disable is needed because AFAICT the idle
>> function is called with the local_irq_disable. For example, the intel_idle
>> driver does not do that and assume the enter_idle function is called with
>> the local irq disabled.
>>
>> Looking at the code :
>>
>> arch/arm/kernel/process.c : pm_idle is wrapped with local_irq_disable /
>> local_irq_enable.
>>
>> arch/x86/kernel/process_32/64.c : pm_idle is called with local_irq_disable
>> but assumes the function will enable local irq
>>
>> arch/ia64/kernel/process.c : the code assumes the idle function will
>> disable/enable the local irq.
>>
>> etc ...
>>
>
> Agree.  I considered this as well but ultimately decided to leave it
> in.  I can remove it for the next patch version though.

IMHO, we should wait for what inputs we have in [1]

Thanks
   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH v3 0/7] Add common cpuidle code for consolidation.

[Kevin Hilman]

Rob Lee <rob.lee@linaro.org> writes:

> On Tue, Jan 24, 2012 at 2:08 PM, Kevin Hilman <khilman@ti.com> wrote:
>> Robert Lee <rob.lee@linaro.org> writes:
>>
>>> This patch series adds a new common cpuidle interface to consolidate code
>>> commonly duplicated by various platforms. ?A patch was then made for each
>>> platform that could immediately take advantage of this code. ?The platform
>>> specific changes are not required by the common code and are only made for
>>> consoldation.
>>
>> I noticed that the generic code uses ktime_get() for measuring time. ?On
>> OMAP, we use getnstimeofday() because I while back I remember having
>> problems with the interaction of CPUidle state measurements and system
>> suspend. ?Any idle activity during system suspend/resume ktime_get()
>> will WARN() because the timekeeping system has been suspended.
>>
>
> When I originally looked at which time mechanism to use, I convinced
> myself that use getnstimeofday() on an SMP system could be susceptible
> to error (or require extra handling) in the case whee a call was made
> to do_settimeofday by another CPU that wasn't idle.  That seemed to
> leave get_monotonic_bootime() or ktime_get().  Off the top of my head,
> I don't remember how I settled on ktime_get, but get_monotonic_bootime
> will try to account for "suspend" time. I'll look into this further.

It might be the case now that with syscore_ops, the timekeeping
suspend/resume is happening early enought that the system will not go
idle before the syscore_ops are done, so you would never see this WARN.
That being said,  any platforms that add syscore_ops could introduce
callbacks that could potentially allow a CPU to hit idle, and you'd get
this WARN from the timekeeping susbystem.

>> Off the top of my head, I don't remember the interactions that triggered
>> this, but I guess all it would require the idle loop to be entered after
>> the syscore_ops->suspend for the timekeeping subsystem (and before the
>> syscore_ops ->resume.)
>>
>> Depending on what syscore_ops are registered, this could be rather
>> platforms specific.
>>
>>> Maintainers and cpuidle idle developers of these platforms, please check to make
>>> sure that you agree with the changes.
>>
>> In earlier version you mentioned that OMAP didn't quite fit the
>> pattern. ?Do you have any suggestions for how to make OMAP fit.
>
> Many platforms are only performing very basic idle operations and the
> time keeping and interrupt disabling/enabling could be made into a
> common wrapper.  But OMAP3 isn't that simple and so benefits by
> performing its own time accounting to accurately account for only the
> idle time (not the idle preparation time).  I think that's OK as the
> current cpuidle functionality allows for this flexibility.  The less
> flexible common code is just to remove unnecessary code duplication
> that exists on several simpler idle implementations.  That said, I
> could still look at using the common_cpuidle_init for OMAP3 to make
> dynamically allocated driver and device objects and just not use
> simple_enter.
>
> I either missed OMAP4 implementation or it wasn't in the kernel
> version I was using at the time.  It appears that the simple_enter
> could be used for OMAP4 in its current form, though I think the
> current implementation accounts for some "idle preparation" time that
> perhaps it doesn't need to? (and thus shouldn't use the common
> simple_enter).  Perhaps this amount of time is small enough that you
> don't care about it or perhaps this is done to avoid time keeping
> issues causes by the timer being disabled.  But it seems the current
> OMAP4 implementation is only enabling cpuidle for one CPU?  The
> current common_cpuidle_init expects o will create and register a
> cpuidle_device for each cpu.  Perhaps that is not what you want?

For current mainline, that is what we want.   Since the CPUs are coupled
(in a cluster), they cannot hit low power states independently.  The way
it is currently implemented in mainline is that CPUidle will not take
effect until one of the CPUs is hot-unplugged.

However, we want to get away from this to a more fine-grained approach.
Colin Cross has proposed support for coupled cpuidle states[1] which
will allow more flexibility in supporting idle states for coupled CPUs.
This is the direction we are going for OMAP4.

Kevin

[1] http://marc.info/?l=linux-omap&m=132442632512812&w=2

[PATCH v3 2/7] ARM: exynos: Modify to use new common cpuidle code.

[Barry Song]

2012/1/24 Robert Lee <rob.lee@linaro.org>:
> Make necessary changes for consolidation with new common cpuidle code.
>
> Signed-off-by: Robert Lee <rob.lee@linaro.org>

Reviewed-by: Barry Song <Baohua.Song@csr.com>

> ---
> ?arch/arm/mach-exynos/cpuidle.c | ? 73 +++------------------------------------
> ?1 files changed, 6 insertions(+), 67 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> index 4ebb382..baa1178 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -13,80 +13,19 @@
> ?#include <linux/cpuidle.h>
> ?#include <linux/io.h>
> ?#include <linux/export.h>
> -#include <linux/time.h>
> -
> ?#include <asm/proc-fns.h>
>
> -static int exynos4_enter_idle(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 ? ? ? ? ? ? ? ? ? = "IDLE",
> - ? ? ? ? ? ? ? .desc ? ? ? ? ? ? ? ? ? = "ARM clock gating(WFI)",
> - ? ? ? },
> -};
> -
> -static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
> -
> -static struct cpuidle_driver exynos4_idle_driver = {
> +static struct cpuidle_driver exynos4_idle_driver __initdata = {
> ? ? ? ?.name ? ? ? ? ? = "exynos4_idle",
> ? ? ? ?.owner ? ? ? ? ?= THIS_MODULE,
> + ? ? ? .states[0] ? ? ?= CPUIDLE_ARM_WFI_STATE,
> + ? ? ? .state_count ? ?= 1,
> ?};
>
> -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;
> -}
> -
> ?static int __init exynos4_init_cpuidle(void)
> ?{
> - ? ? ? int i, max_cpuidle_state, cpu_id;
> - ? ? ? struct cpuidle_device *device;
> - ? ? ? struct cpuidle_driver *drv = &exynos4_idle_driver;
> -
> - ? ? ? /* Setup cpuidle driver */
> - ? ? ? drv->state_count = (sizeof(exynos4_cpuidle_set) /
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?sizeof(struct cpuidle_state));
> - ? ? ? max_cpuidle_state = drv->state_count;
> - ? ? ? for (i = 0; i < max_cpuidle_state; i++) {
> - ? ? ? ? ? ? ? memcpy(&drv->states[i], &exynos4_cpuidle_set[i],
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(struct cpuidle_state));
> - ? ? ? }
> - ? ? ? cpuidle_register_driver(&exynos4_idle_driver);
> -
> - ? ? ? for_each_cpu(cpu_id, cpu_online_mask) {
> - ? ? ? ? ? ? ? device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> - ? ? ? ? ? ? ? device->cpu = cpu_id;
> -
> - ? ? ? ? ? ? ? device->state_count = drv->state_count;
> -
> - ? ? ? ? ? ? ? if (cpuidle_register_device(device)) {
> - ? ? ? ? ? ? ? ? ? ? ? printk(KERN_ERR "CPUidle register device failed\n,");
> - ? ? ? ? ? ? ? ? ? ? ? return -EIO;
> - ? ? ? ? ? ? ? }
> - ? ? ? }
> - ? ? ? return 0;
> + ? ? ? return common_cpuidle_init(&exynos4_idle_driver,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? true,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
> ?}
> ?device_initcall(exynos4_init_cpuidle);
> --
> 1.7.1

[PATCH v3 0/7] Add common cpuidle code for consolidation.

[Russell King - ARM Linux]

On Wed, Jan 25, 2012 at 10:58:44AM -0800, Kevin Hilman wrote:
> It might be the case now that with syscore_ops, the timekeeping
> suspend/resume is happening early enought that the system will not go
> idle before the syscore_ops are done, so you would never see this WARN.
> That being said,  any platforms that add syscore_ops could introduce
> callbacks that could potentially allow a CPU to hit idle, and you'd get
> this WARN from the timekeeping susbystem.

I would really hope that doesn't happen, because syscore_ops stuff will
be called with IRQs off - and you're not allowed to enable interrupts
in the syscore ops callbacks.

So if you did hit idle, you wouldn't be able to receive any interrupt
to wake you from idle state.

[PATCH v3 0/7] Add common cpuidle code for consolidation.

[Rob Lee]

On Tue, Jan 24, 2012 at 2:08 PM, Kevin Hilman <khilman@ti.com> wrote:
> I noticed that the generic code uses ktime_get() for measuring time.  On
> OMAP, we use getnstimeofday() because I while back I remember having
> problems with the interaction of CPUidle state measurements and system
> suspend.  Any idle activity during system suspend/resume ktime_get()
> will WARN() because the timekeeping system has been suspended.
>

It seems that all the time keeping functions including
getnstimeofday() now have the "WARN_ON(timekeeping_suspended)" or
WARN_ON_ONCE. ?So I'd like to stick with ktime_get unless I'm missing
something about the suspend warning or unless there is some other
reason why getnstieofday() is preferred.

[PATCH v3 0/7] Add common cpuidle code for consolidation.

[Kevin Hilman]

Rob Lee <rob.lee@linaro.org> writes:

> On Tue, Jan 24, 2012 at 2:08 PM, Kevin Hilman <khilman@ti.com> wrote:
>> I noticed that the generic code uses ktime_get() for measuring time.  On
>> OMAP, we use getnstimeofday() because I while back I remember having
>> problems with the interaction of CPUidle state measurements and system
>> suspend.  Any idle activity during system suspend/resume ktime_get()
>> will WARN() because the timekeeping system has been suspended.
>
> It seems that all the time keeping functions including
> getnstimeofday() now have the "WARN_ON(timekeeping_suspended)" or
> WARN_ON_ONCE. ?So I'd like to stick with ktime_get unless I'm missing
> something about the suspend warning or unless there is some other
> reason why getnstieofday() is preferred.

I agree.  

I've convinced myself that my use of getnstimeofday() is because of a
problem that doesn't exist anymore in mainline, and now that the
timekeeping core has been converted to use syscore_ops, use of
ktime_get() is good.

Kevin