[PATCH v2 0/2] ARM: IMX5: cpuidle driver

[PATCH v2 0/2] ARM: IMX5: cpuidle driver

[yong.shen at linaro.org]

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.

[PATCH v2 1/2] ARM: IMX5: cpuidle driver

[yong.shen at linaro.org]

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
+
-- 
1.7.1

[PATCH v2 2/2] ARM: IMX5 bbg: add cpuidle parameters

[yong.shen at linaro.org]

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)
-- 
1.7.1

[PATCH v2 1/2] ARM: IMX5: cpuidle driver

[Sascha Hauer]

On Fri, Feb 11, 2011 at 10:36:12AM +0100, yong.shen at 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 at 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 |

[PATCH v2 1/2] ARM: IMX5: cpuidle driver

[Yong Shen]

Hi Sascha,

On Tue, Feb 15, 2011 at 7:27 PM, Sascha Hauer <s.hauer@pengutronix.de>wrote:

> On Fri, Feb 11, 2011 at 10:36:12AM +0100, yong.shen at 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 at 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 |
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110216/fd146cd2/attachment-0001.html>

[PATCH v2 1/2] ARM: IMX5: cpuidle driver

[Sascha Hauer]

On Fri, Feb 11, 2011 at 10:36:12AM +0100, yong.shen at 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 at 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 |

[PATCH v2 1/2] ARM: IMX5: cpuidle driver

[Yong Shen]

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

>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110216/a8359ff2/attachment-0001.html>

[PATCH v2 1/2] ARM: IMX5: cpuidle driver

[Sascha Hauer]

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


-- 
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 |

[PATCH v2 1/2] ARM: IMX5: cpuidle driver

[Yong Shen]

On Wed, Feb 16, 2011 at 10:13 AM, Sascha Hauer <s.hauer@pengutronix.de>wrote:

> 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 |
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110216/1d858dad/attachment-0001.html>

[PATCH v2 1/2] ARM: IMX5: cpuidle driver

[Yong Shen]

>
>
> > +     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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110217/407ee15f/attachment-0001.html>

[PATCH v2 1/2] ARM: IMX5: cpuidle driver

[Sascha Hauer]

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 |

[PATCH v2 1/2] ARM: IMX5: cpuidle driver

[Yong Shen]

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.de>wrote:

> 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 |
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110220/59d8ac2c/attachment-0001.html>

[PATCH v3 1/2] ARM: IMX5: cpuidle driver

[Yong Shen]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110223/510859b3/attachment-0001.html>