[PATCH v2 0/3] Add imx cpuidle

[PATCH v2 0/3] Add imx cpuidle

[Robert Lee]

Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q
platform cpuidle implementation.

Based on v3.4-rc5 plus recently submitted device tree late_initcall patch:
 http://www.spinics.net/lists/arm-kernel/msg171620.html

Changes since v1:
 * Removed some unnecessary  spaces
 * Added return value for an error message
 * Reworked init scheme to use device tree late_initcall.
 * Moved imx6q and imx5 cpuidle functionality to existing files.

Robert Lee (3):
  ARM: imx: Add common imx cpuidle init functionality.
  ARM: imx: Add imx5 cpuidle driver
  ARM: imx: Add imx6q cpuidle driver

 arch/arm/mach-imx/cpuidle-imx6q.c        |   33 ++++++++++++
 arch/arm/mach-imx/mach-imx6q.c           |   18 +++++++
 arch/arm/mach-imx/mm-imx5.c              |   42 ++++++++++++++--
 arch/arm/plat-mxc/Makefile               |    1 +
 arch/arm/plat-mxc/cpuidle.c              |   80 ++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/cpuidle.h |   22 ++++++++
 6 files changed, 193 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
 create mode 100644 arch/arm/plat-mxc/cpuidle.c
 create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h

-- 
1.7.10

[PATCH v2 0/3] Add imx cpuidle

[Robert Lee]

Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q
platform cpuidle implementation.

Based on v3.4-rc5 plus recently submitted device tree late_initcall patch:
 http://www.spinics.net/lists/arm-kernel/msg171620.html

Changes since v1:
 * Removed some unnecessary  spaces
 * Added return value for an error message
 * Reworked init scheme to use device tree late_initcall.
 * Moved imx6q and imx5 cpuidle functionality to existing files.

Robert Lee (3):
  ARM: imx: Add common imx cpuidle init functionality.
  ARM: imx: Add imx5 cpuidle driver
  ARM: imx: Add imx6q cpuidle driver

 arch/arm/mach-imx/cpuidle-imx6q.c        |   33 ++++++++++++
 arch/arm/mach-imx/mach-imx6q.c           |   18 +++++++
 arch/arm/mach-imx/mm-imx5.c              |   42 ++++++++++++++--
 arch/arm/plat-mxc/Makefile               |    1 +
 arch/arm/plat-mxc/cpuidle.c              |   80 ++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/cpuidle.h |   22 ++++++++
 6 files changed, 193 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
 create mode 100644 arch/arm/plat-mxc/cpuidle.c
 create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h

-- 
1.7.10

[PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Robert Lee]

Add common cpuidle init functionality that can be used by various
imx platforms.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/plat-mxc/Makefile               |    1 +
 arch/arm/plat-mxc/cpuidle.c              |   80 ++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/cpuidle.h |   22 ++++++++
 3 files changed, 103 insertions(+)
 create mode 100644 arch/arm/plat-mxc/cpuidle.c
 create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h

diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index e81290c..63b064b 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
 obj-$(CONFIG_MXC_USE_EPIT) += epit.o
 obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
 obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
+obj-$(CONFIG_CPU_IDLE) += cpuidle.o
 ifdef CONFIG_SND_IMX_SOC
 obj-y += ssi-fiq.o
 obj-y += ssi-fiq-ksym.o
diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
new file mode 100644
index 0000000..b7a5e1c
--- /dev/null
+++ b/arch/arm/plat-mxc/cpuidle.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/cpuidle.h>
+#include <linux/hrtimer.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+
+static struct cpuidle_device __percpu * imx_cpuidle_devices;
+
+void imx_cpuidle_devices_uninit(void)
+{
+	int cpu_id;
+	struct cpuidle_device *dev;
+
+	for_each_possible_cpu(cpu_id) {
+		dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+		cpuidle_unregister_device(dev);
+	}
+
+	free_percpu(imx_cpuidle_devices);
+}
+
+int __init imx_cpuidle_init(struct cpuidle_driver *drv)
+{
+	struct cpuidle_device *dev;
+	int cpu_id, ret;
+
+	if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
+		pr_err("%s: Invalid Input\n", __func__);
+		return -EINVAL;
+	}
+
+	ret = cpuidle_register_driver(drv);
+	if (ret) {
+		pr_err("%s: Failed to register cpuidle driver with error: %d\n",
+			 __func__, ret);
+		return ret;
+	}
+
+	imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+	if (imx_cpuidle_devices == NULL) {
+		ret = -ENOMEM;
+		goto unregister_drv;
+	}
+
+	/* initialize state data for each cpuidle_device */
+	for_each_possible_cpu(cpu_id) {
+		dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+		dev->cpu = cpu_id;
+		dev->state_count = drv->state_count;
+
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("%s: Failed to register cpu %u\n",
+				__func__, cpu_id);
+			goto uninit;
+		}
+	}
+
+	return 0;
+
+uninit:
+	imx_cpuidle_devices_uninit();
+
+unregister_drv:
+	cpuidle_unregister_driver(drv);
+	return ret;
+}
diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
new file mode 100644
index 0000000..8612510
--- /dev/null
+++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/cpuidle.h>
+
+#ifdef CONFIG_CPU_IDLE
+extern void imx_cpuidle_devices_uninit(void);
+extern int imx_cpuidle_init(struct cpuidle_driver *drv);
+#else
+static inline void imx_cpuidle_devices_uninit(void) {}
+static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
+{ return -ENODEV; }
+#endif
-- 
1.7.10

[PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Robert Lee]

Add common cpuidle init functionality that can be used by various
imx platforms.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/plat-mxc/Makefile               |    1 +
 arch/arm/plat-mxc/cpuidle.c              |   80 ++++++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/cpuidle.h |   22 ++++++++
 3 files changed, 103 insertions(+)
 create mode 100644 arch/arm/plat-mxc/cpuidle.c
 create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h

diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index e81290c..63b064b 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
 obj-$(CONFIG_MXC_USE_EPIT) += epit.o
 obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
 obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
+obj-$(CONFIG_CPU_IDLE) += cpuidle.o
 ifdef CONFIG_SND_IMX_SOC
 obj-y += ssi-fiq.o
 obj-y += ssi-fiq-ksym.o
diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
new file mode 100644
index 0000000..b7a5e1c
--- /dev/null
+++ b/arch/arm/plat-mxc/cpuidle.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/cpuidle.h>
+#include <linux/hrtimer.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+
+static struct cpuidle_device __percpu * imx_cpuidle_devices;
+
+void imx_cpuidle_devices_uninit(void)
+{
+	int cpu_id;
+	struct cpuidle_device *dev;
+
+	for_each_possible_cpu(cpu_id) {
+		dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+		cpuidle_unregister_device(dev);
+	}
+
+	free_percpu(imx_cpuidle_devices);
+}
+
+int __init imx_cpuidle_init(struct cpuidle_driver *drv)
+{
+	struct cpuidle_device *dev;
+	int cpu_id, ret;
+
+	if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
+		pr_err("%s: Invalid Input\n", __func__);
+		return -EINVAL;
+	}
+
+	ret = cpuidle_register_driver(drv);
+	if (ret) {
+		pr_err("%s: Failed to register cpuidle driver with error: %d\n",
+			 __func__, ret);
+		return ret;
+	}
+
+	imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+	if (imx_cpuidle_devices == NULL) {
+		ret = -ENOMEM;
+		goto unregister_drv;
+	}
+
+	/* initialize state data for each cpuidle_device */
+	for_each_possible_cpu(cpu_id) {
+		dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
+		dev->cpu = cpu_id;
+		dev->state_count = drv->state_count;
+
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("%s: Failed to register cpu %u\n",
+				__func__, cpu_id);
+			goto uninit;
+		}
+	}
+
+	return 0;
+
+uninit:
+	imx_cpuidle_devices_uninit();
+
+unregister_drv:
+	cpuidle_unregister_driver(drv);
+	return ret;
+}
diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
new file mode 100644
index 0000000..8612510
--- /dev/null
+++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/cpuidle.h>
+
+#ifdef CONFIG_CPU_IDLE
+extern void imx_cpuidle_devices_uninit(void);
+extern int imx_cpuidle_init(struct cpuidle_driver *drv);
+#else
+static inline void imx_cpuidle_devices_uninit(void) {}
+static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
+{ return -ENODEV; }
+#endif
-- 
1.7.10

[PATCH v2 2/3] ARM: imx: Add imx5 cpuidle driver

[Robert Lee]

Add imx5 cpuidle driver.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-imx/mm-imx5.c |   42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index d6b7e9f..cbd9bad 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -22,24 +22,59 @@
 #include <mach/common.h>
 #include <mach/devices-common.h>
 #include <mach/iomux-v3.h>
+#include <mach/cpuidle.h>
 
 static struct clk *gpc_dvfs_clk;
 
-static void imx5_idle(void)
+static int imx5_idle(void)
 {
+	int ret = 0;
+
 	/* gpc clock is needed for SRPG */
 	if (gpc_dvfs_clk == NULL) {
 		gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs");
 		if (IS_ERR(gpc_dvfs_clk))
-			return;
+			return -ENODEV;
 	}
 	clk_enable(gpc_dvfs_clk);
 	mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
 	if (!tzic_enable_wake())
 		cpu_do_idle();
+	else
+		ret = -EBUSY;
 	clk_disable(gpc_dvfs_clk);
+
+	return ret;
+}
+
+static int imx5_cpuidle_enter(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int idx)
+{
+	int ret;
+
+	ret = imx5_idle();
+
+	if (ret < 0)
+		return ret;
+
+	return idx;
 }
 
+static struct cpuidle_driver imx5_cpuidle_driver = {
+	.name			= "imx5_cpuidle",
+	.owner			= THIS_MODULE,
+	.en_core_tk_irqen	= 1,
+	.states[0]	= {
+		.enter			= imx5_cpuidle_enter,
+		.exit_latency		= 20, /* max latency at 160MHz */
+		.target_residency	= 1,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "IMX5 SRPG",
+		.desc			= "CPU state retained,powered off",
+	},
+	.state_count		= 1,
+};
+
 /*
  * Define the MX50 memory map.
  */
@@ -103,7 +138,7 @@ void __init imx51_init_early(void)
 	mxc_set_cpu_type(MXC_CPU_MX51);
 	mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
 	mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
-	arm_pm_idle = imx5_idle;
+	arm_pm_idle = (void *)imx5_idle;
 }
 
 void __init imx53_init_early(void)
@@ -238,4 +273,5 @@ void __init imx53_soc_init(void)
 void __init imx51_init_late(void)
 {
 	mx51_neon_fixup();
+	imx_cpuidle_init(&imx5_cpuidle_driver);
 }
-- 
1.7.10

[PATCH v2 2/3] ARM: imx: Add imx5 cpuidle driver

[Robert Lee]

Add imx5 cpuidle driver.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-imx/mm-imx5.c |   42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index d6b7e9f..cbd9bad 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -22,24 +22,59 @@
 #include <mach/common.h>
 #include <mach/devices-common.h>
 #include <mach/iomux-v3.h>
+#include <mach/cpuidle.h>
 
 static struct clk *gpc_dvfs_clk;
 
-static void imx5_idle(void)
+static int imx5_idle(void)
 {
+	int ret = 0;
+
 	/* gpc clock is needed for SRPG */
 	if (gpc_dvfs_clk == NULL) {
 		gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs");
 		if (IS_ERR(gpc_dvfs_clk))
-			return;
+			return -ENODEV;
 	}
 	clk_enable(gpc_dvfs_clk);
 	mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
 	if (!tzic_enable_wake())
 		cpu_do_idle();
+	else
+		ret = -EBUSY;
 	clk_disable(gpc_dvfs_clk);
+
+	return ret;
+}
+
+static int imx5_cpuidle_enter(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int idx)
+{
+	int ret;
+
+	ret = imx5_idle();
+
+	if (ret < 0)
+		return ret;
+
+	return idx;
 }
 
+static struct cpuidle_driver imx5_cpuidle_driver = {
+	.name			= "imx5_cpuidle",
+	.owner			= THIS_MODULE,
+	.en_core_tk_irqen	= 1,
+	.states[0]	= {
+		.enter			= imx5_cpuidle_enter,
+		.exit_latency		= 20, /* max latency@160MHz */
+		.target_residency	= 1,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "IMX5 SRPG",
+		.desc			= "CPU state retained,powered off",
+	},
+	.state_count		= 1,
+};
+
 /*
  * Define the MX50 memory map.
  */
@@ -103,7 +138,7 @@ void __init imx51_init_early(void)
 	mxc_set_cpu_type(MXC_CPU_MX51);
 	mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
 	mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
-	arm_pm_idle = imx5_idle;
+	arm_pm_idle = (void *)imx5_idle;
 }
 
 void __init imx53_init_early(void)
@@ -238,4 +273,5 @@ void __init imx53_soc_init(void)
 void __init imx51_init_late(void)
 {
 	mx51_neon_fixup();
+	imx_cpuidle_init(&imx5_cpuidle_driver);
 }
-- 
1.7.10

[PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver

[Robert Lee]

Add basic imx6q cpuidle driver.  For now, only basic WFI state is
supported.  Deeper idle states will be added in the future.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-imx/cpuidle-imx6q.c |   33 +++++++++++++++++++++++++++++++++
 arch/arm/mach-imx/mach-imx6q.c    |   18 ++++++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c

diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
new file mode 100644
index 0000000..b74557f
--- /dev/null
+++ b/arch/arm/mach-imx/cpuidle-imx6q.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/cpuidle.h>
+#include <linux/export.h>
+#include <asm/cpuidle.h>
+#include <mach/cpuidle.h>
+
+static struct cpuidle_driver imx6q_cpuidle_driver = {
+	.name			= "imx6q_cpuidle",
+	.owner			= THIS_MODULE,
+	.en_core_tk_irqen	= 1,
+	.states[0]		= ARM_CPUIDLE_WFI_STATE,
+	.state_count		= 1,
+};
+
+int __init imx6q_cpuidle_init(void)
+{
+	imx_cpuidle_set_driver(&imx6q_cpuidle_driver);
+
+	return 0;
+}
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index da6c1d9..21e2051 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -21,6 +21,9 @@
 #include <linux/of_platform.h>
 #include <linux/phy.h>
 #include <linux/micrel_phy.h>
+#include <linux/export.h>
+#include <linux/cpuidle.h>
+#include <asm/cpuidle.h>
 #include <asm/smp_twd.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/hardware/gic.h>
@@ -29,6 +32,7 @@
 #include <asm/system_misc.h>
 #include <mach/common.h>
 #include <mach/hardware.h>
+#include <mach/cpuidle.h>
 
 void imx6q_restart(char mode, const char *cmd)
 {
@@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void)
 	imx6q_pm_init();
 }
 
+static struct cpuidle_driver imx6q_cpuidle_driver = {
+	.name			= "imx6q_cpuidle",
+	.owner			= THIS_MODULE,
+	.en_core_tk_irqen	= 1,
+	.states[0]		= ARM_CPUIDLE_WFI_STATE,
+	.state_count		= 1,
+};
+
+static void __init imx6q_init_late(void)
+{
+	imx_cpuidle_init(&imx6q_cpuidle_driver);
+}
+
 static void __init imx6q_map_io(void)
 {
 	imx_lluart_map_io();
@@ -142,6 +159,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)")
 	.handle_irq	= imx6q_handle_irq,
 	.timer		= &imx6q_timer,
 	.init_machine	= imx6q_init_machine,
+	.init_late      = imx6q_init_late,
 	.dt_compat	= imx6q_dt_compat,
 	.restart	= imx6q_restart,
 MACHINE_END
-- 
1.7.10

[PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver

[Robert Lee]

Add basic imx6q cpuidle driver.  For now, only basic WFI state is
supported.  Deeper idle states will be added in the future.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/mach-imx/cpuidle-imx6q.c |   33 +++++++++++++++++++++++++++++++++
 arch/arm/mach-imx/mach-imx6q.c    |   18 ++++++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c

diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
new file mode 100644
index 0000000..b74557f
--- /dev/null
+++ b/arch/arm/mach-imx/cpuidle-imx6q.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/cpuidle.h>
+#include <linux/export.h>
+#include <asm/cpuidle.h>
+#include <mach/cpuidle.h>
+
+static struct cpuidle_driver imx6q_cpuidle_driver = {
+	.name			= "imx6q_cpuidle",
+	.owner			= THIS_MODULE,
+	.en_core_tk_irqen	= 1,
+	.states[0]		= ARM_CPUIDLE_WFI_STATE,
+	.state_count		= 1,
+};
+
+int __init imx6q_cpuidle_init(void)
+{
+	imx_cpuidle_set_driver(&imx6q_cpuidle_driver);
+
+	return 0;
+}
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index da6c1d9..21e2051 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -21,6 +21,9 @@
 #include <linux/of_platform.h>
 #include <linux/phy.h>
 #include <linux/micrel_phy.h>
+#include <linux/export.h>
+#include <linux/cpuidle.h>
+#include <asm/cpuidle.h>
 #include <asm/smp_twd.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/hardware/gic.h>
@@ -29,6 +32,7 @@
 #include <asm/system_misc.h>
 #include <mach/common.h>
 #include <mach/hardware.h>
+#include <mach/cpuidle.h>
 
 void imx6q_restart(char mode, const char *cmd)
 {
@@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void)
 	imx6q_pm_init();
 }
 
+static struct cpuidle_driver imx6q_cpuidle_driver = {
+	.name			= "imx6q_cpuidle",
+	.owner			= THIS_MODULE,
+	.en_core_tk_irqen	= 1,
+	.states[0]		= ARM_CPUIDLE_WFI_STATE,
+	.state_count		= 1,
+};
+
+static void __init imx6q_init_late(void)
+{
+	imx_cpuidle_init(&imx6q_cpuidle_driver);
+}
+
 static void __init imx6q_map_io(void)
 {
 	imx_lluart_map_io();
@@ -142,6 +159,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)")
 	.handle_irq	= imx6q_handle_irq,
 	.timer		= &imx6q_timer,
 	.init_machine	= imx6q_init_machine,
+	.init_late      = imx6q_init_late,
 	.dt_compat	= imx6q_dt_compat,
 	.restart	= imx6q_restart,
 MACHINE_END
-- 
1.7.10

Re: [PATCH v2 0/3] Add imx cpuidle

[Shawn Guo]

On Tue, May 01, 2012 at 09:12:37PM -0500, Robert Lee wrote:
> Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q
> platform cpuidle implementation.
> 
> Based on v3.4-rc5 plus recently submitted device tree late_initcall patch:

Just to clarify, this is not a device tree late_initcall but a machine
specific late_initcall time hook.  It has nothing to do with device
tree.

Regards,
Shawn

>  http://www.spinics.net/lists/arm-kernel/msg171620.html
> 
> Changes since v1:
>  * Removed some unnecessary  spaces
>  * Added return value for an error message
>  * Reworked init scheme to use device tree late_initcall.
>  * Moved imx6q and imx5 cpuidle functionality to existing files.
> 
> Robert Lee (3):
>   ARM: imx: Add common imx cpuidle init functionality.
>   ARM: imx: Add imx5 cpuidle driver
>   ARM: imx: Add imx6q cpuidle driver
> 
>  arch/arm/mach-imx/cpuidle-imx6q.c        |   33 ++++++++++++
>  arch/arm/mach-imx/mach-imx6q.c           |   18 +++++++
>  arch/arm/mach-imx/mm-imx5.c              |   42 ++++++++++++++--
>  arch/arm/plat-mxc/Makefile               |    1 +
>  arch/arm/plat-mxc/cpuidle.c              |   80 ++++++++++++++++++++++++++++++
>  arch/arm/plat-mxc/include/mach/cpuidle.h |   22 ++++++++
>  6 files changed, 193 insertions(+), 3 deletions(-)
>  create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
>  create mode 100644 arch/arm/plat-mxc/cpuidle.c
>  create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
> 
> -- 
> 1.7.10
> 

[PATCH v2 0/3] Add imx cpuidle

[Shawn Guo]

On Tue, May 01, 2012 at 09:12:37PM -0500, Robert Lee wrote:
> Add common imx cpuidle initialization functionality and add a i.MX5 and i.MX6Q
> platform cpuidle implementation.
> 
> Based on v3.4-rc5 plus recently submitted device tree late_initcall patch:

Just to clarify, this is not a device tree late_initcall but a machine
specific late_initcall time hook.  It has nothing to do with device
tree.

Regards,
Shawn

>  http://www.spinics.net/lists/arm-kernel/msg171620.html
> 
> Changes since v1:
>  * Removed some unnecessary  spaces
>  * Added return value for an error message
>  * Reworked init scheme to use device tree late_initcall.
>  * Moved imx6q and imx5 cpuidle functionality to existing files.
> 
> Robert Lee (3):
>   ARM: imx: Add common imx cpuidle init functionality.
>   ARM: imx: Add imx5 cpuidle driver
>   ARM: imx: Add imx6q cpuidle driver
> 
>  arch/arm/mach-imx/cpuidle-imx6q.c        |   33 ++++++++++++
>  arch/arm/mach-imx/mach-imx6q.c           |   18 +++++++
>  arch/arm/mach-imx/mm-imx5.c              |   42 ++++++++++++++--
>  arch/arm/plat-mxc/Makefile               |    1 +
>  arch/arm/plat-mxc/cpuidle.c              |   80 ++++++++++++++++++++++++++++++
>  arch/arm/plat-mxc/include/mach/cpuidle.h |   22 ++++++++
>  6 files changed, 193 insertions(+), 3 deletions(-)
>  create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
>  create mode 100644 arch/arm/plat-mxc/cpuidle.c
>  create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
> 
> -- 
> 1.7.10
> 

Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Shawn Guo]

On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
> Add common cpuidle init functionality that can be used by various
> imx platforms.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/plat-mxc/Makefile               |    1 +
>  arch/arm/plat-mxc/cpuidle.c              |   80 ++++++++++++++++++++++++++++++
>  arch/arm/plat-mxc/include/mach/cpuidle.h |   22 ++++++++
>  3 files changed, 103 insertions(+)
>  create mode 100644 arch/arm/plat-mxc/cpuidle.c
>  create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
> 
> diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
> index e81290c..63b064b 100644
> --- a/arch/arm/plat-mxc/Makefile
> +++ b/arch/arm/plat-mxc/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
>  obj-$(CONFIG_MXC_USE_EPIT) += epit.o
>  obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
>  obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
> +obj-$(CONFIG_CPU_IDLE) += cpuidle.o
>  ifdef CONFIG_SND_IMX_SOC
>  obj-y += ssi-fiq.o
>  obj-y += ssi-fiq-ksym.o
> diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
> new file mode 100644
> index 0000000..b7a5e1c
> --- /dev/null
> +++ b/arch/arm/plat-mxc/cpuidle.c
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/cpuidle.h>
> +#include <linux/hrtimer.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +
> +static struct cpuidle_device __percpu * imx_cpuidle_devices;
> +
> +void imx_cpuidle_devices_uninit(void)
> +{
> +	int cpu_id;
> +	struct cpuidle_device *dev;
> +
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	free_percpu(imx_cpuidle_devices);
> +}

Does this function need to be exported?  I haven't seen it being
used anywhere outside this file.  Also, can it be __init?

> +
> +int __init imx_cpuidle_init(struct cpuidle_driver *drv)
> +{
> +	struct cpuidle_device *dev;
> +	int cpu_id, ret;
> +
> +	if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
> +		pr_err("%s: Invalid Input\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	ret = cpuidle_register_driver(drv);
> +	if (ret) {
> +		pr_err("%s: Failed to register cpuidle driver with error: %d\n",
> +			 __func__, ret);
> +		return ret;
> +	}
> +
> +	imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (imx_cpuidle_devices == NULL) {
> +		ret = -ENOMEM;
> +		goto unregister_drv;
> +	}
> +
> +	/* initialize state data for each cpuidle_device */
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
> +		dev->cpu = cpu_id;
> +		dev->state_count = drv->state_count;
> +
> +		ret = cpuidle_register_device(dev);
> +		if (ret) {
> +			pr_err("%s: Failed to register cpu %u\n",
> +				__func__, cpu_id);

Nit: print ret (error code) too?

> +			goto uninit;
> +		}
> +	}
> +
> +	return 0;
> +
> +uninit:
> +	imx_cpuidle_devices_uninit();
> +
> +unregister_drv:
> +	cpuidle_unregister_driver(drv);
> +	return ret;
> +}
> diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
> new file mode 100644
> index 0000000..8612510
> --- /dev/null
> +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/cpuidle.h>
> +
> +#ifdef CONFIG_CPU_IDLE
> +extern void imx_cpuidle_devices_uninit(void);
> +extern int imx_cpuidle_init(struct cpuidle_driver *drv);
> +#else
> +static inline void imx_cpuidle_devices_uninit(void) {}
> +static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
> +{ return -ENODEV; }

Nit: if it can not be in the same line with function name, we usually
have it be:

{
	return -ENODEV;
}

> +#endif
> -- 
> 1.7.10
> 

-- 
Regards,
Shawn

[PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Shawn Guo]

On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
> Add common cpuidle init functionality that can be used by various
> imx platforms.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/plat-mxc/Makefile               |    1 +
>  arch/arm/plat-mxc/cpuidle.c              |   80 ++++++++++++++++++++++++++++++
>  arch/arm/plat-mxc/include/mach/cpuidle.h |   22 ++++++++
>  3 files changed, 103 insertions(+)
>  create mode 100644 arch/arm/plat-mxc/cpuidle.c
>  create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
> 
> diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
> index e81290c..63b064b 100644
> --- a/arch/arm/plat-mxc/Makefile
> +++ b/arch/arm/plat-mxc/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
>  obj-$(CONFIG_MXC_USE_EPIT) += epit.o
>  obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
>  obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
> +obj-$(CONFIG_CPU_IDLE) += cpuidle.o
>  ifdef CONFIG_SND_IMX_SOC
>  obj-y += ssi-fiq.o
>  obj-y += ssi-fiq-ksym.o
> diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
> new file mode 100644
> index 0000000..b7a5e1c
> --- /dev/null
> +++ b/arch/arm/plat-mxc/cpuidle.c
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/cpuidle.h>
> +#include <linux/hrtimer.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +
> +static struct cpuidle_device __percpu * imx_cpuidle_devices;
> +
> +void imx_cpuidle_devices_uninit(void)
> +{
> +	int cpu_id;
> +	struct cpuidle_device *dev;
> +
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	free_percpu(imx_cpuidle_devices);
> +}

Does this function need to be exported?  I haven't seen it being
used anywhere outside this file.  Also, can it be __init?

> +
> +int __init imx_cpuidle_init(struct cpuidle_driver *drv)
> +{
> +	struct cpuidle_device *dev;
> +	int cpu_id, ret;
> +
> +	if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
> +		pr_err("%s: Invalid Input\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	ret = cpuidle_register_driver(drv);
> +	if (ret) {
> +		pr_err("%s: Failed to register cpuidle driver with error: %d\n",
> +			 __func__, ret);
> +		return ret;
> +	}
> +
> +	imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (imx_cpuidle_devices == NULL) {
> +		ret = -ENOMEM;
> +		goto unregister_drv;
> +	}
> +
> +	/* initialize state data for each cpuidle_device */
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
> +		dev->cpu = cpu_id;
> +		dev->state_count = drv->state_count;
> +
> +		ret = cpuidle_register_device(dev);
> +		if (ret) {
> +			pr_err("%s: Failed to register cpu %u\n",
> +				__func__, cpu_id);

Nit: print ret (error code) too?

> +			goto uninit;
> +		}
> +	}
> +
> +	return 0;
> +
> +uninit:
> +	imx_cpuidle_devices_uninit();
> +
> +unregister_drv:
> +	cpuidle_unregister_driver(drv);
> +	return ret;
> +}
> diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
> new file mode 100644
> index 0000000..8612510
> --- /dev/null
> +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/cpuidle.h>
> +
> +#ifdef CONFIG_CPU_IDLE
> +extern void imx_cpuidle_devices_uninit(void);
> +extern int imx_cpuidle_init(struct cpuidle_driver *drv);
> +#else
> +static inline void imx_cpuidle_devices_uninit(void) {}
> +static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
> +{ return -ENODEV; }

Nit: if it can not be in the same line with function name, we usually
have it be:

{
	return -ENODEV;
}

> +#endif
> -- 
> 1.7.10
> 

-- 
Regards,
Shawn

Re: [PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver

[Shawn Guo]

On Tue, May 01, 2012 at 09:12:40PM -0500, Robert Lee wrote:
> Add basic imx6q cpuidle driver.  For now, only basic WFI state is
> supported.  Deeper idle states will be added in the future.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/mach-imx/cpuidle-imx6q.c |   33 +++++++++++++++++++++++++++++++++

So, this file is not needed any more, I guess.

>  arch/arm/mach-imx/mach-imx6q.c    |   18 ++++++++++++++++++
>  2 files changed, 51 insertions(+)
>  create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
> 
> diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
> new file mode 100644
> index 0000000..b74557f
> --- /dev/null
> +++ b/arch/arm/mach-imx/cpuidle-imx6q.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/cpuidle.h>
> +#include <linux/export.h>
> +#include <asm/cpuidle.h>
> +#include <mach/cpuidle.h>
> +
> +static struct cpuidle_driver imx6q_cpuidle_driver = {
> +	.name			= "imx6q_cpuidle",
> +	.owner			= THIS_MODULE,
> +	.en_core_tk_irqen	= 1,
> +	.states[0]		= ARM_CPUIDLE_WFI_STATE,
> +	.state_count		= 1,
> +};
> +
> +int __init imx6q_cpuidle_init(void)
> +{
> +	imx_cpuidle_set_driver(&imx6q_cpuidle_driver);
> +
> +	return 0;
> +}
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index da6c1d9..21e2051 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -21,6 +21,9 @@
>  #include <linux/of_platform.h>
>  #include <linux/phy.h>
>  #include <linux/micrel_phy.h>
> +#include <linux/export.h>
> +#include <linux/cpuidle.h>
> +#include <asm/cpuidle.h>
>  #include <asm/smp_twd.h>
>  #include <asm/hardware/cache-l2x0.h>
>  #include <asm/hardware/gic.h>
> @@ -29,6 +32,7 @@
>  #include <asm/system_misc.h>
>  #include <mach/common.h>
>  #include <mach/hardware.h>
> +#include <mach/cpuidle.h>

The headers here are mostly sorted in names, so please ...

Regards,
Shawn

>  
>  void imx6q_restart(char mode, const char *cmd)
>  {
> @@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void)
>  	imx6q_pm_init();
>  }
>  
> +static struct cpuidle_driver imx6q_cpuidle_driver = {
> +	.name			= "imx6q_cpuidle",
> +	.owner			= THIS_MODULE,
> +	.en_core_tk_irqen	= 1,
> +	.states[0]		= ARM_CPUIDLE_WFI_STATE,
> +	.state_count		= 1,
> +};
> +
> +static void __init imx6q_init_late(void)
> +{
> +	imx_cpuidle_init(&imx6q_cpuidle_driver);
> +}
> +
>  static void __init imx6q_map_io(void)
>  {
>  	imx_lluart_map_io();
> @@ -142,6 +159,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)")
>  	.handle_irq	= imx6q_handle_irq,
>  	.timer		= &imx6q_timer,
>  	.init_machine	= imx6q_init_machine,
> +	.init_late      = imx6q_init_late,
>  	.dt_compat	= imx6q_dt_compat,
>  	.restart	= imx6q_restart,
>  MACHINE_END
> -- 
> 1.7.10
> 

[PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver

[Shawn Guo]

On Tue, May 01, 2012 at 09:12:40PM -0500, Robert Lee wrote:
> Add basic imx6q cpuidle driver.  For now, only basic WFI state is
> supported.  Deeper idle states will be added in the future.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/mach-imx/cpuidle-imx6q.c |   33 +++++++++++++++++++++++++++++++++

So, this file is not needed any more, I guess.

>  arch/arm/mach-imx/mach-imx6q.c    |   18 ++++++++++++++++++
>  2 files changed, 51 insertions(+)
>  create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
> 
> diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
> new file mode 100644
> index 0000000..b74557f
> --- /dev/null
> +++ b/arch/arm/mach-imx/cpuidle-imx6q.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/cpuidle.h>
> +#include <linux/export.h>
> +#include <asm/cpuidle.h>
> +#include <mach/cpuidle.h>
> +
> +static struct cpuidle_driver imx6q_cpuidle_driver = {
> +	.name			= "imx6q_cpuidle",
> +	.owner			= THIS_MODULE,
> +	.en_core_tk_irqen	= 1,
> +	.states[0]		= ARM_CPUIDLE_WFI_STATE,
> +	.state_count		= 1,
> +};
> +
> +int __init imx6q_cpuidle_init(void)
> +{
> +	imx_cpuidle_set_driver(&imx6q_cpuidle_driver);
> +
> +	return 0;
> +}
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index da6c1d9..21e2051 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -21,6 +21,9 @@
>  #include <linux/of_platform.h>
>  #include <linux/phy.h>
>  #include <linux/micrel_phy.h>
> +#include <linux/export.h>
> +#include <linux/cpuidle.h>
> +#include <asm/cpuidle.h>
>  #include <asm/smp_twd.h>
>  #include <asm/hardware/cache-l2x0.h>
>  #include <asm/hardware/gic.h>
> @@ -29,6 +32,7 @@
>  #include <asm/system_misc.h>
>  #include <mach/common.h>
>  #include <mach/hardware.h>
> +#include <mach/cpuidle.h>

The headers here are mostly sorted in names, so please ...

Regards,
Shawn

>  
>  void imx6q_restart(char mode, const char *cmd)
>  {
> @@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void)
>  	imx6q_pm_init();
>  }
>  
> +static struct cpuidle_driver imx6q_cpuidle_driver = {
> +	.name			= "imx6q_cpuidle",
> +	.owner			= THIS_MODULE,
> +	.en_core_tk_irqen	= 1,
> +	.states[0]		= ARM_CPUIDLE_WFI_STATE,
> +	.state_count		= 1,
> +};
> +
> +static void __init imx6q_init_late(void)
> +{
> +	imx_cpuidle_init(&imx6q_cpuidle_driver);
> +}
> +
>  static void __init imx6q_map_io(void)
>  {
>  	imx_lluart_map_io();
> @@ -142,6 +159,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)")
>  	.handle_irq	= imx6q_handle_irq,
>  	.timer		= &imx6q_timer,
>  	.init_machine	= imx6q_init_machine,
> +	.init_late      = imx6q_init_late,
>  	.dt_compat	= imx6q_dt_compat,
>  	.restart	= imx6q_restart,
>  MACHINE_END
> -- 
> 1.7.10
> 

Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Sascha Hauer]

On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
> Add common cpuidle init functionality that can be used by various
> imx platforms.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
> +
> +int __init imx_cpuidle_init(struct cpuidle_driver *drv)
> +{
> +	struct cpuidle_device *dev;
> +	int cpu_id, ret;
> +
> +	if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {

Please don't check for !drv here. When someone calls this function with
a NULL pointer he should get a nive stack trace allowing him to figure
out what went wrong.

> +		pr_err("%s: Invalid Input\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	ret = cpuidle_register_driver(drv);
> +	if (ret) {
> +		pr_err("%s: Failed to register cpuidle driver with error: %d\n",
> +			 __func__, ret);
> +		return ret;
> +	}
> +
> +	imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (imx_cpuidle_devices == NULL) {
> +		ret = -ENOMEM;
> +		goto unregister_drv;
> +	}
> +
> +	/* initialize state data for each cpuidle_device */
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
> +		dev->cpu = cpu_id;
> +		dev->state_count = drv->state_count;
> +
> +		ret = cpuidle_register_device(dev);
> +		if (ret) {
> +			pr_err("%s: Failed to register cpu %u\n",
> +				__func__, cpu_id);
> +			goto uninit;

You should only unregister the cpuidle devices you successfully
registered. Unregistering not yet registered cpuidle devices probably
has unwanted side effects.

Sascha

> +		}
> +	}
> +
> +	return 0;
> +
> +uninit:
> +	imx_cpuidle_devices_uninit();
> +
> +unregister_drv:
> +	cpuidle_unregister_driver(drv);
> +	return ret;
> +}
> diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
> new file mode 100644
> index 0000000..8612510
> --- /dev/null
> +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/cpuidle.h>
> +
> +#ifdef CONFIG_CPU_IDLE
> +extern void imx_cpuidle_devices_uninit(void);
> +extern int imx_cpuidle_init(struct cpuidle_driver *drv);
> +#else
> +static inline void imx_cpuidle_devices_uninit(void) {}
> +static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
> +{ return -ENODEV; }
> +#endif
> -- 
> 1.7.10
> 
> 

-- 
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/3] ARM: imx: Add common imx cpuidle init functionality.

[Sascha Hauer]

On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
> Add common cpuidle init functionality that can be used by various
> imx platforms.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
> +
> +int __init imx_cpuidle_init(struct cpuidle_driver *drv)
> +{
> +	struct cpuidle_device *dev;
> +	int cpu_id, ret;
> +
> +	if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {

Please don't check for !drv here. When someone calls this function with
a NULL pointer he should get a nive stack trace allowing him to figure
out what went wrong.

> +		pr_err("%s: Invalid Input\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	ret = cpuidle_register_driver(drv);
> +	if (ret) {
> +		pr_err("%s: Failed to register cpuidle driver with error: %d\n",
> +			 __func__, ret);
> +		return ret;
> +	}
> +
> +	imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (imx_cpuidle_devices == NULL) {
> +		ret = -ENOMEM;
> +		goto unregister_drv;
> +	}
> +
> +	/* initialize state data for each cpuidle_device */
> +	for_each_possible_cpu(cpu_id) {
> +		dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
> +		dev->cpu = cpu_id;
> +		dev->state_count = drv->state_count;
> +
> +		ret = cpuidle_register_device(dev);
> +		if (ret) {
> +			pr_err("%s: Failed to register cpu %u\n",
> +				__func__, cpu_id);
> +			goto uninit;

You should only unregister the cpuidle devices you successfully
registered. Unregistering not yet registered cpuidle devices probably
has unwanted side effects.

Sascha

> +		}
> +	}
> +
> +	return 0;
> +
> +uninit:
> +	imx_cpuidle_devices_uninit();
> +
> +unregister_drv:
> +	cpuidle_unregister_driver(drv);
> +	return ret;
> +}
> diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
> new file mode 100644
> index 0000000..8612510
> --- /dev/null
> +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/cpuidle.h>
> +
> +#ifdef CONFIG_CPU_IDLE
> +extern void imx_cpuidle_devices_uninit(void);
> +extern int imx_cpuidle_init(struct cpuidle_driver *drv);
> +#else
> +static inline void imx_cpuidle_devices_uninit(void) {}
> +static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
> +{ return -ENODEV; }
> +#endif
> -- 
> 1.7.10
> 
> 

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

Re: [PATCH v2 2/3] ARM: imx: Add imx5 cpuidle driver

[Sascha Hauer]

On Tue, May 01, 2012 at 09:12:39PM -0500, Robert Lee wrote:
> Add imx5 cpuidle driver.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/mach-imx/mm-imx5.c |   42 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
> index d6b7e9f..cbd9bad 100644
> --- a/arch/arm/mach-imx/mm-imx5.c
> +++ b/arch/arm/mach-imx/mm-imx5.c
> @@ -22,24 +22,59 @@
>  #include <mach/common.h>
>  #include <mach/devices-common.h>
>  #include <mach/iomux-v3.h>
> +#include <mach/cpuidle.h>
>  
>  static struct clk *gpc_dvfs_clk;
>  
> -static void imx5_idle(void)
> +static int imx5_idle(void)
>  {
> +	int ret = 0;
> +
>  	/* gpc clock is needed for SRPG */
>  	if (gpc_dvfs_clk == NULL) {
>  		gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs");
>  		if (IS_ERR(gpc_dvfs_clk))
> -			return;
> +			return -ENODEV;
>  	}
>  	clk_enable(gpc_dvfs_clk);
>  	mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
>  	if (!tzic_enable_wake())
>  		cpu_do_idle();
> +	else
> +		ret = -EBUSY;
>  	clk_disable(gpc_dvfs_clk);
> +
> +	return ret;
> +}
> +
> +static int imx5_cpuidle_enter(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv, int idx)
> +{
> +	int ret;
> +
> +	ret = imx5_idle();
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return idx;
>  }
>  
> +static struct cpuidle_driver imx5_cpuidle_driver = {
> +	.name			= "imx5_cpuidle",
> +	.owner			= THIS_MODULE,
> +	.en_core_tk_irqen	= 1,
> +	.states[0]	= {
> +		.enter			= imx5_cpuidle_enter,
> +		.exit_latency		= 20, /* max latency at 160MHz */
> +		.target_residency	= 1,
> +		.flags			= CPUIDLE_FLAG_TIME_VALID,
> +		.name			= "IMX5 SRPG",
> +		.desc			= "CPU state retained,powered off",
> +	},
> +	.state_count		= 1,
> +};
> +
>  /*
>   * Define the MX50 memory map.
>   */
> @@ -103,7 +138,7 @@ void __init imx51_init_early(void)
>  	mxc_set_cpu_type(MXC_CPU_MX51);
>  	mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
>  	mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
> -	arm_pm_idle = imx5_idle;
> +	arm_pm_idle = (void *)imx5_idle;

I don't like this. It will cover all warnings when the prototype of
arm_pm_idle changes in future. Better add a static void imx5_idle
which calls a static int imx5_do_idle, then you have an idle function
which returns an int.

>  }
>  
>  void __init imx53_init_early(void)
> @@ -238,4 +273,5 @@ void __init imx53_soc_init(void)
>  void __init imx51_init_late(void)
>  {
>  	mx51_neon_fixup();
> +	imx_cpuidle_init(&imx5_cpuidle_driver);
>  }
> -- 
> 1.7.10
> 
> 

-- 
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 2/3] ARM: imx: Add imx5 cpuidle driver

[Sascha Hauer]

On Tue, May 01, 2012 at 09:12:39PM -0500, Robert Lee wrote:
> Add imx5 cpuidle driver.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/mach-imx/mm-imx5.c |   42 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
> index d6b7e9f..cbd9bad 100644
> --- a/arch/arm/mach-imx/mm-imx5.c
> +++ b/arch/arm/mach-imx/mm-imx5.c
> @@ -22,24 +22,59 @@
>  #include <mach/common.h>
>  #include <mach/devices-common.h>
>  #include <mach/iomux-v3.h>
> +#include <mach/cpuidle.h>
>  
>  static struct clk *gpc_dvfs_clk;
>  
> -static void imx5_idle(void)
> +static int imx5_idle(void)
>  {
> +	int ret = 0;
> +
>  	/* gpc clock is needed for SRPG */
>  	if (gpc_dvfs_clk == NULL) {
>  		gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs");
>  		if (IS_ERR(gpc_dvfs_clk))
> -			return;
> +			return -ENODEV;
>  	}
>  	clk_enable(gpc_dvfs_clk);
>  	mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
>  	if (!tzic_enable_wake())
>  		cpu_do_idle();
> +	else
> +		ret = -EBUSY;
>  	clk_disable(gpc_dvfs_clk);
> +
> +	return ret;
> +}
> +
> +static int imx5_cpuidle_enter(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv, int idx)
> +{
> +	int ret;
> +
> +	ret = imx5_idle();
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return idx;
>  }
>  
> +static struct cpuidle_driver imx5_cpuidle_driver = {
> +	.name			= "imx5_cpuidle",
> +	.owner			= THIS_MODULE,
> +	.en_core_tk_irqen	= 1,
> +	.states[0]	= {
> +		.enter			= imx5_cpuidle_enter,
> +		.exit_latency		= 20, /* max latency at 160MHz */
> +		.target_residency	= 1,
> +		.flags			= CPUIDLE_FLAG_TIME_VALID,
> +		.name			= "IMX5 SRPG",
> +		.desc			= "CPU state retained,powered off",
> +	},
> +	.state_count		= 1,
> +};
> +
>  /*
>   * Define the MX50 memory map.
>   */
> @@ -103,7 +138,7 @@ void __init imx51_init_early(void)
>  	mxc_set_cpu_type(MXC_CPU_MX51);
>  	mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
>  	mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
> -	arm_pm_idle = imx5_idle;
> +	arm_pm_idle = (void *)imx5_idle;

I don't like this. It will cover all warnings when the prototype of
arm_pm_idle changes in future. Better add a static void imx5_idle
which calls a static int imx5_do_idle, then you have an idle function
which returns an int.

>  }
>  
>  void __init imx53_init_early(void)
> @@ -238,4 +273,5 @@ void __init imx53_soc_init(void)
>  void __init imx51_init_late(void)
>  {
>  	mx51_neon_fixup();
> +	imx_cpuidle_init(&imx5_cpuidle_driver);
>  }
> -- 
> 1.7.10
> 
> 

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

Re: [PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver

[Rob Lee]

On Tue, May 1, 2012 at 10:23 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, May 01, 2012 at 09:12:40PM -0500, Robert Lee wrote:
>> Add basic imx6q cpuidle driver.  For now, only basic WFI state is
>> supported.  Deeper idle states will be added in the future.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>> ---
>>  arch/arm/mach-imx/cpuidle-imx6q.c |   33 +++++++++++++++++++++++++++++++++
>
> So, this file is not needed any more, I guess.
>

Yes, I missed that.  It shouldn't have been part of the patch.

>>  arch/arm/mach-imx/mach-imx6q.c    |   18 ++++++++++++++++++
>>  2 files changed, 51 insertions(+)
>>  create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
>>
>> diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
>> new file mode 100644
>> index 0000000..b74557f
>> --- /dev/null
>> +++ b/arch/arm/mach-imx/cpuidle-imx6q.c
>> @@ -0,0 +1,33 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + * Copyright 2012 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/cpuidle.h>
>> +#include <linux/export.h>
>> +#include <asm/cpuidle.h>
>> +#include <mach/cpuidle.h>
>> +
>> +static struct cpuidle_driver imx6q_cpuidle_driver = {
>> +     .name                   = "imx6q_cpuidle",
>> +     .owner                  = THIS_MODULE,
>> +     .en_core_tk_irqen       = 1,
>> +     .states[0]              = ARM_CPUIDLE_WFI_STATE,
>> +     .state_count            = 1,
>> +};
>> +
>> +int __init imx6q_cpuidle_init(void)
>> +{
>> +     imx_cpuidle_set_driver(&imx6q_cpuidle_driver);
>> +
>> +     return 0;
>> +}
>> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
>> index da6c1d9..21e2051 100644
>> --- a/arch/arm/mach-imx/mach-imx6q.c
>> +++ b/arch/arm/mach-imx/mach-imx6q.c
>> @@ -21,6 +21,9 @@
>>  #include <linux/of_platform.h>
>>  #include <linux/phy.h>
>>  #include <linux/micrel_phy.h>
>> +#include <linux/export.h>
>> +#include <linux/cpuidle.h>
>> +#include <asm/cpuidle.h>
>>  #include <asm/smp_twd.h>
>>  #include <asm/hardware/cache-l2x0.h>
>>  #include <asm/hardware/gic.h>
>> @@ -29,6 +32,7 @@
>>  #include <asm/system_misc.h>
>>  #include <mach/common.h>
>>  #include <mach/hardware.h>
>> +#include <mach/cpuidle.h>
>
> The headers here are mostly sorted in names, so please ...
>

Do you mean sorted alphabetically per group?  So the last three
includes should look like this instead?
...
#include <mach/common.h>
#include <mach/cpuidle.h>
#include <mach/hardware.h>
...
Thanks,
Rob

> Regards,
> Shawn
>
>>
>>  void imx6q_restart(char mode, const char *cmd)
>>  {
>> @@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void)
>>       imx6q_pm_init();
>>  }
>>
>> +static struct cpuidle_driver imx6q_cpuidle_driver = {
>> +     .name                   = "imx6q_cpuidle",
>> +     .owner                  = THIS_MODULE,
>> +     .en_core_tk_irqen       = 1,
>> +     .states[0]              = ARM_CPUIDLE_WFI_STATE,
>> +     .state_count            = 1,
>> +};
>> +
>> +static void __init imx6q_init_late(void)
>> +{
>> +     imx_cpuidle_init(&imx6q_cpuidle_driver);
>> +}
>> +
>>  static void __init imx6q_map_io(void)
>>  {
>>       imx_lluart_map_io();
>> @@ -142,6 +159,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)")
>>       .handle_irq     = imx6q_handle_irq,
>>       .timer          = &imx6q_timer,
>>       .init_machine   = imx6q_init_machine,
>> +     .init_late      = imx6q_init_late,
>>       .dt_compat      = imx6q_dt_compat,
>>       .restart        = imx6q_restart,
>>  MACHINE_END
>> --
>> 1.7.10
>>

[PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver

[Rob Lee]

On Tue, May 1, 2012 at 10:23 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, May 01, 2012 at 09:12:40PM -0500, Robert Lee wrote:
>> Add basic imx6q cpuidle driver. ?For now, only basic WFI state is
>> supported. ?Deeper idle states will be added in the future.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>> ---
>> ?arch/arm/mach-imx/cpuidle-imx6q.c | ? 33 +++++++++++++++++++++++++++++++++
>
> So, this file is not needed any more, I guess.
>

Yes, I missed that.  It shouldn't have been part of the patch.

>> ?arch/arm/mach-imx/mach-imx6q.c ? ?| ? 18 ++++++++++++++++++
>> ?2 files changed, 51 insertions(+)
>> ?create mode 100644 arch/arm/mach-imx/cpuidle-imx6q.c
>>
>> diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
>> new file mode 100644
>> index 0000000..b74557f
>> --- /dev/null
>> +++ b/arch/arm/mach-imx/cpuidle-imx6q.c
>> @@ -0,0 +1,33 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + * Copyright 2012 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/cpuidle.h>
>> +#include <linux/export.h>
>> +#include <asm/cpuidle.h>
>> +#include <mach/cpuidle.h>
>> +
>> +static struct cpuidle_driver imx6q_cpuidle_driver = {
>> + ? ? .name ? ? ? ? ? ? ? ? ? = "imx6q_cpuidle",
>> + ? ? .owner ? ? ? ? ? ? ? ? ?= THIS_MODULE,
>> + ? ? .en_core_tk_irqen ? ? ? = 1,
>> + ? ? .states[0] ? ? ? ? ? ? ?= ARM_CPUIDLE_WFI_STATE,
>> + ? ? .state_count ? ? ? ? ? ?= 1,
>> +};
>> +
>> +int __init imx6q_cpuidle_init(void)
>> +{
>> + ? ? imx_cpuidle_set_driver(&imx6q_cpuidle_driver);
>> +
>> + ? ? return 0;
>> +}
>> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
>> index da6c1d9..21e2051 100644
>> --- a/arch/arm/mach-imx/mach-imx6q.c
>> +++ b/arch/arm/mach-imx/mach-imx6q.c
>> @@ -21,6 +21,9 @@
>> ?#include <linux/of_platform.h>
>> ?#include <linux/phy.h>
>> ?#include <linux/micrel_phy.h>
>> +#include <linux/export.h>
>> +#include <linux/cpuidle.h>
>> +#include <asm/cpuidle.h>
>> ?#include <asm/smp_twd.h>
>> ?#include <asm/hardware/cache-l2x0.h>
>> ?#include <asm/hardware/gic.h>
>> @@ -29,6 +32,7 @@
>> ?#include <asm/system_misc.h>
>> ?#include <mach/common.h>
>> ?#include <mach/hardware.h>
>> +#include <mach/cpuidle.h>
>
> The headers here are mostly sorted in names, so please ...
>

Do you mean sorted alphabetically per group?  So the last three
includes should look like this instead?
...
#include <mach/common.h>
#include <mach/cpuidle.h>
#include <mach/hardware.h>
...
Thanks,
Rob

> Regards,
> Shawn
>
>>
>> ?void imx6q_restart(char mode, const char *cmd)
>> ?{
>> @@ -86,6 +90,19 @@ static void __init imx6q_init_machine(void)
>> ? ? ? imx6q_pm_init();
>> ?}
>>
>> +static struct cpuidle_driver imx6q_cpuidle_driver = {
>> + ? ? .name ? ? ? ? ? ? ? ? ? = "imx6q_cpuidle",
>> + ? ? .owner ? ? ? ? ? ? ? ? ?= THIS_MODULE,
>> + ? ? .en_core_tk_irqen ? ? ? = 1,
>> + ? ? .states[0] ? ? ? ? ? ? ?= ARM_CPUIDLE_WFI_STATE,
>> + ? ? .state_count ? ? ? ? ? ?= 1,
>> +};
>> +
>> +static void __init imx6q_init_late(void)
>> +{
>> + ? ? imx_cpuidle_init(&imx6q_cpuidle_driver);
>> +}
>> +
>> ?static void __init imx6q_map_io(void)
>> ?{
>> ? ? ? imx_lluart_map_io();
>> @@ -142,6 +159,7 @@ DT_MACHINE_START(IMX6Q, "Freescale i.MX6 Quad (Device Tree)")
>> ? ? ? .handle_irq ? ? = imx6q_handle_irq,
>> ? ? ? .timer ? ? ? ? ?= &imx6q_timer,
>> ? ? ? .init_machine ? = imx6q_init_machine,
>> + ? ? .init_late ? ? ?= imx6q_init_late,
>> ? ? ? .dt_compat ? ? ?= imx6q_dt_compat,
>> ? ? ? .restart ? ? ? ?= imx6q_restart,
>> ?MACHINE_END
>> --
>> 1.7.10
>>

Re: [PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver

[Shawn Guo]

On Wed, May 02, 2012 at 08:50:20AM -0500, Rob Lee wrote:
> >> --- a/arch/arm/mach-imx/mach-imx6q.c
> >> +++ b/arch/arm/mach-imx/mach-imx6q.c
> >> @@ -21,6 +21,9 @@
> >>  #include <linux/of_platform.h>
> >>  #include <linux/phy.h>
> >>  #include <linux/micrel_phy.h>
> >> +#include <linux/export.h>
> >> +#include <linux/cpuidle.h>
> >> +#include <asm/cpuidle.h>
> >>  #include <asm/smp_twd.h>
> >>  #include <asm/hardware/cache-l2x0.h>
> >>  #include <asm/hardware/gic.h>
> >> @@ -29,6 +32,7 @@
> >>  #include <asm/system_misc.h>
> >>  #include <mach/common.h>
> >>  #include <mach/hardware.h>
> >> +#include <mach/cpuidle.h>
> >
> > The headers here are mostly sorted in names, so please ...
> >
> 
> Do you mean sorted alphabetically per group?  So the last three
> includes should look like this instead?
> ...
> #include <mach/common.h>
> #include <mach/cpuidle.h>
> #include <mach/hardware.h>
> ...

Yes.  And do not forget <linux/export.h> and <linux/cpuidle.h>.

-- 
Regards,
Shawn

[PATCH v2 3/3] ARM: imx: Add imx6q cpuidle driver

[Shawn Guo]

On Wed, May 02, 2012 at 08:50:20AM -0500, Rob Lee wrote:
> >> --- a/arch/arm/mach-imx/mach-imx6q.c
> >> +++ b/arch/arm/mach-imx/mach-imx6q.c
> >> @@ -21,6 +21,9 @@
> >> ?#include <linux/of_platform.h>
> >> ?#include <linux/phy.h>
> >> ?#include <linux/micrel_phy.h>
> >> +#include <linux/export.h>
> >> +#include <linux/cpuidle.h>
> >> +#include <asm/cpuidle.h>
> >> ?#include <asm/smp_twd.h>
> >> ?#include <asm/hardware/cache-l2x0.h>
> >> ?#include <asm/hardware/gic.h>
> >> @@ -29,6 +32,7 @@
> >> ?#include <asm/system_misc.h>
> >> ?#include <mach/common.h>
> >> ?#include <mach/hardware.h>
> >> +#include <mach/cpuidle.h>
> >
> > The headers here are mostly sorted in names, so please ...
> >
> 
> Do you mean sorted alphabetically per group?  So the last three
> includes should look like this instead?
> ...
> #include <mach/common.h>
> #include <mach/cpuidle.h>
> #include <mach/hardware.h>
> ...

Yes.  And do not forget <linux/export.h> and <linux/cpuidle.h>.

-- 
Regards,
Shawn

Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Rob Lee]

Shawn,

On Tue, May 1, 2012 at 10:13 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
>> Add common cpuidle init functionality that can be used by various
>> imx platforms.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>> ---
>>  arch/arm/plat-mxc/Makefile               |    1 +
>>  arch/arm/plat-mxc/cpuidle.c              |   80 ++++++++++++++++++++++++++++++
>>  arch/arm/plat-mxc/include/mach/cpuidle.h |   22 ++++++++
>>  3 files changed, 103 insertions(+)
>>  create mode 100644 arch/arm/plat-mxc/cpuidle.c
>>  create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
>>
>> diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
>> index e81290c..63b064b 100644
>> --- a/arch/arm/plat-mxc/Makefile
>> +++ b/arch/arm/plat-mxc/Makefile
>> @@ -16,6 +16,7 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
>>  obj-$(CONFIG_MXC_USE_EPIT) += epit.o
>>  obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
>>  obj-$(CONFIG_CPU_FREQ_IMX)    += cpufreq.o
>> +obj-$(CONFIG_CPU_IDLE) += cpuidle.o
>>  ifdef CONFIG_SND_IMX_SOC
>>  obj-y += ssi-fiq.o
>>  obj-y += ssi-fiq-ksym.o
>> diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
>> new file mode 100644
>> index 0000000..b7a5e1c
>> --- /dev/null
>> +++ b/arch/arm/plat-mxc/cpuidle.c
>> @@ -0,0 +1,80 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + * Copyright 2012 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/io.h>
>> +#include <linux/cpuidle.h>
>> +#include <linux/hrtimer.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +
>> +static struct cpuidle_device __percpu * imx_cpuidle_devices;
>> +
>> +void imx_cpuidle_devices_uninit(void)
>> +{
>> +     int cpu_id;
>> +     struct cpuidle_device *dev;
>> +
>> +     for_each_possible_cpu(cpu_id) {
>> +             dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
>> +             cpuidle_unregister_device(dev);
>> +     }
>> +
>> +     free_percpu(imx_cpuidle_devices);
>> +}
>
> Does this function need to be exported?  I haven't seen it being
> used anywhere outside this file.  Also, can it be __init?
>

Yes to both for now and can be changed back if necessary in the future.

>> +
>> +int __init imx_cpuidle_init(struct cpuidle_driver *drv)
>> +{
>> +     struct cpuidle_device *dev;
>> +     int cpu_id, ret;
>> +
>> +     if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
>> +             pr_err("%s: Invalid Input\n", __func__);
>> +             return -EINVAL;
>> +     }
>> +
>> +     ret = cpuidle_register_driver(drv);
>> +     if (ret) {
>> +             pr_err("%s: Failed to register cpuidle driver with error: %d\n",
>> +                      __func__, ret);
>> +             return ret;
>> +     }
>> +
>> +     imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>> +     if (imx_cpuidle_devices == NULL) {
>> +             ret = -ENOMEM;
>> +             goto unregister_drv;
>> +     }
>> +
>> +     /* initialize state data for each cpuidle_device */
>> +     for_each_possible_cpu(cpu_id) {
>> +             dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
>> +             dev->cpu = cpu_id;
>> +             dev->state_count = drv->state_count;
>> +
>> +             ret = cpuidle_register_device(dev);
>> +             if (ret) {
>> +                     pr_err("%s: Failed to register cpu %u\n",
>> +                             __func__, cpu_id);
>
> Nit: print ret (error code) too?
>

I added the printing of the error code based on Sascha's suggestion in
v1 of this submission.

>> +                     goto uninit;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +
>> +uninit:
>> +     imx_cpuidle_devices_uninit();
>> +
>> +unregister_drv:
>> +     cpuidle_unregister_driver(drv);
>> +     return ret;
>> +}
>> diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
>> new file mode 100644
>> index 0000000..8612510
>> --- /dev/null
>> +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
>> @@ -0,0 +1,22 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + * Copyright 2012 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +#include <linux/cpuidle.h>
>> +
>> +#ifdef CONFIG_CPU_IDLE
>> +extern void imx_cpuidle_devices_uninit(void);
>> +extern int imx_cpuidle_init(struct cpuidle_driver *drv);
>> +#else
>> +static inline void imx_cpuidle_devices_uninit(void) {}
>> +static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
>> +{ return -ENODEV; }
>
> Nit: if it can not be in the same line with function name, we usually
> have it be:
>
> {
>        return -ENODEV;
> }

Understood.  I was just going by what I have seen used in other places
(like include/linux/cpuidle.h).

Thanks,
Rob

>
>> +#endif
>> --
>> 1.7.10
>>
>
> --
> Regards,
> Shawn

[PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Rob Lee]

Shawn,

On Tue, May 1, 2012 at 10:13 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
>> Add common cpuidle init functionality that can be used by various
>> imx platforms.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>> ---
>> ?arch/arm/plat-mxc/Makefile ? ? ? ? ? ? ? | ? ?1 +
>> ?arch/arm/plat-mxc/cpuidle.c ? ? ? ? ? ? ?| ? 80 ++++++++++++++++++++++++++++++
>> ?arch/arm/plat-mxc/include/mach/cpuidle.h | ? 22 ++++++++
>> ?3 files changed, 103 insertions(+)
>> ?create mode 100644 arch/arm/plat-mxc/cpuidle.c
>> ?create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
>>
>> diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
>> index e81290c..63b064b 100644
>> --- a/arch/arm/plat-mxc/Makefile
>> +++ b/arch/arm/plat-mxc/Makefile
>> @@ -16,6 +16,7 @@ obj-$(CONFIG_MXC_ULPI) += ulpi.o
>> ?obj-$(CONFIG_MXC_USE_EPIT) += epit.o
>> ?obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o
>> ?obj-$(CONFIG_CPU_FREQ_IMX) ? ?+= cpufreq.o
>> +obj-$(CONFIG_CPU_IDLE) += cpuidle.o
>> ?ifdef CONFIG_SND_IMX_SOC
>> ?obj-y += ssi-fiq.o
>> ?obj-y += ssi-fiq-ksym.o
>> diff --git a/arch/arm/plat-mxc/cpuidle.c b/arch/arm/plat-mxc/cpuidle.c
>> new file mode 100644
>> index 0000000..b7a5e1c
>> --- /dev/null
>> +++ b/arch/arm/plat-mxc/cpuidle.c
>> @@ -0,0 +1,80 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + * Copyright 2012 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/io.h>
>> +#include <linux/cpuidle.h>
>> +#include <linux/hrtimer.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +
>> +static struct cpuidle_device __percpu * imx_cpuidle_devices;
>> +
>> +void imx_cpuidle_devices_uninit(void)
>> +{
>> + ? ? int cpu_id;
>> + ? ? struct cpuidle_device *dev;
>> +
>> + ? ? for_each_possible_cpu(cpu_id) {
>> + ? ? ? ? ? ? dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
>> + ? ? ? ? ? ? cpuidle_unregister_device(dev);
>> + ? ? }
>> +
>> + ? ? free_percpu(imx_cpuidle_devices);
>> +}
>
> Does this function need to be exported? ?I haven't seen it being
> used anywhere outside this file. ?Also, can it be __init?
>

Yes to both for now and can be changed back if necessary in the future.

>> +
>> +int __init imx_cpuidle_init(struct cpuidle_driver *drv)
>> +{
>> + ? ? struct cpuidle_device *dev;
>> + ? ? int cpu_id, ret;
>> +
>> + ? ? if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
>> + ? ? ? ? ? ? pr_err("%s: Invalid Input\n", __func__);
>> + ? ? ? ? ? ? return -EINVAL;
>> + ? ? }
>> +
>> + ? ? ret = cpuidle_register_driver(drv);
>> + ? ? if (ret) {
>> + ? ? ? ? ? ? pr_err("%s: Failed to register cpuidle driver with error: %d\n",
>> + ? ? ? ? ? ? ? ? ? ? ?__func__, ret);
>> + ? ? ? ? ? ? return ret;
>> + ? ? }
>> +
>> + ? ? imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>> + ? ? if (imx_cpuidle_devices == NULL) {
>> + ? ? ? ? ? ? ret = -ENOMEM;
>> + ? ? ? ? ? ? goto unregister_drv;
>> + ? ? }
>> +
>> + ? ? /* initialize state data for each cpuidle_device */
>> + ? ? for_each_possible_cpu(cpu_id) {
>> + ? ? ? ? ? ? dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
>> + ? ? ? ? ? ? dev->cpu = cpu_id;
>> + ? ? ? ? ? ? dev->state_count = drv->state_count;
>> +
>> + ? ? ? ? ? ? ret = cpuidle_register_device(dev);
>> + ? ? ? ? ? ? if (ret) {
>> + ? ? ? ? ? ? ? ? ? ? pr_err("%s: Failed to register cpu %u\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, cpu_id);
>
> Nit: print ret (error code) too?
>

I added the printing of the error code based on Sascha's suggestion in
v1 of this submission.

>> + ? ? ? ? ? ? ? ? ? ? goto uninit;
>> + ? ? ? ? ? ? }
>> + ? ? }
>> +
>> + ? ? return 0;
>> +
>> +uninit:
>> + ? ? imx_cpuidle_devices_uninit();
>> +
>> +unregister_drv:
>> + ? ? cpuidle_unregister_driver(drv);
>> + ? ? return ret;
>> +}
>> diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
>> new file mode 100644
>> index 0000000..8612510
>> --- /dev/null
>> +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
>> @@ -0,0 +1,22 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + * Copyright 2012 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +#include <linux/cpuidle.h>
>> +
>> +#ifdef CONFIG_CPU_IDLE
>> +extern void imx_cpuidle_devices_uninit(void);
>> +extern int imx_cpuidle_init(struct cpuidle_driver *drv);
>> +#else
>> +static inline void imx_cpuidle_devices_uninit(void) {}
>> +static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
>> +{ return -ENODEV; }
>
> Nit: if it can not be in the same line with function name, we usually
> have it be:
>
> {
> ? ? ? ?return -ENODEV;
> }

Understood.  I was just going by what I have seen used in other places
(like include/linux/cpuidle.h).

Thanks,
Rob

>
>> +#endif
>> --
>> 1.7.10
>>
>
> --
> Regards,
> Shawn

Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Shawn Guo]

On 2 May 2012 21:59, Rob Lee <rob.lee@linaro.org> wrote:
>>> +             ret = cpuidle_register_device(dev);
>>> +             if (ret) {
>>> +                     pr_err("%s: Failed to register cpu %u\n",
>>> +                             __func__, cpu_id);
>>
>> Nit: print ret (error code) too?
>>
>
> I added the printing of the error code based on Sascha's suggestion in
> v1 of this submission.
>
But you did not print "ret" in above pr_err.

Regards,
Shawn

[PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Shawn Guo]

On 2 May 2012 21:59, Rob Lee <rob.lee@linaro.org> wrote:
>>> + ? ? ? ? ? ? ret = cpuidle_register_device(dev);
>>> + ? ? ? ? ? ? if (ret) {
>>> + ? ? ? ? ? ? ? ? ? ? pr_err("%s: Failed to register cpu %u\n",
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, cpu_id);
>>
>> Nit: print ret (error code) too?
>>
>
> I added the printing of the error code based on Sascha's suggestion in
> v1 of this submission.
>
But you did not print "ret" in above pr_err.

Regards,
Shawn

Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Rob Lee]

On Wed, May 2, 2012 at 9:07 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On 2 May 2012 21:59, Rob Lee <rob.lee@linaro.org> wrote:
>>>> +             ret = cpuidle_register_device(dev);
>>>> +             if (ret) {
>>>> +                     pr_err("%s: Failed to register cpu %u\n",
>>>> +                             __func__, cpu_id);
>>>
>>> Nit: print ret (error code) too?
>>>
>>
>> I added the printing of the error code based on Sascha's suggestion in
>> v1 of this submission.
>>
> But you did not print "ret" in above pr_err.
>

Argh, sorry, still trying to wake up I guess.  Thanks.

> Regards,
> Shawn

[PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Rob Lee]

On Wed, May 2, 2012 at 9:07 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On 2 May 2012 21:59, Rob Lee <rob.lee@linaro.org> wrote:
>>>> + ? ? ? ? ? ? ret = cpuidle_register_device(dev);
>>>> + ? ? ? ? ? ? if (ret) {
>>>> + ? ? ? ? ? ? ? ? ? ? pr_err("%s: Failed to register cpu %u\n",
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, cpu_id);
>>>
>>> Nit: print ret (error code) too?
>>>
>>
>> I added the printing of the error code based on Sascha's suggestion in
>> v1 of this submission.
>>
> But you did not print "ret" in above pr_err.
>

Argh, sorry, still trying to wake up I guess.  Thanks.

> Regards,
> Shawn

Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Rob Lee]

Sascha,

On Wed, May 2, 2012 at 2:27 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
>> Add common cpuidle init functionality that can be used by various
>> imx platforms.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>> ---
>> +
>> +int __init imx_cpuidle_init(struct cpuidle_driver *drv)
>> +{
>> +     struct cpuidle_device *dev;
>> +     int cpu_id, ret;
>> +
>> +     if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
>
> Please don't check for !drv here. When someone calls this function with
> a NULL pointer he should get a nive stack trace allowing him to figure
> out what went wrong.
>

Ok, I will change this in v3.  Given your statement, my understanding
is that I should avoid adding checks to make sure a valid driver
object was given as the stack trace information is all the handling
that is needed.  If there is any further logic needed in that rule,
could you elaborate so that I don't make this mistake in the future,
or so that I don't add a check on a driver object in a case that I
should?

>> +             pr_err("%s: Invalid Input\n", __func__);
>> +             return -EINVAL;
>> +     }
>> +
>> +     ret = cpuidle_register_driver(drv);
>> +     if (ret) {
>> +             pr_err("%s: Failed to register cpuidle driver with error: %d\n",
>> +                      __func__, ret);
>> +             return ret;
>> +     }
>> +
>> +     imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>> +     if (imx_cpuidle_devices == NULL) {
>> +             ret = -ENOMEM;
>> +             goto unregister_drv;
>> +     }
>> +
>> +     /* initialize state data for each cpuidle_device */
>> +     for_each_possible_cpu(cpu_id) {
>> +             dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
>> +             dev->cpu = cpu_id;
>> +             dev->state_count = drv->state_count;
>> +
>> +             ret = cpuidle_register_device(dev);
>> +             if (ret) {
>> +                     pr_err("%s: Failed to register cpu %u\n",
>> +                             __func__, cpu_id);
>> +                     goto uninit;
>
> You should only unregister the cpuidle devices you successfully
> registered. Unregistering not yet registered cpuidle devices probably
> has unwanted side effects.
>

I did not add in this handling because the cpuidle_unregister_device()
call already has a "registered" check so extra handling seemed
unnecessary.  But if you still think it is needed just let me know.

cpuidle_unregister_device()
{
...
	if (dev->registered == 0)
		return;
...

Thanks,
Rob

> Sascha
>
>> +             }
>> +     }
>> +
>> +     return 0;
>> +
>> +uninit:
>> +     imx_cpuidle_devices_uninit();
>> +
>> +unregister_drv:
>> +     cpuidle_unregister_driver(drv);
>> +     return ret;
>> +}
>> diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
>> new file mode 100644
>> index 0000000..8612510
>> --- /dev/null
>> +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
>> @@ -0,0 +1,22 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + * Copyright 2012 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +#include <linux/cpuidle.h>
>> +
>> +#ifdef CONFIG_CPU_IDLE
>> +extern void imx_cpuidle_devices_uninit(void);
>> +extern int imx_cpuidle_init(struct cpuidle_driver *drv);
>> +#else
>> +static inline void imx_cpuidle_devices_uninit(void) {}
>> +static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
>> +{ return -ENODEV; }
>> +#endif
>> --
>> 1.7.10
>>
>>
>
> --
> 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/3] ARM: imx: Add common imx cpuidle init functionality.

[Rob Lee]

Sascha,

On Wed, May 2, 2012 at 2:27 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, May 01, 2012 at 09:12:38PM -0500, Robert Lee wrote:
>> Add common cpuidle init functionality that can be used by various
>> imx platforms.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>> ---
>> +
>> +int __init imx_cpuidle_init(struct cpuidle_driver *drv)
>> +{
>> + ? ? struct cpuidle_device *dev;
>> + ? ? int cpu_id, ret;
>> +
>> + ? ? if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
>
> Please don't check for !drv here. When someone calls this function with
> a NULL pointer he should get a nive stack trace allowing him to figure
> out what went wrong.
>

Ok, I will change this in v3.  Given your statement, my understanding
is that I should avoid adding checks to make sure a valid driver
object was given as the stack trace information is all the handling
that is needed.  If there is any further logic needed in that rule,
could you elaborate so that I don't make this mistake in the future,
or so that I don't add a check on a driver object in a case that I
should?

>> + ? ? ? ? ? ? pr_err("%s: Invalid Input\n", __func__);
>> + ? ? ? ? ? ? return -EINVAL;
>> + ? ? }
>> +
>> + ? ? ret = cpuidle_register_driver(drv);
>> + ? ? if (ret) {
>> + ? ? ? ? ? ? pr_err("%s: Failed to register cpuidle driver with error: %d\n",
>> + ? ? ? ? ? ? ? ? ? ? ?__func__, ret);
>> + ? ? ? ? ? ? return ret;
>> + ? ? }
>> +
>> + ? ? imx_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>> + ? ? if (imx_cpuidle_devices == NULL) {
>> + ? ? ? ? ? ? ret = -ENOMEM;
>> + ? ? ? ? ? ? goto unregister_drv;
>> + ? ? }
>> +
>> + ? ? /* initialize state data for each cpuidle_device */
>> + ? ? for_each_possible_cpu(cpu_id) {
>> + ? ? ? ? ? ? dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id);
>> + ? ? ? ? ? ? dev->cpu = cpu_id;
>> + ? ? ? ? ? ? dev->state_count = drv->state_count;
>> +
>> + ? ? ? ? ? ? ret = cpuidle_register_device(dev);
>> + ? ? ? ? ? ? if (ret) {
>> + ? ? ? ? ? ? ? ? ? ? pr_err("%s: Failed to register cpu %u\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, cpu_id);
>> + ? ? ? ? ? ? ? ? ? ? goto uninit;
>
> You should only unregister the cpuidle devices you successfully
> registered. Unregistering not yet registered cpuidle devices probably
> has unwanted side effects.
>

I did not add in this handling because the cpuidle_unregister_device()
call already has a "registered" check so extra handling seemed
unnecessary.  But if you still think it is needed just let me know.

cpuidle_unregister_device()
{
...
	if (dev->registered == 0)
		return;
...

Thanks,
Rob

> Sascha
>
>> + ? ? ? ? ? ? }
>> + ? ? }
>> +
>> + ? ? return 0;
>> +
>> +uninit:
>> + ? ? imx_cpuidle_devices_uninit();
>> +
>> +unregister_drv:
>> + ? ? cpuidle_unregister_driver(drv);
>> + ? ? return ret;
>> +}
>> diff --git a/arch/arm/plat-mxc/include/mach/cpuidle.h b/arch/arm/plat-mxc/include/mach/cpuidle.h
>> new file mode 100644
>> index 0000000..8612510
>> --- /dev/null
>> +++ b/arch/arm/plat-mxc/include/mach/cpuidle.h
>> @@ -0,0 +1,22 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + * Copyright 2012 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +#include <linux/cpuidle.h>
>> +
>> +#ifdef CONFIG_CPU_IDLE
>> +extern void imx_cpuidle_devices_uninit(void);
>> +extern int imx_cpuidle_init(struct cpuidle_driver *drv);
>> +#else
>> +static inline void imx_cpuidle_devices_uninit(void) {}
>> +static inline int imx_cpuidle_init(struct cpuidle_driver *drv)
>> +{ return -ENODEV; }
>> +#endif
>> --
>> 1.7.10
>>
>>
>
> --
> 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 |

Re: [PATCH v2 2/3] ARM: imx: Add imx5 cpuidle driver

[Rob Lee]

Sascha,

On Wed, May 2, 2012 at 2:33 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, May 01, 2012 at 09:12:39PM -0500, Robert Lee wrote:
>> Add imx5 cpuidle driver.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>> ---
>>  arch/arm/mach-imx/mm-imx5.c |   42 +++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
>> index d6b7e9f..cbd9bad 100644
>> --- a/arch/arm/mach-imx/mm-imx5.c
>> +++ b/arch/arm/mach-imx/mm-imx5.c
>> @@ -22,24 +22,59 @@
>>  #include <mach/common.h>
>>  #include <mach/devices-common.h>
>>  #include <mach/iomux-v3.h>
>> +#include <mach/cpuidle.h>
>>
>>  static struct clk *gpc_dvfs_clk;
>>
>> -static void imx5_idle(void)
>> +static int imx5_idle(void)
>>  {
>> +     int ret = 0;
>> +
>>       /* gpc clock is needed for SRPG */
>>       if (gpc_dvfs_clk == NULL) {
>>               gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs");
>>               if (IS_ERR(gpc_dvfs_clk))
>> -                     return;
>> +                     return -ENODEV;
>>       }
>>       clk_enable(gpc_dvfs_clk);
>>       mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
>>       if (!tzic_enable_wake())
>>               cpu_do_idle();
>> +     else
>> +             ret = -EBUSY;
>>       clk_disable(gpc_dvfs_clk);
>> +
>> +     return ret;
>> +}
>> +
>> +static int imx5_cpuidle_enter(struct cpuidle_device *dev,
>> +                             struct cpuidle_driver *drv, int idx)
>> +{
>> +     int ret;
>> +
>> +     ret = imx5_idle();
>> +
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return idx;
>>  }
>>
>> +static struct cpuidle_driver imx5_cpuidle_driver = {
>> +     .name                   = "imx5_cpuidle",
>> +     .owner                  = THIS_MODULE,
>> +     .en_core_tk_irqen       = 1,
>> +     .states[0]      = {
>> +             .enter                  = imx5_cpuidle_enter,
>> +             .exit_latency           = 20, /* max latency at 160MHz */
>> +             .target_residency       = 1,
>> +             .flags                  = CPUIDLE_FLAG_TIME_VALID,
>> +             .name                   = "IMX5 SRPG",
>> +             .desc                   = "CPU state retained,powered off",
>> +     },
>> +     .state_count            = 1,
>> +};
>> +
>>  /*
>>   * Define the MX50 memory map.
>>   */
>> @@ -103,7 +138,7 @@ void __init imx51_init_early(void)
>>       mxc_set_cpu_type(MXC_CPU_MX51);
>>       mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
>>       mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
>> -     arm_pm_idle = imx5_idle;
>> +     arm_pm_idle = (void *)imx5_idle;
>
> I don't like this. It will cover all warnings when the prototype of
> arm_pm_idle changes in future. Better add a static void imx5_idle
> which calls a static int imx5_do_idle, then you have an idle function
> which returns an int.
>

What about using the following:

arm_pm_idle =  (void (*)(void))imx5_idle;

This will give warnings if arm_pm_idle prototype changes.

Thanks,
Rob

>>  }
>>
>>  void __init imx53_init_early(void)
>> @@ -238,4 +273,5 @@ void __init imx53_soc_init(void)
>>  void __init imx51_init_late(void)
>>  {
>>       mx51_neon_fixup();
>> +     imx_cpuidle_init(&imx5_cpuidle_driver);
>>  }
>> --
>> 1.7.10
>>
>>
>
> --
> 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 2/3] ARM: imx: Add imx5 cpuidle driver

[Rob Lee]

Sascha,

On Wed, May 2, 2012 at 2:33 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, May 01, 2012 at 09:12:39PM -0500, Robert Lee wrote:
>> Add imx5 cpuidle driver.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>> ---
>> ?arch/arm/mach-imx/mm-imx5.c | ? 42 +++++++++++++++++++++++++++++++++++++++---
>> ?1 file changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
>> index d6b7e9f..cbd9bad 100644
>> --- a/arch/arm/mach-imx/mm-imx5.c
>> +++ b/arch/arm/mach-imx/mm-imx5.c
>> @@ -22,24 +22,59 @@
>> ?#include <mach/common.h>
>> ?#include <mach/devices-common.h>
>> ?#include <mach/iomux-v3.h>
>> +#include <mach/cpuidle.h>
>>
>> ?static struct clk *gpc_dvfs_clk;
>>
>> -static void imx5_idle(void)
>> +static int imx5_idle(void)
>> ?{
>> + ? ? int ret = 0;
>> +
>> ? ? ? /* gpc clock is needed for SRPG */
>> ? ? ? if (gpc_dvfs_clk == NULL) {
>> ? ? ? ? ? ? ? gpc_dvfs_clk = clk_get(NULL, "gpc_dvfs");
>> ? ? ? ? ? ? ? if (IS_ERR(gpc_dvfs_clk))
>> - ? ? ? ? ? ? ? ? ? ? return;
>> + ? ? ? ? ? ? ? ? ? ? return -ENODEV;
>> ? ? ? }
>> ? ? ? clk_enable(gpc_dvfs_clk);
>> ? ? ? mx5_cpu_lp_set(WAIT_UNCLOCKED_POWER_OFF);
>> ? ? ? if (!tzic_enable_wake())
>> ? ? ? ? ? ? ? cpu_do_idle();
>> + ? ? else
>> + ? ? ? ? ? ? ret = -EBUSY;
>> ? ? ? clk_disable(gpc_dvfs_clk);
>> +
>> + ? ? return ret;
>> +}
>> +
>> +static int imx5_cpuidle_enter(struct cpuidle_device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct cpuidle_driver *drv, int idx)
>> +{
>> + ? ? int ret;
>> +
>> + ? ? ret = imx5_idle();
>> +
>> + ? ? if (ret < 0)
>> + ? ? ? ? ? ? return ret;
>> +
>> + ? ? return idx;
>> ?}
>>
>> +static struct cpuidle_driver imx5_cpuidle_driver = {
>> + ? ? .name ? ? ? ? ? ? ? ? ? = "imx5_cpuidle",
>> + ? ? .owner ? ? ? ? ? ? ? ? ?= THIS_MODULE,
>> + ? ? .en_core_tk_irqen ? ? ? = 1,
>> + ? ? .states[0] ? ? ?= {
>> + ? ? ? ? ? ? .enter ? ? ? ? ? ? ? ? ?= imx5_cpuidle_enter,
>> + ? ? ? ? ? ? .exit_latency ? ? ? ? ? = 20, /* max latency at 160MHz */
>> + ? ? ? ? ? ? .target_residency ? ? ? = 1,
>> + ? ? ? ? ? ? .flags ? ? ? ? ? ? ? ? ?= CPUIDLE_FLAG_TIME_VALID,
>> + ? ? ? ? ? ? .name ? ? ? ? ? ? ? ? ? = "IMX5 SRPG",
>> + ? ? ? ? ? ? .desc ? ? ? ? ? ? ? ? ? = "CPU state retained,powered off",
>> + ? ? },
>> + ? ? .state_count ? ? ? ? ? ?= 1,
>> +};
>> +
>> ?/*
>> ? * Define the MX50 memory map.
>> ? */
>> @@ -103,7 +138,7 @@ void __init imx51_init_early(void)
>> ? ? ? mxc_set_cpu_type(MXC_CPU_MX51);
>> ? ? ? mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
>> ? ? ? mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
>> - ? ? arm_pm_idle = imx5_idle;
>> + ? ? arm_pm_idle = (void *)imx5_idle;
>
> I don't like this. It will cover all warnings when the prototype of
> arm_pm_idle changes in future. Better add a static void imx5_idle
> which calls a static int imx5_do_idle, then you have an idle function
> which returns an int.
>

What about using the following:

arm_pm_idle =  (void (*)(void))imx5_idle;

This will give warnings if arm_pm_idle prototype changes.

Thanks,
Rob

>> ?}
>>
>> ?void __init imx53_init_early(void)
>> @@ -238,4 +273,5 @@ void __init imx53_soc_init(void)
>> ?void __init imx51_init_late(void)
>> ?{
>> ? ? ? mx51_neon_fixup();
>> + ? ? imx_cpuidle_init(&imx5_cpuidle_driver);
>> ?}
>> --
>> 1.7.10
>>
>>
>
> --
> 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 |

Re: [PATCH v2 1/3] ARM: imx: Add common imx cpuidle init functionality.

[Sascha Hauer]

On Wed, May 02, 2012 at 02:16:36PM -0500, Rob Lee wrote:
> Sascha,
> 
> >> +int __init imx_cpuidle_init(struct cpuidle_driver *drv)
> >> +{
> >> +     struct cpuidle_device *dev;
> >> +     int cpu_id, ret;
> >> +
> >> +     if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
> >
> > Please don't check for !drv here. When someone calls this function with
> > a NULL pointer he should get a nive stack trace allowing him to figure
> > out what went wrong.
> >
> 
> Ok, I will change this in v3.  Given your statement, my understanding
> is that I should avoid adding checks to make sure a valid driver
> object was given as the stack trace information is all the handling
> that is needed.  If there is any further logic needed in that rule,
> could you elaborate so that I don't make this mistake in the future,
> or so that I don't add a check on a driver object in a case that I
> should?

Here we have the case that only a Kernel developer will add a call to
this function. For a kernel developer a stack trace is more useful
than a pr_err. Of course this is different when not testing for a NULL
pointer causes subtle bugs in unrelated code.

> >
> > You should only unregister the cpuidle devices you successfully
> > registered. Unregistering not yet registered cpuidle devices probably
> > has unwanted side effects.
> >
> 
> I did not add in this handling because the cpuidle_unregister_device()
> call already has a "registered" check so extra handling seemed
> unnecessary.  But if you still think it is needed just let me know.
> 

It's ok then. I didn't check cpuidle_unregister_device.

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/3] ARM: imx: Add common imx cpuidle init functionality.

[Sascha Hauer]

On Wed, May 02, 2012 at 02:16:36PM -0500, Rob Lee wrote:
> Sascha,
> 
> >> +int __init imx_cpuidle_init(struct cpuidle_driver *drv)
> >> +{
> >> + ? ? struct cpuidle_device *dev;
> >> + ? ? int cpu_id, ret;
> >> +
> >> + ? ? if (!drv || drv->state_count > CPUIDLE_STATE_MAX) {
> >
> > Please don't check for !drv here. When someone calls this function with
> > a NULL pointer he should get a nive stack trace allowing him to figure
> > out what went wrong.
> >
> 
> Ok, I will change this in v3.  Given your statement, my understanding
> is that I should avoid adding checks to make sure a valid driver
> object was given as the stack trace information is all the handling
> that is needed.  If there is any further logic needed in that rule,
> could you elaborate so that I don't make this mistake in the future,
> or so that I don't add a check on a driver object in a case that I
> should?

Here we have the case that only a Kernel developer will add a call to
this function. For a kernel developer a stack trace is more useful
than a pr_err. Of course this is different when not testing for a NULL
pointer causes subtle bugs in unrelated code.

> >
> > You should only unregister the cpuidle devices you successfully
> > registered. Unregistering not yet registered cpuidle devices probably
> > has unwanted side effects.
> >
> 
> I did not add in this handling because the cpuidle_unregister_device()
> call already has a "registered" check so extra handling seemed
> unnecessary.  But if you still think it is needed just let me know.
> 

It's ok then. I didn't check cpuidle_unregister_device.

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 |

Re: [PATCH v2 2/3] ARM: imx: Add imx5 cpuidle driver

[Sascha Hauer]

On Wed, May 02, 2012 at 03:11:35PM -0500, Rob Lee wrote:
> Sascha,
> 
> >>       mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
> >>       mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
> >> -     arm_pm_idle = imx5_idle;
> >> +     arm_pm_idle = (void *)imx5_idle;
> >
> > I don't like this. It will cover all warnings when the prototype of
> > arm_pm_idle changes in future. Better add a static void imx5_idle
> > which calls a static int imx5_do_idle, then you have an idle function
> > which returns an int.
> >
> 
> What about using the following:
> 
> arm_pm_idle =  (void (*)(void))imx5_idle;
> 
> This will give warnings if arm_pm_idle prototype changes.

This surely works but will look suspicious for people looking at the
code.

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 2/3] ARM: imx: Add imx5 cpuidle driver

[Sascha Hauer]

On Wed, May 02, 2012 at 03:11:35PM -0500, Rob Lee wrote:
> Sascha,
> 
> >> ? ? ? mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
> >> ? ? ? mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
> >> - ? ? arm_pm_idle = imx5_idle;
> >> + ? ? arm_pm_idle = (void *)imx5_idle;
> >
> > I don't like this. It will cover all warnings when the prototype of
> > arm_pm_idle changes in future. Better add a static void imx5_idle
> > which calls a static int imx5_do_idle, then you have an idle function
> > which returns an int.
> >
> 
> What about using the following:
> 
> arm_pm_idle =  (void (*)(void))imx5_idle;
> 
> This will give warnings if arm_pm_idle prototype changes.

This surely works but will look suspicious for people looking at the
code.

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 |