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

login
register
mail settings
Submitter Robert Lee
Date Aug. 26, 2011, 2:33 a.m.
Message ID <1314325995-29118-1-git-send-email-rob.lee@linaro.org>
Download mbox | patch
Permalink /patch/111697/
State New
Headers show

Comments

Robert Lee - Aug. 26, 2011, 2:33 a.m.
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
Sascha Hauer - Aug. 26, 2011, 7:27 a.m.
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
Russell King - ARM Linux - Aug. 26, 2011, 8:21 a.m.
> 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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Robert Lee - Aug. 26, 2011, 2:56 p.m.
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 |
Sascha Hauer - Aug. 27, 2011, 11:10 a.m.
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
Shawn Guo - Sept. 9, 2011, 2:33 p.m.
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.
Sascha Hauer - Sept. 12, 2011, 10:11 a.m.
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
Shawn Guo - Sept. 12, 2011, 12:10 p.m.
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.

Patch

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