Hi Sascha,<br><br><div class="gmail_quote">On Tue, Feb 15, 2011 at 7:27 PM, Sascha Hauer <span dir="ltr"><<a href="mailto:s.hauer@pengutronix.de">s.hauer@pengutronix.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5">On Fri, Feb 11, 2011 at 10:36:12AM +0100, <a href="mailto:yong.shen@linaro.org">yong.shen@linaro.org</a> wrote:<br>
> From: Yong Shen <<a href="mailto:yong.shen@freescale.com">yong.shen@freescale.com</a>><br>
><br>
> implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board<br>
> related code.<br>
><br>
> Signed-off-by: Yong Shen <<a href="mailto:yong.shen@freescale.com">yong.shen@freescale.com</a>><br>
> ---<br>
> arch/arm/mach-mx5/Makefile | 1 +<br>
> arch/arm/mach-mx5/cpuidle.c | 113 +++++++++++++++++++++++++++++++++++++++++++<br>
> arch/arm/mach-mx5/cpuidle.h | 26 ++++++++++<br>
> 3 files changed, 140 insertions(+), 0 deletions(-)<br>
> create mode 100644 arch/arm/mach-mx5/cpuidle.c<br>
> create mode 100644 arch/arm/mach-mx5/cpuidle.h<br>
><br>
> diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile<br>
> index 0d43be9..12239e0 100644<br>
> --- a/arch/arm/mach-mx5/Makefile<br>
> +++ b/arch/arm/mach-mx5/Makefile<br>
> @@ -7,6 +7,7 @@ obj-y := cpu.o mm.o clock-mx51-mx53.o devices.o<br>
> obj-$(CONFIG_SOC_IMX50) += mm-mx50.o<br>
><br>
> obj-$(CONFIG_CPU_FREQ_IMX) += cpu_op-mx51.o<br>
> +obj-$(CONFIG_CPU_IDLE) += cpuidle.o<br>
> obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o<br>
> obj-$(CONFIG_MACH_MX51_3DS) += board-mx51_3ds.o<br>
> obj-$(CONFIG_MACH_MX53_EVK) += board-mx53_evk.o<br>
> diff --git a/arch/arm/mach-mx5/cpuidle.c b/arch/arm/mach-mx5/cpuidle.c<br>
> new file mode 100644<br>
> index 0000000..9d77c47<br>
> --- /dev/null<br>
> +++ b/arch/arm/mach-mx5/cpuidle.c<br>
> @@ -0,0 +1,113 @@<br>
> +/*<br>
> + * arch/arm/mach-mx5/cpuidle.c<br>
> + *<br>
> + * This file is licensed under the terms of the GNU General Public<br>
> + * License version 2. This program is licensed "as is" without any<br>
> + * warranty of any kind, whether express or implied.<br>
> + */<br>
> +<br>
> +#include <linux/io.h><br>
> +#include <linux/cpuidle.h><br>
> +#include <asm/proc-fns.h><br>
> +#include <mach/hardware.h><br>
> +#include "cpuidle.h"<br>
> +#include "crm_regs.h"<br>
> +<br>
> +static struct imx_cpuidle_params *imx_cpuidle_params;<br>
> +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)<br>
> +{<br>
> + imx_cpuidle_params = cpuidle_params;<br>
> +}<br>
> +<br>
> +extern int tzic_enable_wake(int is_idle);<br>
<br>
</div></div>Please put this into a header file.<br></blockquote><div>Ok, but I guess I should do it in another patch before this patch set. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im"><br>
> +static int imx_enter_idle(struct cpuidle_device *dev,<br>
> + struct cpuidle_state *state)<br>
> +{<br>
> + struct timeval before, after;<br>
> + int idle_time;<br>
> + u32 plat_lpc, arm_srpgcr, ccm_clpcr;<br>
> + u32 empgc0, empgc1;<br>
> +<br>
> + local_irq_disable();<br>
> + do_gettimeofday(&before);<br>
> +<br>
> + plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &<br>
> + ~(MXC_CORTEXA8_PLAT_LPC_DSM);<br>
> + ccm_clpcr = __raw_readl(MXC_CCM_CLPCR) & ~(MXC_CCM_CLPCR_LPM_MASK);<br>
> + arm_srpgcr = __raw_readl(MXC_SRPG_ARM_SRPGCR) & ~(MXC_SRPGCR_PCR);<br>
> + empgc0 = __raw_readl(MXC_SRPG_EMPGC0_SRPGCR) & ~(MXC_SRPGCR_PCR);<br>
> + empgc1 = __raw_readl(MXC_SRPG_EMPGC1_SRPGCR) & ~(MXC_SRPGCR_PCR);<br>
> +<br>
> + if (state == &dev->states[WAIT_CLK_ON])<br>
> + ;<br>
<br>
</div>An if without code? This looks strange.<br></blockquote><div>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.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5"><br>
> + else if (state == &dev->states[WAIT_CLK_OFF])<br>
> + ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);<br>
> + else if (state == &dev->states[WAIT_CLK_OFF_POWER_OFF]) {<br>
> + /* Wait unclocked, power off */<br>
> + plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM<br>
> + | MXC_CORTEXA8_PLAT_LPC_DBG_DSM;<br>
> + arm_srpgcr |= MXC_SRPGCR_PCR;<br>
> + ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);<br>
> + ccm_clpcr &= ~MXC_CCM_CLPCR_VSTBY;<br>
> + ccm_clpcr &= ~MXC_CCM_CLPCR_SBYOS;<br>
> + if (tzic_enable_wake(1) != 0) {<br>
> + local_irq_enable();<br>
> + return 0;<br>
> + }<br>
> + }<br>
> +<br>
> + __raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);<br>
> + __raw_writel(ccm_clpcr, MXC_CCM_CLPCR);<br>
> + __raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);<br>
> +<br>
> + cpu_do_idle();<br>
> +<br>
> + do_gettimeofday(&after);<br>
> + local_irq_enable();<br>
> + idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +<br>
> + (after.tv_usec - before.tv_usec);<br>
> + return idle_time;<br>
> +}<br>
> +<br>
> +static struct cpuidle_driver imx_cpuidle_driver = {<br>
> + .name = "imx_idle",<br>
> + .owner = THIS_MODULE,<br>
> +};<br>
> +<br>
> +static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);<br>
> +<br>
> +static int __init imx_cpuidle_init(void)<br>
> +{<br>
> + struct cpuidle_device *device;<br>
> + int i;<br>
> +<br>
> + if (imx_cpuidle_params == NULL)<br>
> + return -ENODEV;<br>
> +<br>
> + cpuidle_register_driver(&imx_cpuidle_driver);<br>
> +<br>
> + device = &per_cpu(imx_cpuidle_device, smp_processor_id());<br>
> + device->state_count = IMX_MAX_CPUIDLE_STATE;<br>
> +<br>
> + for (i = 0; i < IMX_MAX_CPUIDLE_STATE && i < CPUIDLE_STATE_MAX; i++) {<br>
> + device->states[i].enter = imx_enter_idle;<br>
> + device->states[i].exit_latency = imx_cpuidle_params[i].latency;<br>
> + device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;<br>
> + }<br>
> +<br>
> + strcpy(device->states[WAIT_CLK_ON].name, "WFI 0");<br>
> + strcpy(device->states[WAIT_CLK_ON].desc, "Wait with clock on");<br>
> + strcpy(device->states[WAIT_CLK_OFF].name, "WFI 1");<br>
> + strcpy(device->states[WAIT_CLK_OFF].desc, "Wait with clock off");<br>
> + strcpy(device->states[WAIT_CLK_OFF_POWER_OFF].name, "WFI 2");<br>
> + strcpy(device->states[WAIT_CLK_OFF_POWER_OFF].desc,<br>
> + "Wait with clock off and power gating");<br>
> +<br>
> + if (cpuidle_register_device(device)) {<br>
> + printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");<br>
> + return -ENODEV;<br>
> + }<br>
> + return 0;<br>
> +}<br>
> +<br>
> +late_initcall(imx_cpuidle_init);<br>
<br>
</div></div>We have a late_initcall here which needs to be protected from other<br>
cpus. On the other hand we depend on board code calling<br>
imx_cpuidle_board_params() before this initcall. I think the board code<br>
should call a imx_cpuidle_init(struct imx_cpuidle_params<br>
*cpuidle_params) instead which makes the flow of execution more clear.<br>
Also, the function should be named mx51_cpuidle_init().<br></blockquote><div><br></div><div>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. </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im"><br>
> diff --git a/arch/arm/mach-mx5/cpuidle.h b/arch/arm/mach-mx5/cpuidle.h<br>
> new file mode 100644<br>
> index 0000000..e5ba495<br>
> --- /dev/null<br>
> +++ b/arch/arm/mach-mx5/cpuidle.h<br>
> @@ -0,0 +1,26 @@<br>
> +/*<br>
> + * arch/arm/mach-mx5/cpuidle.h<br>
> + *<br>
> + * This file is licensed under the terms of the GNU General Public<br>
> + * License version 2. This program is licensed "as is" without any<br>
> + * warranty of any kind, whether express or implied.<br>
> + */<br>
> +<br>
> +enum {<br>
> + WAIT_CLK_ON, /* c1 */<br>
> + WAIT_CLK_OFF, /* c2 */<br>
> + WAIT_CLK_OFF_POWER_OFF, /* c3 */<br>
> + IMX_MAX_CPUIDLE_STATE,<br>
> +};<br>
> +<br>
> +struct imx_cpuidle_params {<br>
> + unsigned int latency;<br>
> +};<br>
> +<br>
> +#ifdef CONFIG_CPU_IDLE<br>
> +extern void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params);<br>
> +#else<br>
> +inline void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)<br>
> +{}<br>
> +#endif<br>
> +<br>
> --<br>
> 1.7.1<br>
><br>
><br>
</div>> _______________________________________________<br>
> linux-arm-kernel mailing list<br>
> <a href="mailto:linux-arm-kernel@lists.infradead.org">linux-arm-kernel@lists.infradead.org</a><br>
> <a href="http://lists.infradead.org/mailman/listinfo/linux-arm-kernel" target="_blank">http://lists.infradead.org/mailman/listinfo/linux-arm-kernel</a><br>
><br>
<font color="#888888"><br>
--<br>
Pengutronix e.K. | |<br>
Industrial Linux Solutions | <a href="http://www.pengutronix.de/" target="_blank">http://www.pengutronix.de/</a> |<br>
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |<br>
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |<br>
</font></blockquote></div><br>