[PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[Daniel Lezcano]

There is a big number of cpuidle drivers for the ARM architecture.

These drivers have been cleaned up and grouped into the drivers/cpuidle
directory to keep track of the changes more easily and ensure the code
is following the same scheme across the drivers.

That had the benefit of simplifying the code and factor out a lot of common
parts. Beside that, as the drivers belong to the 'drivers' directory, we had
to split the arch specific bits and the generic code in order to keep
everything self contained. The platform driver paradigm was used for this
purpose.

Unfortunately, this approach is now no longer accepted and a different solution
must be provided to reach the same goal: one example is the Qualcomm cpuidle
driver upstreaming attempt [1].

In the meantime, ARM64 developed a generic cpuidle driver based on DT definition.

The DT definition provides an 'enable-method' to specify one of the cpu
operations (PSCI, ...).

This patchset unify this driver with ARM32, using the same DT definition.

Thanks with this patchset we can use the 'enable-method' to specify a cpu
operations, hence get rid of the platform driver approach and go further in the
cpuidle driver flexibility via the DT.

Daniel Lezcano (6):
  ARM: cpuidle: Remove duplicate header inclusion
  ARM: cpuidle: Add a cpuidle ops structure to be used for DT
  ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function
  ARM64: cpuidle: Rename cpu_init_idle to a common function name
  ARM64: cpuidle: Remove arm64 reference
  ARM: cpuidle: Enable the ARM64 driver for both ARM32/ARM64

 arch/arm/include/asm/cpuidle.h                     | 12 +++
 arch/arm/include/asm/cpuidle_ops.h                 |  3 +
 arch/arm/kernel/cpuidle.c                          | 87 +++++++++++++++++++++-
 arch/arm/mach-davinci/cpuidle.c                    |  1 -
 arch/arm/mach-imx/cpuidle-imx6q.c                  |  1 -
 arch/arm/mach-imx/cpuidle-imx6sl.c                 |  1 -
 arch/arm/mach-imx/cpuidle-imx6sx.c                 |  1 -
 arch/arm/mach-omap2/cpuidle44xx.c                  |  1 -
 arch/arm/mach-tegra/cpuidle-tegra20.c              |  1 -
 arch/arm/mach-tegra/cpuidle-tegra30.c              |  1 -
 arch/arm64/include/asm/cpuidle.h                   |  9 ++-
 arch/arm64/kernel/cpuidle.c                        |  2 +-
 drivers/cpuidle/Kconfig                            |  7 +-
 drivers/cpuidle/Kconfig.arm                        | 10 +++
 drivers/cpuidle/Kconfig.arm64                      | 13 ----
 drivers/cpuidle/Makefile                           |  5 +-
 drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} | 38 +++++-----
 drivers/cpuidle/cpuidle-at91.c                     |  1 -
 drivers/cpuidle/cpuidle-exynos.c                   |  1 -
 drivers/cpuidle/cpuidle-kirkwood.c                 |  1 -
 drivers/cpuidle/cpuidle-ux500.c                    |  1 -
 drivers/cpuidle/cpuidle-zynq.c                     |  1 -
 22 files changed, 139 insertions(+), 59 deletions(-)
 create mode 100644 arch/arm/include/asm/cpuidle_ops.h
 delete mode 100644 drivers/cpuidle/Kconfig.arm64
 rename drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} (72%)

-- 
1.9.1

[PATCH 1/6] ARM: cpuidle: Remove duplicate header inclusion

[Daniel Lezcano]

The cpu_do_idle() function is always used by the cpuidle drivers.

That led to have each driver including cpuidle.h and proc-fns.h, they are
always paired. That makes a lot of duplicate headers inclusion. Instead of
including both in each .c file, move the proc-fns.h header inclusion in the
cpuidle.h header file directly, so we can save some line of code.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/include/asm/cpuidle.h        | 2 ++
 arch/arm/kernel/cpuidle.c             | 2 +-
 arch/arm/mach-davinci/cpuidle.c       | 1 -
 arch/arm/mach-imx/cpuidle-imx6q.c     | 1 -
 arch/arm/mach-imx/cpuidle-imx6sl.c    | 1 -
 arch/arm/mach-imx/cpuidle-imx6sx.c    | 1 -
 arch/arm/mach-omap2/cpuidle44xx.c     | 1 -
 arch/arm/mach-tegra/cpuidle-tegra20.c | 1 -
 arch/arm/mach-tegra/cpuidle-tegra30.c | 1 -
 drivers/cpuidle/cpuidle-at91.c        | 1 -
 drivers/cpuidle/cpuidle-exynos.c      | 1 -
 drivers/cpuidle/cpuidle-kirkwood.c    | 1 -
 drivers/cpuidle/cpuidle-ux500.c       | 1 -
 drivers/cpuidle/cpuidle-zynq.c        | 1 -
 14 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
index af319ac..348dc81 100644
--- a/arch/arm/include/asm/cpuidle.h
+++ b/arch/arm/include/asm/cpuidle.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_ARM_CPUIDLE_H
 #define __ASM_ARM_CPUIDLE_H
 
+#include <asm/proc-fns.h>
+
 #ifdef CONFIG_CPU_IDLE
 extern int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
 		struct cpuidle_driver *drv, int index);
diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
index 89545f6..45969f8 100644
--- a/arch/arm/kernel/cpuidle.c
+++ b/arch/arm/kernel/cpuidle.c
@@ -10,7 +10,7 @@
  */
 
 #include <linux/cpuidle.h>
-#include <asm/proc-fns.h>
+#include <asm/cpuidle.h>
 
 int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
 		struct cpuidle_driver *drv, int index)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
index e365c1b..306ebc5 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
@@ -17,7 +17,6 @@
 #include <linux/cpuidle.h>
 #include <linux/io.h>
 #include <linux/export.h>
-#include <asm/proc-fns.h>
 #include <asm/cpuidle.h>
 
 #include <mach/cpuidle.h>
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
index d76d086..8e21ccc 100644
--- a/arch/arm/mach-imx/cpuidle-imx6q.c
+++ b/arch/arm/mach-imx/cpuidle-imx6q.c
@@ -9,7 +9,6 @@
 #include <linux/cpuidle.h>
 #include <linux/module.h>
 #include <asm/cpuidle.h>
-#include <asm/proc-fns.h>
 
 #include "common.h"
 #include "cpuidle.h"
diff --git a/arch/arm/mach-imx/cpuidle-imx6sl.c b/arch/arm/mach-imx/cpuidle-imx6sl.c
index 7d92e65..5742a9f 100644
--- a/arch/arm/mach-imx/cpuidle-imx6sl.c
+++ b/arch/arm/mach-imx/cpuidle-imx6sl.c
@@ -9,7 +9,6 @@
 #include <linux/cpuidle.h>
 #include <linux/module.h>
 #include <asm/cpuidle.h>
-#include <asm/proc-fns.h>
 
 #include "common.h"
 #include "cpuidle.h"
diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-imx/cpuidle-imx6sx.c
index 5a36722..2c9f1a8 100644
--- a/arch/arm/mach-imx/cpuidle-imx6sx.c
+++ b/arch/arm/mach-imx/cpuidle-imx6sx.c
@@ -10,7 +10,6 @@
 #include <linux/cpu_pm.h>
 #include <linux/module.h>
 #include <asm/cpuidle.h>
-#include <asm/proc-fns.h>
 #include <asm/suspend.h>
 
 #include "common.h"
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c
index 01e398a..7622dbb 100644
--- a/arch/arm/mach-omap2/cpuidle44xx.c
+++ b/arch/arm/mach-omap2/cpuidle44xx.c
@@ -17,7 +17,6 @@
 #include <linux/clockchips.h>
 
 #include <asm/cpuidle.h>
-#include <asm/proc-fns.h>
 
 #include "common.h"
 #include "pm.h"
diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c
index 4f25a7c..e22b0d9 100644
--- a/arch/arm/mach-tegra/cpuidle-tegra20.c
+++ b/arch/arm/mach-tegra/cpuidle-tegra20.c
@@ -27,7 +27,6 @@
 #include <linux/module.h>
 
 #include <asm/cpuidle.h>
-#include <asm/proc-fns.h>
 #include <asm/smp_plat.h>
 #include <asm/suspend.h>
 
diff --git a/arch/arm/mach-tegra/cpuidle-tegra30.c b/arch/arm/mach-tegra/cpuidle-tegra30.c
index f8815ed..a2400ab4 100644
--- a/arch/arm/mach-tegra/cpuidle-tegra30.c
+++ b/arch/arm/mach-tegra/cpuidle-tegra30.c
@@ -27,7 +27,6 @@
 #include <linux/module.h>
 
 #include <asm/cpuidle.h>
-#include <asm/proc-fns.h>
 #include <asm/smp_plat.h>
 #include <asm/suspend.h>
 
diff --git a/drivers/cpuidle/cpuidle-at91.c b/drivers/cpuidle/cpuidle-at91.c
index aae7bfc..f2446c7 100644
--- a/drivers/cpuidle/cpuidle-at91.c
+++ b/drivers/cpuidle/cpuidle-at91.c
@@ -19,7 +19,6 @@
 #include <linux/cpuidle.h>
 #include <linux/io.h>
 #include <linux/export.h>
-#include <asm/proc-fns.h>
 #include <asm/cpuidle.h>
 
 #define AT91_MAX_STATES	2
diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
index 26f5f29..0c06ea2 100644
--- a/drivers/cpuidle/cpuidle-exynos.c
+++ b/drivers/cpuidle/cpuidle-exynos.c
@@ -19,7 +19,6 @@
 #include <linux/of.h>
 #include <linux/platform_data/cpuidle-exynos.h>
 
-#include <asm/proc-fns.h>
 #include <asm/suspend.h>
 #include <asm/cpuidle.h>
 
diff --git a/drivers/cpuidle/cpuidle-kirkwood.c b/drivers/cpuidle/cpuidle-kirkwood.c
index cea0a6c..d23d8f4 100644
--- a/drivers/cpuidle/cpuidle-kirkwood.c
+++ b/drivers/cpuidle/cpuidle-kirkwood.c
@@ -21,7 +21,6 @@
 #include <linux/cpuidle.h>
 #include <linux/io.h>
 #include <linux/export.h>
-#include <asm/proc-fns.h>
 #include <asm/cpuidle.h>
 
 #define KIRKWOOD_MAX_STATES	2
diff --git a/drivers/cpuidle/cpuidle-ux500.c b/drivers/cpuidle/cpuidle-ux500.c
index 66f81e4..8bf895c 100644
--- a/drivers/cpuidle/cpuidle-ux500.c
+++ b/drivers/cpuidle/cpuidle-ux500.c
@@ -19,7 +19,6 @@
 #include <linux/platform_device.h>
 
 #include <asm/cpuidle.h>
-#include <asm/proc-fns.h>
 
 static atomic_t master = ATOMIC_INIT(0);
 static DEFINE_SPINLOCK(master_lock);
diff --git a/drivers/cpuidle/cpuidle-zynq.c b/drivers/cpuidle/cpuidle-zynq.c
index 002b8c9..543292b 100644
--- a/drivers/cpuidle/cpuidle-zynq.c
+++ b/drivers/cpuidle/cpuidle-zynq.c
@@ -28,7 +28,6 @@
 #include <linux/init.h>
 #include <linux/cpuidle.h>
 #include <linux/platform_device.h>
-#include <asm/proc-fns.h>
 #include <asm/cpuidle.h>
 
 #define ZYNQ_MAX_STATES		2
-- 
1.9.1

[PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT

[Daniel Lezcano]

The current state of the different cpuidle drivers is the different PM
operations are passed via the platform_data using the platform driver
paradigm.

This approach allowed to split the low level PM code from the arch specific
and the generic cpuidle code.

Unfortunately there are complains about this approach as, in the context of the
single kernel image, we have multiple drivers loaded in memory for nothing and
the platform driver is not adequate for cpuidle.

This patch provides a common interface via cpuidle ops for all new cpuidle
driver and a definition for the device tree.

It will allow with the next patches to a have a common definition with ARM64
and share the same cpuidle driver.

The code is optimized to use the __init section intensively in order to reduce
the memory footprint after the driver is initialized and unify the function
names with ARM64.

In order to prevent multiple declarations and the specific cpuidle ops to be
spread across the different headers, a mechanism, similar to the cgroup subsys,
has been introduced.

A new platform willing to add its cpuidle ops must add an entry in the file
cpuidle_ops.h in the current form:

 #if IS_ENABLED(CONFIG_ARM_FOO_CPUIDLE)
 CPUIDLE_OPS(foo)
 #endif

... and use the variable name in the specific low level code:

struct cpuidle_ops foo_cpuidle_ops;

The CPUIDLE_OPS macro will be processed in different way in the cpuidle.c file,
thus allowing to keep untouched the arm cpuidle core code in the future when
a new platform is added.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/include/asm/cpuidle.h     | 10 +++++
 arch/arm/include/asm/cpuidle_ops.h |  3 ++
 arch/arm/kernel/cpuidle.c          | 85 ++++++++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/cpuidle.h   |  5 ++-
 4 files changed, 102 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/include/asm/cpuidle_ops.h

diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
index 348dc81..3d31459 100644
--- a/arch/arm/include/asm/cpuidle.h
+++ b/arch/arm/include/asm/cpuidle.h
@@ -27,4 +27,14 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
  */
 #define ARM_CPUIDLE_WFI_STATE ARM_CPUIDLE_WFI_STATE_PWR(UINT_MAX)
 
+struct cpuidle_ops {
+	const char *name;
+	int (*suspend)(int cpu, unsigned long arg);
+	int (*init)(struct device_node *, int cpu);
+};
+
+extern int arm_cpuidle_suspend(int index);
+
+extern int arm_cpuidle_init(int cpu);
+
 #endif
diff --git a/arch/arm/include/asm/cpuidle_ops.h b/arch/arm/include/asm/cpuidle_ops.h
new file mode 100644
index 0000000..be0a612
--- /dev/null
+++ b/arch/arm/include/asm/cpuidle_ops.h
@@ -0,0 +1,3 @@
+/*
+ * List of cpuidle operations
+ */
diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
index 45969f8..25e9789c 100644
--- a/arch/arm/kernel/cpuidle.c
+++ b/arch/arm/kernel/cpuidle.c
@@ -10,8 +10,29 @@
  */
 
 #include <linux/cpuidle.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <asm/cpuidle.h>
 
+#define CPUIDLE_OPS(__x) extern struct cpuidle_ops __x ## _cpuidle_ops;
+#include <asm/cpuidle_ops.h>
+#undef CPUIDLE_OPS
+
+#define CPUIDLE_OPS(__x) __x ## _cpuidle_ops_id,
+enum cpuidle_ops_id {
+#include <asm/cpuidle_ops.h>
+        CPUIDLE_OPS_COUNT,
+};
+#undef CPUIDLE_OPS
+
+#define CPUIDLE_OPS(__x) [__x ## _cpuidle_ops_id ] = &__x ## _cpuidle_ops,
+static struct cpuidle_ops *supported_cpuidle_ops[] __initconst = {
+#include <asm/cpuidle_ops.h>
+};
+#undef CPUIDLE_OPS
+
+static struct cpuidle_ops cpuidle_ops[NR_CPUS];
+
 int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
 		struct cpuidle_driver *drv, int index)
 {
@@ -19,3 +40,67 @@ int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
 
 	return index;
 }
+
+int arm_cpuidle_suspend(int index)
+{
+	int ret = -EOPNOTSUPP;
+	int cpu = smp_processor_id();
+
+	if (cpuidle_ops[cpu].suspend)
+		ret = cpuidle_ops[cpu].suspend(cpu, index);
+
+	return ret;
+}
+
+static struct cpuidle_ops *__init arm_cpuidle_get_ops(const char *name)
+{
+	int i;
+
+	for (i = 0; i < CPUIDLE_OPS_COUNT; i++) {
+		if (!strcmp(name, supported_cpuidle_ops[i]->name))
+			return supported_cpuidle_ops[i];
+	}
+
+	return NULL;
+}
+
+static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
+{
+	const char *enable_method;
+	struct cpuidle_ops *ops;
+
+	enable_method = of_get_property(dn, "enable-method", NULL);
+	if (!enable_method)
+		return -ENOENT;
+
+	ops = arm_cpuidle_get_ops(enable_method);
+	if (!ops) {
+		pr_warn("%s: unsupported enable-method property: %s\n",
+			dn->full_name, enable_method);
+		return -EOPNOTSUPP;
+	}
+
+	cpuidle_ops[cpu] = *ops; /* structure copy */
+
+	pr_notice("cpuidle: enable-method property '%s'"
+		  " found operations\n", ops->name);
+
+	return 0;
+}
+
+int __init arm_cpuidle_init(int cpu)
+{
+	int ret = -EOPNOTSUPP;
+	struct device_node *cpu_node = of_cpu_device_node_get(cpu);
+
+	if (!cpu_node)
+		return -ENODEV;
+
+	ret = arm_cpuidle_read_ops(cpu_node, cpu);
+	if (!ret && cpuidle_ops[cpu].init)
+		ret = cpuidle_ops[cpu].init(cpu_node, cpu);
+
+	of_node_put(cpu_node);
+
+	return ret;
+}
diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
index 0710654..1bee287 100644
--- a/arch/arm64/include/asm/cpuidle.h
+++ b/arch/arm64/include/asm/cpuidle.h
@@ -15,5 +15,8 @@ static inline int cpu_suspend(unsigned long arg)
 	return -EOPNOTSUPP;
 }
 #endif
-
+static inline int arm_cpuidle_suspend(int index)
+{
+	return cpu_suspend(index);
+}
 #endif
-- 
1.9.1

[PATCH 3/6] ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function

[Daniel Lezcano]

Call the common ARM/ARM64 'arm_cpuidle_suspend' instead of cpu_suspend function
which is specific to ARM64.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle-arm64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
index 39a2c62..0cea244 100644
--- a/drivers/cpuidle/cpuidle-arm64.c
+++ b/drivers/cpuidle/cpuidle-arm64.c
@@ -49,7 +49,7 @@ static int arm64_enter_idle_state(struct cpuidle_device *dev,
 		 * call the CPU ops suspend protocol with idle index as a
 		 * parameter.
 		 */
-		ret = cpu_suspend(idx);
+		arm_cpuidle_suspend(idx);
 
 		cpu_pm_exit();
 	}
-- 
1.9.1

[PATCH 4/6] ARM64: cpuidle: Rename cpu_init_idle to a common function name

[Daniel Lezcano]

With this change the cpuidle-arm64.c file calls the same function name
for both ARM and ARM64.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm64/include/asm/cpuidle.h | 4 ++--
 arch/arm64/kernel/cpuidle.c      | 2 +-
 drivers/cpuidle/cpuidle-arm64.c  | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
index 1bee287..2bc2732 100644
--- a/arch/arm64/include/asm/cpuidle.h
+++ b/arch/arm64/include/asm/cpuidle.h
@@ -2,10 +2,10 @@
 #define __ASM_CPUIDLE_H
 
 #ifdef CONFIG_CPU_IDLE
-extern int cpu_init_idle(unsigned int cpu);
+extern int arm_cpuidle_init(unsigned int cpu);
 extern int cpu_suspend(unsigned long arg);
 #else
-static inline int cpu_init_idle(unsigned int cpu)
+static inline int arm_cpuidle_init(unsigned int cpu)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index 5c08966..a78143a 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -15,7 +15,7 @@
 #include <asm/cpuidle.h>
 #include <asm/cpu_ops.h>
 
-int cpu_init_idle(unsigned int cpu)
+int arm_cpuidle_init(unsigned int cpu)
 {
 	int ret = -EOPNOTSUPP;
 	struct device_node *cpu_node = of_cpu_device_node_get(cpu);
diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
index 0cea244..6ef291c7 100644
--- a/drivers/cpuidle/cpuidle-arm64.c
+++ b/drivers/cpuidle/cpuidle-arm64.c
@@ -110,7 +110,7 @@ static int __init arm64_idle_init(void)
 	 * idle states suspend back-end specific data
 	 */
 	for_each_possible_cpu(cpu) {
-		ret = cpu_init_idle(cpu);
+		ret = arm_cpuidle_init(cpu);
 		if (ret) {
 			pr_err("CPU %d failed to init idle CPU ops\n", cpu);
 			return ret;
-- 
1.9.1

[PATCH 5/6] ARM64: cpuidle: Remove arm64 reference

[Daniel Lezcano]

In the next patch, this driver will be common across ARM/ARM64. Remove all refs
to ARM64 as it will be shared with ARM32.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle-arm64.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
index 6ef291c7..1c94b88 100644
--- a/drivers/cpuidle/cpuidle-arm64.c
+++ b/drivers/cpuidle/cpuidle-arm64.c
@@ -1,5 +1,5 @@
 /*
- * ARM64 generic CPU idle driver.
+ * ARM/ARM64 generic CPU idle driver.
  *
  * Copyright (C) 2014 ARM Ltd.
  * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
@@ -9,7 +9,7 @@
  * published by the Free Software Foundation.
  */
 
-#define pr_fmt(fmt) "CPUidle arm64: " fmt
+#define pr_fmt(fmt) "CPUidle arm: " fmt
 
 #include <linux/cpuidle.h>
 #include <linux/cpumask.h>
@@ -23,7 +23,7 @@
 #include "dt_idle_states.h"
 
 /*
- * arm64_enter_idle_state - Programs CPU to enter the specified state
+ * arm_enter_idle_state - Programs CPU to enter the specified state
  *
  * dev: cpuidle device
  * drv: cpuidle driver
@@ -32,8 +32,8 @@
  * Called from the CPUidle framework to program the device to the
  * specified target state selected by the governor.
  */
-static int arm64_enter_idle_state(struct cpuidle_device *dev,
-				  struct cpuidle_driver *drv, int idx)
+static int arm_enter_idle_state(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int idx)
 {
 	int ret;
 
@@ -57,8 +57,8 @@ static int arm64_enter_idle_state(struct cpuidle_device *dev,
 	return ret ? -1 : idx;
 }
 
-static struct cpuidle_driver arm64_idle_driver = {
-	.name = "arm64_idle",
+static struct cpuidle_driver arm_idle_driver = {
+	.name = "arm_idle",
 	.owner = THIS_MODULE,
 	/*
 	 * State at index 0 is standby wfi and considered standard
@@ -68,32 +68,32 @@ static struct cpuidle_driver arm64_idle_driver = {
 	 * handler for idle state index 0.
 	 */
 	.states[0] = {
-		.enter                  = arm64_enter_idle_state,
+		.enter                  = arm_enter_idle_state,
 		.exit_latency           = 1,
 		.target_residency       = 1,
 		.power_usage		= UINT_MAX,
 		.name                   = "WFI",
-		.desc                   = "ARM64 WFI",
+		.desc                   = "ARM WFI",
 	}
 };
 
-static const struct of_device_id arm64_idle_state_match[] __initconst = {
+static const struct of_device_id arm_idle_state_match[] __initconst = {
 	{ .compatible = "arm,idle-state",
-	  .data = arm64_enter_idle_state },
+	  .data = arm_enter_idle_state },
 	{ },
 };
 
 /*
- * arm64_idle_init
+ * arm_idle_init
  *
- * Registers the arm64 specific cpuidle driver with the cpuidle
+ * Registers the arm specific cpuidle driver with the cpuidle
  * framework. It relies on core code to parse the idle states
  * and initialize them using driver data structures accordingly.
  */
-static int __init arm64_idle_init(void)
+static int __init arm_idle_init(void)
 {
 	int cpu, ret;
-	struct cpuidle_driver *drv = &arm64_idle_driver;
+	struct cpuidle_driver *drv = &arm_idle_driver;
 
 	/*
 	 * Initialize idle states data, starting at index 1.
@@ -101,7 +101,7 @@ static int __init arm64_idle_init(void)
 	 * let the driver initialization fail accordingly since there is no
 	 * reason to initialize the idle driver if only wfi is supported.
 	 */
-	ret = dt_init_idle_driver(drv, arm64_idle_state_match, 1);
+	ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
 	if (ret <= 0)
 		return ret ? : -ENODEV;
 
@@ -119,4 +119,4 @@ static int __init arm64_idle_init(void)
 
 	return cpuidle_register(drv, NULL);
 }
-device_initcall(arm64_idle_init);
+device_initcall(arm_idle_init);
-- 
1.9.1

[PATCH 6/6] ARM: cpuidle: Enable the ARM64 driver for both ARM32/ARM64

[Daniel Lezcano]

ARM32 and ARM64 have the same DT definitions and the same approaches.

The generic ARM cpuidle driver can be put in common for those two
architectures.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/Kconfig                            |  7 +------
 drivers/cpuidle/Kconfig.arm                        | 10 ++++++++++
 drivers/cpuidle/Kconfig.arm64                      | 13 -------------
 drivers/cpuidle/Makefile                           |  5 +----
 drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} |  0
 5 files changed, 12 insertions(+), 23 deletions(-)
 delete mode 100644 drivers/cpuidle/Kconfig.arm64
 rename drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} (100%)

diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index c5029c1..8c7930b 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -29,15 +29,10 @@ config DT_IDLE_STATES
 	bool
 
 menu "ARM CPU Idle Drivers"
-depends on ARM
+depends on ARM || ARM64
 source "drivers/cpuidle/Kconfig.arm"
 endmenu
 
-menu "ARM64 CPU Idle Drivers"
-depends on ARM64
-source "drivers/cpuidle/Kconfig.arm64"
-endmenu
-
 menu "MIPS CPU Idle Drivers"
 depends on MIPS
 source "drivers/cpuidle/Kconfig.mips"
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 8e07c94..f08043e 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -14,6 +14,16 @@ config ARM_BIG_LITTLE_CPUIDLE
 	  define different C-states for little and big cores through the
 	  multiple CPU idle drivers infrastructure.
 
+config ARM_CPUIDLE
+	bool "Generic ARM/ARM64 CPU idle Driver"
+	select DT_IDLE_STATES
+	help
+	  Select this to enable generic cpuidle driver for ARM.
+	  It provides a generic idle driver whose idle states are configured
+	  at run-time through DT nodes. The CPUidle suspend backend is
+	  initialized by calling the CPU operations init idle hook
+	  provided by architecture code.
+
 config ARM_CLPS711X_CPUIDLE
 	bool "CPU Idle Driver for CLPS711X processors"
 	depends on ARCH_CLPS711X || COMPILE_TEST
diff --git a/drivers/cpuidle/Kconfig.arm64 b/drivers/cpuidle/Kconfig.arm64
deleted file mode 100644
index 6effb36..0000000
--- a/drivers/cpuidle/Kconfig.arm64
+++ /dev/null
@@ -1,13 +0,0 @@
-#
-# ARM64 CPU Idle drivers
-#
-
-config ARM64_CPUIDLE
-	bool "Generic ARM64 CPU idle Driver"
-	select DT_IDLE_STATES
-	help
-	  Select this to enable generic cpuidle driver for ARM64.
-	  It provides a generic idle driver whose idle states are configured
-	  at run-time through DT nodes. The CPUidle suspend backend is
-	  initialized by calling the CPU operations init idle hook
-	  provided by architecture code.
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 4d177b9..3ba81b1 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -17,16 +17,13 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
 obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
 obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
 obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
+obj-$(CONFIG_ARM_CPUIDLE)		+= cpuidle-arm.o
 
 ###############################################################################
 # MIPS drivers
 obj-$(CONFIG_MIPS_CPS_CPUIDLE)		+= cpuidle-cps.o
 
 ###############################################################################
-# ARM64 drivers
-obj-$(CONFIG_ARM64_CPUIDLE)		+= cpuidle-arm64.o
-
-###############################################################################
 # POWERPC drivers
 obj-$(CONFIG_PSERIES_CPUIDLE)		+= cpuidle-pseries.o
 obj-$(CONFIG_POWERNV_CPUIDLE)		+= cpuidle-powernv.o
diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm.c
similarity index 100%
rename from drivers/cpuidle/cpuidle-arm64.c
rename to drivers/cpuidle/cpuidle-arm.c
-- 
1.9.1

Re: [PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[Daniel Lezcano]

Catalin, Rob,

do you agree with this patchset ?

Thanks
   -- Daniel

On 03/03/2015 01:29 PM, Daniel Lezcano wrote:
> There is a big number of cpuidle drivers for the ARM architecture.
>
> These drivers have been cleaned up and grouped into the drivers/cpuidle
> directory to keep track of the changes more easily and ensure the code
> is following the same scheme across the drivers.
>
> That had the benefit of simplifying the code and factor out a lot of common
> parts. Beside that, as the drivers belong to the 'drivers' directory, we had
> to split the arch specific bits and the generic code in order to keep
> everything self contained. The platform driver paradigm was used for this
> purpose.
>
> Unfortunately, this approach is now no longer accepted and a different solution
> must be provided to reach the same goal: one example is the Qualcomm cpuidle
> driver upstreaming attempt [1].
>
> In the meantime, ARM64 developed a generic cpuidle driver based on DT definition.
>
> The DT definition provides an 'enable-method' to specify one of the cpu
> operations (PSCI, ...).
>
> This patchset unify this driver with ARM32, using the same DT definition.
>
> Thanks with this patchset we can use the 'enable-method' to specify a cpu
> operations, hence get rid of the platform driver approach and go further in the
> cpuidle driver flexibility via the DT.
>
> Daniel Lezcano (6):
>    ARM: cpuidle: Remove duplicate header inclusion
>    ARM: cpuidle: Add a cpuidle ops structure to be used for DT
>    ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function
>    ARM64: cpuidle: Rename cpu_init_idle to a common function name
>    ARM64: cpuidle: Remove arm64 reference
>    ARM: cpuidle: Enable the ARM64 driver for both ARM32/ARM64
>
>   arch/arm/include/asm/cpuidle.h                     | 12 +++
>   arch/arm/include/asm/cpuidle_ops.h                 |  3 +
>   arch/arm/kernel/cpuidle.c                          | 87 +++++++++++++++++++++-
>   arch/arm/mach-davinci/cpuidle.c                    |  1 -
>   arch/arm/mach-imx/cpuidle-imx6q.c                  |  1 -
>   arch/arm/mach-imx/cpuidle-imx6sl.c                 |  1 -
>   arch/arm/mach-imx/cpuidle-imx6sx.c                 |  1 -
>   arch/arm/mach-omap2/cpuidle44xx.c                  |  1 -
>   arch/arm/mach-tegra/cpuidle-tegra20.c              |  1 -
>   arch/arm/mach-tegra/cpuidle-tegra30.c              |  1 -
>   arch/arm64/include/asm/cpuidle.h                   |  9 ++-
>   arch/arm64/kernel/cpuidle.c                        |  2 +-
>   drivers/cpuidle/Kconfig                            |  7 +-
>   drivers/cpuidle/Kconfig.arm                        | 10 +++
>   drivers/cpuidle/Kconfig.arm64                      | 13 ----
>   drivers/cpuidle/Makefile                           |  5 +-
>   drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} | 38 +++++-----
>   drivers/cpuidle/cpuidle-at91.c                     |  1 -
>   drivers/cpuidle/cpuidle-exynos.c                   |  1 -
>   drivers/cpuidle/cpuidle-kirkwood.c                 |  1 -
>   drivers/cpuidle/cpuidle-ux500.c                    |  1 -
>   drivers/cpuidle/cpuidle-zynq.c                     |  1 -
>   22 files changed, 139 insertions(+), 59 deletions(-)
>   create mode 100644 arch/arm/include/asm/cpuidle_ops.h
>   delete mode 100644 drivers/cpuidle/Kconfig.arm64
>   rename drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} (72%)
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[Kevin Hilman]

Daniel Lezcano <daniel.lezcano@linaro.org> writes:

> There is a big number of cpuidle drivers for the ARM architecture.
>
> These drivers have been cleaned up and grouped into the drivers/cpuidle
> directory to keep track of the changes more easily and ensure the code
> is following the same scheme across the drivers.
>
> That had the benefit of simplifying the code and factor out a lot of common
> parts. Beside that, as the drivers belong to the 'drivers' directory, we had
> to split the arch specific bits and the generic code in order to keep
> everything self contained. The platform driver paradigm was used for this
> purpose.
>
> Unfortunately, this approach is now no longer accepted and a different solution
> must be provided to reach the same goal: one example is the Qualcomm cpuidle
> driver upstreaming attempt [1].
>
> In the meantime, ARM64 developed a generic cpuidle driver based on DT definition.
>
> The DT definition provides an 'enable-method' to specify one of the cpu
> operations (PSCI, ...).
>
> This patchset unify this driver with ARM32, using the same DT definition.
>
> Thanks with this patchset we can use the 'enable-method' to specify a cpu
> operations, hence get rid of the platform driver approach and go further in the
> cpuidle driver flexibility via the DT.

I really like that these two are unified now.

Acked-by: Kevin Hilman <khilman@linaro.org>

Re: [PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[Daniel Lezcano]

On 03/13/2015 06:03 PM, Kevin Hilman wrote:
> Daniel Lezcano <daniel.lezcano@linaro.org> writes:
>
>> There is a big number of cpuidle drivers for the ARM architecture.
>>
>> These drivers have been cleaned up and grouped into the drivers/cpuidle
>> directory to keep track of the changes more easily and ensure the code
>> is following the same scheme across the drivers.
>>
>> That had the benefit of simplifying the code and factor out a lot of common
>> parts. Beside that, as the drivers belong to the 'drivers' directory, we had
>> to split the arch specific bits and the generic code in order to keep
>> everything self contained. The platform driver paradigm was used for this
>> purpose.
>>
>> Unfortunately, this approach is now no longer accepted and a different solution
>> must be provided to reach the same goal: one example is the Qualcomm cpuidle
>> driver upstreaming attempt [1].
>>
>> In the meantime, ARM64 developed a generic cpuidle driver based on DT definition.
>>
>> The DT definition provides an 'enable-method' to specify one of the cpu
>> operations (PSCI, ...).
>>
>> This patchset unify this driver with ARM32, using the same DT definition.
>>
>> Thanks with this patchset we can use the 'enable-method' to specify a cpu
>> operations, hence get rid of the platform driver approach and go further in the
>> cpuidle driver flexibility via the DT.
>
> I really like that these two are unified now.
>
> Acked-by: Kevin Hilman <khilman@linaro.org>

Thanks for reviewing the patchset.

   -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/6] ARM: cpuidle: Remove duplicate header inclusion

[Lorenzo Pieralisi]

On Tue, Mar 03, 2015 at 12:29:32PM +0000, Daniel Lezcano wrote:
> The cpu_do_idle() function is always used by the cpuidle drivers.
> 
> That led to have each driver including cpuidle.h and proc-fns.h, they are
> always paired. That makes a lot of duplicate headers inclusion. Instead of
> including both in each .c file, move the proc-fns.h header inclusion in the
> cpuidle.h header file directly, so we can save some line of code.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> ---
>  arch/arm/include/asm/cpuidle.h        | 2 ++
>  arch/arm/kernel/cpuidle.c             | 2 +-
>  arch/arm/mach-davinci/cpuidle.c       | 1 -
>  arch/arm/mach-imx/cpuidle-imx6q.c     | 1 -
>  arch/arm/mach-imx/cpuidle-imx6sl.c    | 1 -
>  arch/arm/mach-imx/cpuidle-imx6sx.c    | 1 -
>  arch/arm/mach-omap2/cpuidle44xx.c     | 1 -
>  arch/arm/mach-tegra/cpuidle-tegra20.c | 1 -
>  arch/arm/mach-tegra/cpuidle-tegra30.c | 1 -
>  drivers/cpuidle/cpuidle-at91.c        | 1 -
>  drivers/cpuidle/cpuidle-exynos.c      | 1 -
>  drivers/cpuidle/cpuidle-kirkwood.c    | 1 -
>  drivers/cpuidle/cpuidle-ux500.c       | 1 -
>  drivers/cpuidle/cpuidle-zynq.c        | 1 -
>  14 files changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> index af319ac..348dc81 100644
> --- a/arch/arm/include/asm/cpuidle.h
> +++ b/arch/arm/include/asm/cpuidle.h
> @@ -1,6 +1,8 @@
>  #ifndef __ASM_ARM_CPUIDLE_H
>  #define __ASM_ARM_CPUIDLE_H
>  
> +#include <asm/proc-fns.h>
> +
>  #ifdef CONFIG_CPU_IDLE
>  extern int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>  		struct cpuidle_driver *drv, int index);
> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> index 89545f6..45969f8 100644
> --- a/arch/arm/kernel/cpuidle.c
> +++ b/arch/arm/kernel/cpuidle.c
> @@ -10,7 +10,7 @@
>   */
>  
>  #include <linux/cpuidle.h>
> -#include <asm/proc-fns.h>
> +#include <asm/cpuidle.h>
>  
>  int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>  		struct cpuidle_driver *drv, int index)
> diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
> index e365c1b..306ebc5 100644
> --- a/arch/arm/mach-davinci/cpuidle.c
> +++ b/arch/arm/mach-davinci/cpuidle.c
> @@ -17,7 +17,6 @@
>  #include <linux/cpuidle.h>
>  #include <linux/io.h>
>  #include <linux/export.h>
> -#include <asm/proc-fns.h>
>  #include <asm/cpuidle.h>
>  
>  #include <mach/cpuidle.h>
> diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
> index d76d086..8e21ccc 100644
> --- a/arch/arm/mach-imx/cpuidle-imx6q.c
> +++ b/arch/arm/mach-imx/cpuidle-imx6q.c
> @@ -9,7 +9,6 @@
>  #include <linux/cpuidle.h>
>  #include <linux/module.h>
>  #include <asm/cpuidle.h>
> -#include <asm/proc-fns.h>
>  
>  #include "common.h"
>  #include "cpuidle.h"
> diff --git a/arch/arm/mach-imx/cpuidle-imx6sl.c b/arch/arm/mach-imx/cpuidle-imx6sl.c
> index 7d92e65..5742a9f 100644
> --- a/arch/arm/mach-imx/cpuidle-imx6sl.c
> +++ b/arch/arm/mach-imx/cpuidle-imx6sl.c
> @@ -9,7 +9,6 @@
>  #include <linux/cpuidle.h>
>  #include <linux/module.h>
>  #include <asm/cpuidle.h>
> -#include <asm/proc-fns.h>
>  
>  #include "common.h"
>  #include "cpuidle.h"
> diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-imx/cpuidle-imx6sx.c
> index 5a36722..2c9f1a8 100644
> --- a/arch/arm/mach-imx/cpuidle-imx6sx.c
> +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c
> @@ -10,7 +10,6 @@
>  #include <linux/cpu_pm.h>
>  #include <linux/module.h>
>  #include <asm/cpuidle.h>
> -#include <asm/proc-fns.h>
>  #include <asm/suspend.h>
>  
>  #include "common.h"
> diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c
> index 01e398a..7622dbb 100644
> --- a/arch/arm/mach-omap2/cpuidle44xx.c
> +++ b/arch/arm/mach-omap2/cpuidle44xx.c
> @@ -17,7 +17,6 @@
>  #include <linux/clockchips.h>
>  
>  #include <asm/cpuidle.h>
> -#include <asm/proc-fns.h>
>  
>  #include "common.h"
>  #include "pm.h"
> diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c
> index 4f25a7c..e22b0d9 100644
> --- a/arch/arm/mach-tegra/cpuidle-tegra20.c
> +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c
> @@ -27,7 +27,6 @@
>  #include <linux/module.h>
>  
>  #include <asm/cpuidle.h>
> -#include <asm/proc-fns.h>
>  #include <asm/smp_plat.h>
>  #include <asm/suspend.h>
>  
> diff --git a/arch/arm/mach-tegra/cpuidle-tegra30.c b/arch/arm/mach-tegra/cpuidle-tegra30.c
> index f8815ed..a2400ab4 100644
> --- a/arch/arm/mach-tegra/cpuidle-tegra30.c
> +++ b/arch/arm/mach-tegra/cpuidle-tegra30.c
> @@ -27,7 +27,6 @@
>  #include <linux/module.h>
>  
>  #include <asm/cpuidle.h>
> -#include <asm/proc-fns.h>
>  #include <asm/smp_plat.h>
>  #include <asm/suspend.h>
>  
> diff --git a/drivers/cpuidle/cpuidle-at91.c b/drivers/cpuidle/cpuidle-at91.c
> index aae7bfc..f2446c7 100644
> --- a/drivers/cpuidle/cpuidle-at91.c
> +++ b/drivers/cpuidle/cpuidle-at91.c
> @@ -19,7 +19,6 @@
>  #include <linux/cpuidle.h>
>  #include <linux/io.h>
>  #include <linux/export.h>
> -#include <asm/proc-fns.h>
>  #include <asm/cpuidle.h>
>  
>  #define AT91_MAX_STATES	2
> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
> index 26f5f29..0c06ea2 100644
> --- a/drivers/cpuidle/cpuidle-exynos.c
> +++ b/drivers/cpuidle/cpuidle-exynos.c
> @@ -19,7 +19,6 @@
>  #include <linux/of.h>
>  #include <linux/platform_data/cpuidle-exynos.h>
>  
> -#include <asm/proc-fns.h>
>  #include <asm/suspend.h>
>  #include <asm/cpuidle.h>
>  
> diff --git a/drivers/cpuidle/cpuidle-kirkwood.c b/drivers/cpuidle/cpuidle-kirkwood.c
> index cea0a6c..d23d8f4 100644
> --- a/drivers/cpuidle/cpuidle-kirkwood.c
> +++ b/drivers/cpuidle/cpuidle-kirkwood.c
> @@ -21,7 +21,6 @@
>  #include <linux/cpuidle.h>
>  #include <linux/io.h>
>  #include <linux/export.h>
> -#include <asm/proc-fns.h>
>  #include <asm/cpuidle.h>
>  
>  #define KIRKWOOD_MAX_STATES	2
> diff --git a/drivers/cpuidle/cpuidle-ux500.c b/drivers/cpuidle/cpuidle-ux500.c
> index 66f81e4..8bf895c 100644
> --- a/drivers/cpuidle/cpuidle-ux500.c
> +++ b/drivers/cpuidle/cpuidle-ux500.c
> @@ -19,7 +19,6 @@
>  #include <linux/platform_device.h>
>  
>  #include <asm/cpuidle.h>
> -#include <asm/proc-fns.h>
>  
>  static atomic_t master = ATOMIC_INIT(0);
>  static DEFINE_SPINLOCK(master_lock);
> diff --git a/drivers/cpuidle/cpuidle-zynq.c b/drivers/cpuidle/cpuidle-zynq.c
> index 002b8c9..543292b 100644
> --- a/drivers/cpuidle/cpuidle-zynq.c
> +++ b/drivers/cpuidle/cpuidle-zynq.c
> @@ -28,7 +28,6 @@
>  #include <linux/init.h>
>  #include <linux/cpuidle.h>
>  #include <linux/platform_device.h>
> -#include <asm/proc-fns.h>
>  #include <asm/cpuidle.h>
>  
>  #define ZYNQ_MAX_STATES		2
> -- 
> 1.9.1
> 
> 

Re: [PATCH 3/6] ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function

[Catalin Marinas]

On Tue, Mar 03, 2015 at 01:29:34PM +0100, Daniel Lezcano wrote:
> Call the common ARM/ARM64 'arm_cpuidle_suspend' instead of cpu_suspend function
> which is specific to ARM64.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-arm64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
> index 39a2c62..0cea244 100644
> --- a/drivers/cpuidle/cpuidle-arm64.c
> +++ b/drivers/cpuidle/cpuidle-arm64.c
> @@ -49,7 +49,7 @@ static int arm64_enter_idle_state(struct cpuidle_device *dev,
>  		 * call the CPU ops suspend protocol with idle index as a
>  		 * parameter.
>  		 */
> -		ret = cpu_suspend(idx);
> +		arm_cpuidle_suspend(idx);

Nitpick: why don't we just rename the arm one cpuidle_suspend()?

-- 
Catalin

Re: [PATCH 4/6] ARM64: cpuidle: Rename cpu_init_idle to a common function name

[Catalin Marinas]

On Tue, Mar 03, 2015 at 01:29:35PM +0100, Daniel Lezcano wrote:
> diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
> index 0cea244..6ef291c7 100644
> --- a/drivers/cpuidle/cpuidle-arm64.c
> +++ b/drivers/cpuidle/cpuidle-arm64.c
> @@ -110,7 +110,7 @@ static int __init arm64_idle_init(void)
>  	 * idle states suspend back-end specific data
>  	 */
>  	for_each_possible_cpu(cpu) {
> -		ret = cpu_init_idle(cpu);
> +		ret = arm_cpuidle_init(cpu);

Same nitpick here about dropping the arm_ prefix (though here we already
have a cpuidle_init).

-- 
Catalin

Re: [PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[Catalin Marinas]

On Thu, Mar 12, 2015 at 03:25:34PM +0100, Daniel Lezcano wrote:
> do you agree with this patchset ?

In principle yes, apart from some function naming and I'm waiting for
Lorenzo's ack as well. Do you plan to upstream this directly via your
tree? If yes, I'll look in more detail and give some acks.

-- 
Catalin

Re: [PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[Rob Herring]

On Thu, Mar 12, 2015 at 9:25 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> Catalin, Rob,
>
> do you agree with this patchset ?

There's very little to do with DT, but looks fine to me.

Rob

>
> Thanks
>   -- Daniel
>
>
> On 03/03/2015 01:29 PM, Daniel Lezcano wrote:
>>
>> There is a big number of cpuidle drivers for the ARM architecture.
>>
>> These drivers have been cleaned up and grouped into the drivers/cpuidle
>> directory to keep track of the changes more easily and ensure the code
>> is following the same scheme across the drivers.
>>
>> That had the benefit of simplifying the code and factor out a lot of
>> common
>> parts. Beside that, as the drivers belong to the 'drivers' directory, we
>> had
>> to split the arch specific bits and the generic code in order to keep
>> everything self contained. The platform driver paradigm was used for this
>> purpose.
>>
>> Unfortunately, this approach is now no longer accepted and a different
>> solution
>> must be provided to reach the same goal: one example is the Qualcomm
>> cpuidle
>> driver upstreaming attempt [1].
>>
>> In the meantime, ARM64 developed a generic cpuidle driver based on DT
>> definition.
>>
>> The DT definition provides an 'enable-method' to specify one of the cpu
>> operations (PSCI, ...).
>>
>> This patchset unify this driver with ARM32, using the same DT definition.
>>
>> Thanks with this patchset we can use the 'enable-method' to specify a cpu
>> operations, hence get rid of the platform driver approach and go further
>> in the
>> cpuidle driver flexibility via the DT.
>>
>> Daniel Lezcano (6):
>>    ARM: cpuidle: Remove duplicate header inclusion
>>    ARM: cpuidle: Add a cpuidle ops structure to be used for DT
>>    ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function
>>    ARM64: cpuidle: Rename cpu_init_idle to a common function name
>>    ARM64: cpuidle: Remove arm64 reference
>>    ARM: cpuidle: Enable the ARM64 driver for both ARM32/ARM64
>>
>>   arch/arm/include/asm/cpuidle.h                     | 12 +++
>>   arch/arm/include/asm/cpuidle_ops.h                 |  3 +
>>   arch/arm/kernel/cpuidle.c                          | 87
>> +++++++++++++++++++++-
>>   arch/arm/mach-davinci/cpuidle.c                    |  1 -
>>   arch/arm/mach-imx/cpuidle-imx6q.c                  |  1 -
>>   arch/arm/mach-imx/cpuidle-imx6sl.c                 |  1 -
>>   arch/arm/mach-imx/cpuidle-imx6sx.c                 |  1 -
>>   arch/arm/mach-omap2/cpuidle44xx.c                  |  1 -
>>   arch/arm/mach-tegra/cpuidle-tegra20.c              |  1 -
>>   arch/arm/mach-tegra/cpuidle-tegra30.c              |  1 -
>>   arch/arm64/include/asm/cpuidle.h                   |  9 ++-
>>   arch/arm64/kernel/cpuidle.c                        |  2 +-
>>   drivers/cpuidle/Kconfig                            |  7 +-
>>   drivers/cpuidle/Kconfig.arm                        | 10 +++
>>   drivers/cpuidle/Kconfig.arm64                      | 13 ----
>>   drivers/cpuidle/Makefile                           |  5 +-
>>   drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} | 38 +++++-----
>>   drivers/cpuidle/cpuidle-at91.c                     |  1 -
>>   drivers/cpuidle/cpuidle-exynos.c                   |  1 -
>>   drivers/cpuidle/cpuidle-kirkwood.c                 |  1 -
>>   drivers/cpuidle/cpuidle-ux500.c                    |  1 -
>>   drivers/cpuidle/cpuidle-zynq.c                     |  1 -
>>   22 files changed, 139 insertions(+), 59 deletions(-)
>>   create mode 100644 arch/arm/include/asm/cpuidle_ops.h
>>   delete mode 100644 drivers/cpuidle/Kconfig.arm64
>>   rename drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} (72%)
>>
>
>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

Re: [PATCH 3/6] ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function

[Daniel Lezcano]

On 03/13/2015 07:21 PM, Catalin Marinas wrote:
> On Tue, Mar 03, 2015 at 01:29:34PM +0100, Daniel Lezcano wrote:
>> Call the common ARM/ARM64 'arm_cpuidle_suspend' instead of cpu_suspend function
>> which is specific to ARM64.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   drivers/cpuidle/cpuidle-arm64.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
>> index 39a2c62..0cea244 100644
>> --- a/drivers/cpuidle/cpuidle-arm64.c
>> +++ b/drivers/cpuidle/cpuidle-arm64.c
>> @@ -49,7 +49,7 @@ static int arm64_enter_idle_state(struct cpuidle_device *dev,
>>   		 * call the CPU ops suspend protocol with idle index as a
>>   		 * parameter.
>>   		 */
>> -		ret = cpu_suspend(idx);
>> +		arm_cpuidle_suspend(idx);
>
> Nitpick: why don't we just rename the arm one cpuidle_suspend()?

I don't have a strong opinion on that. Actually, the cpuidle_ prefix is 
used by the arch agnostic code in drivers/cpuidle/cpuidle.c.

If Rafael agrees on changing it to this function name, I am ok also.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[Daniel Lezcano]

On 03/13/2015 07:29 PM, Catalin Marinas wrote:
> On Thu, Mar 12, 2015 at 03:25:34PM +0100, Daniel Lezcano wrote:
>> do you agree with this patchset ?
>
> In principle yes, apart from some function naming and I'm waiting for
> Lorenzo's ack as well. Do you plan to upstream this directly via your
> tree? If yes, I'll look in more detail and give some acks.

If you don't care, I would like to upstream via my tree because there is 
a couple of operations (like sharing a branch) I would like to monitor 
to let people build their driver on top of it: renesas and qcom.

Qcom's cpuidle driver is already implemented and waits for this patchset 
to be upstream (the patchset has been around long enough). And I believe 
the Renesas driver will be reworked to use it also.

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[Daniel Lezcano]

On 03/13/2015 09:51 PM, Rob Herring wrote:
> On Thu, Mar 12, 2015 at 9:25 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> Catalin, Rob,
>>
>> do you agree with this patchset ?
>
> There's very little to do with DT, but looks fine to me.

Shall I consider as a acked-by for the entire patchset or only the DT part ?

Thanks for taking the time to look at the patches.

   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/6] ARM64: cpuidle: Rename cpu_init_idle to a common function name

[Catalin Marinas]

On Fri, Mar 13, 2015 at 06:22:46PM +0000, Catalin Marinas wrote:
> On Tue, Mar 03, 2015 at 01:29:35PM +0100, Daniel Lezcano wrote:
> > diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
> > index 0cea244..6ef291c7 100644
> > --- a/drivers/cpuidle/cpuidle-arm64.c
> > +++ b/drivers/cpuidle/cpuidle-arm64.c
> > @@ -110,7 +110,7 @@ static int __init arm64_idle_init(void)
> >  	 * idle states suspend back-end specific data
> >  	 */
> >  	for_each_possible_cpu(cpu) {
> > -		ret = cpu_init_idle(cpu);
> > +		ret = arm_cpuidle_init(cpu);
> 
> Same nitpick here about dropping the arm_ prefix (though here we already
> have a cpuidle_init).

Actually, a question, probably for Lorenzo - why do we need to call
cpu_init_idle() from the driver? Is there any dependency on what the
driver had done before this call? If not, I suggest a core_initcall() in
the arch code for cpu_init_idle(). At a quick look through the code, the
back-end can be initialised on its own.

-- 
Catalin

Re: [PATCH 4/6] ARM64: cpuidle: Rename cpu_init_idle to a common function name

[Lorenzo Pieralisi]

On Sat, Mar 14, 2015 at 11:41:09AM +0000, Catalin Marinas wrote:
> On Fri, Mar 13, 2015 at 06:22:46PM +0000, Catalin Marinas wrote:
> > On Tue, Mar 03, 2015 at 01:29:35PM +0100, Daniel Lezcano wrote:
> > > diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
> > > index 0cea244..6ef291c7 100644
> > > --- a/drivers/cpuidle/cpuidle-arm64.c
> > > +++ b/drivers/cpuidle/cpuidle-arm64.c
> > > @@ -110,7 +110,7 @@ static int __init arm64_idle_init(void)
> > >  	 * idle states suspend back-end specific data
> > >  	 */
> > >  	for_each_possible_cpu(cpu) {
> > > -		ret = cpu_init_idle(cpu);
> > > +		ret = arm_cpuidle_init(cpu);
> > 
> > Same nitpick here about dropping the arm_ prefix (though here we already
> > have a cpuidle_init).
> 
> Actually, a question, probably for Lorenzo - why do we need to call
> cpu_init_idle() from the driver? Is there any dependency on what the
> driver had done before this call? If not, I suggest a core_initcall() in
> the arch code for cpu_init_idle(). At a quick look through the code, the
> back-end can be initialised on its own.

Because we want to register the driver if and only if both the generic
idle states parsing AND the back-end initialization succeed. If we move
the cpu_init_idle() to a core initcall() we still need a way to probe
if the back end initialization succeeded or not, we do not want to
have a driver initialized with back-end calls that fail.

It is also so, because we must guarantee the idle index mapping between
generic driver and the back end, if either initialization fails, the
driver should not be registered.

At the moment the driver is very similar to the x86 generic driver,
which is a good thing for consistency; by the way x86 driver prefixes
the function names with "intel_", I am not too fussed about the
functions naming scheme, open to suggestions.

Lorenzo

Re: [PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[Lorenzo Pieralisi]

On Fri, Mar 13, 2015 at 08:51:38PM +0000, Rob Herring wrote:
> On Thu, Mar 12, 2015 at 9:25 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > Catalin, Rob,
> >
> > do you agree with this patchset ?
> 
> There's very little to do with DT, but looks fine to me.

There are key points related to DT, that are not necessarily
visible within this set, that I fully support BTW.

(1) enable-method: we do need platforms to define an enable-method
    to match cpuidle ops. Nothing new, but we should keep that in mind.
(2) platform back-ends will match idle states according to idle state
    compatible string (to know what function to execute on idle state
    entry for a given idle state the compatible string is used, in DT
    standard way, to match the compatible string and get corresponding
    enter function and parameters/data). The cpuidle_ops suspend method
    will get an index from the idle driver that through a look-up will be
    translated into a idle entry call with respective parameters that are
    back-end specific. PSCI does that already on ARM64.

If we agree on these two points there is not much to debate anymore,
I will review and provide tags according this week, apart from a couple
of nits on functions naming and cpuidle_ops structures replication
the set seems good to go to me.

Lorenzo

> 
> >
> > Thanks
> >   -- Daniel
> >
> >
> > On 03/03/2015 01:29 PM, Daniel Lezcano wrote:
> >>
> >> There is a big number of cpuidle drivers for the ARM architecture.
> >>
> >> These drivers have been cleaned up and grouped into the drivers/cpuidle
> >> directory to keep track of the changes more easily and ensure the code
> >> is following the same scheme across the drivers.
> >>
> >> That had the benefit of simplifying the code and factor out a lot of
> >> common
> >> parts. Beside that, as the drivers belong to the 'drivers' directory, we
> >> had
> >> to split the arch specific bits and the generic code in order to keep
> >> everything self contained. The platform driver paradigm was used for this
> >> purpose.
> >>
> >> Unfortunately, this approach is now no longer accepted and a different
> >> solution
> >> must be provided to reach the same goal: one example is the Qualcomm
> >> cpuidle
> >> driver upstreaming attempt [1].
> >>
> >> In the meantime, ARM64 developed a generic cpuidle driver based on DT
> >> definition.
> >>
> >> The DT definition provides an 'enable-method' to specify one of the cpu
> >> operations (PSCI, ...).
> >>
> >> This patchset unify this driver with ARM32, using the same DT definition.
> >>
> >> Thanks with this patchset we can use the 'enable-method' to specify a cpu
> >> operations, hence get rid of the platform driver approach and go further
> >> in the
> >> cpuidle driver flexibility via the DT.
> >>
> >> Daniel Lezcano (6):
> >>    ARM: cpuidle: Remove duplicate header inclusion
> >>    ARM: cpuidle: Add a cpuidle ops structure to be used for DT
> >>    ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function
> >>    ARM64: cpuidle: Rename cpu_init_idle to a common function name
> >>    ARM64: cpuidle: Remove arm64 reference
> >>    ARM: cpuidle: Enable the ARM64 driver for both ARM32/ARM64
> >>
> >>   arch/arm/include/asm/cpuidle.h                     | 12 +++
> >>   arch/arm/include/asm/cpuidle_ops.h                 |  3 +
> >>   arch/arm/kernel/cpuidle.c                          | 87
> >> +++++++++++++++++++++-
> >>   arch/arm/mach-davinci/cpuidle.c                    |  1 -
> >>   arch/arm/mach-imx/cpuidle-imx6q.c                  |  1 -
> >>   arch/arm/mach-imx/cpuidle-imx6sl.c                 |  1 -
> >>   arch/arm/mach-imx/cpuidle-imx6sx.c                 |  1 -
> >>   arch/arm/mach-omap2/cpuidle44xx.c                  |  1 -
> >>   arch/arm/mach-tegra/cpuidle-tegra20.c              |  1 -
> >>   arch/arm/mach-tegra/cpuidle-tegra30.c              |  1 -
> >>   arch/arm64/include/asm/cpuidle.h                   |  9 ++-
> >>   arch/arm64/kernel/cpuidle.c                        |  2 +-
> >>   drivers/cpuidle/Kconfig                            |  7 +-
> >>   drivers/cpuidle/Kconfig.arm                        | 10 +++
> >>   drivers/cpuidle/Kconfig.arm64                      | 13 ----
> >>   drivers/cpuidle/Makefile                           |  5 +-
> >>   drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} | 38 +++++-----
> >>   drivers/cpuidle/cpuidle-at91.c                     |  1 -
> >>   drivers/cpuidle/cpuidle-exynos.c                   |  1 -
> >>   drivers/cpuidle/cpuidle-kirkwood.c                 |  1 -
> >>   drivers/cpuidle/cpuidle-ux500.c                    |  1 -
> >>   drivers/cpuidle/cpuidle-zynq.c                     |  1 -
> >>   22 files changed, 139 insertions(+), 59 deletions(-)
> >>   create mode 100644 arch/arm/include/asm/cpuidle_ops.h
> >>   delete mode 100644 drivers/cpuidle/Kconfig.arm64
> >>   rename drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} (72%)
> >>
> >
> >
> > --
> >  <http://www.linaro.org/> Linaro.org | Open source software for ARM SoCs
> >
> > Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> > <http://twitter.com/#!/linaroorg> Twitter |
> > <http://www.linaro.org/linaro-blog/> Blog
> >
> 

Re: [PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT

[Lorenzo Pieralisi]

On Tue, Mar 03, 2015 at 12:29:33PM +0000, Daniel Lezcano wrote:
> The current state of the different cpuidle drivers is the different PM

Nit: "The current state of cpuidle drivers is such that different ..."

> operations are passed via the platform_data using the platform driver
> paradigm.
> 
> This approach allowed to split the low level PM code from the arch specific
> and the generic cpuidle code.
> 
> Unfortunately there are complains about this approach as, in the context of the

Nit: s/complains/complaints

> single kernel image, we have multiple drivers loaded in memory for nothing and
> the platform driver is not adequate for cpuidle.
> 
> This patch provides a common interface via cpuidle ops for all new cpuidle
> driver and a definition for the device tree.
> 
> It will allow with the next patches to a have a common definition with ARM64
> and share the same cpuidle driver.
> 
> The code is optimized to use the __init section intensively in order to reduce
> the memory footprint after the driver is initialized and unify the function
> names with ARM64.
> 
> In order to prevent multiple declarations and the specific cpuidle ops to be
> spread across the different headers, a mechanism, similar to the cgroup subsys,
> has been introduced.
> 
> A new platform willing to add its cpuidle ops must add an entry in the file
> cpuidle_ops.h in the current form:
> 
>  #if IS_ENABLED(CONFIG_ARM_FOO_CPUIDLE)
>  CPUIDLE_OPS(foo)
>  #endif
> 
> ... and use the variable name in the specific low level code:
> 
> struct cpuidle_ops foo_cpuidle_ops;
> 
> The CPUIDLE_OPS macro will be processed in different way in the cpuidle.c file,
> thus allowing to keep untouched the arm cpuidle core code in the future when
> a new platform is added.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/include/asm/cpuidle.h     | 10 +++++
>  arch/arm/include/asm/cpuidle_ops.h |  3 ++
>  arch/arm/kernel/cpuidle.c          | 85 ++++++++++++++++++++++++++++++++++++++
>  arch/arm64/include/asm/cpuidle.h   |  5 ++-
>  4 files changed, 102 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/include/asm/cpuidle_ops.h
> 
> diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> index 348dc81..3d31459 100644
> --- a/arch/arm/include/asm/cpuidle.h
> +++ b/arch/arm/include/asm/cpuidle.h
> @@ -27,4 +27,14 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>   */
>  #define ARM_CPUIDLE_WFI_STATE ARM_CPUIDLE_WFI_STATE_PWR(UINT_MAX)
>  
> +struct cpuidle_ops {
> +	const char *name;
> +	int (*suspend)(int cpu, unsigned long arg);
> +	int (*init)(struct device_node *, int cpu);
> +};
> +
> +extern int arm_cpuidle_suspend(int index);
> +
> +extern int arm_cpuidle_init(int cpu);

idle_cpu_suspend()
idle_cpu_init()

?

I am really not fussed about the naming.

To make this and x86 driver name compliant (well, function signatures
are a bit different) we could use:

arm_idle()
arm_idle_cpu_init()

even though I think the arch prefix is useless.

Side note: why is the x86 driver in drivers/idle ? To have another dir :) ?

> +
>  #endif
> diff --git a/arch/arm/include/asm/cpuidle_ops.h b/arch/arm/include/asm/cpuidle_ops.h
> new file mode 100644
> index 0000000..be0a612
> --- /dev/null
> +++ b/arch/arm/include/asm/cpuidle_ops.h
> @@ -0,0 +1,3 @@
> +/*
> + * List of cpuidle operations
> + */
> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> index 45969f8..25e9789c 100644
> --- a/arch/arm/kernel/cpuidle.c
> +++ b/arch/arm/kernel/cpuidle.c
> @@ -10,8 +10,29 @@
>   */
>  
>  #include <linux/cpuidle.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <asm/cpuidle.h>
>  
> +#define CPUIDLE_OPS(__x) extern struct cpuidle_ops __x ## _cpuidle_ops;
> +#include <asm/cpuidle_ops.h>
> +#undef CPUIDLE_OPS
> +
> +#define CPUIDLE_OPS(__x) __x ## _cpuidle_ops_id,
> +enum cpuidle_ops_id {
> +#include <asm/cpuidle_ops.h>
> +        CPUIDLE_OPS_COUNT,
> +};
> +#undef CPUIDLE_OPS
> +
> +#define CPUIDLE_OPS(__x) [__x ## _cpuidle_ops_id ] = &__x ## _cpuidle_ops,
> +static struct cpuidle_ops *supported_cpuidle_ops[] __initconst = {
> +#include <asm/cpuidle_ops.h>
> +};
> +#undef CPUIDLE_OPS
> +
> +static struct cpuidle_ops cpuidle_ops[NR_CPUS];

That's because you want platform cpuidle_ops to be __initdata ?

It should not be a big overhead on arm32 to have a number of
structs equal to NR_CPUS, on arm64 it is the other way around
there are few cpu_ops, but number of CPUs can be high so it
is an array of pointers.

I think it is ok to leave it as it is (or probably make cpuidle_ops
a single struct, I expect enable-method to be common across cpus).

> +
>  int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>  		struct cpuidle_driver *drv, int index)
>  {
> @@ -19,3 +40,67 @@ int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>  
>  	return index;
>  }
> +
> +int arm_cpuidle_suspend(int index)
> +{
> +	int ret = -EOPNOTSUPP;
> +	int cpu = smp_processor_id();
> +
> +	if (cpuidle_ops[cpu].suspend)
> +		ret = cpuidle_ops[cpu].suspend(cpu, index);
> +
> +	return ret;
> +}
> +
> +static struct cpuidle_ops *__init arm_cpuidle_get_ops(const char *name)
> +{
> +	int i;
> +
> +	for (i = 0; i < CPUIDLE_OPS_COUNT; i++) {
> +		if (!strcmp(name, supported_cpuidle_ops[i]->name))
> +			return supported_cpuidle_ops[i];
> +	}
> +
> +	return NULL;
> +}
> +
> +static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
> +{
> +	const char *enable_method;
> +	struct cpuidle_ops *ops;
> +
> +	enable_method = of_get_property(dn, "enable-method", NULL);
> +	if (!enable_method)
> +		return -ENOENT;
> +
> +	ops = arm_cpuidle_get_ops(enable_method);
> +	if (!ops) {
> +		pr_warn("%s: unsupported enable-method property: %s\n",
> +			dn->full_name, enable_method);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	cpuidle_ops[cpu] = *ops; /* structure copy */

See above.

> +
> +	pr_notice("cpuidle: enable-method property '%s'"
> +		  " found operations\n", ops->name);
> +
> +	return 0;
> +}
> +
> +int __init arm_cpuidle_init(int cpu)
> +{
> +	int ret = -EOPNOTSUPP;

Nit: You always assign ret, so there is no point in initializing it.

Lorenzo

> +	struct device_node *cpu_node = of_cpu_device_node_get(cpu);
> +
> +	if (!cpu_node)
> +		return -ENODEV;
> +
> +	ret = arm_cpuidle_read_ops(cpu_node, cpu);
> +	if (!ret && cpuidle_ops[cpu].init)
> +		ret = cpuidle_ops[cpu].init(cpu_node, cpu);
> +
> +	of_node_put(cpu_node);
> +
> +	return ret;
> +}
> diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
> index 0710654..1bee287 100644
> --- a/arch/arm64/include/asm/cpuidle.h
> +++ b/arch/arm64/include/asm/cpuidle.h
> @@ -15,5 +15,8 @@ static inline int cpu_suspend(unsigned long arg)
>  	return -EOPNOTSUPP;
>  }
>  #endif
> -
> +static inline int arm_cpuidle_suspend(int index)
> +{
> +	return cpu_suspend(index);
> +}
>  #endif
> -- 
> 1.9.1
> 
> 

Re: [PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT

[Stephen Boyd]

On 03/03/15 04:29, Daniel Lezcano wrote:
>
> The code is optimized to use the __init section intensively in order to reduce
> the memory footprint after the driver is initialized and unify the function
> names with ARM64.
>
> In order to prevent multiple declarations and the specific cpuidle ops to be
> spread across the different headers, a mechanism, similar to the cgroup subsys,
> has been introduced.
>
> A new platform willing to add its cpuidle ops must add an entry in the file
> cpuidle_ops.h in the current form:
>
>  #if IS_ENABLED(CONFIG_ARM_FOO_CPUIDLE)
>  CPUIDLE_OPS(foo)
>  #endif
>
> ... and use the variable name in the specific low level code:
>
> struct cpuidle_ops foo_cpuidle_ops;
>
> The CPUIDLE_OPS macro will be processed in different way in the cpuidle.c file,
> thus allowing to keep untouched the arm cpuidle core code in the future when
> a new platform is added.
[...]
> diff --git a/arch/arm/include/asm/cpuidle_ops.h b/arch/arm/include/asm/cpuidle_ops.h
> new file mode 100644
> index 0000000..be0a612
> --- /dev/null
> +++ b/arch/arm/include/asm/cpuidle_ops.h
> @@ -0,0 +1,3 @@
> +/*
> + * List of cpuidle operations
> + */
> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> index 45969f8..25e9789c 100644
> --- a/arch/arm/kernel/cpuidle.c
> +++ b/arch/arm/kernel/cpuidle.c
> @@ -10,8 +10,29 @@
>   */
>  
>  #include <linux/cpuidle.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <asm/cpuidle.h>
>  
> +#define CPUIDLE_OPS(__x) extern struct cpuidle_ops __x ## _cpuidle_ops;
> +#include <asm/cpuidle_ops.h>
> +#undef CPUIDLE_OPS
> +
> +#define CPUIDLE_OPS(__x) __x ## _cpuidle_ops_id,
> +enum cpuidle_ops_id {
> +#include <asm/cpuidle_ops.h>
> +        CPUIDLE_OPS_COUNT,
> +};
> +#undef CPUIDLE_OPS
> +
> +#define CPUIDLE_OPS(__x) [__x ## _cpuidle_ops_id ] = &__x ## _cpuidle_ops,
> +static struct cpuidle_ops *supported_cpuidle_ops[] __initconst = {
> +#include <asm/cpuidle_ops.h>
> +};
> +#undef CPUIDLE_OPS
> +
> +static struct cpuidle_ops cpuidle_ops[NR_CPUS];

Is there any reason why we aren't putting these structures into a linker
section like we do for the smp operations structures?

The nice thing about using the linker is it makes it clearer at the
location where we define the structure that it's actually used by
something. Right now the structures are defined non-static in a file and
then we have to know that a CPUIDLE_OPS() define has been made in
another architecture specific asm header file so that this macro magic
works. The commit text says something about multiple declarations and
ops spread across header files, which shouldn't apply if we're using the
linker to find these ops and merge them into an array we can iterate over.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT

[Daniel Lezcano]

On 03/16/2015 07:16 PM, Lorenzo Pieralisi wrote:
> On Tue, Mar 03, 2015 at 12:29:33PM +0000, Daniel Lezcano wrote:
>> The current state of the different cpuidle drivers is the different PM
>
> Nit: "The current state of cpuidle drivers is such that different ..."

Ok.

>> operations are passed via the platform_data using the platform driver
>> paradigm.
>>
>> This approach allowed to split the low level PM code from the arch specific
>> and the generic cpuidle code.
>>
>> Unfortunately there are complains about this approach as, in the context of the
>
> Nit: s/complains/complaints

Ok.

[ ... ]

>> @@ -27,4 +27,14 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>>    */
>>   #define ARM_CPUIDLE_WFI_STATE ARM_CPUIDLE_WFI_STATE_PWR(UINT_MAX)
>>
>> +struct cpuidle_ops {
>> +	const char *name;
>> +	int (*suspend)(int cpu, unsigned long arg);
>> +	int (*init)(struct device_node *, int cpu);
>> +};
>> +
>> +extern int arm_cpuidle_suspend(int index);
>> +
>> +extern int arm_cpuidle_init(int cpu);
>
> idle_cpu_suspend()
> idle_cpu_init()
>
> ?
>
> I am really not fussed about the naming.
>
> To make this and x86 driver name compliant (well, function signatures
> are a bit different) we could use:
>
> arm_idle()
> arm_idle_cpu_init()
>
> even though I think the arch prefix is useless.
>
> Side note: why is the x86 driver in drivers/idle ? To have another dir :) ?

I believe it is there for historical reasons.

[ ... ]

>> +static struct cpuidle_ops cpuidle_ops[NR_CPUS];
>
> That's because you want platform cpuidle_ops to be __initdata ?

Yes.

> It should not be a big overhead on arm32 to have a number of
> structs equal to NR_CPUS, on arm64 it is the other way around
> there are few cpu_ops, but number of CPUs can be high so it
> is an array of pointers.
>
> I think it is ok to leave it as it is (or probably make cpuidle_ops
> a single struct, I expect enable-method to be common across cpus).

I prefer to keep per cpu because I am not sure of this assumption.

[ ... ]

>> +	cpuidle_ops[cpu] = *ops; /* structure copy */
>
> See above.
>
>> +
>> +	pr_notice("cpuidle: enable-method property '%s'"
>> +		  " found operations\n", ops->name);
>> +
>> +	return 0;
>> +}
>> +
>> +int __init arm_cpuidle_init(int cpu)
>> +{
>> +	int ret = -EOPNOTSUPP;
>
> Nit: You always assign ret, so there is no point in initializing it.

Ok, I will fix it.

Thanks for reviewing.

   -- Daniel



-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT

[Lorenzo Pieralisi]

On Mon, Mar 16, 2015 at 10:08:19PM +0000, Stephen Boyd wrote:
> On 03/03/15 04:29, Daniel Lezcano wrote:
> >
> > The code is optimized to use the __init section intensively in order to reduce
> > the memory footprint after the driver is initialized and unify the function
> > names with ARM64.
> >
> > In order to prevent multiple declarations and the specific cpuidle ops to be
> > spread across the different headers, a mechanism, similar to the cgroup subsys,
> > has been introduced.
> >
> > A new platform willing to add its cpuidle ops must add an entry in the file
> > cpuidle_ops.h in the current form:
> >
> >  #if IS_ENABLED(CONFIG_ARM_FOO_CPUIDLE)
> >  CPUIDLE_OPS(foo)
> >  #endif
> >
> > ... and use the variable name in the specific low level code:
> >
> > struct cpuidle_ops foo_cpuidle_ops;
> >
> > The CPUIDLE_OPS macro will be processed in different way in the cpuidle.c file,
> > thus allowing to keep untouched the arm cpuidle core code in the future when
> > a new platform is added.
> [...]
> > diff --git a/arch/arm/include/asm/cpuidle_ops.h b/arch/arm/include/asm/cpuidle_ops.h
> > new file mode 100644
> > index 0000000..be0a612
> > --- /dev/null
> > +++ b/arch/arm/include/asm/cpuidle_ops.h
> > @@ -0,0 +1,3 @@
> > +/*
> > + * List of cpuidle operations
> > + */
> > diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> > index 45969f8..25e9789c 100644
> > --- a/arch/arm/kernel/cpuidle.c
> > +++ b/arch/arm/kernel/cpuidle.c
> > @@ -10,8 +10,29 @@
> >   */
> >  
> >  #include <linux/cpuidle.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <asm/cpuidle.h>
> >  
> > +#define CPUIDLE_OPS(__x) extern struct cpuidle_ops __x ## _cpuidle_ops;
> > +#include <asm/cpuidle_ops.h>
> > +#undef CPUIDLE_OPS
> > +
> > +#define CPUIDLE_OPS(__x) __x ## _cpuidle_ops_id,
> > +enum cpuidle_ops_id {
> > +#include <asm/cpuidle_ops.h>
> > +        CPUIDLE_OPS_COUNT,
> > +};
> > +#undef CPUIDLE_OPS
> > +
> > +#define CPUIDLE_OPS(__x) [__x ## _cpuidle_ops_id ] = &__x ## _cpuidle_ops,
> > +static struct cpuidle_ops *supported_cpuidle_ops[] __initconst = {
> > +#include <asm/cpuidle_ops.h>
> > +};
> > +#undef CPUIDLE_OPS
> > +
> > +static struct cpuidle_ops cpuidle_ops[NR_CPUS];
> 
> Is there any reason why we aren't putting these structures into a linker
> section like we do for the smp operations structures?

I think it can be done with an OF_TABLE, it is a bit of shame cpuidle_ops
should work on UP too otherwise they could have been merged in
smp_ops to create cpu_ops, like arm64 does.

> The nice thing about using the linker is it makes it clearer at the
> location where we define the structure that it's actually used by
> something. Right now the structures are defined non-static in a file and
> then we have to know that a CPUIDLE_OPS() define has been made in
> another architecture specific asm header file so that this macro magic
> works. The commit text says something about multiple declarations and
> ops spread across header files, which shouldn't apply if we're using the
> linker to find these ops and merge them into an array we can iterate over.

It makes sense, see above for UP vs SMP. I wonder if we can't find
something to overcome the UP limitation nicely, the init code in
arch/arm/kernel/devtree.c is identical for smp_ops and cpuidle_ops,
apart from the CONFIG_SMP ifdeffery.

Lorenzo

Re: [PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT

[Stephen Boyd]

On 03/17/15 04:29, Lorenzo Pieralisi wrote:
> On Mon, Mar 16, 2015 at 10:08:19PM +0000, Stephen Boyd wrote:
>> On 03/03/15 04:29, Daniel Lezcano wrote:
>>> The code is optimized to use the __init section intensively in order to reduce
>>> the memory footprint after the driver is initialized and unify the function
>>> names with ARM64.
>>>
>>> In order to prevent multiple declarations and the specific cpuidle ops to be
>>> spread across the different headers, a mechanism, similar to the cgroup subsys,
>>> has been introduced.
>>>
>>> A new platform willing to add its cpuidle ops must add an entry in the file
>>> cpuidle_ops.h in the current form:
>>>
>>>  #if IS_ENABLED(CONFIG_ARM_FOO_CPUIDLE)
>>>  CPUIDLE_OPS(foo)
>>>  #endif
>>>
>>> ... and use the variable name in the specific low level code:
>>>
>>> struct cpuidle_ops foo_cpuidle_ops;
>>>
>>> The CPUIDLE_OPS macro will be processed in different way in the cpuidle.c file,
>>> thus allowing to keep untouched the arm cpuidle core code in the future when
>>> a new platform is added.
>> [...]
>>> diff --git a/arch/arm/include/asm/cpuidle_ops.h b/arch/arm/include/asm/cpuidle_ops.h
>>> new file mode 100644
>>> index 0000000..be0a612
>>> --- /dev/null
>>> +++ b/arch/arm/include/asm/cpuidle_ops.h
>>> @@ -0,0 +1,3 @@
>>> +/*
>>> + * List of cpuidle operations
>>> + */
>>> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
>>> index 45969f8..25e9789c 100644
>>> --- a/arch/arm/kernel/cpuidle.c
>>> +++ b/arch/arm/kernel/cpuidle.c
>>> @@ -10,8 +10,29 @@
>>>   */
>>>  
>>>  #include <linux/cpuidle.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>>  #include <asm/cpuidle.h>
>>>  
>>> +#define CPUIDLE_OPS(__x) extern struct cpuidle_ops __x ## _cpuidle_ops;
>>> +#include <asm/cpuidle_ops.h>
>>> +#undef CPUIDLE_OPS
>>> +
>>> +#define CPUIDLE_OPS(__x) __x ## _cpuidle_ops_id,
>>> +enum cpuidle_ops_id {
>>> +#include <asm/cpuidle_ops.h>
>>> +        CPUIDLE_OPS_COUNT,
>>> +};
>>> +#undef CPUIDLE_OPS
>>> +
>>> +#define CPUIDLE_OPS(__x) [__x ## _cpuidle_ops_id ] = &__x ## _cpuidle_ops,
>>> +static struct cpuidle_ops *supported_cpuidle_ops[] __initconst = {
>>> +#include <asm/cpuidle_ops.h>
>>> +};
>>> +#undef CPUIDLE_OPS
>>> +
>>> +static struct cpuidle_ops cpuidle_ops[NR_CPUS];
>> Is there any reason why we aren't putting these structures into a linker
>> section like we do for the smp operations structures?
> I think it can be done with an OF_TABLE, it is a bit of shame cpuidle_ops
> should work on UP too otherwise they could have been merged in
> smp_ops to create cpu_ops, like arm64 does.

We should merge the two and remove the SMP dependency on arm32.

>> The nice thing about using the linker is it makes it clearer at the
>> location where we define the structure that it's actually used by
>> something. Right now the structures are defined non-static in a file and
>> then we have to know that a CPUIDLE_OPS() define has been made in
>> another architecture specific asm header file so that this macro magic
>> works. The commit text says something about multiple declarations and
>> ops spread across header files, which shouldn't apply if we're using the
>> linker to find these ops and merge them into an array we can iterate over.
> It makes sense, see above for UP vs SMP. I wonder if we can't find
> something to overcome the UP limitation nicely, the init code in
> arch/arm/kernel/devtree.c is identical for smp_ops and cpuidle_ops,
> apart from the CONFIG_SMP ifdeffery.

It should be possible to replace the arm32 smp_operations structure with
something like the arm64 cpu_operations structure. Yes we would have to
drop the SMP dependency, but that will be ok. It would require some work
to make arm32 and arm64 the same, but for these purposes that isn't
really required as long as we can put the cpu idle hook there.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT

[Daniel Lezcano]

On 03/18/2015 02:14 AM, Stephen Boyd wrote:
> On 03/17/15 04:29, Lorenzo Pieralisi wrote:
>> On Mon, Mar 16, 2015 at 10:08:19PM +0000, Stephen Boyd wrote:
>>> On 03/03/15 04:29, Daniel Lezcano wrote:
>>>> The code is optimized to use the __init section intensively in order to reduce
>>>> the memory footprint after the driver is initialized and unify the function
>>>> names with ARM64.
>>>>
>>>> In order to prevent multiple declarations and the specific cpuidle ops to be
>>>> spread across the different headers, a mechanism, similar to the cgroup subsys,
>>>> has been introduced.
>>>>
>>>> A new platform willing to add its cpuidle ops must add an entry in the file
>>>> cpuidle_ops.h in the current form:
>>>>
>>>>   #if IS_ENABLED(CONFIG_ARM_FOO_CPUIDLE)
>>>>   CPUIDLE_OPS(foo)
>>>>   #endif
>>>>
>>>> ... and use the variable name in the specific low level code:
>>>>
>>>> struct cpuidle_ops foo_cpuidle_ops;
>>>>
>>>> The CPUIDLE_OPS macro will be processed in different way in the cpuidle.c file,
>>>> thus allowing to keep untouched the arm cpuidle core code in the future when
>>>> a new platform is added.
>>> [...]
>>>> diff --git a/arch/arm/include/asm/cpuidle_ops.h b/arch/arm/include/asm/cpuidle_ops.h
>>>> new file mode 100644
>>>> index 0000000..be0a612
>>>> --- /dev/null
>>>> +++ b/arch/arm/include/asm/cpuidle_ops.h
>>>> @@ -0,0 +1,3 @@
>>>> +/*
>>>> + * List of cpuidle operations
>>>> + */
>>>> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
>>>> index 45969f8..25e9789c 100644
>>>> --- a/arch/arm/kernel/cpuidle.c
>>>> +++ b/arch/arm/kernel/cpuidle.c
>>>> @@ -10,8 +10,29 @@
>>>>    */
>>>>
>>>>   #include <linux/cpuidle.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>>   #include <asm/cpuidle.h>
>>>>
>>>> +#define CPUIDLE_OPS(__x) extern struct cpuidle_ops __x ## _cpuidle_ops;
>>>> +#include <asm/cpuidle_ops.h>
>>>> +#undef CPUIDLE_OPS
>>>> +
>>>> +#define CPUIDLE_OPS(__x) __x ## _cpuidle_ops_id,
>>>> +enum cpuidle_ops_id {
>>>> +#include <asm/cpuidle_ops.h>
>>>> +        CPUIDLE_OPS_COUNT,
>>>> +};
>>>> +#undef CPUIDLE_OPS
>>>> +
>>>> +#define CPUIDLE_OPS(__x) [__x ## _cpuidle_ops_id ] = &__x ## _cpuidle_ops,
>>>> +static struct cpuidle_ops *supported_cpuidle_ops[] __initconst = {
>>>> +#include <asm/cpuidle_ops.h>
>>>> +};
>>>> +#undef CPUIDLE_OPS
>>>> +
>>>> +static struct cpuidle_ops cpuidle_ops[NR_CPUS];
>>> Is there any reason why we aren't putting these structures into a linker
>>> section like we do for the smp operations structures?
>> I think it can be done with an OF_TABLE, it is a bit of shame cpuidle_ops
>> should work on UP too otherwise they could have been merged in
>> smp_ops to create cpu_ops, like arm64 does.
>
> We should merge the two and remove the SMP dependency on arm32.

I will be happy to do that but right now it would be nice to keep 
focused on bringing the cpuidle ops first, even if we have a bit of code 
duplicated, in order to unblock the cpuidle drivers awaiting for this 
code to be merged.

>>> The nice thing about using the linker is it makes it clearer at the
>>> location where we define the structure that it's actually used by
>>> something. Right now the structures are defined non-static in a file and
>>> then we have to know that a CPUIDLE_OPS() define has been made in
>>> another architecture specific asm header file so that this macro magic
>>> works. The commit text says something about multiple declarations and
>>> ops spread across header files, which shouldn't apply if we're using the
>>> linker to find these ops and merge them into an array we can iterate over.
>> It makes sense, see above for UP vs SMP. I wonder if we can't find
>> something to overcome the UP limitation nicely, the init code in
>> arch/arm/kernel/devtree.c is identical for smp_ops and cpuidle_ops,
>> apart from the CONFIG_SMP ifdeffery.
>
> It should be possible to replace the arm32 smp_operations structure with
> something like the arm64 cpu_operations structure. Yes we would have to
> drop the SMP dependency, but that will be ok. It would require some work
> to make arm32 and arm64 the same, but for these purposes that isn't
> really required as long as we can put the cpu idle hook there.
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/6] ARM64: cpuidle: Rename cpu_init_idle to a common function name

[Daniel Lezcano]

On 03/03/2015 01:29 PM, Daniel Lezcano wrote:
> With this change the cpuidle-arm64.c file calls the same function name
> for both ARM and ARM64.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Hi Catalin,

do you mind to ack the ARM64 if you agree with these changes ?

Thanks.

   -- Daniel

> ---
>   arch/arm64/include/asm/cpuidle.h | 4 ++--
>   arch/arm64/kernel/cpuidle.c      | 2 +-
>   drivers/cpuidle/cpuidle-arm64.c  | 2 +-
>   3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
> index 1bee287..2bc2732 100644
> --- a/arch/arm64/include/asm/cpuidle.h
> +++ b/arch/arm64/include/asm/cpuidle.h
> @@ -2,10 +2,10 @@
>   #define __ASM_CPUIDLE_H
>
>   #ifdef CONFIG_CPU_IDLE
> -extern int cpu_init_idle(unsigned int cpu);
> +extern int arm_cpuidle_init(unsigned int cpu);
>   extern int cpu_suspend(unsigned long arg);
>   #else
> -static inline int cpu_init_idle(unsigned int cpu)
> +static inline int arm_cpuidle_init(unsigned int cpu)
>   {
>   	return -EOPNOTSUPP;
>   }
> diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
> index 5c08966..a78143a 100644
> --- a/arch/arm64/kernel/cpuidle.c
> +++ b/arch/arm64/kernel/cpuidle.c
> @@ -15,7 +15,7 @@
>   #include <asm/cpuidle.h>
>   #include <asm/cpu_ops.h>
>
> -int cpu_init_idle(unsigned int cpu)
> +int arm_cpuidle_init(unsigned int cpu)
>   {
>   	int ret = -EOPNOTSUPP;
>   	struct device_node *cpu_node = of_cpu_device_node_get(cpu);
> diff --git a/drivers/cpuidle/cpuidle-arm64.c b/drivers/cpuidle/cpuidle-arm64.c
> index 0cea244..6ef291c7 100644
> --- a/drivers/cpuidle/cpuidle-arm64.c
> +++ b/drivers/cpuidle/cpuidle-arm64.c
> @@ -110,7 +110,7 @@ static int __init arm64_idle_init(void)
>   	 * idle states suspend back-end specific data
>   	 */
>   	for_each_possible_cpu(cpu) {
> -		ret = cpu_init_idle(cpu);
> +		ret = arm_cpuidle_init(cpu);
>   		if (ret) {
>   			pr_err("CPU %d failed to init idle CPU ops\n", cpu);
>   			return ret;
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT

[Catalin Marinas]

On Tue, Mar 03, 2015 at 01:29:33PM +0100, Daniel Lezcano wrote:
> diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
> index 0710654..1bee287 100644
> --- a/arch/arm64/include/asm/cpuidle.h
> +++ b/arch/arm64/include/asm/cpuidle.h
> @@ -15,5 +15,8 @@ static inline int cpu_suspend(unsigned long arg)
>  	return -EOPNOTSUPP;
>  }
>  #endif
> -
> +static inline int arm_cpuidle_suspend(int index)
> +{
> +	return cpu_suspend(index);
> +}
>  #endif

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH 4/6] ARM64: cpuidle: Rename cpu_init_idle to a common function name

[Catalin Marinas]

On Tue, Mar 03, 2015 at 01:29:35PM +0100, Daniel Lezcano wrote:
> With this change the cpuidle-arm64.c file calls the same function name
> for both ARM and ARM64.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

That said, if you find some names to avoid the arm_ prefix it's even
better but either way my ack still stands.