[PATCH v3 00/14] CPU idle for Armada XP

[PATCH v3 00/14] CPU idle for Armada XP

[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 "ARM: mvebu: Add CPU idle low level support for Marvell
Armada XP" was the low level part of the cpuidle support, it will be
under mach-mvebu/ and hence should go to mvebu subsystem but it should
received an acked-by from Daniel Lezcano or Rafael J. Wysocki.

The 12th 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 13th patch should go to mvebu subsystem.

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-v3
at https://github.com/MISL-EBU-System-SW/mainline-public.git

Thanks,

Changelog:
v2 -> v3:

* Converted the driver to use module_platform_driver. This lead to the
  introduction of a new patch (PATCH 11). Pointed by Daniel Lezcano.

* Used PUIDLE_DRIVER_FLAGS_MASK to store the deep idle information,
  suggested by Daniel Lezcano.

* Removed cpu_init call from armada_370_xp_enter_idle
  function. Pointed by Lorenzo Pieralisi.

* Rebased on v3.12-rc5


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 (14):
  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
  ARM: mvebu: Add CPU idle low level support for Marvell Armada XP
  cpuidle: mvebu: Add initial CPU idle support for Armada 370/XP SoC
  ARM: mvebu: register the cpuidle driver for the Armada XP SoCs
  ARM: dts: mvebu: Add a new set of registers to the PMSU node

 .../devicetree/bindings/arm/armada-370-xp-pmsu.txt |  12 ++-
 arch/arm/boot/dts/armada-xp.dtsi                   |   2 +-
 arch/arm/mach-mvebu/Makefile                       |   1 +
 arch/arm/mach-mvebu/armada-370-xp.c                |   6 ++
 arch/arm/mach-mvebu/coherency.c                    |  12 +--
 arch/arm/mach-mvebu/coherency.h                    |   2 +-
 arch/arm/mach-mvebu/coherency_ll.S                 |  69 +++++++++---
 arch/arm/mach-mvebu/headsmp.S                      |  15 +--
 arch/arm/mach-mvebu/platsmp.c                      |   2 +-
 arch/arm/mach-mvebu/pmsu.c                         | 102 +++++++++++++++++-
 arch/arm/mach-mvebu/suspend-armada-370-xp.S        |  90 ++++++++++++++++
 arch/arm/mm/proc-v7.S                              |  64 ++++++++++-
 drivers/cpuidle/Kconfig.arm                        |   5 +
 drivers/cpuidle/Makefile                           |   1 +
 drivers/cpuidle/cpuidle-armada-370-xp.c            | 117 +++++++++++++++++++++
 include/linux/armada-370-xp-pmsu.h                 |  19 ++++
 16 files changed, 477 insertions(+), 42 deletions(-)
 create mode 100644 arch/arm/mach-mvebu/suspend-armada-370-xp.S
 create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c
 create mode 100644 include/linux/armada-370-xp-pmsu.h

-- 
1.8.1.2

[PATCH v3 01/14] ARM: PJ4B: Add cpu_suspend/cpu_resume hooks for PJ4B

[Gregory CLEMENT]

PJ4B needs extra instructions for suspend and resume, so instead of
using the armv7 version, this commit introduces specific versions for
PJ4B.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mm/proc-v7.S | 64 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index c63d9bd..b14801b 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -162,9 +162,67 @@ ENDPROC(cpu_pj4b_do_idle)
 	globl_equ	cpu_pj4b_do_idle,  	cpu_v7_do_idle
 #endif
 	globl_equ	cpu_pj4b_dcache_clean_area,	cpu_v7_dcache_clean_area
-	globl_equ	cpu_pj4b_do_suspend,	cpu_v7_do_suspend
-	globl_equ	cpu_pj4b_do_resume,	cpu_v7_do_resume
-	globl_equ	cpu_pj4b_suspend_size,	cpu_v7_suspend_size
+#ifdef CONFIG_ARM_CPU_SUSPEND
+ENTRY(cpu_pj4b_do_suspend)
+	stmfd	sp!, {r4 - r10, lr}
+	mrc	p15, 0, r4, c13, c0, 0	@ FCSE/PID
+	mrc	p15, 0, r5, c13, c0, 3	@ User r/o thread ID
+	stmia	r0!, {r4 - r5}
+	mrc	p15, 1, r6, c15, c1, 0  @ save CP15 - extra features
+	mrc	p15, 1, r7, c15, c2, 0	@ save CP15 - Aux Func Modes Ctrl 0
+	mrc	p15, 1, r8, c15, c1, 2	@ save CP15 - Aux Debug Modes Ctrl 2
+	mrc	p15, 1, r9, c15, c1, 1  @ save CP15 - Aux Debug Modes Ctrl 1
+	mrc	p15, 0, r10, c9, c14, 0  @ save CP15 - PMC
+	stmia	r0!, {r6 - r10}
+	mrc	p15, 0, r6, c3, c0, 0	@ Domain ID
+	mrc	p15, 0, r7, c2, c0, 1	@ TTB 1
+	mrc	p15, 0, r11, c2, c0, 2	@ TTB control register
+	mrc	p15, 0, r8, c1, c0, 0	@ Control register
+	mrc	p15, 0, r9, c1, c0, 1	@ Auxiliary control register
+	mrc	p15, 0, r10, c1, c0, 2	@ Co-processor access control
+	stmia	r0, {r6 - r11}
+	ldmfd	sp!, {r4 - r10, pc}
+ENDPROC(cpu_pj4b_do_suspend)
+
+ENTRY(cpu_pj4b_do_resume)
+	mov	ip, #0
+	mcr	p15, 0, ip, c7, c5, 0	@ invalidate I cache
+	mcr	p15, 0, ip, c13, c0, 1	@ set reserved context ID
+	ldmia	r0!, {r4 - r5}
+	mcr	p15, 0, r4, c13, c0, 0	@ FCSE/PID
+	mcr	p15, 0, r5, c13, c0, 3	@ User r/o thread ID
+	ldmia	r0!, {r6 - r10}
+	mcr	p15, 1, r6, c15, c1, 0  @ save CP15 - extra features
+	mcr	p15, 1, r7, c15, c2, 0	@ save CP15 - Aux Func Modes Ctrl 0
+	mcr	p15, 1, r8, c15, c1, 2	@ save CP15 - Aux Debug Modes Ctrl 2
+	mcr	p15, 1, r9, c15, c1, 1  @ save CP15 - Aux Debug Modes Ctrl 1
+	mcr	p15, 0, r10, c9, c14, 0  @ save CP15 - PMC
+	ldmia	r0, {r6 - r11}
+	mcr	p15, 0, ip, c8, c7, 0	@ invalidate TLBs
+	mcr	p15, 0, r6, c3, c0, 0	@ Domain ID
+#ifndef CONFIG_ARM_LPAE
+	ALT_SMP(orr	r1, r1, #TTB_FLAGS_SMP)
+	ALT_UP(orr	r1, r1, #TTB_FLAGS_UP)
+#endif
+	mcr	p15, 0, r1, c2, c0, 0	@ TTB 0
+	mcr	p15, 0, r7, c2, c0, 1	@ TTB 1
+	mcr	p15, 0, r11, c2, c0, 2	@ TTB control register
+	ldr	r4, =PRRR		@ PRRR
+	ldr	r5, =NMRR		@ NMRR
+	mcr	p15, 0, r4, c10, c2, 0	@ write PRRR
+	mcr	p15, 0, r5, c10, c2, 1	@ write NMRR
+	mrc	p15, 0, r4, c1, c0, 1	@ Read Auxiliary control register
+	teq	r4, r9			@ Is it already set?
+	mcrne	p15, 0, r9, c1, c0, 1	@ No, so write it
+	mcr	p15, 0, r10, c1, c0, 2	@ Co-processor access control
+	isb
+	dsb
+	mov	r0, r8			@ control register
+	b	cpu_resume_mmu
+ENDPROC(cpu_pj4b_do_resume)
+#endif
+.globl	cpu_pj4b_suspend_size
+.equ	cpu_pj4b_suspend_size, 4 * 13
 
 #endif
 
-- 
1.8.1.2

[PATCH v3 02/14] ARM: mvebu: ll_set_cpu_coherent no more uses the coherency address as parameter

[Gregory CLEMENT]

Until now the calling functions of ll_set_cpu_coherent() have to know
the base address of the coherency registers. This commit doesn't
expose anymore this address, and replace the argument by a flag to
indicate if we have to use the physical or the virtual address.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/coherency.c    |  6 +++---
 arch/arm/mach-mvebu/coherency_ll.S | 18 +++++++++++++++++-
 arch/arm/mach-mvebu/headsmp.S      | 10 ++--------
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index 58adf2f..d492fb3 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -29,7 +29,7 @@
 #include "armada-370-xp.h"
 
 unsigned long coherency_phys_base;
-static void __iomem *coherency_base;
+void __iomem *coherency_base;
 static void __iomem *coherency_cpu_base;
 
 /* Coherency fabric registers */
@@ -43,7 +43,7 @@ static struct of_device_id of_coherency_table[] = {
 };
 
 /* Function defined in coherency_ll.S */
-int ll_set_cpu_coherent(void __iomem *base_addr, unsigned int hw_cpu_id);
+int ll_set_cpu_coherent(bool use_virt_addr, unsigned int hw_cpu_id);
 
 int set_cpu_coherent(unsigned int hw_cpu_id, int smp_group_id)
 {
@@ -53,7 +53,7 @@ int set_cpu_coherent(unsigned int hw_cpu_id, int smp_group_id)
 		return 1;
 	}
 
-	return ll_set_cpu_coherent(coherency_base, hw_cpu_id);
+	return ll_set_cpu_coherent(true, hw_cpu_id);
 }
 
 static inline void mvebu_hwcc_sync_io_barrier(void)
diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S
index 5476669..8d4e22f 100644
--- a/arch/arm/mach-mvebu/coherency_ll.S
+++ b/arch/arm/mach-mvebu/coherency_ll.S
@@ -22,10 +22,22 @@
 
 	.text
 /*
- * r0: Coherency fabric base register address
+ * r0: if r0==0 => physical addres, else virtual address
  * r1: HW CPU id
  */
 ENTRY(ll_set_cpu_coherent)
+	cmp	r0, #0
+	bne	1f
+    /* use physical address */
+	adr	r0, 3f
+	ldr	r3, [r0]
+	ldr	r0, [r0, r3]
+	b	2f
+1:
+    /* use virtual address */
+	ldr	r0, =(coherency_base)
+	ldr	r0, [r0]
+2:
 	/* Create bit by cpu index */
 	mov	r3, #(1 << 24)
 	lsl	r1, r3, r1
@@ -53,3 +65,7 @@ ENTRY(ll_set_cpu_coherent)
 	mov	r0, #0
 	mov	pc, lr
 ENDPROC(ll_set_cpu_coherent)
+
+	.align 2
+3:
+	.long	coherency_phys_base - .
diff --git a/arch/arm/mach-mvebu/headsmp.S b/arch/arm/mach-mvebu/headsmp.S
index 8a1b0c9..bffaabc 100644
--- a/arch/arm/mach-mvebu/headsmp.S
+++ b/arch/arm/mach-mvebu/headsmp.S
@@ -27,10 +27,8 @@
  * startup
  */
 ENTRY(armada_xp_secondary_startup)
-	/* Get coherency fabric base physical address */
-	adr	r0, 1f
-	ldr	r1, [r0]
-	ldr	r0, [r0, r1]
+	/* Use physical addrss */
+	mov	r0, #0
 
 	/* Read CPU id */
 	mrc     p15, 0, r1, c0, c0, 5
@@ -41,7 +39,3 @@ ENTRY(armada_xp_secondary_startup)
 	b	secondary_startup
 
 ENDPROC(armada_xp_secondary_startup)
-
-	.align 2
-1:
-	.long	coherency_phys_base - .
-- 
1.8.1.2

[PATCH v3 02/14] ARM: mvebu: ll_set_cpu_coherent no more uses the coherency address as parameter

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 14 Oct 2013 15:58:14 +0200, Gregory CLEMENT wrote:
> Until now the calling functions of ll_set_cpu_coherent() have to know
> the base address of the coherency registers. This commit doesn't
> expose anymore this address, and replace the argument by a flag to
> indicate if we have to use the physical or the virtual address.

Why is this necessary? Passing the base address (either physical or
virtual) seems much better than the boolean you're introducing.

The title of the commit is also badly worded I believe, "no more uses"
is not a really great way of expressing what it is doing. It should
probably be:

	ARM: mvebu: make ll_set_cpu_coherent use a boolean to choose coherency fabric address

Nonetheless, a few comments below.

> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/coherency.c    |  6 +++---
>  arch/arm/mach-mvebu/coherency_ll.S | 18 +++++++++++++++++-
>  arch/arm/mach-mvebu/headsmp.S      | 10 ++--------
>  3 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
> index 58adf2f..d492fb3 100644
> --- a/arch/arm/mach-mvebu/coherency.c
> +++ b/arch/arm/mach-mvebu/coherency.c
> @@ -29,7 +29,7 @@
>  #include "armada-370-xp.h"
>  
>  unsigned long coherency_phys_base;
> -static void __iomem *coherency_base;
> +void __iomem *coherency_base;
>  static void __iomem *coherency_cpu_base;
>  
>  /* Coherency fabric registers */
> @@ -43,7 +43,7 @@ static struct of_device_id of_coherency_table[] = {
>  };
>  
>  /* Function defined in coherency_ll.S */
> -int ll_set_cpu_coherent(void __iomem *base_addr, unsigned int hw_cpu_id);
> +int ll_set_cpu_coherent(bool use_virt_addr, unsigned int hw_cpu_id);
>  
>  int set_cpu_coherent(unsigned int hw_cpu_id, int smp_group_id)
>  {
> @@ -53,7 +53,7 @@ int set_cpu_coherent(unsigned int hw_cpu_id, int smp_group_id)
>  		return 1;
>  	}
>  
> -	return ll_set_cpu_coherent(coherency_base, hw_cpu_id);
> +	return ll_set_cpu_coherent(true, hw_cpu_id);
>  }
>  
>  static inline void mvebu_hwcc_sync_io_barrier(void)
> diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S
> index 5476669..8d4e22f 100644
> --- a/arch/arm/mach-mvebu/coherency_ll.S
> +++ b/arch/arm/mach-mvebu/coherency_ll.S
> @@ -22,10 +22,22 @@
>  
>  	.text
>  /*
> - * r0: Coherency fabric base register address
> + * r0: if r0==0 => physical addres, else virtual address

address, not addres. Also, which address are we talking about?

 * r0: if r0 equal 0, then the function will use access the coherency
   fabric through its physical address (to be used when the MMU is
   disabled). Otherwise, the virtual address at which the coherency
   fabric has been mapped will be used (to be used when the MMU is
   enabled).

But I still complain that the original solution of passing the address
was better. If you really have a strong explanation to switch to this
boolean, then don't use a boolean at all: test if the MMU is enabled,
and if not, use the physical address automatically, otherwise, use the
virtual address automatically.

>   * r1: HW CPU id
>   */
>  ENTRY(ll_set_cpu_coherent)
> +	cmp	r0, #0
> +	bne	1f
> +    /* use physical address */

Bad indentation for the comment: it should be one tab and not spaces,
not match the other code around.

> +	adr	r0, 3f

> +	ldr	r3, [r0]
> +	ldr	r0, [r0, r3]
> +	b	2f
> +1:
> +    /* use virtual address */

Same comment for the indentation.

> +	ldr	r0, =(coherency_base)

Are the parenthesis needed?

> +	ldr	r0, [r0]
> +2:
>  	/* Create bit by cpu index */
>  	mov	r3, #(1 << 24)
>  	lsl	r1, r3, r1
> @@ -53,3 +65,7 @@ ENTRY(ll_set_cpu_coherent)
>  	mov	r0, #0
>  	mov	pc, lr
>  ENDPROC(ll_set_cpu_coherent)
> +
> +	.align 2
> +3:
> +	.long	coherency_phys_base - .
> diff --git a/arch/arm/mach-mvebu/headsmp.S b/arch/arm/mach-mvebu/headsmp.S
> index 8a1b0c9..bffaabc 100644
> --- a/arch/arm/mach-mvebu/headsmp.S
> +++ b/arch/arm/mach-mvebu/headsmp.S
> @@ -27,10 +27,8 @@
>   * startup
>   */
>  ENTRY(armada_xp_secondary_startup)
> -	/* Get coherency fabric base physical address */
> -	adr	r0, 1f
> -	ldr	r1, [r0]
> -	ldr	r0, [r0, r1]
> +	/* Use physical addrss */

addrss -> address. Please pay attention to typos, there are such typos
everywhere.

Also, address of what?

> +	mov	r0, #0
>  
>  	/* Read CPU id */
>  	mrc     p15, 0, r1, c0, c0, 5
> @@ -41,7 +39,3 @@ ENTRY(armada_xp_secondary_startup)
>  	b	secondary_startup
>  
>  ENDPROC(armada_xp_secondary_startup)
> -
> -	.align 2
> -1:
> -	.long	coherency_phys_base - .



-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 03/14] ARM: mvebu: ll_set_cpu_coherent always uses the current CPU

[Gregory CLEMENT]

ll_set_cpu_coherent is always used on the current CPU, so instead of
passing the CPU id as argument, ll_set_cpu_coherent() can find it by
itself.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/coherency.c    | 10 +++++-----
 arch/arm/mach-mvebu/coherency.h    |  2 +-
 arch/arm/mach-mvebu/coherency_ll.S |  7 ++++---
 arch/arm/mach-mvebu/headsmp.S      |  4 ----
 arch/arm/mach-mvebu/platsmp.c      |  2 +-
 5 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index d492fb3..1836ba4 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -43,17 +43,17 @@ static struct of_device_id of_coherency_table[] = {
 };
 
 /* Function defined in coherency_ll.S */
-int ll_set_cpu_coherent(bool use_virt_addr, unsigned int hw_cpu_id);
+int ll_set_cpu_coherent(bool use_virt_addr);
 
-int set_cpu_coherent(unsigned int hw_cpu_id, int smp_group_id)
+int set_cpu_coherent(int smp_group_id)
 {
 	if (!coherency_base) {
-		pr_warn("Can't make CPU %d cache coherent.\n", hw_cpu_id);
+		pr_warn("Can't make current CPU cache coherent.\n");
 		pr_warn("Coherency fabric is not initialized\n");
 		return 1;
 	}
 
-	return ll_set_cpu_coherent(true, hw_cpu_id);
+	return ll_set_cpu_coherent(true);
 }
 
 static inline void mvebu_hwcc_sync_io_barrier(void)
@@ -139,7 +139,7 @@ int __init coherency_init(void)
 		sync_cache_w(&coherency_phys_base);
 		coherency_base = of_iomap(np, 0);
 		coherency_cpu_base = of_iomap(np, 1);
-		set_cpu_coherent(cpu_logical_map(smp_processor_id()), 0);
+		set_cpu_coherent(0);
 		of_node_put(np);
 	}
 
diff --git a/arch/arm/mach-mvebu/coherency.h b/arch/arm/mach-mvebu/coherency.h
index df33ad8..d4bc067 100644
--- a/arch/arm/mach-mvebu/coherency.h
+++ b/arch/arm/mach-mvebu/coherency.h
@@ -14,7 +14,7 @@
 #ifndef __MACH_370_XP_COHERENCY_H
 #define __MACH_370_XP_COHERENCY_H
 
-int set_cpu_coherent(int cpu_id, int smp_group_id);
+int set_cpu_coherent(int smp_group_id);
 int coherency_init(void);
 
 #endif	/* __MACH_370_XP_COHERENCY_H */
diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S
index 8d4e22f..fc2e6f7 100644
--- a/arch/arm/mach-mvebu/coherency_ll.S
+++ b/arch/arm/mach-mvebu/coherency_ll.S
@@ -23,7 +23,6 @@
 	.text
 /*
  * r0: if r0==0 => physical addres, else virtual address
- * r1: HW CPU id
  */
 ENTRY(ll_set_cpu_coherent)
 	cmp	r0, #0
@@ -39,8 +38,10 @@ ENTRY(ll_set_cpu_coherent)
 	ldr	r0, [r0]
 2:
 	/* Create bit by cpu index */
-	mov	r3, #(1 << 24)
-	lsl	r1, r3, r1
+	mrc	15, 0, r1, cr0, cr0, 5
+	and	r1, r1, #15
+	mov	r2, #(1 << 24)
+	lsl	r1, r2, r1
 
 	/* Add CPU to SMP group - Atomic */
 	add	r3, r0, #ARMADA_XP_CFB_CTL_REG_OFFSET
diff --git a/arch/arm/mach-mvebu/headsmp.S b/arch/arm/mach-mvebu/headsmp.S
index bffaabc..b2c6e95 100644
--- a/arch/arm/mach-mvebu/headsmp.S
+++ b/arch/arm/mach-mvebu/headsmp.S
@@ -30,10 +30,6 @@ ENTRY(armada_xp_secondary_startup)
 	/* Use physical addrss */
 	mov	r0, #0
 
-	/* Read CPU id */
-	mrc     p15, 0, r1, c0, c0, 5
-	and     r1, r1, #0xF
-
 	/* Add CPU to coherency fabric */
 	bl	ll_set_cpu_coherent
 	b	secondary_startup
diff --git a/arch/arm/mach-mvebu/platsmp.c b/arch/arm/mach-mvebu/platsmp.c
index ff69c2d..1b86e03 100644
--- a/arch/arm/mach-mvebu/platsmp.c
+++ b/arch/arm/mach-mvebu/platsmp.c
@@ -102,7 +102,7 @@ void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
 
 	set_secondary_cpus_clock();
 	flush_cache_all();
-	set_cpu_coherent(cpu_logical_map(smp_processor_id()), 0);
+	set_cpu_coherent(0);
 
 	/*
 	 * In order to boot the secondary CPUs we need to ensure
-- 
1.8.1.2

[PATCH v3 03/14] ARM: mvebu: ll_set_cpu_coherent always uses the current CPU

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 14 Oct 2013 15:58:15 +0200, Gregory CLEMENT wrote:
> ll_set_cpu_coherent is always used on the current CPU, so instead of
> passing the CPU id as argument, ll_set_cpu_coherent() can find it by
> itself.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Of course, it depends of PATCH 02/14, which I don't agree with at
the moment, but I am fine with this PATCH 03/14.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 04/14] ARM: mvebu: Remove the unused argument of set_cpu_coherent()

[Gregory CLEMENT]

set_cpu_coherent() took the SMP group ID as parameter. But this
parameter was never used, and the CPU always use the SMP group 0. So
we can remove this parameter.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/coherency.c | 4 ++--
 arch/arm/mach-mvebu/coherency.h | 2 +-
 arch/arm/mach-mvebu/platsmp.c   | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index 1836ba4..8c9e84d 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -45,7 +45,7 @@ static struct of_device_id of_coherency_table[] = {
 /* Function defined in coherency_ll.S */
 int ll_set_cpu_coherent(bool use_virt_addr);
 
-int set_cpu_coherent(int smp_group_id)
+int set_cpu_coherent(void)
 {
 	if (!coherency_base) {
 		pr_warn("Can't make current CPU cache coherent.\n");
@@ -139,7 +139,7 @@ int __init coherency_init(void)
 		sync_cache_w(&coherency_phys_base);
 		coherency_base = of_iomap(np, 0);
 		coherency_cpu_base = of_iomap(np, 1);
-		set_cpu_coherent(0);
+		set_cpu_coherent();
 		of_node_put(np);
 	}
 
diff --git a/arch/arm/mach-mvebu/coherency.h b/arch/arm/mach-mvebu/coherency.h
index d4bc067..700376e 100644
--- a/arch/arm/mach-mvebu/coherency.h
+++ b/arch/arm/mach-mvebu/coherency.h
@@ -14,7 +14,7 @@
 #ifndef __MACH_370_XP_COHERENCY_H
 #define __MACH_370_XP_COHERENCY_H
 
-int set_cpu_coherent(int smp_group_id);
+int set_cpu_coherent(void);
 int coherency_init(void);
 
 #endif	/* __MACH_370_XP_COHERENCY_H */
diff --git a/arch/arm/mach-mvebu/platsmp.c b/arch/arm/mach-mvebu/platsmp.c
index 1b86e03..a0fe97e 100644
--- a/arch/arm/mach-mvebu/platsmp.c
+++ b/arch/arm/mach-mvebu/platsmp.c
@@ -102,7 +102,7 @@ void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
 
 	set_secondary_cpus_clock();
 	flush_cache_all();
-	set_cpu_coherent(0);
+	set_cpu_coherent();
 
 	/*
 	 * In order to boot the secondary CPUs we need to ensure
-- 
1.8.1.2

[PATCH v3 04/14] ARM: mvebu: Remove the unused argument of set_cpu_coherent()

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 14 Oct 2013 15:58:16 +0200, Gregory CLEMENT wrote:
> set_cpu_coherent() took the SMP group ID as parameter. But this
> parameter was never used, and the CPU always use the SMP group 0. So

use -> uses

But probably better: "But this parameter was never used, and all CPUs
are always joining the SMP group 0".

Other than that:

Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 05/14] ARM: mvebu: Make ll_set_cpu_coherent() more configurable

[Gregory CLEMENT]

ll_set_cpu_coherent does two things to set the coherency for a CPU:

- Adding the CPU to the SMP group 0
- Enabling the snooping on the CPU

Only the second part is needed when coming back from a CPU Idle. This
commit allows to choose to do the first part or not.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/coherency.c    |  4 ++--
 arch/arm/mach-mvebu/coherency_ll.S | 30 ++++++++++++++++++------------
 arch/arm/mach-mvebu/headsmp.S      |  3 +++
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index 8c9e84d..e09bdfd 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -43,7 +43,7 @@ static struct of_device_id of_coherency_table[] = {
 };
 
 /* Function defined in coherency_ll.S */
-int ll_set_cpu_coherent(bool use_virt_addr);
+int ll_set_cpu_coherent(bool use_virt_addr, bool use_smp_group);
 
 int set_cpu_coherent(void)
 {
@@ -53,7 +53,7 @@ int set_cpu_coherent(void)
 		return 1;
 	}
 
-	return ll_set_cpu_coherent(true);
+	return ll_set_cpu_coherent(true, true);
 }
 
 static inline void mvebu_hwcc_sync_io_barrier(void)
diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S
index fc2e6f7..1526b94 100644
--- a/arch/arm/mach-mvebu/coherency_ll.S
+++ b/arch/arm/mach-mvebu/coherency_ll.S
@@ -22,7 +22,8 @@
 
 	.text
 /*
- * r0: if r0==0 => physical addres, else virtual address
+ * r0: Use virtual address if r0 != 0, else physical address
+ * r1: Add CPU to the SMP group if r1 != 0
  */
 ENTRY(ll_set_cpu_coherent)
 	cmp	r0, #0
@@ -38,26 +39,31 @@ ENTRY(ll_set_cpu_coherent)
 	ldr	r0, [r0]
 2:
 	/* Create bit by cpu index */
-	mrc	15, 0, r1, cr0, cr0, 5
-	and	r1, r1, #15
+	mrc	15, 0, r3, cr0, cr0, 5
+	and	r3, r3, #15
 	mov	r2, #(1 << 24)
-	lsl	r1, r2, r1
+	lsl	r3, r2, r3
+
+	/* If r1=!0 then just enable the snooping */
+	cmp	r1, #0
+	beq	2f
 
 	/* Add CPU to SMP group - Atomic */
-	add	r3, r0, #ARMADA_XP_CFB_CTL_REG_OFFSET
+	add	r1, r0, #ARMADA_XP_CFB_CTL_REG_OFFSET
 1:
-	ldrex	r2, [r3]
-	orr	r2, r2, r1
-	strex 	r0, r2, [r3]
+	ldrex	r2, [r1]
+	orr	r2, r2, r3
+	strex 	r0, r2, [r1]
 	cmp	r0, #0
 	bne 1b
 
+2:
 	/* Enable coherency on CPU - Atomic */
-	add	r3, r3, #ARMADA_XP_CFB_CFG_REG_OFFSET
+	add	r1, r1, #ARMADA_XP_CFB_CFG_REG_OFFSET
 1:
-	ldrex	r2, [r3]
-	orr	r2, r2, r1
-	strex	r0, r2, [r3]
+	ldrex	r2, [r1]
+	orr	r2, r2, r3
+	strex	r0, r2, [r1]
 	cmp	r0, #0
 	bne 1b
 
diff --git a/arch/arm/mach-mvebu/headsmp.S b/arch/arm/mach-mvebu/headsmp.S
index b2c6e95..f2732c8 100644
--- a/arch/arm/mach-mvebu/headsmp.S
+++ b/arch/arm/mach-mvebu/headsmp.S
@@ -30,6 +30,9 @@ ENTRY(armada_xp_secondary_startup)
 	/* Use physical addrss */
 	mov	r0, #0
 
+	/* Add CPU to the SMP group*/
+	mov	r1, #1
+
 	/* Add CPU to coherency fabric */
 	bl	ll_set_cpu_coherent
 	b	secondary_startup
-- 
1.8.1.2

[PATCH v3 05/14] ARM: mvebu: Make ll_set_cpu_coherent() more configurable

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 14 Oct 2013 15:58:17 +0200, Gregory CLEMENT wrote:
> ll_set_cpu_coherent does two things to set the coherency for a CPU:
> 
> - Adding the CPU to the SMP group 0
> - Enabling the snooping on the CPU
> 
> Only the second part is needed when coming back from a CPU Idle. This
> commit allows to choose to do the first part or not.

I am not a fan of this change: those two boolean arguments are really
mystical. If a function does do things, and sometimes must be called to
do one thing, and sometimes for two things, it is really calling for a
split of the functions (with potentially the second function calling the
first, or the two functions being called in a row when needed).

Let's have two functions:

	ll_set_cpu_smp_group();
	ll_set_cpu_coherent();

at initialization time, both functions are called. On the exit path of
idle, only the second one is called.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 06/14] ARM: mvebu: Low level functions to disable cache snooping

[Gregory CLEMENT]

When going to deep idle we need to disable the SoC snooping by
"hand". Playing with the coherency fabric requires to use assembly
code to be sure that the compiler doesn't reorder the instructions nor
do wrong optimization.

This function will be called by the low level (in assembly) part of
the CPU idle functions.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/coherency_ll.S | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S
index 1526b94..3fb426e 100644
--- a/arch/arm/mach-mvebu/coherency_ll.S
+++ b/arch/arm/mach-mvebu/coherency_ll.S
@@ -73,6 +73,28 @@ ENTRY(ll_set_cpu_coherent)
 	mov	pc, lr
 ENDPROC(ll_set_cpu_coherent)
 
+/*
+ * r0: if r0==0 => physical addres, else virtual address
+ */
+ENTRY(armada_370_xp_disable_snoop_ena)
+	ldr	r0, =(coherency_base)
+	ldr	r0, [r0]
+	/* Enable SnoopEna - Exclusive */
+	mrc	15, 0, r1, cr0, cr0, 5
+	and	r1, r1, #15
+	mov	r2, #(1 << 24)
+	lsl	r2, r2, r1
+
+1:
+	ldrex	r1, [r0]
+	bic	r1, r1, r2
+	strex	r3, r1, [r0]
+	cmp	r3, #0
+	bne 1b
+
+	mov pc, lr
+ENDPROC(armada_370_xp_disable_snoop_ena)
+
 	.align 2
 3:
-	.long	coherency_phys_base - .
+		.long	coherency_phys_base - .
-- 
1.8.1.2

[PATCH v3 06/14] ARM: mvebu: Low level functions to disable cache snooping

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 14 Oct 2013 15:58:18 +0200, Gregory CLEMENT wrote:
> When going to deep idle we need to disable the SoC snooping by
> "hand". Playing with the coherency fabric requires to use assembly
> code to be sure that the compiler doesn't reorder the instructions nor
> do wrong optimization.
> 
> This function will be called by the low level (in assembly) part of
> the CPU idle functions.

So, this is this opposite of adding the CPU into the coherency fabric,
as is done in ll_set_cpu_coherent(), right? If that's the case, then
the function should be named to be symmetrical with
ll_set_cpu_coherent(), i.e something like ll_set_cpu_uncoherent(), or
rename the two functions to ll_coherency_fabric_register() /
ll_coherency_fabric_unregister() or something.

> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/coherency_ll.S | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S
> index 1526b94..3fb426e 100644
> --- a/arch/arm/mach-mvebu/coherency_ll.S
> +++ b/arch/arm/mach-mvebu/coherency_ll.S
> @@ -73,6 +73,28 @@ ENTRY(ll_set_cpu_coherent)
>  	mov	pc, lr
>  ENDPROC(ll_set_cpu_coherent)
>  
> +/*
> + * r0: if r0==0 => physical addres, else virtual address

addres -> address

Address of what?

> + */
> +ENTRY(armada_370_xp_disable_snoop_ena)
> +	ldr	r0, =(coherency_base)

So it takes an argument that tells whether it should use a physical or
a virtual address, but at the first instruction you overwrite r0. Seems
like the function assumes it's called with the MMU enabled, and the
comment about the argument is wrong.

> +	ldr	r0, [r0]
> +	/* Enable SnoopEna - Exclusive */

An empty new line before the comment would be useful, but honestly I
don't see the relation with the comment. The function is about
disabling the snooping (i.e removing the CPU from the coherency
fabric), but the comment says it enables snooping. Not clear.

> +	mrc	15, 0, r1, cr0, cr0, 5
> +	and	r1, r1, #15
> +	mov	r2, #(1 << 24)
> +	lsl	r2, r2, r1

Some more comment here would be useful: we're reading the current CPU
hardware ID, and creating the bitfield that we need to remove this CPU
from the coherency fabric.

> +
> +1:
> +	ldrex	r1, [r0]
> +	bic	r1, r1, r2
> +	strex	r3, r1, [r0]
> +	cmp	r3, #0
> +	bne 1b

And here we actually remove it from the coherency fabric, with a loop
needed to make sure we don't get cheated by other CPUs doing the same
thing at the same time.

> +
> +	mov pc, lr
> +ENDPROC(armada_370_xp_disable_snoop_ena)
> +
>  	.align 2
>  3:
> -	.long	coherency_phys_base - .
> +		.long	coherency_phys_base - .

This change is unrelated and unnecessary.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 07/14] ARM: mvebu: Add a new set of registers for pmsu

[Gregory CLEMENT]

The Power Management Unit Service block also controls the Coherency
Fabric subsystem. This new set of registers is needed for the CPU idle
implementation for the Armada 370/XP, it allows to enter a deep CPU
idle state where the Coherency Fabric and the L2 cache are power down.

This patch also adds warnings if one of the base registers set can't
be ioremapped.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/pmsu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index 27fc4f0..f6b00fb 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -25,6 +25,7 @@
 
 static void __iomem *pmsu_mp_base;
 static void __iomem *pmsu_reset_base;
+static void __iomem *pmsu_fabric_base;
 
 #define PMSU_BOOT_ADDR_REDIRECT_OFFSET(cpu)	((cpu * 0x100) + 0x24)
 #define PMSU_RESET_CTL_OFFSET(cpu)		(cpu * 0x8)
@@ -66,7 +67,11 @@ int __init armada_370_xp_pmsu_init(void)
 	if (np) {
 		pr_info("Initializing Power Management Service Unit\n");
 		pmsu_mp_base = of_iomap(np, 0);
+		WARN_ON(!pmsu_mp_base);
 		pmsu_reset_base = of_iomap(np, 1);
+		WARN_ON(!pmsu_reset_base);
+		pmsu_fabric_base = of_iomap(np, 2);
+		WARN_ON(!pmsu_fabric_base);
 		of_node_put(np);
 	}
 
-- 
1.8.1.2

[PATCH v3 07/14] ARM: mvebu: Add a new set of registers for pmsu

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 14 Oct 2013 15:58:19 +0200, Gregory CLEMENT wrote:
> The Power Management Unit Service block also controls the Coherency
> Fabric subsystem. This new set of registers is needed for the CPU idle
> implementation for the Armada 370/XP, it allows to enter a deep CPU
> idle state where the Coherency Fabric and the L2 cache are power down.

power -> powered

> This patch also adds warnings if one of the base registers set can't
> be ioremapped.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

I believe the Device Tree binding documentation should be updated in
this patch (which actually changes the driver offering the binding),
rather than in PATCH 14/14 (which updates the particular DT of a given
platform to use this new binding).

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 07/14] ARM: mvebu: Add a new set of registers for pmsu

[Jason Cooper]

On Mon, Oct 14, 2013 at 04:34:49PM +0200, Thomas Petazzoni wrote:
...
> I believe the Device Tree binding documentation should be updated in
> this patch (which actually changes the driver offering the binding),
> rather than in PATCH 14/14 (which updates the particular DT of a given
> platform to use this new binding).

Agreed.

thx,

Jason.

[PATCH v3 08/14] ARM: mvebu: Allow to power down L2 cache controller in idle mode

[Gregory CLEMENT]

This commit adds an exported function which allows to power down the
L2 cache controller and the Coherency Fabric when the CPU goes into
deep idle mode.

This feature is part of the Power Management Service Unit of the
Armada 370 and Armada XP SoCs.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/pmsu.c         | 14 ++++++++++++++
 include/linux/armada-370-xp-pmsu.h | 16 ++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100644 include/linux/armada-370-xp-pmsu.h

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index f6b00fb..445c9a0 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -30,6 +30,9 @@ static void __iomem *pmsu_fabric_base;
 #define PMSU_BOOT_ADDR_REDIRECT_OFFSET(cpu)	((cpu * 0x100) + 0x24)
 #define PMSU_RESET_CTL_OFFSET(cpu)		(cpu * 0x8)
 
+#define L2C_NFABRIC_PM_CTL		    0x4
+#define L2C_NFABRIC_PM_CTL_PWR_DOWN	    BIT(20)
+
 static struct of_device_id of_pmsu_table[] = {
 	{.compatible = "marvell,armada-370-xp-pmsu"},
 	{ /* end of list */ },
@@ -78,4 +81,15 @@ int __init armada_370_xp_pmsu_init(void)
 	return 0;
 }
 
+void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void)
+{
+	int reg;
+
+	/* Enable L2 & Fabric powerdown in Deep-Idle mode - Fabric */
+	reg = readl(pmsu_fabric_base + L2C_NFABRIC_PM_CTL);
+	reg |= L2C_NFABRIC_PM_CTL_PWR_DOWN;
+	writel(reg, pmsu_fabric_base + L2C_NFABRIC_PM_CTL);
+}
+EXPORT_SYMBOL_GPL(armada_370_xp_pmsu_enable_l2_powerdown_onidle);
+
 early_initcall(armada_370_xp_pmsu_init);
diff --git a/include/linux/armada-370-xp-pmsu.h b/include/linux/armada-370-xp-pmsu.h
new file mode 100644
index 0000000..f85cbf7
--- /dev/null
+++ b/include/linux/armada-370-xp-pmsu.h
@@ -0,0 +1,16 @@
+/*
+ * Power Management Service Unit (PMSU) support for Armada 370/XP platforms.
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * 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.
+ */
+
+#ifndef __ARMADA_370_XP__PMSU_H
+#define __ARMADA_370_XP__PMSU_H
+
+void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void);
+
+#endif	/* __ARMADA_370_XP__PMSU_H */
-- 
1.8.1.2

[PATCH v3 08/14] ARM: mvebu: Allow to power down L2 cache controller in idle mode

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 14 Oct 2013 15:58:20 +0200, Gregory CLEMENT wrote:
> This commit adds an exported function which allows to power down the
> L2 cache controller and the Coherency Fabric when the CPU goes into
> deep idle mode.

This comment is slightly misleading I believe: it explains that this
function should be called every time you want to power down the L2
cache and the coherency fabric. However, looking at the cpuidle driver
code, it seems that this function only ever gets called in the
->probe() function, and what it really does is that it adjusts the PMSU
configuration to automatically power down the L2 and coherency fabric
when we enter a certain idle state.

> This feature is part of the Power Management Service Unit of the
> Armada 370 and Armada XP SoCs.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/pmsu.c         | 14 ++++++++++++++
>  include/linux/armada-370-xp-pmsu.h | 16 ++++++++++++++++
>  2 files changed, 30 insertions(+)
>  create mode 100644 include/linux/armada-370-xp-pmsu.h
> 
> diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
> index f6b00fb..445c9a0 100644
> --- a/arch/arm/mach-mvebu/pmsu.c
> +++ b/arch/arm/mach-mvebu/pmsu.c
> @@ -30,6 +30,9 @@ static void __iomem *pmsu_fabric_base;
>  #define PMSU_BOOT_ADDR_REDIRECT_OFFSET(cpu)	((cpu * 0x100) + 0x24)
>  #define PMSU_RESET_CTL_OFFSET(cpu)		(cpu * 0x8)
>  
> +#define L2C_NFABRIC_PM_CTL		    0x4
> +#define L2C_NFABRIC_PM_CTL_PWR_DOWN	    BIT(20)
> +
>  static struct of_device_id of_pmsu_table[] = {
>  	{.compatible = "marvell,armada-370-xp-pmsu"},
>  	{ /* end of list */ },
> @@ -78,4 +81,15 @@ int __init armada_370_xp_pmsu_init(void)
>  	return 0;
>  }
>  
> +void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void)
> +{
> +	int reg;

u32 ?

> +
> +	/* Enable L2 & Fabric powerdown in Deep-Idle mode - Fabric */
> +	reg = readl(pmsu_fabric_base + L2C_NFABRIC_PM_CTL);
> +	reg |= L2C_NFABRIC_PM_CTL_PWR_DOWN;
> +	writel(reg, pmsu_fabric_base + L2C_NFABRIC_PM_CTL);
> +}
> +EXPORT_SYMBOL_GPL(armada_370_xp_pmsu_enable_l2_powerdown_onidle);

We don't need to have thiS EXPORT_SYMBOL_GPL. The cpuidle driver has a
'bool' Kconfig option, so it will also be built into the kernel and
never be built as a module.

I am also wondering if it is really useful to expose this function. If
it's just called at initialization, why not do it in the mach-mvebu
code directly, before registering the cpuidle driver? (I'm not sure
about this one, just proposing, it makes one less SoC-specific function
exposed to the world).

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 09/14] ARM: mvebu: Add the PMSU related part of the cpu idle functions

[Gregory CLEMENT]

The cpu idle support will need to access to the Power Management
Service Unit. This commit adds and exports the prepare and restore
functions that will be used by the CPU idle.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/pmsu.c         | 73 ++++++++++++++++++++++++++++++++++++++
 include/linux/armada-370-xp-pmsu.h |  2 ++
 2 files changed, 75 insertions(+)

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index 445c9a0..d0a02bc 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -30,6 +30,24 @@ static void __iomem *pmsu_fabric_base;
 #define PMSU_BOOT_ADDR_REDIRECT_OFFSET(cpu)	((cpu * 0x100) + 0x24)
 #define PMSU_RESET_CTL_OFFSET(cpu)		(cpu * 0x8)
 
+#define PM_CONTROL_AND_CONFIG(cpu)	((cpu * 0x100) + 0x4)
+#define PM_CONTROL_AND_CONFIG_DFS_REQ		BIT(18)
+#define PM_CONTROL_AND_CONFIG_PWDDN_REQ	BIT(16)
+#define PM_CONTROL_AND_CONFIG_L2_PWDDN		BIT(20)
+
+#define PM_CPU_POWER_DOWN_CONTROL(cpu)	((cpu * 0x100) + 0x8)
+
+#define PM_CPU_POWER_DOWN_DIS_SNP_Q_SKIP	BIT(0)
+
+#define PM_STATUS_AND_MASK(cpu)	((cpu * 0x100) + 0xc)
+#define PM_STATUS_AND_MASK_CPU_IDLE_WAIT	BIT(16)
+#define PM_STATUS_AND_MASK_SNP_Q_EMPTY_WAIT	BIT(17)
+#define PM_STATUS_AND_MASK_IRQ_WAKEUP		BIT(20)
+#define PM_STATUS_AND_MASK_FIQ_WAKEUP		BIT(21)
+#define PM_STATUS_AND_MASK_DBG_WAKEUP		BIT(22)
+#define PM_STATUS_AND_MASK_IRQ_MASK		BIT(24)
+#define PM_STATUS_AND_MASK_FIQ_MASK		BIT(25)
+
 #define L2C_NFABRIC_PM_CTL		    0x4
 #define L2C_NFABRIC_PM_CTL_PWR_DOWN	    BIT(20)
 
@@ -92,4 +110,59 @@ void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void)
 }
 EXPORT_SYMBOL_GPL(armada_370_xp_pmsu_enable_l2_powerdown_onidle);
 
+void armada_370_xp_pmsu_idle_prepare(bool deepidle)
+{
+	unsigned int hw_cpu = cpu_logical_map(smp_processor_id());
+	int reg;
+	/*
+	 * Adjust the PMSU configuration to wait for WFI signal, enable
+	 * IRQ and FIQ as wakeup events, set wait for snoop queue empty
+	 * indication and mask IRQ and FIQ from CPU
+	 */
+	reg = readl(pmsu_mp_base + PM_STATUS_AND_MASK(hw_cpu));
+	reg |= PM_STATUS_AND_MASK_CPU_IDLE_WAIT    |
+	       PM_STATUS_AND_MASK_IRQ_WAKEUP       |
+	       PM_STATUS_AND_MASK_FIQ_WAKEUP       |
+	       PM_STATUS_AND_MASK_SNP_Q_EMPTY_WAIT |
+	       PM_STATUS_AND_MASK_IRQ_MASK         |
+	       PM_STATUS_AND_MASK_FIQ_MASK;
+	writel(reg, pmsu_mp_base + PM_STATUS_AND_MASK(hw_cpu));
+
+	reg = readl(pmsu_mp_base + PM_CONTROL_AND_CONFIG(hw_cpu));
+	/* ask HW to power down the L2 Cache if needed */
+	if (deepidle)
+		reg |= PM_CONTROL_AND_CONFIG_L2_PWDDN;
+
+	/* request power down */
+	reg |= PM_CONTROL_AND_CONFIG_PWDDN_REQ;
+	writel(reg, pmsu_mp_base + PM_CONTROL_AND_CONFIG(hw_cpu));
+
+	/* Disable snoop disable by HW - SW is taking care of it */
+	reg = readl(pmsu_mp_base + PM_CPU_POWER_DOWN_CONTROL(hw_cpu));
+	reg |= PM_CPU_POWER_DOWN_DIS_SNP_Q_SKIP;
+	writel(reg, pmsu_mp_base + PM_CPU_POWER_DOWN_CONTROL(hw_cpu));
+}
+EXPORT_SYMBOL_GPL(armada_370_xp_pmsu_idle_prepare);
+
+noinline void armada_370_xp_pmsu_idle_restore(void)
+{
+	unsigned int hw_cpu = cpu_logical_map(smp_processor_id());
+	int reg;
+
+	/* cancel ask HW to power down the L2 Cache if possible */
+	reg = readl(pmsu_mp_base + PM_CONTROL_AND_CONFIG(hw_cpu));
+	reg &= ~PM_CONTROL_AND_CONFIG_L2_PWDDN;
+	writel(reg, pmsu_mp_base + PM_CONTROL_AND_CONFIG(hw_cpu));
+
+	/* cancel Enable wakeup events */
+	reg = readl(pmsu_mp_base + PM_STATUS_AND_MASK(hw_cpu));
+	reg &= ~(PM_STATUS_AND_MASK_IRQ_WAKEUP | PM_STATUS_AND_MASK_FIQ_WAKEUP);
+	reg &= ~PM_STATUS_AND_MASK_CPU_IDLE_WAIT;
+	reg &= ~PM_STATUS_AND_MASK_SNP_Q_EMPTY_WAIT;
+	/* Mask interrupts */
+	reg &= ~(PM_STATUS_AND_MASK_IRQ_MASK | PM_STATUS_AND_MASK_FIQ_MASK);
+	writel(reg, pmsu_mp_base + PM_STATUS_AND_MASK(hw_cpu));
+}
+EXPORT_SYMBOL_GPL(armada_370_xp_pmsu_idle_restore);
+
 early_initcall(armada_370_xp_pmsu_init);
diff --git a/include/linux/armada-370-xp-pmsu.h b/include/linux/armada-370-xp-pmsu.h
index f85cbf7..235a40c 100644
--- a/include/linux/armada-370-xp-pmsu.h
+++ b/include/linux/armada-370-xp-pmsu.h
@@ -12,5 +12,7 @@
 #define __ARMADA_370_XP__PMSU_H
 
 void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void);
+void armada_370_xp_pmsu_idle_prepare(bool deepidle);
+void armada_370_xp_pmsu_idle_restore(void);
 
 #endif	/* __ARMADA_370_XP__PMSU_H */
-- 
1.8.1.2

[PATCH v3 09/14] ARM: mvebu: Add the PMSU related part of the cpu idle functions

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 14 Oct 2013 15:58:21 +0200, Gregory CLEMENT wrote:
> The cpu idle support will need to access to the Power Management

"to the" -> "the"

> Service Unit. This commit adds and exports the prepare and restore
> functions that will be used by the CPU idle.

CPU idle -> CPU idle driver, or "in the idle path of the cpuidle
driver".

> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/pmsu.c         | 73 ++++++++++++++++++++++++++++++++++++++
>  include/linux/armada-370-xp-pmsu.h |  2 ++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
> index 445c9a0..d0a02bc 100644
> --- a/arch/arm/mach-mvebu/pmsu.c
> +++ b/arch/arm/mach-mvebu/pmsu.c
> @@ -30,6 +30,24 @@ static void __iomem *pmsu_fabric_base;
>  #define PMSU_BOOT_ADDR_REDIRECT_OFFSET(cpu)	((cpu * 0x100) + 0x24)
>  #define PMSU_RESET_CTL_OFFSET(cpu)		(cpu * 0x8)
>  
> +#define PM_CONTROL_AND_CONFIG(cpu)	((cpu * 0x100) + 0x4)
> +#define PM_CONTROL_AND_CONFIG_DFS_REQ		BIT(18)
> +#define PM_CONTROL_AND_CONFIG_PWDDN_REQ	BIT(16)
> +#define PM_CONTROL_AND_CONFIG_L2_PWDDN		BIT(20)
> +
> +#define PM_CPU_POWER_DOWN_CONTROL(cpu)	((cpu * 0x100) + 0x8)
> +
> +#define PM_CPU_POWER_DOWN_DIS_SNP_Q_SKIP	BIT(0)
> +
> +#define PM_STATUS_AND_MASK(cpu)	((cpu * 0x100) + 0xc)
> +#define PM_STATUS_AND_MASK_CPU_IDLE_WAIT	BIT(16)
> +#define PM_STATUS_AND_MASK_SNP_Q_EMPTY_WAIT	BIT(17)
> +#define PM_STATUS_AND_MASK_IRQ_WAKEUP		BIT(20)
> +#define PM_STATUS_AND_MASK_FIQ_WAKEUP		BIT(21)
> +#define PM_STATUS_AND_MASK_DBG_WAKEUP		BIT(22)
> +#define PM_STATUS_AND_MASK_IRQ_MASK		BIT(24)
> +#define PM_STATUS_AND_MASK_FIQ_MASK		BIT(25)

The other registers are prefixed with PMSU_<something>, why not these
ones? Also, keeping them in increasing offset order seems like a good
idea, so they should be before the PMSU_BOOT_ADDR_REDIRECT_OFFSET
register, which is a cpu * 0x100 + 0x24.

(Yeah, I'm in a nitpicking-mood).

>  #define L2C_NFABRIC_PM_CTL		    0x4
>  #define L2C_NFABRIC_PM_CTL_PWR_DOWN	    BIT(20)
>  
> @@ -92,4 +110,59 @@ void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void)
>  }
>  EXPORT_SYMBOL_GPL(armada_370_xp_pmsu_enable_l2_powerdown_onidle);
>  
> +void armada_370_xp_pmsu_idle_prepare(bool deepidle)
> +{
> +	unsigned int hw_cpu = cpu_logical_map(smp_processor_id());
> +	int reg;
> +	/*

One empty line between variables and the first line of code or comment.

> +	 * Adjust the PMSU configuration to wait for WFI signal, enable
> +	 * IRQ and FIQ as wakeup events, set wait for snoop queue empty
> +	 * indication and mask IRQ and FIQ from CPU
> +	 */
> +	reg = readl(pmsu_mp_base + PM_STATUS_AND_MASK(hw_cpu));
> +	reg |= PM_STATUS_AND_MASK_CPU_IDLE_WAIT    |
> +	       PM_STATUS_AND_MASK_IRQ_WAKEUP       |
> +	       PM_STATUS_AND_MASK_FIQ_WAKEUP       |
> +	       PM_STATUS_AND_MASK_SNP_Q_EMPTY_WAIT |
> +	       PM_STATUS_AND_MASK_IRQ_MASK         |
> +	       PM_STATUS_AND_MASK_FIQ_MASK;
> +	writel(reg, pmsu_mp_base + PM_STATUS_AND_MASK(hw_cpu));
> +
> +	reg = readl(pmsu_mp_base + PM_CONTROL_AND_CONFIG(hw_cpu));
> +	/* ask HW to power down the L2 Cache if needed */
> +	if (deepidle)
> +		reg |= PM_CONTROL_AND_CONFIG_L2_PWDDN;
> +
> +	/* request power down */
> +	reg |= PM_CONTROL_AND_CONFIG_PWDDN_REQ;
> +	writel(reg, pmsu_mp_base + PM_CONTROL_AND_CONFIG(hw_cpu));
> +
> +	/* Disable snoop disable by HW - SW is taking care of it */
> +	reg = readl(pmsu_mp_base + PM_CPU_POWER_DOWN_CONTROL(hw_cpu));
> +	reg |= PM_CPU_POWER_DOWN_DIS_SNP_Q_SKIP;
> +	writel(reg, pmsu_mp_base + PM_CPU_POWER_DOWN_CONTROL(hw_cpu));
> +}
> +EXPORT_SYMBOL_GPL(armada_370_xp_pmsu_idle_prepare);

No need for the EXPORT_SYMBOL_GPL() (cpuidle driver will always be
statically compiled).

> +noinline void armada_370_xp_pmsu_idle_restore(void)
> +{
> +	unsigned int hw_cpu = cpu_logical_map(smp_processor_id());
> +	int reg;
> +
> +	/* cancel ask HW to power down the L2 Cache if possible */
> +	reg = readl(pmsu_mp_base + PM_CONTROL_AND_CONFIG(hw_cpu));
> +	reg &= ~PM_CONTROL_AND_CONFIG_L2_PWDDN;
> +	writel(reg, pmsu_mp_base + PM_CONTROL_AND_CONFIG(hw_cpu));
> +
> +	/* cancel Enable wakeup events */
> +	reg = readl(pmsu_mp_base + PM_STATUS_AND_MASK(hw_cpu));
> +	reg &= ~(PM_STATUS_AND_MASK_IRQ_WAKEUP | PM_STATUS_AND_MASK_FIQ_WAKEUP);
> +	reg &= ~PM_STATUS_AND_MASK_CPU_IDLE_WAIT;
> +	reg &= ~PM_STATUS_AND_MASK_SNP_Q_EMPTY_WAIT;
> +	/* Mask interrupts */

Move this comment up with the previous comment, to keep the strategy of
one comment for each read/modify/write cycle.

Also, maybe a comment on top of those functions mentioning that no
locking is needed because they only access per-CPU registers.

> +	reg &= ~(PM_STATUS_AND_MASK_IRQ_MASK | PM_STATUS_AND_MASK_FIQ_MASK);
> +	writel(reg, pmsu_mp_base + PM_STATUS_AND_MASK(hw_cpu));
> +}
> +EXPORT_SYMBOL_GPL(armada_370_xp_pmsu_idle_restore);

No need for the EXPORT_SYMBOL_GPL() (cpuidle driver will always be
statically compiled).

> +
>  early_initcall(armada_370_xp_pmsu_init);
> diff --git a/include/linux/armada-370-xp-pmsu.h b/include/linux/armada-370-xp-pmsu.h
> index f85cbf7..235a40c 100644
> --- a/include/linux/armada-370-xp-pmsu.h
> +++ b/include/linux/armada-370-xp-pmsu.h
> @@ -12,5 +12,7 @@
>  #define __ARMADA_370_XP__PMSU_H
>  
>  void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void);
> +void armada_370_xp_pmsu_idle_prepare(bool deepidle);
> +void armada_370_xp_pmsu_idle_restore(void);
>  
>  #endif	/* __ARMADA_370_XP__PMSU_H */

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 10/14] ARM: mvebu: Set the start address of a CPU in a separate function

[Gregory CLEMENT]

Setting the start (or boot) address of A CPU is no more used only
during SMP bring up, but will also be used by the CPU idle functions
or later by the CPU hot plug ones.

This commit moves it in a separate function and exports it.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/pmsu.c         | 10 ++++++++--
 include/linux/armada-370-xp-pmsu.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index d0a02bc..e394f08 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -56,6 +56,13 @@ static struct of_device_id of_pmsu_table[] = {
 	{ /* end of list */ },
 };
 
+void armada_370_xp_pmsu_set_start_addr(void *start_addr, int hw_cpu)
+{
+	writel(virt_to_phys(start_addr), pmsu_mp_base +
+		PMSU_BOOT_ADDR_REDIRECT_OFFSET(hw_cpu));
+}
+EXPORT_SYMBOL_GPL(armada_370_xp_pmsu_set_start_addr);
+
 #ifdef CONFIG_SMP
 int armada_xp_boot_cpu(unsigned int cpu_id, void *boot_addr)
 {
@@ -68,8 +75,7 @@ int armada_xp_boot_cpu(unsigned int cpu_id, void *boot_addr)
 
 	hw_cpu = cpu_logical_map(cpu_id);
 
-	writel(virt_to_phys(boot_addr), pmsu_mp_base +
-			PMSU_BOOT_ADDR_REDIRECT_OFFSET(hw_cpu));
+	armada_370_xp_pmsu_set_start_addr(boot_addr, hw_cpu);
 
 	/* Release CPU from reset by clearing reset bit*/
 	reg = readl(pmsu_reset_base + PMSU_RESET_CTL_OFFSET(hw_cpu));
diff --git a/include/linux/armada-370-xp-pmsu.h b/include/linux/armada-370-xp-pmsu.h
index 235a40c..e09977b 100644
--- a/include/linux/armada-370-xp-pmsu.h
+++ b/include/linux/armada-370-xp-pmsu.h
@@ -14,5 +14,6 @@
 void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void);
 void armada_370_xp_pmsu_idle_prepare(bool deepidle);
 void armada_370_xp_pmsu_idle_restore(void);
+void armada_370_xp_pmsu_set_start_addr(void *start_addr, int hw_cpu);
 
 #endif	/* __ARMADA_370_XP__PMSU_H */
-- 
1.8.1.2

[PATCH v3 11/14] ARM: mvebu: Add CPU idle low level support for Marvell Armada XP

[Gregory CLEMENT]

This commit adds the low level implementation of CPU Idle.
Currently only Armada XP is supported, but the support
will be extended for Armada 370.

Based on the work of Nadav Haklai.

Signed-off-by: Nadav Haklai <nadavh@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/Makefile                |  1 +
 arch/arm/mach-mvebu/suspend-armada-370-xp.S | 90 +++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)
 create mode 100644 arch/arm/mach-mvebu/suspend-armada-370-xp.S

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 2d04f0e..9cd2705 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o
 obj-$(CONFIG_ARCH_MVEBU)	 += coherency.o coherency_ll.o pmsu.o
 obj-$(CONFIG_SMP)                += platsmp.o headsmp.o
 obj-$(CONFIG_HOTPLUG_CPU)        += hotplug.o
+obj-$(CONFIG_ARM_ARMADA_370_XP_CPUIDLE) += suspend-armada-370-xp.o
diff --git a/arch/arm/mach-mvebu/suspend-armada-370-xp.S b/arch/arm/mach-mvebu/suspend-armada-370-xp.S
new file mode 100644
index 0000000..5f01a68
--- /dev/null
+++ b/arch/arm/mach-mvebu/suspend-armada-370-xp.S
@@ -0,0 +1,90 @@
+/*
+ * CPU idle low level implementation for Marvell Armada 370 and Armada XP SoCs
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * Nadav Haklai <nadavh@marvell.com>
+ *
+ * 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
+	 * 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
+
+	/* Data memory barrier and Data sync barrier */
+	dsb
+	dmb
+
+	bl armada_370_xp_disable_snoop_ena
+
+	dsb				@ Data Synchronization Barrier
+
+/*
+ * ===================================
+ * == WFI instruction => Enter idle ==
+ * ===================================
+ */
+
+	wfi				@ wait for interrupt
+/*
+ * ===================================
+ * == 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
+*/
+ENTRY(armada_370_xp_cpu_resume)
+	mov	r0, #0			@ r0==0 means use physical address
+	mov	r1, #1			@ Add CPU to SMP group
+	bl ll_set_cpu_coherent
+
+	/* Now branch to the common CPU resume function */
+	b	cpu_resume
+
+ENDPROC(armada_370_xp_cpu_resume)
-- 
1.8.1.2

[PATCH v3 11/14] ARM: mvebu: Add CPU idle low level support for Marvell Armada XP

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 14 Oct 2013 15:58:23 +0200, Gregory CLEMENT wrote:
> This commit adds the low level implementation of CPU Idle.
> Currently only Armada XP is supported, but the support
> will be extended for Armada 370.
> 
> Based on the work of Nadav Haklai.
> 
> Signed-off-by: Nadav Haklai <nadavh@marvell.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/Makefile                |  1 +
>  arch/arm/mach-mvebu/suspend-armada-370-xp.S | 90 +++++++++++++++++++++++++++++

I know Daniel rejected this file from being part of drivers/cpuidle/,
but this seems weird to me. We're trying to put as much driver code as
possible to the respective drivers/ directory, and this code is really
part of the cpuidle driver itself. I know there isn't a single .S file
for now in drivers/cpuidle, but at least the cpuidle-calxeda.c file
already has some inline assembly statements.

>  2 files changed, 91 insertions(+)
>  create mode 100644 arch/arm/mach-mvebu/suspend-armada-370-xp.S
> 
> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index 2d04f0e..9cd2705 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o
>  obj-$(CONFIG_ARCH_MVEBU)	 += coherency.o coherency_ll.o pmsu.o
>  obj-$(CONFIG_SMP)                += platsmp.o headsmp.o
>  obj-$(CONFIG_HOTPLUG_CPU)        += hotplug.o
> +obj-$(CONFIG_ARM_ARMADA_370_XP_CPUIDLE) += suspend-armada-370-xp.o

I find it weird to have a Kconfig option from drivers/cpuidle/
affecting the build of things in arch/arm/mach-mvebu/, no?

> diff --git a/arch/arm/mach-mvebu/suspend-armada-370-xp.S b/arch/arm/mach-mvebu/suspend-armada-370-xp.S
> new file mode 100644
> index 0000000..5f01a68
> --- /dev/null
> +++ b/arch/arm/mach-mvebu/suspend-armada-370-xp.S
> @@ -0,0 +1,90 @@
> +/*
> + * CPU idle low level implementation for Marvell Armada 370 and Armada XP SoCs

But the Armada 370 implementation is not there yet, maybe worth
updating the comment to mention that (but keep the indication that
Armada 370 support should come, otherwise the name of the source file
seems a bit strange).

> + *
> + * Copyright (C) 2013 Marvell
> + *
> + * Nadav Haklai <nadavh@marvell.com>
> + *
> + * 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

The name of the function in this comment doesn't match the name of the
function you've chosen below. Please don't re-use vendor code without
checking and rewording the comments as appropriate.

> +* input:
> +*/
> +ENTRY(armada_370_xp_cpu_suspend)

To me "cpu_suspend" sounds like "suspend/resume", not like cpuidle. Is
this routine also going to be used during a suspend/resume cycle, which
would explain its name?

> +/* Save ARM registers */
> +	stmfd	sp!, {r4 - r11, lr}	@ save registers on stack

The first comment is badly aligned, and duplicates the comment after
the @. Generally speaking, this file seems to hesitate between using @
and /* ... */ for comments. Make a decision, and stick to it, if
possible.

> +
> +	bl armada_370_xp_pmsu_idle_prepare
> +	/*

I'd like to have one empty line before comments.

> +	 * 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
> +	 * 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
> +
> +	/* Data memory barrier and Data sync barrier */
> +	dsb
> +	dmb

They are not in the order of the comment, and the comment seems quite
useless: instead of saying *what* the code is doing, it should explain
*why* the code is doing this. The fact that a dmb is a Data Memory
Barrier and a dsb is a Data Sync Barrier is already carried by the
instruction names themselves.

> +
> +	bl armada_370_xp_disable_snoop_ena
> +
> +	dsb				@ Data Synchronization Barrier

Same comment as above.

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

This comment doesn't really look like mainline style, but vendor-style.
Please get rid of this, especially since we know what the wfi
instruction is, or just a:

	/* Enter idle now */
	wfi

> +
> +	wfi				@ wait for interrupt
> +/*
> + * ===================================
> + * == Resume path for non-OFF modes ==
> + * ===================================
> + */

Ditto, just something like:

	/*
         * For all non-OFF modes, we're getting out of idle here. For
	 * OFF modes, what happens is ...
	 */

this is a little bit more useful.

> +
> +/* Enable SnoopEna - Exclusive */

Please align comment and code.

> +	mov	r0, #1			@ r0!=0 means use virtual address
> +	mov	r1, #0			@ Do not add CPU to SMP group
> +	bl ll_set_cpu_coherent

As per comments on the previous patches, I'm not happy with this. Just
make ll_set_cpu_coherent only add to the coherency fabric and not to
the SMP group, and make it detect automatically if it needs to use the
virtual address or not.

Also, intend the name of the function with the other arguments of
assembly instructions.

> +/* Re-enable C-bit if needed */

Please align comment and code.

> +	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
> +*/

So I guess this is where we're ending if we enter one of those "OFF"
idle states. Probably worth mentioning it here.

> +ENTRY(armada_370_xp_cpu_resume)
> +	mov	r0, #0			@ r0==0 means use physical address
> +	mov	r1, #1			@ Add CPU to SMP group
> +	bl ll_set_cpu_coherent

Same comment as above + intend the name of the function with the other
arguments of assembly instructions.

> +
> +	/* Now branch to the common CPU resume function */
> +	b	cpu_resume
> +
> +ENDPROC(armada_370_xp_cpu_resume)

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 12/14] cpuidle: mvebu: Add initial CPU idle support for Armada 370/XP SoC

[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 <nadavh@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/cpuidle/Kconfig.arm             |   5 ++
 drivers/cpuidle/Makefile                |   1 +
 drivers/cpuidle/cpuidle-armada-370-xp.c | 117 ++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c

diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 8e36603..071d960 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -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"
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index cea5ef5..395dab6 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -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
 obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
 obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
 obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c
new file mode 100644
index 0000000..3fa005e
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-armada-370-xp.c
@@ -0,0 +1,117 @@
+/*
+ * Marvell Armada 370 and Armada XP SoC cpuidle driver
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * Nadav Haklai <nadavh@marvell.com>
+ * Gregory CLEMENT <gregory.clement@free-electrons.com>
+ *
+ * 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 <gregory.clement@free-electrons.com>
+ */
+
+#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>
+#include <linux/platform_device.h>
+
+#define ARMADA_370_XP_MAX_STATES	3
+#define ARMADA_370_XP_FLAG_DEEP_IDLE	0x10000
+
+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;
+	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 (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE)
+		deepidle = true;
+
+	cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
+
+	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 |
+						ARMADA_370_XP_FLAG_DEEP_IDLE,
+		.name			= "MV CPU DEEP IDLE",
+		.desc			= "CPU and L2 Fabric power down",
+	},
+	.state_count = ARMADA_370_XP_MAX_STATES,
+};
+
+static int armada_370_xp_cpuidle_probe(struct platform_device *pdev)
+{
+	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);
+}
+
+static int armada_370_xp_cpuidle_remove(struct platform_device *pdev)
+{
+	cpuidle_unregister(&armada_370_xp_idle_driver);
+	return 0;
+}
+
+
+static struct platform_driver armada_370_xp_cpuidle_plat_driver = {
+	.driver = {
+		.name = "cpuidle-armada-370-xp",
+		.owner = THIS_MODULE,
+	},
+	.probe = armada_370_xp_cpuidle_probe,
+	.remove = armada_370_xp_cpuidle_remove,
+};
+
+
+module_platform_driver(armada_370_xp_cpuidle_plat_driver);
+
+MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
+MODULE_DESCRIPTION("Armada 370/XP cpu idle driver");
+MODULE_LICENSE("GPL");
-- 
1.8.1.2

[PATCH v3 12/14] cpuidle: mvebu: Add initial CPU idle support for Armada 370/XP SoC

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 14 Oct 2013 15:58:24 +0200, Gregory CLEMENT wrote:
> +#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>
> +#include <linux/platform_device.h>
> +
> +#define ARMADA_370_XP_MAX_STATES	3
> +#define ARMADA_370_XP_FLAG_DEEP_IDLE	0x10000
> +
> +extern void v7_flush_dcache_all(void);

This function is no longer called by the driver, it seems.

> +/* Functions defined in suspend-armada-370-xp.S */
> +int armada_370_xp_cpu_resume(unsigned long);
> +int armada_370_xp_cpu_suspend(unsigned long);

So the PMSU functions have their prototype in a header in <linux/...>,
but not those ones. Why chose one solution for some functions, and
another one for these functions?

> +static int armada_370_xp_enter_idle(struct cpuidle_device *dev,
> +				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 (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE)
> +		deepidle = true;

This could also be written as:

	deepidle = drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE;

which allows to remove the deepidle = false initialization. But I agree
this is rather a matter of taste, so I don't mind if you chose to keep
your implementation.

> +
> +	cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
> +
> +	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 |
> +						ARMADA_370_XP_FLAG_DEEP_IDLE,
> +		.name			= "MV CPU DEEP IDLE",
> +		.desc			= "CPU and L2 Fabric power down",
> +	},
> +	.state_count = ARMADA_370_XP_MAX_STATES,
> +};
> +
> +static int armada_370_xp_cpuidle_probe(struct platform_device *pdev)
> +{
> +	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;

I am wondering if those checks shouldn't be done by the core mach-mvebu
code before registering the cpuidle platform device, and only register
it if all the prerequisites are available.

> +	pr_info("Initializing Armada-XP CPU power management ");

Not sure this message is really useful, but if it is, it should be
using dev_info(), it should not have a trailing space, and it should
probably not be called "CPU power management", but really "cpuidle" or
something like that. "CPU power management" is a very fuzzy term, which
could be confused with cpufreq, for example.

> +
> +	armada_370_xp_pmsu_enable_l2_powerdown_onidle();
> +
> +	return cpuidle_register(&armada_370_xp_idle_driver, NULL);
> +}
> +
> +static int armada_370_xp_cpuidle_remove(struct platform_device *pdev)
> +{
> +	cpuidle_unregister(&armada_370_xp_idle_driver);
> +	return 0;
> +}
> +
> +

One too many empty line here.

> +static struct platform_driver armada_370_xp_cpuidle_plat_driver = {
> +	.driver = {
> +		.name = "cpuidle-armada-370-xp",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = armada_370_xp_cpuidle_probe,
> +	.remove = armada_370_xp_cpuidle_remove,
> +};
> +
> +

One too many empty line here.

> +module_platform_driver(armada_370_xp_cpuidle_plat_driver);
> +
> +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
> +MODULE_DESCRIPTION("Armada 370/XP cpu idle driver");
> +MODULE_LICENSE("GPL");

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 12/14] cpuidle: mvebu: Add initial CPU idle support for Armada 370/XP SoC

[Daniel Lezcano]

On 10/14/2013 03:58 PM, Gregory CLEMENT wrote:
> 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 <nadavh@marvell.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>   drivers/cpuidle/Kconfig.arm             |   5 ++
>   drivers/cpuidle/Makefile                |   1 +
>   drivers/cpuidle/cpuidle-armada-370-xp.c | 117 ++++++++++++++++++++++++++++++++
>   3 files changed, 123 insertions(+)
>   create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c
>
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 8e36603..071d960 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -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"
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index cea5ef5..395dab6 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -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
>   obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
>   obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
>   obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
> diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c
> new file mode 100644
> index 0000000..3fa005e
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c
> @@ -0,0 +1,117 @@
> +/*
> + * Marvell Armada 370 and Armada XP SoC cpuidle driver
> + *
> + * Copyright (C) 2013 Marvell
> + *
> + * Nadav Haklai <nadavh@marvell.com>
> + * Gregory CLEMENT <gregory.clement@free-electrons.com>
> + *
> + * 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 <gregory.clement@free-electrons.com>
> + */
> +
> +#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>
> +#include <linux/platform_device.h>
> +
> +#define ARMADA_370_XP_MAX_STATES	3
> +#define ARMADA_370_XP_FLAG_DEEP_IDLE	0x10000
> +
> +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;
> +	unsigned int hw_cpu = cpu_logical_map(smp_processor_id());
 >
> +	armada_370_xp_pmsu_set_start_addr(armada_370_xp_cpu_resume, hw_cpu);

What about using cpu_pm_enter() here and move the function above to a 
notifier callback for CPU_PM_ENTER inside the marvell pm code ? The code 
will be more encapsulated, no ?

> +
> +	if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE)
> +		deepidle = true;
> +
> +	cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
> +
> +	armada_370_xp_pmsu_idle_restore();

Same comment by using cpu_pm_exit.

> +
> +	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 |
> +						ARMADA_370_XP_FLAG_DEEP_IDLE,
> +		.name			= "MV CPU DEEP IDLE",
> +		.desc			= "CPU and L2 Fabric power down",
> +	},
> +	.state_count = ARMADA_370_XP_MAX_STATES,
> +};
> +
> +static int armada_370_xp_cpuidle_probe(struct platform_device *pdev)
> +{
> +	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();

Isn't possible to move this function inside pmsu.c ? as well as the 
of_find_compatible functions ?

> +
> +	return cpuidle_register(&armada_370_xp_idle_driver, NULL);
> +}
> +
> +static int armada_370_xp_cpuidle_remove(struct platform_device *pdev)
> +{
> +	cpuidle_unregister(&armada_370_xp_idle_driver);
> +	return 0;
> +}
> +
> +
> +static struct platform_driver armada_370_xp_cpuidle_plat_driver = {
> +	.driver = {
> +		.name = "cpuidle-armada-370-xp",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = armada_370_xp_cpuidle_probe,
> +	.remove = armada_370_xp_cpuidle_remove,
> +};
> +
> +
> +module_platform_driver(armada_370_xp_cpuidle_plat_driver);
> +
> +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
> +MODULE_DESCRIPTION("Armada 370/XP cpu idle driver");
> +MODULE_LICENSE("GPL");

The driver is statically compiled in the kernel, removal of the driver 
is pointless.

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

[PATCH v3 13/14] ARM: mvebu: register the cpuidle driver for the Armada XP SoCs

[Gregory CLEMENT]

The cpuidle is a platform driver so we have to register the device
during the initialization of the boards.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/armada-370-xp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index e2acff9..885d511 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -29,6 +29,10 @@
 #include "common.h"
 #include "coherency.h"
 
+static struct platform_device armada_xp_cpuidle_device = {
+	.name = "cpuidle-armada-370-xp",
+};
+
 static void __init armada_370_xp_map_io(void)
 {
 	debug_ll_io_init();
@@ -48,6 +52,8 @@ static void __init armada_370_xp_timer_and_clk_init(void)
 static void __init armada_370_xp_dt_init(void)
 {
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+	if (of_machine_is_compatible("marvell,armadaxp"))
+		platform_device_register(&armada_xp_cpuidle_device);
 }
 
 static const char * const armada_370_xp_dt_compat[] = {
-- 
1.8.1.2

[PATCH v3 14/14] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[Gregory CLEMENT]

The Power Management Unit Service block also controls the Coherency
Fabric subsystem. This new set of registers is needed for the CPU idle
implementation for the Armada XP, it allows to enter in a deep CPU
idle state where the Coherency Fabric and the L2 cache are powerdown.

Cc: devicetree at vger.kernel.org
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt | 12 +++++++-----
 arch/arm/boot/dts/armada-xp.dtsi                             |  2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt b/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt
index 926b4d6..8a9db0c 100644
--- a/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt
+++ b/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt
@@ -7,14 +7,16 @@ Required properties:
 - compatible: "marvell,armada-370-xp-pmsu"
 
 - reg: Should contain PMSU registers location and length. First pair
-  for the per-CPU SW Reset Control registers, second pair for the
-  Power Management Service Unit.
+  for the per-CPU SW Reset Control registers, second pair for the CPU
+  Power Management Service Unit registers, third pair for the Fabric Power
+  Management Service Unit registers.
 
 Example:
 
-armada-370-xp-pmsu at d0022000 {
+armada-370-xp-pmsu at 22000 {
 	compatible = "marvell,armada-370-xp-pmsu";
-	reg = <0xd0022100 0x430>,
-	      <0xd0020800 0x20>;
+	reg = <0x22100 0x430>,
+	      <0x20800 0x20>,
+	      <0x22000 0x24>;
 };
 
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 3058522..25ec8cd 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -48,7 +48,7 @@
 
 			armada-370-xp-pmsu at 22000 {
 				compatible = "marvell,armada-370-xp-pmsu";
-				reg = <0x22100 0x430>, <0x20800 0x20>;
+				reg = <0x22100 0x430>, <0x20800 0x20>, <0x22000 0x24>;
 			};
 
 			serial at 12200 {
-- 
1.8.1.2