[RFC PATCH 0/3] ARM: mm: add L2 suspend/resume support

[RFC PATCH 0/3] ARM: mm: add L2 suspend/resume support

[Lorenzo Pieralisi]

As a follow up to the last L2 resume patch on alkml:

http://www.spinics.net/lists/arm-kernel/msg141881.html

please have a look at this patchset and merge the required bits if needed,
in particular MMU off for L2 retention use case.

Description:

In order to support the deepest C-states on all ARM platforms, code to resume
L2 state could be added to the kernel to avoid reliance on bootloaders.

If L2 RAM is retained on power down, it is not cleaned on shutdown to preserve
its content and improve performance. Hence, L2 state should be restored before
the MMU is turned on, so that CPU resume code can search the data saved on
power down in the L2 cache.

This patch addresses this issue and extends outercache and l2x0 driver to
support L2 resume in a platform independent way, security issues 
notwithstanding that must be tackled separately.

Since the L2 should be enabled when the MMU is possibly off,
L2 physical address should be passed from platform code or retrieved from 
the device tree so that the code can be made generic and platform
independent. For that purpose, the signature of L2 init function has
to be upgraded.

The resume code should not use the stack and it must fetch data using 
program counter relative loads so that it does not matter if it is run 
in physical or virtual address space.

This patch depends on three patches:

ARM: 7090/1: CACHE-L2X0: filter start address can be 0 and is often 0
http://www.spinics.net/lists/arm-kernel/msg140126.html
available in rmk/for-next commit 2afda86d91b5f15a744182d7ddacf68f6a6054c9

ARM: 7080/1: l2x0: make sure I&D are not locked down on init
http://www.spinics.net/lists/arm-kernel/msg139119.html
available in rmk/for-next commit bac7e6ecf60933b68af910eb4c83a775a8b20b19

ARM: 7009/1: l2x0: Add OF based initialization
http://www.spinics.net/lists/arm-kernel/msg131123.htm
available in rmk/for-next commit 41c86ff5be44e26978282f86c20598181b999142

Platform code initializing L2 has not been patched (l2x0_init requires
a new parameter which is the physical base address) yet, waiting to
see if there are platforms that can take advantage of this code.

Tested on Samsung Origen within suspend and cpuidle code paths.
DT support compile tested.

Lorenzo Pieralisi (3):
  ARM: mm: add outercache resume hook
  ARM: mm: add l2x0 physical address parameter to init
  ARM: mm: add l2x0 suspend/resume support

 arch/arm/include/asm/hardware/cache-l2x0.h |   24 +++++-
 arch/arm/include/asm/outercache.h          |    7 ++
 arch/arm/kernel/asm-offsets.c              |   12 +++
 arch/arm/mm/Makefile                       |    3 +
 arch/arm/mm/cache-l2x0.c                   |   17 +++-
 arch/arm/mm/l2x0-sleep.S                   |  136 ++++++++++++++++++++++++++++
 6 files changed, 193 insertions(+), 6 deletions(-)
 create mode 100644 arch/arm/mm/l2x0-sleep.S

-- 
1.7.4.4

[RFC PATCH 1/3] ARM: mm: add outercache resume hook

[Lorenzo Pieralisi]

In order to support L2 resume functions in a generic way in the kernel, this
patch adds a resume hook to the outercache layer and a simple wrapper
to simplify the function pointer check and the resume function call.

Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Barry Song <Baohua.Song@csr.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm/include/asm/outercache.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/outercache.h b/arch/arm/include/asm/outercache.h
index d838743..1b70d86 100644
--- a/arch/arm/include/asm/outercache.h
+++ b/arch/arm/include/asm/outercache.h
@@ -34,6 +34,7 @@ struct outer_cache_fns {
 	void (*sync)(void);
 #endif
 	void (*set_debug)(unsigned long);
+	void (*resume)(void);
 };
 
 #ifdef CONFIG_OUTER_CACHE
@@ -74,6 +75,11 @@ static inline void outer_disable(void)
 		outer_cache.disable();
 }
 
+static inline void outer_resume(void)
+{
+	if (outer_cache.resume)
+		outer_cache.resume();
+}
 #else
 
 static inline void outer_inv_range(phys_addr_t start, phys_addr_t end)
@@ -86,6 +92,7 @@ static inline void outer_flush_all(void) { }
 static inline void outer_inv_all(void) { }
 static inline void outer_disable(void) { }
 
+static inline void outer_resume(void) { }
 #endif
 
 #ifdef CONFIG_OUTER_CACHE_SYNC
-- 
1.7.4.4

[RFC PATCH 2/3] ARM: mm: add l2x0 physical address parameter to init

[Lorenzo Pieralisi]

This patch redefines the l2x0_init function, and it adds a
parameter defining the L2 physical base address in preparation
for L2 resume support. The device tree init code retrieves the
physical address from the "reg" array and converts it to a
physical address pointer.

Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Barry Song <Baohua.Song@csr.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm/include/asm/hardware/cache-l2x0.h |    3 ++-
 arch/arm/mm/cache-l2x0.c                   |   12 ++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
index c48cb1e..54bf625 100644
--- a/arch/arm/include/asm/hardware/cache-l2x0.h
+++ b/arch/arm/include/asm/hardware/cache-l2x0.h
@@ -94,7 +94,8 @@
 #define L2X0_ADDR_FILTER_EN		1
 
 #ifndef __ASSEMBLY__
-extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
+extern void __init l2x0_init(void __iomem *base, void __iomem *pbase,
+		__u32 aux_val, __u32 aux_mask);
 extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
 #endif
 
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 0d85d22..46a507a 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -299,7 +299,8 @@ static void __init l2x0_unlock(__u32 cache_id)
 	}
 }
 
-void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
+void __init l2x0_init(void __iomem *base, void __iomem *pbase,
+			__u32 aux_val, __u32 aux_mask)
 {
 	__u32 aux;
 	__u32 cache_id;
@@ -455,6 +456,9 @@ static const struct of_device_id l2x0_ids[] __initconst = {
 int __init l2x0_of_init(__u32 aux_val, __u32 aux_mask)
 {
 	struct device_node *np;
+	u32 reg[2];
+	int ret;
+
 	void (*l2_setup)(const struct device_node *np,
 		__u32 *aux_val, __u32 *aux_mask);
 
@@ -465,13 +469,17 @@ int __init l2x0_of_init(__u32 aux_val, __u32 aux_mask)
 	if (!l2x0_base)
 		return -ENOMEM;
 
+	ret = of_property_read_u32_array(np, "reg", reg, ARRAY_SIZE(reg));
+	if (ret < 0)
+		return ret;
+
 	/* L2 configuration can only be changed if the cache is disabled */
 	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & 1)) {
 		l2_setup = of_match_node(l2x0_ids, np)->data;
 		if (l2_setup)
 			l2_setup(np, &aux_val, &aux_mask);
 	}
-	l2x0_init(l2x0_base, aux_val, aux_mask);
+	l2x0_init(l2x0_base, (void __iomem *)reg[0], aux_val, aux_mask);
 	return 0;
 }
 #endif
-- 
1.7.4.4

[RFC PATCH 3/3] ARM: mm: add l2x0 suspend/resume support

[Lorenzo Pieralisi]

When suspend to RAM or cpuidle require the system to enter the deepest C
states, L2 cache logic can be lost.
This patch adds assembly hooks that allow to restore the context for
l2x0 series of L2 controllers upon system resume.

Context is saved once for all at boot time, along with the L2 physical address
and cache type.
The resume code can be called with MMU either on or off and it executes
specific code accordingly, by checking the MMU status in the SCTRL system
register.

Code is in place to check if L2 is already enabled on resume to
avoid writing L2 registers that would cause faults.

The resume hook avoids using the stack since it might be called
before the C environment is up and running and fetches data using
program counter relative addressing so that it can be run both
with MMU on or off to simplify its adoption.

Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Barry Song <Baohua.Song@csr.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm/include/asm/hardware/cache-l2x0.h |   21 ++++-
 arch/arm/kernel/asm-offsets.c              |   12 +++
 arch/arm/mm/Makefile                       |    3 +
 arch/arm/mm/cache-l2x0.c                   |    5 +-
 arch/arm/mm/l2x0-sleep.S                   |  136 ++++++++++++++++++++++++++++
 5 files changed, 174 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/mm/l2x0-sleep.S

diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
index 54bf625..05312eb 100644
--- a/arch/arm/include/asm/hardware/cache-l2x0.h
+++ b/arch/arm/include/asm/hardware/cache-l2x0.h
@@ -97,6 +97,25 @@
 extern void __init l2x0_init(void __iomem *base, void __iomem *pbase,
 		__u32 aux_val, __u32 aux_mask);
 extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
-#endif
 
+#ifndef CONFIG_PM_SLEEP
+static inline void l2x0_resume(void) {}
+static inline void l2x0_save_context(void __iomem *pbase, __u32 cache_id) {}
+#else
+struct l2x0_regs {
+	__u32 aux_ctrl;
+	__u32 tag_latency;
+	__u32 data_latency;
+	__u32 afilter_start;
+	__u32 afilter_end;
+	__u32 debug_ctrl;
+	__u32 prefetch_ctrl;
+	__u32 power_ctrl;
+} __packed;
+
+extern struct l2x0_regs l2x0_data;
+extern void l2x0_resume(void);
+extern void l2x0_save_context(void __iomem *pbase, __u32 cache_id);
+#endif /* CONFIG_PM_SLEEP */
+#endif /* __ASSEMBLY__ */
 #endif
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 16baba2..91d7b7b 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -20,6 +20,7 @@
 #include <asm/thread_info.h>
 #include <asm/memory.h>
 #include <asm/procinfo.h>
+#include <asm/hardware/cache-l2x0.h>
 #include <linux/kbuild.h>
 
 /*
@@ -128,6 +129,17 @@ int main(void)
 #ifdef MULTI_CACHE
   DEFINE(CACHE_FLUSH_KERN_ALL,	offsetof(struct cpu_cache_fns, flush_kern_all));
 #endif
+#ifdef CONFIG_CACHE_L2X0
+  DEFINE(L2X0_REGS_SZ,		sizeof(struct l2x0_regs));
+  DEFINE(L2X0_R_AUX_CTRL,	offsetof(struct l2x0_regs, aux_ctrl));
+  DEFINE(L2X0_R_TAG_LATENCY,	offsetof(struct l2x0_regs, tag_latency));
+  DEFINE(L2X0_R_DATA_LATENCY,	offsetof(struct l2x0_regs, data_latency));
+  DEFINE(L2X0_R_AFILTER_START,	offsetof(struct l2x0_regs, afilter_start));
+  DEFINE(L2X0_R_AFILTER_END,	offsetof(struct l2x0_regs, afilter_end));
+  DEFINE(L2X0_R_DEBUG_CTRL,	offsetof(struct l2x0_regs, debug_ctrl));
+  DEFINE(L2X0_R_PREFETCH_CTRL,	offsetof(struct l2x0_regs, prefetch_ctrl));
+  DEFINE(L2X0_R_POWER_CTRL,	offsetof(struct l2x0_regs, power_ctrl));
+#endif
   BLANK();
   DEFINE(DMA_BIDIRECTIONAL,	DMA_BIDIRECTIONAL);
   DEFINE(DMA_TO_DEVICE,		DMA_TO_DEVICE);
diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index bca7e61..5936d6b 100644
--- a/arch/arm/mm/Makefile
+++ b/arch/arm/mm/Makefile
@@ -98,5 +98,8 @@ AFLAGS_proc-v7.o	:=-Wa,-march=armv7-a
 
 obj-$(CONFIG_CACHE_FEROCEON_L2)	+= cache-feroceon-l2.o
 obj-$(CONFIG_CACHE_L2X0)	+= cache-l2x0.o
+ifeq ($(CONFIG_PM_SLEEP),y)
+obj-$(CONFIG_CACHE_L2X0)	+= l2x0-sleep.o
+endif
 obj-$(CONFIG_CACHE_XSC3L2)	+= cache-xsc3l2.o
 obj-$(CONFIG_CACHE_TAUROS2)	+= cache-tauros2.o
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 46a507a..9b9d619 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -28,7 +28,7 @@
 
 #define CACHE_LINE_SIZE		32
 
-static void __iomem *l2x0_base;
+void __iomem *l2x0_base;
 static DEFINE_SPINLOCK(l2x0_lock);
 static uint32_t l2x0_way_mask;	/* Bitmask of active ways */
 static uint32_t l2x0_size;
@@ -358,7 +358,7 @@ void __init l2x0_init(void __iomem *base, void __iomem *pbase,
 		writel_relaxed(aux, l2x0_base + L2X0_AUX_CTRL);
 
 		l2x0_inv_all();
-
+		l2x0_save_context(pbase, cache_id & L2X0_CACHE_ID_PART_MASK);
 		/* enable L2X0 */
 		writel_relaxed(1, l2x0_base + L2X0_CTRL);
 	}
@@ -371,6 +371,7 @@ void __init l2x0_init(void __iomem *base, void __iomem *pbase,
 	outer_cache.inv_all = l2x0_inv_all;
 	outer_cache.disable = l2x0_disable;
 	outer_cache.set_debug = l2x0_set_debug;
+	outer_cache.resume = l2x0_resume;
 
 	printk(KERN_INFO "%s cache controller enabled\n", type);
 	printk(KERN_INFO "l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n",
diff --git a/arch/arm/mm/l2x0-sleep.S b/arch/arm/mm/l2x0-sleep.S
new file mode 100644
index 0000000..ac4998a
--- /dev/null
+++ b/arch/arm/mm/l2x0-sleep.S
@@ -0,0 +1,136 @@
+/*
+ *  Copyright (C) 2011 ARM Ltd
+ *  Author: Lorenzo Pieralisi
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/linkage.h>
+#include <asm/asm-offsets.h>
+#include <asm/hardware/cache-l2x0.h>
+
+	__INIT
+
+save_l210:
+	ldr	r4, [r1, #L2X0_AUX_CTRL]
+	str	r4, [r0], #4
+	ldmnefd	sp!, {r4 - r11, pc}			@ if l210 returns
+save_l310:
+	ldr	r5, [r1, #L2X0_TAG_LATENCY_CTRL]
+	ldr	r6, [r1, #L2X0_DATA_LATENCY_CTRL]
+	ldr	r7, [r1, #L2X0_ADDR_FILTER_START]
+	ldr	r8, [r1, #L2X0_ADDR_FILTER_END]
+	ldr	r9, [r1, #L2X0_DEBUG_CTRL]
+	ldr	r10, [r1, #L2X0_PREFETCH_CTRL]
+	ldr	r11, [r1, #L2X0_POWER_CTRL]
+	stmia	r0, {r5 - r11}
+	ldmfd	sp!, {r4 - r11, pc}
+
+/*
+ * Save L2x0 registers depending on L2 HW configuration.
+ * A check on cache type is carried out and registers are saved
+ * accordingly, once for all at boot time. L2 is off so there is no
+ * need to clean L2 cache lines containing reg values to DRAM.
+ * r0 = L2 physical base address
+ * r1 = cache type
+ */
+
+ENTRY(l2x0_save_context)
+	stmfd	sp!, {r4 - r11, lr}
+	ldr	r2, =l2x0_phys_base
+	str	r0, [r2]				@ save physical address
+	ldr	r0, =l2x0_base				@ get L2 base address
+	mov	r2, r1					@ stash cache type
+	ldr	r1, [r0]
+	ldr	r0, =l2x0_data				@ get context pointer
+	ldr 	r3, =l2x0_type
+	str	r2, [r3]				@ save cache type
+	cmp	r2, #L2X0_CACHE_ID_PART_L310		@ jump table
+	b 	save_l210
+ENDPROC(l2x0_save_context)
+
+	__FINIT
+/*
+ * Function entered with flags set by jump table in l2x0_resume
+ * If zero flag is set this is a pl310
+ * r0 = l2x0_data
+ * r1 = L2 address
+ */
+resume_l210:
+	ldr	r2, [r0], #4				@ just use scratch regs
+	str	r2, [r1, #L2X0_AUX_CTRL]
+	mov	r3, #0
+	mov	r12, #L2X0_LOCKDOWN_WAY_D_BASE
+	mov	r2, r12
+	str	r3, [r1, r2]
+	add	r2, r2, #4
+	str	r3, [r1, r2]
+	movne	r0, #0x1				@ if l210 we are done
+	strne	r0, [r1, #L2X0_CTRL]			@ enable L2
+	movne	pc, lr
+resume_l310:
+	add	r12, r12, r1
+	add	r12, r12, #L2X0_LOCKDOWN_STRIDE		@ start D lock
+	mov	r2, #0
+	mov	r3, #7
+unlock:
+	str	r2, [r12, #4]				@ I lock
+	str	r2, [r12], #L2X0_LOCKDOWN_STRIDE	@ D lock and increment
+	subs	r3, r3, #1
+	bne 	unlock
+	ldmia	r0!, {r2, r3, r12}
+	str	r2, [r1, #L2X0_TAG_LATENCY_CTRL]
+	str	r3, [r1, #L2X0_DATA_LATENCY_CTRL]
+	str	r12, [r1, #L2X0_ADDR_FILTER_START]
+	ldmia	r0!, {r2, r3, r12}
+	str	r2, [r1, #L2X0_ADDR_FILTER_END]
+	str	r3, [r1, #L2X0_DEBUG_CTRL]
+	str	r12, [r1, #L2X0_PREFETCH_CTRL]
+	ldr	r2, [r0]
+	str	r2, [r1, #L2X0_POWER_CTRL]
+	mov	r0, #0x1
+	str	r0, [r1, #L2X0_CTRL]			@ enable L2
+	mov	pc, lr
+
+/*
+ * Resume function does not use any stack since it might be called
+ * when C environment is not set up yet. It checks the MMU status, and
+ * loads the L2 base address accordingly. Only data accessed if MMU is off is
+ * through PC relative loads, to avoid fetching data from virtual addresses
+ * that would wreak havoc.
+ */
+
+	.data
+	.align
+ENTRY(l2x0_resume)
+	mrc	p15, 0, r2, c1, c0, 0			@ check if MMU is on
+	tst	r2, #0x1
+	ldrne	r0, =l2x0_base
+	adr	r1, l2x0_phys_base
+	ldreq	r1, [r1]				@ it is not r1 = L2 phys
+	ldrne	r1, [r0]				@ it is r1 = L2 virt
+	ldr	r0, [r1, #L2X0_CTRL]			@ L2 enabled ?
+	tst	r0, #0x1
+	movne	pc, lr					@ yes, bug out
+	adr	r0, l2x0_data
+	adr	r2, l2x0_type				@ check L2 type
+	ldr	r2, [r2]
+	cmp	r2, #L2X0_CACHE_ID_PART_L310		@ jump table
+	b 	resume_l210
+ENDPROC(l2x0_restore_context)
+
+l2x0_phys_base:
+	.long 0
+
+l2x0_type:
+	.long 0
+
+	.align
+	.globl	l2x0_data
+	.type	l2x0_data, %object
+l2x0_data:
+	.space	L2X0_REGS_SZ
+	.size	l2x0_data, . - l2x0_data
-- 
1.7.4.4

[RFC PATCH 2/3] ARM: mm: add l2x0 physical address parameter to init

[Russell King - ARM Linux]

On Mon, Sep 26, 2011 at 03:32:40PM +0100, Lorenzo Pieralisi wrote:
> This patch redefines the l2x0_init function, and it adds a
> parameter defining the L2 physical base address in preparation
> for L2 resume support. The device tree init code retrieves the
> physical address from the "reg" array and converts it to a
> physical address pointer.

I've no idea why many people have a really dire time understanding the
following basic fact.  I keep seeing the same things: virtual addresses
as integers and physical addresses as pointers.

Virtual addresses are pointers.
Physical addresses are NOT pointers but integers.

Why?  You CAN'T dereference a physical address when running in the virtual
address space (which is the space which the kernel runs in.)

Stop doing it.

[RFC PATCH 3/3] ARM: mm: add l2x0 suspend/resume support

[Russell King - ARM Linux]

On Mon, Sep 26, 2011 at 03:32:41PM +0100, Lorenzo Pieralisi wrote:
> +	__FINIT
> +/*
> + * Function entered with flags set by jump table in l2x0_resume
> + * If zero flag is set this is a pl310
> + * r0 = l2x0_data
> + * r1 = L2 address
> + */
> +resume_l210:
...
> +	.data
> +	.align
> +ENTRY(l2x0_resume)
...
> +	b 	resume_l210
> +ENDPROC(l2x0_restore_context)

This is not a good idea - jumping from the data segment into the text
segment.  We place initrds and initramfs images between the text and
data segments which can push the branch relocation out of range.

[RFC PATCH 3/3] ARM: mm: add l2x0 suspend/resume support

[Russell King - ARM Linux]

On Mon, Sep 26, 2011 at 03:32:41PM +0100, Lorenzo Pieralisi wrote:
> Context is saved once for all at boot time, along with the L2 physical address
> and cache type.

Why is this assembly code?  As Barry's previous patch shows, it's
entirely possible to save all the state required at init time from
the existing C code without needing any additional functions.

> The resume hook avoids using the stack since it might be called
> before the C environment is up and running and fetches data using
> program counter relative addressing so that it can be run both
> with MMU on or off to simplify its adoption.

What about parts where the security gets in the way of reinitializing
the L2 cache?

> +resume_l210:
> +	ldr	r2, [r0], #4				@ just use scratch regs
> +	str	r2, [r1, #L2X0_AUX_CTRL]
> +	mov	r3, #0
> +	mov	r12, #L2X0_LOCKDOWN_WAY_D_BASE
> +	mov	r2, r12
> +	str	r3, [r1, r2]
> +	add	r2, r2, #4
> +	str	r3, [r1, r2]
> +	movne	r0, #0x1				@ if l210 we are done
> +	strne	r0, [r1, #L2X0_CTRL]			@ enable L2
> +	movne	pc, lr

It's not very obvious where the compare is for this conditional (which is
at the end of l2x0_resume in the .data section.  Also, wouldn't a branch
to the enable at the end of this function be better and a more obvious
flow?

> +resume_l310:
> +	add	r12, r12, r1
> +	add	r12, r12, #L2X0_LOCKDOWN_STRIDE		@ start D lock
> +	mov	r2, #0
> +	mov	r3, #7
> +unlock:
> +	str	r2, [r12, #4]				@ I lock
> +	str	r2, [r12], #L2X0_LOCKDOWN_STRIDE	@ D lock and increment
> +	subs	r3, r3, #1
> +	bne 	unlock

Why not reverse r2 and r3 here?  r3 was already set to zero previously.

And given what you're doing, this code could become:

	mov	r3, #0
	add	r12, r1, #L2X0_LOCKDOWN_WAY_D_BASE
	str	r3, [r12, #4]				@ I lock
	str	r3, [r12], #L2X0_LOCKDOWN_STRIDE	@ D lock and increment
	bne	enable_l2
resume_l310:
	mov	r2, #7
unlock:
	str	r3, [r12, #4]				@ I lock
	str	r3, [r12], #L2X0_LOCKDOWN_STRIDE	@ D lock and increment
	subs	r2, r2, #1
	bne	unlock


> +	ldmia	r0!, {r2, r3, r12}
> +	str	r2, [r1, #L2X0_TAG_LATENCY_CTRL]
> +	str	r3, [r1, #L2X0_DATA_LATENCY_CTRL]
> +	str	r12, [r1, #L2X0_ADDR_FILTER_START]
> +	ldmia	r0!, {r2, r3, r12}
> +	str	r2, [r1, #L2X0_ADDR_FILTER_END]
> +	str	r3, [r1, #L2X0_DEBUG_CTRL]
> +	str	r12, [r1, #L2X0_PREFETCH_CTRL]
> +	ldr	r2, [r0]
> +	str	r2, [r1, #L2X0_POWER_CTRL]

enable_l2:

> +	mov	r0, #0x1
> +	str	r0, [r1, #L2X0_CTRL]			@ enable L2
> +	mov	pc, lr

[RFC PATCH 2/3] ARM: mm: add l2x0 physical address parameter to init

[Lorenzo Pieralisi]

Hi Russell,

thanks for looking at this series.

On Mon, Sep 26, 2011 at 03:51:04PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 26, 2011 at 03:32:40PM +0100, Lorenzo Pieralisi wrote:
> > This patch redefines the l2x0_init function, and it adds a
> > parameter defining the L2 physical base address in preparation
> > for L2 resume support. The device tree init code retrieves the
> > physical address from the "reg" array and converts it to a
> > physical address pointer.
> 
> I've no idea why many people have a really dire time understanding the
> following basic fact.  I keep seeing the same things: virtual addresses
> as integers and physical addresses as pointers.

I should not have converted it to a pointer, you are right, sorry; I am not 
using it as such unless the MMU is off.

> 
> Virtual addresses are pointers.
> Physical addresses are NOT pointers but integers.
> 
> Why?  You CAN'T dereference a physical address when running in the virtual
> address space (which is the space which the kernel runs in.)
> 

Well, to be precise the kernel runs in it when the MMU is on.

By no means, the purpose is NOT to dereference it when the MMU is on and
I am not doing that.

But I agree the physical address should be an integer parameter and be
passed as so, lest someone might want for whatever reason to dereference
it when the MMU is on and unleash hell.

Thank you,
Lorenzo

[RFC PATCH 3/3] ARM: mm: add l2x0 suspend/resume support

[Lorenzo Pieralisi]

On Mon, Sep 26, 2011 at 03:55:03PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 26, 2011 at 03:32:41PM +0100, Lorenzo Pieralisi wrote:
> > +	__FINIT
> > +/*
> > + * Function entered with flags set by jump table in l2x0_resume
> > + * If zero flag is set this is a pl310
> > + * r0 = l2x0_data
> > + * r1 = L2 address
> > + */
> > +resume_l210:
> ...
> > +	.data
> > +	.align
> > +ENTRY(l2x0_resume)
> ...
> > +	b 	resume_l210
> > +ENDPROC(l2x0_restore_context)
>
> This is not a good idea - jumping from the data segment into the text
> segment.  We place initrds and initramfs images between the text and
> data segments which can push the branch relocation out of range.
>

Yes, that's easily fixed, I can move the code in the same section.
That is valid also for callers of e.g. cpu_resume as well though, or the
jump must be an absolute one, because it is the same code pattern, but
from text to data.

Lorenzo

[RFC PATCH 3/3] ARM: mm: add l2x0 suspend/resume support

[Lorenzo Pieralisi]

On Mon, Sep 26, 2011 at 04:06:45PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 26, 2011 at 03:32:41PM +0100, Lorenzo Pieralisi wrote:
> > Context is saved once for all at boot time, along with the L2 physical address
> > and cache type.
> 
> Why is this assembly code?  As Barry's previous patch shows, it's
> entirely possible to save all the state required at init time from
> the existing C code without needing any additional functions.
> 

It need not be, but variables that are needed when the MMU is off (L2
phys address and cache type) must be made visible to C code and
initialized there then.

> > The resume hook avoids using the stack since it might be called
> > before the C environment is up and running and fetches data using
> > program counter relative addressing so that it can be run both
> > with MMU on or off to simplify its adoption.
> 
> What about parts where the security gets in the way of reinitializing
> the L2 cache?
> 

That's a fair point. I think L2 must be initialized by secure code, and
we should not reprogramme it at all.

Having said that, this code only adds support for L2 resume when the
MMU is off to Barry's patch. The security "problem" exists anyway.
I check the control register to see if it is already enabled; all in all 
it is just an assembly stub that can be called before or after cpu_resume,
that's it, to support suspend and idle also when L2 is retained and the
boot firmware just jumps back to Linux on power up.

Whether we need an L2 resume function in the kernel or not, I think it is still
a moot point. Let's keep the discussion going.

> > +resume_l210:
> > +	ldr	r2, [r0], #4				@ just use scratch regs
> > +	str	r2, [r1, #L2X0_AUX_CTRL]
> > +	mov	r3, #0
> > +	mov	r12, #L2X0_LOCKDOWN_WAY_D_BASE
> > +	mov	r2, r12
> > +	str	r3, [r1, r2]
> > +	add	r2, r2, #4
> > +	str	r3, [r1, r2]
> > +	movne	r0, #0x1				@ if l210 we are done
> > +	strne	r0, [r1, #L2X0_CTRL]			@ enable L2
> > +	movne	pc, lr
> 
> It's not very obvious where the compare is for this conditional (which is
> at the end of l2x0_resume in the .data section.  Also, wouldn't a branch
> to the enable at the end of this function be better and a more obvious
> flow?
> 

I added a comment to the function entry for the flags, but it is agreed,
it can be improved; and yes a branch is the way to go.

> > +resume_l310:
> > +	add	r12, r12, r1
> > +	add	r12, r12, #L2X0_LOCKDOWN_STRIDE		@ start D lock
> > +	mov	r2, #0
> > +	mov	r3, #7
> > +unlock:
> > +	str	r2, [r12, #4]				@ I lock
> > +	str	r2, [r12], #L2X0_LOCKDOWN_STRIDE	@ D lock and increment
> > +	subs	r3, r3, #1
> > +	bne 	unlock
> 
> Why not reverse r2 and r3 here?  r3 was already set to zero previously.
> 
> And given what you're doing, this code could become:
> 
> 	mov	r3, #0
> 	add	r12, r1, #L2X0_LOCKDOWN_WAY_D_BASE
> 	str	r3, [r12, #4]				@ I lock
> 	str	r3, [r12], #L2X0_LOCKDOWN_STRIDE	@ D lock and increment
> 	bne	enable_l2
> resume_l310:
> 	mov	r2, #7
> unlock:
> 	str	r3, [r12, #4]				@ I lock
> 	str	r3, [r12], #L2X0_LOCKDOWN_STRIDE	@ D lock and increment
> 	subs	r2, r2, #1
> 	bne	unlock
> 
> 

Definitely, I reshuffled the instructions to adapt the latest changes and I
missed out on this optimization you spotted straight away. Thanks for this.

> > +	ldmia	r0!, {r2, r3, r12}
> > +	str	r2, [r1, #L2X0_TAG_LATENCY_CTRL]
> > +	str	r3, [r1, #L2X0_DATA_LATENCY_CTRL]
> > +	str	r12, [r1, #L2X0_ADDR_FILTER_START]
> > +	ldmia	r0!, {r2, r3, r12}
> > +	str	r2, [r1, #L2X0_ADDR_FILTER_END]
> > +	str	r3, [r1, #L2X0_DEBUG_CTRL]
> > +	str	r12, [r1, #L2X0_PREFETCH_CTRL]
> > +	ldr	r2, [r0]
> > +	str	r2, [r1, #L2X0_POWER_CTRL]
> 
> enable_l2:
> 
> > +	mov	r0, #0x1
> > +	str	r0, [r1, #L2X0_CTRL]			@ enable L2
> > +	mov	pc, lr
> 

Consider it done.

Thanks,
Lorenzo

[RFC PATCH 3/3] ARM: mm: add l2x0 suspend/resume support

[Santosh Shilimkar]

Lorenzo,

On Monday 26 September 2011 10:44 PM, Lorenzo Pieralisi wrote:
> On Mon, Sep 26, 2011 at 04:06:45PM +0100, Russell King - ARM Linux wrote:
>> On Mon, Sep 26, 2011 at 03:32:41PM +0100, Lorenzo Pieralisi wrote:
>>> Context is saved once for all at boot time, along with the L2 physical address
>>> and cache type.
>>
>> Why is this assembly code?  As Barry's previous patch shows, it's
>> entirely possible to save all the state required at init time from
>> the existing C code without needing any additional functions.
>>
> 
I too didn't like this approach.

The platforms which needs to take care of security needs to take
care of the L2 on their own and this code doesn't help them.

Other platform which wants to use the L2 code, looks like are
fine to use the C-version which saves registers in init and restores
it in resume path whenever needed. The patch which was posted
on that was good enough. Regarding turning OFF MMU is really
not necessary unless and until some hardware's are buggy. In that
case, instead of adding that un-necessary code in generic code,
it's better to patch only that buggy SOC code.

Regards
Santosh

[RFC PATCH 3/3] ARM: mm: add l2x0 suspend/resume support

[Lorenzo Pieralisi]

Santosh,

On Tue, Sep 27, 2011 at 06:29:38AM +0100, Santosh Shilimkar wrote:
> Lorenzo,
> 
> On Monday 26 September 2011 10:44 PM, Lorenzo Pieralisi wrote:
> > On Mon, Sep 26, 2011 at 04:06:45PM +0100, Russell King - ARM Linux wrote:
> >> On Mon, Sep 26, 2011 at 03:32:41PM +0100, Lorenzo Pieralisi wrote:
> >>> Context is saved once for all at boot time, along with the L2 physical address
> >>> and cache type.
> >>
> >> Why is this assembly code?  As Barry's previous patch shows, it's
> >> entirely possible to save all the state required at init time from
> >> the existing C code without needing any additional functions.
> >>
> > 
> I too didn't like this approach.
> 
> The platforms which needs to take care of security needs to take
> care of the L2 on their own and this code doesn't help them.
> 
> Other platform which wants to use the L2 code, looks like are
> fine to use the C-version which saves registers in init and restores
> it in resume path whenever needed. The patch which was posted
> on that was good enough. Regarding turning OFF MMU is really
> not necessary unless and until some hardware's are buggy. In that
> case, instead of adding that un-necessary code in generic code,
> it's better to patch only that buggy SOC code.
> 

I summed up the reasons for this patch to exist in the cover letter 
and I mentioned that some bits can be merged IF needed:

http://www.spinics.net/lists/arm-kernel/msg141989.html

Having said that:

- This patch does not switch the MMU off
- This patch does not cover security, and I mentioned that
- Russell referred to saving registers not restoring them, for a reason
- If the resume hook is written in C you cannot call it when the MMU is
  off, which we need if L2 is retained. Unless you think OMAP is the
  only platform supporting L2 RAM retention, but there are people who might
  disagree.

And if you think it is not worth merging since most SoCs will run in
non-secure mode, then that's a very valid point, but it is true for both
patch series, not just for this one.

Lorenzo

[RFC PATCH 3/3] ARM: mm: add l2x0 suspend/resume support

[Santosh Shilimkar]

On Tuesday 27 September 2011 03:58 PM, Lorenzo Pieralisi wrote:
> Santosh,
> 
> On Tue, Sep 27, 2011 at 06:29:38AM +0100, Santosh Shilimkar wrote:
>> Lorenzo,
>>
>> On Monday 26 September 2011 10:44 PM, Lorenzo Pieralisi wrote:
>>> On Mon, Sep 26, 2011 at 04:06:45PM +0100, Russell King - ARM Linux wrote:
>>>> On Mon, Sep 26, 2011 at 03:32:41PM +0100, Lorenzo Pieralisi wrote:
>>>>> Context is saved once for all at boot time, along with the L2 physical address
>>>>> and cache type.
>>>>
>>>> Why is this assembly code?  As Barry's previous patch shows, it's
>>>> entirely possible to save all the state required at init time from
>>>> the existing C code without needing any additional functions.
>>>>
>>>
>> I too didn't like this approach.
>>
>> The platforms which needs to take care of security needs to take
>> care of the L2 on their own and this code doesn't help them.
>>
>> Other platform which wants to use the L2 code, looks like are
>> fine to use the C-version which saves registers in init and restores
>> it in resume path whenever needed. The patch which was posted
>> on that was good enough. Regarding turning OFF MMU is really
>> not necessary unless and until some hardware's are buggy. In that
>> case, instead of adding that un-necessary code in generic code,
>> it's better to patch only that buggy SOC code.
>>
> 
> I summed up the reasons for this patch to exist in the cover letter 
> and I mentioned that some bits can be merged IF needed:
> 
> http://www.spinics.net/lists/arm-kernel/msg141989.html
> 
I read that.

> Having said that:
> 
> - This patch does not switch the MMU off
I noticed that. That comment was miss-placed.

> - This patch does not cover security, and I mentioned that
I never said it is covering security.
"The platforms which needs to take care of security needs to take
care of the L2 on their own and this code doesn't help them."

> - Russell referred to saving registers not restoring them, for a reason
I raised this point on very first patch for L2. My point here was all
the L2 registers cab be saved in init function and C-file.

> - If the resume hook is written in C you cannot call it when the MMU is
>   off, which we need if L2 is retained. Unless you think OMAP is the
>   only platform supporting L2 RAM retention, but there are people who might
>   disagree.
> 
Again I never said that. I guess MX support it as well. But I don't know
whether they have secure limitations as well. If not then probably
this asm resume function can help.

> And if you think it is not worth merging since most SoCs will run in
> non-secure mode, then that's a very valid point, but it is true for both
> patch series, not just for this one.
> 
I agree. From various discussion so far, apart MX, nobody complained
about C-version of the code. Hence the comment. If there are more
platforms which needs it, then the asm version would be useful and
should be preferred over C-version.

Regards
Santosh

[RFC PATCH 3/3] ARM: mm: add l2x0 suspend/resume support

[Russell King - ARM Linux]

On Tue, Sep 27, 2011 at 04:27:32PM +0530, Santosh Shilimkar wrote:
> I agree. From various discussion so far, apart MX, nobody complained
> about C-version of the code. Hence the comment. If there are more
> platforms which needs it, then the asm version would be useful and
> should be preferred over C-version.

Santosh,

Can you also look at Barry Song's version of this infrastructure as
well?  He posted it a couple of days ago:

Subject: [PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode
Message-ID: <1317007569-31213-1-git-send-email-Baohua.Song@csr.com>

with this diffstat:

 4 files changed, 101 insertions(+), 9 deletions(-)

[RFC PATCH 3/3] ARM: mm: add l2x0 suspend/resume support

[Santosh Shilimkar]

On Thursday 29 September 2011 12:54 AM, Russell King - ARM Linux wrote:
> On Tue, Sep 27, 2011 at 04:27:32PM +0530, Santosh Shilimkar wrote:
>> I agree. From various discussion so far, apart MX, nobody complained
>> about C-version of the code. Hence the comment. If there are more
>> platforms which needs it, then the asm version would be useful and
>> should be preferred over C-version.
> 
> Santosh,
> 
> Can you also look at Barry Song's version of this infrastructure as
> well?  He posted it a couple of days ago:
> 
> Subject: [PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode
> Message-ID: <1317007569-31213-1-git-send-email-Baohua.Song@csr.com>
> 
> with this diffstat:
> 
>  4 files changed, 101 insertions(+), 9 deletions(-)
Just finished reviewing it. Apart from missing POR register,
rest of the patch looks fine and should address the asm code
need. I have just requested Shawn to try that out on IMX.

Regards
Santosh