Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q platform cpuidle implementation.
Based on v3.4-rc6 plus recently submitted device tree late_initcall patch: http://www.spinics.net/lists/arm-kernel/msg171620.html
v3 changes: * removed file introduced in v1 no longer needed after v2 [per Shawn Guo] * re-ordered added #includes in alphabetical order [per Shawn Guo] * Remove if(!drv) check to allow handling by stack trace [per Sascha Hauer] * Added missing return value in error meesage [per Shawn Guo] * fixed (void *) casting problem pointed out Sasha Hauer. Used a different type of casting to properly give warning on improper casting: arm_pm_idle = (void (*)(void))imx5_idle; Used this casting instead of adding a dummy caller function because adding the dummy function appears to unnecessarily introduce the following additional operations: static void imx5_pm_idle(void) { a0: e1a0c00d mov ip, sp a4: e92dd800 push {fp, ip, lr, pc} a8: e24cb004 sub fp, ip, #4 imx5_idle(); ac: ebffffd3 bl 0 <imx5_idle> } b0: e89da800 ldm sp, {fp, sp, pc}
v2 changes: * Removed some unnecessary spaces [per Jess Juhl] * Added return value for an error message [per Sascha Hauer] * Reworked init scheme to use device tree late_initcall [per Sascha and Shawn] * Moved imx6q and imx5 cpuidle functionality to existing files.
https://lkml.org/lkml/2012/5/1/363
v1 initial submission:
https://lkml.org/lkml/2012/4/16/644
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/mach-imx6q.c | 19 +++++++ 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 ++++++++ 5 files changed, 161 insertions(+), 3 deletions(-) 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..d4cb511 --- /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/cpuidle.h> +#include <linux/err.h> +#include <linux/hrtimer.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/slab.h> + +static struct cpuidle_device __percpu * imx_cpuidle_devices; + +static void __init 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->state_count > CPUIDLE_STATE_MAX) { + pr_err("%s: state_count exceeds maximum\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, error: %d\n", + __func__, cpu_id, ret); + 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..bc932d1 --- /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 int imx_cpuidle_init(struct cpuidle_driver *drv); +#else +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..0b3a4cc 100644 --- a/arch/arm/mach-imx/mm-imx5.c +++ b/arch/arm/mach-imx/mm-imx5.c @@ -20,26 +20,61 @@
#include <mach/hardware.h> #include <mach/common.h> +#include <mach/cpuidle.h> #include <mach/devices-common.h> #include <mach/iomux-v3.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 (*)(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); }
On Mon, May 07, 2012 at 04:16:46PM -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..0b3a4cc 100644 --- a/arch/arm/mach-imx/mm-imx5.c +++ b/arch/arm/mach-imx/mm-imx5.c @@ -20,26 +20,61 @@ #include <mach/hardware.h> #include <mach/common.h> +#include <mach/cpuidle.h> #include <mach/devices-common.h> #include <mach/iomux-v3.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");
This clk_get should go away here and be moved somewhere to initialization. Also, if getting this clock fails we can still do regular cpu_do_idle. Additionally, if clk_get fails, we'll have a ERR_PTR value in gpc_dvfs_clk in which case the gpc_dvfs_clk == NULL won't trigger next time you are here and then you'll enable a nonexisting clock below.
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",- },
I wonder why you don't add the default ARM_CPUIDLE_WFI_STATE_PWR state. The above is something different, right? It has a greater exit latency than ARM_CPUIDLE_WFI_STATE_PWR, so why don't we add it here aswell?
- .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 (*)(void))imx5_idle;
Still this looks suspicious. Reading this will lead to the question why this prototype is casted. Please just add a imx5_pm_idle with the correct prototype.
} 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
Sascha,
On Wed, May 9, 2012 at 3:02 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Mon, May 07, 2012 at 04:16:46PM -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..0b3a4cc 100644 --- a/arch/arm/mach-imx/mm-imx5.c +++ b/arch/arm/mach-imx/mm-imx5.c @@ -20,26 +20,61 @@
#include <mach/hardware.h> #include <mach/common.h> +#include <mach/cpuidle.h> #include <mach/devices-common.h> #include <mach/iomux-v3.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");
This clk_get should go away here and be moved somewhere to initialization. Also, if getting this clock fails we can still do regular cpu_do_idle. Additionally, if clk_get fails, we'll have a ERR_PTR value in gpc_dvfs_clk in which case the gpc_dvfs_clk == NULL won't trigger next time you are here and then you'll enable a nonexisting clock below.
Agree. I'd prefer to enable this clock during intialization and just leave it running. It is supposed to be a very low power clock and I couldn't measuring any power difference with and without it being enabled
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",
- },
I wonder why you don't add the default ARM_CPUIDLE_WFI_STATE_PWR state. The above is something different, right? It has a greater exit latency than ARM_CPUIDLE_WFI_STATE_PWR, so why don't we add it here aswell?
Yes and no. Yes this is a different state but no, it doesn't have a significantly greater exit latency, or at least a large enough exit latency to warrant an extra state in my opinion. According to the i.MX5 documentation, the extra exit time beyond basic WFI required for the "WAIT_UNCLOCKED_POWER_OFF" state is 500ns (this is due to a difference in i.MX5 hardware implementation compared to all other ARM platforms). In reality, it did require a few more microseconds to perform in my testing just based on the extra register writes in mx5_cpu_lp_set(). I'd like to clean up mx5_cpu_lp_set() and add a global variable to track the previous state and to just exit out if the new state is the same as the old. I could do this cleanup as part of this patchset if you prefer that.
- .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 (*)(void))imx5_idle;
Still this looks suspicious. Reading this will lead to the question why this prototype is casted. Please just add a imx5_pm_idle with the correct prototype.
Ok.
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 09, 2012 at 09:27:02AM -0500, Rob Lee wrote:
Sascha,
This clk_get should go away here and be moved somewhere to initialization. Also, if getting this clock fails we can still do regular cpu_do_idle. Additionally, if clk_get fails, we'll have a ERR_PTR value in gpc_dvfs_clk in which case the gpc_dvfs_clk == NULL won't trigger next time you are here and then you'll enable a nonexisting clock below.
Agree. I'd prefer to enable this clock during intialization and just leave it running. It is supposed to be a very low power clock and I couldn't measuring any power difference with and without it being enabled
Ok, even better.
I wonder why you don't add the default ARM_CPUIDLE_WFI_STATE_PWR state. The above is something different, right? It has a greater exit latency than ARM_CPUIDLE_WFI_STATE_PWR, so why don't we add it here aswell?
Yes and no. Yes this is a different state but no, it doesn't have a significantly greater exit latency, or at least a large enough exit latency to warrant an extra state in my opinion. According to the i.MX5 documentation, the extra exit time beyond basic WFI required for the "WAIT_UNCLOCKED_POWER_OFF" state is 500ns (this is due to a difference in i.MX5 hardware implementation compared to all other ARM platforms). In reality, it did require a few more microseconds to perform in my testing just based on the extra register writes in mx5_cpu_lp_set(). I'd like to clean up mx5_cpu_lp_set() and add a global variable to track the previous state and to just exit out if the new state is the same as the old.
Do you think it's worth it? You buy skipping the read with an additional test.
I could do this cleanup as part of this patchset if you prefer that.
Yes please. Cleanups before adding new features is always a good reason to apply a patch series ;)
Sascha
On Thu, May 10, 2012 at 7:41 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Wed, May 09, 2012 at 09:27:02AM -0500, Rob Lee wrote:
Sascha,
This clk_get should go away here and be moved somewhere to initialization. Also, if getting this clock fails we can still do regular cpu_do_idle. Additionally, if clk_get fails, we'll have a ERR_PTR value in gpc_dvfs_clk in which case the gpc_dvfs_clk == NULL won't trigger next time you are here and then you'll enable a nonexisting clock below.
Agree. I'd prefer to enable this clock during intialization and just leave it running. It is supposed to be a very low power clock and I couldn't measuring any power difference with and without it being enabled
Ok, even better.
I wonder why you don't add the default ARM_CPUIDLE_WFI_STATE_PWR state. The above is something different, right? It has a greater exit latency than ARM_CPUIDLE_WFI_STATE_PWR, so why don't we add it here aswell?
Yes and no. Yes this is a different state but no, it doesn't have a significantly greater exit latency, or at least a large enough exit latency to warrant an extra state in my opinion. According to the i.MX5 documentation, the extra exit time beyond basic WFI required for the "WAIT_UNCLOCKED_POWER_OFF" state is 500ns (this is due to a difference in i.MX5 hardware implementation compared to all other ARM platforms). In reality, it did require a few more microseconds to perform in my testing just based on the extra register writes in mx5_cpu_lp_set(). I'd like to clean up mx5_cpu_lp_set() and add a global variable to track the previous state and to just exit out if the new state is the same as the old.
Do you think it's worth it? You buy skipping the read with an additional test.
I'll run some tests to check.
Thanks, Rob
I could do this cleanup as part of this patchset if you prefer that.
Yes please. Cleanups before adding new features is always a good reason to apply a patch series ;)
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 |
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/mach-imx6q.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index da6c1d9..4fc1fe7 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -10,7 +10,9 @@ * http://www.gnu.org/copyleft/gpl.html */
+#include <linux/cpuidle.h> #include <linux/delay.h> +#include <linux/export.h> #include <linux/init.h> #include <linux/io.h> #include <linux/irq.h> @@ -21,6 +23,7 @@ #include <linux/of_platform.h> #include <linux/phy.h> #include <linux/micrel_phy.h> +#include <asm/cpuidle.h> #include <asm/smp_twd.h> #include <asm/hardware/cache-l2x0.h> #include <asm/hardware/gic.h> @@ -28,8 +31,10 @@ #include <asm/mach/time.h> #include <asm/system_misc.h> #include <mach/common.h> +#include <mach/cpuidle.h> #include <mach/hardware.h>
+ void imx6q_restart(char mode, const char *cmd) { struct device_node *np; @@ -86,6 +91,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 +160,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
-
Rob Lee -
Robert Lee
-
Sascha Hauer