[PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs.

[PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs.

[Robert Lee]

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/plat-mxc/Kconfig   |   10 ++++
 arch/arm/plat-mxc/Makefile  |    2 +
 arch/arm/plat-mxc/cpuidle.c |  112 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/plat-mxc/cpuidle.c

diff --git a/arch/arm/plat-mxc/Kconfig b/arch/arm/plat-mxc/Kconfig
index a5353fc..84672b3 100644
--- a/arch/arm/plat-mxc/Kconfig
+++ b/arch/arm/plat-mxc/Kconfig
@@ -122,4 +122,14 @@ config IRAM_ALLOC
 	bool
 	select GENERIC_ALLOCATOR
 
+config ARCH_HAS_CPUIDLE
+	bool
+
+config MXC_CPUIDLE
+	bool
+	depends on ARCH_HAS_CPUIDLE && CPU_IDLE
+	default y
+	help
+	  Internal node to signify that the ARCH has CPUIDLE support.
+
 endif
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index d53c35f..59f20ac 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -19,6 +19,8 @@ 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_MXC_CPUIDLE) += 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..cd637e1
--- /dev/null
+++ b/arch/arm/plat-mxc/cpuidle.c
@@ -0,0 +1,112 @@
+/*
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/io.h>
+#include <linux/cpuidle.h>
+#include <asm/proc-fns.h>
+#include <mach/hardware.h>
+#include <mach/system.h>
+#include <mach/cpuidle.h>
+
+
+#ifndef MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS
+
+#define MXC_X_MACRO(a, b, c) {c}
+static struct imx_cpuidle_params default_cpuidle_params[] = \
+	MXC_CPUIDLE_TABLE;
+#undef MXC_X_MACRO
+
+static struct imx_cpuidle_params *imx_cpuidle_params = default_cpuidle_params;
+
+#else
+
+static struct imx_cpuidle_params *imx_cpuidle_params;
+
+#endif
+
+
+
+/* in case you want to override the mach level params at the board level */
+void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
+{
+	imx_cpuidle_params = cpuidle_params;
+}
+
+static int imx_enter_idle(struct cpuidle_device *dev,
+			       struct cpuidle_state *state)
+{
+	struct timeval before, after;
+	int idle_time, i;
+
+	/* We only need to pass an index to the mach level so here we
+	 * find the index of the name contained in the cpuidle_state
+	 * to pass. */
+	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++)
+		if (state == &dev->states[i])
+			break;
+
+	local_irq_disable();
+	local_fiq_disable();
+
+	do_gettimeofday(&before);
+
+	imx_cpu_do_idle(i);
+
+	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 struct cpuidle_driver imx_cpuidle_driver = {
+	.name =         "imx_idle",
+	.owner =        THIS_MODULE,
+};
+
+static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
+
+static int __init imx_cpuidle_init(void)
+{
+	struct cpuidle_device *device;
+	int i;
+
+	#define MXC_X_MACRO(a, b, c) #a
+	char *mxc_cpuidle_state_name[] = MXC_CPUIDLE_TABLE;
+	#undef MXC_X_MACRO
+
+	#define MXC_X_MACRO(a, b, c) b
+	char *mxc_cpuidle_state_desc[] = MXC_CPUIDLE_TABLE;
+	#undef MXC_X_MACRO
+
+	if (imx_cpuidle_params == NULL)
+		return -ENODEV;
+
+	cpuidle_register_driver(&imx_cpuidle_driver);
+
+	device = &per_cpu(imx_cpuidle_device, smp_processor_id());
+	device->state_count = MXC_NUM_CPUIDLE_STATES;
+
+	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++) {
+		device->states[i].enter = imx_enter_idle;
+		device->states[i].exit_latency = imx_cpuidle_params[i].latency;
+		device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;
+		strcpy(device->states[i].name, mxc_cpuidle_state_name[i]);
+		strcpy(device->states[i].desc, mxc_cpuidle_state_desc[i]);
+	}
+
+	if (cpuidle_register_device(device)) {
+		printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");
+		return -ENODEV;
+	}
+	return 0;
+}
+
+late_initcall(imx_cpuidle_init);
-- 
1.7.1

[PATCH 2/2] Adding support for the plat-mxc cpuidle driver to mach-mx5. Tested on i.MX51 Babbage board with ARM core running at 800Mhz. Idle OS ARM core power before patch = ~400mW. Idle OS ARM core power after patch = ~1.25mW.

[Robert Lee]

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-mx5/Kconfig                |    4 ++
 arch/arm/mach-mx5/include/mach/cpuidle.h |   45 ++++++++++++++++++++++++++++++
 arch/arm/mach-mx5/system.c               |   42 ++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-mx5/include/mach/cpuidle.h

diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
index b4e7c58..1672cfe 100644
--- a/arch/arm/mach-mx5/Kconfig
+++ b/arch/arm/mach-mx5/Kconfig
@@ -19,6 +19,7 @@ config SOC_IMX50
 	select ARCH_MXC_IOMUX_V3
 	select ARCH_MXC_AUDMUX_V2
 	select ARCH_HAS_CPUFREQ
+	select ARCH_HAS_CPUIDLE
 	select ARCH_MX5
 	select ARCH_MX50
 
@@ -30,6 +31,7 @@ config	SOC_IMX51
 	select ARCH_MXC_IOMUX_V3
 	select ARCH_MXC_AUDMUX_V2
 	select ARCH_HAS_CPUFREQ
+	select ARCH_HAS_CPUIDLE
 	select ARCH_MX5
 
 config	SOC_IMX53
@@ -38,9 +40,11 @@ config	SOC_IMX53
 	select ARM_L1_CACHE_SHIFT_6
 	select MXC_TZIC
 	select ARCH_MXC_IOMUX_V3
+	select ARCH_HAS_CPUIDLE
 	select ARCH_MX5
 	select ARCH_MX53
 
+
 if ARCH_MX50_SUPPORTED
 #comment "i.MX50 machines:"
 
diff --git a/arch/arm/mach-mx5/include/mach/cpuidle.h b/arch/arm/mach-mx5/include/mach/cpuidle.h
new file mode 100644
index 0000000..6c37963
--- /dev/null
+++ b/arch/arm/mach-mx5/include/mach/cpuidle.h
@@ -0,0 +1,45 @@
+/*
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __ARCH_ARM_PLAT_MXC_CPUIDLE_H__
+#define __ARCH_ARM_PLAT_MXC_CPUIDLE_H__
+
+
+/* CPUIDLE state parameters: name, description, exit_latency (us)
+ * exit_latencies were tested using i.MX51 running at 160MHz P-state
+ * for worst case latencies as to not cause a pm_qos violation when
+ * running at lower speeds.
+ */
+#define MXC_CPUIDLE_TABLE {\
+MXC_X_MACRO(POWERED_CLOCKED, "idle cpu clocked, powered.", 12),\
+MXC_X_MACRO(POWERED_NOCLOCK, "idle cpu powered, unclocked.", 14),\
+MXC_X_MACRO(NOPOWER_NOCLOCK, "idle cpu unpowered, unclocked.", 20),\
+MXC_X_MACRO(MXC_NUM_CPUIDLE_STATES, "", -1)}
+
+#define MXC_X_MACRO(a, b, c) a
+enum MXC_CPUIDLE_STATE_NAME MXC_CPUIDLE_TABLE;
+#undef MXC_X_MACRO
+
+struct imx_cpuidle_params {
+	unsigned int latency;
+};
+
+void imx_cpu_do_idle(int cpuidle_state_num);
+
+/* if you want to override the mach level params at the board level,
+ * define MXC_OVERRIDE_CPUIDLE_PARAMS and pass your board level paramaters
+ * into the imx_cpuidle_board_params function */
+
+/* #define MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS */
+
+#ifdef CONFIG_MXC_CPUIDLE
+extern void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params);
+#else
+inline void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
+{}
+#endif
+
+#endif /* #ifndef __ARCH_ARM_PLAT_MXC_CPUIDLE_H__ */
diff --git a/arch/arm/mach-mx5/system.c b/arch/arm/mach-mx5/system.c
index 76ae8dc..6ff938d 100644
--- a/arch/arm/mach-mx5/system.c
+++ b/arch/arm/mach-mx5/system.c
@@ -12,9 +12,16 @@
  */
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/clk.h>
+#include <asm/proc-fns.h>
 #include <mach/hardware.h>
+#include <mach/mxc.h>
+#include <mach/cpuidle.h>
 #include "crm_regs.h"
 
+static int arch_idle_mode = WAIT_UNCLOCKED_POWER_OFF;
+static struct clk *gpc_dvfs_clk;
+
 /* set cpu low power mode before WFI instruction. This function is called
   * mx5 because it can be used for mx50, mx51, and mx53.*/
 void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
@@ -82,3 +89,38 @@ void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
 		__raw_writel(empgc1, MXC_SRPG_EMPGC1_SRPGCR);
 	}
 }
+
+void imx_cpu_do_idle(int cpuidle_state_num)
+{
+
+	int local_arch_idle_mode = arch_idle_mode;
+
+	if (gpc_dvfs_clk == NULL)
+		gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs_clk");
+	/* gpc clock is needed for SRPG */
+	clk_enable(gpc_dvfs_clk);
+
+	/* convert MXC_CPUIDLE_STATE_NAME to mxc_cpu_pwr_mode. */
+	switch (cpuidle_state_num) {
+	case POWERED_CLOCKED:
+		local_arch_idle_mode = WAIT_CLOCKED;
+		break;
+	case POWERED_NOCLOCK:
+		local_arch_idle_mode = WAIT_UNCLOCKED;
+		break;
+	case NOPOWER_NOCLOCK:
+		local_arch_idle_mode = WAIT_UNCLOCKED_POWER_OFF;
+		break;
+	default:
+		break;
+	}
+	/* prepare registers for upcoming idle mode */
+	mx5_cpu_lp_set(local_arch_idle_mode);
+
+	/* enter wfi state which on i.MX5 can trigger */
+	cpu_do_idle();
+
+	clk_disable(gpc_dvfs_clk);
+}
+
+
-- 
1.7.1

[PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs.

[Sascha Hauer]

On Thu, Aug 25, 2011 at 09:33:14PM -0500, Robert Lee wrote:
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
> --- /dev/null
> +++ b/arch/arm/plat-mxc/cpuidle.c
> @@ -0,0 +1,112 @@
> +/*
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/cpuidle.h>
> +#include <asm/proc-fns.h>
> +#include <mach/hardware.h>
> +#include <mach/system.h>
> +#include <mach/cpuidle.h>
> +
> +
> +#ifndef MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS

Either there is a possibility to overwrite the cpuidle parameters
or there is none, but we don't need a define for this. I'm not
convinced we need this possibility at all though.

> +
> +#define MXC_X_MACRO(a, b, c) {c}
> +static struct imx_cpuidle_params default_cpuidle_params[] = \
> +	MXC_CPUIDLE_TABLE;
> +#undef MXC_X_MACRO

Hell! This is one of the worst unnecessary preprocessor abuses I've ever
seen. Do not show this in public again.

> +
> +static struct imx_cpuidle_params *imx_cpuidle_params = default_cpuidle_params;
> +
> +#else
> +
> +static struct imx_cpuidle_params *imx_cpuidle_params;
> +
> +#endif
> +
> +
> +
> +/* in case you want to override the mach level params at the board level */
> +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
> +{
> +	imx_cpuidle_params = cpuidle_params;
> +}
> +
> +static int imx_enter_idle(struct cpuidle_device *dev,
> +			       struct cpuidle_state *state)
> +{
> +	struct timeval before, after;
> +	int idle_time, i;
> +
> +	/* We only need to pass an index to the mach level so here we
> +	 * find the index of the name contained in the cpuidle_state
> +	 * to pass. */
> +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++)

A define for MXC_NUM_CPUIDLE_STATES looks wrong, it is not constant for
different SoCs.

> +		if (state == &dev->states[i])
> +			break;
> +
> +	local_irq_disable();
> +	local_fiq_disable();
> +
> +	do_gettimeofday(&before);
> +
> +	imx_cpu_do_idle(i);

We are currently working on running a single image on as many SoCs we
can. Take a step back and imagine what the linker says when it finds
6 different functions named imx_cpu_do_idle()

> +
> +	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 struct cpuidle_driver imx_cpuidle_driver = {
> +	.name =         "imx_idle",
> +	.owner =        THIS_MODULE,
> +};
> +
> +static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
> +
> +static int __init imx_cpuidle_init(void)
> +{
> +	struct cpuidle_device *device;
> +	int i;
> +
> +	#define MXC_X_MACRO(a, b, c) #a
> +	char *mxc_cpuidle_state_name[] = MXC_CPUIDLE_TABLE;
> +	#undef MXC_X_MACRO
> +
> +	#define MXC_X_MACRO(a, b, c) b
> +	char *mxc_cpuidle_state_desc[] = MXC_CPUIDLE_TABLE;
> +	#undef MXC_X_MACRO
> +
> +	if (imx_cpuidle_params == NULL)
> +		return -ENODEV;
> +
> +	cpuidle_register_driver(&imx_cpuidle_driver);
> +
> +	device = &per_cpu(imx_cpuidle_device, smp_processor_id());
> +	device->state_count = MXC_NUM_CPUIDLE_STATES;
> +
> +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++) {
> +		device->states[i].enter = imx_enter_idle;
> +		device->states[i].exit_latency = imx_cpuidle_params[i].latency;
> +		device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;
> +		strcpy(device->states[i].name, mxc_cpuidle_state_name[i]);
> +		strcpy(device->states[i].desc, mxc_cpuidle_state_desc[i]);
> +	}
> +
> +	if (cpuidle_register_device(device)) {
> +		printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");
> +		return -ENODEV;
> +	}
> +	return 0;

This should really be a platform driver.


I'm glad I'm on holiday for the next two weeks.

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 2/2] Adding support for the plat-mxc cpuidle driver to mach-mx5. Tested on i.MX51 Babbage board with ARM core running at 800Mhz. Idle OS ARM core power before patch = ~400mW. Idle OS ARM core power after patch = ~1.25mW.

[Russell King - ARM Linux]

> Subject: [PATCH 2/2] Adding support for the plat-mxc cpuidle driver to
>         mach-mx5. Tested on i.MX51 Babbage board with ARM core running
>         at 800Mhz. Idle OS ARM core power before patch = ~400mW. Idle
>         OS ARM core power after patch = ~1.25mW.

Please don't type your entire message into the subject line box.  The
subject line box is supposed to be a short summary of the patch, normally
less than 72 characters long.

On Thu, Aug 25, 2011 at 09:33:15PM -0500, Robert Lee wrote:
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/mach-mx5/Kconfig                |    4 ++
>  arch/arm/mach-mx5/include/mach/cpuidle.h |   45 ++++++++++++++++++++++++++++++
>  arch/arm/mach-mx5/system.c               |   42 ++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-mx5/include/mach/cpuidle.h
> 
> diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
> index b4e7c58..1672cfe 100644
> --- a/arch/arm/mach-mx5/Kconfig
> +++ b/arch/arm/mach-mx5/Kconfig
> @@ -19,6 +19,7 @@ config SOC_IMX50
>  	select ARCH_MXC_IOMUX_V3
>  	select ARCH_MXC_AUDMUX_V2
>  	select ARCH_HAS_CPUFREQ
> +	select ARCH_HAS_CPUIDLE
>  	select ARCH_MX5
>  	select ARCH_MX50
>  
> @@ -30,6 +31,7 @@ config	SOC_IMX51
>  	select ARCH_MXC_IOMUX_V3
>  	select ARCH_MXC_AUDMUX_V2
>  	select ARCH_HAS_CPUFREQ
> +	select ARCH_HAS_CPUIDLE
>  	select ARCH_MX5
>  
>  config	SOC_IMX53
> @@ -38,9 +40,11 @@ config	SOC_IMX53
>  	select ARM_L1_CACHE_SHIFT_6
>  	select MXC_TZIC
>  	select ARCH_MXC_IOMUX_V3
> +	select ARCH_HAS_CPUIDLE
>  	select ARCH_MX5
>  	select ARCH_MX53
>  
> +
>  if ARCH_MX50_SUPPORTED
>  #comment "i.MX50 machines:"
>  
> diff --git a/arch/arm/mach-mx5/include/mach/cpuidle.h b/arch/arm/mach-mx5/include/mach/cpuidle.h
> new file mode 100644
> index 0000000..6c37963
> --- /dev/null
> +++ b/arch/arm/mach-mx5/include/mach/cpuidle.h
> @@ -0,0 +1,45 @@
> +/*
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef __ARCH_ARM_PLAT_MXC_CPUIDLE_H__
> +#define __ARCH_ARM_PLAT_MXC_CPUIDLE_H__
> +
> +
> +/* CPUIDLE state parameters: name, description, exit_latency (us)
> + * exit_latencies were tested using i.MX51 running at 160MHz P-state
> + * for worst case latencies as to not cause a pm_qos violation when
> + * running at lower speeds.
> + */
> +#define MXC_CPUIDLE_TABLE {\
> +MXC_X_MACRO(POWERED_CLOCKED, "idle cpu clocked, powered.", 12),\
> +MXC_X_MACRO(POWERED_NOCLOCK, "idle cpu powered, unclocked.", 14),\
> +MXC_X_MACRO(NOPOWER_NOCLOCK, "idle cpu unpowered, unclocked.", 20),\
> +MXC_X_MACRO(MXC_NUM_CPUIDLE_STATES, "", -1)}
> +
> +#define MXC_X_MACRO(a, b, c) a
> +enum MXC_CPUIDLE_STATE_NAME MXC_CPUIDLE_TABLE;
> +#undef MXC_X_MACRO
> +
> +struct imx_cpuidle_params {
> +	unsigned int latency;
> +};
> +
> +void imx_cpu_do_idle(int cpuidle_state_num);
> +
> +/* if you want to override the mach level params at the board level,
> + * define MXC_OVERRIDE_CPUIDLE_PARAMS and pass your board level paramaters
> + * into the imx_cpuidle_board_params function */
> +
> +/* #define MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS */
> +
> +#ifdef CONFIG_MXC_CPUIDLE
> +extern void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params);
> +#else
> +inline void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
> +{}
> +#endif
> +
> +#endif /* #ifndef __ARCH_ARM_PLAT_MXC_CPUIDLE_H__ */
> diff --git a/arch/arm/mach-mx5/system.c b/arch/arm/mach-mx5/system.c
> index 76ae8dc..6ff938d 100644
> --- a/arch/arm/mach-mx5/system.c
> +++ b/arch/arm/mach-mx5/system.c
> @@ -12,9 +12,16 @@
>   */
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
> +#include <linux/clk.h>
> +#include <asm/proc-fns.h>
>  #include <mach/hardware.h>
> +#include <mach/mxc.h>
> +#include <mach/cpuidle.h>
>  #include "crm_regs.h"
>  
> +static int arch_idle_mode = WAIT_UNCLOCKED_POWER_OFF;
> +static struct clk *gpc_dvfs_clk;
> +
>  /* set cpu low power mode before WFI instruction. This function is called
>    * mx5 because it can be used for mx50, mx51, and mx53.*/
>  void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
> @@ -82,3 +89,38 @@ void mx5_cpu_lp_set(enum mxc_cpu_pwr_mode mode)
>  		__raw_writel(empgc1, MXC_SRPG_EMPGC1_SRPGCR);
>  	}
>  }
> +
> +void imx_cpu_do_idle(int cpuidle_state_num)
> +{
> +
> +	int local_arch_idle_mode = arch_idle_mode;
> +
> +	if (gpc_dvfs_clk == NULL)
> +		gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs_clk");
> +	/* gpc clock is needed for SRPG */
> +	clk_enable(gpc_dvfs_clk);
> +
> +	/* convert MXC_CPUIDLE_STATE_NAME to mxc_cpu_pwr_mode. */
> +	switch (cpuidle_state_num) {
> +	case POWERED_CLOCKED:
> +		local_arch_idle_mode = WAIT_CLOCKED;
> +		break;
> +	case POWERED_NOCLOCK:
> +		local_arch_idle_mode = WAIT_UNCLOCKED;
> +		break;
> +	case NOPOWER_NOCLOCK:
> +		local_arch_idle_mode = WAIT_UNCLOCKED_POWER_OFF;
> +		break;
> +	default:
> +		break;
> +	}
> +	/* prepare registers for upcoming idle mode */
> +	mx5_cpu_lp_set(local_arch_idle_mode);
> +
> +	/* enter wfi state which on i.MX5 can trigger */
> +	cpu_do_idle();
> +
> +	clk_disable(gpc_dvfs_clk);
> +}
> +
> +
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs.

[Russell King - ARM Linux]

> Subject: [PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs.

	"Add platform level cpuidle driver for i.MX SoCs."

is more natural to read in the git logs after the patch has been
committed.

On Thu, Aug 25, 2011 at 09:33:14PM -0500, Robert Lee wrote:
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/plat-mxc/Kconfig   |   10 ++++
>  arch/arm/plat-mxc/Makefile  |    2 +
>  arch/arm/plat-mxc/cpuidle.c |  112 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/plat-mxc/cpuidle.c
> 
> diff --git a/arch/arm/plat-mxc/Kconfig b/arch/arm/plat-mxc/Kconfig
> index a5353fc..84672b3 100644
> --- a/arch/arm/plat-mxc/Kconfig
> +++ b/arch/arm/plat-mxc/Kconfig
> @@ -122,4 +122,14 @@ config IRAM_ALLOC
>  	bool
>  	select GENERIC_ALLOCATOR
>  
> +config ARCH_HAS_CPUIDLE
> +	bool
> +
> +config MXC_CPUIDLE
> +	bool
> +	depends on ARCH_HAS_CPUIDLE && CPU_IDLE
> +	default y
> +	help
> +	  Internal node to signify that the ARCH has CPUIDLE support.
> +
>  endif
> diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
> index d53c35f..59f20ac 100644
> --- a/arch/arm/plat-mxc/Makefile
> +++ b/arch/arm/plat-mxc/Makefile
> @@ -19,6 +19,8 @@ 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_MXC_CPUIDLE) += 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..cd637e1
> --- /dev/null
> +++ b/arch/arm/plat-mxc/cpuidle.c
> @@ -0,0 +1,112 @@
> +/*
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/cpuidle.h>
> +#include <asm/proc-fns.h>
> +#include <mach/hardware.h>
> +#include <mach/system.h>
> +#include <mach/cpuidle.h>
> +
> +
> +#ifndef MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS
> +
> +#define MXC_X_MACRO(a, b, c) {c}
> +static struct imx_cpuidle_params default_cpuidle_params[] = \
> +	MXC_CPUIDLE_TABLE;
> +#undef MXC_X_MACRO
> +
> +static struct imx_cpuidle_params *imx_cpuidle_params = default_cpuidle_params;
> +
> +#else
> +
> +static struct imx_cpuidle_params *imx_cpuidle_params;
> +
> +#endif
> +
> +
> +
> +/* in case you want to override the mach level params at the board level */
> +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
> +{
> +	imx_cpuidle_params = cpuidle_params;
> +}
> +
> +static int imx_enter_idle(struct cpuidle_device *dev,
> +			       struct cpuidle_state *state)
> +{
> +	struct timeval before, after;
> +	int idle_time, i;
> +
> +	/* We only need to pass an index to the mach level so here we
> +	 * find the index of the name contained in the cpuidle_state
> +	 * to pass. */
> +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++)
> +		if (state == &dev->states[i])
> +			break;
> +
> +	local_irq_disable();
> +	local_fiq_disable();
> +
> +	do_gettimeofday(&before);
> +
> +	imx_cpu_do_idle(i);
> +
> +	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 struct cpuidle_driver imx_cpuidle_driver = {
> +	.name =         "imx_idle",
> +	.owner =        THIS_MODULE,
> +};
> +
> +static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
> +
> +static int __init imx_cpuidle_init(void)
> +{
> +	struct cpuidle_device *device;
> +	int i;
> +
> +	#define MXC_X_MACRO(a, b, c) #a
> +	char *mxc_cpuidle_state_name[] = MXC_CPUIDLE_TABLE;
> +	#undef MXC_X_MACRO
> +
> +	#define MXC_X_MACRO(a, b, c) b
> +	char *mxc_cpuidle_state_desc[] = MXC_CPUIDLE_TABLE;
> +	#undef MXC_X_MACRO
> +
> +	if (imx_cpuidle_params == NULL)
> +		return -ENODEV;
> +
> +	cpuidle_register_driver(&imx_cpuidle_driver);
> +
> +	device = &per_cpu(imx_cpuidle_device, smp_processor_id());
> +	device->state_count = MXC_NUM_CPUIDLE_STATES;
> +
> +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++) {
> +		device->states[i].enter = imx_enter_idle;
> +		device->states[i].exit_latency = imx_cpuidle_params[i].latency;
> +		device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;
> +		strcpy(device->states[i].name, mxc_cpuidle_state_name[i]);
> +		strcpy(device->states[i].desc, mxc_cpuidle_state_desc[i]);
> +	}
> +
> +	if (cpuidle_register_device(device)) {
> +		printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");
> +		return -ENODEV;
> +	}
> +	return 0;
> +}
> +
> +late_initcall(imx_cpuidle_init);
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs.

[Rob Lee]

Sascha and all,

Just FYI, this is my first submission to the community so I'm sure that I
have much to learn about community style beyond what is given in
the CodingStyle and Submit* documents.  Please give
"community newbie" level details in your feedback.

My comments are below.

On 26 August 2011 02:27, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Thu, Aug 25, 2011 at 09:33:14PM -0500, Robert Lee wrote:
> > Signed-off-by: Robert Lee <rob.lee@linaro.org>
> > ---
> > --- /dev/null
> > +++ b/arch/arm/plat-mxc/cpuidle.c
> > @@ -0,0 +1,112 @@
> > +/*
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. ?This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/cpuidle.h>
> > +#include <asm/proc-fns.h>
> > +#include <mach/hardware.h>
> > +#include <mach/system.h>
> > +#include <mach/cpuidle.h>
> > +
> > +
> > +#ifndef MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS
>
> Either there is a possibility to overwrite the cpuidle parameters
> or there is none, but we don't need a define for this. I'm not
> convinced we need this possibility at all though.
>

This was simply to avoid the unnecessary memory usage by creating
the default values if someone decided to override the default cpuidle
parameters for their build.

> > +
> > +#define MXC_X_MACRO(a, b, c) {c}
> > +static struct imx_cpuidle_params default_cpuidle_params[] = \
> > + ? ? MXC_CPUIDLE_TABLE;
> > +#undef MXC_X_MACRO
>
> Hell! This is one of the worst unnecessary preprocessor abuses I've ever
> seen. Do not show this in public again.
>

Based on your response, it appears that standard C X-macros are not Linux
kernel community friendly.

> > +
> > +static struct imx_cpuidle_params *imx_cpuidle_params = default_cpuidle_params;
> > +
> > +#else
> > +
> > +static struct imx_cpuidle_params *imx_cpuidle_params;
> > +
> > +#endif
> > +
> > +
> > +
> > +/* in case you want to override the mach level params at the board level */
> > +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
> > +{
> > + ? ? imx_cpuidle_params = cpuidle_params;
> > +}
> > +
> > +static int imx_enter_idle(struct cpuidle_device *dev,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_state *state)
> > +{
> > + ? ? struct timeval before, after;
> > + ? ? int idle_time, i;
> > +
> > + ? ? /* We only need to pass an index to the mach level so here we
> > + ? ? ?* find the index of the name contained in the cpuidle_state
> > + ? ? ?* to pass. */
> > + ? ? for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++)
>
> A define for MXC_NUM_CPUIDLE_STATES looks wrong, it is not constant for
> different SoCs.
>

The CPUIDLE states will be constant among a family of SoCs such as mach-mx5, but
the way I've written the driver, I've assumed a mach level defines a
family which now
that I think about it, obviously isn't the case for mach-imx.

If you have time, please give your thoughts on the organization of the mach
directories with regards to mach-imx and mach-mx5 keeping in mind that
i.MX6 will be coming soon.  This will help me in trying to make this driver
more acceptable and I can pass this info on to others to discuss and learn
from as well.

> > + ? ? ? ? ? ? if (state == &dev->states[i])
> > + ? ? ? ? ? ? ? ? ? ? break;
> > +
> > + ? ? local_irq_disable();
> > + ? ? local_fiq_disable();
> > +
> > + ? ? do_gettimeofday(&before);
> > +
> > + ? ? imx_cpu_do_idle(i);
>
> We are currently working on running a single image on as many SoCs we
> can. Take a step back and imagine what the linker says when it finds
> 6 different functions named imx_cpu_do_idle()
>

Understood.  The thought was that the imx_cpu_do_idle() would live in the
mach level system.c file which could then make necessary SoC family
specific calls as needed.  The imx_cpu_do_idle() added to system.c
in the second patch is an example, but in that case it was unnecessary
to make SoC specific calls.

> > +
> > + ? ? 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 struct cpuidle_driver imx_cpuidle_driver = {
> > + ? ? .name = ? ? ? ? "imx_idle",
> > + ? ? .owner = ? ? ? ?THIS_MODULE,
> > +};
> > +
> > +static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
> > +
> > +static int __init imx_cpuidle_init(void)
> > +{
> > + ? ? struct cpuidle_device *device;
> > + ? ? int i;
> > +
> > + ? ? #define MXC_X_MACRO(a, b, c) #a
> > + ? ? char *mxc_cpuidle_state_name[] = MXC_CPUIDLE_TABLE;
> > + ? ? #undef MXC_X_MACRO
> > +
> > + ? ? #define MXC_X_MACRO(a, b, c) b
> > + ? ? char *mxc_cpuidle_state_desc[] = MXC_CPUIDLE_TABLE;
> > + ? ? #undef MXC_X_MACRO
> > +
> > + ? ? if (imx_cpuidle_params == NULL)
> > + ? ? ? ? ? ? return -ENODEV;
> > +
> > + ? ? cpuidle_register_driver(&imx_cpuidle_driver);
> > +
> > + ? ? device = &per_cpu(imx_cpuidle_device, smp_processor_id());
> > + ? ? device->state_count = MXC_NUM_CPUIDLE_STATES;
> > +
> > + ? ? for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++) {
> > + ? ? ? ? ? ? device->states[i].enter = imx_enter_idle;
> > + ? ? ? ? ? ? device->states[i].exit_latency = imx_cpuidle_params[i].latency;
> > + ? ? ? ? ? ? device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;
> > + ? ? ? ? ? ? strcpy(device->states[i].name, mxc_cpuidle_state_name[i]);
> > + ? ? ? ? ? ? strcpy(device->states[i].desc, mxc_cpuidle_state_desc[i]);
> > + ? ? }

Upon looking at my code again, I want to change these strcpy's to a more
buffer overrun friendly copy.

> > +
> > + ? ? if (cpuidle_register_device(device)) {
> > + ? ? ? ? ? ? printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");
> > + ? ? ? ? ? ? return -ENODEV;
> > + ? ? }
> > + ? ? return 0;
>
> This should really be a platform driver.
>
>
> I'm glad I'm on holiday for the next two weeks.
>
> 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 1/2] Adding platform level cpuidle driver for i.MX SoCs.

[Sascha Hauer]

On Fri, Aug 26, 2011 at 09:56:31AM -0500, Rob Lee wrote:
> Sascha and all,
> 
> Just FYI, this is my first submission to the community so I'm sure that I
> have much to learn about community style beyond what is given in
> the CodingStyle and Submit* documents.  Please give
> "community newbie" level details in your feedback.
> 
> My comments are below.
> 
> On 26 August 2011 02:27, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On Thu, Aug 25, 2011 at 09:33:14PM -0500, Robert Lee wrote:
> > > Signed-off-by: Robert Lee <rob.lee@linaro.org>
> > > ---
> > > --- /dev/null
> > > +++ b/arch/arm/plat-mxc/cpuidle.c
> > > @@ -0,0 +1,112 @@
> > > +/*
> > > + * This file is licensed under the terms of the GNU General Public
> > > + * License version 2. ?This program is licensed "as is" without any
> > > + * warranty of any kind, whether express or implied.
> > > + */
> > > +
> > > +#include <linux/io.h>
> > > +#include <linux/cpuidle.h>
> > > +#include <asm/proc-fns.h>
> > > +#include <mach/hardware.h>
> > > +#include <mach/system.h>
> > > +#include <mach/cpuidle.h>
> > > +
> > > +
> > > +#ifndef MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS
> >
> > Either there is a possibility to overwrite the cpuidle parameters
> > or there is none, but we don't need a define for this. I'm not
> > convinced we need this possibility at all though.
> >
> 
> This was simply to avoid the unnecessary memory usage by creating
> the default values if someone decided to override the default cpuidle
> parameters for their build.

Is a board known that wants to override this? If not, just drop it
and we'll wait until someone has valid reasons to do so. I think
this is a SoC only feature and the board has nothing to change here.

> 
> > > +
> > > +#define MXC_X_MACRO(a, b, c) {c}
> > > +static struct imx_cpuidle_params default_cpuidle_params[] = \
> > > + ? ? MXC_CPUIDLE_TABLE;
> > > +#undef MXC_X_MACRO
> >
> > Hell! This is one of the worst unnecessary preprocessor abuses I've ever
> > seen. Do not show this in public again.
> >
> 
> Based on your response, it appears that standard C X-macros are not Linux
> kernel community friendly.

I never heard of standard C X-macros. You have an array of structs here,
there are no macros required to handle them.

> 
> > > +
> > > +static struct imx_cpuidle_params *imx_cpuidle_params = default_cpuidle_params;
> > > +
> > > +#else
> > > +
> > > +static struct imx_cpuidle_params *imx_cpuidle_params;
> > > +
> > > +#endif
> > > +
> > > +
> > > +
> > > +/* in case you want to override the mach level params at the board level */
> > > +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
> > > +{
> > > + ? ? imx_cpuidle_params = cpuidle_params;
> > > +}
> > > +
> > > +static int imx_enter_idle(struct cpuidle_device *dev,
> > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cpuidle_state *state)
> > > +{
> > > + ? ? struct timeval before, after;
> > > + ? ? int idle_time, i;
> > > +
> > > + ? ? /* We only need to pass an index to the mach level so here we
> > > + ? ? ?* find the index of the name contained in the cpuidle_state
> > > + ? ? ?* to pass. */
> > > + ? ? for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++)

btw please use cpuidle_set_statedata() / cpuidle_get_statedata instead
of looping around the states until you found the right one.

> >
> > A define for MXC_NUM_CPUIDLE_STATES looks wrong, it is not constant for
> > different SoCs.
> >
> 
> The CPUIDLE states will be constant among a family of SoCs such as mach-mx5, but
> the way I've written the driver, I've assumed a mach level defines a
> family which now
> that I think about it, obviously isn't the case for mach-imx.
> 
> If you have time, please give your thoughts on the organization of the mach
> directories with regards to mach-imx and mach-mx5 keeping in mind that
> i.MX6 will be coming soon.  This will help me in trying to make this driver
> more acceptable and I can pass this info on to others to discuss and learn
> from as well.

Generally:

- Put everything you need into a single driver (a platform driver)
- Do not call any functions imx_ or mx5_ functions outside your driver
- It seems that the cpuidle driver handles the same resources as
  platform_suspend_ops. I don't know enough of powermanagement to tell
  how both relate to each other, but as both use the same resources,
  both should be handled in the same place. That could mean that there
  are changes necessary to the current mx5 suspend ops.

When this is done, we can still decide whether we need a completely new
driver on i.MX6 or whether we can reuse the old one.

BTW. you don't need to think about i.MX6, you can also think about
i.MX3, i.MX2,..

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 1/2] Adding platform level cpuidle driver for i.MX SoCs.

[Shawn Guo]

On Fri, Aug 26, 2011 at 09:27:04AM +0200, Sascha Hauer wrote:
> On Thu, Aug 25, 2011 at 09:33:14PM -0500, Robert Lee wrote:
> > Signed-off-by: Robert Lee <rob.lee@linaro.org>
> > ---
> > --- /dev/null
> > +++ b/arch/arm/plat-mxc/cpuidle.c
> > @@ -0,0 +1,112 @@
> > +/*
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2.  This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/cpuidle.h>
> > +#include <asm/proc-fns.h>
> > +#include <mach/hardware.h>
> > +#include <mach/system.h>
> > +#include <mach/cpuidle.h>
> > +
> > +
> > +#ifndef MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS
> 
> Either there is a possibility to overwrite the cpuidle parameters
> or there is none, but we don't need a define for this. I'm not
> convinced we need this possibility at all though.
> 
> > +
> > +#define MXC_X_MACRO(a, b, c) {c}
> > +static struct imx_cpuidle_params default_cpuidle_params[] = \
> > +	MXC_CPUIDLE_TABLE;
> > +#undef MXC_X_MACRO
> 
> Hell! This is one of the worst unnecessary preprocessor abuses I've ever
> seen. Do not show this in public again.
> 
> > +
> > +static struct imx_cpuidle_params *imx_cpuidle_params = default_cpuidle_params;
> > +
> > +#else
> > +
> > +static struct imx_cpuidle_params *imx_cpuidle_params;
> > +
> > +#endif
> > +
> > +
> > +
> > +/* in case you want to override the mach level params at the board level */
> > +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
> > +{
> > +	imx_cpuidle_params = cpuidle_params;
> > +}
> > +
> > +static int imx_enter_idle(struct cpuidle_device *dev,
> > +			       struct cpuidle_state *state)
> > +{
> > +	struct timeval before, after;
> > +	int idle_time, i;
> > +
> > +	/* We only need to pass an index to the mach level so here we
> > +	 * find the index of the name contained in the cpuidle_state
> > +	 * to pass. */
> > +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++)
> 
> A define for MXC_NUM_CPUIDLE_STATES looks wrong, it is not constant for
> different SoCs.
> 
> > +		if (state == &dev->states[i])
> > +			break;
> > +
> > +	local_irq_disable();
> > +	local_fiq_disable();
> > +
> > +	do_gettimeofday(&before);
> > +
> > +	imx_cpu_do_idle(i);
> 
> We are currently working on running a single image on as many SoCs we
> can. Take a step back and imagine what the linker says when it finds
> 6 different functions named imx_cpu_do_idle()
> 
> > +
> > +	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 struct cpuidle_driver imx_cpuidle_driver = {
> > +	.name =         "imx_idle",
> > +	.owner =        THIS_MODULE,
> > +};
> > +
> > +static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
> > +
> > +static int __init imx_cpuidle_init(void)
> > +{
> > +	struct cpuidle_device *device;
> > +	int i;
> > +
> > +	#define MXC_X_MACRO(a, b, c) #a
> > +	char *mxc_cpuidle_state_name[] = MXC_CPUIDLE_TABLE;
> > +	#undef MXC_X_MACRO
> > +
> > +	#define MXC_X_MACRO(a, b, c) b
> > +	char *mxc_cpuidle_state_desc[] = MXC_CPUIDLE_TABLE;
> > +	#undef MXC_X_MACRO
> > +
> > +	if (imx_cpuidle_params == NULL)
> > +		return -ENODEV;
> > +
> > +	cpuidle_register_driver(&imx_cpuidle_driver);
> > +
> > +	device = &per_cpu(imx_cpuidle_device, smp_processor_id());
> > +	device->state_count = MXC_NUM_CPUIDLE_STATES;
> > +
> > +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++) {
> > +		device->states[i].enter = imx_enter_idle;
> > +		device->states[i].exit_latency = imx_cpuidle_params[i].latency;
> > +		device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;
> > +		strcpy(device->states[i].name, mxc_cpuidle_state_name[i]);
> > +		strcpy(device->states[i].desc, mxc_cpuidle_state_desc[i]);
> > +	}
> > +
> > +	if (cpuidle_register_device(device)) {
> > +		printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");
> > +		return -ENODEV;
> > +	}
> > +	return 0;
> 
> This should really be a platform driver.
> 
I do not understand the reason behind this comment.  The cpuidle is
not really a platform device which typically has IO and IRQ such
hardware resources requiring a platform driver to talk to.

Looking at the following cpuidle drivers, only davinci implemented it
as a platform driver.

    arch/arm/mach-at91/cpuidle.c
    arch/arm/mach-davinci/cpuidle.c
    arch/arm/mach-exynos4/cpuidle.c
    arch/arm/mach-kirkwood/cpuidle.c
    arch/arm/mach-omap2/cpuidle34xx.c
    arch/arm/mach-shmobile/cpuidle.c

Also, I'm seeing imx cpufreq sitting on the mainline is not a platform
driver either.  I really did not think of any good reason for cpuidle
to be a platform driver necessarily.

-- 
Regards,
Shawn

[PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs.

[Sascha Hauer]

On Fri, Sep 09, 2011 at 10:33:40PM +0800, Shawn Guo wrote:
> On Fri, Aug 26, 2011 at 09:27:04AM +0200, Sascha Hauer wrote:
> > On Thu, Aug 25, 2011 at 09:33:14PM -0500, Robert Lee wrote:
> > > Signed-off-by: Robert Lee <rob.lee@linaro.org>
> > > ---
> > > --- /dev/null
> > > +++ b/arch/arm/plat-mxc/cpuidle.c
> > > @@ -0,0 +1,112 @@
> > > +/*
> > > + * This file is licensed under the terms of the GNU General Public
> > > + * License version 2.  This program is licensed "as is" without any
> > > + * warranty of any kind, whether express or implied.
> > > + */
> > > +
> > > +#include <linux/io.h>
> > > +#include <linux/cpuidle.h>
> > > +#include <asm/proc-fns.h>
> > > +#include <mach/hardware.h>
> > > +#include <mach/system.h>
> > > +#include <mach/cpuidle.h>
> > > +
> > > +
> > > +#ifndef MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS
> > 
> > Either there is a possibility to overwrite the cpuidle parameters
> > or there is none, but we don't need a define for this. I'm not
> > convinced we need this possibility at all though.
> > 
> > > +
> > > +#define MXC_X_MACRO(a, b, c) {c}
> > > +static struct imx_cpuidle_params default_cpuidle_params[] = \
> > > +	MXC_CPUIDLE_TABLE;
> > > +#undef MXC_X_MACRO
> > 
> > Hell! This is one of the worst unnecessary preprocessor abuses I've ever
> > seen. Do not show this in public again.
> > 
> > > +
> > > +static struct imx_cpuidle_params *imx_cpuidle_params = default_cpuidle_params;
> > > +
> > > +#else
> > > +
> > > +static struct imx_cpuidle_params *imx_cpuidle_params;
> > > +
> > > +#endif
> > > +
> > > +
> > > +
> > > +/* in case you want to override the mach level params at the board level */
> > > +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
> > > +{
> > > +	imx_cpuidle_params = cpuidle_params;
> > > +}
> > > +
> > > +static int imx_enter_idle(struct cpuidle_device *dev,
> > > +			       struct cpuidle_state *state)
> > > +{
> > > +	struct timeval before, after;
> > > +	int idle_time, i;
> > > +
> > > +	/* We only need to pass an index to the mach level so here we
> > > +	 * find the index of the name contained in the cpuidle_state
> > > +	 * to pass. */
> > > +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++)
> > 
> > A define for MXC_NUM_CPUIDLE_STATES looks wrong, it is not constant for
> > different SoCs.
> > 
> > > +		if (state == &dev->states[i])
> > > +			break;
> > > +
> > > +	local_irq_disable();
> > > +	local_fiq_disable();
> > > +
> > > +	do_gettimeofday(&before);
> > > +
> > > +	imx_cpu_do_idle(i);
> > 
> > We are currently working on running a single image on as many SoCs we
> > can. Take a step back and imagine what the linker says when it finds
> > 6 different functions named imx_cpu_do_idle()
> > 
> > > +
> > > +	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 struct cpuidle_driver imx_cpuidle_driver = {
> > > +	.name =         "imx_idle",
> > > +	.owner =        THIS_MODULE,
> > > +};
> > > +
> > > +static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
> > > +
> > > +static int __init imx_cpuidle_init(void)
> > > +{
> > > +	struct cpuidle_device *device;
> > > +	int i;
> > > +
> > > +	#define MXC_X_MACRO(a, b, c) #a
> > > +	char *mxc_cpuidle_state_name[] = MXC_CPUIDLE_TABLE;
> > > +	#undef MXC_X_MACRO
> > > +
> > > +	#define MXC_X_MACRO(a, b, c) b
> > > +	char *mxc_cpuidle_state_desc[] = MXC_CPUIDLE_TABLE;
> > > +	#undef MXC_X_MACRO
> > > +
> > > +	if (imx_cpuidle_params == NULL)
> > > +		return -ENODEV;
> > > +
> > > +	cpuidle_register_driver(&imx_cpuidle_driver);
> > > +
> > > +	device = &per_cpu(imx_cpuidle_device, smp_processor_id());
> > > +	device->state_count = MXC_NUM_CPUIDLE_STATES;
> > > +
> > > +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++) {
> > > +		device->states[i].enter = imx_enter_idle;
> > > +		device->states[i].exit_latency = imx_cpuidle_params[i].latency;
> > > +		device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;
> > > +		strcpy(device->states[i].name, mxc_cpuidle_state_name[i]);
> > > +		strcpy(device->states[i].desc, mxc_cpuidle_state_desc[i]);
> > > +	}
> > > +
> > > +	if (cpuidle_register_device(device)) {
> > > +		printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");
> > > +		return -ENODEV;
> > > +	}
> > > +	return 0;
> > 
> > This should really be a platform driver.
> > 
> I do not understand the reason behind this comment.  The cpuidle is
> not really a platform device which typically has IO and IRQ such
> hardware resources requiring a platform driver to talk to.
> 
> Looking at the following cpuidle drivers, only davinci implemented it
> as a platform driver.
> 
>     arch/arm/mach-at91/cpuidle.c
>     arch/arm/mach-davinci/cpuidle.c
>     arch/arm/mach-exynos4/cpuidle.c
>     arch/arm/mach-kirkwood/cpuidle.c
>     arch/arm/mach-omap2/cpuidle34xx.c
>     arch/arm/mach-shmobile/cpuidle.c
> 
> Also, I'm seeing imx cpufreq sitting on the mainline is not a platform
> driver either.  I really did not think of any good reason for cpuidle
> to be a platform driver necessarily.

The reason may not be really of technical nature. A platform driver only
changes the thinking:

- A driver makes sure that the author knows that the hardware may or may
  not be present.
- A driver makes sure that it may run on different types of hardware if
  passed different platform specific data.
- A driver motivates the author to view the code as a single seperated
  topic and to put all related code into the driver.

In the 'real' device driver world all points above are clear to driver
authors, but when it comes to SoC internal stuff people tend to forget
this.

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 1/2] Adding platform level cpuidle driver for i.MX SoCs.

[Shawn Guo]

On Mon, Sep 12, 2011 at 12:11:30PM +0200, Sascha Hauer wrote:
> On Fri, Sep 09, 2011 at 10:33:40PM +0800, Shawn Guo wrote:
> > On Fri, Aug 26, 2011 at 09:27:04AM +0200, Sascha Hauer wrote:
> > > On Thu, Aug 25, 2011 at 09:33:14PM -0500, Robert Lee wrote:
> > > > Signed-off-by: Robert Lee <rob.lee@linaro.org>
> > > > ---
> > > > --- /dev/null
> > > > +++ b/arch/arm/plat-mxc/cpuidle.c
> > > > @@ -0,0 +1,112 @@
> > > > +/*
> > > > + * This file is licensed under the terms of the GNU General Public
> > > > + * License version 2.  This program is licensed "as is" without any
> > > > + * warranty of any kind, whether express or implied.
> > > > + */
> > > > +
> > > > +#include <linux/io.h>
> > > > +#include <linux/cpuidle.h>
> > > > +#include <asm/proc-fns.h>
> > > > +#include <mach/hardware.h>
> > > > +#include <mach/system.h>
> > > > +#include <mach/cpuidle.h>
> > > > +
> > > > +
> > > > +#ifndef MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS
> > > 
> > > Either there is a possibility to overwrite the cpuidle parameters
> > > or there is none, but we don't need a define for this. I'm not
> > > convinced we need this possibility at all though.
> > > 
> > > > +
> > > > +#define MXC_X_MACRO(a, b, c) {c}
> > > > +static struct imx_cpuidle_params default_cpuidle_params[] = \
> > > > +	MXC_CPUIDLE_TABLE;
> > > > +#undef MXC_X_MACRO
> > > 
> > > Hell! This is one of the worst unnecessary preprocessor abuses I've ever
> > > seen. Do not show this in public again.
> > > 
> > > > +
> > > > +static struct imx_cpuidle_params *imx_cpuidle_params = default_cpuidle_params;
> > > > +
> > > > +#else
> > > > +
> > > > +static struct imx_cpuidle_params *imx_cpuidle_params;
> > > > +
> > > > +#endif
> > > > +
> > > > +
> > > > +
> > > > +/* in case you want to override the mach level params at the board level */
> > > > +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
> > > > +{
> > > > +	imx_cpuidle_params = cpuidle_params;
> > > > +}
> > > > +
> > > > +static int imx_enter_idle(struct cpuidle_device *dev,
> > > > +			       struct cpuidle_state *state)
> > > > +{
> > > > +	struct timeval before, after;
> > > > +	int idle_time, i;
> > > > +
> > > > +	/* We only need to pass an index to the mach level so here we
> > > > +	 * find the index of the name contained in the cpuidle_state
> > > > +	 * to pass. */
> > > > +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++)
> > > 
> > > A define for MXC_NUM_CPUIDLE_STATES looks wrong, it is not constant for
> > > different SoCs.
> > > 
> > > > +		if (state == &dev->states[i])
> > > > +			break;
> > > > +
> > > > +	local_irq_disable();
> > > > +	local_fiq_disable();
> > > > +
> > > > +	do_gettimeofday(&before);
> > > > +
> > > > +	imx_cpu_do_idle(i);
> > > 
> > > We are currently working on running a single image on as many SoCs we
> > > can. Take a step back and imagine what the linker says when it finds
> > > 6 different functions named imx_cpu_do_idle()
> > > 
> > > > +
> > > > +	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 struct cpuidle_driver imx_cpuidle_driver = {
> > > > +	.name =         "imx_idle",
> > > > +	.owner =        THIS_MODULE,
> > > > +};
> > > > +
> > > > +static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
> > > > +
> > > > +static int __init imx_cpuidle_init(void)
> > > > +{
> > > > +	struct cpuidle_device *device;
> > > > +	int i;
> > > > +
> > > > +	#define MXC_X_MACRO(a, b, c) #a
> > > > +	char *mxc_cpuidle_state_name[] = MXC_CPUIDLE_TABLE;
> > > > +	#undef MXC_X_MACRO
> > > > +
> > > > +	#define MXC_X_MACRO(a, b, c) b
> > > > +	char *mxc_cpuidle_state_desc[] = MXC_CPUIDLE_TABLE;
> > > > +	#undef MXC_X_MACRO
> > > > +
> > > > +	if (imx_cpuidle_params == NULL)
> > > > +		return -ENODEV;
> > > > +
> > > > +	cpuidle_register_driver(&imx_cpuidle_driver);
> > > > +
> > > > +	device = &per_cpu(imx_cpuidle_device, smp_processor_id());
> > > > +	device->state_count = MXC_NUM_CPUIDLE_STATES;
> > > > +
> > > > +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++) {
> > > > +		device->states[i].enter = imx_enter_idle;
> > > > +		device->states[i].exit_latency = imx_cpuidle_params[i].latency;
> > > > +		device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;
> > > > +		strcpy(device->states[i].name, mxc_cpuidle_state_name[i]);
> > > > +		strcpy(device->states[i].desc, mxc_cpuidle_state_desc[i]);
> > > > +	}
> > > > +
> > > > +	if (cpuidle_register_device(device)) {
> > > > +		printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");
> > > > +		return -ENODEV;
> > > > +	}
> > > > +	return 0;
> > > 
> > > This should really be a platform driver.
> > > 
> > I do not understand the reason behind this comment.  The cpuidle is
> > not really a platform device which typically has IO and IRQ such
> > hardware resources requiring a platform driver to talk to.
> > 
> > Looking at the following cpuidle drivers, only davinci implemented it
> > as a platform driver.
> > 
> >     arch/arm/mach-at91/cpuidle.c
> >     arch/arm/mach-davinci/cpuidle.c
> >     arch/arm/mach-exynos4/cpuidle.c
> >     arch/arm/mach-kirkwood/cpuidle.c
> >     arch/arm/mach-omap2/cpuidle34xx.c
> >     arch/arm/mach-shmobile/cpuidle.c
> > 
> > Also, I'm seeing imx cpufreq sitting on the mainline is not a platform
> > driver either.  I really did not think of any good reason for cpuidle
> > to be a platform driver necessarily.
> 
> The reason may not be really of technical nature. A platform driver only
> changes the thinking:
> 
> - A driver makes sure that the author knows that the hardware may or may
>   not be present.

The authors can know or have known that without creating a unnecessary
platform driver.

> - A driver makes sure that it may run on different types of hardware if
>   passed different platform specific data.

Without creating a platform driver, I'm sure there are some way to pass
platform specific data.  (How omap cpuidle and imx cpufreq did that?)  

> - A driver motivates the author to view the code as a single seperated
>   topic and to put all related code into the driver.
> 
A separated cpuidle.c file can also do the same.

> In the 'real' device driver world all points above are clear to driver
> authors, but when it comes to SoC internal stuff people tend to forget
> this.
> 
Again, it seems to me that we do not need to create an unnecessary
platform driver to get clear about all points above.

With cpuidle implemented as a platform driver, we will need to register
a platform device for it.  It's just silly to me, because we do not
have a cpuidle device at all.

-- 
Regards,
Shawn