[PATCH v3 06/14] ARM: mvebu: Low level functions to disable cache snooping
From: Thomas Petazzoni <hidden>
Date: 2013-10-14 14:32:23
Also in:
linux-pm
Dear Gregory CLEMENT, On Mon, 14 Oct 2013 15:58:18 +0200, Gregory CLEMENT wrote:
When going to deep idle we need to disable the SoC snooping by "hand". Playing with the coherency fabric requires to use assembly code to be sure that the compiler doesn't reorder the instructions nor do wrong optimization. This function will be called by the low level (in assembly) part of the CPU idle functions.
So, this is this opposite of adding the CPU into the coherency fabric, as is done in ll_set_cpu_coherent(), right? If that's the case, then the function should be named to be symmetrical with ll_set_cpu_coherent(), i.e something like ll_set_cpu_uncoherent(), or rename the two functions to ll_coherency_fabric_register() / ll_coherency_fabric_unregister() or something.
quoted hunk ↗ jump to hunk
Signed-off-by: Gregory CLEMENT <redacted> --- arch/arm/mach-mvebu/coherency_ll.S | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S index 1526b94..3fb426e 100644 --- a/arch/arm/mach-mvebu/coherency_ll.S +++ b/arch/arm/mach-mvebu/coherency_ll.S@@ -73,6 +73,28 @@ ENTRY(ll_set_cpu_coherent) mov pc, lr ENDPROC(ll_set_cpu_coherent) +/* + * r0: if r0==0 => physical addres, else virtual address
addres -> address Address of what?
+ */ +ENTRY(armada_370_xp_disable_snoop_ena) + ldr r0, =(coherency_base)
So it takes an argument that tells whether it should use a physical or a virtual address, but at the first instruction you overwrite r0. Seems like the function assumes it's called with the MMU enabled, and the comment about the argument is wrong.
+ ldr r0, [r0] + /* Enable SnoopEna - Exclusive */
An empty new line before the comment would be useful, but honestly I don't see the relation with the comment. The function is about disabling the snooping (i.e removing the CPU from the coherency fabric), but the comment says it enables snooping. Not clear.
+ mrc 15, 0, r1, cr0, cr0, 5 + and r1, r1, #15 + mov r2, #(1 << 24) + lsl r2, r2, r1
Some more comment here would be useful: we're reading the current CPU hardware ID, and creating the bitfield that we need to remove this CPU from the coherency fabric.
+ +1: + ldrex r1, [r0] + bic r1, r1, r2 + strex r3, r1, [r0] + cmp r3, #0 + bne 1b
And here we actually remove it from the coherency fabric, with a loop needed to make sure we don't get cheated by other CPUs doing the same thing at the same time.
+ + mov pc, lr +ENDPROC(armada_370_xp_disable_snoop_ena) + .align 2 3: - .long coherency_phys_base - . + .long coherency_phys_base - .
This change is unrelated and unnecessary. Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com