Hi Sascha,
On Tue, Feb 15, 2011 at 7:27 PM, Sascha Hauer <s.ha...@pengutronix.de>wrote:
> On Fri, Feb 11, 2011 at 10:36:12AM +0100, yong.s...@linaro.org wrote:
> > From: Yong Shen <yong.s...@freescale.com>
> >
> > implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board
> > related code.
> >
> > Signed-off-by: Yong Shen <yong.s...@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-ker...@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 |
>
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
