Add common cpuidle code for consolidation.

The patch adds some cpuidle initialization functionality commonly duplicated by many platforms. The duplicate cpuidle init code of various platfroms has been consolidated to use this common code and successfully rebuilt.

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

Quoted 6 lines · Robert Lee

The patch adds some cpuidle initialization functionality commonly duplicated by many platforms. ?The duplicate cpuidle init code of various platfroms has been consolidated to use this common code and successfully rebuilt.

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

Reviewed-by Mark Brown <[email protected]>

This looks good to me with one small comment:

Review

Quoted 5 lines

(quoted diff) +5 -0

+int cpuidle_def_idle(struct cpuidle_device *dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index) {
+ ? ? cpu_do_idle();
+ ? ? return index;
+}

Odd indentation here.

I'll move my s3c64xx cpuidle code over to this, though I think it'll end up being an incremental patch as Kukjin just said he'd apply it and it'd be nice to get the support into 3.3 if we can.

The patch adds some cpuidle initialization functionality commonly duplicated by many platforms. The duplicate cpuidle init code of various platfroms has been consolidated to use this common code and successfully rebuilt.

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

---
 arch/arm/mach-at91/cpuidle.c     |   98 ++++++++++----------------
 arch/arm/mach-davinci/cpuidle.c  |  143 +++++++++++---------------------------
 arch/arm/mach-exynos/cpuidle.c   |   73 +++----------------
 arch/arm/mach-kirkwood/cpuidle.c |   94 ++++++++-----------------
 arch/arm/mach-shmobile/cpuidle.c |   40 ++---------
 drivers/cpuidle/Makefile         |    2 +-
 drivers/cpuidle/common.c         |  124 +++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h          |   26 +++++++
 8 files changed, 277 insertions(+), 323 deletions(-)
 create mode 100644 drivers/cpuidle/common.c

arch/arm/mach-at91/cpuidle.c +35 -52

index a851e6c..b162aab 100644
@@ -1,6 +1,4 @@
 /*
- * based on arch/arm/mach-kirkwood/cpuidle.c
- *
  * CPU idle support for AT91 SoC
  *
  * This file is licensed under the terms of the GNU General Public

@@ -17,84 +15,60 @@
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/cpuidle.h>
-#include <asm/proc-fns.h>
 #include <linux/io.h>
 #include <linux/export.h>
-
+#include <asm/proc-fns.h>
 #include "pm.h"
 
-#define AT91_MAX_STATES	2
-
-static DEFINE_PER_CPU(struct cpuidle_device, at91_cpuidle_device);
-
-static struct cpuidle_driver at91_idle_driver = {
-	.name =         "at91_idle",
-	.owner =        THIS_MODULE,
-};
+#define AT91_MAX_STATES 2
 
 /* Actual code that puts the SoC in different idle states */
-static int at91_enter_idle(struct cpuidle_device *dev,
+static int at91_idle_dram(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
 			       int index)
 {
-	struct timeval before, after;
-	int idle_time;
 	u32 saved_lpr;
 
-	local_irq_disable();
-	do_gettimeofday(&before);
-	if (index == 0)
-		/* Wait for interrupt state */
-		cpu_do_idle();
-	else if (index == 1) {
-		asm("b 1f; .align 5; 1:");
-		asm("mcr p15, 0, r0, c7, c10, 4");	/* drain write buffer */
-		saved_lpr = sdram_selfrefresh_enable();
-		cpu_do_idle();
-		sdram_selfrefresh_disable(saved_lpr);
-	}
-	do_gettimeofday(&after);
-	local_irq_enable();
-	idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
-			(after.tv_usec - before.tv_usec);
+	asm("b 1f; .align 5; 1:");
+	asm("mcr p15, 0, r0, c7, c10, 4");	/* drain write buffer */
+	saved_lpr = sdram_selfrefresh_enable();
+	cpu_do_idle();
+	sdram_selfrefresh_disable(saved_lpr);
 
-	dev->last_residency = idle_time;
 	return index;
 }
 
+static struct cpuidle_driver at91_idle_driver = {
+	.name =         "at91_idle",
+	.owner =        THIS_MODULE,
+	.states[0] = {
+		.enter			= cpuidle_def_idle,
+		.exit_latency		= 1,
+		.target_residency	= 100000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "WFI",
+		.desc			= "Wait for interrupt",
+	},
+	.states[1] = {
+		.enter			= at91_idle_dram,
+		.exit_latency		= 10,
+		.target_residency	= 100000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "RAM_SR",
+		.desc			= "WFI and RAM Self Refresh",
+	},
+	.state_count = AT91_MAX_STATES,
+};
+
 /* Initialize CPU idle by registering the idle states */
-static int at91_init_cpuidle(void)
+static int __init at91_init_cpuidle(void)
 {
-	struct cpuidle_device *device;
-	struct cpuidle_driver *driver = &at91_idle_driver;
+	int ret;
 
-	device = &per_cpu(at91_cpuidle_device, smp_processor_id());
-	device->state_count = AT91_MAX_STATES;
-	driver->state_count = AT91_MAX_STATES;
+	ret = common_cpuidle_init(&at91_idle_driver,
+					true,
+					NULL);
 
-	/* Wait for interrupt state */
-	driver->states[0].enter = at91_enter_idle;
-	driver->states[0].exit_latency = 1;
-	driver->states[0].target_residency = 10000;
-	driver->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
-	strcpy(driver->states[0].name, "WFI");
-	strcpy(driver->states[0].desc, "Wait for interrupt");
-
-	/* Wait for interrupt and RAM self refresh state */
-	driver->states[1].enter = at91_enter_idle;
-	driver->states[1].exit_latency = 10;
-	driver->states[1].target_residency = 10000;
-	driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID;
-	strcpy(driver->states[1].name, "RAM_SR");

arch/arm/mach-davinci/cpuidle.c +0 -13

 #include <mach/cpuidle.h>
 #include <mach/ddr2.h>
 
 #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,
-};

arch/arm/mach-davinci/cpuidle.c +35 -86

 {
 	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 = {
+	.name	= "cpuidle-davinci",
+	.owner	= THIS_MODULE,
+	.states[0] = {
+		.enter			= cpuidle_def_idle,
+		.exit_latency		= 1,
+		.target_residency	= 100000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "WFI",
+		.desc			= "Wait for interrupt",
 	},
+	.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 +88,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 +112,3 @@ static int __init davinci_cpuidle_init(void)
 						davinci_cpuidle_probe);
 }
 device_initcall(davinci_cpuidle_init);
-

arch/arm/mach-exynos/cpuidle.c +1 -9

index 4ebb382..7c863ca 100644
@@ -13,80 +13,29 @@
 #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,
+static struct cpuidle_driver exynos4_idle_driver = {

arch/arm/mach-exynos/cpuidle.c +2 -47

+	.state_count = 1,
 };
 
-static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
-
-static struct cpuidle_driver exynos4_idle_driver = {
-	.name		= "exynos4_idle",
-	.owner		= THIS_MODULE,
-};
-
-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;
+	int ret;

arch/arm/mach-exynos/cpuidle.c +1 -0

+	return ret;
 }
 device_initcall(exynos4_init_cpuidle);

arch/arm/mach-kirkwood/cpuidle.c +31 -63

index 7088180..965cf25 100644
@@ -24,80 +24,48 @@
 
 #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)
 {
-	struct timeval before, after;
-	int idle_time;
-
-	local_irq_disable();
-	do_gettimeofday(&before);
-	if (index == 0)
-		/* Wait for interrupt state */
-		cpu_do_idle();
-	else if (index == 1) {
-		/*
-		 * Following write will put DDR in self refresh.
-		 * Note that we have 256 cycles before DDR puts it
-		 * self in self-refresh, so the wait-for-interrupt
-		 * call afterwards won't get the DDR from self refresh
-		 * mode.
-		 */
-		writel(0x7, DDR_OPERATION_BASE);
-		cpu_do_idle();
-	}
-	do_gettimeofday(&after);
-	local_irq_enable();
-	idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
-			(after.tv_usec - before.tv_usec);
-
-	/* Update last residency */
-	dev->last_residency = idle_time;
+	writel(0x7, DDR_OPERATION_BASE);
+	cpu_do_idle();
 
 	return index;
 }
 
+static struct cpuidle_driver kirkwood_idle_driver = {
+	.name =         "kirkwood_idle",
+	.owner =        THIS_MODULE,
+	.states[0] = {
+		.enter			= cpuidle_def_idle,
+		.exit_latency		= 1,
+		.target_residency	= 100000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "WFI",
+		.desc			= "Wait for interrupt",
+	},
+	.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;
+	int ret;
 
-	/* 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");
+	ret = common_cpuidle_init(&kirkwood_idle_driver,
+					true,
+					NULL);
 
-	/* 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 ret;
 }
-
 device_initcall(kirkwood_init_cpuidle);

arch/arm/mach-shmobile/cpuidle.c +8 -32

index 1b23342..3e6a393 100644
@@ -16,42 +16,22 @@
 #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 = {
 	.name =		"shmobile_cpuidle",
 	.owner =	THIS_MODULE,
 	.states[0] = {
+		.enter = cpuidle_def_idle,
 		.name = "C1",
 		.desc = "WFI",
 		.exit_latency = 1,

@@ -64,23 +44,19 @@ static struct cpuidle_driver shmobile_cpuidle_driver = {
 
 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;
+	int i, ret;
 	struct cpuidle_driver *drv = &shmobile_cpuidle_driver;
-	int i;
 
-	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);
+		shmobile_cpuidle_setup(&shmobile_cpuidle_driver);
 
-	dev->state_count = drv->state_count;
-	cpuidle_register_device(dev);
+	ret = common_cpuidle_init(&shmobile_cpuidle_driver, true, NULL);
 
-	return 0;
+	return ret;
 }
 late_initcall(shmobile_cpuidle_init);

drivers/cpuidle/Makefile +1 -1

index 5634f88..2928d93 100644
@@ -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/

drivers/cpuidle/common.c new file +79 -0

new file mode 100644
index 0000000..c9d6c14
@@ -0,0 +1,124 @@
+/*
+ * 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 <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);
+}
+
+int __init common_cpuidle_init(struct cpuidle_driver *drv, bool simple,
+			 void (*driver_data_init)(struct cpuidle_device *dev))
+{
+	struct cpuidle_device *dev;
+	int i, cpu_id, ret;
+
+	if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
+		pr_err("%s: Invalid Input\n", __func__);
+		return -EINVAL;

drivers/cpuidle/common.c new file +11 -0

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

drivers/cpuidle/common.c new file +14 -0

+			goto uninit;
+		}
+	}
+
+	return 0;
+uninit:
+
+	common_cpuidle_devices_uninit();
+
+unregister_drv:
+
+	cpuidle_unregister_driver(drv);
+	return ret;
+}

include/linux/cpuidle.h +5 -0

index 7408af8..3ce6955 100644
@@ -142,6 +142,25 @@ 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);
+
+/*

include/linux/cpuidle.h +11 -0

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

@@ -159,6 +178,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 *drv,
+	bool simple, void (*driver_data_init)(struct cpuidle_device *dev))
+{return -ENODEV; }
+static inline void common_cpuidle_devices_uninit(void) { }
 
 #endif
 

Quoted 6 lines · Robert Lee

The patch adds some cpuidle initialization functionality commonly duplicated by many platforms. ?The duplicate cpuidle init code of various platfroms has been consolidated to use this common code and successfully rebuilt.

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

Generally it helps if you copy the reviewers of the previous version as I missed this one until today.

Looks pretty good. A few comments below.

Quoted 131 lines

---

?arch/arm/mach-at91/cpuidle.c ? ? | ? 98 ++++++++++----------------
?arch/arm/mach-davinci/cpuidle.c ?| ?143 +++++++++++---------------------------
?arch/arm/mach-exynos/cpuidle.c ? | ? 73 +++----------------
?arch/arm/mach-kirkwood/cpuidle.c | ? 94 ++++++++-----------------
?arch/arm/mach-shmobile/cpuidle.c | ? 40 ++---------
?drivers/cpuidle/Makefile ? ? ? ? | ? ?2 +-
?drivers/cpuidle/common.c ? ? ? ? | ?124 +++++++++++++++++++++++++++++++++
?include/linux/cpuidle.h ? ? ? ? ?| ? 26 +++++++

?8 files changed, 277 insertions(+), 323 deletions(-) ?create mode 100644 drivers/cpuidle/common.c

arch/arm/mach-at91/cpuidle.c +0 -0

index a851e6c..b162aab 100644
@@ -1,6 +1,4 @@

?/*
- * based on arch/arm/mach-kirkwood/cpuidle.c
- *
? * CPU idle support for AT91 SoC
? *
? * This file is licensed under the terms of the GNU General Public

arch/arm/mach-at91/cpuidle.c +0 -0

@@ -17,84 +15,60 @@

?#include <linux/init.h>
?#include <linux/platform_device.h>
?#include <linux/cpuidle.h>
-#include <asm/proc-fns.h>
?#include <linux/io.h>
?#include <linux/export.h>

arch/arm/mach-at91/cpuidle.c +1 -1

-
+#include <asm/proc-fns.h>

?#include "pm.h"

arch/arm/mach-at91/cpuidle.c +1 -8

-#define AT91_MAX_STATES ? ? ?2
-
-static DEFINE_PER_CPU(struct cpuidle_device, at91_cpuidle_device);
-
-static struct cpuidle_driver at91_idle_driver = {
- ? ? .name = ? ? ? ? "at91_idle",
- ? ? .owner = ? ? ? ?THIS_MODULE,
-};
+#define AT91_MAX_STATES 2

?/* Actual code that puts the SoC in different idle states */

arch/arm/mach-at91/cpuidle.c +1 -1

-static int at91_enter_idle(struct cpuidle_device *dev,
+static int at91_idle_dram(struct cpuidle_device *dev,

? ? ? ? ? ? ? ? ? ? ? struct cpuidle_driver *drv,
? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int index)
?{
- ? ? struct timeval before, after;
- ? ? int idle_time;
? ? ? u32 saved_lpr;

- ? ? local_irq_disable();
- ? ? do_gettimeofday(&before);
- ? ? if (index == 0)
- ? ? ? ? ? ? /* Wait for interrupt state */
- ? ? ? ? ? ? cpu_do_idle();
- ? ? else if (index == 1) {
- ? ? ? ? ? ? asm("b 1f; .align 5; 1:");
- ? ? ? ? ? ? asm("mcr p15, 0, r0, c7, c10, 4"); ? ? ?/* drain write buffer */
- ? ? ? ? ? ? saved_lpr = sdram_selfrefresh_enable();
- ? ? ? ? ? ? cpu_do_idle();
- ? ? ? ? ? ? sdram_selfrefresh_disable(saved_lpr);
- ? ? }
- ? ? do_gettimeofday(&after);
- ? ? local_irq_enable();
- ? ? idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
- ? ? ? ? ? ? ? ? ? ? (after.tv_usec - before.tv_usec);
+ ? ? asm("b 1f; .align 5; 1:");
+ ? ? asm("mcr p15, 0, r0, c7, c10, 4"); ? ? ?/* drain write buffer */
+ ? ? saved_lpr = sdram_selfrefresh_enable();
+ ? ? cpu_do_idle();
+ ? ? sdram_selfrefresh_disable(saved_lpr);

- ? ? dev->last_residency = idle_time;
? ? ? return index;
?}

arch/arm/mach-at91/cpuidle.c +22 -0

+static struct cpuidle_driver at91_idle_driver = {
+ ? ? .name = ? ? ? ? "at91_idle",
+ ? ? .owner = ? ? ? ?THIS_MODULE,
+ ? ? .states[0] = {
+ ? ? ? ? ? ? .enter ? ? ? ? ? ? ? ? ?= cpuidle_def_idle,
+ ? ? ? ? ? ? .exit_latency ? ? ? ? ? = 1,
+ ? ? ? ? ? ? .target_residency ? ? ? = 100000,
+ ? ? ? ? ? ? .flags ? ? ? ? ? ? ? ? ?= CPUIDLE_FLAG_TIME_VALID,
+ ? ? ? ? ? ? .name ? ? ? ? ? ? ? ? ? = "WFI",
+ ? ? ? ? ? ? .desc ? ? ? ? ? ? ? ? ? = "Wait for interrupt",
+ ? ? },
+ ? ? .states[1] = {
+ ? ? ? ? ? ? .enter ? ? ? ? ? ? ? ? ?= at91_idle_dram,
+ ? ? ? ? ? ? .exit_latency ? ? ? ? ? = 10,
+ ? ? ? ? ? ? .target_residency ? ? ? = 100000,
+ ? ? ? ? ? ? .flags ? ? ? ? ? ? ? ? ?= CPUIDLE_FLAG_TIME_VALID,
+ ? ? ? ? ? ? .name ? ? ? ? ? ? ? ? ? = "RAM_SR",
+ ? ? ? ? ? ? .desc ? ? ? ? ? ? ? ? ? = "WFI and RAM Self Refresh",
+ ? ? },
+ ? ? .state_count = AT91_MAX_STATES,
+};
+

?/* Initialize CPU idle by registering the idle states */

arch/arm/mach-at91/cpuidle.c +1 -1

-static int at91_init_cpuidle(void)
+static int __init at91_init_cpuidle(void)

?{
- ? ? struct cpuidle_device *device;
- ? ? struct cpuidle_driver *driver = &at91_idle_driver;
+ ? ? int ret;

- ? ? device = &per_cpu(at91_cpuidle_device, smp_processor_id());
- ? ? device->state_count = AT91_MAX_STATES;
- ? ? driver->state_count = AT91_MAX_STATES;
+ ? ? ret = common_cpuidle_init(&at91_idle_driver,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? true,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);

- ? ? /* Wait for interrupt state */
- ? ? driver->states[0].enter = at91_enter_idle;
- ? ? driver->states[0].exit_latency = 1;
- ? ? driver->states[0].target_residency = 10000;
- ? ? driver->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
- ? ? strcpy(driver->states[0].name, "WFI");
- ? ? strcpy(driver->states[0].desc, "Wait for interrupt");

arch/arm/mach-at91/cpuidle.c +0 -7

-
- ? ? /* Wait for interrupt and RAM self refresh state */
- ? ? driver->states[1].enter = at91_enter_idle;
- ? ? driver->states[1].exit_latency = 10;
- ? ? driver->states[1].target_residency = 10000;
- ? ? driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID;
- ? ? strcpy(driver->states[1].name, "RAM_SR");

arch/arm/mach-at91/cpuidle.c

Quoted 10 lines

arch/arm/mach-at91/cpuidle.c +1 -9

- ? ? strcpy(driver->states[1].desc, "WFI and RAM Self Refresh");
-
- ? ? cpuidle_register_driver(&at91_idle_driver);
-
- ? ? if (cpuidle_register_device(device)) {
- ? ? ? ? ? ? printk(KERN_ERR "at91_init_cpuidle: Failed registering\n");
- ? ? ? ? ? ? return -EIO;
- ? ? }
- ? ? return 0;
+ ? ? return ret;

This can all be 1 line:

return common_cpuidle_init(&at91_idle_driver, true, NULL);

Quoted 8 lines

?}
-
?device_initcall(at91_init_cpuidle);

arch/arm/mach-davinci/cpuidle.c +0 -0

index a30c7c5..b12f30b 100644
@@ -18,104 +18,69 @@

Quoted 4 lines

?#include <linux/io.h>
?#include <linux/export.h>
?#include <asm/proc-fns.h>
-

Extra whitespace change.

Quoted 23 lines

?#include <mach/cpuidle.h>
?#include <mach/ddr2.h>

?#define DAVINCI_CPUIDLE_MAX_STATES ? 2

arch/arm/mach-davinci/cpuidle.c +1 -15

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

arch/arm/mach-davinci/cpuidle.c

Quoted 5 lines

arch/arm/mach-davinci/cpuidle.c +4 -1

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

Line up the parameters.

Quoted 188 lines

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

arch/arm/mach-davinci/cpuidle.c +6 -8

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

?}

arch/arm/mach-davinci/cpuidle.c +10 -4

-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 = {
+ ? ? .name ? = "cpuidle-davinci",
+ ? ? .owner ?= THIS_MODULE,
+ ? ? .states[0] = {
+ ? ? ? ? ? ? .enter ? ? ? ? ? ? ? ? ?= cpuidle_def_idle,
+ ? ? ? ? ? ? .exit_latency ? ? ? ? ? = 1,
+ ? ? ? ? ? ? .target_residency ? ? ? = 100000,
+ ? ? ? ? ? ? .flags ? ? ? ? ? ? ? ? ?= CPUIDLE_FLAG_TIME_VALID,
+ ? ? ? ? ? ? .name ? ? ? ? ? ? ? ? ? = "WFI",
+ ? ? ? ? ? ? .desc ? ? ? ? ? ? ? ? ? = "Wait for interrupt",

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

arch/arm/mach-davinci/cpuidle.c +2 -4

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

arch/arm/mach-davinci/cpuidle.c +1 -19

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

arch/arm/mach-davinci/cpuidle.c +6 -34

@@ -123,42 +88,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 +112,3 @@ static int __init davinci_cpuidle_init(void)
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? davinci_cpuidle_probe);
 ?}
 ?device_initcall(davinci_cpuidle_init);
-

arch/arm/mach-exynos/cpuidle.c +0 -0

index 4ebb382..7c863ca 100644
@@ -13,80 +13,29 @@

?#include <linux/cpuidle.h>
?#include <linux/io.h>
?#include <linux/export.h>

arch/arm/mach-exynos/cpuidle.c +0 -2

-#include <linux/time.h>
-

?#include <asm/proc-fns.h>

arch/arm/mach-exynos/cpuidle.c +5 -7

-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,
+static struct cpuidle_driver exynos4_idle_driver = {
+ ? ? .name ? ? ? ? ? = "exynos4_idle",
+ ? ? .owner ? ? ? ? ?= THIS_MODULE,
+ ? ? .states[0] = {
+ ? ? ? ? ? ? .enter ? ? ? ? ? ? ? ? ?= cpuidle_def_idle,

? ? ? ? ? ? ? .exit_latency ? ? ? ? ? = 1,
? ? ? ? ? ? ? .target_residency ? ? ? = 100000,
? ? ? ? ? ? ? .flags ? ? ? ? ? ? ? ? ?= CPUIDLE_FLAG_TIME_VALID, ? ? ? ? ? ? ? .name ? ? ? ? ? ? ? ? ? = "IDLE",

Quoted 2 lines

? ? ? ? ? ? ? .desc ? ? ? ? ? ? ? ? ? = "ARM clock gating(WFI)", ? ? ? },

As this is just plain wfi and shouldn't really be different per platform, it would be nice to get rid of all of this state info. Perhaps a macro with all the data since each driver needs to init each state. The target residency value looks kind of suspect. You should only go to wfi if you expect to be idle for 100ms?

Quoted 53 lines

+ ? ? .state_count = 1,
?};

arch/arm/mach-exynos/cpuidle.c +0 -28

-static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
-
-static struct cpuidle_driver exynos4_idle_driver = {
- ? ? .name ? ? ? ? ? = "exynos4_idle",
- ? ? .owner ? ? ? ? ?= THIS_MODULE,
-};
-
-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;

arch/arm/mach-exynos/cpuidle.c +1 -16

-
- ? ? /* 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;
+ ? ? int ret;

arch/arm/mach-exynos/cpuidle.c

Quoted 10 lines

arch/arm/mach-exynos/cpuidle.c +3 -6

 
- ? ? ? ? ? ? if (cpuidle_register_device(device)) {
- ? ? ? ? ? ? ? ? ? ? printk(KERN_ERR "CPUidle register device failed\n,");
- ? ? ? ? ? ? ? ? ? ? return -EIO;
- ? ? ? ? ? ? }
- ? ? }
- ? ? return 0;
+ ? ? ret = common_cpuidle_init(&exynos4_idle_driver,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? true,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);

This can all be 1 line:

return common_cpuidle_init(&exynos4_idle_driver, true, NULL);

Quoted 297 lines

+ ? ? return ret;
?}
?device_initcall(exynos4_init_cpuidle);

arch/arm/mach-kirkwood/cpuidle.c +0 -0

index 7088180..965cf25 100644
@@ -24,80 +24,48 @@
 

?#define KIRKWOOD_MAX_STATES ?2

arch/arm/mach-kirkwood/cpuidle.c +0 -7

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

arch/arm/mach-kirkwood/cpuidle.c +1 -1

-static int kirkwood_enter_idle(struct cpuidle_device *dev,
+static int kirkwood_idle_ddr(struct cpuidle_device *dev,

? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct cpuidle_driver *drv, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int index)
?{
- ? ? struct timeval before, after;
- ? ? int idle_time;

arch/arm/mach-kirkwood/cpuidle.c +2 -24

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

arch/arm/mach-kirkwood/cpuidle.c +22 -0

+static struct cpuidle_driver kirkwood_idle_driver = {
+ ? ? .name = ? ? ? ? "kirkwood_idle",
+ ? ? .owner = ? ? ? ?THIS_MODULE,
+ ? ? .states[0] = {
+ ? ? ? ? ? ? .enter ? ? ? ? ? ? ? ? ?= cpuidle_def_idle,
+ ? ? ? ? ? ? .exit_latency ? ? ? ? ? = 1,
+ ? ? ? ? ? ? .target_residency ? ? ? = 100000,
+ ? ? ? ? ? ? .flags ? ? ? ? ? ? ? ? ?= CPUIDLE_FLAG_TIME_VALID,
+ ? ? ? ? ? ? .name ? ? ? ? ? ? ? ? ? = "WFI",
+ ? ? ? ? ? ? .desc ? ? ? ? ? ? ? ? ? = "Wait for interrupt",
+ ? ? },
+ ? ? .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 */

arch/arm/mach-kirkwood/cpuidle.c +1 -1

-static int kirkwood_init_cpuidle(void)
+static int __init kirkwood_init_cpuidle(void)

?{
- ? ? struct cpuidle_device *device;
- ? ? struct cpuidle_driver *driver = &kirkwood_idle_driver;

arch/arm/mach-kirkwood/cpuidle.c +5 -25

-
- ? ? device = &per_cpu(kirkwood_cpuidle_device, smp_processor_id());
- ? ? device->state_count = KIRKWOOD_MAX_STATES;
- ? ? driver->state_count = KIRKWOOD_MAX_STATES;
+ ? ? int ret;
 
- ? ? /* 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");
+ ? ? ret = common_cpuidle_init(&kirkwood_idle_driver,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? true,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
 
- ? ? /* 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 ret;

?}
-
?device_initcall(kirkwood_init_cpuidle);

arch/arm/mach-shmobile/cpuidle.c +0 -0

index 1b23342..3e6a393 100644
@@ -16,42 +16,22 @@

?#include <asm/system.h>
?#include <asm/io.h>

arch/arm/mach-shmobile/cpuidle.c +1 -8

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

arch/arm/mach-shmobile/cpuidle.c +0 -6

-
- ? ? before = ktime_get();
-
- ? ? local_irq_disable();
- ? ? local_fiq_disable();
-

? ? ? shmobile_cpuidle_modes[index]();

- ? ? local_irq_enable();
- ? ? local_fiq_enable();

arch/arm/mach-shmobile/cpuidle.c +0 -4

-
- ? ? 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 = { ? ? ? .name = ? ? ? ? "shmobile_cpuidle",
? ? ? .owner = ? ? ? ?THIS_MODULE,
? ? ? .states[0] = {
+ ? ? ? ? ? ? .enter = cpuidle_def_idle,
? ? ? ? ? ? ? .name = "C1",
? ? ? ? ? ? ? .desc = "WFI",
? ? ? ? ? ? ? .exit_latency = 1,

arch/arm/mach-shmobile/cpuidle.c +6 -10

@@ -64,23 +44,19 @@ static struct cpuidle_driver shmobile_cpuidle_driver = {
 
 ?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;
+ ? ? int i, ret;
 ? ? ? struct cpuidle_driver *drv = &shmobile_cpuidle_driver;
- ? ? int i;
 
- ? ? 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);
+ ? ? ? ? ? ? shmobile_cpuidle_setup(&shmobile_cpuidle_driver);
 
- ? ? dev->state_count = drv->state_count;
- ? ? cpuidle_register_device(dev);
+ ? ? ret = common_cpuidle_init(&shmobile_cpuidle_driver, true, NULL);
 
- ? ? return 0;
+ ? ? return ret;
 ?}
 ?late_initcall(shmobile_cpuidle_init);

drivers/cpuidle/Makefile +0 -0

index 5634f88..2928d93 100644
@@ -2,4 +2,4 @@

?# Makefile for cpuidle.
?#

drivers/cpuidle/Makefile +1 -1

-obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
+obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/

drivers/cpuidle/common.c new file +83 -0

new file mode 100644
index 0000000..c9d6c14
@@ -0,0 +1,124 @@
+/*
+ * 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 <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);
+}
+
+int __init common_cpuidle_init(struct cpuidle_driver *drv, bool simple,
+ ? ? ? ? ? ? ? ? ? ? ?void (*driver_data_init)(struct cpuidle_device *dev))
+{
+ ? ? struct cpuidle_device *dev;
+ ? ? int i, cpu_id, ret;
+
+ ? ? if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
+ ? ? ? ? ? ? pr_err("%s: Invalid Input\n", __func__);
+ ? ? ? ? ? ? return -EINVAL;
+ ? ? }
+
+ ? ? for (i = 0; simple && (i < drv->state_count); i++) {
+ ? ? ? ? ? ? do_idle[i] = drv->states[i].enter;

drivers/cpuidle/common.c line 89

Quoted 6 lines

drivers/cpuidle/common.c new file +6 -0

+ ? ? ? ? ? ? drv->states[i].enter = simple_enter;
+ ? ? }
+
+ ? ? if (cpuidle_register_driver(drv)) {
+ ? ? ? ? ? ? pr_err("%s: Failed to register cpuidle driver\n", __func__);
+ ? ? ? ? ? ? return -ENODEV;

It's better to return the error code of the failed function.

Quoted 15 lines

drivers/cpuidle/common.c new file +15 -0

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

drivers/cpuidle/common.c line 110

Quoted 6 lines

drivers/cpuidle/common.c new file +6 -0

+ ? ? ? ? ? ? ? ? ? ? driver_data_init(dev);
+
+ ? ? ? ? ? ? if (cpuidle_register_device(dev)) {
+ ? ? ? ? ? ? ? ? ? ? pr_err("%s: Failed to register cpu %u\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, cpu_id);
+ ? ? ? ? ? ? ? ? ? ? ret = -ENODEV;

Same here.

Quoted 31 lines

drivers/cpuidle/common.c new file +14 -0

+ ? ? ? ? ? ? ? ? ? ? goto uninit;
+ ? ? ? ? ? ? }
+ ? ? }
+
+ ? ? return 0;
+uninit:
+
+ ? ? common_cpuidle_devices_uninit();
+
+unregister_drv:
+
+ ? ? cpuidle_unregister_driver(drv);
+ ? ? return ret;
+}

include/linux/cpuidle.h +9 -0

index 7408af8..3ce6955 100644
@@ -142,6 +142,25 @@ 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);
+
+/*
+ * Common cpuidle init interface to provide common cpuidle functionality
+ * used by many platforms.
+ *
+ * Set simple to true if you want to use the provided common handling

include/linux/cpuidle.h extern void cpuidle_resume_and_unlock(void); · line 159

Quoted 6 lines

include/linux/cpuidle.h +6 -0

+ * for each enter() callback.
+ *
+ * Provide a callback function if you want to modify the cpuidle_device
+ * data after it has been created and before the cpuidle_device has been
+ * registered.
+ */

Function descriptions should be in .c file and in DocBook format.

Quoted 21 lines

include/linux/cpuidle.h +4 -0

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

include/linux/cpuidle.h +7 -0

@@ -159,6 +178,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 *drv,
+ ? ? bool simple, void (*driver_data_init)(struct cpuidle_device *dev))
+{return -ENODEV; }
+static inline void common_cpuidle_devices_uninit(void) { }
 

?#endif

Quoted 13 lines · Rob Herring

Quoted 11 lines · Robert Lee

+static struct cpuidle_driver exynos4_idle_driver = {
+ ? ? .name ? ? ? ? ? = "exynos4_idle",
+ ? ? .owner ? ? ? ? ?= THIS_MODULE,
+ ? ? .states[0] = {
+ ? ? ? ? ? ? .enter ? ? ? ? ? ? ? ? ?= cpuidle_def_idle, ? ? ? ? ? ? ? .exit_latency ? ? ? ? ? = 1,
? ? ? ? ? ? ? .target_residency ? ? ? = 100000,
? ? ? ? ? ? ? .flags ? ? ? ? ? ? ? ? ?= CPUIDLE_FLAG_TIME_VALID, ? ? ? ? ? ? ? .name ? ? ? ? ? ? ? ? ? = "IDLE",
? ? ? ? ? ? ? .desc ? ? ? ? ? ? ? ? ? = "ARM clock gating(WFI)", ? ? ? },

Quoted 6 lines

As this is just plain wfi and shouldn't really be different per platform, it would be nice to get rid of all of this state info. Perhaps a macro with all the data since each driver needs to init each state. The target residency value looks kind of suspect. You should only go to wfi if you expect to be idle for 100ms?

Ok, I'll look at add a ARM specific macro for a generic WFI state in v3.

For the target_residency value, I don't understand why the values being used are there other than some of the original ARM platform implementations were based on the Intel implementations per their comments, and the Intel value was used. ?From the comments in drivers/cpuidle/governors/menu.c: "state entry and exit have an energy cost, and a certain amount of time in the ?C state is required to actually break even on this cost. CPUIDLE provides us this duration in the target_residency field". ?The governor code uses it accordingly. I'm not aware that a pure WFI only state uses additional energy to enter and exit compared to spinning in a loop, so I think the "target_residency" of a pure WFI state value should 0 or 1. ?If a platform skips a pure WFI idle state and also implements additional platform specific power savings in their "WFI" idle state, then they may need to adjust exit_latency and target_residency accordingly.

Quoted 23 lines · Rob Lee

Quoted 13 lines · Rob Herring

arch/arm/mach-exynos/cpuidle.c line 28

Quoted 11 lines · Robert Lee

arch/arm/mach-exynos/cpuidle.c +5 -0

+static struct cpuidle_driver exynos4_idle_driver = {
+     .name           = "exynos4_idle",
+     .owner          = THIS_MODULE,
+     .states[0] = {
+             .enter                  = cpuidle_def_idle,
              .exit_latency           = 1,
              .target_residency       = 100000,
              .flags                  = CPUIDLE_FLAG_TIME_VALID,
              .name                   = "IDLE",
              .desc                   = "ARM clock gating(WFI)",
      },

Quoted 6 lines

As this is just plain wfi and shouldn't really be different per platform, it would be nice to get rid of all of this state info. Perhaps a macro with all the data since each driver needs to init each state. The target residency value looks kind of suspect. You should only go to wfi if you expect to be idle for 100ms?

Ok, I'll look at add a ARM specific macro for a generic WFI state in v3.

Quoted 14 lines

For the target_residency value, I don't understand why the values being used are there other than some of the original ARM platform implementations were based on the Intel implementations per their comments, and the Intel value was used. From the comments in drivers/cpuidle/governors/menu.c: "state entry and exit have an energy cost, and a certain amount of time in the C state is required to actually break even on this cost. CPUIDLE provides us this duration in the target_residency field". The governor code uses it accordingly. I'm not aware that a pure WFI only state uses additional energy to enter and exit compared to spinning in a loop, so I think the "target_residency" of a pure WFI state value should 0 or 1. If a platform skips a pure WFI idle state and also implements additional platform specific power savings in their "WFI" idle state, then they may need to adjust exit_latency and target_residency accordingly.

Extra data point: on OMAP4 we chose .target_residency = 5 for simple WFI state:
http://git.omapzoom.org/?p=kernel/omap.git;a=blob;f=arch/arm/mach-omap2/cpuidle44xx.c;h=383944076085f808b8764baf11df0589229010e5;hb=p-android-omap-3.0#l113

Quoted 39 lines · Mike

Quoted 37 lines · Rob Lee

Quoted 13 lines · Rob Herring

Quoted 11 lines · Robert Lee

+static struct cpuidle_driver exynos4_idle_driver = {
+ ? ? .name ? ? ? ? ? = "exynos4_idle",
+ ? ? .owner ? ? ? ? ?= THIS_MODULE,
+ ? ? .states[0] = {
+ ? ? ? ? ? ? .enter ? ? ? ? ? ? ? ? ?= cpuidle_def_idle, ? ? ? ? ? ? ? .exit_latency ? ? ? ? ? = 1,
? ? ? ? ? ? ? .target_residency ? ? ? = 100000,
? ? ? ? ? ? ? .flags ? ? ? ? ? ? ? ? ?= CPUIDLE_FLAG_TIME_VALID, ? ? ? ? ? ? ? .name ? ? ? ? ? ? ? ? ? = "IDLE",
? ? ? ? ? ? ? .desc ? ? ? ? ? ? ? ? ? = "ARM clock gating(WFI)", ? ? ? },

Quoted 6 lines

As this is just plain wfi and shouldn't really be different per platform, it would be nice to get rid of all of this state info. Perhaps a macro with all the data since each driver needs to init each state. The target residency value looks kind of suspect. You should only go to wfi if you expect to be idle for 100ms?

Ok, I'll look at add a ARM specific macro for a generic WFI state in v3.

For the target_residency value, I don't understand why the values being used are there other than some of the original ARM platform implementations were based on the Intel implementations per their comments, and the Intel value was used. ?From the comments in drivers/cpuidle/governors/menu.c: "state entry and exit have an energy cost, and a certain amount of time in the ?C state is required to actually break even on this cost. CPUIDLE provides us this duration in the target_residency field". ?The governor code uses it accordingly. I'm not aware that a pure WFI only state uses additional energy to enter and exit compared to spinning in a loop, so I think the "target_residency" of a pure WFI state value should 0 or 1. ?If a platform skips a pure WFI idle state and also implements additional platform specific power savings in their "WFI" idle state, then they may need to adjust exit_latency and target_residency accordingly.

Quoted 2 lines

Extra data point: on OMAP4 we chose .target_residency = 5 for simple WFI state:
http://git.omapzoom.org/?p=kernel/omap.git;a=blob;f=arch/arm/mach-omap2/cpuidle44xx.c;h=383944076085f808b8764baf11df0589229010e5;hb=p-android-omap-3.0#l113

Is there any data behind that value is or is just a different made up value?

The default/highest power state is always state 0, so that will be picked unless states 1-N meet the requirements. There is no spin loop unless a driver implements one. So it doesn't really matter what the target_residency or exit_latency values are for state 0. I would use 1 for both.

Rob

Quoted 2 lines · Robert Lee

+ ? ? asm("b 1f; .align 5; 1:");
+ ? ? asm("mcr p15, 0, r0, c7, c10, 4"); ? ? ?/* drain write buffer */

Err, no. ?The compiler is free to add whatever instructions it sees fit between these two asm() statements.

If you want to ask the compiler to issue a set of assembly instructions as a block, you have to use just one asm() statement.

Quoted 1 line · Robert Lee

+ ? ? clk_enable(&gpc_dvfs_clk);

Should these enables be in the cpuidle code? ?The device appears to have been working fine without them thus far... ?Alternatively, if they should be on anyway does this need to be split out and sent as a bug fix?

Quoted 8 lines · Mark Brown

Quoted 1 line · Robert Lee

+ ? ? clk_enable(&gpc_dvfs_clk);

Should these enables be in the cpuidle code? ?The device appears to have been working fine without them thus far... ?Alternatively, if they should be on anyway does this need to be split out and sent as a bug fix?

This clock is used by the existing pm_suspend code for i.MX51 and other future code being worked on. Since it uses extremely minimal power and is required to be enabled during low power modes, it seemed cleanest to just enable it during clock init.

But I forgot to remove
it from it's enabling from i.MX51 pm_suspend code so I can do that for v3.

On 22 December 2011 11:50, Mark Brown

Quoted 8 lines · Robert Lee

Quoted 2 lines

+ ? ? clk_enable(&gpc_dvfs_clk);

Should these enables be in the cpuidle code? ?The device appears to have been working fine without them thus far... ?Alternatively, if they should be on anyway does this need to be split out and sent as a bug fix?

This clock is used by the existing pm_suspend code for i.MX51 and other future code being worked on. Since it uses extremely minimal power and is required to be enabled during low power modes, it seemed cleanest to just enable it during clock init. But I forgot to remove it from it's enabling from i.MX51 pm_suspend code so I can do that for v3.

Quoted 10 lines · Rob Lee

On 22 December 2011 11:50, Mark Brown

Quoted 8 lines · Robert Lee

Quoted 2 lines

+ ? ? clk_enable(&gpc_dvfs_clk);

Should these enables be in the cpuidle code? ?The device appears to have been working fine without them thus far... ?Alternatively, if they should be on anyway does this need to be split out and sent as a bug fix?

Quoted 6 lines

This clock is used by the existing pm_suspend code for i.MX51 and other future code being worked on. ?Since it uses extremely minimal power and is required to be enabled during low power modes, it seemed cleanest to just enable it during clock init. ?But I forgot to remove it from it's enabling from i.MX51 pm_suspend code so I can do that for v3.

Sounds like it's worth splitting out and getting it merged as quickly as possible then? ?It wasn't the code I was querying, it was the way it is being merged.