| Message ID | 1315288107-14689-2-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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.
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
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
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.
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.
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.
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
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.
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
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.
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.
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,
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(-)