| Message ID | 1307926019-28958-1-git-send-email-ccross@android.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 13, 2011 at 01:46:58AM +0100, Colin Cross wrote: > Remove __init annotation from l2x0_init so it can be used to > reinitialize the l2x0 after it has been reset during suspend. > > Only print the init messages the first time l2x0_init is called. > > Add l2x0_enable to re-enable the l2x0 after l2x0_disable if > the l2x0 was not reset. > > l2x0_disable cannot use writel, as writel calls wmb(), and wmb() > may call outer_cache_sync, which takes the same spinlock as > l2x0_disable. > > Signed-off-by: Colin Cross <ccross@android.com> > --- > arch/arm/include/asm/hardware/cache-l2x0.h | 3 ++- > arch/arm/mm/cache-l2x0.c | 18 ++++++++++++++---- > 2 files changed, 16 insertions(+), 5 deletions(-) > [...] > +/* enables l2x0 after l2x0_disable, does not invalidate */ > +void l2x0_enable(void) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&l2x0_lock, flags); > + writel_relaxed(1, l2x0_base + L2X0_CTRL); > + spin_unlock_irqrestore(&l2x0_lock, flags); > +} > + > static void l2x0_disable(void) > { > unsigned long flags; > > spin_lock_irqsave(&l2x0_lock, flags); > - writel(0, l2x0_base + L2X0_CTRL); > + writel_relaxed(0, l2x0_base + L2X0_CTRL); > spin_unlock_irqrestore(&l2x0_lock, flags); > } This function is still dodgy to use, since we are disabling L2 possibly with current function stack lines allocated, which might wreak havoc. > > -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; > @@ -340,7 +350,7 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask) > outer_cache.disable = l2x0_disable; > outer_cache.set_debug = l2x0_set_debug; > > - printk(KERN_INFO "%s cache controller enabled\n", type); > - printk(KERN_INFO "l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n", > + pr_info_once("%s cache controller enabled\n", type); > + pr_info_once("l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n", > ways, cache_id, aux, l2x0_size); > } We might still need hooks to save/restore PL310 context since registers like tag and data RAM control are gone on reset. Furthermore, on L2 RAM retention we want to reinit L2 without having to invalidate L2 RAM(s), which means we cannot use l2x0_init as it is. I added a couple of hooks to outer_cache in order to carry out this task, patch coming. Lorenzo
On Mon, Jun 13, 2011 at 3:19 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Mon, Jun 13, 2011 at 01:46:58AM +0100, Colin Cross wrote: >> Remove __init annotation from l2x0_init so it can be used to >> reinitialize the l2x0 after it has been reset during suspend. >> >> Only print the init messages the first time l2x0_init is called. >> >> Add l2x0_enable to re-enable the l2x0 after l2x0_disable if >> the l2x0 was not reset. >> >> l2x0_disable cannot use writel, as writel calls wmb(), and wmb() >> may call outer_cache_sync, which takes the same spinlock as >> l2x0_disable. >> >> Signed-off-by: Colin Cross <ccross@android.com> >> --- >> arch/arm/include/asm/hardware/cache-l2x0.h | 3 ++- >> arch/arm/mm/cache-l2x0.c | 18 ++++++++++++++---- >> 2 files changed, 16 insertions(+), 5 deletions(-) >> > > [...] > >> +/* enables l2x0 after l2x0_disable, does not invalidate */ >> +void l2x0_enable(void) >> +{ >> + unsigned long flags; >> + >> + spin_lock_irqsave(&l2x0_lock, flags); >> + writel_relaxed(1, l2x0_base + L2X0_CTRL); >> + spin_unlock_irqrestore(&l2x0_lock, flags); >> +} >> + >> static void l2x0_disable(void) >> { >> unsigned long flags; >> >> spin_lock_irqsave(&l2x0_lock, flags); >> - writel(0, l2x0_base + L2X0_CTRL); >> + writel_relaxed(0, l2x0_base + L2X0_CTRL); >> spin_unlock_irqrestore(&l2x0_lock, flags); >> } > > This function is still dodgy to use, since we are disabling L2 > possibly with current function stack lines allocated, which might wreak havoc. From my JTAG debugging, I think the PL310 (at least the one I'm working with) flushes on disable. I had a problem where reads with the cache disabled were getting incorrect data because the data was stuck in the cache. With the CPU paused, I disabled the PL310 with the L2X0_CTRL register, and my data appeared in memory. I'll try to find some documentation to support this. Otherwise, wouldn't a flush_cache_all() and l2x0_flush_all() in l2x0_disable() ensure the stack lines were flushed out? Probably needs a readl_releaxed as well, to ensure the disable has taken effect. >> >> -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; >> @@ -340,7 +350,7 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask) >> outer_cache.disable = l2x0_disable; >> outer_cache.set_debug = l2x0_set_debug; >> >> - printk(KERN_INFO "%s cache controller enabled\n", type); >> - printk(KERN_INFO "l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n", >> + pr_info_once("%s cache controller enabled\n", type); >> + pr_info_once("l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n", >> ways, cache_id, aux, l2x0_size); >> } > > We might still need hooks to save/restore PL310 context since registers > like tag and data RAM control are gone on reset. Furthermore, on L2 RAM > retention we want to reinit L2 without having to invalidate L2 RAM(s), > which means we cannot use l2x0_init as it is. I added a couple of hooks to > outer_cache in order to carry out this task, patch coming. I'm assuming that every platform has a function to set up the tag ram control the way they want them, which then calls l2x0_init. That platform cache init function can be called instead of directly calling l2x0_init from the resume path. Calling l2x0_enable instead of l2x0_init solves the retention problem. > Lorenzo > >
On Mon, Jun 13, 2011 at 07:20:34PM +0100, Colin Cross wrote: > On Mon, Jun 13, 2011 at 3:19 AM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > On Mon, Jun 13, 2011 at 01:46:58AM +0100, Colin Cross wrote: > >> Remove __init annotation from l2x0_init so it can be used to > >> reinitialize the l2x0 after it has been reset during suspend. > >> > >> Only print the init messages the first time l2x0_init is called. > >> > >> Add l2x0_enable to re-enable the l2x0 after l2x0_disable if > >> the l2x0 was not reset. > >> > >> l2x0_disable cannot use writel, as writel calls wmb(), and wmb() > >> may call outer_cache_sync, which takes the same spinlock as > >> l2x0_disable. > >> > >> Signed-off-by: Colin Cross <ccross@android.com> > >> --- > >> ?arch/arm/include/asm/hardware/cache-l2x0.h | ? ?3 ++- > >> ?arch/arm/mm/cache-l2x0.c ? ? ? ? ? ? ? ? ? | ? 18 ++++++++++++++---- > >> ?2 files changed, 16 insertions(+), 5 deletions(-) > >> > > > > [...] > > > >> +/* enables l2x0 after l2x0_disable, does not invalidate */ > >> +void l2x0_enable(void) > >> +{ > >> + ? ? unsigned long flags; > >> + > >> + ? ? spin_lock_irqsave(&l2x0_lock, flags); > >> + ? ? writel_relaxed(1, l2x0_base + L2X0_CTRL); > >> + ? ? spin_unlock_irqrestore(&l2x0_lock, flags); > >> +} > >> + > >> ?static void l2x0_disable(void) > >> ?{ > >> ? ? ? unsigned long flags; > >> > >> ? ? ? spin_lock_irqsave(&l2x0_lock, flags); > >> - ? ? writel(0, l2x0_base + L2X0_CTRL); > >> + ? ? writel_relaxed(0, l2x0_base + L2X0_CTRL); > >> ? ? ? spin_unlock_irqrestore(&l2x0_lock, flags); > >> ?} > > > > This function is still dodgy to use, since we are disabling L2 > > possibly with current function stack lines allocated, which might wreak havoc. > > From my JTAG debugging, I think the PL310 (at least the one I'm > working with) flushes on disable. I had a problem where reads with > the cache disabled were getting incorrect data because the data was > stuck in the cache. With the CPU paused, I disabled the PL310 with > the L2X0_CTRL register, and my data appeared in memory. I'll try to > find some documentation to support this. > According to the PL310 TRM (r3p2 3-9) on disable the PL310 syncs, which means it flushes internal buffers, but it is not supposed to clean dirty lines. It seems unsafe to me to use the stack and other cacheable data right before disabling. > Otherwise, wouldn't a flush_cache_all() and l2x0_flush_all() in > l2x0_disable() ensure the stack lines were flushed out? Probably > needs a readl_releaxed as well, to ensure the disable has taken > effect. > I reckon the safest option is to clean and disable in one C function. Moreover at that point in time spinlocks might not be an option anymore (CPU out of coherency), which makes those functions unusable. > >> > >> -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; > >> @@ -340,7 +350,7 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask) > >> ? ? ? outer_cache.disable = l2x0_disable; > >> ? ? ? outer_cache.set_debug = l2x0_set_debug; > >> > >> - ? ? printk(KERN_INFO "%s cache controller enabled\n", type); > >> - ? ? printk(KERN_INFO "l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n", > >> + ? ? pr_info_once("%s cache controller enabled\n", type); > >> + ? ? pr_info_once("l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n", > >> ? ? ? ? ? ? ? ? ? ? ? ways, cache_id, aux, l2x0_size); > >> ?} > > > > We might still need hooks to save/restore PL310 context since registers > > like tag and data RAM control are gone on reset. Furthermore, on L2 RAM > > retention we want to reinit L2 without having to invalidate L2 RAM(s), > > which means we cannot use l2x0_init as it is. I added a couple of hooks to > > outer_cache in order to carry out this task, patch coming. > > I'm assuming that every platform has a function to set up the tag ram > control the way they want them, which then calls l2x0_init. That > platform cache init function can be called instead of directly calling > l2x0_init from the resume path. I agree that it might solve the problem, and I understand that it allows to reuse some code, but this will force us to call into platform code to reinit L2 (which goes against the grain of platform notifiers); if we manage to write a function doing the whole thing in a single place it might be easier to use and factor out some replicated code in the process. Lorenzo
On 6/14/2011 11:00 PM, Lorenzo Pieralisi wrote: > On Mon, Jun 13, 2011 at 07:20:34PM +0100, Colin Cross wrote: >> On Mon, Jun 13, 2011 at 3:19 AM, Lorenzo Pieralisi >> <lorenzo.pieralisi@arm.com> wrote: >>> On Mon, Jun 13, 2011 at 01:46:58AM +0100, Colin Cross wrote: >>>> Remove __init annotation from l2x0_init so it can be used to >>>> reinitialize the l2x0 after it has been reset during suspend. >>>> >>>> Only print the init messages the first time l2x0_init is called. >>>> >>>> Add l2x0_enable to re-enable the l2x0 after l2x0_disable if >>>> the l2x0 was not reset. >>>> >>>> l2x0_disable cannot use writel, as writel calls wmb(), and wmb() >>>> may call outer_cache_sync, which takes the same spinlock as >>>> l2x0_disable. >>>> >>>> Signed-off-by: Colin Cross<ccross@android.com> >>>> --- >>>> ?arch/arm/include/asm/hardware/cache-l2x0.h | ? ?3 ++- >>>> ?arch/arm/mm/cache-l2x0.c ? ? ? ? ? ? ? ? ? | ? 18 ++++++++++++++---- >>>> ?2 files changed, 16 insertions(+), 5 deletions(-) >>>> >>> >>> [...] >>> >>>> +/* enables l2x0 after l2x0_disable, does not invalidate */ >>>> +void l2x0_enable(void) >>>> +{ >>>> + ? ? unsigned long flags; >>>> + >>>> + ? ? spin_lock_irqsave(&l2x0_lock, flags); >>>> + ? ? writel_relaxed(1, l2x0_base + L2X0_CTRL); >>>> + ? ? spin_unlock_irqrestore(&l2x0_lock, flags); >>>> +} >>>> + >>>> ?static void l2x0_disable(void) >>>> ?{ >>>> ? ? ? unsigned long flags; >>>> >>>> ? ? ? spin_lock_irqsave(&l2x0_lock, flags); >>>> - ? ? writel(0, l2x0_base + L2X0_CTRL); >>>> + ? ? writel_relaxed(0, l2x0_base + L2X0_CTRL); >>>> ? ? ? spin_unlock_irqrestore(&l2x0_lock, flags); >>>> ?} >>> >>> This function is still dodgy to use, since we are disabling L2 >>> possibly with current function stack lines allocated, which might wreak havoc. >> >> From my JTAG debugging, I think the PL310 (at least the one I'm >> working with) flushes on disable. I had a problem where reads with >> the cache disabled were getting incorrect data because the data was >> stuck in the cache. With the CPU paused, I disabled the PL310 with >> the L2X0_CTRL register, and my data appeared in memory. I'll try to >> find some documentation to support this. >> > > According to the PL310 TRM (r3p2 3-9) on disable the PL310 syncs, which means > it flushes internal buffers, but it is not supposed to clean dirty lines. > It seems unsafe to me to use the stack and other cacheable data right before > disabling. > That's right. Disabling L2 won't clean the cache lines. They need to be cleaned before entering low power state where you loose it. Though, cleaning of L2 cache would work even with L2 enable is not set.
Hi guys, On Tue, Jun 14, 2011 at 06:30:19PM +0100, Lorenzo Pieralisi wrote: > On Mon, Jun 13, 2011 at 07:20:34PM +0100, Colin Cross wrote: > > According to the PL310 TRM (r3p2 3-9) on disable the PL310 syncs, which means > it flushes internal buffers, but it is not supposed to clean dirty lines. > It seems unsafe to me to use the stack and other cacheable data right before > disabling. > > > Otherwise, wouldn't a flush_cache_all() and l2x0_flush_all() in > > l2x0_disable() ensure the stack lines were flushed out? Probably > > needs a readl_releaxed as well, to ensure the disable has taken > > effect. > > > > I reckon the safest option is to clean and disable in one C > function. Moreover at that point in time spinlocks might not be an option > anymore (CPU out of coherency), which makes those functions unusable. I was planning to add this functionality as part of my kexec patches since otherwise the l2x0_lock gets blown away on disable. I initially attributed this to loss of coherency, but that's clearly not the case as I still had L1 running. Colin - I'll add you to CC for v3 of that series. Will
2011/6/13 Colin Cross <ccross@android.com>: > Remove __init annotation from l2x0_init so it can be used to > reinitialize the l2x0 after it has been reset during suspend. __init before l2x0_init can be deleted and make it available after resume. We just tried the following flow for l2 cache on CSR SiRFprimaII, it seems to be working: suspend: sync and invalidate l2 cache, then disable L2 v7_flush_kern_cache_all #ifdef CONFIG_CACHE_L2X0 ldr r0, =SIRFSOC_L2CC_VA_BASE ldr r1, =L2X0_CLEAN_INV_WAY mov r2, #0xff str r2, [r0,r1] mov r2, #0 1: ldr r3, [r0,r1] cmp r2,r3 bne 1b ldr r1, =L2X0_CACHE_SYNC mov r2, #0 str r2, [r0,r1] ldr r1, =L2X0_CTRL mov r2, #0 str r2, [r0,r1] #endif resume: re-initilized l2x: l2x0_init((void __iomem *)SIRFSOC_L2CC_VA_BASE, 0x00040000, 0x00000000); > > Only print the init messages the first time l2x0_init is called. > > Add l2x0_enable to re-enable the l2x0 after l2x0_disable if > the l2x0 was not reset. > > l2x0_disable cannot use writel, as writel calls wmb(), and wmb() > may call outer_cache_sync, which takes the same spinlock as > l2x0_disable. > > Signed-off-by: Colin Cross <ccross@android.com> > --- > arch/arm/include/asm/hardware/cache-l2x0.h | 3 ++- > arch/arm/mm/cache-l2x0.c | 18 ++++++++++++++---- > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h > index 16bd480..8fd87fe 100644 > --- a/arch/arm/include/asm/hardware/cache-l2x0.h > +++ b/arch/arm/include/asm/hardware/cache-l2x0.h > @@ -73,7 +73,8 @@ > #define L2X0_AUX_CTRL_EARLY_BRESP_SHIFT 30 > > #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); > +extern void l2x0_enable(void); > #endif > > #endif > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c > index ef59099..4db0d9c 100644 > --- a/arch/arm/mm/cache-l2x0.c > +++ b/arch/arm/mm/cache-l2x0.c > @@ -261,16 +261,26 @@ static void l2x0_flush_range(unsigned long start, unsigned long end) > spin_unlock_irqrestore(&l2x0_lock, flags); > } > > +/* enables l2x0 after l2x0_disable, does not invalidate */ > +void l2x0_enable(void) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&l2x0_lock, flags); > + writel_relaxed(1, l2x0_base + L2X0_CTRL); > + spin_unlock_irqrestore(&l2x0_lock, flags); > +} > + > static void l2x0_disable(void) > { > unsigned long flags; > > spin_lock_irqsave(&l2x0_lock, flags); > - writel(0, l2x0_base + L2X0_CTRL); > + writel_relaxed(0, l2x0_base + L2X0_CTRL); > spin_unlock_irqrestore(&l2x0_lock, flags); > } > can you show your context calling l2x0_disable and l2x0_enable? -barry
diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h index 16bd480..8fd87fe 100644 --- a/arch/arm/include/asm/hardware/cache-l2x0.h +++ b/arch/arm/include/asm/hardware/cache-l2x0.h @@ -73,7 +73,8 @@ #define L2X0_AUX_CTRL_EARLY_BRESP_SHIFT 30 #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); +extern void l2x0_enable(void); #endif #endif diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c index ef59099..4db0d9c 100644 --- a/arch/arm/mm/cache-l2x0.c +++ b/arch/arm/mm/cache-l2x0.c @@ -261,16 +261,26 @@ static void l2x0_flush_range(unsigned long start, unsigned long end) spin_unlock_irqrestore(&l2x0_lock, flags); } +/* enables l2x0 after l2x0_disable, does not invalidate */ +void l2x0_enable(void) +{ + unsigned long flags; + + spin_lock_irqsave(&l2x0_lock, flags); + writel_relaxed(1, l2x0_base + L2X0_CTRL); + spin_unlock_irqrestore(&l2x0_lock, flags); +} + static void l2x0_disable(void) { unsigned long flags; spin_lock_irqsave(&l2x0_lock, flags); - writel(0, l2x0_base + L2X0_CTRL); + writel_relaxed(0, l2x0_base + L2X0_CTRL); 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; @@ -340,7 +350,7 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask) outer_cache.disable = l2x0_disable; outer_cache.set_debug = l2x0_set_debug; - printk(KERN_INFO "%s cache controller enabled\n", type); - printk(KERN_INFO "l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n", + pr_info_once("%s cache controller enabled\n", type); + pr_info_once("l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n", ways, cache_id, aux, l2x0_size); }
Remove __init annotation from l2x0_init so it can be used to reinitialize the l2x0 after it has been reset during suspend. Only print the init messages the first time l2x0_init is called. Add l2x0_enable to re-enable the l2x0 after l2x0_disable if the l2x0 was not reset. l2x0_disable cannot use writel, as writel calls wmb(), and wmb() may call outer_cache_sync, which takes the same spinlock as l2x0_disable. Signed-off-by: Colin Cross <ccross@android.com> --- arch/arm/include/asm/hardware/cache-l2x0.h | 3 ++- arch/arm/mm/cache-l2x0.c | 18 ++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-)