[PATCH v2 0/2] make reinitialization of ARM core components possible

[PATCH v2 0/2] make reinitialization of ARM core components possible

[Shawn Guo]

When ARM core resumes from low-power mode where losing power, for my
example: CA-9 MPCore resumes from Dormant/Shutdown, we have to
reinitialize components like L2 Cache, GIC and SCU to bring system
back to work.

The patch set basically removes __init annotation from a bunch of
initialization functions, so that platform resume procedure can call
into them again to set those components up.

Changes since v1:
 * Drop the GIC patch, as it's been handled by [1] nicely.
 * Drop the change of adding empty l2x0_of_init(), as it's been
   handled by [2].

Shawn Guo (2):
      ARM: cache-l2x0: remove __init annotation from initialization functions
      ARM: smp_scu: remove __init annotation from scu_enable()

 arch/arm/include/asm/hardware/cache-l2x0.h |    2 +-
 arch/arm/kernel/smp_scu.c                  |    2 +-
 arch/arm/mm/cache-l2x0.c                   |   10 +++++-----
 3 files changed, 7 insertions(+), 7 deletions(-)

[1] [PATCH v2 0/5] CPU PM notifiers
http://thread.gmane.org/gmane.linux.ports.arm.kernel/131212/focus=131353

[2] [PATCH 1/7] ARM: l2x0: add empty l2x0_of_init
http://article.gmane.org/gmane.linux.ports.arm.kernel/130878

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Shawn Guo]

If ARM core gets powered off during suspend, L2 cache controller
has to be reinitialized by resume procedure.

The patch removes __init annotation from a few initialization
functions to make the reinitialization possible.  For example,
platform resume function can call l2x0_of_init() to get L2 cache
back to work.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/include/asm/hardware/cache-l2x0.h |    2 +-
 arch/arm/mm/cache-l2x0.c                   |   10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
index d22765c..d270310 100644
--- a/arch/arm/include/asm/hardware/cache-l2x0.h
+++ b/arch/arm/include/asm/hardware/cache-l2x0.h
@@ -89,7 +89,7 @@
 #define L2X0_ADDR_FILTER_EN		1
 
 #ifndef __ASSEMBLY__
-extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
+extern void l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
 #if defined(CONFIG_CACHE_L2X0) && defined(CONFIG_OF)
 extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
 #else
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index c035b9a..7835cb6 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -280,7 +280,7 @@ static void l2x0_disable(void)
 	spin_unlock_irqrestore(&l2x0_lock, flags);
 }
 
-void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
+void l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
 {
 	__u32 aux;
 	__u32 cache_id;
@@ -356,7 +356,7 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
 }
 
 #ifdef CONFIG_OF
-static void __init l2x0_of_setup(const struct device_node *np,
+static void l2x0_of_setup(const struct device_node *np,
 				 __u32 *aux_val, __u32 *aux_mask)
 {
 	u32 data[2] = { 0, 0 };
@@ -390,7 +390,7 @@ static void __init l2x0_of_setup(const struct device_node *np,
 	*aux_mask &= ~mask;
 }
 
-static void __init pl310_of_setup(const struct device_node *np,
+static void pl310_of_setup(const struct device_node *np,
 				  __u32 *aux_val, __u32 *aux_mask)
 {
 	u32 data[3] = { 0, 0, 0 };
@@ -424,14 +424,14 @@ static void __init pl310_of_setup(const struct device_node *np,
 	}
 }
 
-static const struct of_device_id l2x0_ids[] __initconst = {
+static const struct of_device_id l2x0_ids[] = {
 	{ .compatible = "arm,pl310-cache", .data = pl310_of_setup },
 	{ .compatible = "arm,l220-cache", .data = l2x0_of_setup },
 	{ .compatible = "arm,l210-cache", .data = l2x0_of_setup },
 	{}
 };
 
-int __init l2x0_of_init(__u32 aux_val, __u32 aux_mask)
+int l2x0_of_init(__u32 aux_val, __u32 aux_mask)
 {
 	struct device_node *np;
 	void (*l2_setup)(const struct device_node *np,
-- 
1.7.4.1

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Linus Walleij]

On Tue, Sep 6, 2011 at 7:48 AM, Shawn Guo <shawn.guo@linaro.org> wrote:

> The patch removes __init annotation from a few initialization
> functions to make the reinitialization possible. ?For example,
> platform resume function can call l2x0_of_init() to get L2 cache
> back to work.

This will collide with patch 7080/1:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7080/1

Before you put it into the patch tracker can you please rebase it
on that patch and also remove the __init tag from the
l2x0_unlock() function?

Thanks,
Linus Walleij

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Shawn Guo]

On Tue, Sep 06, 2011 at 09:19:03AM +0200, Linus Walleij wrote:
> On Tue, Sep 6, 2011 at 7:48 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> > The patch removes __init annotation from a few initialization
> > functions to make the reinitialization possible. ?For example,
> > platform resume function can call l2x0_of_init() to get L2 cache
> > back to work.
> 
> This will collide with patch 7080/1:
> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7080/1
> 
> Before you put it into the patch tracker can you please rebase it
> on that patch and also remove the __init tag from the
> l2x0_unlock() function?
> 
Yes, will do, if rmk asks me to put the patch into patch tracker.

-- 
Regards,
Shawn

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Russell King - ARM Linux]

On Tue, Sep 06, 2011 at 01:48:26PM +0800, Shawn Guo wrote:
> If ARM core gets powered off during suspend, L2 cache controller
> has to be reinitialized by resume procedure.
> 
> The patch removes __init annotation from a few initialization
> functions to make the reinitialization possible.  For example,
> platform resume function can call l2x0_of_init() to get L2 cache
> back to work.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/include/asm/hardware/cache-l2x0.h |    2 +-
>  arch/arm/mm/cache-l2x0.c                   |   10 +++++-----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
> index d22765c..d270310 100644
> --- a/arch/arm/include/asm/hardware/cache-l2x0.h
> +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
> @@ -89,7 +89,7 @@
>  #define L2X0_ADDR_FILTER_EN		1
>  
>  #ifndef __ASSEMBLY__
> -extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
> +extern void l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
>  #if defined(CONFIG_CACHE_L2X0) && defined(CONFIG_OF)
>  extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
>  #else
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index c035b9a..7835cb6 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -280,7 +280,7 @@ static void l2x0_disable(void)
>  	spin_unlock_irqrestore(&l2x0_lock, flags);
>  }
>  
> -void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
> +void l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
>  {
>  	__u32 aux;
>  	__u32 cache_id;

I'm unconvinced about the wise-ness of this.  We read-modify-write the
auxillary control register, which means some bits are preserved from
the initial boot.  If the boot loader sets the L2 cache up for normal
boot and not for resume, we'll end up with different L2 cache settings.

We've historically seen this kind of thing with boot loaders over the
years, to the point where systems boot at one CPU clock rate but resume
at some other CPU clock rate.

So, it may be wiser to implement a 'save' and 'restore' interface
instead so that we can be sure that things are setup in the same way
after resume.

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Santosh]

On Wednesday 14 September 2011 02:12 PM, Russell King - ARM Linux wrote:
> On Tue, Sep 06, 2011 at 01:48:26PM +0800, Shawn Guo wrote:
>> If ARM core gets powered off during suspend, L2 cache controller
>> has to be reinitialized by resume procedure.
>>
>> The patch removes __init annotation from a few initialization
>> functions to make the reinitialization possible.  For example,
>> platform resume function can call l2x0_of_init() to get L2 cache
>> back to work.
>>
>> Signed-off-by: Shawn Guo<shawn.guo@linaro.org>
>> ---
>>   arch/arm/include/asm/hardware/cache-l2x0.h |    2 +-
>>   arch/arm/mm/cache-l2x0.c                   |   10 +++++-----
>>   2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
>> index d22765c..d270310 100644
>> --- a/arch/arm/include/asm/hardware/cache-l2x0.h
>> +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
>> @@ -89,7 +89,7 @@
>>   #define L2X0_ADDR_FILTER_EN		1
>>
>>   #ifndef __ASSEMBLY__
>> -extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
>> +extern void l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
>>   #if defined(CONFIG_CACHE_L2X0)&&  defined(CONFIG_OF)
>>   extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
>>   #else
>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
>> index c035b9a..7835cb6 100644
>> --- a/arch/arm/mm/cache-l2x0.c
>> +++ b/arch/arm/mm/cache-l2x0.c
>> @@ -280,7 +280,7 @@ static void l2x0_disable(void)
>>   	spin_unlock_irqrestore(&l2x0_lock, flags);
>>   }
>>
>> -void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
>> +void l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
>>   {
>>   	__u32 aux;
>>   	__u32 cache_id;
>
> I'm unconvinced about the wise-ness of this.  We read-modify-write the
> auxillary control register, which means some bits are preserved from
> the initial boot.  If the boot loader sets the L2 cache up for normal
> boot and not for resume, we'll end up with different L2 cache settings.
>
> We've historically seen this kind of thing with boot loaders over the
> years, to the point where systems boot at one CPU clock rate but resume
> at some other CPU clock rate.
>
> So, it may be wiser to implement a 'save' and 'restore' interface
> instead so that we can be sure that things are setup in the same way
> after resume.
>
I agree with Russell and has same concern when I commented on V1 of this
patch.

As mentioned earlier, you need to save only once since non of the L2
configuration values will change. So 'save ones' and 'restore always
when context lost' should be the right way to go about it.

Regards
Santosh

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Russell King - ARM Linux]

On Wed, Sep 14, 2011 at 02:23:54PM +0530, Santosh wrote:
> As mentioned earlier, you need to save only once since non of the L2
> configuration values will change. So 'save ones' and 'restore always
> when context lost' should be the right way to go about it.

Might as well arrange for the save to be at initialization time -
there's no point saving the configuration at every suspend.

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Barry Song]

2011/9/14 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Tue, Sep 06, 2011 at 01:48:26PM +0800, Shawn Guo wrote:
>> If ARM core gets powered off during suspend, L2 cache controller
>> has to be reinitialized by resume procedure.
>>
>> The patch removes __init annotation from a few initialization
>> functions to make the reinitialization possible. ?For example,
>> platform resume function can call l2x0_of_init() to get L2 cache
>> back to work.
>>
>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> ---
>> ?arch/arm/include/asm/hardware/cache-l2x0.h | ? ?2 +-
>> ?arch/arm/mm/cache-l2x0.c ? ? ? ? ? ? ? ? ? | ? 10 +++++-----
>> ?2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
>> index d22765c..d270310 100644
>> --- a/arch/arm/include/asm/hardware/cache-l2x0.h
>> +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
>> @@ -89,7 +89,7 @@
>> ?#define L2X0_ADDR_FILTER_EN ? ? ? ? ?1
>>
>> ?#ifndef __ASSEMBLY__
>> -extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
>> +extern void l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
>> ?#if defined(CONFIG_CACHE_L2X0) && defined(CONFIG_OF)
>> ?extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
>> ?#else
>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
>> index c035b9a..7835cb6 100644
>> --- a/arch/arm/mm/cache-l2x0.c
>> +++ b/arch/arm/mm/cache-l2x0.c
>> @@ -280,7 +280,7 @@ static void l2x0_disable(void)
>> ? ? ? spin_unlock_irqrestore(&l2x0_lock, flags);
>> ?}
>>
>> -void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
>> +void l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
>> ?{
>> ? ? ? __u32 aux;
>> ? ? ? __u32 cache_id;
>
> I'm unconvinced about the wise-ness of this. ?We read-modify-write the
> auxillary control register, which means some bits are preserved from
> the initial boot. ?If the boot loader sets the L2 cache up for normal
> boot and not for resume, we'll end up with different L2 cache settings.
>
> We've historically seen this kind of thing with boot loaders over the
> years, to the point where systems boot at one CPU clock rate but resume
> at some other CPU clock rate.
>
> So, it may be wiser to implement a 'save' and 'restore' interface
> instead so that we can be sure that things are setup in the same way
> after resume.

could we have an outer_cache save/restore as below,

struct outer_cache_fns {
        void (*inv_range)(unsigned long, unsigned long);
        void (*clean_range)(unsigned long, unsigned long);
        void (*flush_range)(unsigned long, unsigned long);
        void (*flush_all)(void);
        void (*inv_all)(void);
        void (*disable)(void);
#ifdef CONFIG_OUTER_CACHE_SYNC
        void (*sync)(void);
#endif
        void (*set_debug)(unsigned long);
 +       void (*save)(void);
 +      void (*restore)(void);
};

then let cache-l2x0.c fill the two callbacks, and all systems call
outer_save/outer_restore?

-barry

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Shawn Guo]

On Wed, Sep 14, 2011 at 09:42:38AM +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 06, 2011 at 01:48:26PM +0800, Shawn Guo wrote:
> > If ARM core gets powered off during suspend, L2 cache controller
> > has to be reinitialized by resume procedure.
> > 
> > The patch removes __init annotation from a few initialization
> > functions to make the reinitialization possible.  For example,
> > platform resume function can call l2x0_of_init() to get L2 cache
> > back to work.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  arch/arm/include/asm/hardware/cache-l2x0.h |    2 +-
> >  arch/arm/mm/cache-l2x0.c                   |   10 +++++-----
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
> > index d22765c..d270310 100644
> > --- a/arch/arm/include/asm/hardware/cache-l2x0.h
> > +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
> > @@ -89,7 +89,7 @@
> >  #define L2X0_ADDR_FILTER_EN		1
> >  
> >  #ifndef __ASSEMBLY__
> > -extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
> > +extern void l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
> >  #if defined(CONFIG_CACHE_L2X0) && defined(CONFIG_OF)
> >  extern int l2x0_of_init(__u32 aux_val, __u32 aux_mask);
> >  #else
> > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> > index c035b9a..7835cb6 100644
> > --- a/arch/arm/mm/cache-l2x0.c
> > +++ b/arch/arm/mm/cache-l2x0.c
> > @@ -280,7 +280,7 @@ static void l2x0_disable(void)
> >  	spin_unlock_irqrestore(&l2x0_lock, flags);
> >  }
> >  
> > -void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
> > +void l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
> >  {
> >  	__u32 aux;
> >  	__u32 cache_id;
> 
> I'm unconvinced about the wise-ness of this.  We read-modify-write the
> auxillary control register, which means some bits are preserved from
> the initial boot.  If the boot loader sets the L2 cache up for normal
> boot and not for resume, we'll end up with different L2 cache settings.
> 
> We've historically seen this kind of thing with boot loaders over the
> years, to the point where systems boot at one CPU clock rate but resume
> at some other CPU clock rate.
> 
I would think this is a problem in the kernel.  Kernel initialization
code should put all these stuff into a known state to ensure boot and
resume of the kernel do not result in a different state, shouldn't it?

> So, it may be wiser to implement a 'save' and 'restore' interface
> instead so that we can be sure that things are setup in the same way
> after resume.
> 
This will introduce extra codes in every single platform.

-- 
Regards,
Shawn

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Russell King - ARM Linux]

On Thu, Sep 15, 2011 at 09:39:39AM +0800, Shawn Guo wrote:
> On Wed, Sep 14, 2011 at 09:42:38AM +0100, Russell King - ARM Linux wrote:
> > I'm unconvinced about the wise-ness of this.  We read-modify-write the
> > auxillary control register, which means some bits are preserved from
> > the initial boot.  If the boot loader sets the L2 cache up for normal
> > boot and not for resume, we'll end up with different L2 cache settings.
> > 
> > We've historically seen this kind of thing with boot loaders over the
> > years, to the point where systems boot at one CPU clock rate but resume
> > at some other CPU clock rate.
> > 
> I would think this is a problem in the kernel.  Kernel initialization
> code should put all these stuff into a known state to ensure boot and
> resume of the kernel do not result in a different state, shouldn't it?

grep the kernel for l2x0_init() and look at the mask and value registers.
Note that any bit set in the mask is preserved from boot time.

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Barry Song]

2011/9/15 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Thu, Sep 15, 2011 at 09:39:39AM +0800, Shawn Guo wrote:
>> On Wed, Sep 14, 2011 at 09:42:38AM +0100, Russell King - ARM Linux wrote:
>> > I'm unconvinced about the wise-ness of this. ?We read-modify-write the
>> > auxillary control register, which means some bits are preserved from
>> > the initial boot. ?If the boot loader sets the L2 cache up for normal
>> > boot and not for resume, we'll end up with different L2 cache settings.
>> >
>> > We've historically seen this kind of thing with boot loaders over the
>> > years, to the point where systems boot at one CPU clock rate but resume
>> > at some other CPU clock rate.
>> >
>> I would think this is a problem in the kernel. ?Kernel initialization
>> code should put all these stuff into a known state to ensure boot and
>> resume of the kernel do not result in a different state, shouldn't it?
>
> grep the kernel for l2x0_init() and look at the mask and value registers.
> Note that any bit set in the mask is preserved from boot time.

if we have a save/restore interface, it looks it will be very
complicated. different l2 need to save different registers.

pl310:
tag-latency(not all pl310 has TAG_LATENCY_CTRL ctrl setting)
arm,data-latency(not all pl310 has L2X0_DATA_LATENCY_CTRL ctrl setting)
arm,filter-ranges(not all pl310 has filter range setting)
L2X0_AUX_CTRL
L2X0_CTRL
So the save interface needs to know what should be saved. but who can
tell them those if the save interface is not in SoC-specific file but
in arch/arm/mm/cache-l2x0.c?

when we resume, we must disable l2 if bootloader has enabled it and
restore all registers.

so it looks like making l2 resume specific to chip is also the right
choice even for chips which will lose l2 in suspend cycles.

-barry

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Russell King - ARM Linux]

On Fri, Sep 16, 2011 at 11:24:36AM +0800, Barry Song wrote:
> if we have a save/restore interface, it looks it will be very
> complicated. different l2 need to save different registers.

Why?  It's quite simple as far as I can see:

static u32 l2_aux_ctrl;

void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
{
	...
	aux = readl_relaxed(l2x0_base + L2X0_AUX_CTRL);

	aux &= aux_mask;
	aux |= aux_val;

	l2_aux_ctrl = aux;
	...
}

void l2x0_resume(void)
{
	bool need_setup = false;

	if (l2_aux_ctrl != readl_relaxed(l2x0_base + L2X0_AUX_CTRL))
		need_setup = true;
	        
	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & 1)) {
		/* Make sure that I&D is not locked down when starting */
		l2x0_unlock(cache_id);

		/* l2x0 controller is disabled */
		writel_relaxed(l2_aux_ctrl, l2x0_base + L2X0_AUX_CTRL);

		l2x0_inv_all();

		/* enable L2X0 */
		writel_relaxed(1, l2x0_base + L2X0_CTRL);
	}
}

and we can do a similar thing when initializing the PL310 and resuming
the PL310 - add a new outer_cache callback called 'resume' to be pointed
at the relevant resume function which knows which registers to restore.

> when we resume, we must disable l2 if bootloader has enabled it and
> restore all registers.

That's not possible in SoCs operating in non-secure mode from generic
code, as some of these registers will not be accessible.  They can only
be programmed from platform specific code due to the complexities of
dealing with the abhorrent secure monitor stuff.

I'm now starting to think that we don't actually want any resume code
at the L2 level - most SoCs will be operating in non-secure mode (I
believe it's only ARM's development platforms which operate in secure
mode) and so most of the generic code which will need to write to the
L2 control registers on resume will fail.

Even re-calling the initialization functions probably does nothing on
parts operating in secure mode - whether at boot or at resume.

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Barry Song]

2011/9/17 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Fri, Sep 16, 2011 at 11:24:36AM +0800, Barry Song wrote:
>> if we have a save/restore interface, it looks it will be very
>> complicated. different l2 need to save different registers.
>
> Why? ?It's quite simple as far as I can see:
>
> static u32 l2_aux_ctrl;
>
> void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
> {
> ? ? ? ?...
> ? ? ? ?aux = readl_relaxed(l2x0_base + L2X0_AUX_CTRL);
>
> ? ? ? ?aux &= aux_mask;
> ? ? ? ?aux |= aux_val;
>
> ? ? ? ?l2_aux_ctrl = aux;
> ? ? ? ?...
> }
>
> void l2x0_resume(void)
> {
> ? ? ? ?bool need_setup = false;
>
> ? ? ? ?if (l2_aux_ctrl != readl_relaxed(l2x0_base + L2X0_AUX_CTRL))
> ? ? ? ? ? ? ? ?need_setup = true;
>
> ? ? ? ?if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & 1)) {
> ? ? ? ? ? ? ? ?/* Make sure that I&D is not locked down when starting */
> ? ? ? ? ? ? ? ?l2x0_unlock(cache_id);
>
> ? ? ? ? ? ? ? ?/* l2x0 controller is disabled */
> ? ? ? ? ? ? ? ?writel_relaxed(l2_aux_ctrl, l2x0_base + L2X0_AUX_CTRL);
>
> ? ? ? ? ? ? ? ?l2x0_inv_all();
>
> ? ? ? ? ? ? ? ?/* enable L2X0 */
> ? ? ? ? ? ? ? ?writel_relaxed(1, l2x0_base + L2X0_CTRL);
> ? ? ? ?}
> }
>

for aux ctrl, it is really simple. but how about the following:

 393 static void __init pl310_of_setup(const struct device_node *np,
 394                                   __u32 *aux_val, __u32 *aux_mask)
 395 {
 396         u32 data[3] = { 0, 0, 0 };
 397         u32 tag[3] = { 0, 0, 0 };
 398         u32 filter[2] = { 0, 0 };
 399
 400         of_property_read_u32_array(np, "arm,tag-latency", tag,
ARRAY_SIZE(tag));
 401         if (tag[0] && tag[1] && tag[2])
 402                 writel_relaxed(
 403                         ((tag[0] - 1) << L2X0_LATENCY_CTRL_RD_SHIFT) |
 404                         ((tag[1] - 1) << L2X0_LATENCY_CTRL_WR_SHIFT) |
 405                         ((tag[2] - 1) << L2X0_LATENCY_CTRL_SETUP_SHIFT),
 406                         l2x0_base + L2X0_TAG_LATENCY_CTRL);
 407
 408         of_property_read_u32_array(np, "arm,data-latency",
 409                                    data, ARRAY_SIZE(data));
 410         if (data[0] && data[1] && data[2])
 411                 writel_relaxed(
 412                         ((data[0] - 1) << L2X0_LATENCY_CTRL_RD_SHIFT) |
 413                         ((data[1] - 1) << L2X0_LATENCY_CTRL_WR_SHIFT) |
 414                         ((data[2] - 1) << L2X0_LATENCY_CTRL_SETUP_SHIFT),
 415                         l2x0_base + L2X0_DATA_LATENCY_CTRL);
 416
 417         of_property_read_u32_array(np, "arm,filter-ranges",
 418                                    filter, ARRAY_SIZE(filter));
 419         if (filter[0] && filter[1]) {
 420                 writel_relaxed(ALIGN(filter[0] + filter[1], SZ_1M),
 421                                l2x0_base + L2X0_ADDR_FILTER_END);
 422                 writel_relaxed((filter[0] & ~(SZ_1M - 1)) |
L2X0_ADDR_FILTER_EN,
 423                                l2x0_base + L2X0_ADDR_FILTER_START);
 424         }
 425 }

not all l2 have all these registers. for example, it seems only prima2
has L2X0_ADDR_FILTER_START/END by now. and only some platforms set
latency too.

one possible way is that we save one register if we have the related
property in dts. but it can be wrong too. we can't decide whether we
should save one register only according to whether dt has the
property.  for example, if bootloader has setup all of them when cold
boot, users maybe don't add these "arm,data-latency" prop in dts at
all.

it seems we need some other ways to know what we should save for
latency ctrl, filter range....

> and we can do a similar thing when initializing the PL310 and resuming
> the PL310 - add a new outer_cache callback called 'resume' to be pointed
> at the relevant resume function which knows which registers to restore.

in my last reply, i also suggested this:
struct outer_cache_fns {
       void (*inv_range)(unsigned long, unsigned long);
       void (*clean_range)(unsigned long, unsigned long);
       void (*flush_range)(unsigned long, unsigned long);
       void (*flush_all)(void);
       void (*inv_all)(void);
       void (*disable)(void);
#ifdef CONFIG_OUTER_CACHE_SYNC
       void (*sync)(void);
#endif
       void (*set_debug)(unsigned long);
 +       void (*save)(void);
 +      void (*restore)(void);
};

but it looks we only need resume since we can save all in init phase:

struct outer_cache_fns {
       void (*inv_range)(unsigned long, unsigned long);
       void (*clean_range)(unsigned long, unsigned long);
       void (*flush_range)(unsigned long, unsigned long);
       void (*flush_all)(void);
       void (*inv_all)(void);
       void (*disable)(void);
#ifdef CONFIG_OUTER_CACHE_SYNC
       void (*sync)(void);
#endif
       void (*set_debug)(unsigned long);
 +       void (*resume)(void);
};

>
>> when we resume, we must disable l2 if bootloader has enabled it and
>> restore all registers.
>
> That's not possible in SoCs operating in non-secure mode from generic
> code, as some of these registers will not be accessible. ?They can only
> be programmed from platform specific code due to the complexities of
> dealing with the abhorrent secure monitor stuff.
>
> I'm now starting to think that we don't actually want any resume code
> at the L2 level - most SoCs will be operating in non-secure mode (I
> believe it's only ARM's development platforms which operate in secure
> mode) and so most of the generic code which will need to write to the
> L2 control registers on resume will fail.

that is probably real. at least our team never get any requirement to
use secure mode.

>
> Even re-calling the initialization functions probably does nothing on
> parts operating in secure mode - whether at boot or at resume.
>

-barry

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Russell King - ARM Linux]

On Sat, Sep 17, 2011 at 10:41:58PM +0800, Barry Song wrote:
> for aux ctrl, it is really simple. but how about the following:
> 
>  393 static void __init pl310_of_setup(const struct device_node *np,
>  394                                   __u32 *aux_val, __u32 *aux_mask)
>  395 {
>  396         u32 data[3] = { 0, 0, 0 };
>  397         u32 tag[3] = { 0, 0, 0 };
>  398         u32 filter[2] = { 0, 0 };
>  399
>  400         of_property_read_u32_array(np, "arm,tag-latency", tag,
> ARRAY_SIZE(tag));
>  401         if (tag[0] && tag[1] && tag[2])
>  402                 writel_relaxed(
>  403                         ((tag[0] - 1) << L2X0_LATENCY_CTRL_RD_SHIFT) |
>  404                         ((tag[1] - 1) << L2X0_LATENCY_CTRL_WR_SHIFT) |
>  405                         ((tag[2] - 1) << L2X0_LATENCY_CTRL_SETUP_SHIFT),
>  406                         l2x0_base + L2X0_TAG_LATENCY_CTRL);
>  407
>  408         of_property_read_u32_array(np, "arm,data-latency",
>  409                                    data, ARRAY_SIZE(data));
>  410         if (data[0] && data[1] && data[2])
>  411                 writel_relaxed(
>  412                         ((data[0] - 1) << L2X0_LATENCY_CTRL_RD_SHIFT) |
>  413                         ((data[1] - 1) << L2X0_LATENCY_CTRL_WR_SHIFT) |
>  414                         ((data[2] - 1) << L2X0_LATENCY_CTRL_SETUP_SHIFT),
>  415                         l2x0_base + L2X0_DATA_LATENCY_CTRL);
>  416
>  417         of_property_read_u32_array(np, "arm,filter-ranges",
>  418                                    filter, ARRAY_SIZE(filter));
>  419         if (filter[0] && filter[1]) {
>  420                 writel_relaxed(ALIGN(filter[0] + filter[1], SZ_1M),
>  421                                l2x0_base + L2X0_ADDR_FILTER_END);
>  422                 writel_relaxed((filter[0] & ~(SZ_1M - 1)) |
> L2X0_ADDR_FILTER_EN,
>  423                                l2x0_base + L2X0_ADDR_FILTER_START);
>  424         }
>  425 }
> 
> not all l2 have all these registers. for example, it seems only prima2
> has L2X0_ADDR_FILTER_START/END by now. and only some platforms set
> latency too.

Save them as normal.  If there aren't the values in DT, read them in the
above functions and save the value.  Then...

> > and we can do a similar thing when initializing the PL310 and resuming
> > the PL310 - add a new outer_cache callback called 'resume' to be pointed
> > at the relevant resume function which knows which registers to restore.

Do that.

> in my last reply, i also suggested this:
> struct outer_cache_fns {
>        void (*inv_range)(unsigned long, unsigned long);
>        void (*clean_range)(unsigned long, unsigned long);
>        void (*flush_range)(unsigned long, unsigned long);
>        void (*flush_all)(void);
>        void (*inv_all)(void);
>        void (*disable)(void);
> #ifdef CONFIG_OUTER_CACHE_SYNC
>        void (*sync)(void);
> #endif
>        void (*set_debug)(unsigned long);
>  +       void (*save)(void);
>  +      void (*restore)(void);
> };
> 
> but it looks we only need resume since we can save all in init phase:

Correct.

> > That's not possible in SoCs operating in non-secure mode from generic
> > code, as some of these registers will not be accessible. ?They can only
> > be programmed from platform specific code due to the complexities of
> > dealing with the abhorrent secure monitor stuff.
> >
> > I'm now starting to think that we don't actually want any resume code
> > at the L2 level - most SoCs will be operating in non-secure mode (I
> > believe it's only ARM's development platforms which operate in secure
> > mode) and so most of the generic code which will need to write to the
> > L2 control registers on resume will fail.
> 
> that is probably real. at least our team never get any requirement to
> use secure mode.

So probably the best we can do in generic code is present platform code
with a data structure describing the intended register values which were
used at init time, and we leave it to platform code to do the necessary
reprogramming that's required in the way that's required for the
abhorrent secure monitor on their platform.

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Barry Song]

2011/9/17 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sat, Sep 17, 2011 at 10:41:58PM +0800, Barry Song wrote:
>> for aux ctrl, it is really simple. but how about the following:
>>
>> ?393 static void __init pl310_of_setup(const struct device_node *np,
>> ?394 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __u32 *aux_val, __u32 *aux_mask)
>> ?395 {
>> ?396 ? ? ? ? u32 data[3] = { 0, 0, 0 };
>> ?397 ? ? ? ? u32 tag[3] = { 0, 0, 0 };
>> ?398 ? ? ? ? u32 filter[2] = { 0, 0 };
>> ?399
>> ?400 ? ? ? ? of_property_read_u32_array(np, "arm,tag-latency", tag,
>> ARRAY_SIZE(tag));
>> ?401 ? ? ? ? if (tag[0] && tag[1] && tag[2])
>> ?402 ? ? ? ? ? ? ? ? writel_relaxed(
>> ?403 ? ? ? ? ? ? ? ? ? ? ? ? ((tag[0] - 1) << L2X0_LATENCY_CTRL_RD_SHIFT) |
>> ?404 ? ? ? ? ? ? ? ? ? ? ? ? ((tag[1] - 1) << L2X0_LATENCY_CTRL_WR_SHIFT) |
>> ?405 ? ? ? ? ? ? ? ? ? ? ? ? ((tag[2] - 1) << L2X0_LATENCY_CTRL_SETUP_SHIFT),
>> ?406 ? ? ? ? ? ? ? ? ? ? ? ? l2x0_base + L2X0_TAG_LATENCY_CTRL);
>> ?407
>> ?408 ? ? ? ? of_property_read_u32_array(np, "arm,data-latency",
>> ?409 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?data, ARRAY_SIZE(data));
>> ?410 ? ? ? ? if (data[0] && data[1] && data[2])
>> ?411 ? ? ? ? ? ? ? ? writel_relaxed(
>> ?412 ? ? ? ? ? ? ? ? ? ? ? ? ((data[0] - 1) << L2X0_LATENCY_CTRL_RD_SHIFT) |
>> ?413 ? ? ? ? ? ? ? ? ? ? ? ? ((data[1] - 1) << L2X0_LATENCY_CTRL_WR_SHIFT) |
>> ?414 ? ? ? ? ? ? ? ? ? ? ? ? ((data[2] - 1) << L2X0_LATENCY_CTRL_SETUP_SHIFT),
>> ?415 ? ? ? ? ? ? ? ? ? ? ? ? l2x0_base + L2X0_DATA_LATENCY_CTRL);
>> ?416
>> ?417 ? ? ? ? of_property_read_u32_array(np, "arm,filter-ranges",
>> ?418 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?filter, ARRAY_SIZE(filter));
>> ?419 ? ? ? ? if (filter[0] && filter[1]) {
>> ?420 ? ? ? ? ? ? ? ? writel_relaxed(ALIGN(filter[0] + filter[1], SZ_1M),
>> ?421 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?l2x0_base + L2X0_ADDR_FILTER_END);
>> ?422 ? ? ? ? ? ? ? ? writel_relaxed((filter[0] & ~(SZ_1M - 1)) |
>> L2X0_ADDR_FILTER_EN,
>> ?423 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?l2x0_base + L2X0_ADDR_FILTER_START);
>> ?424 ? ? ? ? }
>> ?425 }
>>
>> not all l2 have all these registers. for example, it seems only prima2
>> has L2X0_ADDR_FILTER_START/END by now. and only some platforms set
>> latency too.
>
> Save them as normal. ?If there aren't the values in DT, read them in the
> above functions and save the value. ?Then...
>
>> > and we can do a similar thing when initializing the PL310 and resuming
>> > the PL310 - add a new outer_cache callback called 'resume' to be pointed
>> > at the relevant resume function which knows which registers to restore.
>
> Do that.
>
>> in my last reply, i also suggested this:
>> struct outer_cache_fns {
>> ? ? ? ?void (*inv_range)(unsigned long, unsigned long);
>> ? ? ? ?void (*clean_range)(unsigned long, unsigned long);
>> ? ? ? ?void (*flush_range)(unsigned long, unsigned long);
>> ? ? ? ?void (*flush_all)(void);
>> ? ? ? ?void (*inv_all)(void);
>> ? ? ? ?void (*disable)(void);
>> #ifdef CONFIG_OUTER_CACHE_SYNC
>> ? ? ? ?void (*sync)(void);
>> #endif
>> ? ? ? ?void (*set_debug)(unsigned long);
>> ?+ ? ? ? void (*save)(void);
>> ?+ ? ? ?void (*restore)(void);
>> };
>>
>> but it looks we only need resume since we can save all in init phase:
>
> Correct.
Do you think the following is what you want?

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/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 0d85d22..4722707 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -32,6 +32,14 @@ static void __iomem *l2x0_base;
 static DEFINE_SPINLOCK(l2x0_lock);
 static uint32_t l2x0_way_mask;	/* Bitmask of active ways */
 static uint32_t l2x0_size;
+static u32 l2x0_aux_ctrl;
+static u32 l2x0_tag_latency, l2x0_data_latency, l2x0_filter_start,
l2x0_filter_end;
+
+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)
 {
@@ -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_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_tag_latency = readl_relaxed(l2x0_base + L2X0_TAG_LATENCY_CTRL);
+	l2x0_data_latency = readl_relaxed(l2x0_base + L2X0_DATA_LATENCY_CTRL);
+	l2x0_filter_end = readl_relaxed(l2x0_base + L2X0_ADDR_FILTER_END);
+	l2x0_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_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_tag_latency, l2x0_base + L2X0_TAG_LATENCY_CTRL);
+		writel_relaxed(l2x0_data_latency, l2x0_base + L2X0_DATA_LATENCY_CTRL);
+		writel_relaxed(l2x0_filter_end, l2x0_base + L2X0_ADDR_FILTER_END);
+		writel_relaxed(l2x0_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


-barry

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Barry Song]

2011/9/17 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sat, Sep 17, 2011 at 10:41:58PM +0800, Barry Song wrote:
>> for aux ctrl, it is really simple. but how about the following:
>>
>> ?393 static void __init pl310_of_setup(const struct device_node *np,
>> ?394 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __u32 *aux_val, __u32 *aux_mask)
>> ?395 {
>> ?396 ? ? ? ? u32 data[3] = { 0, 0, 0 };
>> ?397 ? ? ? ? u32 tag[3] = { 0, 0, 0 };
>> ?398 ? ? ? ? u32 filter[2] = { 0, 0 };
>> ?399
>> ?400 ? ? ? ? of_property_read_u32_array(np, "arm,tag-latency", tag,
>> ARRAY_SIZE(tag));
>> ?401 ? ? ? ? if (tag[0] && tag[1] && tag[2])
>> ?402 ? ? ? ? ? ? ? ? writel_relaxed(
>> ?403 ? ? ? ? ? ? ? ? ? ? ? ? ((tag[0] - 1) << L2X0_LATENCY_CTRL_RD_SHIFT) |
>> ?404 ? ? ? ? ? ? ? ? ? ? ? ? ((tag[1] - 1) << L2X0_LATENCY_CTRL_WR_SHIFT) |
>> ?405 ? ? ? ? ? ? ? ? ? ? ? ? ((tag[2] - 1) << L2X0_LATENCY_CTRL_SETUP_SHIFT),
>> ?406 ? ? ? ? ? ? ? ? ? ? ? ? l2x0_base + L2X0_TAG_LATENCY_CTRL);
>> ?407
>> ?408 ? ? ? ? of_property_read_u32_array(np, "arm,data-latency",
>> ?409 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?data, ARRAY_SIZE(data));
>> ?410 ? ? ? ? if (data[0] && data[1] && data[2])
>> ?411 ? ? ? ? ? ? ? ? writel_relaxed(
>> ?412 ? ? ? ? ? ? ? ? ? ? ? ? ((data[0] - 1) << L2X0_LATENCY_CTRL_RD_SHIFT) |
>> ?413 ? ? ? ? ? ? ? ? ? ? ? ? ((data[1] - 1) << L2X0_LATENCY_CTRL_WR_SHIFT) |
>> ?414 ? ? ? ? ? ? ? ? ? ? ? ? ((data[2] - 1) << L2X0_LATENCY_CTRL_SETUP_SHIFT),
>> ?415 ? ? ? ? ? ? ? ? ? ? ? ? l2x0_base + L2X0_DATA_LATENCY_CTRL);
>> ?416
>> ?417 ? ? ? ? of_property_read_u32_array(np, "arm,filter-ranges",
>> ?418 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?filter, ARRAY_SIZE(filter));
>> ?419 ? ? ? ? if (filter[0] && filter[1]) {
>> ?420 ? ? ? ? ? ? ? ? writel_relaxed(ALIGN(filter[0] + filter[1], SZ_1M),
>> ?421 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?l2x0_base + L2X0_ADDR_FILTER_END);
>> ?422 ? ? ? ? ? ? ? ? writel_relaxed((filter[0] & ~(SZ_1M - 1)) |
>> L2X0_ADDR_FILTER_EN,
>> ?423 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?l2x0_base + L2X0_ADDR_FILTER_START);
>> ?424 ? ? ? ? }
>> ?425 }
>>
>> not all l2 have all these registers. for example, it seems only prima2
>> has L2X0_ADDR_FILTER_START/END by now. and only some platforms set
>> latency too.
>
> Save them as normal. ?If there aren't the values in DT, read them in the
> above functions and save the value. ?Then...
Do you think the following is what you want?

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/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 0d85d22..4722707 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -32,6 +32,14 @@ static void __iomem *l2x0_base;
 static DEFINE_SPINLOCK(l2x0_lock);
 static uint32_t l2x0_way_mask;	/* Bitmask of active ways */
 static uint32_t l2x0_size;
+static u32 l2x0_aux_ctrl;
+static u32 l2x0_tag_latency, l2x0_data_latency, l2x0_filter_start,
l2x0_filter_end;
+
+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)
 {
@@ -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_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_tag_latency = readl_relaxed(l2x0_base + L2X0_TAG_LATENCY_CTRL);
+	l2x0_data_latency = readl_relaxed(l2x0_base + L2X0_DATA_LATENCY_CTRL);
+	l2x0_filter_end = readl_relaxed(l2x0_base + L2X0_ADDR_FILTER_END);
+	l2x0_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_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_tag_latency, l2x0_base + L2X0_TAG_LATENCY_CTRL);
+		writel_relaxed(l2x0_data_latency, l2x0_base + L2X0_DATA_LATENCY_CTRL);
+		writel_relaxed(l2x0_filter_end, l2x0_base + L2X0_ADDR_FILTER_END);
+		writel_relaxed(l2x0_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


-barry

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Russell King - ARM Linux]

On Mon, Sep 19, 2011 at 01:33:39PM +0800, Barry Song wrote:
> Do you think the following is what you want?

Almost.  A couple of things:

1. Making the variables static means that folk like OMAP can't read the
   values at resume time from their assembly (forcing them to save and
   restore them, rather than using the already saved copy.)

2. It probably makes sense to make a structure out of the saved state
   information so that assembly code doesn't have to individually find
   the address of each variable.  Instead, they can find the address of
   the structure (in physical memory if that's what they need) and use
   offsets.

With (2) its probably worth adding a comment about the structure being
used in platform code and it should only ever be appended to.
(Alternatively, we could use the asm-offsets.h generation stuff to
create preprocessor symbols for the offsets in the struct if we put the
struct in a header file.)

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Barry Song]

2011/9/24 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Mon, Sep 19, 2011 at 01:33:39PM +0800, Barry Song wrote:
>> Do you think the following is what you want?
>
> Almost. ?A couple of things:
>
> 1. Making the variables static means that folk like OMAP can't read the
> ? values at resume time from their assembly (forcing them to save and
> ? restore them, rather than using the already saved copy.)
>
> 2. It probably makes sense to make a structure out of the saved state
> ? information so that assembly code doesn't have to individually find
> ? the address of each variable. ?Instead, they can find the address of
> ? the structure (in physical memory if that's what they need) and use
> ? offsets.
>
> With (2) its probably worth adding a comment about the structure being
> used in platform code and it should only ever be appended to.
> (Alternatively, we could use the asm-offsets.h generation stuff to
> create preprocessor symbols for the offsets in the struct if we put the
> struct in a header file.)
>
 Then the incremental changes:

diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h
b/arch/arm/include/asm/hardware/cache-l2x0.h
index 733606b..99f2793 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 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/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 98d03fb..b44f333 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -32,8 +32,8 @@ static void __iomem *l2x0_base;
 static DEFINE_SPINLOCK(l2x0_lock);
 static uint32_t l2x0_way_mask;	/* Bitmask of active ways */
 static uint32_t l2x0_size;
-static u32 l2x0_aux_ctrl;
-static u32 l2x0_tag_latency, l2x0_data_latency, l2x0_filter_start,
l2x0_filter_end;
+
+struct l2x0_regs l2x0_saved_regs;

 struct l2x0_of_data {
 	void (*setup)(const struct device_node *, __u32 *, __u32 *);
@@ -364,7 +364,7 @@ void l2x0_init(void __iomem *base, __u32 aux_val,
__u32 aux_mask)
 		/* l2x0 controller is disabled */
 		writel_relaxed(aux, l2x0_base + L2X0_AUX_CTRL);

-		l2x0_aux_ctrl = aux;
+		l2x0_saved_regs.aux_ctrl = aux;

 		l2x0_inv_all();

@@ -457,10 +457,10 @@ static void pl310_of_setup(const struct device_node *np,

 static void __init pl310_save(void)
 {
-	l2x0_tag_latency = readl_relaxed(l2x0_base + L2X0_TAG_LATENCY_CTRL);
-	l2x0_data_latency = readl_relaxed(l2x0_base + L2X0_DATA_LATENCY_CTRL);
-	l2x0_filter_end = readl_relaxed(l2x0_base + L2X0_ADDR_FILTER_END);
-	l2x0_filter_start = readl_relaxed(l2x0_base + L2X0_ADDR_FILTER_START);
+	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)
@@ -469,7 +469,7 @@ static void l2x0_resume(void)
 		/* restore aux ctrl and enable l2 */
 		l2x0_unlock(readl_relaxed(l2x0_base + L2X0_CACHE_ID));

-		writel_relaxed(l2x0_aux_ctrl, l2x0_base + L2X0_AUX_CTRL);
+		writel_relaxed(l2x0_saved_regs.aux_ctrl, l2x0_base + L2X0_AUX_CTRL);

 		l2x0_inv_all();

@@ -481,10 +481,10 @@ static void pl310_resume(void)
 {
 	if (!(readl_relaxed(l2x0_base + L2X0_CTRL) & 1)) {
 		/* restore pl310 setup */
-		writel_relaxed(l2x0_tag_latency, l2x0_base + L2X0_TAG_LATENCY_CTRL);
-		writel_relaxed(l2x0_data_latency, l2x0_base + L2X0_DATA_LATENCY_CTRL);
-		writel_relaxed(l2x0_filter_end, l2x0_base + L2X0_ADDR_FILTER_END);
-		writel_relaxed(l2x0_filter_start, l2x0_base + L2X0_ADDR_FILTER_START);
+		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();

--

-barry

[PATCH v2 1/2] ARM: cache-l2x0: remove __init annotation from initialization functions

[Shawn Guo]

On Sat, Sep 17, 2011 at 11:45:18AM +0100, Russell King - ARM Linux wrote:
> I'm now starting to think that we don't actually want any resume code
> at the L2 level - most SoCs will be operating in non-secure mode (I
> believe it's only ARM's development platforms which operate in secure
> mode)

On imx6q, I can program PL310 without SMC call at resume entry, which
probably means imx6q is a SoC operating in secure mode.

Regards,
Shawn

> and so most of the generic code which will need to write to the
> L2 control registers on resume will fail.

[PATCH v2 2/2] ARM: smp_scu: remove __init annotation from scu_enable()

[Shawn Guo]

When Cortex-A9 MPCore resumes from Dormant or Shutdown modes,
SCU needs to be re-enabled.  This patch removes __init annotation
from function scu_enable(), so that platform resume procedure can
call it to re-enable SCU.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/kernel/smp_scu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 79ed5e7..5b6d536 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -33,7 +33,7 @@ unsigned int __init scu_get_core_count(void __iomem *scu_base)
 /*
  * Enable the SCU
  */
-void __init scu_enable(void __iomem *scu_base)
+void scu_enable(void __iomem *scu_base)
 {
 	u32 scu_ctrl;
 
-- 
1.7.4.1

[PATCH v2 2/2] ARM: smp_scu: remove __init annotation from scu_enable()

[Shawn Guo]

On Tue, Sep 06, 2011 at 01:48:27PM +0800, Shawn Guo wrote:
> When Cortex-A9 MPCore resumes from Dormant or Shutdown modes,
> SCU needs to be re-enabled.  This patch removes __init annotation
> from function scu_enable(), so that platform resume procedure can
> call it to re-enable SCU.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---

Hi Russell,

Do you have any comment on this patch?  Can I put it into patch tracker?

Regards,
Shawn

>  arch/arm/kernel/smp_scu.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 79ed5e7..5b6d536 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -33,7 +33,7 @@ unsigned int __init scu_get_core_count(void __iomem *scu_base)
>  /*
>   * Enable the SCU
>   */
> -void __init scu_enable(void __iomem *scu_base)
> +void scu_enable(void __iomem *scu_base)
>  {
>  	u32 scu_ctrl;
>  
> -- 
> 1.7.4.1

[PATCH v2 2/2] ARM: smp_scu: remove __init annotation from scu_enable()

[Shawn Guo]

On Sat, Sep 17, 2011 at 04:32:37PM +0800, Shawn Guo wrote:
> On Tue, Sep 06, 2011 at 01:48:27PM +0800, Shawn Guo wrote:
> > When Cortex-A9 MPCore resumes from Dormant or Shutdown modes,
> > SCU needs to be re-enabled.  This patch removes __init annotation
> > from function scu_enable(), so that platform resume procedure can
> > call it to re-enable SCU.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> 
> Hi Russell,
> 
> Do you have any comment on this patch?  Can I put it into patch tracker?
> 
Ping x 2.

As imx6q suspend support depends on this patch, I really need to know
if you will merge it.  Otherwise, I will probably have to copy the code
into imx6q platform code just like exynos4 is doing.

-- 
Regards,
Shawn

[PATCH v2 2/2] ARM: smp_scu: remove __init annotation from scu_enable()

[Russell King - ARM Linux]

On Thu, Sep 22, 2011 at 11:04:01PM +0800, Shawn Guo wrote:
> On Sat, Sep 17, 2011 at 04:32:37PM +0800, Shawn Guo wrote:
> > On Tue, Sep 06, 2011 at 01:48:27PM +0800, Shawn Guo wrote:
> > > When Cortex-A9 MPCore resumes from Dormant or Shutdown modes,
> > > SCU needs to be re-enabled.  This patch removes __init annotation
> > > from function scu_enable(), so that platform resume procedure can
> > > call it to re-enable SCU.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > ---
> > 
> > Hi Russell,
> > 
> > Do you have any comment on this patch?  Can I put it into patch tracker?
> > 
> Ping x 2.
> 
> As imx6q suspend support depends on this patch, I really need to know
> if you will merge it.  Otherwise, I will probably have to copy the code
> into imx6q platform code just like exynos4 is doing.

It's fine.

[PATCH v2 2/2] ARM: smp_scu: remove __init annotation from scu_enable()

[Shawn Guo]

On Fri, Sep 23, 2011 at 09:49:55PM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 22, 2011 at 11:04:01PM +0800, Shawn Guo wrote:
> > On Sat, Sep 17, 2011 at 04:32:37PM +0800, Shawn Guo wrote:
> > > On Tue, Sep 06, 2011 at 01:48:27PM +0800, Shawn Guo wrote:
> > > > When Cortex-A9 MPCore resumes from Dormant or Shutdown modes,
> > > > SCU needs to be re-enabled.  This patch removes __init annotation
> > > > from function scu_enable(), so that platform resume procedure can
> > > > call it to re-enable SCU.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > ---
> > > 
> > > Hi Russell,
> > > 
> > > Do you have any comment on this patch?  Can I put it into patch tracker?
> > > 
> > Ping x 2.
> > 
> > As imx6q suspend support depends on this patch, I really need to know
> > if you will merge it.  Otherwise, I will probably have to copy the code
> > into imx6q platform code just like exynos4 is doing.
> 
> It's fine.
> 
I'm unsure about this comment.  Does it mean that the patch looks fine
for being merged, or it's fine to duplicate the code in imx6q platform?

-- 
Regards,
Shawn

[PATCH v2 2/2] ARM: smp_scu: remove __init annotation from scu_enable()

[Russell King - ARM Linux]

On Sat, Sep 24, 2011 at 06:39:09PM +0800, Shawn Guo wrote:
> On Fri, Sep 23, 2011 at 09:49:55PM +0100, Russell King - ARM Linux wrote:
> > On Thu, Sep 22, 2011 at 11:04:01PM +0800, Shawn Guo wrote:
> > > On Sat, Sep 17, 2011 at 04:32:37PM +0800, Shawn Guo wrote:
> > > > On Tue, Sep 06, 2011 at 01:48:27PM +0800, Shawn Guo wrote:
> > > > > When Cortex-A9 MPCore resumes from Dormant or Shutdown modes,
> > > > > SCU needs to be re-enabled.  This patch removes __init annotation
> > > > > from function scu_enable(), so that platform resume procedure can
> > > > > call it to re-enable SCU.
> > > > > 
> > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > ---
> > > > 
> > > > Hi Russell,
> > > > 
> > > > Do you have any comment on this patch?  Can I put it into patch tracker?
> > > > 
> > > Ping x 2.
> > > 
> > > As imx6q suspend support depends on this patch, I really need to know
> > > if you will merge it.  Otherwise, I will probably have to copy the code
> > > into imx6q platform code just like exynos4 is doing.
> > 
> > It's fine.
> > 
> I'm unsure about this comment.  Does it mean that the patch looks fine
> for being merged, or it's fine to duplicate the code in imx6q platform?

The patch looks fine.

[PATCH v2 0/2] make reinitialization of ARM core components possible

[Santosh]

On Tuesday 06 September 2011 11:18 AM, Shawn Guo wrote:
> When ARM core resumes from low-power mode where losing power, for my
> example: CA-9 MPCore resumes from Dormant/Shutdown, we have to
> reinitialize components like L2 Cache, GIC and SCU to bring system
> back to work.
>
I think you mean to drop GIC from above as well.

> The patch set basically removes __init annotation from a bunch of
> initialization functions, so that platform resume procedure can call
> into them again to set those components up.
>
> Changes since v1:
>   * Drop the GIC patch, as it's been handled by [1] nicely.
>   * Drop the change of adding empty l2x0_of_init(), as it's been
>     handled by [2].
>
> Shawn Guo (2):
>        ARM: cache-l2x0: remove __init annotation from initialization functions
Do you really need to do a re-init function for L2.
Look at the OMAP code, You just need to restore the configuration 
registers and that's it. And the save can be done only once because
these are configuration register which won't change in the live system.

>        ARM: smp_scu: remove __init annotation from scu_enable()
>
This init is really small one so should be ok but I still feel a
simple register restore is better.

[PATCH v2 0/2] make reinitialization of ARM core components possible

[Santosh]

Shawn,

On Tuesday 06 September 2011 11:31 AM, Santosh wrote:
> On Tuesday 06 September 2011 11:18 AM, Shawn Guo wrote:
>> When ARM core resumes from low-power mode where losing power, for my
>> example: CA-9 MPCore resumes from Dormant/Shutdown, we have to
>> reinitialize components like L2 Cache, GIC and SCU to bring system
>> back to work.

[..]

>> Shawn Guo (2):
>> ARM: cache-l2x0: remove __init annotation from initialization functions
> Do you really need to do a re-init function for L2.
> Look at the OMAP code, You just need to restore the configuration
> registers and that's it. And the save can be done only once because
> these are configuration register which won't change in the live system.
>
BTW, if it works for rest of the architectures, then I don't have
objection on these patches.
These patches definitely avoid more code getting added. :)

Regards
Santosh

[PATCH v2 0/2] make reinitialization of ARM core components possible

[Shawn Guo]

On Tue, Sep 06, 2011 at 11:31:02AM +0530, Santosh wrote:
> On Tuesday 06 September 2011 11:18 AM, Shawn Guo wrote:
> >When ARM core resumes from low-power mode where losing power, for my
> >example: CA-9 MPCore resumes from Dormant/Shutdown, we have to
> >reinitialize components like L2 Cache, GIC and SCU to bring system
> >back to work.
> >
> I think you mean to drop GIC from above as well.
> 
> >The patch set basically removes __init annotation from a bunch of
> >initialization functions, so that platform resume procedure can call
> >into them again to set those components up.
> >
> >Changes since v1:
> >  * Drop the GIC patch, as it's been handled by [1] nicely.
> >  * Drop the change of adding empty l2x0_of_init(), as it's been
> >    handled by [2].
> >
> >Shawn Guo (2):
> >       ARM: cache-l2x0: remove __init annotation from initialization functions
> Do you really need to do a re-init function for L2.
> Look at the OMAP code, You just need to restore the configuration
> registers and that's it. And the save can be done only once because
> these are configuration register which won't change in the live system.
> 
OMAP and IMX6Q retains L2 cache, we have to go save/restore approach
to work with generic cpu_suspend/resume routines.  But for platforms
that will power off L2 during suspend, I think re-init is the easiest
way for them get L2 back to work.

> >       ARM: smp_scu: remove __init annotation from scu_enable()
> >
> This init is really small one so should be ok but I still feel a
> simple register restore is better.
> 
Obviously, some people would disagree that.  We can see that function
exynos4_scu_enable() found in arch/arm/mach-exynos4/pm.c is a direct
copy of scu_enable().  We can save code duplication in platforms by
simply remove the __init annotation from scu_enable().

-- 
Regards,
Shawn

[PATCH v2 0/2] make reinitialization of ARM core components possible

[Russell King - ARM Linux]

On Mon, Sep 12, 2011 at 01:41:41PM +0800, Shawn Guo wrote:
> OMAP and IMX6Q retains L2 cache, we have to go save/restore approach
> to work with generic cpu_suspend/resume routines.  But for platforms
> that will power off L2 during suspend, I think re-init is the easiest
> way for them get L2 back to work.

It would be much better to re-init with the same configuration values
used during the initial init - rather than using the mask + values
method which preserves whatever the boot loader gave us.

We know with quite a bit of experience that we can't rely on boot loaders
setting things up in the same way on initial boot and resume, so we should
not rely on the boot loader setting the same L2 configuration on resume
as was set during boot.

[PATCH v2 0/2] make reinitialization of ARM core components possible

[Shawn Guo]

On Wed, Sep 14, 2011 at 08:07:39PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 12, 2011 at 01:41:41PM +0800, Shawn Guo wrote:
> > OMAP and IMX6Q retains L2 cache, we have to go save/restore approach
> > to work with generic cpu_suspend/resume routines.  But for platforms
> > that will power off L2 during suspend, I think re-init is the easiest
> > way for them get L2 back to work.
> 
> It would be much better to re-init with the same configuration values
> used during the initial init - rather than using the mask + values
> method which preserves whatever the boot loader gave us.
> 
The re-init approach calls the exactly same function used for the
initial init during resume.  So if this is a problem, it's the problem
of initial init.  The initial init code should initialize the L2 into
a known state with "values" rather than "mask + values" method.

> We know with quite a bit of experience that we can't rely on boot loaders
> setting things up in the same way on initial boot and resume, so we should
> not rely on the boot loader setting the same L2 configuration on resume
> as was set during boot.
> 
Right, we should not reply on boot loader.  My point is we should not
reply on boot loader on the initial boot.  Then we can get the same
L2 configuration on resume as was set during initial boot.

-- 
Regards,
Shawn

[PATCH v2 0/2] make reinitialization of ARM core components possible

[Russell King - ARM Linux]

On Thu, Sep 15, 2011 at 09:53:39AM +0800, Shawn Guo wrote:
> The re-init approach calls the exactly same function used for the
> initial init during resume.  So if this is a problem, it's the problem
> of initial init.  The initial init code should initialize the L2 into
> a known state with "values" rather than "mask + values" method.

That may not be possible - the kernel may have no knowledge of the correct
settings for the number of ways and cacheline size etc for the platform.
It may be that these settings have been configured by the secure monitor
rather than the boot loader.

I couldn't tell you what the correct settings would be for Versatile
Express for instance, and I bet that the correct settings are a function
of the FPGA images loaded onto the board.