change log: 1. move code to arch/arm/mach-mx5/, since it is cpu-type specific 2. provide a interface for various board to register board-specific params.
From: Yong Shen yong.shen@freescale.com
implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board related code.
Signed-off-by: Yong Shen yong.shen@freescale.com --- arch/arm/mach-mx5/Makefile | 1 + arch/arm/mach-mx5/cpuidle.c | 113 +++++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-mx5/cpuidle.h | 26 ++++++++++ 3 files changed, 140 insertions(+), 0 deletions(-) create mode 100644 arch/arm/mach-mx5/cpuidle.c create mode 100644 arch/arm/mach-mx5/cpuidle.h
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile index 0d43be9..12239e0 100644 --- a/arch/arm/mach-mx5/Makefile +++ b/arch/arm/mach-mx5/Makefile @@ -7,6 +7,7 @@ obj-y := cpu.o mm.o clock-mx51-mx53.o devices.o obj-$(CONFIG_SOC_IMX50) += mm-mx50.o
obj-$(CONFIG_CPU_FREQ_IMX) += cpu_op-mx51.o +obj-$(CONFIG_CPU_IDLE) += cpuidle.o obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o obj-$(CONFIG_MACH_MX53_EVK) += board-mx53_evk.o diff --git a/arch/arm/mach-mx5/cpuidle.c b/arch/arm/mach-mx5/cpuidle.c new file mode 100644 index 0000000..9d77c47 --- /dev/null +++ b/arch/arm/mach-mx5/cpuidle.c @@ -0,0 +1,113 @@ +/* + * arch/arm/mach-mx5/cpuidle.c + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/io.h> +#include <linux/cpuidle.h> +#include <asm/proc-fns.h> +#include <mach/hardware.h> +#include "cpuidle.h" +#include "crm_regs.h" + +static struct imx_cpuidle_params *imx_cpuidle_params; +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params) +{ + imx_cpuidle_params = cpuidle_params; +} + +extern int tzic_enable_wake(int is_idle); +static int imx_enter_idle(struct cpuidle_device *dev, + struct cpuidle_state *state) +{ + struct timeval before, after; + int idle_time; + u32 plat_lpc, arm_srpgcr, ccm_clpcr; + u32 empgc0, empgc1; + + local_irq_disable(); + do_gettimeofday(&before); + + plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) & + ~(MXC_CORTEXA8_PLAT_LPC_DSM); + ccm_clpcr = __raw_readl(MXC_CCM_CLPCR) & ~(MXC_CCM_CLPCR_LPM_MASK); + arm_srpgcr = __raw_readl(MXC_SRPG_ARM_SRPGCR) & ~(MXC_SRPGCR_PCR); + empgc0 = __raw_readl(MXC_SRPG_EMPGC0_SRPGCR) & ~(MXC_SRPGCR_PCR); + empgc1 = __raw_readl(MXC_SRPG_EMPGC1_SRPGCR) & ~(MXC_SRPGCR_PCR); + + if (state == &dev->states[WAIT_CLK_ON]) + ; + else if (state == &dev->states[WAIT_CLK_OFF]) + ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET); + else if (state == &dev->states[WAIT_CLK_OFF_POWER_OFF]) { + /* Wait unclocked, power off */ + plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM + | MXC_CORTEXA8_PLAT_LPC_DBG_DSM; + arm_srpgcr |= MXC_SRPGCR_PCR; + ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET); + ccm_clpcr &= ~MXC_CCM_CLPCR_VSTBY; + ccm_clpcr &= ~MXC_CCM_CLPCR_SBYOS; + if (tzic_enable_wake(1) != 0) { + local_irq_enable(); + return 0; + } + } + + __raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC); + __raw_writel(ccm_clpcr, MXC_CCM_CLPCR); + __raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR); + + cpu_do_idle(); + + do_gettimeofday(&after); + local_irq_enable(); + idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC + + (after.tv_usec - before.tv_usec); + return idle_time; +} + +static struct cpuidle_driver imx_cpuidle_driver = { + .name = "imx_idle", + .owner = THIS_MODULE, +}; + +static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device); + +static int __init imx_cpuidle_init(void) +{ + struct cpuidle_device *device; + int i; + + if (imx_cpuidle_params == NULL) + return -ENODEV; + + cpuidle_register_driver(&imx_cpuidle_driver); + + device = &per_cpu(imx_cpuidle_device, smp_processor_id()); + device->state_count = IMX_MAX_CPUIDLE_STATE; + + for (i = 0; i < IMX_MAX_CPUIDLE_STATE && i < CPUIDLE_STATE_MAX; i++) { + device->states[i].enter = imx_enter_idle; + device->states[i].exit_latency = imx_cpuidle_params[i].latency; + device->states[i].flags = CPUIDLE_FLAG_TIME_VALID; + } + + strcpy(device->states[WAIT_CLK_ON].name, "WFI 0"); + strcpy(device->states[WAIT_CLK_ON].desc, "Wait with clock on"); + strcpy(device->states[WAIT_CLK_OFF].name, "WFI 1"); + strcpy(device->states[WAIT_CLK_OFF].desc, "Wait with clock off"); + strcpy(device->states[WAIT_CLK_OFF_POWER_OFF].name, "WFI 2"); + strcpy(device->states[WAIT_CLK_OFF_POWER_OFF].desc, + "Wait with clock off and power gating"); + + if (cpuidle_register_device(device)) { + printk(KERN_ERR "imx_cpuidle_init: Failed registering\n"); + return -ENODEV; + } + return 0; +} + +late_initcall(imx_cpuidle_init); diff --git a/arch/arm/mach-mx5/cpuidle.h b/arch/arm/mach-mx5/cpuidle.h new file mode 100644 index 0000000..e5ba495 --- /dev/null +++ b/arch/arm/mach-mx5/cpuidle.h @@ -0,0 +1,26 @@ +/* + * arch/arm/mach-mx5/cpuidle.h + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +enum { + WAIT_CLK_ON, /* c1 */ + WAIT_CLK_OFF, /* c2 */ + WAIT_CLK_OFF_POWER_OFF, /* c3 */ + IMX_MAX_CPUIDLE_STATE, +}; + +struct imx_cpuidle_params { + unsigned int latency; +}; + +#ifdef CONFIG_CPU_IDLE +extern void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params); +#else +inline void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params) +{} +#endif +
On Fri, Feb 11, 2011 at 10:36:12AM +0100, yong.shen@linaro.org wrote:
From: Yong Shen yong.shen@freescale.com
implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board related code.
Signed-off-by: Yong Shen yong.shen@freescale.com
arch/arm/mach-mx5/Makefile | 1 + arch/arm/mach-mx5/cpuidle.c | 113 +++++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-mx5/cpuidle.h | 26 ++++++++++ 3 files changed, 140 insertions(+), 0 deletions(-) create mode 100644 arch/arm/mach-mx5/cpuidle.c create mode 100644 arch/arm/mach-mx5/cpuidle.h
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile index 0d43be9..12239e0 100644 --- a/arch/arm/mach-mx5/Makefile +++ b/arch/arm/mach-mx5/Makefile @@ -7,6 +7,7 @@ obj-y := cpu.o mm.o clock-mx51-mx53.o devices.o obj-$(CONFIG_SOC_IMX50) += mm-mx50.o obj-$(CONFIG_CPU_FREQ_IMX) += cpu_op-mx51.o +obj-$(CONFIG_CPU_IDLE) += cpuidle.o obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o obj-$(CONFIG_MACH_MX53_EVK) += board-mx53_evk.o diff --git a/arch/arm/mach-mx5/cpuidle.c b/arch/arm/mach-mx5/cpuidle.c new file mode 100644 index 0000000..9d77c47 --- /dev/null +++ b/arch/arm/mach-mx5/cpuidle.c @@ -0,0 +1,113 @@ +/*
- arch/arm/mach-mx5/cpuidle.c
- This file is licensed under the terms of the GNU General Public
- License version 2. This program is licensed "as is" without any
- warranty of any kind, whether express or implied.
- */
+#include <linux/io.h> +#include <linux/cpuidle.h> +#include <asm/proc-fns.h> +#include <mach/hardware.h> +#include "cpuidle.h" +#include "crm_regs.h"
+static struct imx_cpuidle_params *imx_cpuidle_params; +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params) +{
- imx_cpuidle_params = cpuidle_params;
+}
+extern int tzic_enable_wake(int is_idle);
Please put this into a header file.
+static int imx_enter_idle(struct cpuidle_device *dev,
struct cpuidle_state *state)+{
- struct timeval before, after;
- int idle_time;
- u32 plat_lpc, arm_srpgcr, ccm_clpcr;
- u32 empgc0, empgc1;
- local_irq_disable();
- do_gettimeofday(&before);
- plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &
~(MXC_CORTEXA8_PLAT_LPC_DSM);- ccm_clpcr = __raw_readl(MXC_CCM_CLPCR) & ~(MXC_CCM_CLPCR_LPM_MASK);
- arm_srpgcr = __raw_readl(MXC_SRPG_ARM_SRPGCR) & ~(MXC_SRPGCR_PCR);
- empgc0 = __raw_readl(MXC_SRPG_EMPGC0_SRPGCR) & ~(MXC_SRPGCR_PCR);
- empgc1 = __raw_readl(MXC_SRPG_EMPGC1_SRPGCR) & ~(MXC_SRPGCR_PCR);
- if (state == &dev->states[WAIT_CLK_ON])
;
An if without code? This looks strange.
- else if (state == &dev->states[WAIT_CLK_OFF])
ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);- else if (state == &dev->states[WAIT_CLK_OFF_POWER_OFF]) {
/* Wait unclocked, power off */plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM| MXC_CORTEXA8_PLAT_LPC_DBG_DSM;arm_srpgcr |= MXC_SRPGCR_PCR;ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);ccm_clpcr &= ~MXC_CCM_CLPCR_VSTBY;ccm_clpcr &= ~MXC_CCM_CLPCR_SBYOS;if (tzic_enable_wake(1) != 0) {local_irq_enable();return 0;}- }
- __raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);
- __raw_writel(ccm_clpcr, MXC_CCM_CLPCR);
- __raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);
- cpu_do_idle();
- do_gettimeofday(&after);
- local_irq_enable();
- idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
(after.tv_usec - before.tv_usec);- return idle_time;
+}
+static struct cpuidle_driver imx_cpuidle_driver = {
- .name = "imx_idle",
- .owner = THIS_MODULE,
+};
+static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
+static int __init imx_cpuidle_init(void) +{
- struct cpuidle_device *device;
- int i;
- if (imx_cpuidle_params == NULL)
return -ENODEV;- cpuidle_register_driver(&imx_cpuidle_driver);
- device = &per_cpu(imx_cpuidle_device, smp_processor_id());
- device->state_count = IMX_MAX_CPUIDLE_STATE;
- for (i = 0; i < IMX_MAX_CPUIDLE_STATE && i < CPUIDLE_STATE_MAX; i++) {
device->states[i].enter = imx_enter_idle;device->states[i].exit_latency = imx_cpuidle_params[i].latency;device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;- }
- strcpy(device->states[WAIT_CLK_ON].name, "WFI 0");
- strcpy(device->states[WAIT_CLK_ON].desc, "Wait with clock on");
- strcpy(device->states[WAIT_CLK_OFF].name, "WFI 1");
- strcpy(device->states[WAIT_CLK_OFF].desc, "Wait with clock off");
- strcpy(device->states[WAIT_CLK_OFF_POWER_OFF].name, "WFI 2");
- strcpy(device->states[WAIT_CLK_OFF_POWER_OFF].desc,
"Wait with clock off and power gating");- if (cpuidle_register_device(device)) {
printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");return -ENODEV;- }
- return 0;
+}
+late_initcall(imx_cpuidle_init);
We have a late_initcall here which needs to be protected from other cpus. On the other hand we depend on board code calling imx_cpuidle_board_params() before this initcall. I think the board code should call a imx_cpuidle_init(struct imx_cpuidle_params *cpuidle_params) instead which makes the flow of execution more clear. Also, the function should be named mx51_cpuidle_init().
diff --git a/arch/arm/mach-mx5/cpuidle.h b/arch/arm/mach-mx5/cpuidle.h new file mode 100644 index 0000000..e5ba495 --- /dev/null +++ b/arch/arm/mach-mx5/cpuidle.h @@ -0,0 +1,26 @@ +/*
- arch/arm/mach-mx5/cpuidle.h
- This file is licensed under the terms of the GNU General Public
- License version 2. This program is licensed "as is" without any
- warranty of any kind, whether express or implied.
- */
+enum {
- WAIT_CLK_ON, /* c1 */
- WAIT_CLK_OFF, /* c2 */
- WAIT_CLK_OFF_POWER_OFF, /* c3 */
- IMX_MAX_CPUIDLE_STATE,
+};
+struct imx_cpuidle_params {
- unsigned int latency;
+};
+#ifdef CONFIG_CPU_IDLE +extern void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params); +#else +inline void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params) +{} +#endif
-- 1.7.1
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Sascha,
On Tue, Feb 15, 2011 at 7:27 PM, Sascha Hauer s.hauer@pengutronix.dewrote:
On Fri, Feb 11, 2011 at 10:36:12AM +0100, yong.shen@linaro.org wrote:
From: Yong Shen yong.shen@freescale.com
implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board related code.
Signed-off-by: Yong Shen yong.shen@freescale.com
arch/arm/mach-mx5/Makefile | 1 + arch/arm/mach-mx5/cpuidle.c | 113
+++++++++++++++++++++++++++++++++++++++++++
arch/arm/mach-mx5/cpuidle.h | 26 ++++++++++ 3 files changed, 140 insertions(+), 0 deletions(-) create mode 100644 arch/arm/mach-mx5/cpuidle.c create mode 100644 arch/arm/mach-mx5/cpuidle.h
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile index 0d43be9..12239e0 100644 --- a/arch/arm/mach-mx5/Makefile +++ b/arch/arm/mach-mx5/Makefile @@ -7,6 +7,7 @@ obj-y := cpu.o mm.o clock-mx51-mx53.o devices.o obj-$(CONFIG_SOC_IMX50) += mm-mx50.o
obj-$(CONFIG_CPU_FREQ_IMX) += cpu_op-mx51.o +obj-$(CONFIG_CPU_IDLE) += cpuidle.o obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o obj-$(CONFIG_MACH_MX53_EVK) += board-mx53_evk.o diff --git a/arch/arm/mach-mx5/cpuidle.c b/arch/arm/mach-mx5/cpuidle.c new file mode 100644 index 0000000..9d77c47 --- /dev/null +++ b/arch/arm/mach-mx5/cpuidle.c @@ -0,0 +1,113 @@ +/*
- arch/arm/mach-mx5/cpuidle.c
- This file is licensed under the terms of the GNU General Public
- License version 2. This program is licensed "as is" without any
- warranty of any kind, whether express or implied.
- */
+#include <linux/io.h> +#include <linux/cpuidle.h> +#include <asm/proc-fns.h> +#include <mach/hardware.h> +#include "cpuidle.h" +#include "crm_regs.h"
+static struct imx_cpuidle_params *imx_cpuidle_params; +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params) +{
imx_cpuidle_params = cpuidle_params;+}
+extern int tzic_enable_wake(int is_idle);
Please put this into a header file.
Ok, but I guess I should do it in another patch before this patch set.
+static int imx_enter_idle(struct cpuidle_device *dev,
struct cpuidle_state *state)+{
struct timeval before, after;int idle_time;u32 plat_lpc, arm_srpgcr, ccm_clpcr;u32 empgc0, empgc1;local_irq_disable();do_gettimeofday(&before);plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &~(MXC_CORTEXA8_PLAT_LPC_DSM);ccm_clpcr = __raw_readl(MXC_CCM_CLPCR) & ~(MXC_CCM_CLPCR_LPM_MASK);arm_srpgcr = __raw_readl(MXC_SRPG_ARM_SRPGCR) & ~(MXC_SRPGCR_PCR);empgc0 = __raw_readl(MXC_SRPG_EMPGC0_SRPGCR) & ~(MXC_SRPGCR_PCR);empgc1 = __raw_readl(MXC_SRPG_EMPGC1_SRPGCR) & ~(MXC_SRPGCR_PCR);if (state == &dev->states[WAIT_CLK_ON]);An if without code? This looks strange.
Yes, a little bit. I meant to treat this like a explanation for different cases, and I also can remove it and put a comments here.
else if (state == &dev->states[WAIT_CLK_OFF])ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);else if (state == &dev->states[WAIT_CLK_OFF_POWER_OFF]) {/* Wait unclocked, power off */plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM| MXC_CORTEXA8_PLAT_LPC_DBG_DSM;arm_srpgcr |= MXC_SRPGCR_PCR;ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);ccm_clpcr &= ~MXC_CCM_CLPCR_VSTBY;ccm_clpcr &= ~MXC_CCM_CLPCR_SBYOS;if (tzic_enable_wake(1) != 0) {local_irq_enable();return 0;}}__raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);__raw_writel(ccm_clpcr, MXC_CCM_CLPCR);__raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);cpu_do_idle();do_gettimeofday(&after);local_irq_enable();idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +(after.tv_usec - before.tv_usec);return idle_time;+}
+static struct cpuidle_driver imx_cpuidle_driver = {
.name = "imx_idle",.owner = THIS_MODULE,+};
+static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
+static int __init imx_cpuidle_init(void) +{
struct cpuidle_device *device;int i;if (imx_cpuidle_params == NULL)return -ENODEV;cpuidle_register_driver(&imx_cpuidle_driver);device = &per_cpu(imx_cpuidle_device, smp_processor_id());device->state_count = IMX_MAX_CPUIDLE_STATE;for (i = 0; i < IMX_MAX_CPUIDLE_STATE && i < CPUIDLE_STATE_MAX;i++) {
device->states[i].enter = imx_enter_idle;device->states[i].exit_latency =imx_cpuidle_params[i].latency;
device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;}strcpy(device->states[WAIT_CLK_ON].name, "WFI 0");strcpy(device->states[WAIT_CLK_ON].desc, "Wait with clock on");strcpy(device->states[WAIT_CLK_OFF].name, "WFI 1");strcpy(device->states[WAIT_CLK_OFF].desc, "Wait with clock off");strcpy(device->states[WAIT_CLK_OFF_POWER_OFF].name, "WFI 2");strcpy(device->states[WAIT_CLK_OFF_POWER_OFF].desc,"Wait with clock off and power gating");if (cpuidle_register_device(device)) {printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");return -ENODEV;}return 0;+}
+late_initcall(imx_cpuidle_init);
We have a late_initcall here which needs to be protected from other cpus. On the other hand we depend on board code calling imx_cpuidle_board_params() before this initcall. I think the board code should call a imx_cpuidle_init(struct imx_cpuidle_params *cpuidle_params) instead which makes the flow of execution more clear. Also, the function should be named mx51_cpuidle_init().
Then, I assume the best way might be that remove imx_cpuidle_board_params(), and let board code call mx51_cpuidle_init(), at the same time the params can be passed in by the same funciton.
diff --git a/arch/arm/mach-mx5/cpuidle.h b/arch/arm/mach-mx5/cpuidle.h new file mode 100644 index 0000000..e5ba495 --- /dev/null +++ b/arch/arm/mach-mx5/cpuidle.h @@ -0,0 +1,26 @@ +/*
- arch/arm/mach-mx5/cpuidle.h
- This file is licensed under the terms of the GNU General Public
- License version 2. This program is licensed "as is" without any
- warranty of any kind, whether express or implied.
- */
+enum {
WAIT_CLK_ON, /* c1 */WAIT_CLK_OFF, /* c2 */WAIT_CLK_OFF_POWER_OFF, /* c3 */IMX_MAX_CPUIDLE_STATE,+};
+struct imx_cpuidle_params {
unsigned int latency;+};
+#ifdef CONFIG_CPU_IDLE +extern void imx_cpuidle_board_params(struct imx_cpuidle_params
*cpuidle_params);
+#else +inline void imx_cpuidle_board_params(struct imx_cpuidle_params
*cpuidle_params)
+{} +#endif
-- 1.7.1
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-- 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 |
return 0;+}
+late_initcall(imx_cpuidle_init);
We have a late_initcall here which needs to be protected from other cpus. On the other hand we depend on board code calling imx_cpuidle_board_params() before this initcall. I think the board code should call a imx_cpuidle_init(struct imx_cpuidle_params *cpuidle_params) instead which makes the flow of execution more clear.
imx_cpuidle_init can not be called directly in board code, since it is too
early to register cpuidle driver and device which depend on some other system resource.
Yong
On Thu, Feb 17, 2011 at 09:18:11AM +0100, Yong Shen wrote:
return 0;+}
+late_initcall(imx_cpuidle_init);
We have a late_initcall here which needs to be protected from other cpus. On the other hand we depend on board code calling imx_cpuidle_board_params() before this initcall. I think the board code should call a imx_cpuidle_init(struct imx_cpuidle_params *cpuidle_params) instead which makes the flow of execution more clear.
imx_cpuidle_init can not be called directly in board code, since it is too
early to register cpuidle driver and device which depend on some other system resource.
I see. Maybe we should make this a platform driver then like for example davinci does.
Sascha
Hi Sascha,
I had sent out v3 patch before the your last comments. I noticed how davinci is doing, but some SOCs like omap, they also do it in another way like my code. However, if you prefer the way davinci is doing, I will redo it. Please confirm.
thanks Yong
On Thu, Feb 17, 2011 at 11:54 AM, Sascha Hauer s.hauer@pengutronix.dewrote:
On Thu, Feb 17, 2011 at 09:18:11AM +0100, Yong Shen wrote:
return 0;+}
+late_initcall(imx_cpuidle_init);
We have a late_initcall here which needs to be protected from other cpus. On the other hand we depend on board code calling imx_cpuidle_board_params() before this initcall. I think the board code should call a imx_cpuidle_init(struct imx_cpuidle_params *cpuidle_params) instead which makes the flow of execution more clear.
imx_cpuidle_init can not be called directly in board code, since it is
too
early to register cpuidle driver and device which depend on some other system resource.
I see. Maybe we should make this a platform driver then like for example davinci does.
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 |
Hi Sascha,
I noticed there were no comments for v3, but you had comments for v2 which was posted after v3. See below:
I see. Maybe we should make this a platform driver then like for example davinci does.
I had sent out v3 patch before the your last comments. I noticed how davinci is doing, but some SOCs like omap, they also do it in
another way like my code.
However, if you prefer the way davinci is doing, I will redo it. Please
confirm
Shall we still follow davinci's code?
Thanks Yong
On Fri, Feb 11, 2011 at 10:36:12AM +0100, yong.shen@linaro.org wrote:
From: Yong Shen yong.shen@freescale.com
implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board related code.
Signed-off-by: Yong Shen yong.shen@freescale.com
arch/arm/mach-mx5/Makefile | 1 + arch/arm/mach-mx5/cpuidle.c | 113 +++++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-mx5/cpuidle.h | 26 ++++++++++ 3 files changed, 140 insertions(+), 0 deletions(-) create mode 100644 arch/arm/mach-mx5/cpuidle.c create mode 100644 arch/arm/mach-mx5/cpuidle.h
diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile index 0d43be9..12239e0 100644 --- a/arch/arm/mach-mx5/Makefile +++ b/arch/arm/mach-mx5/Makefile @@ -7,6 +7,7 @@ obj-y := cpu.o mm.o clock-mx51-mx53.o devices.o obj-$(CONFIG_SOC_IMX50) += mm-mx50.o obj-$(CONFIG_CPU_FREQ_IMX) += cpu_op-mx51.o +obj-$(CONFIG_CPU_IDLE) += cpuidle.o obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o obj-$(CONFIG_MACH_MX53_EVK) += board-mx53_evk.o diff --git a/arch/arm/mach-mx5/cpuidle.c b/arch/arm/mach-mx5/cpuidle.c new file mode 100644 index 0000000..9d77c47 --- /dev/null +++ b/arch/arm/mach-mx5/cpuidle.c @@ -0,0 +1,113 @@ +/*
- arch/arm/mach-mx5/cpuidle.c
- This file is licensed under the terms of the GNU General Public
- License version 2. This program is licensed "as is" without any
- warranty of any kind, whether express or implied.
- */
+#include <linux/io.h> +#include <linux/cpuidle.h> +#include <asm/proc-fns.h> +#include <mach/hardware.h> +#include "cpuidle.h" +#include "crm_regs.h"
+static struct imx_cpuidle_params *imx_cpuidle_params; +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params) +{
- imx_cpuidle_params = cpuidle_params;
+}
+extern int tzic_enable_wake(int is_idle); +static int imx_enter_idle(struct cpuidle_device *dev,
struct cpuidle_state *state)+{
- struct timeval before, after;
- int idle_time;
- u32 plat_lpc, arm_srpgcr, ccm_clpcr;
- u32 empgc0, empgc1;
- local_irq_disable();
- do_gettimeofday(&before);
- plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &
~(MXC_CORTEXA8_PLAT_LPC_DSM);
One thing that strikes me here is the fact that this code can probably run on i.MX53 aswell, right? It's only that these registers have different addresses on i.MX53. The MXC_ prefix is therefore not a good idea. Switching this to MX51_ and having an additional MX53_ register leads to code duplication. This shows that it's a bad idea to code fixed addresses in the code. We should go for base + offset instead so that this code will have a better start on i.MX53. This of course needs changes in the current crm_regs.h and probably in the i.MX51/53 clock code.
- ccm_clpcr = __raw_readl(MXC_CCM_CLPCR) & ~(MXC_CCM_CLPCR_LPM_MASK);
- arm_srpgcr = __raw_readl(MXC_SRPG_ARM_SRPGCR) & ~(MXC_SRPGCR_PCR);
- empgc0 = __raw_readl(MXC_SRPG_EMPGC0_SRPGCR) & ~(MXC_SRPGCR_PCR);
- empgc1 = __raw_readl(MXC_SRPG_EMPGC1_SRPGCR) & ~(MXC_SRPGCR_PCR);
- if (state == &dev->states[WAIT_CLK_ON])
;- else if (state == &dev->states[WAIT_CLK_OFF])
ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);- else if (state == &dev->states[WAIT_CLK_OFF_POWER_OFF]) {
/* Wait unclocked, power off */plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM| MXC_CORTEXA8_PLAT_LPC_DBG_DSM;arm_srpgcr |= MXC_SRPGCR_PCR;ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);ccm_clpcr &= ~MXC_CCM_CLPCR_VSTBY;ccm_clpcr &= ~MXC_CCM_CLPCR_SBYOS;if (tzic_enable_wake(1) != 0) {local_irq_enable();return 0;}- }
- __raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);
- __raw_writel(ccm_clpcr, MXC_CCM_CLPCR);
- __raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);
- cpu_do_idle();
- do_gettimeofday(&after);
- local_irq_enable();
- idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
(after.tv_usec - before.tv_usec);- return idle_time;
+}
+static struct cpuidle_driver imx_cpuidle_driver = {
- .name = "imx_idle",
- .owner = THIS_MODULE,
+};
+static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
+static int __init imx_cpuidle_init(void) +{
- struct cpuidle_device *device;
- int i;
- if (imx_cpuidle_params == NULL)
return -ENODEV;- cpuidle_register_driver(&imx_cpuidle_driver);
- device = &per_cpu(imx_cpuidle_device, smp_processor_id());
- device->state_count = IMX_MAX_CPUIDLE_STATE;
- for (i = 0; i < IMX_MAX_CPUIDLE_STATE && i < CPUIDLE_STATE_MAX; i++) {
device->states[i].enter = imx_enter_idle;device->states[i].exit_latency = imx_cpuidle_params[i].latency;device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;- }
- strcpy(device->states[WAIT_CLK_ON].name, "WFI 0");
- strcpy(device->states[WAIT_CLK_ON].desc, "Wait with clock on");
- strcpy(device->states[WAIT_CLK_OFF].name, "WFI 1");
- strcpy(device->states[WAIT_CLK_OFF].desc, "Wait with clock off");
- strcpy(device->states[WAIT_CLK_OFF_POWER_OFF].name, "WFI 2");
- strcpy(device->states[WAIT_CLK_OFF_POWER_OFF].desc,
"Wait with clock off and power gating");- if (cpuidle_register_device(device)) {
printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");return -ENODEV;- }
- return 0;
+}
+late_initcall(imx_cpuidle_init); diff --git a/arch/arm/mach-mx5/cpuidle.h b/arch/arm/mach-mx5/cpuidle.h new file mode 100644 index 0000000..e5ba495 --- /dev/null +++ b/arch/arm/mach-mx5/cpuidle.h @@ -0,0 +1,26 @@ +/*
- arch/arm/mach-mx5/cpuidle.h
- This file is licensed under the terms of the GNU General Public
- License version 2. This program is licensed "as is" without any
- warranty of any kind, whether express or implied.
- */
+enum {
- WAIT_CLK_ON, /* c1 */
- WAIT_CLK_OFF, /* c2 */
- WAIT_CLK_OFF_POWER_OFF, /* c3 */
- IMX_MAX_CPUIDLE_STATE,
+};
+struct imx_cpuidle_params {
- unsigned int latency;
+};
+#ifdef CONFIG_CPU_IDLE +extern void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params); +#else +inline void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params) +{} +#endif
-- 1.7.1
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Sascha,
local_irq_disable();do_gettimeofday(&before);plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &~(MXC_CORTEXA8_PLAT_LPC_DSM);One thing that strikes me here is the fact that this code can probably run on i.MX53 aswell, right? It's only that these registers have different addresses on i.MX53. The MXC_ prefix is therefore not a good idea. Switching this to MX51_ and having an additional MX53_ register leads to code duplication. This shows that it's a bad idea to code fixed addresses in the code. We should go for base + offset instead so that this code will have a better start on i.MX53. This of course needs changes in the current crm_regs.h and probably in the i.MX51/53 clock code.
Yes, for mx53, it is similar. But for the case you are talking about, is it easier that we keep MXC_ prefix in this file and define MXC_ to MX51 or MX53 in crm_regs.h according to which board is running? In addition, registers for this code are not in one section, which means many BASEx + offset there, if I understand right. Do you have a sample for 'base + offset' case? since mx53 just came in, I am not sure about such case.
yong
On Wed, Feb 16, 2011 at 09:37:47AM +0100, Yong Shen wrote:
Hi Sascha,
local_irq_disable();do_gettimeofday(&before);plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &~(MXC_CORTEXA8_PLAT_LPC_DSM);One thing that strikes me here is the fact that this code can probably run on i.MX53 aswell, right? It's only that these registers have different addresses on i.MX53. The MXC_ prefix is therefore not a good idea. Switching this to MX51_ and having an additional MX53_ register leads to code duplication. This shows that it's a bad idea to code fixed addresses in the code. We should go for base + offset instead so that this code will have a better start on i.MX53. This of course needs changes in the current crm_regs.h and probably in the i.MX51/53 clock code.
Yes, for mx53, it is similar. But for the case you are talking about, is it easier that we keep MXC_ prefix in this file and define MXC_ to MX51 or MX53 in crm_regs.h according to which board is running?
I don't understand. How can we 'define' (which is compile time) to something depending on the board (which is runtime)?
In addition, registers for this code are not in one section, which means many BASEx + offset there, if I understand right. Do you have a sample for 'base + offset' case? since mx53 just came in, I am not sure about such case.
Forget it. I just realized that more or less by accident the virtual addresses for the i.MX51 and i.MX53 are the same.
Sascha
On Wed, Feb 16, 2011 at 10:13 AM, Sascha Hauer s.hauer@pengutronix.dewrote:
On Wed, Feb 16, 2011 at 09:37:47AM +0100, Yong Shen wrote:
Hi Sascha,
local_irq_disable();do_gettimeofday(&before);plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &~(MXC_CORTEXA8_PLAT_LPC_DSM);One thing that strikes me here is the fact that this code can probably run on i.MX53 aswell, right? It's only that these registers have different addresses on i.MX53. The MXC_ prefix is therefore not a good idea. Switching this to MX51_ and having an additional MX53_ register leads to code duplication. This shows that it's a bad idea to code fixed addresses in the code. We should go for base + offset instead so that this code will have a better start on i.MX53. This of course needs changes in the current crm_regs.h and probably in the i.MX51/53 clock code.
Yes, for mx53, it is similar. But for the case you are talking about, is it easier that we keep MXC_ prefix in this file and define MXC_ to MX51 or MX53 in crm_regs.h
according
to which board is running?
I don't understand. How can we 'define' (which is compile time) to something depending on the board (which is runtime)?
I ignored the goal is one image running on multiple SOCs.
In addition, registers for this code are not in one section, which means many BASEx + offset there, if I understand right. Do you have a sample
for
'base + offset' case? since mx53 just came in, I am not sure about such case.
Forget it. I just realized that more or less by accident the virtual addresses for the i.MX51 and i.MX53 are the same.
So, the conclusion is: still using MXC_ prefix in this period. right? Correct me.
Yong
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 |
From: Yong Shen yong.shen@freescale.com
This parameters are workable, but need further tuning.
Signed-off-by: Yong Shen yong.shen@freescale.com --- arch/arm/mach-mx5/board-mx51_babbage.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-mx5/board-mx51_babbage.c b/arch/arm/mach-mx5/board-mx51_babbage.c index d9d402e..168b09c 100644 --- a/arch/arm/mach-mx5/board-mx51_babbage.c +++ b/arch/arm/mach-mx5/board-mx51_babbage.c @@ -37,6 +37,7 @@ #include "devices-imx51.h" #include "devices.h" #include "cpu_op-mx51.h" +#include "cpuidle.h"
#define BABBAGE_USB_HUB_RESET IMX_GPIO_NR(1, 7) #define BABBAGE_USBH1_STP IMX_GPIO_NR(1, 27) @@ -333,6 +334,11 @@ static const struct spi_imx_master mx51_babbage_spi_pdata __initconst = { .num_chipselect = ARRAY_SIZE(mx51_babbage_spi_cs), };
+static struct imx_cpuidle_params babage_cpuidle_params[] = { + {100,}, + {150,}, + {1000,}, +}; /* * Board specific initialization. */ @@ -383,6 +389,8 @@ static void __init mxc_board_init(void) ARRAY_SIZE(mx51_babbage_spi_board_info)); imx51_add_ecspi(0, &mx51_babbage_spi_pdata); imx51_add_imx2_wdt(0, NULL); + + imx_cpuidle_board_params(babage_cpuidle_params); }
static void __init mx51_babbage_timer_init(void)
-
Sascha Hauer -
Yong Shen -
yong.shen＠linaro.org