| Message ID | 1314325995-29118-1-git-send-email-rob.lee@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
> 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
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 |
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
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.
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
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.
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);
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