[PATCH v2 0/3] ARM: imx: Add cpuidle for imx platforms

[PATCH v2 0/3] ARM: imx: Add cpuidle for imx platforms

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

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

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

[Robert Lee]

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

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 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

diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index d53c35f..8939f79 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -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
diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
new file mode 100644
index 0000000..9e4c176
--- /dev/null
+++ b/arch/arm/plat-mxc/cpuidle.c
@@ -0,0 +1,151 @@
+/*
+ * 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/io.h>
+#include <linux/cpuidle.h>
+#include <linux/err.h>
+#include <asm/proc-fns.h>
+#include <mach/cpuidle.h>
+
+static int (*mach_cpuidle)(struct cpuidle_device *dev,
+			       struct cpuidle_state *state);
+static struct cpuidle_driver *imx_cpuidle_driver;
+static struct __initdata cpuidle_device * device;
+
+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);
+
+	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;
+}
+
+static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
+
+int __init imx_cpuidle_init(struct imx_cpuidle_data *cpuidle_data)
+{
+	int i, cpu_id;
+
+	if (cpuidle_data == NULL) {
+		pr_err("%s: cpuidle_data pointer NULL\n", __func__);
+		return -EINVAL;
+	}
+
+	if (cpuidle_data->mach_cpuidle == NULL) {
+		pr_err("%s: idle callback function NULL\n", __func__);
+		return -EINVAL;
+	}
+
+	imx_cpuidle_driver = cpuidle_data->imx_cpuidle_driver;
+
+	mach_cpuidle = cpuidle_data->mach_cpuidle;
+
+	/* register imx_cpuidle driver */
+	if (cpuidle_register_driver(imx_cpuidle_driver)) {
+		pr_err("%s: Failed to register cpuidle driver\n", __func__);
+		return -ENODEV;
+	}
+
+	/* if provided, initialize the mach level cpuidle functionality */
+	if (cpuidle_data->mach_cpuidle_init) {
+		if (cpuidle_data->mach_cpuidle_init(cpuidle_data)) {
+			pr_err("%s: Failed to register cpuidle driver\n",
+				 __func__);
+			cpuidle_unregister_driver(imx_cpuidle_driver);
+			return -ENODEV;
+		}
+	}
+
+	/* initialize state data for each cpuidle_device */
+	for_each_cpu(cpu_id, cpu_online_mask) {
+
+		device = &per_cpu(imx_cpuidle_device, cpu_id);
+		device->cpu = cpu_id;
+
+		device->state_count = min((unsigned int) CPUIDLE_STATE_MAX,
+			cpuidle_data->num_states);
+
+		device->prepare = cpuidle_data->prepare;
+
+		for (i = 0; i < device->state_count; i++) {
+			strlcpy(device->states[i].name,
+				cpuidle_data->state_data[i].name,
+				CPUIDLE_NAME_LEN);
+
+			strlcpy(device->states[i].desc,
+				cpuidle_data->state_data[i].desc,
+				CPUIDLE_DESC_LEN);
+
+			device->states[i].driver_data =
+				(void *)cpuidle_data->
+				state_data[i].mach_cpu_pwr_state;
+
+			/*
+			 * Because the imx_enter_idle function measures
+			 * and returns a valid time for all imx SoCs,
+			 * we always set this flag.
+			 */
+			device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;
+
+			device->states[i].exit_latency =
+				cpuidle_data->state_data[i].exit_latency;
+
+			device->states[i].power_usage =
+				cpuidle_data->state_data[i].power_usage;
+
+			device->states[i].target_residency =
+				cpuidle_data->state_data[i].target_residency;
+
+			device->states[i].enter = imx_enter_idle;
+		}
+	}
+
+	return 0;
+}
+
+int __init imx_cpuidle_dev_init(void)
+{
+	int cpu_id;
+
+	if (device == NULL) {
+		pr_err("%s: Failed to register (No device)\n", __func__);
+		return -ENODEV;
+	}
+
+	for_each_cpu(cpu_id, cpu_online_mask) {
+		device = &per_cpu(imx_cpuidle_device, cpu_id);
+		if (cpuidle_register_device(device)) {
+			pr_err("%s: Failed to register\n", __func__);
+			return -ENODEV;
+		}
+	}
+
+	return 0;
+}
+late_initcall(imx_cpuidle_dev_init);
diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
new file mode 100644
index 0000000..2bb109e
--- /dev/null
+++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
@@ -0,0 +1,56 @@
+/*
+ * 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_PLAT_MXC_CPUIDLE_H__
+#define __ARCH_ARM_PLAT_MXC_CPUIDLE_H__
+
+#include <linux/cpuidle.h>
+#include <mach/hardware.h>
+
+/* for passing cpuidle state info to the cpuidle driver. */
+struct imx_cpuidle_state_data {
+	enum mxc_cpu_pwr_mode	mach_cpu_pwr_state;
+	char			*name;
+	char			*desc;
+	/* time in uS to exit this idle state */
+	unsigned int		exit_latency;
+	/* OPTIONAL - power usage of this idle state in mW */
+	unsigned int		power_usage;
+	/* OPTIONAL - in uS. See drivers/cpuidle/governors/menu.c for usage */
+	unsigned int		target_residency;
+};
+
+struct imx_cpuidle_data {
+	unsigned int num_states;
+	struct cpuidle_driver *imx_cpuidle_driver;
+	struct imx_cpuidle_state_data *state_data;
+	int (*mach_cpuidle)(struct cpuidle_device *dev,
+		struct cpuidle_state *state);
+
+	/* OPTIONAL - parameter of mach_cpuidle_init func below */
+	void *mach_init_data;
+	/* OPTIONAL - callback for mach level cpuidle initialization */
+	int (*mach_cpuidle_init)(void *mach_init_data);
+	/* OPTIONAL - Search drivers/cpuidle/cpuidle.c for usage */
+	int (*prepare)(struct cpuidle_device *dev);
+};
+
+#ifdef CONFIG_CPU_IDLE
+int imx_cpuidle_init(struct imx_cpuidle_data *cpuidle_data);
+#else
+static inline int imx_cpuidle_init(struct imx_cpuidle_data *cpuidle_data)
+{
+	return -EINVAL;
+}
+#endif /* CONFIG_CPU_IDLE */
+
+#endif /* __ARCH_ARM_PLAT_MXC_CPUIDLE_H__ */
-- 
1.7.1

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

[Robert Lee]

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

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-mx5/system.c              |   35 +++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/system.h |    3 ++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-mx5/system.c b/arch/arm/mach-mx5/system.c
index 76ae8dc..85d6dd7 100644
--- a/arch/arm/mach-mx5/system.c
+++ b/arch/arm/mach-mx5/system.c
@@ -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;
+}
diff --git a/arch/arm/plat-mxc/include/mach/system.h b/arch/arm/plat-mxc/include/mach/system.h
index 51f02a9..f7c4be5 100644
--- a/arch/arm/plat-mxc/include/mach/system.h
+++ b/arch/arm/plat-mxc/include/mach/system.h
@@ -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)
 {
-- 
1.7.1

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

[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 <rob.lee@linaro.org>
---
 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(-)

diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index 383e7cd..dc5413b 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
@@ -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
diff --git a/arch/arm/mach-mx5/cpu_op-mx51.c b/arch/arm/mach-mx5/cpu_op-mx51.c
index 9d34c3d..d85ced3 100644
--- a/arch/arm/mach-mx5/cpu_op-mx51.c
+++ b/arch/arm/mach-mx5/cpu_op-mx51.c
@@ -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 */
diff --git a/arch/arm/mach-mx5/cpu_op-mx51.h b/arch/arm/mach-mx5/cpu_op-mx51.h
index 97477fe..60c60fd 100644
--- a/arch/arm/mach-mx5/cpu_op-mx51.h
+++ b/arch/arm/mach-mx5/cpu_op-mx51.h
@@ -12,3 +12,4 @@
  */
 
 extern struct cpu_op *mx51_get_cpu_op(int *op);
+extern struct imx_cpuidle_data mx51_cpuidle_data;
diff --git a/arch/arm/mach-mx5/mm.c b/arch/arm/mach-mx5/mm.c
index baea6e5..d20827e 100644
--- a/arch/arm/mach-mx5/mm.c
+++ b/arch/arm/mach-mx5/mm.c
@@ -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)
-- 
1.7.1

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

[Russell King - ARM Linux]

On Fri, Sep 16, 2011 at 12:27:48PM -0500, Robert Lee wrote:
> 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:

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

[PATCH v2 0/3] ARM: imx: Add cpuidle for imx platforms

[Sascha Hauer]

Hi Robert,

On Fri, Sep 16, 2011 at 12:27:47PM -0500, Robert Lee wrote:
> 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.

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

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

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

[PATCH v2 0/3] ARM: imx: Add cpuidle for imx platforms

[Amit Kucheria]

On 11 Sep 20, Sascha Hauer wrote:
> Hi Robert,
> 
> On Fri, Sep 16, 2011 at 12:27:47PM -0500, Robert Lee wrote:
> > 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.
> 
> > 
> > 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...
> 
> >  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.

[PATCH v2 0/3] ARM: imx: Add cpuidle for imx platforms

[Rob Lee]

Hello Sascha,

On 20 September 2011 02:16, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Hi Robert,
>
> On Fri, Sep 16, 2011 at 12:27:47PM -0500, Robert Lee wrote:
>> 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.
>

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.

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

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

[PATCH v2 0/3] ARM: imx: Add cpuidle for imx platforms

[Rob Lee]

Hello Amit,

On 20 September 2011 03:03, Amit Kucheria <amit.kucheria@linaro.org> wrote:
> On 11 Sep 20, Sascha Hauer wrote:
>> Hi Robert,
>>
>> On Fri, Sep 16, 2011 at 12:27:47PM -0500, Robert Lee wrote:
>> > 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.
>>
>> >
>> > 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...
>>
>> > ?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.
>

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.

>
>

[PATCH v2 0/3] ARM: imx: Add cpuidle for imx platforms

[Sascha Hauer]

On Tue, Sep 20, 2011 at 10:40:49AM -0500, Rob Lee wrote:
> Hello Sascha,
> 
> On 20 September 2011 02:16, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > Hi Robert,
> >
> > On Fri, Sep 16, 2011 at 12:27:47PM -0500, Robert Lee wrote:
> >> 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.
> >
> 
> 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.
> 
> >>
> >> 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...
> >
> >> ?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.

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.

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


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

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

[Rob Lee]

Hello Russell,

On 16 September 2011 16:36, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Sep 16, 2011 at 12:27:48PM -0500, Robert Lee wrote:
>> 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:
>
>> +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);
>> +
>> + ? ? 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;
>> +}
>

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?

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

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

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

[Rob Lee]

On 25 September 2011 22:54, Rob Lee <rob.lee@linaro.org> wrote:
> Hello Russell,
>
> On 16 September 2011 16:36, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Fri, Sep 16, 2011 at 12:27:48PM -0500, Robert Lee wrote:
>>> 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:
>>
>>> +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);
>>> +
>>> + ? ? 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;
>>> +}
>>
>
> 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.

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