make reinitialization of ARM core components possible

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.

Quoted 4 lines · Shawn Guo

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?

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 <[email protected]>

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

arch/arm/include/asm/hardware/cache-l2x0.h +1 -1

index d22765c..d270310 100644
@@ -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

arch/arm/mm/cache-l2x0.c +0 -0

index c035b9a..7835cb6 100644

Quoted 32 lines · 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<[email protected]>

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

arch/arm/include/asm/hardware/cache-l2x0.h +1 -1

index d22765c..d270310 100644
@@ -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

arch/arm/mm/cache-l2x0.c +0 -0

index c035b9a..7835cb6 100644

arch/arm/mm/cache-l2x0.c static void l2x0_disable(void) · line 286

Quoted 8 lines

arch/arm/mm/cache-l2x0.c +1 -1

@@ -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.

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.

Quoted 32 lines

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 <[email protected]>

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

arch/arm/include/asm/hardware/cache-l2x0.h +0 -0

index d22765c..d270310 100644
@@ -89,7 +89,7 @@

?#define L2X0_ADDR_FILTER_EN ? ? ? ? ?1

?#ifndef __ASSEMBLY__

arch/arm/include/asm/hardware/cache-l2x0.h +1 -1

-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

arch/arm/mm/cache-l2x0.c +0 -0

index c035b9a..7835cb6 100644

arch/arm/mm/cache-l2x0.c static void l2x0_disable(void) · line 286

Quoted 8 lines

arch/arm/mm/cache-l2x0.c +1 -1

@@ -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.

Quoted 32 lines · 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 <[email protected]>

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

arch/arm/include/asm/hardware/cache-l2x0.h +1 -1

index d22765c..d270310 100644
@@ -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

arch/arm/mm/cache-l2x0.c +0 -0

index c035b9a..7835cb6 100644

arch/arm/mm/cache-l2x0.c static void l2x0_disable(void) · line 286

Quoted 8 lines

arch/arm/mm/cache-l2x0.c +1 -1

@@ -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.

Quoted 5 lines · Russell King - ARM Linux

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.

Quoted 4 lines

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?

Quoted 10 lines · Russell King - ARM Linux

Quoted 9 lines

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.

Quoted 3 lines

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.

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

Quoted 2 lines

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.

Quoted 3 lines

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.

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.

Quoted 3 lines

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:

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 38 lines

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 }

Quoted 3 lines

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

Quoted 3 lines

Quoted 3 lines

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.

Quoted 16 lines

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

Quoted 1 line

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

Correct.

Quoted 38 lines

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 }

Quoted 3 lines

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?

Quoted 1 line

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.)

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.

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 <[email protected]>

---

 arch/arm/kernel/smp_scu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/kernel/smp_scu.c +1 -1

index 79ed5e7..5b6d536 100644
@@ -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;
 

Signature · 1 line

1.7.4.1

Quoted 5 lines · 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.

Quoted 2 lines

Signed-off-by Shawn Guo <[email protected]>

---

Hi Russell,

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

Quoted 11 lines · Shawn Guo

Quoted 7 lines · 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 <[email protected]>

---

Hi Russell,

Quoted 2 lines

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.

Quoted 14 lines · Shawn Guo

Quoted 13 lines · Shawn Guo

Quoted 7 lines · 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 <[email protected]>

---

Hi Russell,

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

Quoted 5 lines

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.

Quoted 17 lines · Russell King - ARM Linux

Quoted 16 lines · Shawn Guo

Quoted 6 lines · Shawn Guo

Quoted 5 lines · 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.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Shawn Guo <[email protected]>

---

Hi Russell,

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

Ping x 2.

Quoted 6 lines

Quoted 3 lines

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?

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()

Quoted 4 lines · 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.

Quoted 2 lines

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.

Quoted 5 lines · 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.

I think you mean to drop GIC from above as well.

Quoted 9 lines

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].

Quoted 2 lines

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.

Quoted 2 lines

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.

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.

Quoted 4 lines · Shawn Guo

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.

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.