CPU idle for Armada XP

Add the wfi, cpu idle and cpu deep idle power states support for the Armada XP SoCs.

All the latencies and the power consumption values used at the "armada_370_xp_idle_driver" structure are preliminary and will be modified in the future after running some measurements and analysis.

Based on the work of Nadav Haklai.

Signed-off-by Nadav Haklai <[email protected]>

Signed-off-by Gregory CLEMENT <[email protected]>

---
 drivers/cpuidle/Kconfig.arm             |   5 ++
 drivers/cpuidle/Makefile                |   1 +
 drivers/cpuidle/cpuidle-armada-370-xp.c | 103 ++++++++++++++++++++++++++++++++
 drivers/cpuidle/suspend-armada-370-xp.S |  91 ++++++++++++++++++++++++++++

 4 files changed, 200 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c
 create mode 100644 drivers/cpuidle/suspend-armada-370-xp.S

drivers/cpuidle/Kconfig.arm +5 -0

index 8e36603..071d960 100644
@@ -1,6 +1,11 @@
 #
 # ARM CPU Idle drivers
 #
+config ARM_ARMADA_370_XP_CPUIDLE
+	bool "CPU Idle Driver for Armada 370/XP family processors"
+	depends on ARCH_MVEBU
+	help
+	  Select this to enable cpuidle on Armada 370/XP processors.
 
 config ARM_HIGHBANK_CPUIDLE
 	bool "CPU Idle Driver for Calxeda processors"

drivers/cpuidle/Makefile +1 -0

index cea5ef5..90fc4a6 100644
@@ -7,6 +7,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
 
 ##################################################################################
 # ARM SoC drivers
+obj-$(CONFIG_ARM_ARMADA_370_XP_CPUIDLE) += cpuidle-armada-370-xp.o suspend-armada-370-xp.o
 obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
 obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
 obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o

drivers/cpuidle/cpuidle-armada-370-xp.c new file +41 -0

new file mode 100644
index 0000000..7c78d92
@@ -0,0 +1,103 @@
+/*
+ * Marvell Armada 370 and Armada XP SoC cpuidle driver
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * Nadav Haklai <[email protected]>
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * Maintainer: Gregory CLEMENT <[email protected]>
+ */
+
+#include <linux/cpuidle.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/suspend.h>
+#include <asm/suspend.h>
+#include <linux/smp.h>
+#include <asm/cpuidle.h>
+#include <asm/smp_plat.h>
+#include <linux/armada-370-xp-pmsu.h>
+
+#define ARMADA_370_XP_MAX_STATES	3
+
+enum mv_pm_states {
+	WFI = 0,
+	MV_CPU_IDLE,
+	MV_CPU_DEEP_IDLE,
+};
+
+extern void v7_flush_dcache_all(void);
+
+/* Functions defined in suspend-armada-370-xp.S */
+int armada_370_xp_cpu_resume(unsigned long);
+int armada_370_xp_cpu_suspend(unsigned long);
+
+static int armada_370_xp_enter_idle(struct cpuidle_device *dev,

drivers/cpuidle/cpuidle-armada-370-xp.c new file +22 -0

+	cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
+
+	cpu_init();
+
+	armada_370_xp_pmsu_idle_restore();
+
+	return index;
+}
+
+static struct cpuidle_driver armada_370_xp_idle_driver = {
+	.name			= "armada_370_xp_idle",
+	.states[0]		= ARM_CPUIDLE_WFI_STATE,
+	.states[1]		= {
+		.enter			= armada_370_xp_enter_idle,
+		.exit_latency		= 10,
+		.power_usage		= 50,
+		.target_residency	= 100,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "MV CPU IDLE",
+		.desc			= "CPU power down",
+	},
+	.states[2]		= {

drivers/cpuidle/cpuidle-armada-370-xp.c new file +7 -0

+
+static int __init armada_370_xp_cpuidle_init(void)
+{
+	if (!of_find_compatible_node(NULL, NULL, "marvell,armada-370-xp-pmsu"))
+		return -ENODEV;
+
+	if (!of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"))

drivers/cpuidle/cpuidle-armada-370-xp.c new file +1 -0

+MODULE_LICENSE("GPL");

Quoted 9 lines · Gregory CLEMENT

Add the wfi, cpu idle and cpu deep idle power states support for the Armada XP SoCs.

All the latencies and the power consumption values used at the "armada_370_xp_idle_driver" structure are preliminary and will be modified in the future after running some measurements and analysis.

Based on the work of Nadav Haklai.

Quoted 7 lines

Signed-off-by Nadav Haklai <[email protected]>

Signed-off-by Gregory CLEMENT <[email protected]>

---
 drivers/cpuidle/Kconfig.arm             |   5 ++
 drivers/cpuidle/Makefile                |   1 +
 drivers/cpuidle/cpuidle-armada-370-xp.c | 103 ++++++++++++++++++++++++++++++++
 drivers/cpuidle/suspend-armada-370-xp.S |  91 ++++++++++++++++++++++++++++

Somehow, you will have to move "suspend-armada-370-xp.S" into arch/arm.

Quoted 80 lines

 4 files changed, 200 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c
 create mode 100644 drivers/cpuidle/suspend-armada-370-xp.S

drivers/cpuidle/Kconfig.arm +5 -0

index 8e36603..071d960 100644
@@ -1,6 +1,11 @@
 #
 # ARM CPU Idle drivers
 #
+config ARM_ARMADA_370_XP_CPUIDLE
+	bool "CPU Idle Driver for Armada 370/XP family processors"
+	depends on ARCH_MVEBU
+	help
+	  Select this to enable cpuidle on Armada 370/XP processors.
 
 config ARM_HIGHBANK_CPUIDLE
 	bool "CPU Idle Driver for Calxeda processors"

drivers/cpuidle/Makefile +1 -0

index cea5ef5..90fc4a6 100644
@@ -7,6 +7,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
 
 ##################################################################################
 # ARM SoC drivers
+obj-$(CONFIG_ARM_ARMADA_370_XP_CPUIDLE) += cpuidle-armada-370-xp.o suspend-armada-370-xp.o
 obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
 obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
 obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o

drivers/cpuidle/cpuidle-armada-370-xp.c new file +41 -0

new file mode 100644
index 0000000..7c78d92
@@ -0,0 +1,103 @@
+/*
+ * Marvell Armada 370 and Armada XP SoC cpuidle driver
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * Nadav Haklai <[email protected]>
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * Maintainer: Gregory CLEMENT <[email protected]>
+ */
+
+#include <linux/cpuidle.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/suspend.h>
+#include <asm/suspend.h>
+#include <linux/smp.h>
+#include <asm/cpuidle.h>
+#include <asm/smp_plat.h>
+#include <linux/armada-370-xp-pmsu.h>
+
+#define ARMADA_370_XP_MAX_STATES	3
+
+enum mv_pm_states {
+	WFI = 0,
+	MV_CPU_IDLE,
+	MV_CPU_DEEP_IDLE,
+};
+
+extern void v7_flush_dcache_all(void);
+
+/* Functions defined in suspend-armada-370-xp.S */
+int armada_370_xp_cpu_resume(unsigned long);
+int armada_370_xp_cpu_suspend(unsigned long);
+
+static int armada_370_xp_enter_idle(struct cpuidle_device *dev,

drivers/cpuidle/cpuidle-armada-370-xp.c line 51

Quoted 10 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +10 -0

+				struct cpuidle_driver *drv,
+				int index)
+{
+	bool deepidle = false;
+	unsigned int hw_cpu = cpu_logical_map(smp_processor_id());
+
+	armada_370_xp_pmsu_set_start_addr(armada_370_xp_cpu_resume, hw_cpu);
+
+	if (index == MV_CPU_DEEP_IDLE)
+		deepidle = true;

That looks a bit hackish no ? :)

Can't you use the CPUIDLE_DRIVER_FLAGS_MASK to store this information ?

Quoted 27 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +26 -0

+	cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
+
+	cpu_init();
+
+	armada_370_xp_pmsu_idle_restore();
+
+	return index;
+}
+
+static struct cpuidle_driver armada_370_xp_idle_driver = {
+	.name			= "armada_370_xp_idle",
+	.states[0]		= ARM_CPUIDLE_WFI_STATE,
+	.states[1]		= {
+		.enter			= armada_370_xp_enter_idle,
+		.exit_latency		= 10,
+		.power_usage		= 50,
+		.target_residency	= 100,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "MV CPU IDLE",
+		.desc			= "CPU power down",
+	},
+	.states[2]		= {
+		.enter			= armada_370_xp_enter_idle,
+		.exit_latency		= 100,
+		.power_usage		= 5,
+		.target_residency	= 1000,

drivers/cpuidle/cpuidle-armada-370-xp.c line 84

Quoted 6 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +6 -0

+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "MV CPU DEEP IDLE",
+		.desc			= "CPU and L2 Fabric power down",
+	},
+	.state_count = ARMADA_370_XP_MAX_STATES,
+};

What about the local timers ? Are they shutdown ?

Quoted 13 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +12 -0

+
+static int __init armada_370_xp_cpuidle_init(void)
+{
+	if (!of_find_compatible_node(NULL, NULL, "marvell,armada-370-xp-pmsu"))
+		return -ENODEV;
+
+	if (!of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"))
+		return -ENODEV;
+
+	pr_info("Initializing Armada-XP CPU power management ");
+
+	armada_370_xp_pmsu_enable_l2_powerdown_onidle();

drivers/cpuidle/cpuidle-armada-370-xp.c line 101

Quoted 5 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +5 -0

+
+	return cpuidle_register(&armada_370_xp_idle_driver, NULL);
+}
+
+module_init(armada_370_xp_cpuidle_init);

Isn't it possible to replace it by module_platform_driver ? like ux500 or kirkwood ?

Thanks
-- Daniel

Quoted 1 line

drivers/cpuidle/cpuidle-armada-370-xp.c new file +1 -0

+MODULE_LICENSE("GPL");

[ ... ]

Hi Daniel,

thanks for you review,

Quoted 18 lines · Daniel Lezcano

Quoted 16 lines · Gregory CLEMENT

Add the wfi, cpu idle and cpu deep idle power states support for the Armada XP SoCs.

All the latencies and the power consumption values used at the "armada_370_xp_idle_driver" structure are preliminary and will be modified in the future after running some measurements and analysis.

Based on the work of Nadav Haklai.

Signed-off-by Nadav Haklai <[email protected]>

Signed-off-by Gregory CLEMENT <[email protected]>

---
 drivers/cpuidle/Kconfig.arm             |   5 ++
 drivers/cpuidle/Makefile                |   1 +
 drivers/cpuidle/cpuidle-armada-370-xp.c | 103 ++++++++++++++++++++++++++++++++
 drivers/cpuidle/suspend-armada-370-xp.S |  91 ++++++++++++++++++++++++++++

Quoted 1 line

Somehow, you will have to move "suspend-armada-370-xp.S" into arch/arm.

Does it mean that you want I move it for the next version?

Quoted 17 lines

Quoted 17 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +17 -0

+static struct cpuidle_driver armada_370_xp_idle_driver = {
+	.name			= "armada_370_xp_idle",
+	.states[0]		= ARM_CPUIDLE_WFI_STATE,
+	.states[1]		= {
+		.enter			= armada_370_xp_enter_idle,
+		.exit_latency		= 10,
+		.power_usage		= 50,
+		.target_residency	= 100,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "MV CPU IDLE",
+		.desc			= "CPU power down",
+	},
+	.states[2]		= {
+		.enter			= armada_370_xp_enter_idle,
+		.exit_latency		= 100,
+		.power_usage		= 5,
+		.target_residency	= 1000,

Quoted 8 lines

drivers/cpuidle/cpuidle-armada-370-xp.c line 84

Quoted 6 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +6 -0

+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "MV CPU DEEP IDLE",
+		.desc			= "CPU and L2 Fabric power down",
+	},
+	.state_count = ARMADA_370_XP_MAX_STATES,
+};

What about the local timers ? Are they shutdown ?

I need to chekc it.

Quoted 11 lines

Quoted 11 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +11 -0

+static int __init armada_370_xp_cpuidle_init(void)
+{
+	if (!of_find_compatible_node(NULL, NULL, "marvell,armada-370-xp-pmsu"))
+		return -ENODEV;
+
+	if (!of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"))
+		return -ENODEV;
+
+	pr_info("Initializing Armada-XP CPU power management ");
+
+	armada_370_xp_pmsu_enable_l2_powerdown_onidle();

Quoted 8 lines

drivers/cpuidle/cpuidle-armada-370-xp.c line 101

Quoted 5 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +5 -0

+
+	return cpuidle_register(&armada_370_xp_idle_driver, NULL);
+}
+
+module_init(armada_370_xp_cpuidle_init);

Isn't it possible to replace it by module_platform_driver ? like ux500 or kirkwood ?

It should be possible indeed, I will check it.

Quoted 6 lines

Quoted 4 lines

drivers/cpuidle/cpuidle-armada-370-xp.c line 84

Quoted 2 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +2 -0

+	.state_count = ARMADA_370_XP_MAX_STATES,
+};

What about the local timers ? Are they shutdown ?

I need to chekc it.

Ok, if it is the case, there is the flag CPUIDLE_FLAG_TIMER_STOP to tell the cpuidle framework to switch to the broadcast timer with this state.

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +11 -0

+static int __init armada_370_xp_cpuidle_init(void)
+{
+	if (!of_find_compatible_node(NULL, NULL, "marvell,armada-370-xp-pmsu"))
+		return -ENODEV;
+
+	if (!of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"))
+		return -ENODEV;
+
+	pr_info("Initializing Armada-XP CPU power management ");
+
+	armada_370_xp_pmsu_enable_l2_powerdown_onidle();

Quoted 8 lines

drivers/cpuidle/cpuidle-armada-370-xp.c line 101

Quoted 5 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +5 -0

+
+	return cpuidle_register(&armada_370_xp_idle_driver, NULL);
+}
+
+module_init(armada_370_xp_cpuidle_init);

Isn't it possible to replace it by module_platform_driver ? like ux500 or kirkwood ?

Quoted 1 line

It should be possible indeed, I will check it.

That would be great. It is a nicer approach for the single zImage IMHO.

Thanks !
-- Daniel

drivers/cpuidle/cpuidle-armada-370-xp.c new file +19 -0

+#define ARMADA_370_XP_MAX_STATES       3
+
+enum mv_pm_states {
+       WFI = 0,
+       MV_CPU_IDLE,
+       MV_CPU_DEEP_IDLE,
+};
+
+extern void v7_flush_dcache_all(void);
+
+/* Functions defined in suspend-armada-370-xp.S */
+int armada_370_xp_cpu_resume(unsigned long);
+int armada_370_xp_cpu_suspend(unsigned long);
+
+static int armada_370_xp_enter_idle(struct cpuidle_device *dev,
+                               struct cpuidle_driver *drv,
+                               int index)
+{
+       bool deepidle = false;

drivers/cpuidle/cpuidle-armada-370-xp.c new file +48 -0

+
+       armada_370_xp_pmsu_idle_restore();
+
+       return index;
+}
+
+static struct cpuidle_driver armada_370_xp_idle_driver = {
+       .name                   = "armada_370_xp_idle",
+       .states[0]              = ARM_CPUIDLE_WFI_STATE,
+       .states[1]              = {
+               .enter                  = armada_370_xp_enter_idle,
+               .exit_latency           = 10,
+               .power_usage            = 50,
+               .target_residency       = 100,
+               .flags                  = CPUIDLE_FLAG_TIME_VALID,
+               .name                   = "MV CPU IDLE",
+               .desc                   = "CPU power down",
+       },
+       .states[2]              = {
+               .enter                  = armada_370_xp_enter_idle,
+               .exit_latency           = 100,
+               .power_usage            = 5,
+               .target_residency       = 1000,
+               .flags                  = CPUIDLE_FLAG_TIME_VALID,
+               .name                   = "MV CPU DEEP IDLE",
+               .desc                   = "CPU and L2 Fabric power down",
+       },
+       .state_count = ARMADA_370_XP_MAX_STATES,
+};
+
+static int __init armada_370_xp_cpuidle_init(void)
+{
+       if (!of_find_compatible_node(NULL, NULL, "marvell,armada-370-xp-pmsu"))
+               return -ENODEV;
+
+       if (!of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"))
+               return -ENODEV;
+
+       pr_info("Initializing Armada-XP CPU power management ");
+
+       armada_370_xp_pmsu_enable_l2_powerdown_onidle();
+
+       return cpuidle_register(&armada_370_xp_idle_driver, NULL);
+}
+
+module_init(armada_370_xp_cpuidle_init);
+
+MODULE_LICENSE("GPL");

drivers/cpuidle/suspend-armada-370-xp.S new file +33 -0

new file mode 100644
index 0000000..490bb9b
@@ -0,0 +1,91 @@
+/*
+ * CPU idle low level implementation for Marvell Armada 370 and Armada XP SoCs
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * Nadav Haklai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ */
+#include <linux/linkage.h>
+
+
+/*
+* armadaxp_cpu_suspend: enter cpu deepIdle state
+* input:
+*/
+ENTRY(armada_370_xp_cpu_suspend)
+/* Save ARM registers */
+       stmfd   sp!, {r4 - r11, lr}     @ save registers on stack
+
+       bl armada_370_xp_pmsu_idle_prepare
+       /*
+        * Invalidate L1 data cache. Even though only invalidate is
+        * necessary exported flush API is used here. Doing clean
+        * on already clean cache would be almost NOP.
+        */
+       bl v7_flush_dcache_all
+
+       /*
+        * Clear the SCTLR.C bit to prevent further data cache

drivers/cpuidle/suspend-armada-370-xp.S new file +5 -0

+       /* Data memory barrier and Data sync barrier */
+       dsb
+       dmb
+
+       bl armada_370_xp_disable_snoop_ena

drivers/cpuidle/suspend-armada-370-xp.S new file +23 -0

+/*
+ * ===================================
+ * == Resume path for non-OFF modes ==
+ * ===================================
+ */
+
+/* Enable SnoopEna - Exclusive */
+       mov     r0, #1                  @ r0!=0 means use virtual address
+       mov     r1, #0                  @ Do not add CPU to SMP group
+       bl ll_set_cpu_coherent
+
+/* Re-enable C-bit if needed */
+       mrc     p15, 0, r0, c1, c0, 0
+       tst     r0, #(1 << 2)           @ Check C bit enabled?
+       orreq   r0, r0, #(1 << 2)       @ Enable the C bit if cleared
+       mcreq   p15, 0, r0, c1, c0, 0
+       isb
+
+       ldmfd   sp!, {r4 - r11, pc}     @ restore regs and return
+ENDPROC(armada_370_xp_cpu_suspend)
+
+/*
+* armada_370_xp_cpu_resume: exit cpu deepIdle state

drivers/cpuidle/cpuidle-armada-370-xp.c line 55

Quoted 6 lines

drivers/cpuidle/cpuidle-armada-370-xp.c new file +6 -0

+       if (index == MV_CPU_DEEP_IDLE)
+               deepidle = true;
+
+       cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
+
+       cpu_init();

I do not think cpu_init() is needed. Either core resumes through cpu_resume, and there cpu_init() is called for you, or there is no reason to call cpu_init() since CPU was not shutdown.

Quoted 34 lines

drivers/cpuidle/suspend-armada-370-xp.S new file +33 -0

@@ -0,0 +1,91 @@
+/*
+ * CPU idle low level implementation for Marvell Armada 370 and Armada XP SoCs
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * Nadav Haklai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ */
+#include <linux/linkage.h>
+
+
+/*
+* armadaxp_cpu_suspend: enter cpu deepIdle state
+* input:
+*/
+ENTRY(armada_370_xp_cpu_suspend)
+/* Save ARM registers */
+       stmfd   sp!, {r4 - r11, lr}     @ save registers on stack
+
+       bl armada_370_xp_pmsu_idle_prepare
+       /*
+        * Invalidate L1 data cache. Even though only invalidate is
+        * necessary exported flush API is used here. Doing clean
+        * on already clean cache would be almost NOP.
+        */
+       bl v7_flush_dcache_all
+
+       /*
+        * Clear the SCTLR.C bit to prevent further data cache

drivers/cpuidle/suspend-armada-370-xp.S line 43

Quoted 10 lines

drivers/cpuidle/suspend-armada-370-xp.S new file +10 -0

+        * allocation. Clearing SCTLR.C would make all the data accesses
+        * strongly ordered and would not hit the cache.
+        */
+       mrc     p15, 0, r0, c1, c0, 0
+       bic     r0, r0, #(1 << 2)       @ Disable the C bit
+       mcr     p15, 0, r0, c1, c0, 0
+       isb
+
+       bl v7_flush_dcache_all
+

This code looks familiar and the first cache flush is not needed.

look at arch/arm/mach-vexpress/tc2_pm.c -> tc2_pm_down()

I know that probably the SMP bit in ACTLR is managed differently in this architecture but still, cache flushing should still apply and the TC2 sequence is the proper one, unless you explain to me why that does not work on this chipset.

Quoted 11 lines

drivers/cpuidle/suspend-armada-370-xp.S new file +10 -0

+       /* Data memory barrier and Data sync barrier */
+       dsb
+       dmb
+
+       bl armada_370_xp_disable_snoop_ena
+
+       dsb                             @ Data Synchronization Barrier
+
+/*
+ * ===================================

drivers/cpuidle/suspend-armada-370-xp.S line 58

Quoted 5 lines

drivers/cpuidle/suspend-armada-370-xp.S new file +5 -0

+ * == WFI instruction => Enter idle ==
+ * ===================================
+ */
+
+       wfi                             @ wait for interrupt

Here core is running out of coherency and I do not see any tlb invalidation in the resume path from non-OFF mode. Please can you elaborate on this ?

I have further comments on the set (in particular on the inner workings of coherency and how CPUs get in and out of idle - ie how this is managed by the power controller) but I will keep them for next version, when the first set of review comments is addressed.

Quoted 27 lines

Quoted 14 lines

drivers/cpuidle/suspend-armada-370-xp.S new file +14 -0

+ENTRY(armada_370_xp_cpu_suspend)
+/* Save ARM registers */
+       stmfd   sp!, {r4 - r11, lr}     @ save registers on stack
+
+       bl armada_370_xp_pmsu_idle_prepare
+       /*
+        * Invalidate L1 data cache. Even though only invalidate is
+        * necessary exported flush API is used here. Doing clean
+        * on already clean cache would be almost NOP.
+        */
+       bl v7_flush_dcache_all
+
+       /*
+        * Clear the SCTLR.C bit to prevent further data cache

drivers/cpuidle/suspend-armada-370-xp.S line 43

Quoted 10 lines

drivers/cpuidle/suspend-armada-370-xp.S new file +10 -0

+        * allocation. Clearing SCTLR.C would make all the data accesses
+        * strongly ordered and would not hit the cache.
+        */
+       mrc     p15, 0, r0, c1, c0, 0
+       bic     r0, r0, #(1 << 2)       @ Disable the C bit
+       mcr     p15, 0, r0, c1, c0, 0
+       isb
+
+       bl v7_flush_dcache_all
+

This code looks familiar and the first cache flush is not needed.

Quoted 6 lines

look at arch/arm/mach-vexpress/tc2_pm.c -> tc2_pm_down()

I know that probably the SMP bit in ACTLR is managed differently in this architecture but still, cache flushing should still apply and the TC2 sequence is the proper one, unless you explain to me why that does not work on this chipset.

Here we follow the advices you gave in your presentation at Linaro Connect
Q2-2012: "Idling ARMs in a Busy World: Linux Power Management for ARM multi-cluster systems"
http://www.linaro.org/documents/download/84a5990d6f227c90e3d3a3ad2af0e22c4fd1d0bd4a6c7
at p21.

Quoting the author of the original version od the code :
"Since the second flush is almost NOPs, I preferred to keep them both in the same sequence."

Quoted 11 lines

Quoted 11 lines

drivers/cpuidle/suspend-armada-370-xp.S new file +10 -0

+       /* Data memory barrier and Data sync barrier */
+       dsb
+       dmb
+
+       bl armada_370_xp_disable_snoop_ena
+
+       dsb                             @ Data Synchronization Barrier
+
+/*
+ * ===================================

Quoted 9 lines

Review

Quoted 5 lines

+ * == WFI instruction => Enter idle ==
+ * ===================================
+ */

(quoted diff) +2 -0

+
+       wfi                             @ wait for interrupt

Here core is running out of coherency and I do not see any tlb invalidation in the resume path from non-OFF mode. Please can you elaborate on this ?

The TLB invalidation should take place in /arch/arm/kernel/suspend.c cpu_suspend( ) in case the return value is 0 caused by cpu_suspend_abort.
Did we missed something?

Quoted 5 lines

I have further comments on the set (in particular on the inner workings of coherency and how CPUs get in and out of idle - ie how this is managed by the power controller) but I will keep them for next version, when the first set of review comments is addressed.

You can make your comments on the last version I have just sent.

Thanks for your review

Hello,

This patch set adds the CPU idle support for Armada XP and prepares the support for Armada 370. This was based on the work of Nadav Haklai.

Quoted 2 lines · Gregory CLEMENT

Hello,

Quoted 3 lines

This patch set adds the CPU idle support for Armada XP and prepares the support for Armada 370. This was based on the work of Nadav Haklai.

Hi Gregory.

Kirkwood has the ability to put the DDR into self refresh mode, which is used as part of the second level idle mode. Does 370/XP have this?

For XP, with it being SMP, it would be a bit more complex, since you would not want to use it unless all CPUs were idle.

Thanks
Andrew

Quoted 8 lines · Andrew Lunn

Quoted 5 lines · Gregory CLEMENT

Hello,

This patch set adds the CPU idle support for Armada XP and prepares the support for Armada 370. This was based on the work of Nadav Haklai.

Quoted 4 lines

Hi Gregory.

Kirkwood has the ability to put the DDR into self refresh mode, which is used as part of the second level idle mode. Does 370/XP have this?

Indeed there is a self refresh bit on a DDR related register. I thought this kind of feature is more likely used for suspend to ram.

Quoted 4 lines · Andrew Lunn

Quoted 2 lines · Gregory CLEMENT

Hello,

Quoted 8 lines

Quoted 3 lines

This patch set adds the CPU idle support for Armada XP and prepares the support for Armada 370. This was based on the work of Nadav Haklai.

Hi Gregory.

Kirkwood has the ability to put the DDR into self refresh mode, which is used as part of the second level idle mode. Does 370/XP have this?

Indeed there is a self refresh bit on a DDR related register. I thought this kind of feature is more likely used for suspend to ram.

We plan to also submit the suspend to ram but not immediately.

Quoted 3 lines

For XP, with it being SMP, it would be a bit more complex, since you would not want to use it unless all CPUs were idle.

I wonder how the others SMP ARM SoCs deal with it, I hope this time there will be a framework available and we won't have to create it! ;)

Quoted 17 lines · Gregory CLEMENT

Quoted 4 lines · Andrew Lunn

Quoted 2 lines · Gregory CLEMENT

Hello,

Quoted 8 lines

Quoted 3 lines

This patch set adds the CPU idle support for Armada XP and prepares the support for Armada 370. This was based on the work of Nadav Haklai.

Hi Gregory.

Kirkwood has the ability to put the DDR into self refresh mode, which is used as part of the second level idle mode. Does 370/XP have this?

Indeed there is a self refresh bit on a DDR related register. I thought this kind of feature is more likely used for suspend to ram.

Quoted 8 lines

We plan to also submit the suspend to ram but not immediately.

Quoted 3 lines

For XP, with it being SMP, it would be a bit more complex, since you would not want to use it unless all CPUs were idle.

I wonder how the others SMP ARM SoCs deal with it, I hope this time there will be a framework available and we won't have to create it! ;)

You shouldn't have to invent anything here.

For low-power states that require all CPUs to be idle, we have "coupled" CPUidle states (c.f. drivers/cpuidle/coupled.) OMAP and Tegra are using this, but I believe Daniel wants to move away from this, so I'll let him elaborate.

Hello,

This patch set adds the CPU idle support for Armada XP and prepares the support for Armada 370. This was based on the work of Nadav Haklai.

Most of the patches modify the mvebu code in order to prepare the support for CPU idle, hence the patches 2 to 10 should go to mvebu subsystem (and then arm-soc).

The first patch should go through ARM subsystem and should be taken by Russell King.

The 11th patch 'cpuidle: mvebu: Add initial cpu idle support for Armada 370/XP SoC' is the only one who should go to the cpuidle subsystem. But of course I would like that Daniel Lezcano or Rafael J. Wysocki have a look on the whole series.

The last patch should also go to mvebu subsystem (and then arm-soc) but with an Acked-by from on of the device tree maintainer.

The whole series is also available in the branch CPU-idle-ArmadaXP-v2 at https://github.com/MISL-EBU-System-SW/mainline-public.git

Thanks,

Changelog:

v1 -> v2:

* Removed the pm_level kernel parameter. As Kevin Hilman pointed, its usage can be replaced by using /sys/devices/system/cpu/cpu*/cpuidle/state*/disable or the kernel parameter cpuidle.off.

* Used BIT() macro (reported by Ezequiel)

* Made the function more readable the armada_370_xp_pmsu_idle_prepare() function (reported by Thomas)

* Moved the config entry in Kconfig.arm, and rename the config symbol according the pattern used by other arm cpu: ARM_"soc name"_CPUIDLE

* Moved the build rule under the new ARM SoC section in the Makefile

* Rebased on Linus Torvalds master branch of Thursday September 12

Gregory CLEMENT (12):
ARM: PJ4B: Add cpu_suspend/cpu_resume hooks for PJ4B
ARM: mvebu: ll_set_cpu_coherent no more uses the coherency address as
parameter
ARM: mvebu: ll_set_cpu_coherent always uses the current CPU
ARM: mvebu: Remove the unused argument of set_cpu_coherent()
ARM: mvebu: Make ll_set_cpu_coherent() more configurable
ARM: mvebu: Low level functions to disable cache snooping
ARM: mvebu: Add a new set of registers for pmsu
ARM: mvebu: Allow to power down L2 cache controller in idle mode
ARM: mvebu: Add the PMSU related part of the cpu idle functions
ARM: mvebu: Set the start address of a CPU in a separate function
cpuidle: mvebu: Add initial CPU idle support for Armada 370/XP SoC
ARM: dts: mvebu: Add a new set of registers to the PMSU node

Gregory,

Quoted 47 lines · Gregory CLEMENT

Hello,

This patch set adds the CPU idle support for Armada XP and prepares the support for Armada 370. This was based on the work of Nadav Haklai.

Most of the patches modify the mvebu code in order to prepare the support for CPU idle, hence the patches 2 to 10 should go to mvebu subsystem (and then arm-soc).

The first patch should go through ARM subsystem and should be taken by Russell King.

The 11th patch 'cpuidle: mvebu: Add initial cpu idle support for Armada 370/XP SoC' is the only one who should go to the cpuidle subsystem. But of course I would like that Daniel Lezcano or Rafael J. Wysocki have a look on the whole series.

The last patch should also go to mvebu subsystem (and then arm-soc) but with an Acked-by from on of the device tree maintainer.

The whole series is also available in the branch CPU-idle-ArmadaXP-v2 at https://github.com/MISL-EBU-System-SW/mainline-public.git

Thanks,

Changelog:

v1 -> v2:

* Removed the pm_level kernel parameter. As Kevin Hilman pointed, its usage can be replaced by using /sys/devices/system/cpu/cpu*/cpuidle/state*/disable or the kernel parameter cpuidle.off.

* Used BIT() macro (reported by Ezequiel)

* Made the function more readable the armada_370_xp_pmsu_idle_prepare() function (reported by Thomas)

* Moved the config entry in Kconfig.arm, and rename the config symbol according the pattern used by other arm cpu: ARM_"soc name"_CPUIDLE

* Moved the build rule under the new ARM SoC section in the Makefile

* Rebased on Linus Torvalds master branch of Thursday September 12

Quoted 14 lines

Gregory CLEMENT (12):
ARM: PJ4B: Add cpu_suspend/cpu_resume hooks for PJ4B
ARM: mvebu: ll_set_cpu_coherent no more uses the coherency address as
parameter
ARM: mvebu: ll_set_cpu_coherent always uses the current CPU
ARM: mvebu: Remove the unused argument of set_cpu_coherent()
ARM: mvebu: Make ll_set_cpu_coherent() more configurable
ARM: mvebu: Low level functions to disable cache snooping
ARM: mvebu: Add a new set of registers for pmsu
ARM: mvebu: Allow to power down L2 cache controller in idle mode
ARM: mvebu: Add the PMSU related part of the cpu idle functions
ARM: mvebu: Set the start address of a CPU in a separate function
cpuidle: mvebu: Add initial CPU idle support for Armada 370/XP SoC
ARM: dts: mvebu: Add a new set of registers to the PMSU node

Is a v3 of this series on it's way?

On 02/10/2013 20:39, Jason Cooper wrote:

Quoted 1 line

Is a v3 of this series on it's way?

Yes I got some feedback from the Marvell engineers about the tricky part.

However I am quite busy this week, I am not sure to be able to sent it before Monday.