Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q platform cpuidle implementation.
Based on v3.4-rc5 plus recently submitted device tree late_initcall patch: http://www.spinics.net/lists/arm-kernel/msg171620.html
Changes since v1: * Removed some unnecessary spaces * Added return value for an error message * Reworked init scheme to use device tree late_initcall. * Moved imx6q and imx5 cpuidle functionality to existing files.
Robert Lee (3): ARM: imx: Add common imx cpuidle init functionality. ARM: imx: Add imx5 cpuidle driver ARM: imx: Add imx6q cpuidle driver
arch/arm/mach-imx/cpuidle-imx6q.c | 33 ++++++++++++ arch/arm/mach-imx/mach-imx6q.c | 18 +++++++ arch/arm/mach-imx/mm-imx5.c | 42 ++++++++++++++-- arch/arm/plat-mxc/Makefile | 1 + arch/arm/plat-mxc/cpuidle.c | 80 ++++++++++++++++++++++++++++++ arch/arm/plat-mxc/include/mach/cpuidle.h | 22 ++++++++ 6 files changed, 193 insertions(+), 3 deletions(-) create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c create mode 100644 arch/arm/plat-mxc/cpuidle.c create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
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 | 1 + arch/arm/plat-mxc/cpuidle.c | 80 ++++++++++++++++++++++++++++++ arch/arm/plat-mxc/include/mach/cpuidle.h | 22 ++++++++ 3 files changed, 103 insertions(+) create mode 100644 arch/arm/plat-mxc/cpuidle.c create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile index e81290c..63b064b 100644 --- a/arch/arm/plat-mxc/Makefile +++ b/arch/arm/plat-mxc/Makefile @@ -16,6 +16,7 @@ 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..b7a5e1c --- /dev/null +++ b/arch/arm/plat-mxc/cpuidle.c @@ -0,0 +1,80 @@ +/* + * 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; + +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); +} + +int __init imx_cpuidle_init(struct cpuidle_driver *drv) +{ + 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 with error: %d\n", + __func__, ret); + 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; +} 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..8612510 --- /dev/null +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h @@ -0,0 +1,22 @@ +/* + * 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> + +#ifdef CONFIG_CPU_IDLE +extern void imx_cpuidle_devices_uninit(void); +extern int imx_cpuidle_init(struct cpuidle_driver *drv); +#else +static inline void imx_cpuidle_devices_uninit(void) {} +static inline int imx_cpuidle_init(struct cpuidle_driver *drv) +{ return -ENODEV; } +#endif
Add imx5 cpuidle driver.
Signed-off-by: Robert Lee rob.lee@linaro.org --- arch/arm/mach-imx/mm-imx5.c | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c index d6b7e9f..cbd9bad 100644 --- a/arch/arm/mach-imx/mm-imx5.c +++ b/arch/arm/mach-imx/mm-imx5.c @@ -22,24 +22,59 @@ #include <mach/common.h> #include <mach/devices-common.h> #include <mach/iomux-v3.h> +#include <mach/cpuidle.h>
static struct clk *gpc_dvfs_clk;
-static void imx5_idle(void) +static int imx5_idle(void) { + int ret = 0; + /* gpc clock is needed for SRPG */ if (gpc_dvfs_clk == NULL) { gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs"); if (IS_ERR(gpc_dvfs_clk)) - return; + return -ENODEV; } clk_enable(gpc_dvfs_clk); mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF); if (!tzic_enable_wake()) cpu_do_idle(); + else + ret = -EBUSY; clk_disable(gpc_dvfs_clk); + + return ret; +} + +static int imx5_cpuidle_enter(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int idx) +{ + int ret; + + ret = imx5_idle(); + + if (ret < 0) + return ret; + + return idx; }
+static struct cpuidle_driver imx5_cpuidle_driver = { + .name = "imx5_cpuidle", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, + .states[0] = { + .enter = imx5_cpuidle_enter, + .exit_latency = 20, /* max latency at 160MHz */ + .target_residency = 1, + .flags = CPUIDLE_FLAG_TIME_VALID, + .name = "IMX5 SRPG", + .desc = "CPU state retained,powered off", + }, + .state_count = 1, +}; + /* * Define the MX50 memory map. */ @@ -103,7 +138,7 @@ void __init imx51_init_early(void) mxc_set_cpu_type(MXC_CPU_MX51); mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR)); mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR)); - arm_pm_idle = imx5_idle; + arm_pm_idle = (void *)imx5_idle; }
void __init imx53_init_early(void) @@ -238,4 +273,5 @@ void __init imx53_soc_init(void) void __init imx51_init_late(void) { mx51_neon_fixup(); + imx_cpuidle_init(&imx5_cpuidle_driver); }
Add basic imx6q cpuidle driver. For now, only basic WFI state is supported. Deeper idle states will be added in the future.
Signed-off-by: Robert Lee rob.lee@linaro.org --- arch/arm/mach-imx/cpuidle-imx6q.c | 33 +++++++++++++++++++++++++++++++++ arch/arm/mach-imx/mach-imx6q.c | 18 ++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c new file mode 100644 index 0000000..b74557f --- /dev/null +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -0,0 +1,33 @@ +/* + * 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/init.h> +#include <linux/cpuidle.h> +#include <linux/export.h> +#include <asm/cpuidle.h> +#include <mach/cpuidle.h> + +static struct cpuidle_driver imx6q_cpuidle_driver = { + .name = "imx6q_cpuidle", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, + .states[0] = ARM_CPUIDLE_WFI_STATE, + .state_count = 1, +}; + +int __init imx6q_cpuidle_init(void) +{ + imx_cpuidle_set_driver(&imx6q_cpuidle_driver); + + return 0; +} diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index da6c1d9..21e2051 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -21,6 +21,9 @@ #include <linux/of_platform.h> #include <linux/phy.h> #include <linux/micrel_phy.h> +#include <linux/export.h> +#include <linux/cpuidle.h> +#include <asm/cpuidle.h> #include <asm/smp_twd.h> #include <asm/hardware/cache-l2x0.h> #include <asm/hardware/gic.h> @@ -29,6 +32,7 @@ #include <asm/system_misc.h> #include <mach/common.h> #include <mach/hardware.h> +#include <mach/cpuidle.h>
void imx6q_restart(char mode, const char *cmd) { @@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void) imx6q_pm_init(); }
+static struct cpuidle_driver imx6q_cpuidle_driver = { + .name = "imx6q_cpuidle", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, + .states[0] = ARM_CPUIDLE_WFI_STATE, + .state_count = 1, +}; + +static void __init imx6q_init_late(void) +{ + imx_cpuidle_init(&imx6q_cpuidle_driver); +} + static void __init imx6q_map_io(void) { imx_lluart_map_io(); @@ -142,6 +159,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)") .handle_irq = imx6q_handle_irq, .timer = &imx6q_timer, .init_machine = imx6q_init_machine, + .init_late = imx6q_init_late, .dt_compat = imx6q_dt_compat, .restart = imx6q_restart, MACHINE_END
On Tue, May 01, 2012 at 09:12:37PM -0500, Robert Lee wrote:
Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q platform cpuidle implementation.
Based on v3.4-rc5 plus recently submitted device tree late_initcall patch:
Just to clarify, this is not a device tree late_initcall but a machine specific late_initcall time hook. It has nothing to do with device tree.
Regards, Shawn
http://www.spinics.net/lists/arm-kernel/msg171620.html
Changes since v1:
- Removed some unnecessary spaces
- Added return value for an error message
- Reworked init scheme to use device tree late_initcall.
- Moved imx6q and imx5 cpuidle functionality to existing files.
Robert Lee (3): ARM: imx: Add common imx cpuidle init functionality. ARM: imx: Add imx5 cpuidle driver ARM: imx: Add imx6q cpuidle driver
arch/arm/mach-imx/cpuidle-imx6q.c | 33 ++++++++++++ arch/arm/mach-imx/mach-imx6q.c | 18 +++++++ arch/arm/mach-imx/mm-imx5.c | 42 ++++++++++++++-- arch/arm/plat-mxc/Makefile | 1 + arch/arm/plat-mxc/cpuidle.c | 80 ++++++++++++++++++++++++++++++ arch/arm/plat-mxc/include/mach/cpuidle.h | 22 ++++++++ 6 files changed, 193 insertions(+), 3 deletions(-) create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c create mode 100644 arch/arm/plat-mxc/cpuidle.c create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
-- 1.7.10
On Tue, May 01, 2012 at 09:12:38PM -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
arch/arm/plat-mxc/Makefile | 1 + arch/arm/plat-mxc/cpuidle.c | 80 ++++++++++++++++++++++++++++++ arch/arm/plat-mxc/include/mach/cpuidle.h | 22 ++++++++ 3 files changed, 103 insertions(+) create mode 100644 arch/arm/plat-mxc/cpuidle.c create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile index e81290c..63b064b 100644 --- a/arch/arm/plat-mxc/Makefile +++ b/arch/arm/plat-mxc/Makefile @@ -16,6 +16,7 @@ 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..b7a5e1c --- /dev/null +++ b/arch/arm/plat-mxc/cpuidle.c @@ -0,0 +1,80 @@ +/*
- 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:
- */
+#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;
+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);
+}
Does this function need to be exported? I haven't seen it being used anywhere outside this file. Also, can it be __init?
+int __init imx_cpuidle_init(struct cpuidle_driver *drv) +{
- 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 with error: %d\n",__func__, ret);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);
Nit: print ret (error code) too?
goto uninit;}- }
- return 0;
+uninit:
- imx_cpuidle_devices_uninit();
+unregister_drv:
- cpuidle_unregister_driver(drv);
- return ret;
+} 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..8612510 --- /dev/null +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h @@ -0,0 +1,22 @@ +/*
- 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:
- */
+#include <linux/cpuidle.h>
+#ifdef CONFIG_CPU_IDLE +extern void imx_cpuidle_devices_uninit(void); +extern int imx_cpuidle_init(struct cpuidle_driver *drv); +#else +static inline void imx_cpuidle_devices_uninit(void) {} +static inline int imx_cpuidle_init(struct cpuidle_driver *drv) +{ return -ENODEV; }
Nit: if it can not be in the same line with function name, we usually have it be:
{ return -ENODEV; }
+#endif
1.7.10
On Tue, May 01, 2012 at 09:12:40PM -0500, Robert Lee wrote:
Add basic imx6q cpuidle driver. For now, only basic WFI state is supported. Deeper idle states will be added in the future.
Signed-off-by: Robert Lee rob.lee@linaro.org
arch/arm/mach-imx/cpuidle-imx6q.c | 33 +++++++++++++++++++++++++++++++++
So, this file is not needed any more, I guess.
arch/arm/mach-imx/mach-imx6q.c | 18 ++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c new file mode 100644 index 0000000..b74557f --- /dev/null +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -0,0 +1,33 @@ +/*
- 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:
- */
+#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/cpuidle.h> +#include <linux/export.h> +#include <asm/cpuidle.h> +#include <mach/cpuidle.h>
+static struct cpuidle_driver imx6q_cpuidle_driver = {
- .name = "imx6q_cpuidle",
- .owner = THIS_MODULE,
- .en_core_tk_irqen = 1,
- .states[0] = ARM_CPUIDLE_WFI_STATE,
- .state_count = 1,
+};
+int __init imx6q_cpuidle_init(void) +{
- imx_cpuidle_set_driver(&imx6q_cpuidle_driver);
- return 0;
+} diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index da6c1d9..21e2051 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -21,6 +21,9 @@ #include <linux/of_platform.h> #include <linux/phy.h> #include <linux/micrel_phy.h> +#include <linux/export.h> +#include <linux/cpuidle.h> +#include <asm/cpuidle.h> #include <asm/smp_twd.h> #include <asm/hardware/cache-l2x0.h> #include <asm/hardware/gic.h> @@ -29,6 +32,7 @@ #include <asm/system_misc.h> #include <mach/common.h> #include <mach/hardware.h> +#include <mach/cpuidle.h>
The headers here are mostly sorted in names, so please ...
Regards, Shawn
void imx6q_restart(char mode, const char *cmd) { @@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void) imx6q_pm_init(); } +static struct cpuidle_driver imx6q_cpuidle_driver = {
- .name = "imx6q_cpuidle",
- .owner = THIS_MODULE,
- .en_core_tk_irqen = 1,
- .states[0] = ARM_CPUIDLE_WFI_STATE,
- .state_count = 1,
+};
+static void __init imx6q_init_late(void) +{
- imx_cpuidle_init(&imx6q_cpuidle_driver);
+}
static void __init imx6q_map_io(void) { imx_lluart_map_io(); @@ -142,6 +159,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)") .handle_irq = imx6q_handle_irq, .timer = &imx6q_timer, .init_machine = imx6q_init_machine,
- .init_late = imx6q_init_late, .dt_compat = imx6q_dt_compat, .restart = imx6q_restart,
MACHINE_END
1.7.10
On Tue, May 01, 2012 at 09:12:38PM -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
+int __init imx_cpuidle_init(struct cpuidle_driver *drv) +{
- struct cpuidle_device *dev;
- int cpu_id, ret;
- if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
Please don't check for !drv here. When someone calls this function with a NULL pointer he should get a nive stack trace allowing him to figure out what went wrong.
pr_err("%s: Invalid Input\n", __func__);return -EINVAL;- }
- ret = cpuidle_register_driver(drv);
- if (ret) {
pr_err("%s: Failed to register cpuidle driver with error: %d\n",__func__, ret);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;
You should only unregister the cpuidle devices you successfully registered. Unregistering not yet registered cpuidle devices probably has unwanted side effects.
Sascha
}- }
- return 0;
+uninit:
- imx_cpuidle_devices_uninit();
+unregister_drv:
- cpuidle_unregister_driver(drv);
- return ret;
+} 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..8612510 --- /dev/null +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h @@ -0,0 +1,22 @@ +/*
- 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:
- */
+#include <linux/cpuidle.h>
+#ifdef CONFIG_CPU_IDLE +extern void imx_cpuidle_devices_uninit(void); +extern int imx_cpuidle_init(struct cpuidle_driver *drv); +#else +static inline void imx_cpuidle_devices_uninit(void) {} +static inline int imx_cpuidle_init(struct cpuidle_driver *drv) +{ return -ENODEV; }
+#endif
1.7.10
On Tue, May 01, 2012 at 09:12:39PM -0500, Robert Lee wrote:
Add imx5 cpuidle driver.
Signed-off-by: Robert Lee rob.lee@linaro.org
arch/arm/mach-imx/mm-imx5.c | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c index d6b7e9f..cbd9bad 100644 --- a/arch/arm/mach-imx/mm-imx5.c +++ b/arch/arm/mach-imx/mm-imx5.c @@ -22,24 +22,59 @@ #include <mach/common.h> #include <mach/devices-common.h> #include <mach/iomux-v3.h> +#include <mach/cpuidle.h> static struct clk *gpc_dvfs_clk; -static void imx5_idle(void) +static int imx5_idle(void) {
- int ret = 0;
- /* gpc clock is needed for SRPG */ if (gpc_dvfs_clk == NULL) { gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs"); if (IS_ERR(gpc_dvfs_clk))
return;
return -ENODEV;- else
ret = -EBUSY;- return ret;
+}
+static int imx5_cpuidle_enter(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int idx)+{
- int ret;
- ret = imx5_idle();
- if (ret < 0)
return ret;- return idx;
} +static struct cpuidle_driver imx5_cpuidle_driver = {
- .name = "imx5_cpuidle",
- .owner = THIS_MODULE,
- .en_core_tk_irqen = 1,
- .states[0] = {
.enter = imx5_cpuidle_enter,.exit_latency = 20, /* max latency at 160MHz */.target_residency = 1,.flags = CPUIDLE_FLAG_TIME_VALID,.name = "IMX5 SRPG",.desc = "CPU state retained,powered off",- },
- .state_count = 1,
+};
/*
- Define the MX50 memory map.
*/ @@ -103,7 +138,7 @@ void __init imx51_init_early(void) mxc_set_cpu_type(MXC_CPU_MX51); mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR)); mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
- arm_pm_idle = imx5_idle;
- arm_pm_idle = (void *)imx5_idle;
I don't like this. It will cover all warnings when the prototype of arm_pm_idle changes in future. Better add a static void imx5_idle which calls a static int imx5_do_idle, then you have an idle function which returns an int.
} void __init imx53_init_early(void) @@ -238,4 +273,5 @@ void __init imx53_soc_init(void) void __init imx51_init_late(void) { mx51_neon_fixup();
- imx_cpuidle_init(&imx5_cpuidle_driver);
}
1.7.10
On Tue, May 1, 2012 at 10:23 PM, Shawn Guo shawn.guo@linaro.org wrote:
On Tue, May 01, 2012 at 09:12:40PM -0500, Robert Lee wrote:
Add basic imx6q cpuidle driver. For now, only basic WFI state is supported. Deeper idle states will be added in the future.
Signed-off-by: Robert Lee rob.lee@linaro.org
arch/arm/mach-imx/cpuidle-imx6q.c | 33 +++++++++++++++++++++++++++++++++
So, this file is not needed any more, I guess.
Yes, I missed that. It shouldn't have been part of the patch.
arch/arm/mach-imx/mach-imx6q.c | 18 ++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c new file mode 100644 index 0000000..b74557f --- /dev/null +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -0,0 +1,33 @@ +/*
- 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:
- */
+#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/cpuidle.h> +#include <linux/export.h> +#include <asm/cpuidle.h> +#include <mach/cpuidle.h>
+static struct cpuidle_driver imx6q_cpuidle_driver = {
- .name = "imx6q_cpuidle",
- .owner = THIS_MODULE,
- .en_core_tk_irqen = 1,
- .states[0] = ARM_CPUIDLE_WFI_STATE,
- .state_count = 1,
+};
+int __init imx6q_cpuidle_init(void) +{
- imx_cpuidle_set_driver(&imx6q_cpuidle_driver);
- return 0;
+} diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index da6c1d9..21e2051 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -21,6 +21,9 @@ #include <linux/of_platform.h> #include <linux/phy.h> #include <linux/micrel_phy.h> +#include <linux/export.h> +#include <linux/cpuidle.h> +#include <asm/cpuidle.h> #include <asm/smp_twd.h> #include <asm/hardware/cache-l2x0.h> #include <asm/hardware/gic.h> @@ -29,6 +32,7 @@ #include <asm/system_misc.h> #include <mach/common.h> #include <mach/hardware.h> +#include <mach/cpuidle.h>
The headers here are mostly sorted in names, so please ...
Do you mean sorted alphabetically per group? So the last three includes should look like this instead? ... #include <mach/common.h> #include <mach/cpuidle.h> #include <mach/hardware.h> ... Thanks, Rob
Regards, Shawn
void imx6q_restart(char mode, const char *cmd) { @@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void) imx6q_pm_init(); }
+static struct cpuidle_driver imx6q_cpuidle_driver = {
- .name = "imx6q_cpuidle",
- .owner = THIS_MODULE,
- .en_core_tk_irqen = 1,
- .states[0] = ARM_CPUIDLE_WFI_STATE,
- .state_count = 1,
+};
+static void __init imx6q_init_late(void) +{
- imx_cpuidle_init(&imx6q_cpuidle_driver);
+}
static void __init imx6q_map_io(void) { imx_lluart_map_io(); @@ -142,6 +159,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)") .handle_irq = imx6q_handle_irq, .timer = &imx6q_timer, .init_machine = imx6q_init_machine,
- .init_late = imx6q_init_late,
.dt_compat = imx6q_dt_compat, .restart = imx6q_restart, MACHINE_END -- 1.7.10
On Wed, May 02, 2012 at 08:50:20AM -0500, Rob Lee wrote:
--- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -21,6 +21,9 @@ #include <linux/of_platform.h> #include <linux/phy.h> #include <linux/micrel_phy.h> +#include <linux/export.h> +#include <linux/cpuidle.h> +#include <asm/cpuidle.h> #include <asm/smp_twd.h> #include <asm/hardware/cache-l2x0.h> #include <asm/hardware/gic.h> @@ -29,6 +32,7 @@ #include <asm/system_misc.h> #include <mach/common.h> #include <mach/hardware.h> +#include <mach/cpuidle.h>
The headers here are mostly sorted in names, so please ...
Do you mean sorted alphabetically per group? So the last three includes should look like this instead? ... #include <mach/common.h> #include <mach/cpuidle.h> #include <mach/hardware.h> ...
Yes. And do not forget <linux/export.h> and <linux/cpuidle.h>.
Shawn,
On Tue, May 1, 2012 at 10:13 PM, Shawn Guo shawn.guo@linaro.org wrote:
On Tue, May 01, 2012 at 09:12:38PM -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
arch/arm/plat-mxc/Makefile | 1 + arch/arm/plat-mxc/cpuidle.c | 80 ++++++++++++++++++++++++++++++ arch/arm/plat-mxc/include/mach/cpuidle.h | 22 ++++++++ 3 files changed, 103 insertions(+) create mode 100644 arch/arm/plat-mxc/cpuidle.c create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile index e81290c..63b064b 100644 --- a/arch/arm/plat-mxc/Makefile +++ b/arch/arm/plat-mxc/Makefile @@ -16,6 +16,7 @@ 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..b7a5e1c --- /dev/null +++ b/arch/arm/plat-mxc/cpuidle.c @@ -0,0 +1,80 @@ +/*
- 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:
- */
+#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;
+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);
+}
Does this function need to be exported? I haven't seen it being used anywhere outside this file. Also, can it be __init?
Yes to both for now and can be changed back if necessary in the future.
+int __init imx_cpuidle_init(struct cpuidle_driver *drv) +{
- 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 with error: %d\n",
- __func__, ret);
- 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);
Nit: print ret (error code) too?
I added the printing of the error code based on Sascha's suggestion in v1 of this submission.
- goto uninit;
- }
- }
- return 0;
+uninit:
- imx_cpuidle_devices_uninit();
+unregister_drv:
- cpuidle_unregister_driver(drv);
- return ret;
+} 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..8612510 --- /dev/null +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h @@ -0,0 +1,22 @@ +/*
- 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:
- */
+#include <linux/cpuidle.h>
+#ifdef CONFIG_CPU_IDLE +extern void imx_cpuidle_devices_uninit(void); +extern int imx_cpuidle_init(struct cpuidle_driver *drv); +#else +static inline void imx_cpuidle_devices_uninit(void) {} +static inline int imx_cpuidle_init(struct cpuidle_driver *drv) +{ return -ENODEV; }
Nit: if it can not be in the same line with function name, we usually have it be:
{ return -ENODEV; }
Understood. I was just going by what I have seen used in other places (like include/linux/cpuidle.h).
Thanks, Rob
+#endif
1.7.10
-- Regards, Shawn
On 2 May 2012 21:59, Rob Lee rob.lee@linaro.org wrote:
- ret = cpuidle_register_device(dev);
- if (ret) {
- pr_err("%s: Failed to register cpu %u\n",
- __func__, cpu_id);
Nit: print ret (error code) too?
I added the printing of the error code based on Sascha's suggestion in v1 of this submission.
But you did not print "ret" in above pr_err.
Regards, Shawn
On Wed, May 2, 2012 at 9:07 AM, Shawn Guo shawn.guo@linaro.org wrote:
On 2 May 2012 21:59, Rob Lee rob.lee@linaro.org wrote:
- ret = cpuidle_register_device(dev);
- if (ret) {
- pr_err("%s: Failed to register cpu %u\n",
- __func__, cpu_id);
Nit: print ret (error code) too?
I added the printing of the error code based on Sascha's suggestion in v1 of this submission.
But you did not print "ret" in above pr_err.
Argh, sorry, still trying to wake up I guess. Thanks.
Regards, Shawn
Sascha,
On Wed, May 2, 2012 at 2:27 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Tue, May 01, 2012 at 09:12:38PM -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
+int __init imx_cpuidle_init(struct cpuidle_driver *drv) +{
- struct cpuidle_device *dev;
- int cpu_id, ret;
- if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
Please don't check for !drv here. When someone calls this function with a NULL pointer he should get a nive stack trace allowing him to figure out what went wrong.
Ok, I will change this in v3. Given your statement, my understanding is that I should avoid adding checks to make sure a valid driver object was given as the stack trace information is all the handling that is needed. If there is any further logic needed in that rule, could you elaborate so that I don't make this mistake in the future, or so that I don't add a check on a driver object in a case that I should?
- pr_err("%s: Invalid Input\n", __func__);
- return -EINVAL;
- }
- ret = cpuidle_register_driver(drv);
- if (ret) {
- pr_err("%s: Failed to register cpuidle driver with error: %d\n",
- __func__, ret);
- 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;
You should only unregister the cpuidle devices you successfully registered. Unregistering not yet registered cpuidle devices probably has unwanted side effects.
I did not add in this handling because the cpuidle_unregister_device() call already has a "registered" check so extra handling seemed unnecessary. But if you still think it is needed just let me know.
cpuidle_unregister_device() { ... if (dev->registered == 0) return; ...
Thanks, Rob
Sascha
- }
- }
- return 0;
+uninit:
- imx_cpuidle_devices_uninit();
+unregister_drv:
- cpuidle_unregister_driver(drv);
- return ret;
+} 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..8612510 --- /dev/null +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h @@ -0,0 +1,22 @@ +/*
- 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:
- */
+#include <linux/cpuidle.h>
+#ifdef CONFIG_CPU_IDLE +extern void imx_cpuidle_devices_uninit(void); +extern int imx_cpuidle_init(struct cpuidle_driver *drv); +#else +static inline void imx_cpuidle_devices_uninit(void) {} +static inline int imx_cpuidle_init(struct cpuidle_driver *drv) +{ return -ENODEV; }
+#endif
1.7.10
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Sascha,
On Wed, May 2, 2012 at 2:33 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Tue, May 01, 2012 at 09:12:39PM -0500, Robert Lee wrote:
Add imx5 cpuidle driver.
Signed-off-by: Robert Lee rob.lee@linaro.org
arch/arm/mach-imx/mm-imx5.c | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c index d6b7e9f..cbd9bad 100644 --- a/arch/arm/mach-imx/mm-imx5.c +++ b/arch/arm/mach-imx/mm-imx5.c @@ -22,24 +22,59 @@ #include <mach/common.h> #include <mach/devices-common.h> #include <mach/iomux-v3.h> +#include <mach/cpuidle.h>
static struct clk *gpc_dvfs_clk;
-static void imx5_idle(void) +static int imx5_idle(void) {
- int ret = 0;
/* gpc clock is needed for SRPG */ if (gpc_dvfs_clk == NULL) { gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs"); if (IS_ERR(gpc_dvfs_clk))
- return;
- return -ENODEV;
} clk_enable(gpc_dvfs_clk); mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF); if (!tzic_enable_wake()) cpu_do_idle();
- else
- ret = -EBUSY;
clk_disable(gpc_dvfs_clk);
- return ret;
+}
+static int imx5_cpuidle_enter(struct cpuidle_device *dev,
- struct cpuidle_driver *drv, int idx)
+{
- int ret;
- ret = imx5_idle();
- if (ret < 0)
- return ret;
- return idx;
}
+static struct cpuidle_driver imx5_cpuidle_driver = {
- .name = "imx5_cpuidle",
- .owner = THIS_MODULE,
- .en_core_tk_irqen = 1,
- .states[0] = {
- .enter = imx5_cpuidle_enter,
- .exit_latency = 20, /* max latency at 160MHz */
- .target_residency = 1,
- .flags = CPUIDLE_FLAG_TIME_VALID,
- .name = "IMX5 SRPG",
- .desc = "CPU state retained,powered off",
- },
- .state_count = 1,
+};
/* * Define the MX50 memory map. */ @@ -103,7 +138,7 @@ void __init imx51_init_early(void) mxc_set_cpu_type(MXC_CPU_MX51); mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR)); mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
- arm_pm_idle = imx5_idle;
- arm_pm_idle = (void *)imx5_idle;
I don't like this. It will cover all warnings when the prototype of arm_pm_idle changes in future. Better add a static void imx5_idle which calls a static int imx5_do_idle, then you have an idle function which returns an int.
What about using the following:
arm_pm_idle = (void (*)(void))imx5_idle;
This will give warnings if arm_pm_idle prototype changes.
Thanks, Rob
}
void __init imx53_init_early(void) @@ -238,4 +273,5 @@ void __init imx53_soc_init(void) void __init imx51_init_late(void) { mx51_neon_fixup();
- imx_cpuidle_init(&imx5_cpuidle_driver);
}
1.7.10
-- 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 Wed, May 02, 2012 at 02:16:36PM -0500, Rob Lee wrote:
Sascha,
+int __init imx_cpuidle_init(struct cpuidle_driver *drv) +{
- struct cpuidle_device *dev;
- int cpu_id, ret;
- if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
Please don't check for !drv here. When someone calls this function with a NULL pointer he should get a nive stack trace allowing him to figure out what went wrong.
Ok, I will change this in v3. Given your statement, my understanding is that I should avoid adding checks to make sure a valid driver object was given as the stack trace information is all the handling that is needed. If there is any further logic needed in that rule, could you elaborate so that I don't make this mistake in the future, or so that I don't add a check on a driver object in a case that I should?
Here we have the case that only a Kernel developer will add a call to this function. For a kernel developer a stack trace is more useful than a pr_err. Of course this is different when not testing for a NULL pointer causes subtle bugs in unrelated code.
You should only unregister the cpuidle devices you successfully registered. Unregistering not yet registered cpuidle devices probably has unwanted side effects.
I did not add in this handling because the cpuidle_unregister_device() call already has a "registered" check so extra handling seemed unnecessary. But if you still think it is needed just let me know.
It's ok then. I didn't check cpuidle_unregister_device.
Sascha
On Wed, May 02, 2012 at 03:11:35PM -0500, Rob Lee wrote:
Sascha,
mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR)); mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
- arm_pm_idle = imx5_idle;
- arm_pm_idle = (void *)imx5_idle;
I don't like this. It will cover all warnings when the prototype of arm_pm_idle changes in future. Better add a static void imx5_idle which calls a static int imx5_do_idle, then you have an idle function which returns an int.
What about using the following:
arm_pm_idle = (void (*)(void))imx5_idle;
This will give warnings if arm_pm_idle prototype changes.
This surely works but will look suspicious for people looking at the code.
Sascha
-
Rob Lee -
Robert Lee
-
Sascha Hauer -
Shawn Guo