[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Barry Song]

we save the l2x0 registers at the first initialization, and platform codes
can get them to restore l2x0 status after wakeup.

Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 -v2:
 define the structure l2x0_regs to hold all saved registers;
 make saved copy of registers non-static so that platform codes can access them;
 add asm_offset for l2x0 saved regs(l2x0_regs struct) so that asm codes can use

 arch/arm/include/asm/hardware/cache-l2x0.h |   13 +++++
 arch/arm/include/asm/outercache.h          |    7 +++
 arch/arm/kernel/asm-offsets.c              |    9 +++
 arch/arm/mm/cache-l2x0.c                   |   81 ++++++++++++++++++++++++---
 4 files changed, 101 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
index c48cb1e..4f9e81d 100644
--- a/arch/arm/include/asm/hardware/cache-l2x0.h
+++ b/arch/arm/include/asm/hardware/cache-l2x0.h
@@ -96,6 +96,19 @@
 #ifndef __ASSEMBLY__
 extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
 extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
+
+struct l2x0_regs {
+	unsigned long aux_ctrl;
+	/*
+	 * Whether the following registers need to be saved/restored
+	 * depends on platform
+	 */
+	unsigned long tag_latency;
+	unsigned long data_latency;
+	unsigned long filter_start;
+	unsigned long filter_end;
+};
+
 #endif
 
 #endif
diff --git a/arch/arm/include/asm/outercache.h b/arch/arm/include/asm/outercache.h
index d838743..53426c6 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,12 @@ 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)
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 16baba2..b8d72a8 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>
 
 /*
@@ -92,6 +93,14 @@ int main(void)
   DEFINE(S_OLD_R0,		offsetof(struct pt_regs, ARM_ORIG_r0));
   DEFINE(S_FRAME_SIZE,		sizeof(struct pt_regs));
   BLANK();
+#ifdef CONFIG_CACHE_L2X0
+  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_FILTER_START,	offsetof(struct l2x0_regs, filter_start));
+  DEFINE(L2X0_R_FILTER_END,	offsetof(struct l2x0_regs, filter_end));
+  BLANK();
+#endif
 #ifdef CONFIG_CPU_HAS_ASID
   DEFINE(MM_CONTEXT_ID,		offsetof(struct mm_struct, context.id));
   BLANK();
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 0d85d22..c22bee1 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -33,6 +33,14 @@ static DEFINE_SPINLOCK(l2x0_lock);
 static uint32_t l2x0_way_mask;	/* Bitmask of active ways */
 static uint32_t l2x0_size;
 
+struct l2x0_regs l2x0_saved_regs;
+
+struct l2x0_of_data {
+	void (*setup)(const struct device_node *,__u32 *, __u32 *);
+	void (*save)(void);
+	void (*resume)(void);
+};
+
 static inline void cache_wait_way(void __iomem *reg, unsigned long mask)
 {
 	/* wait for cache operation by line or way to complete */
@@ -280,7 +288,7 @@ static void l2x0_disable(void)
 	spin_unlock_irqrestore(&l2x0_lock, flags);
 }
 
-static void __init l2x0_unlock(__u32 cache_id)
+static void l2x0_unlock(__u32 cache_id)
 {
 	int lockregs;
 	int i;
@@ -356,6 +364,8 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
 		/* l2x0 controller is disabled */
 		writel_relaxed(aux, l2x0_base + L2X0_AUX_CTRL);
 
+		l2x0_saved_regs.aux_ctrl = aux;
+
 		l2x0_inv_all();
 
 		/* enable L2X0 */
@@ -445,18 +455,64 @@ static void __init pl310_of_setup(const struct device_node *np,
 	}
 }
 
+static void __init pl310_save(void)
+{
+	l2x0_saved_regs.tag_latency = readl_relaxed(l2x0_base + L2X0_TAG_LATENCY_CTRL);
+	l2x0_saved_regs.data_latency = readl_relaxed(l2x0_base + L2X0_DATA_LATENCY_CTRL);
+	l2x0_saved_regs.filter_end = readl_relaxed(l2x0_base + L2X0_ADDR_FILTER_END);
+	l2x0_saved_regs.filter_start = readl_relaxed(l2x0_base + L2X0_ADDR_FILTER_START);
+}
+
+static void l2x0_resume(void)
+{
+	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & 1)) {
+		/* restore aux ctrl and enable l2 */
+		l2x0_unlock(readl_relaxed(l2x0_base + L2X0_CACHE_ID));
+
+		writel_relaxed(l2x0_saved_regs.aux_ctrl, l2x0_base + L2X0_AUX_CTRL);
+
+		l2x0_inv_all();
+
+		writel_relaxed(1, l2x0_base + L2X0_CTRL);
+	}
+}
+
+static void pl310_resume(void)
+{
+	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & 1)) {
+		/* restore pl310 setup */
+		writel_relaxed(l2x0_saved_regs.tag_latency, l2x0_base + L2X0_TAG_LATENCY_CTRL);
+		writel_relaxed(l2x0_saved_regs.data_latency, l2x0_base + L2X0_DATA_LATENCY_CTRL);
+		writel_relaxed(l2x0_saved_regs.filter_end, l2x0_base + L2X0_ADDR_FILTER_END);
+		writel_relaxed(l2x0_saved_regs.filter_start, l2x0_base + L2X0_ADDR_FILTER_START);
+	}
+
+	l2x0_resume();
+}
+
+static const struct l2x0_of_data pl310_data = {
+	pl310_of_setup,
+	pl310_save,
+	pl310_resume,
+};
+
+static const struct l2x0_of_data l2x0_data = {
+	l2x0_of_setup,
+	NULL,
+	l2x0_resume,
+};
+
 static const struct of_device_id l2x0_ids[] __initconst = {
-	{ .compatible = "arm,pl310-cache", .data = pl310_of_setup },
-	{ .compatible = "arm,l220-cache", .data = l2x0_of_setup },
-	{ .compatible = "arm,l210-cache", .data = l2x0_of_setup },
+	{ .compatible = "arm,pl310-cache", .data = (void *)&pl310_data },
+	{ .compatible = "arm,l220-cache", .data = (void *)&l2x0_data },
+	{ .compatible = "arm,l210-cache", .data = (void *)&l2x0_data },
 	{}
 };
 
 int __init l2x0_of_init(__u32 aux_val, __u32 aux_mask)
 {
 	struct device_node *np;
-	void (*l2_setup)(const struct device_node *np,
-		__u32 *aux_val, __u32 *aux_mask);
+	struct l2x0_of_data *data;
 
 	np = of_find_matching_node(NULL, l2x0_ids);
 	if (!np)
@@ -465,13 +521,20 @@ int __init l2x0_of_init(__u32 aux_val, __u32 aux_mask)
 	if (!l2x0_base)
 		return -ENOMEM;
 
+	data = of_match_node(l2x0_ids, np)->data;
+
 	/* 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);
+		if (data->setup)
+			data->setup(np, &aux_val, &aux_mask);
 	}
+
+	if (data->save)
+		data->save();
+
 	l2x0_init(l2x0_base, aux_val, aux_mask);
+
+	outer_cache.resume = data->resume;
 	return 0;
 }
 #endif
-- 
1.7.1



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Santosh Shilimkar]

Barry,

On Monday 26 September 2011 08:56 AM, Barry Song wrote:
> we save the l2x0 registers at the first initialization, and platform codes
> can get them to restore l2x0 status after wakeup.
> 
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---
>  -v2:
>  define the structure l2x0_regs to hold all saved registers;
>  make saved copy of registers non-static so that platform codes can access them;
>  add asm_offset for l2x0 saved regs(l2x0_regs struct) so that asm codes can use
> 
>  arch/arm/include/asm/hardware/cache-l2x0.h |   13 +++++
>  arch/arm/include/asm/outercache.h          |    7 +++
>  arch/arm/kernel/asm-offsets.c              |    9 +++
>  arch/arm/mm/cache-l2x0.c                   |   81 ++++++++++++++++++++++++---
>  4 files changed, 101 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
> index c48cb1e..4f9e81d 100644
> --- a/arch/arm/include/asm/hardware/cache-l2x0.h
> +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
> @@ -96,6 +96,19 @@
>  #ifndef __ASSEMBLY__
>  extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
>  extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
> +
> +struct l2x0_regs {
> +	unsigned long aux_ctrl;
> +	/*
> +	 * Whether the following registers need to be saved/restored
> +	 * depends on platform
> +	 */
> +	unsigned long tag_latency;
> +	unsigned long data_latency;
> +	unsigned long filter_start;
> +	unsigned long filter_end;
> +};
> +
You are missing POR register here which is available on
PL310 versions. You should add that.

[...]
>  static inline void outer_inv_range(phys_addr_t start, phys_addr_t end)
> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
> index 16baba2..b8d72a8 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>
>  
>  /*
> @@ -92,6 +93,14 @@ int main(void)
>    DEFINE(S_OLD_R0,		offsetof(struct pt_regs, ARM_ORIG_r0));
>    DEFINE(S_FRAME_SIZE,		sizeof(struct pt_regs));
>    BLANK();
> +#ifdef CONFIG_CACHE_L2X0
> +  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_FILTER_START,	offsetof(struct l2x0_regs, filter_start));
> +  DEFINE(L2X0_R_FILTER_END,	offsetof(struct l2x0_regs, filter_end));
Add POR as commented earlier.

Rest of the patch looks good to me. I have ignored DT related
changes since I don't understand them.

Shawn,
Have you tried this patch on IMX and see if you are
able to use it from the asm code?

Regards
Santosh

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Shawn Guo]

On Thu, Sep 29, 2011 at 11:05:58AM +0530, Santosh Shilimkar wrote:
> Barry,
> 
> On Monday 26 September 2011 08:56 AM, Barry Song wrote:
> > we save the l2x0 registers at the first initialization, and platform codes
> > can get them to restore l2x0 status after wakeup.
> > 
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > ---
> >  -v2:
> >  define the structure l2x0_regs to hold all saved registers;
> >  make saved copy of registers non-static so that platform codes can access them;
> >  add asm_offset for l2x0 saved regs(l2x0_regs struct) so that asm codes can use
> > 
> >  arch/arm/include/asm/hardware/cache-l2x0.h |   13 +++++
> >  arch/arm/include/asm/outercache.h          |    7 +++
> >  arch/arm/kernel/asm-offsets.c              |    9 +++
> >  arch/arm/mm/cache-l2x0.c                   |   81 ++++++++++++++++++++++++---
> >  4 files changed, 101 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
> > index c48cb1e..4f9e81d 100644
> > --- a/arch/arm/include/asm/hardware/cache-l2x0.h
> > +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
> > @@ -96,6 +96,19 @@
> >  #ifndef __ASSEMBLY__
> >  extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
> >  extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
> > +
> > +struct l2x0_regs {
> > +	unsigned long aux_ctrl;
> > +	/*
> > +	 * Whether the following registers need to be saved/restored
> > +	 * depends on platform
> > +	 */
> > +	unsigned long tag_latency;
> > +	unsigned long data_latency;
> > +	unsigned long filter_start;
> > +	unsigned long filter_end;
> > +};
> > +
> You are missing POR register here which is available on
> PL310 versions. You should add that.
> 
> [...]
> >  static inline void outer_inv_range(phys_addr_t start, phys_addr_t end)
> > diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
> > index 16baba2..b8d72a8 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>
> >  
> >  /*
> > @@ -92,6 +93,14 @@ int main(void)
> >    DEFINE(S_OLD_R0,		offsetof(struct pt_regs, ARM_ORIG_r0));
> >    DEFINE(S_FRAME_SIZE,		sizeof(struct pt_regs));
> >    BLANK();
> > +#ifdef CONFIG_CACHE_L2X0
> > +  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_FILTER_START,	offsetof(struct l2x0_regs, filter_start));
> > +  DEFINE(L2X0_R_FILTER_END,	offsetof(struct l2x0_regs, filter_end));
> Add POR as commented earlier.
> 
> Rest of the patch looks good to me. I have ignored DT related
> changes since I don't understand them.
> 
> Shawn,
> Have you tried this patch on IMX and see if you are
> able to use it from the asm code?
> 
Will give it a test today.

-- 
Regards,
Shawn

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Barry Song]

2011/9/29 Shawn Guo <shawn.guo@freescale.com>:
> On Thu, Sep 29, 2011 at 11:05:58AM +0530, Santosh Shilimkar wrote:
>> Barry,
>>
>> On Monday 26 September 2011 08:56 AM, Barry Song wrote:
>> > we save the l2x0 registers at the first initialization, and platform codes
>> > can get them to restore l2x0 status after wakeup.
>> >
>> > Cc: Shawn Guo <shawn.guo@linaro.org>
>> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> > Signed-off-by: Barry Song <Baohua.Song@csr.com>
>> > ---
>> > ?-v2:
>> > ?define the structure l2x0_regs to hold all saved registers;
>> > ?make saved copy of registers non-static so that platform codes can access them;
>> > ?add asm_offset for l2x0 saved regs(l2x0_regs struct) so that asm codes can use
>> >
>> > ?arch/arm/include/asm/hardware/cache-l2x0.h | ? 13 +++++
>> > ?arch/arm/include/asm/outercache.h ? ? ? ? ?| ? ?7 +++
>> > ?arch/arm/kernel/asm-offsets.c ? ? ? ? ? ? ?| ? ?9 +++
>> > ?arch/arm/mm/cache-l2x0.c ? ? ? ? ? ? ? ? ? | ? 81 ++++++++++++++++++++++++---
>> > ?4 files changed, 101 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
>> > index c48cb1e..4f9e81d 100644
>> > --- a/arch/arm/include/asm/hardware/cache-l2x0.h
>> > +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
>> > @@ -96,6 +96,19 @@
>> > ?#ifndef __ASSEMBLY__
>> > ?extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
>> > ?extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
>> > +
>> > +struct l2x0_regs {
>> > + ? unsigned long aux_ctrl;
>> > + ? /*
>> > + ? ?* Whether the following registers need to be saved/restored
>> > + ? ?* depends on platform
>> > + ? ?*/
>> > + ? unsigned long tag_latency;
>> > + ? unsigned long data_latency;
>> > + ? unsigned long filter_start;
>> > + ? unsigned long filter_end;
>> > +};
>> > +
>> You are missing POR register here which is available on
>> PL310 versions. You should add that.
>>
>> [...]
>> > ?static inline void outer_inv_range(phys_addr_t start, phys_addr_t end)
>> > diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
>> > index 16baba2..b8d72a8 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>
>> >
>> > ?/*
>> > @@ -92,6 +93,14 @@ int main(void)
>> > ? ?DEFINE(S_OLD_R0, ? ? ? ? offsetof(struct pt_regs, ARM_ORIG_r0));
>> > ? ?DEFINE(S_FRAME_SIZE, ? ? ? ? ? ? sizeof(struct pt_regs));
>> > ? ?BLANK();
>> > +#ifdef CONFIG_CACHE_L2X0
>> > + ?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_FILTER_START, ? ? ?offsetof(struct l2x0_regs, filter_start));
>> > + ?DEFINE(L2X0_R_FILTER_END, ? ? ? ?offsetof(struct l2x0_regs, filter_end));
>> Add POR as commented earlier.
>>
>> Rest of the patch looks good to me. I have ignored DT related
>> changes since I don't understand them.
>>
>> Shawn,
>> Have you tried this patch on IMX and see if you are
>> able to use it from the asm code?
>>
> Will give it a test today

well, thanks. once after you test and think it is ok, i'll send v3
with fixing the lost POR.

>
> --
> Regards,
> Shawn

-barry

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Russell King - ARM Linux]

On Thu, Sep 29, 2011 at 05:28:29PM +0800, Shawn Guo wrote:
> On Sun, Sep 25, 2011 at 08:26:09PM -0700, Barry Song wrote:
> > we save the l2x0 registers at the first initialization, and platform codes
> > can get them to restore l2x0 status after wakeup.
> > 
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > ---
> >  -v2:
> >  define the structure l2x0_regs to hold all saved registers;
> >  make saved copy of registers non-static so that platform codes can access them;
> >  add asm_offset for l2x0 saved regs(l2x0_regs struct) so that asm codes can use
> > 
> 
> I'm trying to migrate imx6q to this infrastructure, but seeing some
> facts that might stop me from the effort.  Please help me understand
> these facts are correct.
> 
> It seems to me that the only benefit that imx6q gains from the
> infrastructure is the register saving, and I fail to see any benefit
> on the restoring.

That's correct - because I believe that's the only part which is worth
doing in common code.  The restoring tends to be platform dependent so
I don't see the point of having a bunch of code in common files to do
that which almost no one uses.

>  * I still have to save physical base of l2x0 for register restoring.

That could be added to the structure.

>  * I do not have the physical address of l2x0_saved_regs, and I have
>    to manage to get it somehow.

That's true no matter what - we could store the physical address of
the struct somewhere, but then in order for you to obtain that, you'd
somehow need to know the physical address of _that_ location too - so
there's no benefit there.

It can be obtained simply by: __pa(l2x0_saved_regs) in C code, and
saving that in a location known to the platform prior to suspend.

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Shawn Guo]

On Sun, Sep 25, 2011 at 08:26:09PM -0700, Barry Song wrote:
> we save the l2x0 registers at the first initialization, and platform codes
> can get them to restore l2x0 status after wakeup.
> 
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---
>  -v2:
>  define the structure l2x0_regs to hold all saved registers;
>  make saved copy of registers non-static so that platform codes can access them;
>  add asm_offset for l2x0 saved regs(l2x0_regs struct) so that asm codes can use
> 

I'm trying to migrate imx6q to this infrastructure, but seeing some
facts that might stop me from the effort.  Please help me understand
these facts are correct.

It seems to me that the only benefit that imx6q gains from the
infrastructure is the register saving, and I fail to see any benefit
on the restoring.

 * I still have to save physical base of l2x0 for register restoring.
 * I do not have the physical address of l2x0_saved_regs, and I have
   to manage to get it somehow.

-- 
Regards,
Shawn

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Barry Song]

2011/9/29 Shawn Guo <shawn.guo@freescale.com>:
> On Sun, Sep 25, 2011 at 08:26:09PM -0700, Barry Song wrote:
>> we save the l2x0 registers at the first initialization, and platform codes
>> can get them to restore l2x0 status after wakeup.
>>
>> Cc: Shawn Guo <shawn.guo@linaro.org>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>> ---
>> ?-v2:
>> ?define the structure l2x0_regs to hold all saved registers;
>> ?make saved copy of registers non-static so that platform codes can access them;
>> ?add asm_offset for l2x0 saved regs(l2x0_regs struct) so that asm codes can use
>>
>
> I'm trying to migrate imx6q to this infrastructure, but seeing some
> facts that might stop me from the effort. ?Please help me understand
> these facts are correct.
>
> It seems to me that the only benefit that imx6q gains from the
> infrastructure is the register saving, and I fail to see any benefit
> on the restoring.
>
> ?* I still have to save physical base of l2x0 for register restoring.
> ?* I do not have the physical address of l2x0_saved_regs, and I have
> ? to manage to get it somehow.

well. the phyical address should be added so that mmu-off asm codes can get it.

>
> --
> Regards,
> Shawn

-barry

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Lorenzo Pieralisi]

On Thu, Sep 29, 2011 at 06:35:58AM +0100, Santosh Shilimkar wrote:
> Barry,
> 
> On Monday 26 September 2011 08:56 AM, Barry Song wrote:
> > we save the l2x0 registers at the first initialization, and platform codes
> > can get them to restore l2x0 status after wakeup.
> > 
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > ---

[...]

> > +
> > +struct l2x0_regs {
> > +	unsigned long aux_ctrl;
> > +	/*
> > +	 * Whether the following registers need to be saved/restored
> > +	 * depends on platform
> > +	 */
> > +	unsigned long tag_latency;
> > +	unsigned long data_latency;
> > +	unsigned long filter_start;
> > +	unsigned long filter_end;
> > +};
> > +
> You are missing POR register here which is available on
> PL310 versions. You should add that.
> 

In latest versions there are Prefetch and Power control registers, but
it depends on the revision. What should we do in this case Santosh ?
PCR is a superset of POR from r3p0 onwards, but Power control ? Leave it
to platform code ?

> [...]
> >  static inline void outer_inv_range(phys_addr_t start, phys_addr_t end)
> > diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
> > index 16baba2..b8d72a8 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>
> >  
> >  /*
> > @@ -92,6 +93,14 @@ int main(void)
> >    DEFINE(S_OLD_R0,		offsetof(struct pt_regs, ARM_ORIG_r0));
> >    DEFINE(S_FRAME_SIZE,		sizeof(struct pt_regs));
> >    BLANK();
> > +#ifdef CONFIG_CACHE_L2X0
> > +  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_FILTER_START,	offsetof(struct l2x0_regs, filter_start));
> > +  DEFINE(L2X0_R_FILTER_END,	offsetof(struct l2x0_regs, filter_end));
> Add POR as commented earlier.

See above.

Thanks,
Lorenzo

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Santosh Shilimkar]

On Thursday 29 September 2011 03:52 PM, Lorenzo Pieralisi wrote:
> On Thu, Sep 29, 2011 at 06:35:58AM +0100, Santosh Shilimkar wrote:
>> Barry,
>>
>> On Monday 26 September 2011 08:56 AM, Barry Song wrote:
>>> we save the l2x0 registers at the first initialization, and platform codes
>>> can get them to restore l2x0 status after wakeup.
>>>
>>> Cc: Shawn Guo <shawn.guo@linaro.org>
>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>>> ---
> 
> [...]
> 
>>> +
>>> +struct l2x0_regs {
>>> +	unsigned long aux_ctrl;
>>> +	/*
>>> +	 * Whether the following registers need to be saved/restored
>>> +	 * depends on platform
>>> +	 */
>>> +	unsigned long tag_latency;
>>> +	unsigned long data_latency;
>>> +	unsigned long filter_start;
>>> +	unsigned long filter_end;
>>> +};
>>> +
>> You are missing POR register here which is available on
>> PL310 versions. You should add that.
>>
> 
> In latest versions there are Prefetch and Power control registers, but
> it depends on the revision. What should we do in this case Santosh ?
> PCR is a superset of POR from r3p0 onwards, but Power control ? Leave it
> to platform code ?
> 
Good point Lorenzo.
C code resume can be fixed by marking the validity based
on PL310 revision register.
The asm code restore with MMU OFF code, would be handled
with platform code with this patch and platform code can choose
restore of only supported registers.

Regards
Santosh

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Shawn Guo]

On Thu, Sep 29, 2011 at 10:24:06AM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 29, 2011 at 05:28:29PM +0800, Shawn Guo wrote:
> > On Sun, Sep 25, 2011 at 08:26:09PM -0700, Barry Song wrote:
> > > we save the l2x0 registers at the first initialization, and platform codes
> > > can get them to restore l2x0 status after wakeup.
> > > 
> > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > > ---
> > >  -v2:
> > >  define the structure l2x0_regs to hold all saved registers;
> > >  make saved copy of registers non-static so that platform codes can access them;
> > >  add asm_offset for l2x0 saved regs(l2x0_regs struct) so that asm codes can use
> > > 
> > 
> > I'm trying to migrate imx6q to this infrastructure, but seeing some
> > facts that might stop me from the effort.  Please help me understand
> > these facts are correct.
> > 
> > It seems to me that the only benefit that imx6q gains from the
> > infrastructure is the register saving, and I fail to see any benefit
> > on the restoring.
> 
> That's correct - because I believe that's the only part which is worth
> doing in common code.  The restoring tends to be platform dependent so
> I don't see the point of having a bunch of code in common files to do
> that which almost no one uses.
> 
> >  * I still have to save physical base of l2x0 for register restoring.
> 
> That could be added to the structure.
> 
> >  * I do not have the physical address of l2x0_saved_regs, and I have
> >    to manage to get it somehow.
> 
> That's true no matter what - we could store the physical address of
> the struct somewhere, but then in order for you to obtain that, you'd
> somehow need to know the physical address of _that_ location too - so
> there's no benefit there.
> 
> It can be obtained simply by: __pa(l2x0_saved_regs) in C code, and
> saving that in a location known to the platform prior to suspend.
> 
Yeah, that's why I want to get imx6q stay away from this infrastructure
right now.  I do not see any simplicity and cleanup on imx6q current
code by migrating to this infrastructure.

-- 
Regards,
Shawn

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Russell King - ARM Linux]

On Thu, Sep 29, 2011 at 08:44:20PM +0800, Shawn Guo wrote:
> Yeah, that's why I want to get imx6q stay away from this infrastructure
> right now.  I do not see any simplicity and cleanup on imx6q current
> code by migrating to this infrastructure.

Why?  If the data is already saved for you, then there's no reason not
to use it.  The fact that some generic code doesn't give you _exactly_
everything you'd want is not a reason to avoid it.

The cleanup for imx6q is that it would no longer have to have its own
distinct code for saving the register values - and that's a danmed good
thing.

The idea here is that we consolidate what _can_ be consolidated (which
is the register saving.)

If you feel soo strongly that it's not worth doing, then let's stop
wasting time and review effort on this, and instead have _every_ SoC
implementing their own private L2 cache handling on resume.

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Shawn Guo]

On Thu, Sep 29, 2011 at 01:50:26PM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 29, 2011 at 08:44:20PM +0800, Shawn Guo wrote:
> > Yeah, that's why I want to get imx6q stay away from this infrastructure
> > right now.  I do not see any simplicity and cleanup on imx6q current
> > code by migrating to this infrastructure.
> 
> Why?  If the data is already saved for you, then there's no reason not
> to use it.  The fact that some generic code doesn't give you _exactly_
> everything you'd want is not a reason to avoid it.
> 
> The cleanup for imx6q is that it would no longer have to have its own
> distinct code for saving the register values - and that's a danmed good
> thing.
> 
It cleans up the register saving but requires additional code handling
physical address of l2x0_saved_regs.

> The idea here is that we consolidate what _can_ be consolidated (which
> is the register saving.)
> 
> If you feel soo strongly that it's not worth doing, then let's stop
> wasting time and review effort on this, and instead have _every_ SoC
> implementing their own private L2 cache handling on resume.
> 
This infrastructure is definitely good thing for platform that L2 will
be lost during suspend.  But for imx6q which retains L2, I would not
migrate it until the physical base of L2 and l2x0_saved_regs itself can
be retrieved from infrastructure too.

-- 
Regards,
Shawn

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Russell King - ARM Linux]

On Thu, Sep 29, 2011 at 09:12:31PM +0800, Shawn Guo wrote:
> On Thu, Sep 29, 2011 at 01:50:26PM +0100, Russell King - ARM Linux wrote:
> > On Thu, Sep 29, 2011 at 08:44:20PM +0800, Shawn Guo wrote:
> > > Yeah, that's why I want to get imx6q stay away from this infrastructure
> > > right now.  I do not see any simplicity and cleanup on imx6q current
> > > code by migrating to this infrastructure.
> > 
> > Why?  If the data is already saved for you, then there's no reason not
> > to use it.  The fact that some generic code doesn't give you _exactly_
> > everything you'd want is not a reason to avoid it.
> > 
> > The cleanup for imx6q is that it would no longer have to have its own
> > distinct code for saving the register values - and that's a danmed good
> > thing.
> > 
> It cleans up the register saving but requires additional code handling
> physical address of l2x0_saved_regs.
> 
> > The idea here is that we consolidate what _can_ be consolidated (which
> > is the register saving.)
> > 
> > If you feel soo strongly that it's not worth doing, then let's stop
> > wasting time and review effort on this, and instead have _every_ SoC
> > implementing their own private L2 cache handling on resume.
> > 
> This infrastructure is definitely good thing for platform that L2 will
> be lost during suspend.  But for imx6q which retains L2, I would not
> migrate it until the physical base of L2 and l2x0_saved_regs itself can
> be retrieved from infrastructure too.

Putting the physical base of the L2 cache inside l2x0_saved_regs is
trivial (and I think will probably be done.)

What _can't_ be done - because it's _totally_ idiotic as I previously
described - is to do anything about the physical address of l2x0_saved_regs
itself.

Look - think about it for a moment.  Let's say we do:

struct l2x0_saved_regs l2x0_saved_regs;
unsigned long phys_l2x0_saved_regs;

l2x0_init()
{
	phys_l2x0_saved_regs = __pa(&l2x0_saved_regs);
}

Does this buy us anything?  It most certainly does not - it actually
buys us additional complexity, because now we need to have platform
code knowing the _physical_ address of phys_l2x0_saved_regs in order to
get the _physical_ address of l2x0_saved_regs.  Totally idiotic and
pointless - platform code might as well just use __pa(&l2x0_saved_regs)
directly in its suspend path to place it _somewhere_ that its own L2
resume code (which common code has no knowledge of) can access.

It's not that big a deal in any case - you can do this in your platform
code:

	.data
.globl phys_l2x0_saved_regs
phys_l2x0_saved_regs:
	.long	0

which the resume code can then access directly - and then obtain the values
directly from the l2x0_saved_reg structure.

And in your suspend initialization:

extern unsigned long phys_l2x0_saved_regs;

int my_suspend_init()
{
...
	phys_l2x0_saved_regs = __pa(&l2x0_saved_regs);
}

So I think you're barking up the wrong tree if you think there's
*anything* which generic code can do to make access to l2x0_saved_regs
any easier than it is in this patch.

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Shawn Guo]

On Thu, Sep 29, 2011 at 02:26:49PM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 29, 2011 at 09:12:31PM +0800, Shawn Guo wrote:
> > On Thu, Sep 29, 2011 at 01:50:26PM +0100, Russell King - ARM Linux wrote:
> > > On Thu, Sep 29, 2011 at 08:44:20PM +0800, Shawn Guo wrote:
> > > > Yeah, that's why I want to get imx6q stay away from this infrastructure
> > > > right now.  I do not see any simplicity and cleanup on imx6q current
> > > > code by migrating to this infrastructure.
> > > 
> > > Why?  If the data is already saved for you, then there's no reason not
> > > to use it.  The fact that some generic code doesn't give you _exactly_
> > > everything you'd want is not a reason to avoid it.
> > > 
> > > The cleanup for imx6q is that it would no longer have to have its own
> > > distinct code for saving the register values - and that's a danmed good
> > > thing.
> > > 
> > It cleans up the register saving but requires additional code handling
> > physical address of l2x0_saved_regs.
> > 
> > > The idea here is that we consolidate what _can_ be consolidated (which
> > > is the register saving.)
> > > 
> > > If you feel soo strongly that it's not worth doing, then let's stop
> > > wasting time and review effort on this, and instead have _every_ SoC
> > > implementing their own private L2 cache handling on resume.
> > > 
> > This infrastructure is definitely good thing for platform that L2 will
> > be lost during suspend.  But for imx6q which retains L2, I would not
> > migrate it until the physical base of L2 and l2x0_saved_regs itself can
> > be retrieved from infrastructure too.
> 
> Putting the physical base of the L2 cache inside l2x0_saved_regs is
> trivial (and I think will probably be done.)
> 
> What _can't_ be done - because it's _totally_ idiotic as I previously
> described - is to do anything about the physical address of l2x0_saved_regs
> itself.
> 
> Look - think about it for a moment.  Let's say we do:
> 
> struct l2x0_saved_regs l2x0_saved_regs;
> unsigned long phys_l2x0_saved_regs;
> 
> l2x0_init()
> {
> 	phys_l2x0_saved_regs = __pa(&l2x0_saved_regs);
> }
> 
> Does this buy us anything?  It most certainly does not - it actually
> buys us additional complexity, because now we need to have platform
> code knowing the _physical_ address of phys_l2x0_saved_regs in order to
> get the _physical_ address of l2x0_saved_regs.  Totally idiotic and
> pointless - platform code might as well just use __pa(&l2x0_saved_regs)
> directly in its suspend path to place it _somewhere_ that its own L2
> resume code (which common code has no knowledge of) can access.
> 
> It's not that big a deal in any case - you can do this in your platform
> code:
> 
> 	.data
> .globl phys_l2x0_saved_regs
> phys_l2x0_saved_regs:
> 	.long	0
> 
> which the resume code can then access directly - and then obtain the values
> directly from the l2x0_saved_reg structure.
> 
> And in your suspend initialization:
> 
> extern unsigned long phys_l2x0_saved_regs;
> 
> int my_suspend_init()
> {
> ...
> 	phys_l2x0_saved_regs = __pa(&l2x0_saved_regs);
> }
> 
> So I think you're barking up the wrong tree if you think there's
> *anything* which generic code can do to make access to l2x0_saved_regs
> any easier than it is in this patch.
> 
Yes, you are right.  __pa(&l2x0_saved_regs) can be handled in imx6q
platform code.

-- 
Regards,
Shawn

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Shawn Guo]

On Sun, Sep 25, 2011 at 08:26:09PM -0700, Barry Song wrote:
> we save the l2x0 registers at the first initialization, and platform codes
> can get them to restore l2x0 status after wakeup.
> 
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---

Barry,

You may want to fix those checkpatch error and warnings in v3.

total: 1 errors, 15 warnings, 181 lines checked

-- 
Regards,
Shawn

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Barry Song]

Hi Shawn,

2011/9/29 Shawn Guo <shawn.guo@freescale.com>:
> On Sun, Sep 25, 2011 at 08:26:09PM -0700, Barry Song wrote:
>> we save the l2x0 registers at the first initialization, and platform codes
>> can get them to restore l2x0 status after wakeup.
>>
>> Cc: Shawn Guo <shawn.guo@linaro.org>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>> ---
>
> Barry,
>
> You may want to fix those checkpatch error and warnings in v3.

Thanks. i will.
since people have basically agreen about what should be fixed and what
should not be, i'd like to send v3 tomorrow. you might help to test on
imx6q as well.

>
> total: 1 errors, 15 warnings, 181 lines checked
>
> --
> Regards,
> Shawn

-barry

[PATCH v2] ARM: cache-l2x0: add resume entry for l2 in secure mode

[Shawn Guo]

On Thu, Sep 29, 2011 at 10:55:29PM +0800, Barry Song wrote:
> Hi Shawn,
> 
> 2011/9/29 Shawn Guo <shawn.guo@freescale.com>:
> > On Sun, Sep 25, 2011 at 08:26:09PM -0700, Barry Song wrote:
> >> we save the l2x0 registers at the first initialization, and platform codes
> >> can get them to restore l2x0 status after wakeup.
> >>
> >> Cc: Shawn Guo <shawn.guo@linaro.org>
> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> >> ---
> >
> > Barry,
> >
> > You may want to fix those checkpatch error and warnings in v3.
> 
> Thanks. i will.
> since people have basically agreen about what should be fixed and what
> should not be, i'd like to send v3 tomorrow. you might help to test on
> imx6q as well.
> 
Sure.

-- 
Regards,
Shawn