| Message ID | 1334620214-25803-2-git-send-email-rob.lee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, 16 Apr 2012, Robert Lee wrote: > Add common cpuidle init functionality that can be used by various > imx platforms. > > Signed-off-by: Robert Lee <rob.lee@linaro.org> > --- > arch/arm/plat-mxc/Makefile | 2 + > arch/arm/plat-mxc/cpuidle.c | 89 ++++++++++++++++++++++++++++++ > arch/arm/plat-mxc/include/mach/cpuidle.h | 26 +++++++++ > 3 files changed, 117 insertions(+) > create mode 100644 arch/arm/plat-mxc/cpuidle.c > create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h > A few pedantic/ignorable comments below. > diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile > index e81290c..7c9e05f 100644 > --- a/arch/arm/plat-mxc/Makefile > +++ b/arch/arm/plat-mxc/Makefile > @@ -16,6 +16,8 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o > obj-$(CONFIG_MXC_USE_EPIT) += epit.o > obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o > obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o > +obj-$(CONFIG_CPU_IDLE) += cpuidle.o > + > ifdef CONFIG_SND_IMX_SOC > obj-y += ssi-fiq.o > obj-y += ssi-fiq-ksym.o > diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c > new file mode 100644 > index 0000000..d1c9301 > --- /dev/null > +++ b/arch/arm/plat-mxc/cpuidle.c > @@ -0,0 +1,89 @@ > +/* > + * Copyright 2012 Freescale Semiconductor, Inc. > + * Copyright 2012 Linaro Ltd. > + * > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > + > +#include <linux/kernel.h> > +#include <linux/io.h> > +#include <linux/cpuidle.h> > +#include <linux/hrtimer.h> > +#include <linux/err.h> > +#include <linux/slab.h> > + > +static struct cpuidle_device __percpu * imx_cpuidle_devices; > +static struct cpuidle_driver *drv; > + > +void __init imx_cpuidle_set_driver(struct cpuidle_driver *p) > +{ > + drv = p; > +} > + > +void imx_cpuidle_devices_uninit(void) > +{ > + int cpu_id; > + struct cpuidle_device *dev; > + > + for_each_possible_cpu(cpu_id) { > + dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id); > + cpuidle_unregister_device(dev); > + } > + > + free_percpu(imx_cpuidle_devices); > +} > + > +static int __init imx_cpuidle_init(void) > +{ > + struct cpuidle_device *dev; > + int cpu_id, ret; > + > + if (!drv || drv->state_count > CPUIDLE_STATE_MAX) { > + pr_err("%s: Invalid Input\n", __func__); > + return -EINVAL; > + } > + > + ret = cpuidle_register_driver(drv); > + Pointless blank line. I'd suggest removing it. > + if (ret) { > + pr_err("%s: Failed to register cpuidle driver\n", __func__); > + return ret; > + } > + > + imx_cpuidle_devices = alloc_percpu(struct cpuidle_device); > + Same here. Pointless blank line. > + if (imx_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(imx_cpuidle_devices, cpu_id); > + dev->cpu = cpu_id; > + dev->state_count = drv->state_count; > + > + ret = cpuidle_register_device(dev); > + if (ret) { > + pr_err("%s: Failed to register cpu %u\n", > + __func__, cpu_id); > + goto uninit; > + } > + } > + > + return 0; > + > +uninit: > + imx_cpuidle_devices_uninit(); > + > +unregister_drv: > + cpuidle_unregister_driver(drv); > + Blank line here seems pointless. > + return ret; > +} > +late_initcall(imx_cpuidle_init); > diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h > new file mode 100644 > index 0000000..6509f19 > --- /dev/null > +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h > @@ -0,0 +1,26 @@ > +/* > + * Copyright 2012 Freescale Semiconductor, Inc. > + * Copyright 2012 Linaro Ltd. > + * > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > + > +#include <linux/cpuidle.h> > + > +extern int imx5_cpuidle_init(void); > +extern int imx6q_cpuidle_init(void); > + > +#ifdef CONFIG_CPU_IDLE > +extern void imx_cpuidle_devices_uninit(void); > +extern void imx_cpuidle_set_driver(struct cpuidle_driver *p); > +extern int imx5_cpuidle_init(void); > +extern int imx6q_cpuidle_init(void); > +#else > +static inline void imx_cpuidle_devices_uninit(void) {} > +static inline void imx_cpuidle_set_driver(struct cpuidle_driver *p) {} > +#endif >
On Mon, Apr 16, 2012 at 06:50:12PM -0500, Robert Lee wrote: > Add common cpuidle init functionality that can be used by various > imx platforms. > > Signed-off-by: Robert Lee <rob.lee@linaro.org> > --- > diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile > index e81290c..7c9e05f 100644 > --- a/arch/arm/plat-mxc/Makefile > +++ b/arch/arm/plat-mxc/Makefile > @@ -16,6 +16,8 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o > obj-$(CONFIG_MXC_USE_EPIT) += epit.o > obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o > obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o > +obj-$(CONFIG_CPU_IDLE) += cpuidle.o > + > ifdef CONFIG_SND_IMX_SOC > obj-y += ssi-fiq.o > obj-y += ssi-fiq-ksym.o > diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c > new file mode 100644 > index 0000000..d1c9301 > --- /dev/null > +++ b/arch/arm/plat-mxc/cpuidle.c > @@ -0,0 +1,89 @@ > +/* > + * Copyright 2012 Freescale Semiconductor, Inc. > + * Copyright 2012 Linaro Ltd. > + * > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > + > +#include <linux/kernel.h> > +#include <linux/io.h> > +#include <linux/cpuidle.h> > +#include <linux/hrtimer.h> > +#include <linux/err.h> > +#include <linux/slab.h> > + > +static struct cpuidle_device __percpu * imx_cpuidle_devices; > +static struct cpuidle_driver *drv; > + > +void __init imx_cpuidle_set_driver(struct cpuidle_driver *p) > +{ > + drv = p; > +} You like it complicated, eh? Why do you introduce a function which sets a variable... > + > +void imx_cpuidle_devices_uninit(void) > +{ > + int cpu_id; > + struct cpuidle_device *dev; > + > + for_each_possible_cpu(cpu_id) { > + dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id); > + cpuidle_unregister_device(dev); > + } > + > + free_percpu(imx_cpuidle_devices); > +} > + > +static int __init imx_cpuidle_init(void) ... instead of passing it directly to the function which uses it? > +{ > + struct cpuidle_device *dev; > + int cpu_id, ret; > + > + if (!drv || drv->state_count > CPUIDLE_STATE_MAX) { > + pr_err("%s: Invalid Input\n", __func__); You introduce a pointless error message on SoCs not setting a cpuidle driver. When will people learn that initcalls do not only run on machines they have on their desk? > + return -EINVAL; > + } > + > + ret = cpuidle_register_driver(drv); > + > + if (ret) { > + pr_err("%s: Failed to register cpuidle driver\n", __func__); It's always nice to add the return value to the error message to get a clue *what* went wrong. The function name though is rather uninteresting. Sascha
On Tue, Apr 17, 2012 at 2:43 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Mon, Apr 16, 2012 at 06:50:12PM -0500, Robert Lee wrote: >> Add common cpuidle init functionality that can be used by various >> imx platforms. >> >> Signed-off-by: Robert Lee <rob.lee@linaro.org> >> --- >> diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile >> index e81290c..7c9e05f 100644 >> --- a/arch/arm/plat-mxc/Makefile >> +++ b/arch/arm/plat-mxc/Makefile >> @@ -16,6 +16,8 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o >> obj-$(CONFIG_MXC_USE_EPIT) += epit.o >> obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o >> obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o >> +obj-$(CONFIG_CPU_IDLE) += cpuidle.o >> + >> ifdef CONFIG_SND_IMX_SOC >> obj-y += ssi-fiq.o >> obj-y += ssi-fiq-ksym.o >> diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c >> new file mode 100644 >> index 0000000..d1c9301 >> --- /dev/null >> +++ b/arch/arm/plat-mxc/cpuidle.c >> @@ -0,0 +1,89 @@ >> +/* >> + * Copyright 2012 Freescale Semiconductor, Inc. >> + * Copyright 2012 Linaro Ltd. >> + * >> + * The code contained herein is licensed under the GNU General Public >> + * License. You may obtain a copy of the GNU General Public License >> + * Version 2 or later at the following locations: >> + * >> + * http://www.opensource.org/licenses/gpl-license.html >> + * http://www.gnu.org/copyleft/gpl.html >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/io.h> >> +#include <linux/cpuidle.h> >> +#include <linux/hrtimer.h> >> +#include <linux/err.h> >> +#include <linux/slab.h> >> + >> +static struct cpuidle_device __percpu * imx_cpuidle_devices; >> +static struct cpuidle_driver *drv; >> + >> +void __init imx_cpuidle_set_driver(struct cpuidle_driver *p) >> +{ >> + drv = p; >> +} > > You like it complicated, eh? Why do you introduce a function which sets > a variable... > This complication is used to deal with the timing of various levels of init calls. More explanation below. >> + >> +void imx_cpuidle_devices_uninit(void) >> +{ >> + int cpu_id; >> + struct cpuidle_device *dev; >> + >> + for_each_possible_cpu(cpu_id) { >> + dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id); >> + cpuidle_unregister_device(dev); >> + } >> + >> + free_percpu(imx_cpuidle_devices); >> +} >> + >> +static int __init imx_cpuidle_init(void) > > ... instead of passing it directly to the function which uses it? > If I called imx_cpuidle_init directly from imx5 or imx6q init routines, it would be getting called before the coreinit_call of core cpuidle causing a failure. There were various other directions to take and all seemed less desirable than this one. One alternative would be to add a function to return the pointer to the cpuidle driver object based on the machine type. Functionality exists to identify imx5 as a machine type but not imx6q, so I couldn't use that machine based method without adding that extra code. Another alternative would be to add a general platform lateinit_call function to each platforms that support cpuidle. >> +{ >> + struct cpuidle_device *dev; >> + int cpu_id, ret; >> + >> + if (!drv || drv->state_count > CPUIDLE_STATE_MAX) { >> + pr_err("%s: Invalid Input\n", __func__); > > You introduce a pointless error message on SoCs not setting a cpuidle > driver. When will people learn that initcalls do not only run on > machines they have on their desk? > Will fix in next version. >> + return -EINVAL; >> + } >> + >> + ret = cpuidle_register_driver(drv); >> + >> + if (ret) { >> + pr_err("%s: Failed to register cpuidle driver\n", __func__); > > It's always nice to add the return value to the error message to get a > clue *what* went wrong. The function name though is rather > uninteresting. Will add the return value in the next version. > > 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 Tue, Apr 17, 2012 at 08:54:03AM -0500, Rob Lee wrote: > >> +void __init imx_cpuidle_set_driver(struct cpuidle_driver *p) > >> +{ > >> + drv = p; > >> +} > > > > You like it complicated, eh? Why do you introduce a function which sets > > a variable... > > This complication is used to deal with the timing of various levels of > init calls. More explanation below. Regardless of how you end up solving this, it's probably a good idea to document the rationale, perhaps cribbing from what you describe below.. > If I called imx_cpuidle_init directly from imx5 or imx6q init > routines, it would be getting called before the coreinit_call of core > cpuidle causing a failure. There were various other directions to > take and all seemed less desirable than this one. > > One alternative would be to add a function to return the pointer to > the cpuidle driver object based on the machine type. Functionality > exists to identify imx5 as a machine type but not imx6q, so I couldn't > use that machine based method without adding that extra code. > > Another alternative would be to add a general platform lateinit_call > function to each platforms that support cpuidle. ..in a comment; without it, the code indeed looks bizarre.
On Tue, Apr 17, 2012 at 9:13 AM, Christian Robottom Reis <kiko@linaro.org> wrote: > On Tue, Apr 17, 2012 at 08:54:03AM -0500, Rob Lee wrote: >> >> +void __init imx_cpuidle_set_driver(struct cpuidle_driver *p) >> >> +{ >> >> + drv = p; >> >> +} >> > >> > You like it complicated, eh? Why do you introduce a function which sets >> > a variable... >> >> This complication is used to deal with the timing of various levels of >> init calls. More explanation below. > > Regardless of how you end up solving this, it's probably a good idea > to document the rationale, perhaps cribbing from what you describe > below.. Agree. Adding comments to the function itself seems to be the most relevant location so I can add that information there if the function remains. > >> If I called imx_cpuidle_init directly from imx5 or imx6q init >> routines, it would be getting called before the coreinit_call of core >> cpuidle causing a failure. There were various other directions to >> take and all seemed less desirable than this one. >> >> One alternative would be to add a function to return the pointer to >> the cpuidle driver object based on the machine type. Functionality >> exists to identify imx5 as a machine type but not imx6q, so I couldn't >> use that machine based method without adding that extra code. >> >> Another alternative would be to add a general platform lateinit_call >> function to each platforms that support cpuidle. > > ..in a comment; without it, the code indeed looks bizarre. > -- > Christian Robottom Reis, Engineering VP > Brazil (GMT-3) | [+55] 16 9112 6430 | [+1] 612 216 4935 > Linaro.org: Open Source Software for ARM SoCs
On Tue, Apr 17, 2012 at 08:54:03AM -0500, Rob Lee wrote: > On Tue, Apr 17, 2012 at 2:43 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > On Mon, Apr 16, 2012 at 06:50:12PM -0500, Robert Lee wrote: > >> Add common cpuidle init functionality that can be used by various > >> imx platforms. > >> > >> Signed-off-by: Robert Lee <rob.lee@linaro.org> > >> --- > >> diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile > >> index e81290c..7c9e05f 100644 > >> --- a/arch/arm/plat-mxc/Makefile > >> +++ b/arch/arm/plat-mxc/Makefile > >> @@ -16,6 +16,8 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o > >> obj-$(CONFIG_MXC_USE_EPIT) += epit.o > >> obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o > >> obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o > >> +obj-$(CONFIG_CPU_IDLE) += cpuidle.o > >> + > >> ifdef CONFIG_SND_IMX_SOC > >> obj-y += ssi-fiq.o > >> obj-y += ssi-fiq-ksym.o > >> diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c > >> new file mode 100644 > >> index 0000000..d1c9301 > >> --- /dev/null > >> +++ b/arch/arm/plat-mxc/cpuidle.c > >> @@ -0,0 +1,89 @@ > >> +/* > >> + * Copyright 2012 Freescale Semiconductor, Inc. > >> + * Copyright 2012 Linaro Ltd. > >> + * > >> + * The code contained herein is licensed under the GNU General Public > >> + * License. You may obtain a copy of the GNU General Public License > >> + * Version 2 or later at the following locations: > >> + * > >> + * http://www.opensource.org/licenses/gpl-license.html > >> + * http://www.gnu.org/copyleft/gpl.html > >> + */ > >> + > >> +#include <linux/kernel.h> > >> +#include <linux/io.h> > >> +#include <linux/cpuidle.h> > >> +#include <linux/hrtimer.h> > >> +#include <linux/err.h> > >> +#include <linux/slab.h> > >> + > >> +static struct cpuidle_device __percpu * imx_cpuidle_devices; > >> +static struct cpuidle_driver *drv; > >> + > >> +void __init imx_cpuidle_set_driver(struct cpuidle_driver *p) > >> +{ > >> + drv = p; > >> +} > > > > You like it complicated, eh? Why do you introduce a function which sets > > a variable... > > > > This complication is used to deal with the timing of various levels of > init calls. More explanation below. > > >> + > >> +void imx_cpuidle_devices_uninit(void) > >> +{ > >> + int cpu_id; > >> + struct cpuidle_device *dev; > >> + > >> + for_each_possible_cpu(cpu_id) { > >> + dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id); > >> + cpuidle_unregister_device(dev); > >> + } > >> + > >> + free_percpu(imx_cpuidle_devices); > >> +} > >> + > >> +static int __init imx_cpuidle_init(void) > > > > ... instead of passing it directly to the function which uses it? > > > > If I called imx_cpuidle_init directly from imx5 or imx6q init > routines, it would be getting called before the coreinit_call of core > cpuidle causing a failure. There were various other directions to > take and all seemed less desirable than this one. > > One alternative would be to add a function to return the pointer to > the cpuidle driver object based on the machine type. Functionality > exists to identify imx5 as a machine type but not imx6q, so I couldn't > use that machine based method without adding that extra code. > > Another alternative would be to add a general platform lateinit_call > function to each platforms that support cpuidle. Just put the initcall into mm-imx5.c and check the cpu type. Then you also don't have to make imx5_idle global. Sascha
>> >> +void __init imx_cpuidle_set_driver(struct cpuidle_driver *p) >> >> +{ >> >> + drv = p; >> >> +} >> > >> > You like it complicated, eh? Why do you introduce a function which sets >> > a variable... >> > >> >> This complication is used to deal with the timing of various levels of >> init calls. More explanation below. >> >> >> + >> >> +void imx_cpuidle_devices_uninit(void) >> >> +{ >> >> + int cpu_id; >> >> + struct cpuidle_device *dev; >> >> + >> >> + for_each_possible_cpu(cpu_id) { >> >> + dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id); >> >> + cpuidle_unregister_device(dev); >> >> + } >> >> + >> >> + free_percpu(imx_cpuidle_devices); >> >> +} >> >> + >> >> +static int __init imx_cpuidle_init(void) >> > >> > ... instead of passing it directly to the function which uses it? >> > >> >> If I called imx_cpuidle_init directly from imx5 or imx6q init >> routines, it would be getting called before the coreinit_call of core >> cpuidle causing a failure. There were various other directions to >> take and all seemed less desirable than this one. >> >> One alternative would be to add a function to return the pointer to >> the cpuidle driver object based on the machine type. Functionality >> exists to identify imx5 as a machine type but not imx6q, so I couldn't >> use that machine based method without adding that extra code. >> >> Another alternative would be to add a general platform lateinit_call >> function to each platforms that support cpuidle. > > Just put the initcall into mm-imx5.c and check the cpu type. Then you > also don't have to make imx5_idle global. That solution is currently available for imx5 but for imx6q it implies adding the cpu type support for imx6q. Are you ok with that? > > 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 |
>>> If I called imx_cpuidle_init directly from imx5 or imx6q init >>> routines, it would be getting called before the coreinit_call of core >>> cpuidle causing a failure. There were various other directions to >>> take and all seemed less desirable than this one. >>> >>> One alternative would be to add a function to return the pointer to >>> the cpuidle driver object based on the machine type. Functionality >>> exists to identify imx5 as a machine type but not imx6q, so I couldn't >>> use that machine based method without adding that extra code. >>> >>> Another alternative would be to add a general platform lateinit_call >>> function to each platforms that support cpuidle. >> >> Just put the initcall into mm-imx5.c and check the cpu type. Then you >> also don't have to make imx5_idle global. > > That solution is currently available for imx5 but for imx6q it implies > adding the cpu type support for imx6q. Are you ok with that? Sascha or Shawn, any further comments on my question? Thanks, Rob
On Wed, Apr 18, 2012 at 11:18:55PM -0500, Rob Lee wrote: > >>> If I called imx_cpuidle_init directly from imx5 or imx6q init > >>> routines, it would be getting called before the coreinit_call of core > >>> cpuidle causing a failure. There were various other directions to > >>> take and all seemed less desirable than this one. > >>> > >>> One alternative would be to add a function to return the pointer to > >>> the cpuidle driver object based on the machine type. Functionality > >>> exists to identify imx5 as a machine type but not imx6q, so I couldn't > >>> use that machine based method without adding that extra code. > >>> > >>> Another alternative would be to add a general platform lateinit_call > >>> function to each platforms that support cpuidle. > >> > >> Just put the initcall into mm-imx5.c and check the cpu type. Then you > >> also don't have to make imx5_idle global. > > > > That solution is currently available for imx5 but for imx6q it implies > > adding the cpu type support for imx6q. Are you ok with that? > > Sascha or Shawn, any further comments on my question? I don't think we need a cpu_is_imx6q(), but having some i.MX6 specific hook at device_initcall time can't be too wrong. Shawn? Sascha
On Thu, Apr 19, 2012 at 08:43:08AM +0200, Sascha Hauer wrote: > > Sascha or Shawn, any further comments on my question? > Sorry for the late response, Rob. > I don't think we need a cpu_is_imx6q(), but having some i.MX6 specific > hook at device_initcall time can't be too wrong. Shawn? > Yep, it works for me.
>> I don't think we need a cpu_is_imx6q(), but having some i.MX6 specific >> hook at device_initcall time can't be too wrong. Shawn? >> > Yep, it works for me. > Sascha, Shawn, thanks for the response. Since device_initcall isn't platform specific, it seems I would still need a cpu_is_imx6q() function or similiar functionality from a device tree call. Or do you have something else in mind that I'm not seeing? Thanks, Rob > -- > Regards, > Shawn
On Sun, Apr 22, 2012 at 11:44:39PM -0500, Rob Lee wrote: > >> I don't think we need a cpu_is_imx6q(), but having some i.MX6 specific > >> hook at device_initcall time can't be too wrong. Shawn? > >> > > Yep, it works for me. > > > Sascha, Shawn, thanks for the response. > > Since device_initcall isn't platform specific, it seems I would still > need a cpu_is_imx6q() function or similiar functionality from a device > tree call. Or do you have something else in mind that I'm not seeing? > I guess Sascha is asking for something like the following. static int __init imx_device_init(void) { imx5_device_init(); imx6_device_init(); } device_initcall(imx_device_init) static int __init imx6_device_init(void) { /* * do whatever needs to get done in device_initcall time */ }
On Mon, Apr 23, 2012 at 01:18:21PM +0800, Shawn Guo wrote: > On Sun, Apr 22, 2012 at 11:44:39PM -0500, Rob Lee wrote: > > >> I don't think we need a cpu_is_imx6q(), but having some i.MX6 specific > > >> hook at device_initcall time can't be too wrong. Shawn? > > >> > > > Yep, it works for me. > > > > > Sascha, Shawn, thanks for the response. > > > > Since device_initcall isn't platform specific, it seems I would still > > need a cpu_is_imx6q() function or similiar functionality from a device > > tree call. Or do you have something else in mind that I'm not seeing? > > > I guess Sascha is asking for something like the following. > > static int __init imx_device_init(void) > { > imx5_device_init(); > imx6_device_init(); > } > device_initcall(imx_device_init) > > static int __init imx6_device_init(void) > { > /* > * do whatever needs to get done in device_initcall time > */ > } The problem is more how we can detect that we are actually running a i.MX6 SoC. We could directly ask the devicetree in an initcall or we could introduce a cpu_is_mx6() just like we have a macro for all other i.MX SoCs. Sascha
On Mon, Apr 23, 2012 at 08:27:39AM +0200, Sascha Hauer wrote: > On Mon, Apr 23, 2012 at 01:18:21PM +0800, Shawn Guo wrote: > > On Sun, Apr 22, 2012 at 11:44:39PM -0500, Rob Lee wrote: > > > >> I don't think we need a cpu_is_imx6q(), but having some i.MX6 specific > > > >> hook at device_initcall time can't be too wrong. Shawn? > > > >> > > > > Yep, it works for me. > > > > > > > Sascha, Shawn, thanks for the response. > > > > > > Since device_initcall isn't platform specific, it seems I would still > > > need a cpu_is_imx6q() function or similiar functionality from a device > > > tree call. Or do you have something else in mind that I'm not seeing? > > > > > I guess Sascha is asking for something like the following. > > > > static int __init imx_device_init(void) > > { > > imx5_device_init(); > > imx6_device_init(); > > } > > device_initcall(imx_device_init) > > > > static int __init imx6_device_init(void) > > { > > /* > > * do whatever needs to get done in device_initcall time > > */ > > } > > The problem is more how we can detect that we are actually running a > i.MX6 SoC. We could directly ask the devicetree in an initcall or we > could introduce a cpu_is_mx6() just like we have a macro for all other > i.MX SoCs. > Oops, my reply was a brain-dead one. Hmm, then it seems that we have to introduce cpu_is_mx6() which I tried hard to avoid. I do not have a preference between defining a macro and asking device tree.
On Mon, Apr 23, 2012 at 02:53:01PM +0800, Shawn Guo wrote: > On Mon, Apr 23, 2012 at 08:27:39AM +0200, Sascha Hauer wrote: > > On Mon, Apr 23, 2012 at 01:18:21PM +0800, Shawn Guo wrote: > > > On Sun, Apr 22, 2012 at 11:44:39PM -0500, Rob Lee wrote: > > > > >> I don't think we need a cpu_is_imx6q(), but having some i.MX6 specific > > > > >> hook at device_initcall time can't be too wrong. Shawn? > > > > >> > > > > > Yep, it works for me. > > > > > > > > > Sascha, Shawn, thanks for the response. > > > > > > > > Since device_initcall isn't platform specific, it seems I would still > > > > need a cpu_is_imx6q() function or similiar functionality from a device > > > > tree call. Or do you have something else in mind that I'm not seeing? > > > > > > > I guess Sascha is asking for something like the following. > > > > > > static int __init imx_device_init(void) > > > { > > > imx5_device_init(); > > > imx6_device_init(); > > > } > > > device_initcall(imx_device_init) > > > > > > static int __init imx6_device_init(void) > > > { > > > /* > > > * do whatever needs to get done in device_initcall time > > > */ > > > } > > > > The problem is more how we can detect that we are actually running a > > i.MX6 SoC. We could directly ask the devicetree in an initcall or we > > could introduce a cpu_is_mx6() just like we have a macro for all other > > i.MX SoCs. > > > Oops, my reply was a brain-dead one. Hmm, then it seems that we have > to introduce cpu_is_mx6() which I tried hard to avoid. I do not have > a preference between defining a macro and asking device tree. Since we already have a place in early setup code in which we know that we are running on an i.MX6 I suggest for the sake of the symmetry of the universe that we introduce a cpu_is_mx6. Sascha
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile index e81290c..7c9e05f 100644 --- a/arch/arm/plat-mxc/Makefile +++ b/arch/arm/plat-mxc/Makefile @@ -16,6 +16,8 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o obj-$(CONFIG_MXC_USE_EPIT) += epit.o obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o +obj-$(CONFIG_CPU_IDLE) += cpuidle.o + ifdef CONFIG_SND_IMX_SOC obj-y += ssi-fiq.o obj-y += ssi-fiq-ksym.o diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c new file mode 100644 index 0000000..d1c9301 --- /dev/null +++ b/arch/arm/plat-mxc/cpuidle.c @@ -0,0 +1,89 @@ +/* + * Copyright 2012 Freescale Semiconductor, Inc. + * Copyright 2012 Linaro Ltd. + * + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ + +#include <linux/kernel.h> +#include <linux/io.h> +#include <linux/cpuidle.h> +#include <linux/hrtimer.h> +#include <linux/err.h> +#include <linux/slab.h> + +static struct cpuidle_device __percpu * imx_cpuidle_devices; +static struct cpuidle_driver *drv; + +void __init imx_cpuidle_set_driver(struct cpuidle_driver *p) +{ + drv = p; +} + +void imx_cpuidle_devices_uninit(void) +{ + int cpu_id; + struct cpuidle_device *dev; + + for_each_possible_cpu(cpu_id) { + dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id); + cpuidle_unregister_device(dev); + } + + free_percpu(imx_cpuidle_devices); +} + +static int __init imx_cpuidle_init(void) +{ + struct cpuidle_device *dev; + int cpu_id, ret; + + if (!drv || drv->state_count > CPUIDLE_STATE_MAX) { + pr_err("%s: Invalid Input\n", __func__); + return -EINVAL; + } + + ret = cpuidle_register_driver(drv); + + if (ret) { + pr_err("%s: Failed to register cpuidle driver\n", __func__); + return ret; + } + + imx_cpuidle_devices = alloc_percpu(struct cpuidle_device); + + if (imx_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(imx_cpuidle_devices, cpu_id); + dev->cpu = cpu_id; + dev->state_count = drv->state_count; + + ret = cpuidle_register_device(dev); + if (ret) { + pr_err("%s: Failed to register cpu %u\n", + __func__, cpu_id); + goto uninit; + } + } + + return 0; + +uninit: + imx_cpuidle_devices_uninit(); + +unregister_drv: + cpuidle_unregister_driver(drv); + + return ret; +} +late_initcall(imx_cpuidle_init); diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h new file mode 100644 index 0000000..6509f19 --- /dev/null +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h @@ -0,0 +1,26 @@ +/* + * Copyright 2012 Freescale Semiconductor, Inc. + * Copyright 2012 Linaro Ltd. + * + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ + +#include <linux/cpuidle.h> + +extern int imx5_cpuidle_init(void); +extern int imx6q_cpuidle_init(void); + +#ifdef CONFIG_CPU_IDLE +extern void imx_cpuidle_devices_uninit(void); +extern void imx_cpuidle_set_driver(struct cpuidle_driver *p); +extern int imx5_cpuidle_init(void); +extern int imx6q_cpuidle_init(void); +#else +static inline void imx_cpuidle_devices_uninit(void) {} +static inline void imx_cpuidle_set_driver(struct cpuidle_driver *p) {} +#endif
Add common cpuidle init functionality that can be used by various imx platforms. Signed-off-by: Robert Lee <rob.lee@linaro.org> --- arch/arm/plat-mxc/Makefile | 2 + arch/arm/plat-mxc/cpuidle.c | 89 ++++++++++++++++++++++++++++++ arch/arm/plat-mxc/include/mach/cpuidle.h | 26 +++++++++ 3 files changed, 117 insertions(+) create mode 100644 arch/arm/plat-mxc/cpuidle.c create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h