[PATCH v4 00/13] CPU idle for Armada XP

[PATCH v4 00/13] 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.

There were many changes done, see the changelog for the details. I
should have managed to comply with all the reviewer wishes.

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

The first patch should go through ARM subsystem and should be taken by
Russell King. As I didn't receive any comment against this patch, I
planed to submit to Russell's patch system.

The 7th patch should also go to mvebu subsystem (and then arm-soc) but
it would be nice to have an Acked-by from on of the device tree
maintainer.

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 whole series is also available in the branch CPU-idle-ArmadaXP-v4
at https://github.com/MISL-EBU-System-SW/mainline-public.git

Thanks,

Changelog:
v3 -> v4:

* factorized the code in coherency_ll.S and make it autodetect as mush
  as possible

* reordered the introduction of the device tree binding

* removed all the EXPORT_SYMBOL_GPL as the driver can only be built
  into the kernel and never be built as a module.

* moved the armada_370_xp_pmsu_enable_l2_powerdown_onidle function in
  armada_370_cp.c file during the initialization of the platform.

* fixed various coding style issue and typos pointed by Thomas

* fixed all the coding issue style, made the comments more coherent
  and add more comment in the suspend-armada-370-xp.S file.

* moved all the device tree related check from
  armada_370_xp_cpuidle_probe to armada_370_xp_dt_init.

* used cpu_pm_enter() instead of directly calling platform code in
  Armada_370_xp_enter_idle.

* convert the sequence to disable the coherency to the one used in
  TC2.

* Rebased on v3.14-rc1

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 (13):
  ARM: PJ4B: Add cpu_suspend/cpu_resume hooks for PJ4B
  ARM: mvebu: remove the address parameter for ll_set_cpu_coherent
  ARM: mvebu: ll_set_cpu_coherent always uses the current CPU
  ARM: mvebu: Remove the unused argument of set_cpu_coherent()
  ARM: mvebu: Low level function to disable HW coherency support
  ARM: mvebu: Add a new set of registers for pmsu
  ARM: dts: mvebu: Add a new set of registers to the PMSU node
  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: Register notifier callback for the cpuidle transition
  cpuidle: mvebu: Add initial CPU idle support for Armada 370/XP SoC
  ARM: mvebu: register the cpuidle driver for the Armada XP SoCs

 .../devicetree/bindings/arm/armada-370-xp-pmsu.txt |  12 +-
 arch/arm/boot/dts/armada-xp.dtsi                   |   2 +-
 arch/arm/mach-mvebu/armada-370-xp.c                |  12 ++
 arch/arm/mach-mvebu/coherency.c                    |  12 +-
 arch/arm/mach-mvebu/coherency.h                    |   2 +-
 arch/arm/mach-mvebu/coherency_ll.S                 |  86 +++++++++---
 arch/arm/mach-mvebu/headsmp.S                      |  15 +--
 arch/arm/mach-mvebu/platsmp.c                      |   2 +-
 arch/arm/mach-mvebu/pmsu.c                         | 147 ++++++++++++++++++++-
 arch/arm/mach-mvebu/pmsu.h                         |   2 +
 arch/arm/mm/proc-v7.S                              |  64 ++++++++-
 drivers/cpuidle/Kconfig.arm                        |   5 +
 drivers/cpuidle/Makefile                           |   1 +
 drivers/cpuidle/cpuidle-armada-370-xp.c            | 120 +++++++++++++++++
 14 files changed, 427 insertions(+), 55 deletions(-)
 create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c

-- 
1.8.1.2

[PATCH v4 01/13] 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 bd1781979a39..11117423a9b4 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -169,9 +169,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 v4 01/13] ARM: PJ4B: Add cpu_suspend/cpu_resume hooks for PJ4B

[Lorenzo Pieralisi]

Hi Gregory,

On Thu, Feb 13, 2014 at 05:33:24PM +0000, Gregory CLEMENT wrote:
> 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 bd1781979a39..11117423a9b4 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -169,9 +169,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

A couple of questions:

1) Do the extra registers ever change after coldboot ?
2) Do you need to restore them before turning the MMU and caches on ?
3) Most of the code is copy'n'paste from v7, is not it possible to reuse
   that code by doing processor specific save/restore and then jump to
   the v7 functions ?

Thanks,
Lorenzo

[PATCH v4 01/13] ARM: PJ4B: Add cpu_suspend/cpu_resume hooks for PJ4B

[Gregory CLEMENT]

On 14/02/2014 17:06, Lorenzo Pieralisi wrote:
> Hi Gregory,
> 
> On Thu, Feb 13, 2014 at 05:33:24PM +0000, Gregory CLEMENT wrote:
>> 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 bd1781979a39..11117423a9b4 100644
>> --- a/arch/arm/mm/proc-v7.S
>> +++ b/arch/arm/mm/proc-v7.S
>> @@ -169,9 +169,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
> 
> A couple of questions:
> 
> 1) Do the extra registers ever change after coldboot ?
> 2) Do you need to restore them before turning the MMU and caches on ?

yes we need it.

> 3) Most of the code is copy'n'paste from v7, is not it possible to reuse
>    that code by doing processor specific save/restore and then jump to
>    the v7 functions ?

Yes it was possible and I have done it in the 5th version I have just sent.

> 
> Thanks,
> Lorenzo
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v4 02/13] ARM: mvebu: remove the address parameter for ll_set_cpu_coherent

[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. This was needed to be able to use either
a virtual or a physical address. This patch add a check of the MMU bit
to choose the accurate address, then the calling function do have pass
this information.

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

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index 4e9d58148ca7..88dd507221fc 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -30,7 +30,7 @@
 #include "coherency.h"
 
 unsigned long coherency_phys_base;
-static void __iomem *coherency_base;
+void __iomem *coherency_base;
 static void __iomem *coherency_cpu_base;
 
 /* Coherency fabric registers */
@@ -44,7 +44,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(unsigned int hw_cpu_id);
 
 int set_cpu_coherent(unsigned int hw_cpu_id, int smp_group_id)
 {
@@ -54,7 +54,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(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 ee7598fe75db..1f2bcd4b5424 100644
--- a/arch/arm/mach-mvebu/coherency_ll.S
+++ b/arch/arm/mach-mvebu/coherency_ll.S
@@ -21,13 +21,27 @@
 #define ARMADA_XP_CFB_CFG_REG_OFFSET 0x4
 
 #include <asm/assembler.h>
+#include <asm/cp15.h>
 
 	.text
 /*
- * r0: Coherency fabric base register address
- * r1: HW CPU id
+ * r0: HW CPU id
  */
 ENTRY(ll_set_cpu_coherent)
+	mrc	p15, 0, r1, c1, c0, 0
+	tst	r1, #CR_M @ Check MMU bit enabled
+	bne	1f
+
+	/* use physical address of the coherency register*/
+	adr	r0, 3f
+	ldr	r3, [r0]
+	ldr	r0, [r0, r3]
+	b	2f
+1:
+	/* use virtual address of the coherency register*/
+	ldr	r0, =coherency_base
+	ldr	r0, [r0]
+2:
 	/* Create bit by cpu index */
 	mov	r3, #(1 << 24)
 	lsl	r1, r3, r1
@@ -56,3 +70,7 @@ ARM_BE8(rev	r1, r1)
 	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 3dd80df428f7..f30bc8d78871 100644
--- a/arch/arm/mach-mvebu/headsmp.S
+++ b/arch/arm/mach-mvebu/headsmp.S
@@ -31,11 +31,6 @@
 ENTRY(armada_xp_secondary_startup)
  ARM_BE8(setend	be )			@ go BE8 if entered LE
 
-	/* Get coherency fabric base physical address */
-	adr	r0, 1f
-	ldr	r1, [r0]
-	ldr	r0, [r0, r1]
-
 	/* Read CPU id */
 	mrc     p15, 0, r1, c0, c0, 5
 	and     r1, r1, #0xF
@@ -45,7 +40,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 v4 02/13] ARM: mvebu: remove the address parameter for ll_set_cpu_coherent

[Thomas Petazzoni]

Gregory,

On Thu, 13 Feb 2014 18:33:25 +0100, 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. This was needed to be able to use either
> a virtual or a physical address. This patch add a check of the MMU bit
> to choose the accurate address, then the calling function do have pass
> this information.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

I think your commit log needs some review:

I don't understand "This commit doesn't expose anymore this address."
from your commit log. Do you mean that the address is no longer passed
as argument to ll_set_cpu_coherent() ?

Similarly, the sentence "This patch add a check of the MMU bit to choose
the accurate address, then the calling function do have pass this
information." seems incorrect, as the commit does quite the opposite:
the calling function do *NOT* have to pass the base address of the
coherency.

Finally, your commit log doesn't explain *why* this change is
necessary. At first sight, switching from using a function argument to
a global variable looks like a step in the wrong direction. An
explanation as to why this is the right direction seems needed here.

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

[PATCH v4 03/13] 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 | 12 ++++++------
 arch/arm/mach-mvebu/headsmp.S      |  4 ----
 arch/arm/mach-mvebu/platsmp.c      |  2 +-
 5 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index 88dd507221fc..51010dbbf7e4 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -44,17 +44,17 @@ static struct of_device_id of_coherency_table[] = {
 };
 
 /* Function defined in coherency_ll.S */
-int ll_set_cpu_coherent(unsigned int hw_cpu_id);
+int ll_set_cpu_coherent(void);
 
-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(hw_cpu_id);
+	return ll_set_cpu_coherent();
 }
 
 static inline void mvebu_hwcc_sync_io_barrier(void)
@@ -140,7 +140,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 760226c41353..c7e5df368d98 100644
--- a/arch/arm/mach-mvebu/coherency.h
+++ b/arch/arm/mach-mvebu/coherency.h
@@ -16,7 +16,7 @@
 
 extern unsigned long coherency_phys_base;
 
-int set_cpu_coherent(unsigned 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 1f2bcd4b5424..6cb26b919787 100644
--- a/arch/arm/mach-mvebu/coherency_ll.S
+++ b/arch/arm/mach-mvebu/coherency_ll.S
@@ -24,9 +24,7 @@
 #include <asm/cp15.h>
 
 	.text
-/*
- * r0: HW CPU id
- */
+
 ENTRY(ll_set_cpu_coherent)
 	mrc	p15, 0, r1, c1, c0, 0
 	tst	r1, #CR_M @ Check MMU bit enabled
@@ -43,9 +41,11 @@ ENTRY(ll_set_cpu_coherent)
 	ldr	r0, [r0]
 2:
 	/* Create bit by cpu index */
-	mov	r3, #(1 << 24)
-	lsl	r1, r3, r1
-ARM_BE8(rev	r1, r1)
+	mrc	15, 0, r1, cr0, cr0, 5
+	and	r1, r1, #15
+	mov	r2, #(1 << 24)
+	lsl	r1, r2, r1
+	ARM_BE8(rev	r1, 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 f30bc8d78871..cf7abe6554f7 100644
--- a/arch/arm/mach-mvebu/headsmp.S
+++ b/arch/arm/mach-mvebu/headsmp.S
@@ -31,10 +31,6 @@
 ENTRY(armada_xp_secondary_startup)
  ARM_BE8(setend	be )			@ go BE8 if entered LE
 
-	/* 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 a6da03f5b24e..a99d71a747f0 100644
--- a/arch/arm/mach-mvebu/platsmp.c
+++ b/arch/arm/mach-mvebu/platsmp.c
@@ -102,7 +102,7 @@ static 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 v4 03/13] ARM: mvebu: ll_set_cpu_coherent always uses the current CPU

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Thu, 13 Feb 2014 18:33:26 +0100, 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>
> ---
>  arch/arm/mach-mvebu/coherency.c    | 10 +++++-----
>  arch/arm/mach-mvebu/coherency.h    |  2 +-
>  arch/arm/mach-mvebu/coherency_ll.S | 12 ++++++------
>  arch/arm/mach-mvebu/headsmp.S      |  4 ----
>  arch/arm/mach-mvebu/platsmp.c      |  2 +-
>  5 files changed, 13 insertions(+), 17 deletions(-)

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

with just one thing I am not entirely sure about:

>  	/* Create bit by cpu index */
> -	mov	r3, #(1 << 24)
> -	lsl	r1, r3, r1
> -ARM_BE8(rev	r1, r1)
> +	mrc	15, 0, r1, cr0, cr0, 5
> +	and	r1, r1, #15
> +	mov	r2, #(1 << 24)
> +	lsl	r1, r2, r1
> +	ARM_BE8(rev	r1, r1)

Have you tested this in a big endian configuration? I'd like to make
sure we have gotten the ARM_BE8(rev ...) thing correct.

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

[PATCH v4 03/13] ARM: mvebu: ll_set_cpu_coherent always uses the current CPU

[Gregory CLEMENT]

On 19/02/2014 17:09, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Thu, 13 Feb 2014 18:33:26 +0100, 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>
>> ---
>>  arch/arm/mach-mvebu/coherency.c    | 10 +++++-----
>>  arch/arm/mach-mvebu/coherency.h    |  2 +-
>>  arch/arm/mach-mvebu/coherency_ll.S | 12 ++++++------
>>  arch/arm/mach-mvebu/headsmp.S      |  4 ----
>>  arch/arm/mach-mvebu/platsmp.c      |  2 +-
>>  5 files changed, 13 insertions(+), 17 deletions(-)
> 
> Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> with just one thing I am not entirely sure about:
> 
>>  	/* Create bit by cpu index */
>> -	mov	r3, #(1 << 24)
>> -	lsl	r1, r3, r1
>> -ARM_BE8(rev	r1, r1)
>> +	mrc	15, 0, r1, cr0, cr0, 5
>> +	and	r1, r1, #15
>> +	mov	r2, #(1 << 24)
>> +	lsl	r1, r2, r1
>> +	ARM_BE8(rev	r1, r1)
> 
> Have you tested this in a big endian configuration? I'd like to make
> sure we have gotten the ARM_BE8(rev ...) thing correct.

No I didn't test it in BE. This change is really similar of
what have been done before so I am quite confident that it works in
BE as well as after the commit "bca028e7c253 ARM: mvebu: support
running big-endian". As you have a BE setup maybe you could check
it and add your Tested-by.


Thanks,

Gregory


> 
> Thomas
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v4 04/13] 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    |  8 ++---
 arch/arm/mach-mvebu/coherency.h    |  2 +-
 arch/arm/mach-mvebu/coherency_ll.S | 61 ++++++++++++++++++++++++--------------
 arch/arm/mach-mvebu/headsmp.S      |  4 +--
 arch/arm/mach-mvebu/platsmp.c      |  2 +-
 5 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index 51010dbbf7e4..931c62be2bce 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -44,9 +44,9 @@ static struct of_device_id of_coherency_table[] = {
 };
 
 /* Function defined in coherency_ll.S */
-int ll_set_cpu_coherent(void);
+int ll_set_cpu_coherent_and_smp(void);
 
-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");
@@ -54,7 +54,7 @@ int set_cpu_coherent(int smp_group_id)
 		return 1;
 	}
 
-	return ll_set_cpu_coherent();
+	return ll_set_cpu_coherent_and_smp();
 }
 
 static inline void mvebu_hwcc_sync_io_barrier(void)
@@ -140,7 +140,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 c7e5df368d98..dff16612dd93 100644
--- a/arch/arm/mach-mvebu/coherency.h
+++ b/arch/arm/mach-mvebu/coherency.h
@@ -15,8 +15,8 @@
 #define __MACH_370_XP_COHERENCY_H
 
 extern unsigned long coherency_phys_base;
+int set_cpu_coherent(void);
 
-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 6cb26b919787..7b42b4b08a80 100644
--- a/arch/arm/mach-mvebu/coherency_ll.S
+++ b/arch/arm/mach-mvebu/coherency_ll.S
@@ -25,52 +25,67 @@
 
 	.text
 
-ENTRY(ll_set_cpu_coherent)
+	.macro modify_coherent_reg join_smp
 	mrc	p15, 0, r1, c1, c0, 0
 	tst	r1, #CR_M @ Check MMU bit enabled
 	bne	1f
 
-	/* use physical address of the coherency register*/
-	adr	r0, 3f
-	ldr	r3, [r0]
-	ldr	r0, [r0, r3]
+	/* use physical address of the coherency register */
+	adr	r1, 3f
+	ldr	r3, [r1]
+	ldr	r1, [r1, r3]
 	b	2f
 1:
-	/* use virtual address of the coherency register*/
-	ldr	r0, =coherency_base
-	ldr	r0, [r0]
+	/* use virtual address of the coherency register */
+	ldr	r1, =coherency_base
+	ldr	r1, [r1]
 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
-	ARM_BE8(rev	r1, r1)
+	lsl	r3, r2, r3
+	ARM_BE8(rev	r3, r3)
 
-	/* Add CPU to SMP group - Atomic */
-	add	r3, r0, #ARMADA_XP_CFB_CTL_REG_OFFSET
+	.if \join_smp == 1
+	/* Add CPU to SMP group - Atomic (only if the flag is set) */
+	add	r0, r1, #ARMADA_XP_CFB_CFG_REG_OFFSET
 1:
-	ldrex	r2, [r3]
-	orr	r2, r2, r1
-	strex 	r0, r2, [r3]
-	cmp	r0, #0
+	ldrex	r2, [r0]
+	orr	r2, r2, r3
+	strex 	r1, r2, [r0]
+	cmp	r1, #0
 	bne 1b
 
+	/* get back to the base register */
+	sub	r1, r0, #ARMADA_XP_CFB_CFG_REG_OFFSET
+	.endif
+
 	/* Enable coherency on CPU - Atomic */
-	add	r3, r3, #ARMADA_XP_CFB_CFG_REG_OFFSET
+	add	r0, r1, #ARMADA_XP_CFB_CTL_REG_OFFSET
 1:
-	ldrex	r2, [r3]
-	orr	r2, r2, r1
-	strex	r0, r2, [r3]
-	cmp	r0, #0
+	ldrex	r2, [r0]
+	orr	r2, r2, r3
+	strex	r1, r2, [r0]
+	cmp	r1, #0
 	bne 1b
 
 	dsb
 
 	mov	r0, #0
 	mov	pc, lr
+	.endm
+
+/*  Enable coherency on CPU */
+ENTRY(ll_set_cpu_coherent)
+	modify_coherent_reg join_smp = 0
 ENDPROC(ll_set_cpu_coherent)
 
+/* Add CPU to SMP group */
+ENTRY(ll_set_cpu_coherent_and_smp)
+	modify_coherent_reg join_smp = 1
+ENDPROC(ll_set_cpu_coherent_and_smp)
+
 	.align 2
 3:
 	.long	coherency_phys_base - .
diff --git a/arch/arm/mach-mvebu/headsmp.S b/arch/arm/mach-mvebu/headsmp.S
index cf7abe6554f7..924fb96775c5 100644
--- a/arch/arm/mach-mvebu/headsmp.S
+++ b/arch/arm/mach-mvebu/headsmp.S
@@ -31,8 +31,8 @@
 ENTRY(armada_xp_secondary_startup)
  ARM_BE8(setend	be )			@ go BE8 if entered LE
 
-	/* Add CPU to coherency fabric */
-	bl	ll_set_cpu_coherent
+	bl	ll_set_cpu_coherent_and_smp
+
 	b	secondary_startup
 
 ENDPROC(armada_xp_secondary_startup)
diff --git a/arch/arm/mach-mvebu/platsmp.c b/arch/arm/mach-mvebu/platsmp.c
index a99d71a747f0..f2f1830063c8 100644
--- a/arch/arm/mach-mvebu/platsmp.c
+++ b/arch/arm/mach-mvebu/platsmp.c
@@ -102,7 +102,7 @@ static 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 v4 04/13] ARM: mvebu: Remove the unused argument of set_cpu_coherent()

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Thu, 13 Feb 2014 18:33:27 +0100, 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
> we can remove this parameter.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

This patch does much more than what the title and commit log says. The
title and commit log says the patch is removing the useless parameter
of set_cpu_coherent(), which I'm fine with.

But this patch also renames ll_set_cpu_coherent() to
ll_set_cpu_coherent_and_smp(), introduces the modify_coherent_reg
macro, etc.

And actually, I'm really unhappy about this modify_coherent_reg macro.
It is a large blurb of assembly with some conditions in the middle. I
have already stated this in previous reviews of this patch series, but
you have not taken into account my comments: this stuff should be split
in separate functions, each function doing *one* thing, and then those
functions are called in sequence depending on what needs to be done
whether you're starting a secondary CPU, entering or exiting deep idle,
etc.

Here is what I would prefer to see:

/* Returns with r1 filled with the coherency base address */
ENTRY(ll_get_coherency_base)
	mrc	p15, 0, r1, c1, c0, 0
	tst	r1, #CR_M @ Check MMU bit enabled
	bne	1f

	/* use physical address of the coherency register */
	adr	r1, 3f
	ldr	r3, [r1]
	ldr	r1, [r1, r3]
	b	2f
1:
	/* use virtual address of the coherency register */
	ldr	r1, =coherency_base
	ldr	r1, [r1]
2:
	mov	pc, lr
ENDPROC(ll_get_coherency_base)

/* Returns with the CPU ID in r3 */
ENTRY(ll_get_cpuid)
	mrc	15, 0, r3, cr0, cr0, 5
	and	r3, r3, #15
	mov	r2, #(1 << 24)
	lsl	r3, r2, r3
	ARM_BE8(rev	r3, r3)
	mov	pc, lr
ENDPROC(ll_get_cpuid)

ENTRY(ll_add_cpu_to_smp_group)
	bl	ll_get_coherency_base
	bl	ll_get_cpuid
	add	r0, r1, #ARMADA_XP_CFB_CFG_REG_OFFSET
1:
	ldrex	r2, [r0]
	orr	r2, r2, r3
	strex	r1, r2, [r0]
	cmp	r1, #0
	bne	1b
	mov	pc, lr
ENDPROC(ll_add_cpu_to_smp_group)

ENTRY(ll_enable_coherency)
	bl	ll_get_coherency_base
	bl	ll_get_cpuid
	add	r0, r1, #ARMADA_XP_CFB_CTL_REG_OFFSET
1:
	ldrex	r2, [r0]
	orr	r2, r2, r3
	strex	r1, r2, [r0]
	cmp	r1, #0
	bne	1b
	dsb
	mov	pc, lr
ENDPROC(ll_enable_coherency)

ENTRY(ll_disable_coherency)
	bl	ll_get_coherency_base
	bl	ll_get_cpuid
	add	r0, r1, #ARMADA_XP_CFB_CTL_REG_OFFSET
1:
	ldrex	r2, [r0]
	bic	r2, r2, r3
	strex	r1, r2, [r0]
	cmp	r1, #0
	bne	1b
	dsb
	mov	pc, lr
ENDPROC(ll_disable_coherency)

And then, your C code calls ll_disable_coherency(),
ll_enable_coherency() and/or ll_add_cpu_to_smp_group().

An alternative solution is to implement more of this stuff in C, with
only a small helper function in assembly to do the atomic bit set or
clear magic:

/* First argument:  address
   Second argument: bit mask 
*/
ENTRY(ll_atomic_bit_set)
1:
	ldrex	r2, [r0]
	orr	r2, r2, r1
	strex	r3, r2, [r0]
	cmp	r3, #0
	bne	1b
	dsb
	mov	pc, lr
ENDPROC(ll_atomic_bit_set)

ENTRY(ll_atomic_bit_clear)
1:
	ldrex	r2, [r0]
	bic	r2, r2, r1
	strex	r3, r2, [r0]
	cmp	r3, #0
	bne	1b
	dsb
	mov	pc, lr
ENDPROC(ll_atomic_bit_clear)

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

[PATCH v4 05/13] ARM: mvebu: Low level function to disable HW coherency support

[Gregory CLEMENT]

When going to deep idle we need to disable the SoC snooping (aka
hardware coherency support). 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 commit extends the modify_coherent_reg macro in order to manage
enabling (setting a bit) and disabling (clearing the same bit) the
hardware coherency support.

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 | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S
index 7b42b4b08a80..686aba2adfc7 100644
--- a/arch/arm/mach-mvebu/coherency_ll.S
+++ b/arch/arm/mach-mvebu/coherency_ll.S
@@ -25,7 +25,7 @@
 
 	.text
 
-	.macro modify_coherent_reg join_smp
+	.macro modify_coherent_reg join_smp clear_coherency
 	mrc	p15, 0, r1, c1, c0, 0
 	tst	r1, #CR_M @ Check MMU bit enabled
 	bne	1f
@@ -61,11 +61,19 @@
 	sub	r1, r0, #ARMADA_XP_CFB_CFG_REG_OFFSET
 	.endif
 
-	/* Enable coherency on CPU - Atomic */
+	/*
+	 * Enable coherency on CPU - Atomic (or disable depending of
+	 * the clear_coherency flag)
+	 */
 	add	r0, r1, #ARMADA_XP_CFB_CTL_REG_OFFSET
+
 1:
 	ldrex	r2, [r0]
+	.if \clear_coherency == 1
+	bic	r2, r2, r3
+	.else
 	orr	r2, r2, r3
+	.endif
 	strex	r1, r2, [r0]
 	cmp	r1, #0
 	bne 1b
@@ -78,12 +86,17 @@
 
 /*  Enable coherency on CPU */
 ENTRY(ll_set_cpu_coherent)
-	modify_coherent_reg join_smp = 0
+	modify_coherent_reg join_smp = 0, clear_coherency = 0
 ENDPROC(ll_set_cpu_coherent)
 
+/*  Disable coherency on CPU */
+ENTRY(ll_clear_cpu_coherent)
+	modify_coherent_reg join_smp = 0, clear_coherency = 1
+ENDPROC(ll_clear_cpu_coherent)
+
 /* Add CPU to SMP group */
 ENTRY(ll_set_cpu_coherent_and_smp)
-	modify_coherent_reg join_smp = 1
+	modify_coherent_reg join_smp = 1, clear_coherency = 0
 ENDPROC(ll_set_cpu_coherent_and_smp)
 
 	.align 2
-- 
1.8.1.2

[PATCH v4 06/13] 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 powered
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 d71ef53107c4..aa5ef7439d0f 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -26,6 +26,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)
@@ -67,7 +68,11 @@ static 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 v4 07/13] 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 926b4d6aae7e..8a9db0c32ba5 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 b8b84a22f0f3..f717da4f4d97 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -113,7 +113,7 @@
 
 			armada-370-xp-pmsu at 22000 {
 				compatible = "marvell,armada-370-xp-pmsu";
-				reg = <0x22100 0x400>, <0x20800 0x20>;
+				reg = <0x22100 0x400>, <0x20800 0x20>, <0x22000 0x24>;
 			};
 
 			eth2: ethernet at 30000 {
-- 
1.8.1.2

[PATCH v4 07/13] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[Jason Cooper]

On Thu, Feb 13, 2014 at 06:33:30PM +0100, 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 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 926b4d6aae7e..8a9db0c32ba5 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.

Please mention explicitly that previous versions of this binding only
specified the first two registers.  I haven't dug through the rest of
this series yet, but I assume the driver will behave sanely when
encountering an old dtb w/o the third reg entry?

thx,

Jason.

>  
>  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 b8b84a22f0f3..f717da4f4d97 100644
> --- a/arch/arm/boot/dts/armada-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-xp.dtsi
> @@ -113,7 +113,7 @@
>  
>  			armada-370-xp-pmsu at 22000 {
>  				compatible = "marvell,armada-370-xp-pmsu";
> -				reg = <0x22100 0x400>, <0x20800 0x20>;
> +				reg = <0x22100 0x400>, <0x20800 0x20>, <0x22000 0x24>;
>  			};
>  
>  			eth2: ethernet at 30000 {
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 07/13] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Thu, 13 Feb 2014 18:33:30 +0100, Gregory CLEMENT wrote:

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

I am not too happy with this because:

 *) We have to remove the second register pair from the PMSU. I haven't
    yet posted the patches on LAKML for SMP on 375 and 38x, but the 375
    doesn't have a PMSU. It however has the same CPU reset control
    registers as 370 and XP. Therefore, these 0x20800 registers have to
    be handled by a separate driver (that uses the reset framework),
    and not handled by the PMSU driver. This way, Armada 370/XP can use
    PMSU+CPU reset, while 375 will only use CPU reset.

 *) I think making the PMSU register start at 0x22100 was a mistake.
    The PMSU registers actually start at 0x22000 and they seem to go all
    the way to 0x23000. So we should have only one register pair. This
    of course raises some backward compatibility questions for the DT.

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

[PATCH v4 07/13] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[Gregory CLEMENT]

On 19/02/2014 17:00, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Thu, 13 Feb 2014 18:33:30 +0100, Gregory CLEMENT wrote:
> 
>>  - 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>;
> 
> I am not too happy with this because:
> 
>  *) We have to remove the second register pair from the PMSU. I haven't
>     yet posted the patches on LAKML for SMP on 375 and 38x, but the 375
>     doesn't have a PMSU. It however has the same CPU reset control
>     registers as 370 and XP. Therefore, these 0x20800 registers have to
>     be handled by a separate driver (that uses the reset framework),
>     and not handled by the PMSU driver. This way, Armada 370/XP can use
>     PMSU+CPU reset, while 375 will only use CPU reset.
> 
>  *) I think making the PMSU register start at 0x22100 was a mistake.
>     The PMSU registers actually start at 0x22000 and they seem to go all
>     the way to 0x23000. So we should have only one register pair. This
>     of course raises some backward compatibility questions for the DT.

I agree to use something like:

armada-370-xp-pmsu at 22000 {
	compatible = "marvell,armada-xp-pmsu"; /* new compatible string */
	reg = <0x22000 0x1000>;
	};


and I think the best option would be to introduce a new compatible string
for this. In the same time we continue to support the old compatible string
but we print a big warning during the kernel boot that this compatible string
is deprecated, and we will finally remove it a few release.

Thanks,

Gregory


> 
> Thomas
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
+33 602 196 044

[PATCH v4 07/13] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Wed, 19 Feb 2014 18:49:13 +0100, Gregory CLEMENT wrote:

> I agree to use something like:
> 
> armada-370-xp-pmsu at 22000 {
> 	compatible = "marvell,armada-xp-pmsu"; /* new compatible string */

But this PMSU is identical on 370, no? So the marvell,armada-xp-pmsu
compatible string is maybe not the most appropriate one?

> 	reg = <0x22000 0x1000>;
> 	};
> 
> 
> and I think the best option would be to introduce a new compatible string
> for this. In the same time we continue to support the old compatible string
> but we print a big warning during the kernel boot that this compatible string
> is deprecated, and we will finally remove it a few release.

How do you support the new features of the L2 stuff with the old
compatible string? By substracting 0x100 to the register base address?

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

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

[Gregory CLEMENT]

This commit adds an function which 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 | 24 ++++++++++++++++++++++--
 arch/arm/mach-mvebu/pmsu.h |  1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index aa5ef7439d0f..36d946d53f54 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -28,8 +28,15 @@ 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)
+/* PMSU MP registers */
+#define PMSU_BOOT_ADDR_REDIRECT_OFFSET(cpu) ((cpu * 0x100) + 0x24)
+
+/* PMSU reset registers */
+#define PMSU_RESET_CTL_OFFSET(cpu)	    (cpu * 0x8)
+
+/* PMSU fabric registers */
+#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"},
@@ -79,4 +86,17 @@ static int __init armada_370_xp_pmsu_init(void)
 	return 0;
 }
 
+void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void)
+{
+	u32 reg;
+
+	if (pmsu_fabric_base == NULL)
+		return;
+
+	/* 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);
+}
+
 early_initcall(armada_370_xp_pmsu_init);
diff --git a/arch/arm/mach-mvebu/pmsu.h b/arch/arm/mach-mvebu/pmsu.h
index 07a737c6b95d..054cdd8b0ece 100644
--- a/arch/arm/mach-mvebu/pmsu.h
+++ b/arch/arm/mach-mvebu/pmsu.h
@@ -12,5 +12,6 @@
 #define __MACH_MVEBU_PMSU_H
 
 int armada_xp_boot_cpu(unsigned int cpu_id, void *phys_addr);
+void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void);
 
 #endif	/* __MACH_370_XP_PMSU_H */
-- 
1.8.1.2

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

[Gregory CLEMENT]

The cpu idle support will need to access to Power Management Service
Unit. This commit adds the prepare and restore functions that will be
used in the idle path of the cpuidle driver.

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

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index 36d946d53f54..ec745d59fea8 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -29,6 +29,24 @@ static void __iomem *pmsu_reset_base;
 static void __iomem *pmsu_fabric_base;
 
 /* PMSU MP registers */
+#define PMSU_CONTROL_AND_CONFIG(cpu)	    ((cpu * 0x100) + 0x4)
+#define PMSU_CONTROL_AND_CONFIG_DFS_REQ		BIT(18)
+#define PMSU_CONTROL_AND_CONFIG_PWDDN_REQ	BIT(16)
+#define PMSU_CONTROL_AND_CONFIG_L2_PWDDN	BIT(20)
+
+#define PMSU_CPU_POWER_DOWN_CONTROL(cpu)    ((cpu * 0x100) + 0x8)
+
+#define PMSU_CPU_POWER_DOWN_DIS_SNP_Q_SKIP	BIT(0)
+
+#define PMSU_STATUS_AND_MASK(cpu)	    ((cpu * 0x100) + 0xc)
+#define PMSU_STATUS_AND_MASK_CPU_IDLE_WAIT	BIT(16)
+#define PMSU_STATUS_AND_MASK_SNP_Q_EMPTY_WAIT	BIT(17)
+#define PMSU_STATUS_AND_MASK_IRQ_WAKEUP		BIT(20)
+#define PMSU_STATUS_AND_MASK_FIQ_WAKEUP		BIT(21)
+#define PMSU_STATUS_AND_MASK_DBG_WAKEUP		BIT(22)
+#define PMSU_STATUS_AND_MASK_IRQ_MASK		BIT(24)
+#define PMSU_STATUS_AND_MASK_FIQ_MASK		BIT(25)
+
 #define PMSU_BOOT_ADDR_REDIRECT_OFFSET(cpu) ((cpu * 0x100) + 0x24)
 
 /* PMSU reset registers */
@@ -99,4 +117,71 @@ void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void)
 	writel(reg, pmsu_fabric_base + L2C_NFABRIC_PM_CTL);
 }
 
+static void armada_370_xp_cpu_resume(void)
+{
+	asm volatile("bl    ll_set_cpu_coherent_and_smp\n\t"
+		     "b	    cpu_resume\n\t");
+}
+
+/* No locking is needed because we only access per-CPU registers */
+void armada_370_xp_pmsu_idle_prepare(bool deepidle)
+{
+	unsigned int hw_cpu = cpu_logical_map(smp_processor_id());
+	u32 reg;
+
+	if (pmsu_mp_base == NULL)
+		return;
+
+	/*
+	 * 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 + PMSU_STATUS_AND_MASK(hw_cpu));
+	reg |= PMSU_STATUS_AND_MASK_CPU_IDLE_WAIT    |
+	       PMSU_STATUS_AND_MASK_IRQ_WAKEUP       |
+	       PMSU_STATUS_AND_MASK_FIQ_WAKEUP       |
+	       PMSU_STATUS_AND_MASK_SNP_Q_EMPTY_WAIT |
+	       PMSU_STATUS_AND_MASK_IRQ_MASK         |
+	       PMSU_STATUS_AND_MASK_FIQ_MASK;
+	writel(reg, pmsu_mp_base + PMSU_STATUS_AND_MASK(hw_cpu));
+
+	reg = readl(pmsu_mp_base + PMSU_CONTROL_AND_CONFIG(hw_cpu));
+	/* ask HW to power down the L2 Cache if needed */
+	if (deepidle)
+		reg |= PMSU_CONTROL_AND_CONFIG_L2_PWDDN;
+
+	/* request power down */
+	reg |= PMSU_CONTROL_AND_CONFIG_PWDDN_REQ;
+	writel(reg, pmsu_mp_base + PMSU_CONTROL_AND_CONFIG(hw_cpu));
+
+	/* Disable snoop disable by HW - SW is taking care of it */
+	reg = readl(pmsu_mp_base + PMSU_CPU_POWER_DOWN_CONTROL(hw_cpu));
+	reg |= PMSU_CPU_POWER_DOWN_DIS_SNP_Q_SKIP;
+	writel(reg, pmsu_mp_base + PMSU_CPU_POWER_DOWN_CONTROL(hw_cpu));
+}
+
+/* No locking is needed because we only access per-CPU registers */
+static noinline void armada_370_xp_pmsu_idle_restore(void)
+{
+	unsigned int hw_cpu = cpu_logical_map(smp_processor_id());
+	u32 reg;
+
+	if (pmsu_mp_base == NULL)
+		return;
+
+	/* cancel ask HW to power down the L2 Cache if possible */
+	reg = readl(pmsu_mp_base + PMSU_CONTROL_AND_CONFIG(hw_cpu));
+	reg &= ~PMSU_CONTROL_AND_CONFIG_L2_PWDDN;
+	writel(reg, pmsu_mp_base + PMSU_CONTROL_AND_CONFIG(hw_cpu));
+
+	/* cancel Enable wakeup events and mask interrupts */
+	reg = readl(pmsu_mp_base + PMSU_STATUS_AND_MASK(hw_cpu));
+	reg &= ~(PMSU_STATUS_AND_MASK_IRQ_WAKEUP | PMSU_STATUS_AND_MASK_FIQ_WAKEUP);
+	reg &= ~PMSU_STATUS_AND_MASK_CPU_IDLE_WAIT;
+	reg &= ~PMSU_STATUS_AND_MASK_SNP_Q_EMPTY_WAIT;
+	reg &= ~(PMSU_STATUS_AND_MASK_IRQ_MASK | PMSU_STATUS_AND_MASK_FIQ_MASK);
+	writel(reg, pmsu_mp_base + PMSU_STATUS_AND_MASK(hw_cpu));
+}
+
 early_initcall(armada_370_xp_pmsu_init);
-- 
1.8.1.2

[PATCH v4 10/13] 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.

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

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index ec745d59fea8..162ae1399f2a 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -61,6 +61,12 @@ static struct of_device_id of_pmsu_table[] = {
 	{ /* end of list */ },
 };
 
+static 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));
+}
+
 #ifdef CONFIG_SMP
 int armada_xp_boot_cpu(unsigned int cpu_id, void *boot_addr)
 {
@@ -73,8 +79,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));
-- 
1.8.1.2

[PATCH v4 11/13] ARM: mvebu: Register notifier callback for the cpuidle transition

[Gregory CLEMENT]

In order to have well encapsulated code use notifier callback for
CPU_PM_ENTER and CPU_PM_EXIT inside the mvebu power management code.

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

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index 162ae1399f2a..0e54077c31cb 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -16,6 +16,7 @@
  * other SOC units
  */
 
+#include <linux/cpu_pm.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/of_address.h>
@@ -189,4 +190,27 @@ static noinline void armada_370_xp_pmsu_idle_restore(void)
 	writel(reg, pmsu_mp_base + PMSU_STATUS_AND_MASK(hw_cpu));
 }
 
+static int armada_370_xp_cpu_pm_notify(struct notifier_block *self,
+				    unsigned long action, void *hcpu)
+{
+	if (action == CPU_PM_ENTER) {
+		unsigned int hw_cpu = cpu_logical_map(smp_processor_id());
+		armada_370_xp_pmsu_set_start_addr(armada_370_xp_cpu_resume,
+						hw_cpu);
+	} else if (action == CPU_PM_EXIT) {
+		armada_370_xp_pmsu_idle_restore();
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block armada_370_xp_cpu_pm_notifier = {
+	.notifier_call = armada_370_xp_cpu_pm_notify,
+};
+
+int __init armada_370_xp_cpu_pm_init(void)
+{
+	return cpu_pm_register_notifier(&armada_370_xp_cpu_pm_notifier);
+}
+
 early_initcall(armada_370_xp_pmsu_init);
diff --git a/arch/arm/mach-mvebu/pmsu.h b/arch/arm/mach-mvebu/pmsu.h
index 054cdd8b0ece..e7a766fba757 100644
--- a/arch/arm/mach-mvebu/pmsu.h
+++ b/arch/arm/mach-mvebu/pmsu.h
@@ -13,5 +13,6 @@
 
 int armada_xp_boot_cpu(unsigned int cpu_id, void *phys_addr);
 void armada_370_xp_pmsu_enable_l2_powerdown_onidle(void);
+int armada_370_xp_cpu_pm_init(void);
 
 #endif	/* __MACH_370_XP_PMSU_H */
-- 
1.8.1.2

[PATCH v4 12/13] 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 | 120 ++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c

diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index d988948a89a0..f377eb0840e3 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_BIG_LITTLE_CPUIDLE
 	bool "Support for ARM big.LITTLE processors"
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index f71ae1b373c5..9902d052bd87 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_BIG_LITTLE_CPUIDLE)	+= cpuidle-big_little.o
 obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
 obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c
new file mode 100644
index 000000000000..57c69812e79d
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-armada-370-xp.c
@@ -0,0 +1,120 @@
+/*
+ * 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/cpu_pm.h>
+#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/platform_device.h>
+#include <asm/cp15.h>
+#include <asm/cacheflush.h>
+
+#define ARMADA_370_XP_MAX_STATES	3
+#define ARMADA_370_XP_FLAG_DEEP_IDLE	0x10000
+extern void armada_370_xp_pmsu_idle_prepare(bool deepidle);
+extern void ll_clear_cpu_coherent(void);
+extern void ll_set_cpu_coherent(void);
+
+noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle)
+{
+	armada_370_xp_pmsu_idle_prepare(deepidle);
+
+	v7_exit_coherency_flush(all);
+
+	ll_clear_cpu_coherent();
+
+	dsb();
+
+	wfi();
+
+	ll_set_cpu_coherent();
+
+	asm volatile(
+	"mrc	p15, 0, %0, c1, c0, 0 \n\t"
+	"tst	%0, #(1 << 2) \n\t"
+	"orreq	r0, %0, #(1 << 2) \n\t"
+	"mcreq	p15, 0, %0, c1, c0, 0 \n\t"
+	"isb	"
+	: : "r" (0));
+
+	return 0;
+}
+
+static int armada_370_xp_enter_idle(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv,
+				int index)
+{
+	bool deepidle = false;
+	cpu_pm_enter();
+
+	if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE)
+		deepidle = true;
+
+	cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
+
+	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)
+{
+	return cpuidle_register(&armada_370_xp_idle_driver, NULL);
+}
+
+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,
+};
+
+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 v4 12/13] cpuidle: mvebu: Add initial CPU idle support for Armada 370/XP SoC

[Lorenzo Pieralisi]

On Thu, Feb 13, 2014 at 05:33:35PM +0000, Gregory CLEMENT wrote:

[...]

> diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c
> new file mode 100644
> index 000000000000..57c69812e79d
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c
> @@ -0,0 +1,120 @@
> +/*
> + * 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/cpu_pm.h>
> +#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/platform_device.h>
> +#include <asm/cp15.h>
> +#include <asm/cacheflush.h>

You should order them <linux/...>, then <asm/...>

> +
> +#define ARMADA_370_XP_MAX_STATES	3

Is this macro really needed ?

> +#define ARMADA_370_XP_FLAG_DEEP_IDLE	0x10000
> +extern void armada_370_xp_pmsu_idle_prepare(bool deepidle);
> +extern void ll_clear_cpu_coherent(void);
> +extern void ll_set_cpu_coherent(void);
> +
> +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle)
> +{
> +	armada_370_xp_pmsu_idle_prepare(deepidle);
> +
> +	v7_exit_coherency_flush(all);

The macro above clears the C bit in SCTLR and exit coherency (clears SMP
bit in SCTLR), let's keep this in mind, see below.

> +	ll_clear_cpu_coherent();

And the macro above uses ldr/str exclusives, and this is done with MMU
on and off (on cold-boot before jumping to secondary_startup and also
before jumping to cpu_resume in armada_370_xp_cpu_resume).

Can you explain to me how load/store exclusives work on this platform ?

ARM ARM A3.4.5

"It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can be
performed to a memory region with the Device or Strongly-ordered memory
attribute. Unless the implementation documentation explicitly states that
LDREX and STREX operations to a memory region with the Device or
Strongly-ordered attribute are permitted, the effect of such operations is
UNPREDICTABLE."

At least code must be commented and an explanation on why this works has
to be given.

> +
> +	dsb();
> +
> +	wfi();
> +
> +	ll_set_cpu_coherent();
> +
> +	asm volatile(
> +	"mrc	p15, 0, %0, c1, c0, 0 \n\t"
> +	"tst	%0, #(1 << 2) \n\t"
> +	"orreq	r0, %0, #(1 << 2) \n\t"
> +	"mcreq	p15, 0, %0, c1, c0, 0 \n\t"
> +	"isb	"
> +	: : "r" (0));

First of all, complex code like this must be commented.

Moreover, this sequence is wrong. If wfi completes the kernel would explode.

1) where is the SMP bit in SCTLR restored ?
2) where are tlbs flushed (ie processors run out of coherency for _some_
   time, so tlbs might be stale) ?

> +
> +	return 0;
> +}
> +
> +static int armada_370_xp_enter_idle(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv,
> +				int index)
> +{
> +	bool deepidle = false;
> +	cpu_pm_enter();
> +
> +	if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE)
> +		deepidle = true;
> +
> +	cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
> +
> +	cpu_pm_exit();
> +
> +	return index;

You should check the cpu_suspend return value and demote the idle state
accordingly, if it failed.

Thanks,
Lorenzo

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

[Gregory CLEMENT]

On 14/02/2014 18:00, Lorenzo Pieralisi wrote:
> On Thu, Feb 13, 2014 at 05:33:35PM +0000, Gregory CLEMENT wrote:
> 
> [...]
> 
>> diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c
>> new file mode 100644
>> index 000000000000..57c69812e79d
>> --- /dev/null
>> +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c
>> @@ -0,0 +1,120 @@
>> +/*
>> + * 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/cpu_pm.h>
>> +#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/platform_device.h>
>> +#include <asm/cp15.h>
>> +#include <asm/cacheflush.h>
> 
> You should order them <linux/...>, then <asm/...>

OK done in v5

> 
>> +
>> +#define ARMADA_370_XP_MAX_STATES	3
> 
> Is this macro really needed ?

Well most of the other driver use it. And instead of having
this number directly written in the state_count field I prefer
to using a name for this value. But I think it is more a matter
of taste here.

> 
>> +#define ARMADA_370_XP_FLAG_DEEP_IDLE	0x10000
>> +extern void armada_370_xp_pmsu_idle_prepare(bool deepidle);
>> +extern void ll_clear_cpu_coherent(void);
>> +extern void ll_set_cpu_coherent(void);
>> +
>> +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle)
>> +{
>> +	armada_370_xp_pmsu_idle_prepare(deepidle);
>> +
>> +	v7_exit_coherency_flush(all);
> 
> The macro above clears the C bit in SCTLR and exit coherency (clears SMP
> bit in SCTLR), let's keep this in mind, see below.
> 
>> +	ll_clear_cpu_coherent();
> 
> And the macro above uses ldr/str exclusives, and this is done with MMU
> on and off (on cold-boot before jumping to secondary_startup and also
> before jumping to cpu_resume in armada_370_xp_cpu_resume).
> 
> Can you explain to me how load/store exclusives work on this platform ?
> 
> ARM ARM A3.4.5
> 
> "It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can be
> performed to a memory region with the Device or Strongly-ordered memory
> attribute. Unless the implementation documentation explicitly states that
> LDREX and STREX operations to a memory region with the Device or
> Strongly-ordered attribute are permitted, the effect of such operations is
> UNPREDICTABLE."
> 

Armada XP has an exclusive monitor that can track transactions to Device and/or
SO and as such also when MMU is disabled the exclusive transactions will be
functional.

> At least code must be commented and an explanation on why this works has
> to be given.

I have added this information in comment for the v5.
> 
>> +
>> +	dsb();
>> +
>> +	wfi();
>> +
>> +	ll_set_cpu_coherent();
>> +
>> +	asm volatile(
>> +	"mrc	p15, 0, %0, c1, c0, 0 \n\t"
>> +	"tst	%0, #(1 << 2) \n\t"
>> +	"orreq	r0, %0, #(1 << 2) \n\t"
>> +	"mcreq	p15, 0, %0, c1, c0, 0 \n\t"
>> +	"isb	"
>> +	: : "r" (0));
> 
> First of all, complex code like this must be commented.
> 
> Moreover, this sequence is wrong. If wfi completes the kernel would explode.
> 
> 1) where is the SMP bit in SCTLR restored ?
It is restored in this uncommeted piece of code. I added comment
now in the v5.

> 2) where are tlbs flushed (ie processors run out of coherency for _some_
>    time, so tlbs might be stale) ?

Right it was missing, I added it.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int armada_370_xp_enter_idle(struct cpuidle_device *dev,
>> +				struct cpuidle_driver *drv,
>> +				int index)
>> +{
>> +	bool deepidle = false;
>> +	cpu_pm_enter();
>> +
>> +	if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE)
>> +		deepidle = true;
>> +
>> +	cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
>> +
>> +	cpu_pm_exit();
>> +
>> +	return index;
> 
> You should check the cpu_suspend return value and demote the idle state
> accordingly, if it failed.

Done in v5.

> 
> Thanks,
> Lorenzo
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Daniel Lezcano]

On 02/13/2014 06:33 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 | 120 ++++++++++++++++++++++++++++++++
>   3 files changed, 126 insertions(+)
>   create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c
>
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index d988948a89a0..f377eb0840e3 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_BIG_LITTLE_CPUIDLE
>   	bool "Support for ARM big.LITTLE processors"
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index f71ae1b373c5..9902d052bd87 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_BIG_LITTLE_CPUIDLE)	+= cpuidle-big_little.o
>   obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
>   obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
> diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c
> new file mode 100644
> index 000000000000..57c69812e79d
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c
> @@ -0,0 +1,120 @@
> +/*
> + * 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>
> + */

Hi Gregory,

> +#include <linux/cpu_pm.h>
> +#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/platform_device.h>
> +#include <asm/cp15.h>
> +#include <asm/cacheflush.h>

the convention for the drivers inside this directory is to not include 
<asm/*>.

> +#define ARMADA_370_XP_MAX_STATES	3
> +#define ARMADA_370_XP_FLAG_DEEP_IDLE	0x10000
> +extern void armada_370_xp_pmsu_idle_prepare(bool deepidle);
> +extern void ll_clear_cpu_coherent(void);
> +extern void ll_set_cpu_coherent(void);

Here you have to declare three low level functions which should not be 
visible in this driver.

I think you can go one step more in the code encapsulation by doing like 
the cpuidle-at91 driver:
	* Move armada_370_xp_cpu_suspend inside arch/arm/mach-mvebu/pm.c or 
whereever you want
	* Assign the suspend function to the platform device data file
	* Get the suspend function as a callback for suspend from the field above


> +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle)
> +{
> +	armada_370_xp_pmsu_idle_prepare(deepidle);
> +
> +	v7_exit_coherency_flush(all);
> +
> +	ll_clear_cpu_coherent();
> +
> +	dsb();
> +
> +	wfi();
> +
> +	ll_set_cpu_coherent();
> +
> +	asm volatile(
> +	"mrc	p15, 0, %0, c1, c0, 0 \n\t"
> +	"tst	%0, #(1 << 2) \n\t"
> +	"orreq	r0, %0, #(1 << 2) \n\t"
> +	"mcreq	p15, 0, %0, c1, c0, 0 \n\t"
> +	"isb	"
> +	: : "r" (0));
> +
> +	return 0;
> +}
> +
> +static int armada_370_xp_enter_idle(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv,
> +				int index)
> +{
> +	bool deepidle = false;
> +	cpu_pm_enter();
> +
> +	if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE)
> +		deepidle = true;
> +
> +	cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
> +
> +	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)
> +{
> +	return cpuidle_register(&armada_370_xp_idle_driver, NULL);
> +}
> +
> +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,
> +};
> +
> +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");
>


-- 
  <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 v4 12/13] cpuidle: mvebu: Add initial CPU idle support for Armada 370/XP SoC

[Gregory CLEMENT]

On 17/02/2014 09:49, Daniel Lezcano wrote:
> On 02/13/2014 06:33 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 | 120 ++++++++++++++++++++++++++++++++
>>   3 files changed, 126 insertions(+)
>>   create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c
>>
>> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
>> index d988948a89a0..f377eb0840e3 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_BIG_LITTLE_CPUIDLE
>>   	bool "Support for ARM big.LITTLE processors"
>> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
>> index f71ae1b373c5..9902d052bd87 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_BIG_LITTLE_CPUIDLE)	+= cpuidle-big_little.o
>>   obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
>>   obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
>> diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c
>> new file mode 100644
>> index 000000000000..57c69812e79d
>> --- /dev/null
>> +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c
>> @@ -0,0 +1,120 @@
>> +/*
>> + * 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>
>> + */
> 
> Hi Gregory,
> 
>> +#include <linux/cpu_pm.h>
>> +#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/platform_device.h>
>> +#include <asm/cp15.h>
>> +#include <asm/cacheflush.h>
> 
> the convention for the drivers inside this directory is to not include 
> <asm/*>.

Well it is not the case yet:

git grep asm drivers/cpuidle/cpuidle-* | wc -l
25

Most of the time it was asm/cpuidle.h which was needed for using
ARM_CPUIDLE_WFI_STATE. However I try to remove most of the header from
asm/ by following your other advises.

> 
>> +#define ARMADA_370_XP_MAX_STATES	3
>> +#define ARMADA_370_XP_FLAG_DEEP_IDLE	0x10000
>> +extern void armada_370_xp_pmsu_idle_prepare(bool deepidle);
>> +extern void ll_clear_cpu_coherent(void);
>> +extern void ll_set_cpu_coherent(void);
> 
> Here you have to declare three low level functions which should not be 
> visible in this driver.
> 
> I think you can go one step more in the code encapsulation by doing like 
> the cpuidle-at91 driver:
> 	* Move armada_370_xp_cpu_suspend inside arch/arm/mach-mvebu/pm.c or 
> whereever you want
> 	* Assign the suspend function to the platform device data file
> 	* Get the suspend function as a callback for suspend from the field above

Ok I have done it in the 5th version.

> 
> 
>> +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle)
>> +{
>> +	armada_370_xp_pmsu_idle_prepare(deepidle);
>> +
>> +	v7_exit_coherency_flush(all);
>> +
>> +	ll_clear_cpu_coherent();
>> +
>> +	dsb();
>> +
>> +	wfi();
>> +
>> +	ll_set_cpu_coherent();
>> +
>> +	asm volatile(
>> +	"mrc	p15, 0, %0, c1, c0, 0 \n\t"
>> +	"tst	%0, #(1 << 2) \n\t"
>> +	"orreq	r0, %0, #(1 << 2) \n\t"
>> +	"mcreq	p15, 0, %0, c1, c0, 0 \n\t"
>> +	"isb	"
>> +	: : "r" (0));
>> +
>> +	return 0;
>> +}
>> +
>> +static int armada_370_xp_enter_idle(struct cpuidle_device *dev,
>> +				struct cpuidle_driver *drv,
>> +				int index)
>> +{
>> +	bool deepidle = false;
>> +	cpu_pm_enter();
>> +
>> +	if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE)
>> +		deepidle = true;
>> +
>> +	cpu_suspend(deepidle, armada_370_xp_cpu_suspend);
>> +
>> +	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)
>> +{
>> +	return cpuidle_register(&armada_370_xp_idle_driver, NULL);
>> +}
>> +
>> +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,
>> +};
>> +
>> +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");
>>
> 
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Thu, 13 Feb 2014 18:33:35 +0100, Gregory CLEMENT wrote:

> +config ARM_ARMADA_370_XP_CPUIDLE
> +	bool "CPU Idle Driver for Armada 370/XP family processors"

Sorry to bring the naming issue, but it looks like the Armada 38x has a
PMSU that looks almost identical to the Armada XP PMSU, except that it
doesn't have the L2 fabric registers (probably because Armada 38x uses
the PL310 and not the Marvell L2 cache).

Therefore, should this cpuidle driver be named Armada 370/XP, or
ARMADA_MVEBU for example?

> +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle)



> +{
> +	armada_370_xp_pmsu_idle_prepare(deepidle);
> +
> +	v7_exit_coherency_flush(all);
> +
> +	ll_clear_cpu_coherent();
> +
> +	dsb();
> +
> +	wfi();
> +
> +	ll_set_cpu_coherent();
> +
> +	asm volatile(
> +	"mrc	p15, 0, %0, c1, c0, 0 \n\t"
> +	"tst	%0, #(1 << 2) \n\t"
> +	"orreq	r0, %0, #(1 << 2) \n\t"
> +	"mcreq	p15, 0, %0, c1, c0, 0 \n\t"
> +	"isb	"
> +	: : "r" (0));

I believe a little comment on top of this assembly block would be good
to have, to at least give a quick idea on what's going on.

Also, I'm a bit unsure about your choice of mixing C and assembly here.
This function is already calling ll_clear_cpu_coherent() and
ll_set_cpu_coherent() that are assembly functions implement in
coherency_ll.S. Shouldn't we do the same for this final bit of assembly?

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

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

[Gregory CLEMENT]

On 19/02/2014 17:51, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Thu, 13 Feb 2014 18:33:35 +0100, Gregory CLEMENT wrote:
> 
>> +config ARM_ARMADA_370_XP_CPUIDLE
>> +	bool "CPU Idle Driver for Armada 370/XP family processors"
> 
> Sorry to bring the naming issue, but it looks like the Armada 38x has a
> PMSU that looks almost identical to the Armada XP PMSU, except that it
> doesn't have the L2 fabric registers (probably because Armada 38x uses
> the PL310 and not the Marvell L2 cache).
> 
> Therefore, should this cpuidle driver be named Armada 370/XP, or
> ARMADA_MVEBU for example?

Well most of the code is related to the coherency and the L2 cache, so
a different L2 cache is a significant difference. The CPU are also different
for example the PJ4B can  use LDREX/STREX without MMU whereas the Cortex A9
can't.

> 
>> +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle)
> 
> 
> 
>> +{
>> +	armada_370_xp_pmsu_idle_prepare(deepidle);
>> +
>> +	v7_exit_coherency_flush(all);
>> +
>> +	ll_clear_cpu_coherent();
>> +
>> +	dsb();
>> +
>> +	wfi();
>> +
>> +	ll_set_cpu_coherent();
>> +
>> +	asm volatile(
>> +	"mrc	p15, 0, %0, c1, c0, 0 \n\t"
>> +	"tst	%0, #(1 << 2) \n\t"
>> +	"orreq	r0, %0, #(1 << 2) \n\t"
>> +	"mcreq	p15, 0, %0, c1, c0, 0 \n\t"
>> +	"isb	"
>> +	: : "r" (0));
> 
> I believe a little comment on top of this assembly block would be good
> to have, to at least give a quick idea on what's going on.

I am on it, I had the same remark from Lorenzo Pieralisi.

> 
> Also, I'm a bit unsure about your choice of mixing C and assembly here.
> This function is already calling ll_clear_cpu_coherent() and
> ll_set_cpu_coherent() that are assembly functions implement in
> coherency_ll.S. Shouldn't we do the same for this final bit of assembly?

I made several tries when I converted most of the code in C, so I am not
sure but I think that using a C function didn't work here. But as Lorenzo
pointed they were mistakes in this code, so once I will have fixed them, I
will try again.


Thanks,

Gregory


> 
> Thomas
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Wed, 19 Feb 2014 18:19:51 +0100, Gregory CLEMENT wrote:

> > Sorry to bring the naming issue, but it looks like the Armada 38x has a
> > PMSU that looks almost identical to the Armada XP PMSU, except that it
> > doesn't have the L2 fabric registers (probably because Armada 38x uses
> > the PL310 and not the Marvell L2 cache).
> > 
> > Therefore, should this cpuidle driver be named Armada 370/XP, or
> > ARMADA_MVEBU for example?
> 
> Well most of the code is related to the coherency and the L2 cache, so
> a different L2 cache is a significant difference. The CPU are also different
> for example the PJ4B can  use LDREX/STREX without MMU whereas the Cortex A9
> can't.

Right, ok.

> > Also, I'm a bit unsure about your choice of mixing C and assembly here.
> > This function is already calling ll_clear_cpu_coherent() and
> > ll_set_cpu_coherent() that are assembly functions implement in
> > coherency_ll.S. Shouldn't we do the same for this final bit of assembly?
> 
> I made several tries when I converted most of the code in C, so I am not
> sure but I think that using a C function didn't work here. But as Lorenzo
> pointed they were mistakes in this code, so once I will have fixed them, I
> will try again.

Having this is C would certainly be a lot better, but my comment was
merely to move this tiny bit of assembly somewhere else, but keep it as
assembly if it's really needed.

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

[PATCH v4 13/13] 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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index f6c9d1d85c14..81b42980311c 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -30,6 +30,11 @@
 #include "common.h"
 #include "coherency.h"
 #include "mvebu-soc-id.h"
+#include "pmsu.h"
+
+static struct platform_device armada_xp_cpuidle_device = {
+	.name = "cpuidle-armada-370-xp",
+};
 
 static void __init armada_370_xp_map_io(void)
 {
@@ -80,6 +85,13 @@ static void __init armada_370_xp_dt_init(void)
 	if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
 		i2c_quirk();
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+	if (of_find_compatible_node(NULL, NULL, "marvell,armada-370-xp-pmsu")
+		&& of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric")
+		&& of_machine_is_compatible("marvell,armadaxp")) {
+		armada_370_xp_pmsu_enable_l2_powerdown_onidle();
+		armada_370_xp_cpu_pm_init();
+		platform_device_register(&armada_xp_cpuidle_device);
+	}
 }
 
 static const char * const armada_370_xp_dt_compat[] = {
-- 
1.8.1.2

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

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Thu, 13 Feb 2014 18:33:36 +0100, Gregory CLEMENT wrote:
> 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 | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
> index f6c9d1d85c14..81b42980311c 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -30,6 +30,11 @@
>  #include "common.h"
>  #include "coherency.h"
>  #include "mvebu-soc-id.h"
> +#include "pmsu.h"
> +
> +static struct platform_device armada_xp_cpuidle_device = {
> +	.name = "cpuidle-armada-370-xp",
> +};
>  
>  static void __init armada_370_xp_map_io(void)
>  {
> @@ -80,6 +85,13 @@ static void __init armada_370_xp_dt_init(void)
>  	if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
>  		i2c_quirk();
>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +	if (of_find_compatible_node(NULL, NULL, "marvell,armada-370-xp-pmsu")
> +		&& of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric")
> +		&& of_machine_is_compatible("marvell,armadaxp")) {
> +		armada_370_xp_pmsu_enable_l2_powerdown_onidle();
> +		armada_370_xp_cpu_pm_init();
> +		platform_device_register(&armada_xp_cpuidle_device);
> +	}

What about putting this in pmsu.c, in an arch_initcall() (or some other
initcall level) ? The cpuidle feature is really tied to the PMSU, so I
believe it makes sense to have the cpuidle-armada-370-xp
platform_device declared and registered in pmsu.c. As an added bonus,
you don't need to expose
armada_370_xp_pmsu_enable_l2_powerdown_onidle() and
armada_370_xp_cpu_pm_init() in a header: they can remain static
functions private to pmsu.c.

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

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

[Gregory CLEMENT]

On 19/02/2014 17:46, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Thu, 13 Feb 2014 18:33:36 +0100, Gregory CLEMENT wrote:
>> 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 | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
>> index f6c9d1d85c14..81b42980311c 100644
>> --- a/arch/arm/mach-mvebu/armada-370-xp.c
>> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
>> @@ -30,6 +30,11 @@
>>  #include "common.h"
>>  #include "coherency.h"
>>  #include "mvebu-soc-id.h"
>> +#include "pmsu.h"
>> +
>> +static struct platform_device armada_xp_cpuidle_device = {
>> +	.name = "cpuidle-armada-370-xp",
>> +};
>>  
>>  static void __init armada_370_xp_map_io(void)
>>  {
>> @@ -80,6 +85,13 @@ static void __init armada_370_xp_dt_init(void)
>>  	if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
>>  		i2c_quirk();
>>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>> +	if (of_find_compatible_node(NULL, NULL, "marvell,armada-370-xp-pmsu")
>> +		&& of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric")
>> +		&& of_machine_is_compatible("marvell,armadaxp")) {
>> +		armada_370_xp_pmsu_enable_l2_powerdown_onidle();
>> +		armada_370_xp_cpu_pm_init();
>> +		platform_device_register(&armada_xp_cpuidle_device);
>> +	}
> 
> What about putting this in pmsu.c, in an arch_initcall() (or some other
> initcall level) ? The cpuidle feature is really tied to the PMSU, so I
> believe it makes sense to have the cpuidle-armada-370-xp
> platform_device declared and registered in pmsu.c. As an added bonus,
> you don't need to expose
> armada_370_xp_pmsu_enable_l2_powerdown_onidle() and
> armada_370_xp_cpu_pm_init() in a header: they can remain static
> functions private to pmsu.c.

It sounds like a good idea, the tricky part will be to find the accurate
initcall level: not too early and not too late.

Thanks,

Gregory

> 
> Thomas
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Wed, 19 Feb 2014 17:52:31 +0100, Gregory CLEMENT wrote:

> > What about putting this in pmsu.c, in an arch_initcall() (or some other
> > initcall level) ? The cpuidle feature is really tied to the PMSU, so I
> > believe it makes sense to have the cpuidle-armada-370-xp
> > platform_device declared and registered in pmsu.c. As an added bonus,
> > you don't need to expose
> > armada_370_xp_pmsu_enable_l2_powerdown_onidle() and
> > armada_370_xp_cpu_pm_init() in a header: they can remain static
> > functions private to pmsu.c.
> 
> It sounds like a good idea, the tricky part will be to find the accurate
> initcall level: not too early and not too late.

Make it an arch_initcall(). This is the point where ->init_machine() is
called in armada-370-xp.c.

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