[RFC PATCH 0/8] Add common ARM cpuidle init code

[RFC PATCH 0/8] Add common ARM cpuidle init code

[Robert Lee]

The patchset adds some cpuidle initialization functionality commonly
duplicated by many ARM platforms.  The duplicate cpuidle init code of the
various ARM platforms has been modified to use this common code.
The omap cpuidle initialization impelmentation doesn't quite fit into
this common framework so it was left alone.

All the modified ARM platforms were built against the new code and
tests were ran on a i.MX51 Babbage board using the new mx5 cpuidle
implementation included in this patchset.

Questions: 

Is arch/arm/common/ the correct location for this code?

Should the mx5 implementation be moved to a separate patchset?

Robert Lee (8):
  ARM: Add commonly used cpuidle init code
  ARM: at91: Replace init with new common ARM cpuidle init code
  ARM: kirkwood: Replace init with new common ARM cpuidle init code
  ARM: exynos: Replace init with new common ARM cpuidle init code
  ARM: davinci: Replace init with new common ARM cpuidle init code
  ARM: shmobile: Replace init with new common ARM cpuidle init code
  ARM: imx: Add mx5 clock changes necessary for low power
  ARM: imx: Add mx5 cpuidle implmentation

 arch/arm/common/Makefile            |    1 +
 arch/arm/common/cpuidle.c           |  132 +++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/cpuidle.h      |   25 +++++++
 arch/arm/mach-at91/cpuidle.c        |   68 ++++++------------
 arch/arm/mach-davinci/cpuidle.c     |   73 ++++++-------------
 arch/arm/mach-exynos/cpuidle.c      |   66 +++--------------
 arch/arm/mach-kirkwood/cpuidle.c    |   63 ++++++-----------
 arch/arm/mach-mx5/Makefile          |    3 +-
 arch/arm/mach-mx5/clock-mx51-mx53.c |    3 +
 arch/arm/mach-mx5/cpuidle.c         |   59 ++++++++++++++++
 arch/arm/mach-shmobile/cpuidle.c    |   33 ++-------
 11 files changed, 308 insertions(+), 218 deletions(-)
 create mode 100644 arch/arm/common/cpuidle.c
 create mode 100644 arch/arm/include/asm/cpuidle.h
 create mode 100644 arch/arm/mach-mx5/cpuidle.c

[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code

[Robert Lee]

Add commonly used cpuidle init code to avoid unecessary duplication.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/common/Makefile       |    1 +
 arch/arm/common/cpuidle.c      |  132 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/cpuidle.h |   25 ++++++++
 3 files changed, 158 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/common/cpuidle.c
 create mode 100644 arch/arm/include/asm/cpuidle.h

diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
index 6ea9b6f..0865f69 100644
--- a/arch/arm/common/Makefile
+++ b/arch/arm/common/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_ARCH_IXP2000)	+= uengine.o
 obj-$(CONFIG_ARCH_IXP23XX)	+= uengine.o
 obj-$(CONFIG_PCI_HOST_ITE8152)  += it8152.o
 obj-$(CONFIG_ARM_TIMER_SP804)	+= timer-sp.o
+obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
diff --git a/arch/arm/common/cpuidle.c b/arch/arm/common/cpuidle.c
new file mode 100644
index 0000000..e9a46a3
--- /dev/null
+++ b/arch/arm/common/cpuidle.c
@@ -0,0 +1,132 @@
+/*
+ * 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/cpuidle.h>
+#include <asm/proc-fns.h>
+
+static struct cpuidle_device __percpu * arm_cpuidle_devices;
+
+static int (*mach_cpuidle)(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv,
+				int index);
+
+static int arm_enter_idle(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv, int index)
+{
+	ktime_t time_start, time_end;
+
+	local_irq_disable();
+	local_fiq_disable();
+
+	time_start = ktime_get();
+
+	index = mach_cpuidle(dev, drv, index);
+
+	time_end = ktime_get();
+
+	local_fiq_enable();
+	local_irq_enable();
+
+	dev->last_residency =
+		(int)ktime_to_us(ktime_sub(time_end, time_start));
+
+	return index;
+}
+
+void arm_cpuidle_devices_uninit(void)
+{
+	int cpu_id;
+	struct cpuidle_device *dev;
+
+	for_each_possible_cpu(cpu_id) {
+		dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
+		cpuidle_unregister_device(dev);
+	}
+
+	free_percpu(arm_cpuidle_devices);
+	return;
+}
+
+int __init arm_cpuidle_init(struct cpuidle_driver *drv,
+	int (*common_enter)(struct cpuidle_device *dev,
+		struct cpuidle_driver *drv, int index),
+	void *driver_data[])
+{
+	struct cpuidle_device *dev;
+	int i, cpu_id;
+
+	if (drv == NULL) {
+		pr_err("%s: cpuidle_driver pointer NULL\n", __func__);
+		return -EINVAL;
+	}
+
+	if (drv->state_count > CPUIDLE_STATE_MAX)
+		pr_err("%s: state count exceeds maximum\n", __func__);
+
+	mach_cpuidle = common_enter;
+
+	/* if state enter function not specified, use common_enter function */
+	for (i = 0; i < drv->state_count; i++) {
+		if (drv->states[i].enter == NULL) {
+			if (mach_cpuidle == NULL) {
+				pr_err("%s: 'enter' function pointer NULL\n",
+				__func__);
+				return -EINVAL;
+			} else
+				drv->states[i].enter = arm_enter_idle;
+		}
+	}
+
+	if (cpuidle_register_driver(drv)) {
+		pr_err("%s: Failed to register cpuidle driver\n", __func__);
+		return -ENODEV;
+	}
+
+	arm_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+	if (arm_cpuidle_devices == NULL) {
+		cpuidle_unregister_driver(drv);
+		return -ENOMEM;
+	}
+
+	/* initialize state data for each cpuidle_device */
+	for_each_possible_cpu(cpu_id) {
+
+		dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
+		dev->cpu = cpu_id;
+		dev->state_count = drv->state_count;
+
+		if (driver_data)
+			for (i = 0; i < dev->state_count; i++) {
+				dev->states_usage[i].driver_data =
+							driver_data[i];
+			}
+
+		if (cpuidle_register_device(dev)) {
+			pr_err("%s: Failed to register cpu %u\n",
+				__func__, cpu_id);
+			return -ENODEV;
+		}
+	}
+
+	return 0;
+}
diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
new file mode 100644
index 0000000..86faa74
--- /dev/null
+++ b/arch/arm/include/asm/cpuidle.h
@@ -0,0 +1,25 @@
+/*
+ * 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
+ */
+
+#ifndef __ARCH_ARM_ASM_CPUIDLE_H__
+#define __ARCH_ARM_ASM_CPUIDLE_H__
+
+#include <linux/cpuidle.h>
+
+extern int arm_cpuidle_init(struct cpuidle_driver *drv,
+	int (*common_enter)(struct cpuidle_device *dev,
+		struct cpuidle_driver *drv, int index),
+	void *driver_data[]);
+
+extern void arm_cpuidle_devices_uninit(void);
+
+#endif /* __ARCH_ARM_ASM_CPUIDLE_H__ */
-- 
1.7.1

[RFC PATCH 2/8] ARM: at91: Replace init with new common ARM cpuidle init code

[Robert Lee]

Replace duplicated cpuidle init functionality with common ARM cpuidle
init implementation.

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

diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index a851e6c..bd0bbfc 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
@@ -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,19 +15,32 @@
 #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 <asm/cpuidle.h>
 #include "pm.h"
 
-#define AT91_MAX_STATES	2
-
-static DEFINE_PER_CPU(struct cpuidle_device, at91_cpuidle_device);
+#define AT91_MAX_STATES 2
 
 static struct cpuidle_driver at91_idle_driver = {
 	.name =         "at91_idle",
 	.owner =        THIS_MODULE,
+	.states[0] = {
+		.exit_latency		= 1,
+		.target_residency	= 100000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "WFI",
+		.desc			= "Wait for interrupt",
+	},
+	.states[1] = {
+		.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,
 };
 
 /* Actual code that puts the SoC in different idle states */
@@ -37,12 +48,8 @@ static int at91_enter_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
 			       int index)
 {
-	struct timeval before, after;
-	int idle_time;
 	u32 saved_lpr;
 
-	local_irq_disable();
-	do_gettimeofday(&before);
 	if (index == 0)
 		/* Wait for interrupt state */
 		cpu_do_idle();
@@ -53,48 +60,19 @@ static int at91_enter_idle(struct cpuidle_device *dev,
 		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);
 
-	dev->last_residency = idle_time;
 	return index;
 }
 
 /* Initialize CPU idle by registering the idle states */
 static int at91_init_cpuidle(void)
 {
-	struct cpuidle_device *device;
-	struct cpuidle_driver *driver = &at91_idle_driver;
+	int ret;
 
-	device = &per_cpu(at91_cpuidle_device, smp_processor_id());
-	device->state_count = AT91_MAX_STATES;
-	driver->state_count = AT91_MAX_STATES;
+	ret = arm_cpuidle_init(&at91_idle_driver,
+					at91_enter_idle,
+					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");
-	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;
 }
-
 device_initcall(at91_init_cpuidle);
-- 
1.7.1

[RFC PATCH 3/8] ARM: kirkwood: Replace init with new common ARM cpuidle init code

[Robert Lee]

Replace duplicated cpuidle init functionality with common ARM cpuidle
init implementation.

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

diff --git a/arch/arm/mach-kirkwood/cpuidle.c b/arch/arm/mach-kirkwood/cpuidle.c
index 7088180..6913ffd 100644
--- a/arch/arm/mach-kirkwood/cpuidle.c
+++ b/arch/arm/mach-kirkwood/cpuidle.c
@@ -20,6 +20,7 @@
 #include <linux/io.h>
 #include <linux/export.h>
 #include <asm/proc-fns.h>
+#include <asm/cpuidle.h>
 #include <mach/kirkwood.h>
 
 #define KIRKWOOD_MAX_STATES	2
@@ -27,20 +28,28 @@
 static struct cpuidle_driver kirkwood_idle_driver = {
 	.name =         "kirkwood_idle",
 	.owner =        THIS_MODULE,
+	.states[0] = {
+		.exit_latency		= 1,
+		.target_residency	= 100000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "WFI",
+		.desc			= "Wait for interrupt",
+	},
+	.states[1] = {
+		.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,
 };
 
-static DEFINE_PER_CPU(struct cpuidle_device, kirkwood_cpuidle_device);
-
 /* Actual code that puts the SoC in different idle states */
 static int kirkwood_enter_idle(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
 			       int index)
 {
-	struct timeval before, after;
-	int idle_time;
-
-	local_irq_disable();
-	do_gettimeofday(&before);
 	if (index == 0)
 		/* Wait for interrupt state */
 		cpu_do_idle();
@@ -55,13 +64,6 @@ static int kirkwood_enter_idle(struct cpuidle_device *dev,
 		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;
 
 	return index;
 }
@@ -69,35 +71,12 @@ static int kirkwood_enter_idle(struct cpuidle_device *dev,
 /* Initialize CPU idle by registering the idle states */
 static int kirkwood_init_cpuidle(void)
 {
-	struct cpuidle_device *device;
-	struct cpuidle_driver *driver = &kirkwood_idle_driver;
-
-	device = &per_cpu(kirkwood_cpuidle_device, smp_processor_id());
-	device->state_count = KIRKWOOD_MAX_STATES;
-	driver->state_count = KIRKWOOD_MAX_STATES;
+	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 = arm_cpuidle_init(&kirkwood_idle_driver,
+					kirkwood_enter_idle,
+					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);
-- 
1.7.1

[RFC PATCH 4/8] ARM: exynos: Replace init with new common ARM cpuidle init code

[Robert Lee]

Replace duplicated cpuidle init functionality with common ARM cpuidle
init implementation.

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

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index 4ebb382..a21b7da 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -13,80 +13,38 @@
 #include <linux/cpuidle.h>
 #include <linux/io.h>
 #include <linux/export.h>
-#include <linux/time.h>
-
 #include <asm/proc-fns.h>
+#include <asm/cpuidle.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 = {
+	.name		= "exynos4_idle",
+	.owner		= THIS_MODULE,
+	.states[0] = {
 		.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 = {
-	.name		= "exynos4_idle",
-	.owner		= THIS_MODULE,
+	.state_count = 1,
 };
 
 static int exynos4_enter_idle(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
+			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;
 
-		if (cpuidle_register_device(device)) {
-			printk(KERN_ERR "CPUidle register device failed\n,");
-			return -EIO;
-		}
-	}
-	return 0;
+	ret = arm_cpuidle_init(&exynos4_idle_driver,
+					exynos4_enter_idle,
+					NULL);
+	return ret;
 }
 device_initcall(exynos4_init_cpuidle);
-- 
1.7.1

[RFC PATCH 5/8] ARM: davinci: Replace init with new common ARM cpuidle init code

[Robert Lee]

Replace duplicated cpuidle init functionality with common ARM cpuidle
init implementation.

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

diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
index a30c7c5..d154c56 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
@@ -18,7 +18,7 @@
 #include <linux/io.h>
 #include <linux/export.h>
 #include <asm/proc-fns.h>
-
+#include <asm/cpuidle.h>
 #include <mach/cpuidle.h>
 #include <mach/ddr2.h>
 
@@ -36,9 +36,23 @@ struct davinci_ops {
 static struct cpuidle_driver davinci_idle_driver = {
 	.name	= "cpuidle-davinci",
 	.owner	= THIS_MODULE,
+	.states[0] = {
+		.exit_latency		= 1,
+		.target_residency	= 100000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "WFI",
+		.desc			= "Wait for interrupt",
+	},
+	.states[1] = {
+		.exit_latency		= 10,
+		.target_residency	= 100000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "DDR SR",
+		.desc			= "WFI and DDR Self Refresh",
+	},
+	.state_count = DAVINCI_CPUIDLE_MAX_STATES,
 };
 
-static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device);
 static void __iomem *ddr2_reg_base;
 
 static void davinci_save_ddr_power(int enter, bool pdown)
@@ -77,6 +91,8 @@ static struct davinci_ops davinci_states[DAVINCI_CPUIDLE_MAX_STATES] = {
 	},
 };
 
+void *states_usage_driver_data[DAVINCI_CPUIDLE_MAX_STATES] __initdata;
+
 /* Actual code that puts the SoC in different idle states */
 static int davinci_enter_idle(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
@@ -84,11 +100,6 @@ static int davinci_enter_idle(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);
@@ -97,25 +108,14 @@ static int davinci_enter_idle(struct cpuidle_device *dev,
 	if (ops && ops->exit)
 		ops->exit(ops->flags);
 
-	do_gettimeofday(&after);
-	local_irq_enable();
-	idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
-			(after.tv_usec - before.tv_usec);
-
-	dev->last_residency = idle_time;
-
 	return index;
 }
 
 static int __init davinci_cpuidle_probe(struct platform_device *pdev)
 {
 	int ret;
-	struct cpuidle_device *device;
-	struct cpuidle_driver *driver = &davinci_idle_driver;
 	struct davinci_cpuidle_config *pdata = pdev->dev.platform_data;
 
-	device = &per_cpu(davinci_cpuidle_device, smp_processor_id());
-
 	if (!pdata) {
 		dev_err(&pdev->dev, "cannot get platform data\n");
 		return -ENOENT;
@@ -123,42 +123,16 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev)
 
 	ddr2_reg_base = pdata->ddr2_ctlr_base;
 
-	/* Wait for interrupt state */
-	driver->states[0].enter = davinci_enter_idle;
-	driver->states[0].exit_latency = 1;
-	driver->states[0].target_residency = 10000;
-	driver->states[0].flags = CPUIDLE_FLAG_TIME_VALID;
-	strcpy(driver->states[0].name, "WFI");
-	strcpy(driver->states[0].desc, "Wait for interrupt");
-
-	/* Wait for interrupt and DDR self refresh state */
-	driver->states[1].enter = davinci_enter_idle;
-	driver->states[1].exit_latency = 10;
-	driver->states[1].target_residency = 10000;
-	driver->states[1].flags = CPUIDLE_FLAG_TIME_VALID;
-	strcpy(driver->states[1].name, "DDR SR");
-	strcpy(driver->states[1].desc, "WFI and DDR Self Refresh");
 	if (pdata->ddr2_pdown)
 		davinci_states[1].flags |= DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN;
-	cpuidle_set_statedata(&device->states_usage[1], &davinci_states[1]);
-
-	device->state_count = DAVINCI_CPUIDLE_MAX_STATES;
-	driver->state_count = DAVINCI_CPUIDLE_MAX_STATES;
 
-	ret = cpuidle_register_driver(&davinci_idle_driver);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register driver\n");
-		return ret;
-	}
+	states_usage_driver_data[1] = &davinci_states[1];
 
-	ret = cpuidle_register_device(device);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register device\n");
-		cpuidle_unregister_driver(&davinci_idle_driver);
-		return ret;
-	}
+	ret = arm_cpuidle_init(&davinci_idle_driver,
+				davinci_enter_idle,
+				states_usage_driver_data);
 
-	return 0;
+	return ret;
 }
 
 static struct platform_driver davinci_cpuidle_driver = {
@@ -174,4 +148,3 @@ static int __init davinci_cpuidle_init(void)
 						davinci_cpuidle_probe);
 }
 device_initcall(davinci_cpuidle_init);
-
-- 
1.7.1

[RFC PATCH 6/8] ARM: shmobile: Replace init with new common ARM cpuidle init code

[Robert Lee]

Replace duplicated cpuidle init functionality with common ARM cpuidle
init implementation.

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

diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c
index 1b23342..0314fa2 100644
--- a/arch/arm/mach-shmobile/cpuidle.c
+++ b/arch/arm/mach-shmobile/cpuidle.c
@@ -15,6 +15,7 @@
 #include <linux/err.h>
 #include <asm/system.h>
 #include <asm/io.h>
+#include <asm/cpuidle.h>
 
 static void shmobile_enter_wfi(void)
 {
@@ -29,25 +30,11 @@ 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,
@@ -66,21 +53,15 @@ void (*shmobile_cpuidle_setup)(struct cpuidle_driver *drv);
 
 static int shmobile_cpuidle_init(void)
 {
-	struct cpuidle_device *dev = &shmobile_cpuidle_dev;
-	struct cpuidle_driver *drv = &shmobile_cpuidle_driver;
-	int i;
-
-	for (i = 0; i < CPUIDLE_STATE_MAX; i++)
-		drv->states[i].enter = shmobile_cpuidle_enter;
+	int ret;
 
 	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 = arm_cpuidle_init(&shmobile_cpuidle_driver,
+					shmobile_cpuidle_enter,
+					NULL);
 
-	return 0;
+	return ret;
 }
 late_initcall(shmobile_cpuidle_init);
-- 
1.7.1

[RFC PATCH 7/8] ARM: imx: Add mx5 clock changes necessary for low power

[Robert Lee]

Add mx5 clock changes necessary to enter low power operating modes.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-mx5/clock-mx51-mx53.c |    3 +++
 1 files changed, 3 insertions(+), 0 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());
-- 
1.7.1

[RFC PATCH 8/8] ARM: imx: Add mx5 cpuidle implmentation

[Robert Lee]

Add mx5 cpuidle implmentation based upon the common ARM cpuidle
initialization code.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-mx5/Makefile  |    3 +-
 arch/arm/mach-mx5/cpuidle.c |   59 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 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..3584df5
--- /dev/null
+++ b/arch/arm/mach-mx5/cpuidle.c
@@ -0,0 +1,59 @@
+/*
+ * 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 <asm/proc-fns.h>
+#include <asm/cpuidle.h>
+#include <mach/common.h>
+
+static struct cpuidle_driver imx5_cpuidle_driver = {
+	.name	= "imx5_cpuidle",
+	.owner	= THIS_MODULE,
+	.states[0] = {
+		.exit_latency		= 12,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "WFI",
+		.desc			= "idle cpu powered, unclocked.",
+	},
+	.states[1] = {
+		.exit_latency		= 20,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "WFI SRPG",
+		.desc			= "idle cpu unpowered, unclocked.",
+	},
+	.state_count = 2,
+};
+
+int imx5_enter_idle(struct cpuidle_device *dev,
+			struct cpuidle_driver *drv,
+			      int index)
+{
+	if (likely(index))
+		mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
+	else
+		mx5_cpu_lp_set(WAIT_UNCLOCKED);
+	cpu_do_idle();
+	return index;
+}
+
+static int __init imx5_init_cpuidle(void)
+{
+	int ret;
+
+	ret = arm_cpuidle_init(&imx5_cpuidle_driver,
+					imx5_enter_idle,
+					NULL);
+	return ret;
+}
+late_initcall(imx5_init_cpuidle);
-- 
1.7.1

[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code

[Mark Brown]

On Mon, Dec 05, 2011 at 10:38:04PM -0600, Robert Lee wrote:

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

Apart from the IRQ disables this code isn't really ARM specific at all
and I bet it'd be useful on a lot of other architectures.

> +	time_start = ktime_get();

> +	index = mach_cpuidle(dev, drv, index);

Given the number of systems that at least start off with just a call to
cpu_do_idle() here shouldn't we have a defualt mach_cpu_idle() which
does that?  Currently the code requires the caller to provide one.

Please CC me on any iterations, I've got a S3C64xx implementation I'm
pushing which could probably use this.

[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code

[Rob Herring]

On 12/05/2011 10:38 PM, Robert Lee wrote:
> Add commonly used cpuidle init code to avoid unecessary duplication.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/common/Makefile       |    1 +
>  arch/arm/common/cpuidle.c      |  132 ++++++++++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/cpuidle.h |   25 ++++++++

Why not move cpuidle drivers to drivers/idle/ ?

>  3 files changed, 158 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/common/cpuidle.c
>  create mode 100644 arch/arm/include/asm/cpuidle.h
> 
> diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
> index 6ea9b6f..0865f69 100644
> --- a/arch/arm/common/Makefile
> +++ b/arch/arm/common/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_ARCH_IXP2000)	+= uengine.o
>  obj-$(CONFIG_ARCH_IXP23XX)	+= uengine.o
>  obj-$(CONFIG_PCI_HOST_ITE8152)  += it8152.o
>  obj-$(CONFIG_ARM_TIMER_SP804)	+= timer-sp.o
> +obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
> diff --git a/arch/arm/common/cpuidle.c b/arch/arm/common/cpuidle.c
> new file mode 100644
> index 0000000..e9a46a3
> --- /dev/null
> +++ b/arch/arm/common/cpuidle.c
> @@ -0,0 +1,132 @@
> +/*
> + * 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/cpuidle.h>
> +#include <asm/proc-fns.h>
> +
> +static struct cpuidle_device __percpu * arm_cpuidle_devices;
> +
> +static int (*mach_cpuidle)(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv,
> +				int index);
> +
> +static int arm_enter_idle(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)

I think how this works is backwards. This function is only called if
there is a NULL enter function for a state, but mach_cpuidle is global.
Ideally, I want to call this function for every state, but call a
different mach_cpuidle function for each state. Yes, you could select a
specific function for each state within the mach_cpuidle function, but
that seems silly to make the per state function global and then have to
select a per state function again. And if many users are doing that,
then it belongs in the common code.

> +{
> +	ktime_t time_start, time_end;
> +
> +	local_irq_disable();
> +	local_fiq_disable();
> +
> +	time_start = ktime_get();
> +
> +	index = mach_cpuidle(dev, drv, index);
> +
> +	time_end = ktime_get();
> +
> +	local_fiq_enable();
> +	local_irq_enable();
> +
> +	dev->last_residency =
> +		(int)ktime_to_us(ktime_sub(time_end, time_start));
> +
> +	return index;
> +}
> +
> +void arm_cpuidle_devices_uninit(void)
> +{
> +	int cpu_id;
> +	struct cpuidle_device *dev;
> +
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	free_percpu(arm_cpuidle_devices);
> +	return;

Don't need return statement.

> +}
> +
> +int __init arm_cpuidle_init(struct cpuidle_driver *drv,
> +	int (*common_enter)(struct cpuidle_device *dev,
> +		struct cpuidle_driver *drv, int index),
> +	void *driver_data[])
> +{
> +	struct cpuidle_device *dev;
> +	int i, cpu_id;
> +
> +	if (drv == NULL) {
> +		pr_err("%s: cpuidle_driver pointer NULL\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (drv->state_count > CPUIDLE_STATE_MAX)
> +		pr_err("%s: state count exceeds maximum\n", __func__);

return an error?

You can collapse these 2 parameter checks down to:

if (!drv || (drv->state_count > CPUIDLE_STATE_MAX))

> +
> +	mach_cpuidle = common_enter;
> +
> +	/* if state enter function not specified, use common_enter function */
> +	for (i = 0; i < drv->state_count; i++) {
> +		if (drv->states[i].enter == NULL) {
> +			if (mach_cpuidle == NULL) {

Usually !mach_cpuidle is preferred for NULL checks. You can move this
check out of the loop.

> +				pr_err("%s: 'enter' function pointer NULL\n",
> +				__func__);
> +				return -EINVAL;
> +			} else
> +				drv->states[i].enter = arm_enter_idle;
> +		}
> +	}
> +
> +	if (cpuidle_register_driver(drv)) {
> +		pr_err("%s: Failed to register cpuidle driver\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	arm_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (arm_cpuidle_devices == NULL) {
> +		cpuidle_unregister_driver(drv);
> +		return -ENOMEM;
> +	}
> +
> +	/* initialize state data for each cpuidle_device */
> +	for_each_possible_cpu(cpu_id) {
> +
> +		dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
> +		dev->cpu = cpu_id;
> +		dev->state_count = drv->state_count;
> +
> +		if (driver_data)
> +			for (i = 0; i < dev->state_count; i++) {

This would be more concise and less indentation:

for (i = 0; driver_data, i < dev->state_count; i++)

> +				dev->states_usage[i].driver_data =
> +							driver_data[i];
> +			}
> +
> +		if (cpuidle_register_device(dev)) {
> +			pr_err("%s: Failed to register cpu %u\n",
> +				__func__, cpu_id);
> +			return -ENODEV;

Leaking memory here from alloc_percpu.

Also, need to unregister driver. It's usually cleaner to use goto's for
error clean-up.

Rob

[RFC PATCH 0/8] Add common ARM cpuidle init code

[Shawn Guo]

On Mon, Dec 05, 2011 at 10:38:03PM -0600, Robert Lee wrote:
> The patchset adds some cpuidle initialization functionality commonly
> duplicated by many ARM platforms.  The duplicate cpuidle init code of the
> various ARM platforms has been modified to use this common code.
> The omap cpuidle initialization impelmentation doesn't quite fit into
> this common framework so it was left alone.
> 
> All the modified ARM platforms were built against the new code and
> tests were ran on a i.MX51 Babbage board using the new mx5 cpuidle
> implementation included in this patchset.
> 
> Questions: 
> 
> Is arch/arm/common/ the correct location for this code?
> 
With many drivers are being moved into drivers/* from arch/arm/mach-*
and arch/arm/plat-*, I'm wondering if we have any problem with doing
the same thing on cpuidle.  If we can move these cpuidle drivers into
drivers/cpuidle (or drivers/idle, I'm not sure why two folders), that
will be the best place for such consolidation to me.

Cc-ed linux-pm and Len Brown for helping understand.

Regards,
Shawn

> Should the mx5 implementation be moved to a separate patchset?
> 
> Robert Lee (8):
>   ARM: Add commonly used cpuidle init code
>   ARM: at91: Replace init with new common ARM cpuidle init code
>   ARM: kirkwood: Replace init with new common ARM cpuidle init code
>   ARM: exynos: Replace init with new common ARM cpuidle init code
>   ARM: davinci: Replace init with new common ARM cpuidle init code
>   ARM: shmobile: Replace init with new common ARM cpuidle init code
>   ARM: imx: Add mx5 clock changes necessary for low power
>   ARM: imx: Add mx5 cpuidle implmentation
> 
>  arch/arm/common/Makefile            |    1 +
>  arch/arm/common/cpuidle.c           |  132 +++++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/cpuidle.h      |   25 +++++++
>  arch/arm/mach-at91/cpuidle.c        |   68 ++++++------------
>  arch/arm/mach-davinci/cpuidle.c     |   73 ++++++-------------
>  arch/arm/mach-exynos/cpuidle.c      |   66 +++--------------
>  arch/arm/mach-kirkwood/cpuidle.c    |   63 ++++++-----------
>  arch/arm/mach-mx5/Makefile          |    3 +-
>  arch/arm/mach-mx5/clock-mx51-mx53.c |    3 +
>  arch/arm/mach-mx5/cpuidle.c         |   59 ++++++++++++++++
>  arch/arm/mach-shmobile/cpuidle.c    |   33 ++-------
>  11 files changed, 308 insertions(+), 218 deletions(-)
>  create mode 100644 arch/arm/common/cpuidle.c
>  create mode 100644 arch/arm/include/asm/cpuidle.h
>  create mode 100644 arch/arm/mach-mx5/cpuidle.c

[RFC PATCH 0/8] Add common ARM cpuidle init code

[Arnd Bergmann]

On Thursday 08 December 2011, Shawn Guo wrote:
> > 
> With many drivers are being moved into drivers/* from arch/arm/mach-*
> and arch/arm/plat-*, I'm wondering if we have any problem with doing
> the same thing on cpuidle.  If we can move these cpuidle drivers into
> drivers/cpuidle (or drivers/idle, I'm not sure why two folders), that
> will be the best place for such consolidation to me.
> 
> Cc-ed linux-pm and Len Brown for helping understand.

I think we should definitely move them into one of the two directories.
The one thing making this a bit nasty is that many SoC implementation
need to access some global registers for this and don't have a "device"
that can be used for cpuidle with its own register ranges, but it's
still better to have the code in a single place IMHO.

	Arnd

[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code

[Rob Lee]

On 6 December 2011 05:47, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Dec 05, 2011 at 10:38:04PM -0600, Robert Lee wrote:
>
>> +static int arm_enter_idle(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index)
>> +{
>> + ? ? ktime_t time_start, time_end;
>> +
>> + ? ? local_irq_disable();
>> + ? ? local_fiq_disable();
>
> Apart from the IRQ disables this code isn't really ARM specific at all
> and I bet it'd be useful on a lot of other architectures.
>

I agree and had considered this as well.  Can you (or anyone else)
suggest the best/most community friendly method of doing this?  If
this code is moved to drivers/idle or drivers/cpuidle, then perhaps
just making empty fiq enable/disable functions would be ok.

>> + ? ? time_start = ktime_get();
>
>> + ? ? index = mach_cpuidle(dev, drv, index);
>
> Given the number of systems that at least start off with just a call to
> cpu_do_idle() here shouldn't we have a defualt mach_cpu_idle() which
> does that? ?Currently the code requires the caller to provide one.
>

Good point.  I'll add this to v2.

> Please CC me on any iterations, I've got a S3C64xx implementation I'm
> pushing which could probably use this.

Will do Mark.  Thanks for your review and comments.

[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code

[Rob Lee]

Rob, thanks for your thorough review.  Comments and questions below.

On 6 December 2011 09:06, Rob Herring <robherring2@gmail.com> wrote:
> On 12/05/2011 10:38 PM, Robert Lee wrote:
>> Add commonly used cpuidle init code to avoid unecessary duplication.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>> ---
>> ?arch/arm/common/Makefile ? ? ? | ? ?1 +
>> ?arch/arm/common/cpuidle.c ? ? ?| ?132 ++++++++++++++++++++++++++++++++++++++++
>> ?arch/arm/include/asm/cpuidle.h | ? 25 ++++++++
>
> Why not move cpuidle drivers to drivers/idle/ ?
>

Or to drivers/cpuidle?  I am not sure the reasoning behind a
drivers/idle directory if everything there is a cpuidle driver
implementation.  I originally did locate this common cpuidle code in
drivers/idle/arm_idle.c and put the head file in arch/arm/include/asm
but this threw a checkpatch warning so I submitted with this placement
to start with.  If the local_fiq_enable/disable calls can be handled
in a community friendly way for any architecture, then perhaps I can
just move the header file code to linux/include/cpuidle.h.

Do you have suggestions about making this functionality available for
any architecture and what is the most community friendly method of
doing this?  I suppose function declarations for
local_fiq_enable/disable could be given.  Then, one could either
define them here as empty functions or could have two idle functions,
arm_enter_idle and nonarm_enter_idle and the architecture could be
read or passed in to determine which one to set the cpuidle states'
enter functions to.

>> ?3 files changed, 158 insertions(+), 0 deletions(-)
>> ?create mode 100644 arch/arm/common/cpuidle.c
>> ?create mode 100644 arch/arm/include/asm/cpuidle.h
>>
>> diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
>> index 6ea9b6f..0865f69 100644
>> --- a/arch/arm/common/Makefile
>> +++ b/arch/arm/common/Makefile
>> @@ -17,3 +17,4 @@ obj-$(CONFIG_ARCH_IXP2000) ?+= uengine.o
>> ?obj-$(CONFIG_ARCH_IXP23XX) ? += uengine.o
>> ?obj-$(CONFIG_PCI_HOST_ITE8152) ?+= it8152.o
>> ?obj-$(CONFIG_ARM_TIMER_SP804) ? ? ? ?+= timer-sp.o
>> +obj-$(CONFIG_CPU_IDLE) ? ? ? ? ? ? ? += cpuidle.o
>> diff --git a/arch/arm/common/cpuidle.c b/arch/arm/common/cpuidle.c
>> new file mode 100644
>> index 0000000..e9a46a3
>> --- /dev/null
>> +++ b/arch/arm/common/cpuidle.c
>> @@ -0,0 +1,132 @@
>> +/*
>> + * 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/cpuidle.h>
>> +#include <asm/proc-fns.h>
>> +
>> +static struct cpuidle_device __percpu * arm_cpuidle_devices;
>> +
>> +static int (*mach_cpuidle)(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? int index);
>> +
>> +static int arm_enter_idle(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index)
>
> I think how this works is backwards. This function is only called if
> there is a NULL enter function for a state, but mach_cpuidle is global.
> Ideally, I want to call this function for every state, but call a
> different mach_cpuidle function for each state. Yes, you could select a
> specific function for each state within the mach_cpuidle function, but
> that seems silly to make the per state function global and then have to
> select a per state function again. And if many users are doing that,
> then it belongs in the common code.
>
>> +{
>> + ? ? ktime_t time_start, time_end;
>> +
>> + ? ? local_irq_disable();
>> + ? ? local_fiq_disable();
>> +
>> + ? ? time_start = ktime_get();
>> +
>> + ? ? index = mach_cpuidle(dev, drv, index);
>> +
>> + ? ? time_end = ktime_get();
>> +
>> + ? ? local_fiq_enable();
>> + ? ? local_irq_enable();
>> +
>> + ? ? dev->last_residency =
>> + ? ? ? ? ? ? (int)ktime_to_us(ktime_sub(time_end, time_start));
>> +
>> + ? ? return index;
>> +}
>> +
>> +void arm_cpuidle_devices_uninit(void)
>> +{
>> + ? ? int cpu_id;
>> + ? ? struct cpuidle_device *dev;
>> +
>> + ? ? for_each_possible_cpu(cpu_id) {
>> + ? ? ? ? ? ? dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
>> + ? ? ? ? ? ? cpuidle_unregister_device(dev);
>> + ? ? }
>> +
>> + ? ? free_percpu(arm_cpuidle_devices);
>> + ? ? return;
>
> Don't need return statement.
>
>> +}
>> +
>> +int __init arm_cpuidle_init(struct cpuidle_driver *drv,
>> + ? ? int (*common_enter)(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? struct cpuidle_driver *drv, int index),
>> + ? ? void *driver_data[])
>> +{
>> + ? ? struct cpuidle_device *dev;
>> + ? ? int i, cpu_id;
>> +
>> + ? ? if (drv == NULL) {
>> + ? ? ? ? ? ? pr_err("%s: cpuidle_driver pointer NULL\n", __func__);
>> + ? ? ? ? ? ? return -EINVAL;
>> + ? ? }
>> +
>> + ? ? if (drv->state_count > CPUIDLE_STATE_MAX)
>> + ? ? ? ? ? ? pr_err("%s: state count exceeds maximum\n", __func__);
>
> return an error?
>
> You can collapse these 2 parameter checks down to:
>
> if (!drv || (drv->state_count > CPUIDLE_STATE_MAX))
>
>> +
>> + ? ? mach_cpuidle = common_enter;
>> +
>> + ? ? /* if state enter function not specified, use common_enter function */
>> + ? ? for (i = 0; i < drv->state_count; i++) {
>> + ? ? ? ? ? ? if (drv->states[i].enter == NULL) {
>> + ? ? ? ? ? ? ? ? ? ? if (mach_cpuidle == NULL) {
>
> Usually !mach_cpuidle is preferred for NULL checks. You can move this
> check out of the loop.
>
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? pr_err("%s: 'enter' function pointer NULL\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? ? ? ? ? ? ? ? } else
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? drv->states[i].enter = arm_enter_idle;
>> + ? ? ? ? ? ? }
>> + ? ? }
>> +
>> + ? ? if (cpuidle_register_driver(drv)) {
>> + ? ? ? ? ? ? pr_err("%s: Failed to register cpuidle driver\n", __func__);
>> + ? ? ? ? ? ? return -ENODEV;
>> + ? ? }
>> +
>> + ? ? arm_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>> + ? ? if (arm_cpuidle_devices == NULL) {
>> + ? ? ? ? ? ? cpuidle_unregister_driver(drv);
>> + ? ? ? ? ? ? return -ENOMEM;
>> + ? ? }
>> +
>> + ? ? /* initialize state data for each cpuidle_device */
>> + ? ? for_each_possible_cpu(cpu_id) {
>> +
>> + ? ? ? ? ? ? dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
>> + ? ? ? ? ? ? dev->cpu = cpu_id;
>> + ? ? ? ? ? ? dev->state_count = drv->state_count;
>> +
>> + ? ? ? ? ? ? if (driver_data)
>> + ? ? ? ? ? ? ? ? ? ? for (i = 0; i < dev->state_count; i++) {
>
> This would be more concise and less indentation:
>
> for (i = 0; driver_data, i < dev->state_count; i++)
>
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev->states_usage[i].driver_data =
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? driver_data[i];
>> + ? ? ? ? ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? if (cpuidle_register_device(dev)) {
>> + ? ? ? ? ? ? ? ? ? ? pr_err("%s: Failed to register cpu %u\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, cpu_id);

arm_cpuidle_devices_uninit();

>> + ? ? ? ? ? ? ? ? ? ? return -ENODEV;
>
> Leaking memory here from alloc_percpu.
>
> Also, need to unregister driver. It's usually cleaner to use goto's for
> error clean-up.
>

In this case, just adding a call  to arm_cpuidle_devices_uninit()
seems clean right?

> Rob

[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code

[Mark Brown]

On Thu, Dec 08, 2011 at 11:34:34AM -0600, Rob Lee wrote:

> I agree and had considered this as well.  Can you (or anyone else)
> suggest the best/most community friendly method of doing this?  If
> this code is moved to drivers/idle or drivers/cpuidle, then perhaps
> just making empty fiq enable/disable functions would be ok.

Sounds like a reasonable plan, or providing an arch hook mechanism as
well as a SoC hook mechanism and allowing them to use that.

[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code

[Rob Herring]

On 12/08/2011 03:46 PM, Rob Lee wrote:
> Rob, thanks for your thorough review.  Comments and questions below.
> 
> On 6 December 2011 09:06, Rob Herring <robherring2@gmail.com> wrote:
>> On 12/05/2011 10:38 PM, Robert Lee wrote:
>>> Add commonly used cpuidle init code to avoid unecessary duplication.
>>>
>>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>>> ---
>>>  arch/arm/common/Makefile       |    1 +
>>>  arch/arm/common/cpuidle.c      |  132 ++++++++++++++++++++++++++++++++++++++++
>>>  arch/arm/include/asm/cpuidle.h |   25 ++++++++
>>
>> Why not move cpuidle drivers to drivers/idle/ ?
>>
> 
> Or to drivers/cpuidle?  I am not sure the reasoning behind a

It would make sense to me that they should be combined. I'm not sure of
the history here.

> drivers/idle directory if everything there is a cpuidle driver
> implementation.  I originally did locate this common cpuidle code in
> drivers/idle/arm_idle.c and put the head file in arch/arm/include/asm
> but this threw a checkpatch warning so I submitted with this placement

What warning?

> to start with.  If the local_fiq_enable/disable calls can be handled
> in a community friendly way for any architecture, then perhaps I can
> just move the header file code to linux/include/cpuidle.h.

Maybe we should think about whether we even need to disable fiq. You
probably don't use low latency interrupt and a high latency low power
mode together.

> Do you have suggestions about making this functionality available for
> any architecture and what is the most community friendly method of
> doing this?  I suppose function declarations for
> local_fiq_enable/disable could be given.  Then, one could either
> define them here as empty functions or could have two idle functions,
> arm_enter_idle and nonarm_enter_idle and the architecture could be
> read or passed in to determine which one to set the cpuidle states'
> enter functions to.

I'm not sure I understand the issue. You can include asm headers and
make things depend on CONFIG_ARM so no other arch builds code with
local_fiq_enable.

The other approach would be to define arch specific cpu_idle functions
for pre and post idle.

>>> +static int (*mach_cpuidle)(struct cpuidle_device *dev,
>>> +                            struct cpuidle_driver *drv,
>>> +                             int index);
>>> +
>>> +static int arm_enter_idle(struct cpuidle_device *dev,
>>> +                            struct cpuidle_driver *drv, int index)
>>
>> I think how this works is backwards. This function is only called if
>> there is a NULL enter function for a state, but mach_cpuidle is global.
>> Ideally, I want to call this function for every state, but call a
>> different mach_cpuidle function for each state. Yes, you could select a
>> specific function for each state within the mach_cpuidle function, but
>> that seems silly to make the per state function global and then have to
>> select a per state function again. And if many users are doing that,
>> then it belongs in the common code.


No comments on this!?


>>> +{
>>> +     ktime_t time_start, time_end;
>>> +
>>> +     local_irq_disable();
>>> +     local_fiq_disable();
>>> +
>>> +     time_start = ktime_get();
>>> +
>>> +     index = mach_cpuidle(dev, drv, index);
>>> +
>>> +     time_end = ktime_get();
>>> +
>>> +     local_fiq_enable();
>>> +     local_irq_enable();
>>> +
>>> +     dev->last_residency =
>>> +             (int)ktime_to_us(ktime_sub(time_end, time_start));
>>> +
>>> +     return index;
>>> +}
>>> +
>>> +void arm_cpuidle_devices_uninit(void)
>>> +{
>>> +     int cpu_id;
>>> +     struct cpuidle_device *dev;
>>> +
>>> +     for_each_possible_cpu(cpu_id) {
>>> +             dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
>>> +             cpuidle_unregister_device(dev);
>>> +     }
>>> +
>>> +     free_percpu(arm_cpuidle_devices);
>>> +     return;
>>
>> Don't need return statement.
>>
>>> +}
>>> +
>>> +int __init arm_cpuidle_init(struct cpuidle_driver *drv,
>>> +     int (*common_enter)(struct cpuidle_device *dev,
>>> +             struct cpuidle_driver *drv, int index),
>>> +     void *driver_data[])
>>> +{
>>> +     struct cpuidle_device *dev;
>>> +     int i, cpu_id;
>>> +
>>> +     if (drv == NULL) {
>>> +             pr_err("%s: cpuidle_driver pointer NULL\n", __func__);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     if (drv->state_count > CPUIDLE_STATE_MAX)
>>> +             pr_err("%s: state count exceeds maximum\n", __func__);
>>
>> return an error?
>>
>> You can collapse these 2 parameter checks down to:
>>
>> if (!drv || (drv->state_count > CPUIDLE_STATE_MAX))
>>
>>> +
>>> +     mach_cpuidle = common_enter;
>>> +
>>> +     /* if state enter function not specified, use common_enter function */
>>> +     for (i = 0; i < drv->state_count; i++) {
>>> +             if (drv->states[i].enter == NULL) {
>>> +                     if (mach_cpuidle == NULL) {
>>
>> Usually !mach_cpuidle is preferred for NULL checks. You can move this
>> check out of the loop.
>>
>>> +                             pr_err("%s: 'enter' function pointer NULL\n",
>>> +                             __func__);
>>> +                             return -EINVAL;
>>> +                     } else
>>> +                             drv->states[i].enter = arm_enter_idle;
>>> +             }
>>> +     }
>>> +
>>> +     if (cpuidle_register_driver(drv)) {
>>> +             pr_err("%s: Failed to register cpuidle driver\n", __func__);
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     arm_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>>> +     if (arm_cpuidle_devices == NULL) {
>>> +             cpuidle_unregister_driver(drv);
>>> +             return -ENOMEM;
>>> +     }
>>> +
>>> +     /* initialize state data for each cpuidle_device */
>>> +     for_each_possible_cpu(cpu_id) {
>>> +
>>> +             dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
>>> +             dev->cpu = cpu_id;
>>> +             dev->state_count = drv->state_count;
>>> +
>>> +             if (driver_data)
>>> +                     for (i = 0; i < dev->state_count; i++) {
>>
>> This would be more concise and less indentation:
>>
>> for (i = 0; driver_data, i < dev->state_count; i++)
>>
>>> +                             dev->states_usage[i].driver_data =
>>> +                                                     driver_data[i];
>>> +                     }
>>> +
>>> +             if (cpuidle_register_device(dev)) {
>>> +                     pr_err("%s: Failed to register cpu %u\n",
>>> +                             __func__, cpu_id);
> 
> arm_cpuidle_devices_uninit();
> 
>>> +                     return -ENODEV;
>>
>> Leaking memory here from alloc_percpu.
>>
>> Also, need to unregister driver. It's usually cleaner to use goto's for
>> error clean-up.
>>
> 
> In this case, just adding a call  to arm_cpuidle_devices_uninit()
> seems clean right?
> 
Really you should only undo what you have setup. In most cases uninit
functions just uninit everything unconditionally. This gets a bit messy
when you have loops that can fail.

arm_cpuidle_devices_uninit is not unregistering the driver, so that
needs fixing as well.

Rob

[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code

[Rob Lee]

On 9 December 2011 07:54, Rob Herring <robherring2@gmail.com> wrote:
> On 12/08/2011 03:46 PM, Rob Lee wrote:
>> Rob, thanks for your thorough review. ?Comments and questions below.
>>
>> On 6 December 2011 09:06, Rob Herring <robherring2@gmail.com> wrote:
>>> On 12/05/2011 10:38 PM, Robert Lee wrote:
>>>> Add commonly used cpuidle init code to avoid unecessary duplication.
>>>>
>>>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>>>> ---
>>>> ?arch/arm/common/Makefile ? ? ? | ? ?1 +
>>>> ?arch/arm/common/cpuidle.c ? ? ?| ?132 ++++++++++++++++++++++++++++++++++++++++
>>>> ?arch/arm/include/asm/cpuidle.h | ? 25 ++++++++
>>>
>>> Why not move cpuidle drivers to drivers/idle/ ?
>>>
>>
>> Or to drivers/cpuidle? ?I am not sure the reasoning behind a
>
> It would make sense to me that they should be combined. I'm not sure of
> the history here.
>
>> drivers/idle directory if everything there is a cpuidle driver
>> implementation. ?I originally did locate this common cpuidle code in
>> drivers/idle/arm_idle.c and put the head file in arch/arm/include/asm
>> but this threw a checkpatch warning so I submitted with this placement
>
> What warning?
>

I'll move things back to drivers/idle and try it again  and send the
specific warning later today.

>> to start with. ?If the local_fiq_enable/disable calls can be handled
>> in a community friendly way for any architecture, then perhaps I can
>> just move the header file code to linux/include/cpuidle.h.
>
> Maybe we should think about whether we even need to disable fiq. You
> probably don't use low latency interrupt and a high latency low power
> mode together.
>
>> Do you have suggestions about making this functionality available for
>> any architecture and what is the most community friendly method of
>> doing this? ?I suppose function declarations for
>> local_fiq_enable/disable could be given. ?Then, one could either
>> define them here as empty functions or could have two idle functions,
>> arm_enter_idle and nonarm_enter_idle and the architecture could be
>> read or passed in to determine which one to set the cpuidle states'
>> enter functions to.
>
> I'm not sure I understand the issue. You can include asm headers and
> make things depend on CONFIG_ARM so no other arch builds code with
> local_fiq_enable.
>
> The other approach would be to define arch specific cpu_idle functions
> for pre and post idle.
>
>>>> +static int (*mach_cpuidle)(struct cpuidle_device *dev,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? int index);
>>>> +
>>>> +static int arm_enter_idle(struct cpuidle_device *dev,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_driver *drv, int index)
>>>
>>> I think how this works is backwards. This function is only called if
>>> there is a NULL enter function for a state, but mach_cpuidle is global.
>>> Ideally, I want to call this function for every state, but call a
>>> different mach_cpuidle function for each state. Yes, you could select a
>>> specific function for each state within the mach_cpuidle function, but
>>> that seems silly to make the per state function global and then have to
>>> select a per state function again. And if many users are doing that,
>>> then it belongs in the common code.
>
>
> No comments on this!?
>
>

I agree with your viewpoint and will fix this in v2.  The same goes
for all of your other comments that I made no response to.  Should I
post my proposed fix here before submitting v2?

>>>> +{
>>>> + ? ? ktime_t time_start, time_end;
>>>> +
>>>> + ? ? local_irq_disable();
>>>> + ? ? local_fiq_disable();
>>>> +
>>>> + ? ? time_start = ktime_get();
>>>> +
>>>> + ? ? index = mach_cpuidle(dev, drv, index);
>>>> +
>>>> + ? ? time_end = ktime_get();
>>>> +
>>>> + ? ? local_fiq_enable();
>>>> + ? ? local_irq_enable();
>>>> +
>>>> + ? ? dev->last_residency =
>>>> + ? ? ? ? ? ? (int)ktime_to_us(ktime_sub(time_end, time_start));
>>>> +
>>>> + ? ? return index;
>>>> +}
>>>> +
>>>> +void arm_cpuidle_devices_uninit(void)
>>>> +{
>>>> + ? ? int cpu_id;
>>>> + ? ? struct cpuidle_device *dev;
>>>> +
>>>> + ? ? for_each_possible_cpu(cpu_id) {
>>>> + ? ? ? ? ? ? dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
>>>> + ? ? ? ? ? ? cpuidle_unregister_device(dev);
>>>> + ? ? }
>>>> +
>>>> + ? ? free_percpu(arm_cpuidle_devices);
>>>> + ? ? return;
>>>
>>> Don't need return statement.
>>>
>>>> +}
>>>> +
>>>> +int __init arm_cpuidle_init(struct cpuidle_driver *drv,
>>>> + ? ? int (*common_enter)(struct cpuidle_device *dev,
>>>> + ? ? ? ? ? ? struct cpuidle_driver *drv, int index),
>>>> + ? ? void *driver_data[])
>>>> +{
>>>> + ? ? struct cpuidle_device *dev;
>>>> + ? ? int i, cpu_id;
>>>> +
>>>> + ? ? if (drv == NULL) {
>>>> + ? ? ? ? ? ? pr_err("%s: cpuidle_driver pointer NULL\n", __func__);
>>>> + ? ? ? ? ? ? return -EINVAL;
>>>> + ? ? }
>>>> +
>>>> + ? ? if (drv->state_count > CPUIDLE_STATE_MAX)
>>>> + ? ? ? ? ? ? pr_err("%s: state count exceeds maximum\n", __func__);
>>>
>>> return an error?
>>>
>>> You can collapse these 2 parameter checks down to:
>>>
>>> if (!drv || (drv->state_count > CPUIDLE_STATE_MAX))
>>>
>>>> +
>>>> + ? ? mach_cpuidle = common_enter;
>>>> +
>>>> + ? ? /* if state enter function not specified, use common_enter function */
>>>> + ? ? for (i = 0; i < drv->state_count; i++) {
>>>> + ? ? ? ? ? ? if (drv->states[i].enter == NULL) {
>>>> + ? ? ? ? ? ? ? ? ? ? if (mach_cpuidle == NULL) {
>>>
>>> Usually !mach_cpuidle is preferred for NULL checks. You can move this
>>> check out of the loop.
>>>
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? pr_err("%s: 'enter' function pointer NULL\n",
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__);
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>>>> + ? ? ? ? ? ? ? ? ? ? } else
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? drv->states[i].enter = arm_enter_idle;
>>>> + ? ? ? ? ? ? }
>>>> + ? ? }
>>>> +
>>>> + ? ? if (cpuidle_register_driver(drv)) {
>>>> + ? ? ? ? ? ? pr_err("%s: Failed to register cpuidle driver\n", __func__);
>>>> + ? ? ? ? ? ? return -ENODEV;
>>>> + ? ? }
>>>> +
>>>> + ? ? arm_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>>>> + ? ? if (arm_cpuidle_devices == NULL) {
>>>> + ? ? ? ? ? ? cpuidle_unregister_driver(drv);
>>>> + ? ? ? ? ? ? return -ENOMEM;
>>>> + ? ? }
>>>> +
>>>> + ? ? /* initialize state data for each cpuidle_device */
>>>> + ? ? for_each_possible_cpu(cpu_id) {
>>>> +
>>>> + ? ? ? ? ? ? dev = per_cpu_ptr(arm_cpuidle_devices, cpu_id);
>>>> + ? ? ? ? ? ? dev->cpu = cpu_id;
>>>> + ? ? ? ? ? ? dev->state_count = drv->state_count;
>>>> +
>>>> + ? ? ? ? ? ? if (driver_data)
>>>> + ? ? ? ? ? ? ? ? ? ? for (i = 0; i < dev->state_count; i++) {
>>>
>>> This would be more concise and less indentation:
>>>
>>> for (i = 0; driver_data, i < dev->state_count; i++)
>>>
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev->states_usage[i].driver_data =
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? driver_data[i];
>>>> + ? ? ? ? ? ? ? ? ? ? }
>>>> +
>>>> + ? ? ? ? ? ? if (cpuidle_register_device(dev)) {
>>>> + ? ? ? ? ? ? ? ? ? ? pr_err("%s: Failed to register cpu %u\n",
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, cpu_id);
>>
>> arm_cpuidle_devices_uninit();
>>
>>>> + ? ? ? ? ? ? ? ? ? ? return -ENODEV;
>>>
>>> Leaking memory here from alloc_percpu.
>>>
>>> Also, need to unregister driver. It's usually cleaner to use goto's for
>>> error clean-up.
>>>
>>
>> In this case, just adding a call ?to arm_cpuidle_devices_uninit()
>> seems clean right?
>>
> Really you should only undo what you have setup. In most cases uninit
> functions just uninit everything unconditionally. This gets a bit messy
> when you have loops that can fail.
>
> arm_cpuidle_devices_uninit is not unregistering the driver, so that
> needs fixing as well.
>
> Rob

[RFC PATCH 1/8] ARM: Add commonly used cpuidle init code

[Nicolas Pitre]

On Fri, 9 Dec 2011, Rob Herring wrote:

> On 12/08/2011 03:46 PM, Rob Lee wrote:
> > to start with.  If the local_fiq_enable/disable calls can be handled
> > in a community friendly way for any architecture, then perhaps I can
> > just move the header file code to linux/include/cpuidle.h.
> 
> Maybe we should think about whether we even need to disable fiq. You
> probably don't use low latency interrupt and a high latency low power
> mode together.

Agreed.  FIQs should be invisible and normally not be interfered with by 
generic kernel code.  If some CPU specific special low power mode really 
needs FIQs to be masked out, then that should be done by the code 
dealing with that mode.  And that is valid only if you do make use of 
FIQs in the first place.


Nicolas

[RFC PATCH 0/8] Add common ARM cpuidle init code

[Daniel Lezcano]

On 12/08/2011 04:37 PM, Arnd Bergmann wrote:
> On Thursday 08 December 2011, Shawn Guo wrote:
>>>
>> With many drivers are being moved into drivers/* from arch/arm/mach-*
>> and arch/arm/plat-*, I'm wondering if we have any problem with doing
>> the same thing on cpuidle.  If we can move these cpuidle drivers into
>> drivers/cpuidle (or drivers/idle, I'm not sure why two folders), that
>> will be the best place for such consolidation to me.
>>
>> Cc-ed linux-pm and Len Brown for helping understand.
>
> I think we should definitely move them into one of the two directories.
> The one thing making this a bit nasty is that many SoC implementation
> need to access some global registers for this and don't have a "device"
> that can be used for cpuidle with its own register ranges, but it's
> still better to have the code in a single place IMHO.

Yes, I agree. Moreover that will facilitate to factor out the code which 
is spread all around the different architecture.

The places where there is the drivers cpuidle code are at drivers/idle, 
driver/acpi, arch/arm, arch/sh/kernel/cpu/mobile/cpuidle, etc ...

IMHO, before doing some code factoring out, we should move all these 
drivers to the cpuidle directory, no ?

drivers/idle/intel_idle.c => drivers/cpuidle/intel.c
drivers/acpi/processor_idle.c => drivers/cpuidle/acpi.c

arch/arm/mach-omap2/cpuidle34xx.c => drivers/cpuidle/omap-34xx.c
arch/arm/mach-omap2/cpuidle44xx.c => drivers/cpuidle/omap-44xx.c
arch/arm/mach-shmobile/cpuidle.c  => drivers/cpuidle/arm-shmobile.c
arch/arm/mach-davinci/cpuidle.c => drivers/cpuidle/davinci.c
arch/arm/mach-at91/cpuidle.c => drivers/cpuidle/at91.c
arch/arm/mach-s3c64xx/cpuidle.c => drivers/cpuidle/s3c64xx.c
arch/arm/mach-exynos/cpuidle.c => drivers/cpuidle/exynos.c
arch/arm/mach-kirkwood/cpuidle.c => drivers/cpuidle/kirkwood.c
arch/arcm/mach-msm/idle.S => drivers/cpuidle/msm.S

That could be a first step and then we move the other archs which could 
be a bit more tricky like the powerpc.

Does it make sense ?

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

[RFC PATCH 0/8] Add common ARM cpuidle init code

[Arnd Bergmann]

On Tuesday 03 January 2012, Daniel Lezcano wrote:
> drivers/idle/intel_idle.c => drivers/cpuidle/intel.c
> drivers/acpi/processor_idle.c => drivers/cpuidle/acpi.c
> 
> arch/arm/mach-omap2/cpuidle34xx.c => drivers/cpuidle/omap-34xx.c
> arch/arm/mach-omap2/cpuidle44xx.c => drivers/cpuidle/omap-44xx.c
> arch/arm/mach-shmobile/cpuidle.c  => drivers/cpuidle/arm-shmobile.c
> arch/arm/mach-davinci/cpuidle.c => drivers/cpuidle/davinci.c
> arch/arm/mach-at91/cpuidle.c => drivers/cpuidle/at91.c
> arch/arm/mach-s3c64xx/cpuidle.c => drivers/cpuidle/s3c64xx.c
> arch/arm/mach-exynos/cpuidle.c => drivers/cpuidle/exynos.c
> arch/arm/mach-kirkwood/cpuidle.c => drivers/cpuidle/kirkwood.c
> arch/arcm/mach-msm/idle.S => drivers/cpuidle/msm.S
> 
> That could be a first step and then we move the other archs which could 
> be a bit more tricky like the powerpc.
> 
> Does it make sense ?

Sounds good to me. However, if any of the drivers can be built as
loadable modules, or if there is any intention to make them so,
the file name should be globally unique and follow a common naming
scheme, e.g. idle-intel.c, idle-acpi.c, ...

	Arnd

[RFC PATCH 0/8] Add common ARM cpuidle init code

[Daniel Lezcano]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/03/2012 05:02 PM, Arnd Bergmann wrote:
> On Tuesday 03 January 2012, Daniel Lezcano wrote:
>> drivers/idle/intel_idle.c => drivers/cpuidle/intel.c
>> drivers/acpi/processor_idle.c => drivers/cpuidle/acpi.c
>>
>> arch/arm/mach-omap2/cpuidle34xx.c => drivers/cpuidle/omap-34xx.c
>> arch/arm/mach-omap2/cpuidle44xx.c => drivers/cpuidle/omap-44xx.c
>> arch/arm/mach-shmobile/cpuidle.c  => drivers/cpuidle/arm-shmobile.c
>> arch/arm/mach-davinci/cpuidle.c => drivers/cpuidle/davinci.c
>> arch/arm/mach-at91/cpuidle.c => drivers/cpuidle/at91.c
>> arch/arm/mach-s3c64xx/cpuidle.c => drivers/cpuidle/s3c64xx.c
>> arch/arm/mach-exynos/cpuidle.c => drivers/cpuidle/exynos.c
>> arch/arm/mach-kirkwood/cpuidle.c => drivers/cpuidle/kirkwood.c
>> arch/arcm/mach-msm/idle.S => drivers/cpuidle/msm.S
>>
>> That could be a first step and then we move the other archs which could 
>> be a bit more tricky like the powerpc.
>>
>> Does it make sense ?
> 
> Sounds good to me. However, if any of the drivers can be built as
> loadable modules, or if there is any intention to make them so,
> the file name should be globally unique and follow a common naming
> scheme, e.g. idle-intel.c, idle-acpi.c, ...

Makes sense.

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

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJPBBkzAAoJEAKBbMCpUGYAGoYH/3rFgCMaMXk71dEmm19gsq3l
FtSRbqeP+OXT+yDEaOG8cQr807Bn899WQrj6UY6gyRTHVPqlaPa4kKi09Jsn90BX
NMVVs4NyzZUrbV0Bofdw8D2Y2cMmVCs8xZfaU4FXa7eOvxEzSQImJVZhNvE8aGB3
b2h1r6Pp0fwXzJqJVvNRdzo49OONn/9l70YLf1pcimLpsmV/P3K5RTcTWMaYbuGW
FOudqawFQ8xDG7lQPMT4/kiVbHXNKq1VeDZvmenVvXdg0kRm9ptJ3kV4e5TrO1lO
RsiZAQiUn2NMt7RHZJimLzTd2ZXl3U2sQbRYbfe35VonSy8DjiCNkbjefVBj/JM=
=fVpl
-----END PGP SIGNATURE-----

[RFC PATCH 0/8] Add common ARM cpuidle init code

[Rob Lee]

Shawn and Arnd,

Sorry for my late reply.  For some reason, I didn't see this thread
until after Daniel's response.

Per an offline conversation Daniel and I had, since Daniel has
volunteered to move the other drivers and has already submitted a RFC
for at91 cpuidle, I will submit a v3 of the common cpuidle code which
will include moving the i.MX5 cpuidle code to drivers/cpuidle to have
the best chance of getting this code and an i.MX5 cpuidle driver into
3.3.  If my code get's merged, then Daniel will re-base his changes
against the common code.