Skip to contentSkip to conversation

ARM: IMX5: cpuidle driver

PATCH Superseded yong.shen at linaro.org 2 participants 13 messages Feb 11, 2011 – Feb 23, 2011

Subsystems (2)
Arm (32-bit) Arm sub-architectures
lore
Share & tools
lore

yong.shen at linaro.org

author
#0 lore raw

v2 cover ARM: IMX5: cpuidle driver

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.

author
#1 lore raw

PATCH v2 1/2 ARM: IMX5: cpuidle driver

From: Yong Shen <[email protected]>

implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board related code.

Signed-off-by Yong Shen <[email protected]>
---
 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
arch/arm/mach-mx5/Makefile +1 -0
index 0d43be9..12239e0 100644
@@ -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
arch/arm/mach-mx5/cpuidle.c new file +113 -0

113 lines not on this page · raw message

arch/arm/mach-mx5/cpuidle.h new file +26 -0
new file mode 100644
index 0000000..e5ba495
@@ -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
+
Signature · 1 line
1.7.1
#2 lore raw

PATCH v2 1/2 ARM: IMX5: cpuidle driver

Quoted 44 lines · yong.shen at linaro.org

From: Yong Shen <[email protected]>

implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board related code.

Signed-off-by Yong Shen <[email protected]>
---
 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
arch/arm/mach-mx5/Makefile +1 -0
index 0d43be9..12239e0 100644
@@ -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
arch/arm/mach-mx5/cpuidle.c new file +12 -0
new file mode 100644
index 0000000..9d77c47
@@ -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>

arch/arm/mach-mx5/cpuidle.c line 22

Quoted 10 lines
arch/arm/mach-mx5/cpuidle.c new file +10 -0
+#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.

Quoted 10 lines
arch/arm/mach-mx5/cpuidle.c new file +10 -0
+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);

arch/arm/mach-mx5/cpuidle.c line 42

Quoted 10 lines
arch/arm/mach-mx5/cpuidle.c new file +10 -0
+
+	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.

Quoted 61 lines
arch/arm/mach-mx5/cpuidle.c new file +61 -0
+	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,

arch/arm/mach-mx5/cpuidle.c line 113

Quoted 10 lines
arch/arm/mach-mx5/cpuidle.c new file +10 -0
+			"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().

Quoted 41 lines
arch/arm/mach-mx5/cpuidle.h new file +26 -0
new file mode 100644
index 0000000..e5ba495
@@ -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
+
Signature · 7 lines
1.7.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 4 lines
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 |
author
#3 lore raw

PATCH v2 1/2 ARM: IMX5: cpuidle driver

Hi Sascha,

Quoted 50 lines · Sascha Hauer
Quoted 9 lines · yong.shen at linaro.org

From: Yong Shen <[email protected]>

implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board related code.

Signed-off-by Yong Shen <[email protected]>
---
 arch/arm/mach-mx5/Makefile  |    1 +
 arch/arm/mach-mx5/cpuidle.c |  113

+++++++++++++++++++++++++++++++++++++++++++

Quoted 39 lines
 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
arch/arm/mach-mx5/Makefile +1 -0
index 0d43be9..12239e0 100644
@@ -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
arch/arm/mach-mx5/cpuidle.c new file +16 -0
new file mode 100644
index 0000000..9d77c47
@@ -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;
Quoted 9 lines

arch/arm/mach-mx5/cpuidle.c line 22

Quoted 6 lines
arch/arm/mach-mx5/cpuidle.c new file +6 -0
+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.

Quoted 15 lines
Quoted 15 lines
arch/arm/mach-mx5/cpuidle.c new file +14 -0
+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);
Quoted 9 lines

arch/arm/mach-mx5/cpuidle.c line 42

Quoted 6 lines
arch/arm/mach-mx5/cpuidle.c new file +6 -0
+     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.

Quoted 73 lines
Quoted 41 lines
arch/arm/mach-mx5/cpuidle.c new file +40 -0
+     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;

arch/arm/mach-mx5/cpuidle.c

Quoted 10 lines
arch/arm/mach-mx5/cpuidle.c new file +10 -0
+
+     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++) {

arch/arm/mach-mx5/cpuidle.c line 94

Quoted 2 lines
arch/arm/mach-mx5/cpuidle.c new file +2 -0
+             device->states[i].enter = imx_enter_idle;
+             device->states[i].exit_latency =

imx_cpuidle_params[i].latency;

Quoted 18 lines
arch/arm/mach-mx5/cpuidle.c new file +18 -0
+             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;
+}
+
Quoted 9 lines

arch/arm/mach-mx5/cpuidle.c line 113

Quoted 1 line
arch/arm/mach-mx5/cpuidle.c new file +1 -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.

Quoted 46 lines
Quoted 18 lines
arch/arm/mach-mx5/cpuidle.h new file +11 -0
new file mode 100644
index 0000000..e5ba495
@@ -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 */

arch/arm/mach-mx5/cpuidle.h line 21

Quoted 10 lines
arch/arm/mach-mx5/cpuidle.h new file +10 -0
+     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);

arch/arm/mach-mx5/cpuidle.h line 23

Quoted 2 lines
arch/arm/mach-mx5/cpuidle.h new file +2 -0
+#else
+inline void imx_cpuidle_board_params(struct imx_cpuidle_params

*cpuidle_params)

Quoted 7 lines
arch/arm/mach-mx5/cpuidle.h new file +3 -1
+{}
+#endif
+
--

1.7.1

Quoted 5 lines

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--

Quoted 5 lines
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>

author
#4 lore raw

PATCH v2 1/2 ARM: IMX5: cpuidle driver

In reply to Sascha Hauer
Quoted 13 lines

arch/arm/mach-mx5/cpuidle.c line 113

Quoted 6 lines
arch/arm/mach-mx5/cpuidle.c new file +4 -0
+     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.

Quoted 1 line

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>

#5 lore raw

PATCH v2 1/2 ARM: IMX5: cpuidle driver

Quoted 13 lines · Yong Shen
Quoted 13 lines

arch/arm/mach-mx5/cpuidle.c line 113

Quoted 6 lines
arch/arm/mach-mx5/cpuidle.c new file +4 -0
+     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.

Quoted 3 lines
Quoted 1 line

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

Signature · 4 lines
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 |
author
#6 lore raw

PATCH v2 1/2 ARM: IMX5: cpuidle driver

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

Quoted 22 lines · Sascha Hauer
Quoted 13 lines · Yong Shen
Quoted 13 lines

arch/arm/mach-mx5/cpuidle.c line 113

Quoted 6 lines
arch/arm/mach-mx5/cpuidle.c new file +4 -0
+     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.

Quoted 1 line
Quoted 1 line

imx_cpuidle_init can not be called directly in board code, since it is

too

Quoted 2 lines

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.

Quoted 8 lines

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>

author
#7 lore raw

PATCH v3 1/2 ARM: IMX5: cpuidle driver

Hi Sascha,

I noticed there were no comments for v3, but you had comments for v2 which was posted after v3. See below:

Quoted 5 lines
Quoted 2 lines

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.

Quoted 1 line

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>

#8 lore raw

PATCH v2 1/2 ARM: IMX5: cpuidle driver

In reply to yong.shen at linaro.org
Quoted 57 lines · yong.shen at linaro.org

From: Yong Shen <[email protected]>

implement cpuidle driver for iMX5 SOCs, leave cpuidle params to board related code.

Signed-off-by Yong Shen <[email protected]>
---
 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
arch/arm/mach-mx5/Makefile +1 -0
index 0d43be9..12239e0 100644
@@ -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
arch/arm/mach-mx5/cpuidle.c new file +25 -0
new file mode 100644
index 0000000..9d77c47
@@ -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)
+{

arch/arm/mach-mx5/cpuidle.c line 35

Quoted 10 lines
arch/arm/mach-mx5/cpuidle.c new file +10 -0
+	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.

Quoted 119 lines
arch/arm/mach-mx5/cpuidle.c new file +78 -0
+	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);
arch/arm/mach-mx5/cpuidle.h new file +26 -0
new file mode 100644
index 0000000..e5ba495
@@ -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
+
Signature · 7 lines
1.7.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 4 lines
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 |
author
#9 lore raw

PATCH v2 1/2 ARM: IMX5: cpuidle driver

Hi Sascha,

Quoted 6 lines

arch/arm/mach-mx5/cpuidle.c line 35

Quoted 5 lines
arch/arm/mach-mx5/cpuidle.c new file +5 -0
+     local_irq_disable();
+     do_gettimeofday(&before);
+
+     plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &
+         ~(MXC_CORTEXA8_PLAT_LPC_DSM);
Quoted 10 lines

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

Quoted 2 lines

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

#10 lore raw

PATCH v2 1/2 ARM: IMX5: cpuidle driver

Quoted 19 lines · Yong Shen

Hi Sascha,

Quoted 6 lines

arch/arm/mach-mx5/cpuidle.c line 35

Quoted 5 lines
arch/arm/mach-mx5/cpuidle.c new file +5 -0
+     local_irq_disable();
+     do_gettimeofday(&before);
+
+     plat_lpc = __raw_readl(MXC_CORTEXA8_PLAT_LPC) &
+         ~(MXC_CORTEXA8_PLAT_LPC_DSM);
Quoted 10 lines

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.

Quoted 4 lines

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

Quoted 4 lines

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

Signature · 4 lines
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 |
author
#11 lore raw

PATCH v2 1/2 ARM: IMX5: cpuidle driver

Quoted 20 lines · Sascha Hauer
Quoted 19 lines · Yong Shen

Hi Sascha,

Quoted 16 lines

arch/arm/mach-mx5/cpuidle.c line 35

Quoted 5 lines
arch/arm/mach-mx5/cpuidle.c new file +5 -0
+     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.

Quoted 8 lines
Quoted 3 lines

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

Quoted 1 line

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.

Quoted 12 lines
Quoted 5 lines

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

Quoted 2 lines

'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

Quoted 10 lines

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>

author
#12 lore raw

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

In reply to yong.shen at linaro.org

From: Yong Shen <[email protected]>

This parameters are workable, but need further tuning.

Signed-off-by Yong Shen <[email protected]>
---
 arch/arm/mach-mx5/board-mx51_babbage.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
arch/arm/mach-mx5/board-mx51_babbage.c +8 -0
index d9d402e..168b09c 100644
@@ -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)
Signature · 1 line
1.7.1