ARM: imx: Add cpuidle for imx platforms

PATCH Superseded Robert Lee 4 participants 12 messages Sep 16, 2011 – Sep 26, 2011

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape

lore

Share & tools

lore

Robert Lee

author

2011-09-16 17:27 UTC #0 lore raw

v2 cover ARM: imx: Add cpuidle for imx platforms

This patch series adds a common imx cpuidle driver, some common mach-mx5 level cpuidle functionality, and an i.MX51 instance of using this driver.

The patch series is based on v3.1-rc2.

Changes since v1:
* To address all the problems found during review of v1, a complete re-write was needed.

Robert Lee (3):
ARM: imx: Add imx cpuidle driver
ARM: imx: Add cpuidle for mach-mx5
ARM: imx: Add cpuidle for i.MX51

 arch/arm/mach-mx5/Makefile               |    3 +-
 arch/arm/mach-mx5/cpu_op-mx51.c          |   35 +++++++-
 arch/arm/mach-mx5/cpu_op-mx51.h          |    1 +
 arch/arm/mach-mx5/mm.c                   |    4 +
 arch/arm/mach-mx5/system.c               |   35 +++++++
 arch/arm/plat-mxc/Makefile               |    1 +
 arch/arm/plat-mxc/cpuidle.c              |  151 ++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/cpuidle.h |   56 +++++++++++
 arch/arm/plat-mxc/include/mach/system.h  |    3 +
 9 files changed, 286 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/plat-mxc/cpuidle.c
 create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h

Robert Lee

author

2011-09-16 17:27 UTC #1 lore raw

PATCH v2 1/3 ARM: imx: Add imx cpuidle driver

Introduce a new cpuidle driver which provides the common cpuidle functionality necessary for any imx soc cpuidle implementation.

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

---
 arch/arm/plat-mxc/Makefile               |    1 +
 arch/arm/plat-mxc/cpuidle.c              |  151 ++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/cpuidle.h |   56 +++++++++++
 3 files changed, 208 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/plat-mxc/cpuidle.c
 create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h

arch/arm/plat-mxc/Makefile +1 -0

index d53c35f..8939f79 100644
@@ -19,6 +19,7 @@ obj-$(CONFIG_ARCH_MXC_AUDMUX_V1) += audmux-v1.o
 obj-$(CONFIG_ARCH_MXC_AUDMUX_V2) += audmux-v2.o
 obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
 obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
+obj-$(CONFIG_CPU_IDLE) += cpuidle.o
 ifdef CONFIG_SND_IMX_SOC
 obj-y += ssi-fiq.o
 obj-y += ssi-fiq-ksym.o

arch/arm/plat-mxc/cpuidle.c new file +151 -0

151 lines not on this page · raw message

arch/arm/plat-mxc/include/mach/cpuidle.h new file +56 -0

56 lines not on this page · raw message

Signature · 1 line

1.7.1

Russell King - ARM Linux

2011-09-16 21:36 UTC #2 lore raw

PATCH v2 1/3 ARM: imx: Add imx cpuidle driver

Quoted 2 lines · Robert Lee

Introduce a new cpuidle driver which provides the common cpuidle functionality necessary for any imx soc cpuidle implementation.

I think its probably about time we said no to this duplication of CPU idle infrastructure. There seems to be a common pattern appearing through all SoCs - they're all doing this:

Quoted 13 lines

arch/arm/plat-mxc/cpuidle.c new file +13 -0

+static int imx_enter_idle(struct cpuidle_device *dev,
+			       struct cpuidle_state *state)
+{
+	struct timeval before, after;
+	int idle_time;
+
+	local_irq_disable();
+	local_fiq_disable();
+
+	do_gettimeofday(&before);
+
+	mach_cpuidle(dev, state);
+

arch/arm/plat-mxc/cpuidle.c line 47

Quoted 10 lines

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

+	do_gettimeofday(&after);
+
+	local_fiq_enable();
+	local_irq_enable();
+
+	idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
+		(after.tv_usec - before.tv_usec);
+
+	return idle_time;
+}

in some form, where 'do_gettimeofday' might be ktime_get() or getnstimeofday(). If we can standardize on which of the many time functions can be used (which would be a definite plus) we should move this out to common code.

Maybe also the initialization code could be standardized and improved too - for instance, what if you boot with maxcpus=1 on a platform supporting 2 CPUs, and you bring CPU1 online from userspace? When these CPU idle initialization functions are called, only one CPU will be online, and as they use 'for_each_cpu(cpu_id, cpu_online_mask)' CPU1 will be missing the cpu idle init.

Rob Lee

author

2011-09-26 03:54 UTC #3 lore raw

PATCH v2 1/3 ARM: imx: Add imx cpuidle driver

Hello Russell,

Quoted 23 lines · Russell King - ARM Linux

Quoted 2 lines · Robert Lee
Introduce a new cpuidle driver which provides the common cpuidle functionality necessary for any imx soc cpuidle implementation.
I think its probably about time we said no to this duplication of CPU idle infrastructure. ?There seems to be a common pattern appearing through all SoCs - they're all doing this:
Quoted 15 lines
(quoted diff) +15 -0
+static int imx_enter_idle(struct cpuidle_device *dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_state *state)
+{
+ ? ? struct timeval before, after;
+ ? ? int idle_time;
+
+ ? ? local_irq_disable();
+ ? ? local_fiq_disable();
+
+ ? ? do_gettimeofday(&before);
+
+ ? ? mach_cpuidle(dev, state);
+
+ ? ? do_gettimeofday(&after);
+

Quoted 9 lines

arch/arm/plat-mxc/cpuidle.c line 47
Quoted 8 lines
+ ? ? local_fiq_enable();
+ ? ? local_irq_enable();
arch/arm/plat-mxc/cpuidle.c new file +6 -0
+
+ ? ? idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
+ ? ? ? ? ? ? (after.tv_usec - before.tv_usec);
+
+ ? ? return idle_time;
+}

If I understand you correctly, it sounds like you are suggesting adding cpuidle initialization functionality that is common for all ARM. Did you mean just the above function or also the functionality found in imx_cpuidle_init and imx_cpuidle_dev_init? For this common ARM functionality, would a cpuidle.c file in arch/arm/common/ be the right location?

Quoted 4 lines

in some form, where 'do_gettimeofday' might be ktime_get() or getnstimeofday(). ?If we can standardize on which of the many time functions can be used (which would be a definite plus) we should move this out to common code.

Looking into the time keeping functionality more, of the time functions you mentioned I think that ktime_get is preferable as its result it effectively a monotonic time that won't be changed with calls to do_settimeofday unlike the other two functions whose use of xtime only could result in an error in the reported time difference on SMP systems.

Quoted 8 lines

Maybe also the initialization code could be standardized and improved too - for instance, what if you boot with maxcpus=1 on a platform supporting 2 CPUs, and you bring CPU1 online from userspace? ?When these CPU idle initialization functions are called, only one CPU will be online, and as they use 'for_each_cpu(cpu_id, cpu_online_mask)' CPU1 will be missing the cpu idle init.

Rob Lee

author

2011-09-26 18:59 UTC #4 lore raw

PATCH v2 1/3 ARM: imx: Add imx cpuidle driver

Quoted 37 lines · Rob Lee

Hello Russell,
Quoted 23 lines · Russell King - ARM Linux
Quoted 2 lines · Robert Lee
Introduce a new cpuidle driver which provides the common cpuidle functionality necessary for any imx soc cpuidle implementation.
I think its probably about time we said no to this duplication of CPU idle infrastructure. ?There seems to be a common pattern appearing through all SoCs - they're all doing this:
Quoted 15 lines
(quoted diff) +15 -0
+static int imx_enter_idle(struct cpuidle_device *dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_state *state)
+{
+ ? ? struct timeval before, after;
+ ? ? int idle_time;
+
+ ? ? local_irq_disable();
+ ? ? local_fiq_disable();
+
+ ? ? do_gettimeofday(&before);
+
+ ? ? mach_cpuidle(dev, state);
+
+ ? ? do_gettimeofday(&after);
+
Quoted 9 lines
arch/arm/plat-mxc/cpuidle.c line 47
Quoted 8 lines
+ ? ? local_fiq_enable();
+ ? ? local_irq_enable();
arch/arm/plat-mxc/cpuidle.c new file +6 -0
+
+ ? ? idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
+ ? ? ? ? ? ? (after.tv_usec - before.tv_usec);
+
+ ? ? return idle_time;
+}

Quoted 6 lines

If I understand you correctly, it sounds like you are suggesting adding cpuidle initialization functionality that is common for all ARM. Did you mean just the above function or also the functionality found in imx_cpuidle_init and imx_cpuidle_dev_init? ?For this common ARM functionality, would a cpuidle.c file in arch/arm/common/ be the right location?

After looking at this further today, it appears that there is precedence for putting a cpuidle driver common to a particular cpu architecture in drivers/idle. So perhaps a new file "arm_idle.c" that has common cpuidle functionality could be added to that location.

Quoted 21 lines

Quoted 5 lines
in some form, where 'do_gettimeofday' might be ktime_get() or getnstimeofday(). ?If we can standardize on which of the many time functions can be used (which would be a definite plus) we should move this out to common code.
Looking into the time keeping functionality more, of the time functions you mentioned I think that ktime_get is preferable as its result it effectively a monotonic time that won't be changed with calls to do_settimeofday unlike the other two functions whose use of xtime only could result in an error in the reported time difference on SMP systems.
Quoted 8 lines
Maybe also the initialization code could be standardized and improved too - for instance, what if you boot with maxcpus=1 on a platform supporting 2 CPUs, and you bring CPU1 online from userspace? ?When these CPU idle initialization functions are called, only one CPU will be online, and as they use 'for_each_cpu(cpu_id, cpu_online_mask)' CPU1 will be missing the cpu idle init.

Robert Lee

author

2011-09-16 17:27 UTC #5 lore raw

PATCH v2 2/3 ARM: imx: Add cpuidle for mach-mx5

In reply to Robert Lee

Add functionality for initialization and handling of a cpuidle driver requests entering a cpu idle state.

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

---
 arch/arm/mach-mx5/system.c              |   35 +++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/system.h |    3 ++
 2 files changed, 38 insertions(+), 0 deletions(-)

arch/arm/mach-mx5/system.c +35 -0

index 76ae8dc..85d6dd7 100644
@@ -12,7 +12,11 @@
  */
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <asm/proc-fns.h>
 #include <mach/hardware.h>
+#include <mach/cpuidle.h>
 #include "crm_regs.h"
 
 /* set cpu low power mode before WFI instruction. This function is called

@@ -82,3 +86,34 @@ void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
 		__raw_writel(empgc1, MXC_SRPG_EMPGC1_SRPGCR);
 	}
 }
+
+int mx5_cpuidle(struct cpuidle_device *dev, struct cpuidle_state *state)
+{
+	mx5_cpu_lp_set((enum mxc_cpu_pwr_mode)state->driver_data);
+
+	cpu_do_idle();
+
+	return 0;
+}
+
+int __init mx5_cpuidle_init(void * init_data)
+{
+	int ret;
+	struct clk *gpc_dvfs_clk;
+
+	gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs");
+
+	if (IS_ERR(gpc_dvfs_clk)) {
+		pr_err("%s: Failed to get gpc_dvfs clock\n", __func__);
+		return (int)gpc_dvfs_clk;
+	}
+
+	ret = clk_enable(gpc_dvfs_clk);
+
+	if (IS_ERR(&ret)) {
+		pr_err("%s: Failed to enable gpc_dvfs clock\n", __func__);
+		return ret;
+	}
+
+	return 0;
+}

arch/arm/plat-mxc/include/mach/system.h +3 -0

index 51f02a9..f7c4be5 100644
@@ -17,10 +17,13 @@
 #ifndef __ASM_ARCH_MXC_SYSTEM_H__
 #define __ASM_ARCH_MXC_SYSTEM_H__
 
+#include <linux/cpuidle.h>
 #include <mach/hardware.h>
 #include <mach/common.h>
 
 extern void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode);
+extern int mx5_cpuidle_init(void *init_data);
+extern int mx5_cpuidle(struct cpuidle_device *dev, struct cpuidle_state *state);
 
 static inline void arch_idle(void)
 {

Signature · 1 line

1.7.1

Robert Lee

author

2011-09-16 17:27 UTC #6 lore raw

PATCH v2 3/3 ARM: imx: Add cpuidle for i.MX51

In reply to Robert Lee

Add the i.MX51 SoC specific cpuidle operating point data and other necessary data. Add init call to fill in imx cpuidle driver data structures.

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

---
 arch/arm/mach-mx5/Makefile      |    3 +--
 arch/arm/mach-mx5/cpu_op-mx51.c |   35 ++++++++++++++++++++++++++++++++++-
 arch/arm/mach-mx5/cpu_op-mx51.h |    1 +
 arch/arm/mach-mx5/mm.c          |    4 ++++
 4 files changed, 40 insertions(+), 3 deletions(-)

arch/arm/mach-mx5/Makefile +1 -2

index 383e7cd..dc5413b 100644
@@ -5,9 +5,8 @@
 # Object file lists.
 obj-y   := cpu.o mm.o clock-mx51-mx53.o devices.o ehci.o system.o
 obj-$(CONFIG_SOC_IMX50) += mm-mx50.o
-
+obj-$(CONFIG_SOC_IMX51)	+= cpu_op-mx51.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
 obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o
 obj-$(CONFIG_MACH_MX53_EVK) += board-mx53_evk.o

arch/arm/mach-mx5/cpu_op-mx51.c +34 -1

index 9d34c3d..d85ced3 100644
@@ -12,9 +12,12 @@
  */
 
 #include <linux/types.h>
-#include <mach/hardware.h>
 #include <linux/kernel.h>
+#include <asm/proc-fns.h>
+#include <mach/cpuidle.h>
+#include <mach/system.h>
 
+#ifdef CONFIG_CPU_FREQ_IMX
 static struct cpu_op mx51_cpu_op[] = {
 	{
 	.cpu_rate = 160000000,},

@@ -27,3 +30,33 @@ struct cpu_op *mx51_get_cpu_op(int *op)
 	*op = ARRAY_SIZE(mx51_cpu_op);
 	return mx51_cpu_op;
 }
+#endif /* CONFIG_CPU_FREQ_IMX */
+
+#ifdef CONFIG_CPU_IDLE
+static struct imx_cpuidle_state_data mx51_cpuidle_state_data[] __initdata = {
+	{
+		.name = "powered_noclock",
+		.desc = "idle cpu powered, unclocked.",
+		.exit_latency = 12,
+		.mach_cpu_pwr_state = WAIT_UNCLOCKED,
+	}, {
+		.name = "nopower_noclock",
+		.desc = "idle cpu unpowered, unclocked.",
+		.exit_latency = 20,
+		.mach_cpu_pwr_state = WAIT_UNCLOCKED_POWER_OFF,
+	}
+};
+
+static struct cpuidle_driver imx51_cpuidle_driver = {
+	.name	= "imx51_cpuidle",
+	.owner	= THIS_MODULE,
+};
+
+struct imx_cpuidle_data mx51_cpuidle_data __initdata = {
+	.imx_cpuidle_driver = &imx51_cpuidle_driver,
+	.state_data = mx51_cpuidle_state_data,
+	.mach_cpuidle = mx5_cpuidle,
+	.mach_cpuidle_init = mx5_cpuidle_init,
+	.num_states = ARRAY_SIZE(mx51_cpuidle_state_data),
+};
+#endif /* CONFIG_CPU_IDLE */

arch/arm/mach-mx5/cpu_op-mx51.h +1 -0

index 97477fe..60c60fd 100644
@@ -12,3 +12,4 @@
  */
 
 extern struct cpu_op *mx51_get_cpu_op(int *op);
+extern struct imx_cpuidle_data mx51_cpuidle_data;

arch/arm/mach-mx5/mm.c +4 -0

index baea6e5..d20827e 100644
@@ -20,6 +20,8 @@
 #include <mach/common.h>
 #include <mach/devices-common.h>
 #include <mach/iomux-v3.h>
+#include <mach/cpuidle.h>
+#include "cpu_op-mx51.h"
 
 /*
  * Define the MX51 memory map.

@@ -148,6 +150,8 @@ void __init imx51_soc_init(void)
 
 	/* i.mx51 has the i.mx35 type sdma */
 	imx_add_imx_sdma("imx35-sdma", MX51_SDMA_BASE_ADDR, MX51_INT_SDMA, &imx51_sdma_pdata);
+
+	imx_cpuidle_init(&mx51_cpuidle_data);
 }
 
 void __init imx53_soc_init(void)

Signature · 1 line

1.7.1

Sascha Hauer

maintainer

2011-09-20 07:16 UTC #7 lore raw

v2 cover ARM: imx: Add cpuidle for imx platforms

In reply to Robert Lee

Hi Robert,

Quoted 9 lines · Robert Lee

This patch series adds a common imx cpuidle driver, some common mach-mx5 level cpuidle functionality, and an i.MX51 instance of using this driver.
The patch series is based on v3.1-rc2.
Changes since v1:
* To address all the problems found during review of v1, a complete re-write was needed.

Much better than the last version, thanks.

Quoted 8 lines

Robert Lee (3):
ARM: imx: Add imx cpuidle driver
ARM: imx: Add cpuidle for mach-mx5
ARM: imx: Add cpuidle for i.MX51
 arch/arm/mach-mx5/Makefile               |    3 +-
 arch/arm/mach-mx5/cpu_op-mx51.c          |   35 +++++++-

Your additions to this file...

Quoted 3 lines

 arch/arm/mach-mx5/cpu_op-mx51.h          |    1 +
 arch/arm/mach-mx5/mm.c                   |    4 +
 arch/arm/mach-mx5/system.c               |   35 +++++++

And this one should go to arch/arm/mach-mx5/cpuidle.c, then we have all i.MX5 specific cpuidle stuff together without ifdeffing the code. I assume that cpuidle support for i.MX53 will look identically, right?

Quoted 9 lines

 arch/arm/plat-mxc/Makefile               |    1 +
 arch/arm/plat-mxc/cpuidle.c              |  151 ++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/cpuidle.h |   56 +++++++++++
 arch/arm/plat-mxc/include/mach/system.h  |    3 +
 9 files changed, 286 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/plat-mxc/cpuidle.c
 create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Amit Kucheria

2011-09-20 08:03 UTC #8 lore raw

v2 cover ARM: imx: Add cpuidle for imx platforms

Quoted 26 lines · Sascha Hauer

Hi Robert,
Quoted 9 lines · Robert Lee
This patch series adds a common imx cpuidle driver, some common mach-mx5 level cpuidle functionality, and an i.MX51 instance of using this driver.
The patch series is based on v3.1-rc2.
Changes since v1:
* To address all the problems found during review of v1, a complete re-write was needed.
Much better than the last version, thanks.
Quoted 8 lines
Robert Lee (3):
ARM: imx: Add imx cpuidle driver
ARM: imx: Add cpuidle for mach-mx5
ARM: imx: Add cpuidle for i.MX51
 arch/arm/mach-mx5/Makefile               |    3 +-
 arch/arm/mach-mx5/cpu_op-mx51.c          |   35 +++++++-
Your additions to this file...

Quoted 7 lines

Quoted 3 lines
 arch/arm/mach-mx5/cpu_op-mx51.h          |    1 +
 arch/arm/mach-mx5/mm.c                   |    4 +
 arch/arm/mach-mx5/system.c               |   35 +++++++
And this one should go to arch/arm/mach-mx5/cpuidle.c, then we have all i.MX5 specific cpuidle stuff together without ifdeffing the code. I assume that cpuidle support for i.MX53 will look identically, right?

Yes, let's not separate only for i.MX51. i.MX53 and even i.MX6 might look similar. They can always tune the exit latency criteria if needed.

Rob Lee

author

2011-09-20 15:47 UTC #9 lore raw

v2 cover ARM: imx: Add cpuidle for imx platforms

Hello Amit,

Quoted 31 lines · Amit Kucheria

Quoted 30 lines · Sascha Hauer
Hi Robert,
Quoted 9 lines · Robert Lee
This patch series adds a common imx cpuidle driver, some common mach-mx5 level cpuidle functionality, and an i.MX51 instance of using this driver.
The patch series is based on v3.1-rc2.
Changes since v1:
* To address all the problems found during review of v1, a complete re-write was needed.
Much better than the last version, thanks.
Quoted 8 lines
Robert Lee (3):
? ARM: imx: Add imx cpuidle driver
? ARM: imx: Add cpuidle for mach-mx5
? ARM: imx: Add cpuidle for i.MX51
?arch/arm/mach-mx5/Makefile ? ? ? ? ? ? ? | ? ?3 +- ?arch/arm/mach-mx5/cpu_op-mx51.c ? ? ? ? ?| ? 35 +++++++-
Your additions to this file...
Quoted 3 lines
?arch/arm/mach-mx5/cpu_op-mx51.h ? ? ? ? ?| ? ?1 +
?arch/arm/mach-mx5/mm.c ? ? ? ? ? ? ? ? ? | ? ?4 +
?arch/arm/mach-mx5/system.c ? ? ? ? ? ? ? | ? 35 +++++++

Quoted 7 lines

Quoted 3 lines
And this one should go to arch/arm/mach-mx5/cpuidle.c, then we have all i.MX5 specific cpuidle stuff together without ifdeffing the code. I assume that cpuidle support for i.MX53 will look identically, right?
Yes, let's not separate only for i.MX51. i.MX53 and even i.MX6 might look similar. They can always tune the exit latency criteria if needed.

i.MX6 will have significantly different exit_latency values and may need to have target residency values as well. Also, i.MX6 may have a different number of states, so I think it is best to have separate sets of SoC state data as needed instead of trying to make one set of state data to be used by all i.MX6.

Quoted 2 lines

Rob Lee

author

2011-09-20 15:40 UTC #10 lore raw

v2 cover ARM: imx: Add cpuidle for imx platforms

In reply to Sascha Hauer

Hello Sascha,

Quoted 7 lines · Sascha Hauer

Hi Robert,
Quoted 4 lines · Robert Lee
This patch series adds a common imx cpuidle driver, some common mach-mx5 level cpuidle functionality, and an i.MX51 instance of using this driver.

Quoted 8 lines

Quoted 5 lines
The patch series is based on v3.1-rc2.
Changes since v1:
* To address all the problems found during review of v1, a complete re-write was needed.
Much better than the last version, thanks.

Thanks to Shawn Guo for thoroughly reviewing and providing feedback on this version and Jason Hui contributed to the reviewing as well despite both of them being very busy with there own submissions.

Quoted 11 lines

Quoted 8 lines
Robert Lee (3):
? ARM: imx: Add imx cpuidle driver
? ARM: imx: Add cpuidle for mach-mx5
? ARM: imx: Add cpuidle for i.MX51
?arch/arm/mach-mx5/Makefile ? ? ? ? ? ? ? | ? ?3 +- ?arch/arm/mach-mx5/cpu_op-mx51.c ? ? ? ? ?| ? 35 +++++++-
Your additions to this file...

Quoted 8 lines

Quoted 3 lines
?arch/arm/mach-mx5/cpu_op-mx51.h ? ? ? ? ?| ? ?1 +
?arch/arm/mach-mx5/mm.c ? ? ? ? ? ? ? ? ? | ? ?4 +
?arch/arm/mach-mx5/system.c ? ? ? ? ? ? ? | ? 35 +++++++
And this one should go to arch/arm/mach-mx5/cpuidle.c, then we have all i.MX5 specific cpuidle stuff together without ifdeffing the code. I assume that cpuidle support for i.MX53 will look identically, right?

After considering it further after the first patch, there can be differences between the i.MX51 and i.MX53 state data as i.MX51 supports an additional idle state that i.MX53 does not. This state was not added to this version of the patch because it's additional power savings is very small and there is fairly time consuming characterization needed to come up with accurate cpuidle state data. But to allow for the possibility that this state could be added in the future, it seemed the choice was to add this init data to the an i.MX51 specific file such as cpu_op-mx51.c/h and add ifdefs for cpuidle and cpufreq. Or, make a cpuidle.c file as you suggest and add ifdefs for the different mx5 SoCs. Just let me know which way you think is better given this information.

For the call to imx_cpuidle_init from imx51_soc_init in mm.c, this seems like a good location to initialize for all imx51_soc. If I don't call the init from here, can you recommend another location that I should call it from?

Quoted 18 lines

Quoted 9 lines

?arch/arm/plat-mxc/Makefile ? ? ? ? ? ? ? | ? ?1 +
?arch/arm/plat-mxc/cpuidle.c ? ? ? ? ? ? ?| ?151 ++++++++++++++++++++++++++++++
?arch/arm/plat-mxc/include/mach/cpuidle.h | ? 56 +++++++++++
?arch/arm/plat-mxc/include/mach/system.h ?| ? ?3 +
?9 files changed, 286 insertions(+), 3 deletions(-) ?create mode 100644 arch/arm/plat-mxc/cpuidle.c ?create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h

Sascha

Pengutronix e.K. ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ? ? ? ? ? ? ? ? ? ? ? ? ? |
Industrial Linux Solutions ? ? ? ? ? ? ? ? | http://www.pengutronix.de/ ?|
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 ? ?|
Amtsgericht Hildesheim, HRA 2686 ? ? ? ? ? | Fax: ? +49-5121-206917-5555 |

Sascha Hauer

maintainer

2011-09-20 19:07 UTC #11 lore raw

v2 cover ARM: imx: Add cpuidle for imx platforms

Quoted 44 lines · Rob Lee

Hello Sascha,
Quoted 7 lines · Sascha Hauer
Hi Robert,
Quoted 4 lines · Robert Lee
This patch series adds a common imx cpuidle driver, some common mach-mx5 level cpuidle functionality, and an i.MX51 instance of using this driver.
Quoted 8 lines
Quoted 5 lines
The patch series is based on v3.1-rc2.
Changes since v1:
* To address all the problems found during review of v1, a complete re-write was needed.
Much better than the last version, thanks.
Thanks to Shawn Guo for thoroughly reviewing and providing feedback on this version and Jason Hui contributed to the reviewing as well despite both of them being very busy with there own submissions.
Quoted 11 lines
Quoted 8 lines
Robert Lee (3):
? ARM: imx: Add imx cpuidle driver
? ARM: imx: Add cpuidle for mach-mx5
? ARM: imx: Add cpuidle for i.MX51
?arch/arm/mach-mx5/Makefile ? ? ? ? ? ? ? | ? ?3 +- ?arch/arm/mach-mx5/cpu_op-mx51.c ? ? ? ? ?| ? 35 +++++++-
Your additions to this file...
Quoted 8 lines
Quoted 3 lines
?arch/arm/mach-mx5/cpu_op-mx51.h ? ? ? ? ?| ? ?1 +
?arch/arm/mach-mx5/mm.c ? ? ? ? ? ? ? ? ? | ? ?4 +
?arch/arm/mach-mx5/system.c ? ? ? ? ? ? ? | ? 35 +++++++
And this one should go to arch/arm/mach-mx5/cpuidle.c, then we have all i.MX5 specific cpuidle stuff together without ifdeffing the code. I assume that cpuidle support for i.MX53 will look identically, right?

Quoted 12 lines

After considering it further after the first patch, there can be differences between the i.MX51 and i.MX53 state data as i.MX51 supports an additional idle state that i.MX53 does not. This state was not added to this version of the patch because it's additional power savings is very small and there is fairly time consuming characterization needed to come up with accurate cpuidle state data. But to allow for the possibility that this state could be added in the future, it seemed the choice was to add this init data to the an i.MX51 specific file such as cpu_op-mx51.c/h and add ifdefs for cpuidle and cpufreq. Or, make a cpuidle.c file as you suggest and add ifdefs for the different mx5 SoCs. Just let me know which way you think is better given this information.

Just put it in cpuidle.c for now. Better to have the cpuidle stuff together in one file. We can still decide to move it to another place should the need arise.

Quoted 5 lines

For the call to imx_cpuidle_init from imx51_soc_init in mm.c, this seems like a good location to initialize for all imx51_soc. If I don't call the init from here, can you recommend another location that I should call it from?

Calling it from mm.c is fine.

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |