Add common ARM cpuidle init code

Review

Quoted 7 lines · Robert Lee

(quoted diff) +7 -0

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

Quoted 3 lines

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

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.

Add commonly used cpuidle init code to avoid unecessary duplication.

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

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

arch/arm/common/Makefile +1 -0

index 6ea9b6f..0865f69 100644
@@ -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

arch/arm/common/cpuidle.c new file +24 -0

new file mode 100644
index 0000000..e9a46a3
@@ -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>

arch/arm/common/cpuidle.c new file +23 -0

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

arch/arm/common/cpuidle.c new file +7 -0

+}
+
+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[])
+{

arch/arm/common/cpuidle.c new file +18 -0

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

Quoted 7 lines · Robert Lee

Add commonly used cpuidle init code to avoid unecessary duplication.

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

---

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

Quoted 43 lines

?3 files changed, 158 insertions(+), 0 deletions(-) ?create mode 100644 arch/arm/common/cpuidle.c ?create mode 100644 arch/arm/include/asm/cpuidle.h

arch/arm/common/Makefile +0 -0

index 6ea9b6f..0865f69 100644
@@ -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

arch/arm/common/Makefile +1 -0

+obj-$(CONFIG_CPU_IDLE) ? ? ? ? ? ? ? += cpuidle.o

arch/arm/common/cpuidle.c new file +24 -0

new file mode 100644
index 0000000..e9a46a3
@@ -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>

arch/arm/common/cpuidle.c line 34

Quoted 10 lines

arch/arm/common/cpuidle.c new file +10 -0

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

Quoted 23 lines

arch/arm/common/cpuidle.c new file +23 -0

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

arch/arm/common/cpuidle.c line 67

Quoted 10 lines

arch/arm/common/cpuidle.c new file +10 -0

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

Quoted 7 lines

arch/arm/common/cpuidle.c new file +7 -0

+}
+
+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[])
+{

arch/arm/common/cpuidle.c line 84

Quoted 10 lines

arch/arm/common/cpuidle.c new file +10 -0

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

arch/arm/common/cpuidle.c line 91

Quoted 7 lines

arch/arm/common/cpuidle.c new file +7 -0

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

Quoted 18 lines

arch/arm/common/cpuidle.c new file +18 -0

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

arch/arm/common/cpuidle.c line 119

Quoted 10 lines

arch/arm/common/cpuidle.c new file +10 -0

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

Quoted 7 lines

arch/arm/common/cpuidle.c new file +7 -0

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

arch/arm/common/cpuidle.c line 127

Quoted 1 line

arch/arm/common/cpuidle.c new file +1 -0

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

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

Quoted 9 lines · Rob Herring

Quoted 7 lines · Robert Lee

Add commonly used cpuidle init code to avoid unecessary duplication.

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

---
 arch/arm/common/Makefile       |    1 +
 arch/arm/common/cpuidle.c      |  132 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/cpuidle.h |   25 ++++++++

Quoted 2 lines

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.

Quoted 7 lines

Quoted 6 lines

arch/arm/common/cpuidle.c new file +6 -0

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

Quoted 8 lines

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.

Quoted 102 lines

Quoted 23 lines

arch/arm/common/cpuidle.c new file +23 -0

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

arch/arm/common/cpuidle.c line 67

Quoted 10 lines

arch/arm/common/cpuidle.c new file +10 -0

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

Quoted 7 lines

arch/arm/common/cpuidle.c new file +7 -0

+}
+
+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[])
+{

arch/arm/common/cpuidle.c line 84

Quoted 10 lines

arch/arm/common/cpuidle.c new file +10 -0

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

arch/arm/common/cpuidle.c line 91

Quoted 7 lines

arch/arm/common/cpuidle.c new file +7 -0

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

Quoted 18 lines

arch/arm/common/cpuidle.c new file +18 -0

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

arch/arm/common/cpuidle.c line 119

Quoted 10 lines

arch/arm/common/cpuidle.c new file +10 -0

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

Quoted 9 lines

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

Quoted 7 lines

arch/arm/common/cpuidle.c new file +7 -0

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

Quoted 2 lines

Quoted 1 line

+ return -ENODEV;

Quoted 5 lines

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?

Quoted 12 lines · Rob Lee

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

Quoted 9 lines · Rob Herring

Quoted 7 lines · Robert Lee

Add commonly used cpuidle init code to avoid unecessary duplication.

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

---

?arch/arm/common/Makefile ? ? ? | ? ?1 +
?arch/arm/common/cpuidle.c ? ? ?| ?132 ++++++++++++++++++++++++++++++++++++++++
?arch/arm/include/asm/cpuidle.h | ? 25 ++++++++

Quoted 4 lines

Quoted 2 lines

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.

Quoted 4 lines

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?

Quoted 3 lines

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.

Quoted 8 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

arch/arm/common/cpuidle.c new file +6 -0

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

Quoted 8 lines

Quoted 8 lines

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

Quoted 116 lines

Quoted 102 lines

Quoted 23 lines

(quoted diff) +23 -0

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

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 7 lines

(quoted diff) +7 -0

+}
+
+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[])
+{

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Review

Quoted 7 lines

(quoted diff) +7 -0

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

Quoted 18 lines

+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? pr_err("%s: 'enter' function pointer NULL\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
+ ? ? ? ? ? ? ? ? ? ? } else
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? drv->states[i].enter = arm_enter_idle;
+ ? ? ? ? ? ? }
+ ? ? }

(quoted diff) +11 -0

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

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 9 lines

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

Quoted 7 lines

+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? 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();

Quoted 2 lines

Quoted 1 line

+ ? ? ? ? ? ? ? ? ? ? return -ENODEV;

Quoted 9 lines

Quoted 5 lines

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

Quoted 3 lines · Rob Lee

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.

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

Quoted 1 line

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.

Quoted 7 lines · Shawn Guo

Quoted 1 line

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.

Quoted 1 line

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.

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 ?

Quoted 16 lines · Daniel Lezcano

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.

Quoted 1 line

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