| Message ID | 1327379854-12403-2-git-send-email-rob.lee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 01/23/2012 10:37 PM, Robert Lee wrote: > The patch adds some cpuidle initialization functionality commonly > duplicated by many platforms. > > Signed-off-by: Robert Lee <rob.lee@linaro.org> A few cosmetic comments below, but otherwise looks good. Reviewed-by: Rob Herring <rob.herring@calxeda.com> > --- > drivers/cpuidle/Makefile | 2 +- > drivers/cpuidle/common.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/cpuidle.h | 24 +++++++ > 3 files changed, 177 insertions(+), 1 deletions(-) > create mode 100644 drivers/cpuidle/common.c > > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile > index 5634f88..2928d93 100644 > --- a/drivers/cpuidle/Makefile > +++ b/drivers/cpuidle/Makefile > @@ -2,4 +2,4 @@ > # Makefile for cpuidle. > # > > -obj-y += cpuidle.o driver.o governor.o sysfs.o governors/ > +obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/ > diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c > new file mode 100644 > index 0000000..dafa758 > --- /dev/null > +++ b/drivers/cpuidle/common.c > @@ -0,0 +1,152 @@ > +/* > + * Copyright 2011 Freescale Semiconductor, Inc. > + * Copyright 2011 Linaro Ltd. 2012 now... > + * > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > + > +/* > + * This code performs provides some commonly used cpuidle setup functionality > + * used by many ARM SoC platforms. Providing this functionality here > + * reduces the duplication of this code for each ARM platform that uses it. > + */ > + > +#include <linux/kernel.h> > +#include <linux/io.h> > +#include <linux/cpuidle.h> > +#include <linux/hrtimer.h> > +#include <linux/err.h> > +#include <linux/slab.h> > +#include <asm/proc-fns.h> > + > +static struct cpuidle_device __percpu * common_cpuidle_devices; > + > +static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index); > + > +int cpuidle_def_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + cpu_do_idle(); > + return index; > +} > + > +static int simple_enter(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + ktime_t time_start, time_end; > + > + local_irq_disable(); > + > + time_start = ktime_get(); > + > + index = do_idle[index](dev, drv, index); > + > + time_end = ktime_get(); > + > + local_irq_enable(); > + > + dev->last_residency = > + (int)ktime_to_us(ktime_sub(time_end, time_start)); > + > + return index; > +} > + > +void common_cpuidle_devices_uninit(void) > +{ > + int cpu_id; > + struct cpuidle_device *dev; > + > + for_each_possible_cpu(cpu_id) { > + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); > + cpuidle_unregister_device(dev); > + } > + > + free_percpu(common_cpuidle_devices); > +} > + > +/** > + * common_cpuidle_init() - Provides some commonly used init functionality. > + * @pdrv Pointer to your cpuidle_driver object. > + * @simple Use the common simple_enter wrapper? remove the ? > + * @driver_data_init Pointer to your platform function to initialize your > + * platform specific driver data. Use NULL if platform > + * specific data is not needed. > + * > + * Common cpuidle init interface to provide common cpuidle functionality > + * used by many platforms. > + */ > +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple, > + void (*driver_data_init)(struct cpuidle_device *dev)) > +{ > + struct cpuidle_device *dev; > + struct cpuidle_driver *drv; > + int i, cpu_id, ret; > + > + if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) { > + pr_err("%s: Invalid Input\n", __func__); Using pr_fmt rather than function name is preferred. > + return -EINVAL; > + } > + > + drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL); > + if (!drv) { > + pr_err("%s: no memory for cpuidle driver\n", __func__); > + return -ENOMEM; > + } > + > + *drv = *pdrv; Perhaps a comment so it is clear you intend this to be struct copy. > + > + for (i = 0; simple && (i < drv->state_count); i++) { > + do_idle[i] = drv->states[i].enter; > + drv->states[i].enter = simple_enter; > + } > + > + ret = cpuidle_register_driver(drv); > + if (ret) { > + pr_err("%s: Failed to register cpuidle driver\n", __func__); > + goto free_drv; > + } > + > + common_cpuidle_devices = alloc_percpu(struct cpuidle_device); > + if (common_cpuidle_devices == NULL) { > + ret = -ENOMEM; > + goto unregister_drv; > + } > + > + /* initialize state data for each cpuidle_device */ > + for_each_possible_cpu(cpu_id) { > + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); > + dev->cpu = cpu_id; > + dev->state_count = drv->state_count; > + > + if (driver_data_init) > + driver_data_init(dev); > + > + ret = cpuidle_register_device(dev); > + if (ret) { > + pr_err("%s: Failed to register cpu %u\n", > + __func__, cpu_id); > + goto uninit; > + } > + } > + > + return 0; > +uninit: > + > + common_cpuidle_devices_uninit(); > + > +unregister_drv: > + > + cpuidle_unregister_driver(drv); > + > +free_drv: > + > + kfree(drv); > + > + return ret; Remove all the blank lines in the error handling. > +} > diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h > index 712abcc..2aa89db 100644 > --- a/include/linux/cpuidle.h > +++ b/include/linux/cpuidle.h > @@ -56,6 +56,16 @@ struct cpuidle_state { > > #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000) > > +/* Common ARM WFI state */ > +#define CPUIDLE_ARM_WFI_STATE {\ > + .enter = cpuidle_def_idle,\ > + .exit_latency = 2,\ > + .target_residency = 1,\ > + .flags = CPUIDLE_FLAG_TIME_VALID,\ > + .name = "WFI",\ > + .desc = "ARM core clock gating (WFI)",\ > +} > + > /** > * cpuidle_get_statedata - retrieves private driver state data > * @st_usage: the state usage statistics > @@ -141,6 +151,13 @@ extern void cpuidle_resume_and_unlock(void); > extern int cpuidle_enable_device(struct cpuidle_device *dev); > extern void cpuidle_disable_device(struct cpuidle_device *dev); > > +/* provide a default idle function */ > +extern int cpuidle_def_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index); > +extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple, > + void (*driver_data_init)(struct cpuidle_device *dev)); > +extern void common_cpuidle_devices_uninit(void); > + > #else > static inline void disable_cpuidle(void) { } > static inline int cpuidle_idle_call(void) { return -ENODEV; } > @@ -157,6 +174,13 @@ static inline void cpuidle_resume_and_unlock(void) { } > static inline int cpuidle_enable_device(struct cpuidle_device *dev) > {return -ENODEV; } > static inline void cpuidle_disable_device(struct cpuidle_device *dev) { } > +static inline int cpuidle_def_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{return -ENODEV; } > +static inline int common_cpuidle_init(struct cpuidle_driver *pdrv, > + bool simple, void (*driver_data_init)(struct cpuidle_device *dev)) > +{return -ENODEV; } > +static inline void common_cpuidle_devices_uninit(void) { } > > #endif >
Robert Lee <rob.lee@linaro.org> writes: > The patch adds some cpuidle initialization functionality commonly > duplicated by many platforms. > > Signed-off-by: Robert Lee <rob.lee@linaro.org> [...] > +static int simple_enter(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + ktime_t time_start, time_end; > + > + local_irq_disable(); Is this needed? arch/arm/kernel/process.c:cpu_idle() already disables IRQs > + time_start = ktime_get(); > + > + index = do_idle[index](dev, drv, index); > + > + time_end = ktime_get(); > + > + local_irq_enable(); > + > + dev->last_residency = > + (int)ktime_to_us(ktime_sub(time_end, time_start)); I don't think this cast is quite right here, since last_residency is an int, it needs to be capped, or you'll end up with negative last_residency. I think you need to do something like is done in poll_idle() in drivers/cpuidle/cpuidle.c: diff = ktime_to_us(ktime_sub(t2, t1)); if (diff > INT_MAX) diff = INT_MAX; Speaking of poll_idle(), simple_idle() looks quite similar to it. Maybe they can be combined? Kevin
> > A few cosmetic comments below, but otherwise looks good. > > Reviewed-by: Rob Herring <rob.herring@calxeda.com> > Rob, thanks for the review. Agree with your comments and will fix in the next version.
On Tue, Jan 24, 2012 at 2:16 PM, Kevin Hilman <khilman@ti.com> wrote: > Robert Lee <rob.lee@linaro.org> writes: > >> The patch adds some cpuidle initialization functionality commonly >> duplicated by many platforms. >> >> Signed-off-by: Robert Lee <rob.lee@linaro.org> > > [...] > >> +static int simple_enter(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + ktime_t time_start, time_end; >> + >> + local_irq_disable(); > > Is this needed? arch/arm/kernel/process.c:cpu_idle() already disables IRQs > As far as I can tell, all calls into cpuidle_idle_call have already called this. I had left it just in case someone wanted to call cpuidle_idle_call directly, but that probably doesn't make sense. Perhaps in the comments of cpuidle_idle_call I just list this as a requirement and remove it from simple_enter. >> + time_start = ktime_get(); >> + >> + index = do_idle[index](dev, drv, index); >> + >> + time_end = ktime_get(); >> + >> + local_irq_enable(); >> + >> + dev->last_residency = >> + (int)ktime_to_us(ktime_sub(time_end, time_start)); > > I don't think this cast is quite right here, since last_residency is an > int, it needs to be capped, or you'll end up with negative last_residency. > > I think you need to do something like is done in poll_idle() in > drivers/cpuidle/cpuidle.c: > > diff = ktime_to_us(ktime_sub(t2, t1)); > if (diff > INT_MAX) > diff = INT_MAX; > > Thanks, I missed that. Will fix in v4. > Speaking of poll_idle(), simple_idle() looks quite similar to it. Maybe > they can be combined? Good point. I'll look into that. > > Kevin >
On Mon, Jan 23, 2012 at 8:37 PM, Robert Lee <rob.lee@linaro.org> wrote: > diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c > new file mode 100644 > index 0000000..dafa758 > --- /dev/null > +++ b/drivers/cpuidle/common.c > @@ -0,0 +1,152 @@ > +/* > + * Copyright 2011 Freescale Semiconductor, Inc. > + * Copyright 2011 Linaro Ltd. > + * > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > + > +/* > + * This code performs provides some commonly used cpuidle setup functionality s/performs // > +static struct cpuidle_device __percpu * common_cpuidle_devices; You can use DECLARE_PER_CPU here. Is there any particular reason to allocate these dynamically? You can replace the code above with, static DEFINE_PER_CPU(struct cpuidle_device, common_cpuidle_devices); I might change the variable name to "cpu_cpuidle_device" in that case since you are addressing a single CPU when using the per cpu accessor functions and "common_cpuidle_devices" sounds like an array or a list or something. No big deal to keep the current name though. > +static int simple_enter(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + ktime_t time_start, time_end; > + > + local_irq_disable(); > + > + time_start = ktime_get(); > + > + index = do_idle[index](dev, drv, index); > + > + time_end = ktime_get(); > + > + local_irq_enable(); > + > + dev->last_residency = > + (int)ktime_to_us(ktime_sub(time_end, time_start)); Sometimes an attempt to enter some C-state fails and the do_idle will return immediately. What do you think about having do_idle return -EERROR in this case and the conditionally setting last_residency to zero in those cases? The point is that a C-state's total residency time should not increase in the case where the hardware did not successfully transition into that C-state. I've observed many times where a specific low power state was actually achieved in the hardware but /sys/devices/system/cpu/cpuN/cpuidle/stateM/time keeps incrementing (albeit in very tiny increments). Something like, if (IS_ERR(index)) dev->last_residency = 0; else ... Note: I haven't been through the CPUidle core in a while so maybe the above suggestion violates some other requirements/assumptions... > + > + return index; > +} > + > +void common_cpuidle_devices_uninit(void) > +{ > + int cpu_id; > + struct cpuidle_device *dev; > + > + for_each_possible_cpu(cpu_id) { > + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); > + cpuidle_unregister_device(dev); > + } > + > + free_percpu(common_cpuidle_devices); See note above about statically allocating the per-cpu variables. > +} > + > +/** > + * common_cpuidle_init() - Provides some commonly used init functionality. > + * @pdrv Pointer to your cpuidle_driver object. > + * @simple Use the common simple_enter wrapper? > + * @driver_data_init Pointer to your platform function to initialize your > + * platform specific driver data. Use NULL if platform > + * specific data is not needed. > + * > + * Common cpuidle init interface to provide common cpuidle functionality > + * used by many platforms. > + */ > +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple, > + void (*driver_data_init)(struct cpuidle_device *dev)) > +{ > + struct cpuidle_device *dev; > + struct cpuidle_driver *drv; > + int i, cpu_id, ret; > + > + if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) { > + pr_err("%s: Invalid Input\n", __func__); > + return -EINVAL; > + } > + > + drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL); > + if (!drv) { > + pr_err("%s: no memory for cpuidle driver\n", __func__); > + return -ENOMEM; > + } > + > + *drv = *pdrv; > + > + for (i = 0; simple && (i < drv->state_count); i++) { > + do_idle[i] = drv->states[i].enter; > + drv->states[i].enter = simple_enter; > + } > + > + ret = cpuidle_register_driver(drv); > + if (ret) { > + pr_err("%s: Failed to register cpuidle driver\n", __func__); > + goto free_drv; > + } > + > + common_cpuidle_devices = alloc_percpu(struct cpuidle_device); > + if (common_cpuidle_devices == NULL) { > + ret = -ENOMEM; > + goto unregister_drv; > + } See note above about statically allocating these. Regards, Mike
On 01/24/2012 05:37 AM, Robert Lee wrote: > The patch adds some cpuidle initialization functionality commonly > duplicated by many platforms. > > Signed-off-by: Robert Lee<rob.lee@linaro.org> > --- Hi Rob, nice work. The result is interesting. I have a few comments below. > drivers/cpuidle/Makefile | 2 +- > drivers/cpuidle/common.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/cpuidle.h | 24 +++++++ > 3 files changed, 177 insertions(+), 1 deletions(-) > create mode 100644 drivers/cpuidle/common.c > > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile > index 5634f88..2928d93 100644 > --- a/drivers/cpuidle/Makefile > +++ b/drivers/cpuidle/Makefile > @@ -2,4 +2,4 @@ > # Makefile for cpuidle. > # > > -obj-y += cpuidle.o driver.o governor.o sysfs.o governors/ > +obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/ > diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c > new file mode 100644 > index 0000000..dafa758 > --- /dev/null > +++ b/drivers/cpuidle/common.c > @@ -0,0 +1,152 @@ > +/* > + * Copyright 2011 Freescale Semiconductor, Inc. > + * Copyright 2011 Linaro Ltd. > + * > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > + > +/* > + * This code performs provides some commonly used cpuidle setup functionality > + * used by many ARM SoC platforms. Providing this functionality here > + * reduces the duplication of this code for each ARM platform that uses it. > + */ > + > +#include<linux/kernel.h> > +#include<linux/io.h> > +#include<linux/cpuidle.h> > +#include<linux/hrtimer.h> > +#include<linux/err.h> > +#include<linux/slab.h> > +#include<asm/proc-fns.h> > + > +static struct cpuidle_device __percpu * common_cpuidle_devices; > + > +static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index); > + > +int cpuidle_def_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + cpu_do_idle(); > + return index; > +} > + > +static int simple_enter(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + ktime_t time_start, time_end; > + > + local_irq_disable(); > + > + time_start = ktime_get(); > + > + index = do_idle[index](dev, drv, index); > + > + time_end = ktime_get(); > + > + local_irq_enable(); > + > + dev->last_residency = > + (int)ktime_to_us(ktime_sub(time_end, time_start)); > + > + return index; > +} > + > +void common_cpuidle_devices_uninit(void) > +{ > + int cpu_id; > + struct cpuidle_device *dev; > + > + for_each_possible_cpu(cpu_id) { > + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); > + cpuidle_unregister_device(dev); > + } > + > + free_percpu(common_cpuidle_devices); > +} If the registering sequence aborts, won't cpuidle_unregister_device leads to a kernel warning as it could be specified with a cpu which has *not* been registered yet ? Perhaps we should pass the cpuid from where the cpu has failed an do a reverse unregister sequence. void common_cpuidle_devices_uninit(int cpu) { for (cpu--; cpu >= 0; cpu--) { device = &per_cpu(common_cpuidle_devices, cpu); cpuidle_unregister_device(device); } ... > + > +/** > + * common_cpuidle_init() - Provides some commonly used init functionality. > + * @pdrv Pointer to your cpuidle_driver object. > + * @simple Use the common simple_enter wrapper? > + * @driver_data_init Pointer to your platform function to initialize your > + * platform specific driver data. Use NULL if platform > + * specific data is not needed. > + * > + * Common cpuidle init interface to provide common cpuidle functionality > + * used by many platforms. > + */ > +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple, > + void (*driver_data_init)(struct cpuidle_device *dev)) > +{ > + struct cpuidle_device *dev; > + struct cpuidle_driver *drv; > + int i, cpu_id, ret; > + > + if (!pdrv || pdrv->state_count> CPUIDLE_STATE_MAX) { > + pr_err("%s: Invalid Input\n", __func__); > + return -EINVAL; > + } > + > + drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL); > + if (!drv) { > + pr_err("%s: no memory for cpuidle driver\n", __func__); > + return -ENOMEM; > + } > + > + *drv = *pdrv; Rob can you explain why is needed to copy this structure ? Maybe kmemdup is more adequate here. drv = kmemdup(pdrv, sizeof(*drv), GFP_KERNEL); > + > + for (i = 0; simple&& (i< drv->state_count); i++) { > + do_idle[i] = drv->states[i].enter; > + drv->states[i].enter = simple_enter; > + } Do we really need a 'simple' parameter ? Is there an idle enter function which does not correspond to the 'simple' scheme except omap3/4 ? Maybe I am wrong but that looks a bit hacky because we are trying to override the functions the driver had previously defined and in order to prevent to modify the cpuidle.c core and more code. I am wondering if it is possible to move the usual: [ local_irq_disable(), getnstimeofday(before), myidle, getnstimeofday(after), local_irq_enable(), dev->last_residency = after-before, return index ] to cpuidle.c/cpuidle_idle_call and wrap the entered_state = target_state->enter(dev, drv, next_state) with these simple scheme. Also I am not sure local_irq_disable is needed because AFAICT the idle function is called with the local_irq_disable. For example, the intel_idle driver does not do that and assume the enter_idle function is called with the local irq disabled. Looking at the code : arch/arm/kernel/process.c : pm_idle is wrapped with local_irq_disable / local_irq_enable. arch/x86/kernel/process_32/64.c : pm_idle is called with local_irq_disable but assumes the function will enable local irq arch/ia64/kernel/process.c : the code assumes the idle function will disable/enable the local irq. etc ... It seems the code with the different arch is non consistent except there is a technical reason I don't know. May be making them consistent will help to factor out this part of the code and make the common framework more simple. It is just a suggestion and IMO that could be done later on top of this patchset. > + ret = cpuidle_register_driver(drv); > + if (ret) { > + pr_err("%s: Failed to register cpuidle driver\n", __func__); > + goto free_drv; > + } > + > + common_cpuidle_devices = alloc_percpu(struct cpuidle_device); > + if (common_cpuidle_devices == NULL) { > + ret = -ENOMEM; > + goto unregister_drv; > + } > + > + /* initialize state data for each cpuidle_device */ > + for_each_possible_cpu(cpu_id) { > + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); > + dev->cpu = cpu_id; > + dev->state_count = drv->state_count; > + > + if (driver_data_init) > + driver_data_init(dev); > + > + ret = cpuidle_register_device(dev); > + if (ret) { > + pr_err("%s: Failed to register cpu %u\n", > + __func__, cpu_id); > + goto uninit; > + } > + } > + > + return 0; > +uninit: > + > + common_cpuidle_devices_uninit(); > + > +unregister_drv: > + > + cpuidle_unregister_driver(drv); > + > +free_drv: > + > + kfree(drv); > + > + return ret; > +} > diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h > index 712abcc..2aa89db 100644 > --- a/include/linux/cpuidle.h > +++ b/include/linux/cpuidle.h > @@ -56,6 +56,16 @@ struct cpuidle_state { > > #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000) > > +/* Common ARM WFI state */ > +#define CPUIDLE_ARM_WFI_STATE {\ > + .enter = cpuidle_def_idle,\ > + .exit_latency = 2,\ > + .target_residency = 1,\ > + .flags = CPUIDLE_FLAG_TIME_VALID,\ > + .name = "WFI",\ > + .desc = "ARM core clock gating (WFI)",\ > +} > + > /** > * cpuidle_get_statedata - retrieves private driver state data > * @st_usage: the state usage statistics > @@ -141,6 +151,13 @@ extern void cpuidle_resume_and_unlock(void); > extern int cpuidle_enable_device(struct cpuidle_device *dev); > extern void cpuidle_disable_device(struct cpuidle_device *dev); > > +/* provide a default idle function */ > +extern int cpuidle_def_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index); > +extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple, > + void (*driver_data_init)(struct cpuidle_device *dev)); > +extern void common_cpuidle_devices_uninit(void); > + > #else > static inline void disable_cpuidle(void) { } > static inline int cpuidle_idle_call(void) { return -ENODEV; } > @@ -157,6 +174,13 @@ static inline void cpuidle_resume_and_unlock(void) { } > static inline int cpuidle_enable_device(struct cpuidle_device *dev) > {return -ENODEV; } > static inline void cpuidle_disable_device(struct cpuidle_device *dev) { } > +static inline int cpuidle_def_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{return -ENODEV; } > +static inline int common_cpuidle_init(struct cpuidle_driver *pdrv, > + bool simple, void (*driver_data_init)(struct cpuidle_device *dev)) > +{return -ENODEV; } > +static inline void common_cpuidle_devices_uninit(void) { } > > #endif >
Mike, thanks for your review. Comments below. On Tue, Jan 24, 2012 at 5:46 PM, Turquette, Mike <mturquette@ti.com> wrote: > On Mon, Jan 23, 2012 at 8:37 PM, Robert Lee <rob.lee@linaro.org> wrote: >> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c >> new file mode 100644 >> index 0000000..dafa758 >> --- /dev/null >> +++ b/drivers/cpuidle/common.c >> @@ -0,0 +1,152 @@ >> +/* >> + * Copyright 2011 Freescale Semiconductor, Inc. >> + * Copyright 2011 Linaro Ltd. >> + * >> + * The code contained herein is licensed under the GNU General Public >> + * License. You may obtain a copy of the GNU General Public License >> + * Version 2 or later at the following locations: >> + * >> + * http://www.opensource.org/licenses/gpl-license.html >> + * http://www.gnu.org/copyleft/gpl.html >> + */ >> + >> +/* >> + * This code performs provides some commonly used cpuidle setup functionality > > s/performs // > >> +static struct cpuidle_device __percpu * common_cpuidle_devices; > > You can use DECLARE_PER_CPU here. Ok. Do you mean that DECLARE_PER_CPU is preferred in this case? > > Is there any particular reason to allocate these dynamically? You can > replace the code above with, > > static DEFINE_PER_CPU(struct cpuidle_device, common_cpuidle_devices); > I was thinking of the single kernel with multiple platform support case. In that particular case, it seems better to create the number of device objects you need at run time. > I might change the variable name to "cpu_cpuidle_device" in that case > since you are addressing a single CPU when using the per cpu accessor > functions and "common_cpuidle_devices" sounds like an array or a list > or something. No big deal to keep the current name though. > >> +static int simple_enter(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + ktime_t time_start, time_end; >> + >> + local_irq_disable(); >> + >> + time_start = ktime_get(); >> + >> + index = do_idle[index](dev, drv, index); >> + >> + time_end = ktime_get(); >> + >> + local_irq_enable(); >> + >> + dev->last_residency = >> + (int)ktime_to_us(ktime_sub(time_end, time_start)); > > Sometimes an attempt to enter some C-state fails and the do_idle will > return immediately. What do you think about having do_idle return > -EERROR in this case and the conditionally setting last_residency to > zero in those cases? The point is that a C-state's total residency > time should not increase in the case where the hardware did not > successfully transition into that C-state. I've observed many times > where a specific low power state was actually achieved in the hardware > but /sys/devices/system/cpu/cpuN/cpuidle/stateM/time keeps > incrementing (albeit in very tiny increments). Something like, > > if (IS_ERR(index)) > dev->last_residency = 0; > else > ... > > Note: I haven't been through the CPUidle core in a while so maybe the > above suggestion violates some other requirements/assumptions... > Good suggestion. I'll look into adding this to v4. >> + >> + return index; >> +} >> + >> +void common_cpuidle_devices_uninit(void) >> +{ >> + int cpu_id; >> + struct cpuidle_device *dev; >> + >> + for_each_possible_cpu(cpu_id) { >> + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); >> + cpuidle_unregister_device(dev); >> + } >> + >> + free_percpu(common_cpuidle_devices); > > See note above about statically allocating the per-cpu variables. > >> +} >> + >> +/** >> + * common_cpuidle_init() - Provides some commonly used init functionality. >> + * @pdrv Pointer to your cpuidle_driver object. >> + * @simple Use the common simple_enter wrapper? >> + * @driver_data_init Pointer to your platform function to initialize your >> + * platform specific driver data. Use NULL if platform >> + * specific data is not needed. >> + * >> + * Common cpuidle init interface to provide common cpuidle functionality >> + * used by many platforms. >> + */ >> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple, >> + void (*driver_data_init)(struct cpuidle_device *dev)) >> +{ >> + struct cpuidle_device *dev; >> + struct cpuidle_driver *drv; >> + int i, cpu_id, ret; >> + >> + if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) { >> + pr_err("%s: Invalid Input\n", __func__); >> + return -EINVAL; >> + } >> + >> + drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL); >> + if (!drv) { >> + pr_err("%s: no memory for cpuidle driver\n", __func__); >> + return -ENOMEM; >> + } >> + >> + *drv = *pdrv; >> + >> + for (i = 0; simple && (i < drv->state_count); i++) { >> + do_idle[i] = drv->states[i].enter; >> + drv->states[i].enter = simple_enter; >> + } >> + >> + ret = cpuidle_register_driver(drv); >> + if (ret) { >> + pr_err("%s: Failed to register cpuidle driver\n", __func__); >> + goto free_drv; >> + } >> + >> + common_cpuidle_devices = alloc_percpu(struct cpuidle_device); >> + if (common_cpuidle_devices == NULL) { >> + ret = -ENOMEM; >> + goto unregister_drv; >> + } > > See note above about statically allocating these. > > Regards, > Mike > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Daniel, thanks for your review. I think you and Mike timed sending your responses :) Comments below. On Tue, Jan 24, 2012 at 5:49 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 01/24/2012 05:37 AM, Robert Lee wrote: >> >> The patch adds some cpuidle initialization functionality commonly >> duplicated by many platforms. >> >> Signed-off-by: Robert Lee<rob.lee@linaro.org> >> --- > > > Hi Rob, > > nice work. The result is interesting. I have a few comments below. > > > >> drivers/cpuidle/Makefile | 2 +- >> drivers/cpuidle/common.c | 152 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/cpuidle.h | 24 +++++++ >> 3 files changed, 177 insertions(+), 1 deletions(-) >> create mode 100644 drivers/cpuidle/common.c >> >> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile >> index 5634f88..2928d93 100644 >> --- a/drivers/cpuidle/Makefile >> +++ b/drivers/cpuidle/Makefile >> @@ -2,4 +2,4 @@ >> # Makefile for cpuidle. >> # >> >> -obj-y += cpuidle.o driver.o governor.o sysfs.o governors/ >> +obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/ >> diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c >> new file mode 100644 >> index 0000000..dafa758 >> --- /dev/null >> +++ b/drivers/cpuidle/common.c >> @@ -0,0 +1,152 @@ >> +/* >> + * Copyright 2011 Freescale Semiconductor, Inc. >> + * Copyright 2011 Linaro Ltd. >> + * >> + * The code contained herein is licensed under the GNU General Public >> + * License. You may obtain a copy of the GNU General Public License >> + * Version 2 or later at the following locations: >> + * >> + * http://www.opensource.org/licenses/gpl-license.html >> + * http://www.gnu.org/copyleft/gpl.html >> + */ >> + >> +/* >> + * This code performs provides some commonly used cpuidle setup >> functionality >> + * used by many ARM SoC platforms. Providing this functionality here >> + * reduces the duplication of this code for each ARM platform that uses >> it. >> + */ >> + >> +#include<linux/kernel.h> >> +#include<linux/io.h> >> +#include<linux/cpuidle.h> >> +#include<linux/hrtimer.h> >> +#include<linux/err.h> >> +#include<linux/slab.h> >> +#include<asm/proc-fns.h> >> + >> +static struct cpuidle_device __percpu * common_cpuidle_devices; >> + >> +static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index); >> + >> +int cpuidle_def_idle(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + cpu_do_idle(); >> + return index; >> +} >> + >> +static int simple_enter(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + ktime_t time_start, time_end; >> + >> + local_irq_disable(); >> + >> + time_start = ktime_get(); >> + >> + index = do_idle[index](dev, drv, index); >> + >> + time_end = ktime_get(); >> + >> + local_irq_enable(); >> + >> + dev->last_residency = >> + (int)ktime_to_us(ktime_sub(time_end, time_start)); >> + >> + return index; >> +} >> + >> +void common_cpuidle_devices_uninit(void) >> +{ >> + int cpu_id; >> + struct cpuidle_device *dev; >> + >> + for_each_possible_cpu(cpu_id) { >> + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); >> + cpuidle_unregister_device(dev); >> + } >> + >> + free_percpu(common_cpuidle_devices); >> +} > > > If the registering sequence aborts, won't cpuidle_unregister_device leads to > a kernel warning as it could be specified with a cpu which has *not* been > registered yet ? > I think you may have been looking at cpuidle_unregister_driver. Here is cpuidle_unregister_device which seems to handle a device not yet registered ok: void cpuidle_unregister_device(struct cpuidle_device *dev) { struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); if (dev->registered == 0) return; ... > Perhaps we should pass the cpuid from where the cpu has failed an do a > reverse unregister sequence. > > void common_cpuidle_devices_uninit(int cpu) > { > for (cpu--; cpu >= 0; cpu--) { > device = &per_cpu(common_cpuidle_devices, cpu); > cpuidle_unregister_device(device); > } > ... > > > >> + >> +/** >> + * common_cpuidle_init() - Provides some commonly used init >> functionality. >> + * @pdrv Pointer to your cpuidle_driver object. >> + * @simple Use the common simple_enter wrapper? >> + * @driver_data_init Pointer to your platform function to initialize >> your >> + * platform specific driver data. Use NULL if >> platform >> + * specific data is not needed. >> + * >> + * Common cpuidle init interface to provide common cpuidle functionality >> + * used by many platforms. >> + */ >> +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple, >> + void (*driver_data_init)(struct cpuidle_device >> *dev)) >> +{ >> + struct cpuidle_device *dev; >> + struct cpuidle_driver *drv; >> + int i, cpu_id, ret; >> + >> + if (!pdrv || pdrv->state_count> CPUIDLE_STATE_MAX) { >> + pr_err("%s: Invalid Input\n", __func__); >> + return -EINVAL; >> + } >> + >> + drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL); >> + if (!drv) { >> + pr_err("%s: no memory for cpuidle driver\n", __func__); >> + return -ENOMEM; >> + } >> + >> + *drv = *pdrv; > > > Rob can you explain why is needed to copy this structure ? > I made the original platform cpuidle_driver objects __initdata so I need to copy over to the dynamically allocated structure. > Maybe kmemdup is more adequate here. > > drv = kmemdup(pdrv, sizeof(*drv), GFP_KERNEL); > Is this preferred by the community over direct structure copies? Or is there some other advantage? >> + >> + for (i = 0; simple&& (i< drv->state_count); i++) { >> >> + do_idle[i] = drv->states[i].enter; >> + drv->states[i].enter = simple_enter; >> + } > > > Do we really need a 'simple' parameter ? Is there an idle enter function > which does not correspond to the 'simple' scheme except omap3/4 ? > > Maybe I am wrong but that looks a bit hacky because we are trying to > override the functions the driver had previously defined and in order to > prevent to modify the cpuidle.c core and more code. > > I am wondering if it is possible to move the usual: > > [ local_irq_disable(), getnstimeofday(before), myidle, > getnstimeofday(after), local_irq_enable(), dev->last_residency = > after-before, return index ] > > to cpuidle.c/cpuidle_idle_call and wrap the > entered_state = target_state->enter(dev, drv, next_state) > with these simple scheme. > Yes, I considered the same thing and originally made a version of this patch with direct changes to cpuidle_idle_call. But I concluded that since this common code's main purpose is just to consolidate code duplication on *some* (but not all) cpuidle implementations, it was better to create the extra simple_enter wrapper than to add additional code in cpuidle_idle_call that other platforms don't need. I'd be happy to submit a version of this patch with cpuidle_idle_call changes though and let the community decide. If anyone else thinks this is a good or bad idea, please give your input. > Also I am not sure local_irq_disable is needed because AFAICT the idle > function is called with the local_irq_disable. For example, the intel_idle > driver does not do that and assume the enter_idle function is called with > the local irq disabled. > > Looking at the code : > > arch/arm/kernel/process.c : pm_idle is wrapped with local_irq_disable / > local_irq_enable. > > arch/x86/kernel/process_32/64.c : pm_idle is called with local_irq_disable > but assumes the function will enable local irq > > arch/ia64/kernel/process.c : the code assumes the idle function will > disable/enable the local irq. > > etc ... > Agree. I considered this as well but ultimately decided to leave it in. I can remove it for the next patch version though. > It seems the code with the different arch is non consistent except there is > a technical reason I don't know. May be making them consistent will help to > factor out this part of the code and make the common framework more simple. > > It is just a suggestion and IMO that could be done later on top of this > patchset. > > > >> + ret = cpuidle_register_driver(drv); >> + if (ret) { >> + pr_err("%s: Failed to register cpuidle driver\n", >> __func__); >> + goto free_drv; >> + } >> + >> + common_cpuidle_devices = alloc_percpu(struct cpuidle_device); >> + if (common_cpuidle_devices == NULL) { >> + ret = -ENOMEM; >> + goto unregister_drv; >> + } >> + >> + /* initialize state data for each cpuidle_device */ >> + for_each_possible_cpu(cpu_id) { >> + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); >> + dev->cpu = cpu_id; >> + dev->state_count = drv->state_count; >> + >> + if (driver_data_init) >> + driver_data_init(dev); >> + >> + ret = cpuidle_register_device(dev); >> + if (ret) { >> + pr_err("%s: Failed to register cpu %u\n", >> + __func__, cpu_id); >> + goto uninit; >> + } >> + } >> + >> + return 0; >> +uninit: >> + >> + common_cpuidle_devices_uninit(); >> + >> +unregister_drv: >> + >> + cpuidle_unregister_driver(drv); >> + >> +free_drv: >> + >> + kfree(drv); >> + >> + return ret; >> +} >> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h >> index 712abcc..2aa89db 100644 >> --- a/include/linux/cpuidle.h >> +++ b/include/linux/cpuidle.h >> @@ -56,6 +56,16 @@ struct cpuidle_state { >> >> #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000) >> >> +/* Common ARM WFI state */ >> +#define CPUIDLE_ARM_WFI_STATE {\ >> + .enter = cpuidle_def_idle,\ >> + .exit_latency = 2,\ >> + .target_residency = 1,\ >> + .flags = CPUIDLE_FLAG_TIME_VALID,\ >> + .name = "WFI",\ >> + .desc = "ARM core clock gating (WFI)",\ >> +} >> + >> /** >> * cpuidle_get_statedata - retrieves private driver state data >> * @st_usage: the state usage statistics >> @@ -141,6 +151,13 @@ extern void cpuidle_resume_and_unlock(void); >> extern int cpuidle_enable_device(struct cpuidle_device *dev); >> extern void cpuidle_disable_device(struct cpuidle_device *dev); >> >> +/* provide a default idle function */ >> +extern int cpuidle_def_idle(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index); >> +extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple, >> + void (*driver_data_init)(struct cpuidle_device >> *dev)); >> +extern void common_cpuidle_devices_uninit(void); >> + >> #else >> static inline void disable_cpuidle(void) { } >> static inline int cpuidle_idle_call(void) { return -ENODEV; } >> @@ -157,6 +174,13 @@ static inline void cpuidle_resume_and_unlock(void) { >> } >> static inline int cpuidle_enable_device(struct cpuidle_device *dev) >> {return -ENODEV; } >> static inline void cpuidle_disable_device(struct cpuidle_device *dev) { } >> +static inline int cpuidle_def_idle(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{return -ENODEV; } >> +static inline int common_cpuidle_init(struct cpuidle_driver *pdrv, >> + bool simple, void (*driver_data_init)(struct cpuidle_device *dev)) >> +{return -ENODEV; } >> +static inline void common_cpuidle_devices_uninit(void) { } >> >> #endif >> > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog >
On 01/25/2012 03:38 AM, Rob Lee wrote: > Daniel, thanks for your review. I think you and Mike timed sending > your responses :) Comments below. [ ... ] >>> + for_each_possible_cpu(cpu_id) { >>> + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); >>> + cpuidle_unregister_device(dev); >>> + } >>> + >>> + free_percpu(common_cpuidle_devices); >>> +} >> >> >> If the registering sequence aborts, won't cpuidle_unregister_device leads to >> a kernel warning as it could be specified with a cpu which has *not* been >> registered yet ? >> > > I think you may have been looking at cpuidle_unregister_driver. Here > is cpuidle_unregister_device which seems to handle a device not yet > registered ok: > > void cpuidle_unregister_device(struct cpuidle_device *dev) > { > struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); > struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); > > if (dev->registered == 0) > return; > ... Ok, it is harmless. I could have looked at that ... :) >>> + >>> + drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL); >>> + if (!drv) { >>> + pr_err("%s: no memory for cpuidle driver\n", __func__); >>> + return -ENOMEM; >>> + } >>> + >>> + *drv = *pdrv; [ ... ] >> Rob can you explain why is needed to copy this structure ? >> > > I made the original platform cpuidle_driver objects __initdata so I > need to copy over to the dynamically allocated structure. Yes, but why declare a static object to be freed and allocate a new one and copy it ? Why don't just use the pdrv parameter of the function ? >> Maybe kmemdup is more adequate here. >> >> drv = kmemdup(pdrv, sizeof(*drv), GFP_KERNEL); >> > > Is this preferred by the community over direct structure copies? Or > is there some other advantage? It does kmalloc + memcpy. And *drv = *pdrv is converted to a memcpy by the compiler. So using kmemdup generates the same code as kmalloc + memcpy, or kmalloc + *drv = *pdrv >>> + >>> + for (i = 0; simple&& (i< drv->state_count); i++) { >>> >>> + do_idle[i] = drv->states[i].enter; >>> + drv->states[i].enter = simple_enter; >>> + } >> >> >> Do we really need a 'simple' parameter ? Is there an idle enter function >> which does not correspond to the 'simple' scheme except omap3/4 ? >> >> Maybe I am wrong but that looks a bit hacky because we are trying to >> override the functions the driver had previously defined and in order to >> prevent to modify the cpuidle.c core and more code. >> >> I am wondering if it is possible to move the usual: >> >> [ local_irq_disable(), getnstimeofday(before), myidle, >> getnstimeofday(after), local_irq_enable(), dev->last_residency = >> after-before, return index ] >> >> to cpuidle.c/cpuidle_idle_call and wrap the >> entered_state = target_state->enter(dev, drv, next_state) >> with these simple scheme. >> > > Yes, I considered the same thing and originally made a version of this > patch with direct changes to cpuidle_idle_call. But I concluded that > since this common code's main purpose is just to consolidate code > duplication on *some* (but not all) cpuidle implementations, it was > better to create the extra simple_enter wrapper than to add additional > code in cpuidle_idle_call that other platforms don't need. I'd be > happy to submit a version of this patch with cpuidle_idle_call changes > though and let the community decide. If anyone else thinks this is a > good or bad idea, please give your input. [1] >> Also I am not sure local_irq_disable is needed because AFAICT the idle >> function is called with the local_irq_disable. For example, the intel_idle >> driver does not do that and assume the enter_idle function is called with >> the local irq disabled. >> >> Looking at the code : >> >> arch/arm/kernel/process.c : pm_idle is wrapped with local_irq_disable / >> local_irq_enable. >> >> arch/x86/kernel/process_32/64.c : pm_idle is called with local_irq_disable >> but assumes the function will enable local irq >> >> arch/ia64/kernel/process.c : the code assumes the idle function will >> disable/enable the local irq. >> >> etc ... >> > > Agree. I considered this as well but ultimately decided to leave it > in. I can remove it for the next patch version though. IMHO, we should wait for what inputs we have in [1] Thanks -- Daniel
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile index 5634f88..2928d93 100644 --- a/drivers/cpuidle/Makefile +++ b/drivers/cpuidle/Makefile @@ -2,4 +2,4 @@ # Makefile for cpuidle. # -obj-y += cpuidle.o driver.o governor.o sysfs.o governors/ +obj-y += cpuidle.o driver.o governor.o sysfs.o common.o governors/ diff --git a/drivers/cpuidle/common.c b/drivers/cpuidle/common.c new file mode 100644 index 0000000..dafa758 --- /dev/null +++ b/drivers/cpuidle/common.c @@ -0,0 +1,152 @@ +/* + * Copyright 2011 Freescale Semiconductor, Inc. + * Copyright 2011 Linaro Ltd. + * + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ + +/* + * This code performs provides some commonly used cpuidle setup functionality + * used by many ARM SoC platforms. Providing this functionality here + * reduces the duplication of this code for each ARM platform that uses it. + */ + +#include <linux/kernel.h> +#include <linux/io.h> +#include <linux/cpuidle.h> +#include <linux/hrtimer.h> +#include <linux/err.h> +#include <linux/slab.h> +#include <asm/proc-fns.h> + +static struct cpuidle_device __percpu * common_cpuidle_devices; + +static int (*do_idle[CPUIDLE_STATE_MAX])(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index); + +int cpuidle_def_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + cpu_do_idle(); + return index; +} + +static int simple_enter(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + ktime_t time_start, time_end; + + local_irq_disable(); + + time_start = ktime_get(); + + index = do_idle[index](dev, drv, index); + + time_end = ktime_get(); + + local_irq_enable(); + + dev->last_residency = + (int)ktime_to_us(ktime_sub(time_end, time_start)); + + return index; +} + +void common_cpuidle_devices_uninit(void) +{ + int cpu_id; + struct cpuidle_device *dev; + + for_each_possible_cpu(cpu_id) { + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); + cpuidle_unregister_device(dev); + } + + free_percpu(common_cpuidle_devices); +} + +/** + * common_cpuidle_init() - Provides some commonly used init functionality. + * @pdrv Pointer to your cpuidle_driver object. + * @simple Use the common simple_enter wrapper? + * @driver_data_init Pointer to your platform function to initialize your + * platform specific driver data. Use NULL if platform + * specific data is not needed. + * + * Common cpuidle init interface to provide common cpuidle functionality + * used by many platforms. + */ +int __init common_cpuidle_init(struct cpuidle_driver *pdrv, bool simple, + void (*driver_data_init)(struct cpuidle_device *dev)) +{ + struct cpuidle_device *dev; + struct cpuidle_driver *drv; + int i, cpu_id, ret; + + if (!pdrv || pdrv->state_count > CPUIDLE_STATE_MAX) { + pr_err("%s: Invalid Input\n", __func__); + return -EINVAL; + } + + drv = kmalloc(sizeof(struct cpuidle_driver), GFP_KERNEL); + if (!drv) { + pr_err("%s: no memory for cpuidle driver\n", __func__); + return -ENOMEM; + } + + *drv = *pdrv; + + for (i = 0; simple && (i < drv->state_count); i++) { + do_idle[i] = drv->states[i].enter; + drv->states[i].enter = simple_enter; + } + + ret = cpuidle_register_driver(drv); + if (ret) { + pr_err("%s: Failed to register cpuidle driver\n", __func__); + goto free_drv; + } + + common_cpuidle_devices = alloc_percpu(struct cpuidle_device); + if (common_cpuidle_devices == NULL) { + ret = -ENOMEM; + goto unregister_drv; + } + + /* initialize state data for each cpuidle_device */ + for_each_possible_cpu(cpu_id) { + dev = per_cpu_ptr(common_cpuidle_devices, cpu_id); + dev->cpu = cpu_id; + dev->state_count = drv->state_count; + + if (driver_data_init) + driver_data_init(dev); + + ret = cpuidle_register_device(dev); + if (ret) { + pr_err("%s: Failed to register cpu %u\n", + __func__, cpu_id); + goto uninit; + } + } + + return 0; +uninit: + + common_cpuidle_devices_uninit(); + +unregister_drv: + + cpuidle_unregister_driver(drv); + +free_drv: + + kfree(drv); + + return ret; +} diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 712abcc..2aa89db 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -56,6 +56,16 @@ struct cpuidle_state { #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000) +/* Common ARM WFI state */ +#define CPUIDLE_ARM_WFI_STATE {\ + .enter = cpuidle_def_idle,\ + .exit_latency = 2,\ + .target_residency = 1,\ + .flags = CPUIDLE_FLAG_TIME_VALID,\ + .name = "WFI",\ + .desc = "ARM core clock gating (WFI)",\ +} + /** * cpuidle_get_statedata - retrieves private driver state data * @st_usage: the state usage statistics @@ -141,6 +151,13 @@ extern void cpuidle_resume_and_unlock(void); extern int cpuidle_enable_device(struct cpuidle_device *dev); extern void cpuidle_disable_device(struct cpuidle_device *dev); +/* provide a default idle function */ +extern int cpuidle_def_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index); +extern int common_cpuidle_init(struct cpuidle_driver *drv, bool simple, + void (*driver_data_init)(struct cpuidle_device *dev)); +extern void common_cpuidle_devices_uninit(void); + #else static inline void disable_cpuidle(void) { } static inline int cpuidle_idle_call(void) { return -ENODEV; } @@ -157,6 +174,13 @@ static inline void cpuidle_resume_and_unlock(void) { } static inline int cpuidle_enable_device(struct cpuidle_device *dev) {return -ENODEV; } static inline void cpuidle_disable_device(struct cpuidle_device *dev) { } +static inline int cpuidle_def_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{return -ENODEV; } +static inline int common_cpuidle_init(struct cpuidle_driver *pdrv, + bool simple, void (*driver_data_init)(struct cpuidle_device *dev)) +{return -ENODEV; } +static inline void common_cpuidle_devices_uninit(void) { } #endif
The patch adds some cpuidle initialization functionality commonly duplicated by many platforms. Signed-off-by: Robert Lee <rob.lee@linaro.org> --- drivers/cpuidle/Makefile | 2 +- drivers/cpuidle/common.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++ include/linux/cpuidle.h | 24 +++++++ 3 files changed, 177 insertions(+), 1 deletions(-) create mode 100644 drivers/cpuidle/common.c