[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Daniel Walker]

From: Willie Ruan <wruan@quicinc.com>

The application processor in Qualcomm's MSM chips can be powered off
(also called power collapse) during suspend and idle time. L2 cache power
will be collapsed too on current chips. To handle this scenario, we need
to add a function (l2x0_suspend here) to flush L2 cache before the power
loss and another function (l2x0_resume) to restore the cache settings
after the power is restored.

To minimize performance impact these two functions should be called as
close as possible to the point where power is collapsed and restored.
Since it is possible that the power could not be turned off, e.g. due to
a pending interrupt, the parameter 'collapsed' should be used for the
l2x0_resume function to indicate the actual power collapse state.

Signed-off-by: Willie Ruan <wruan@quicinc.com>
Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 arch/arm/include/asm/hardware/cache-l2x0.h |    3 ++
 arch/arm/mm/cache-l2x0.c                   |   46 ++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
index cdb9022..a86b948 100644
--- a/arch/arm/include/asm/hardware/cache-l2x0.h
+++ b/arch/arm/include/asm/hardware/cache-l2x0.h
@@ -55,4 +55,7 @@
 extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
 #endif
 
+extern void l2x0_suspend(void);
+extern void l2x0_resume(int collapsed);
+
 #endif
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index cb8fc65..c746878 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -2,6 +2,7 @@
  * arch/arm/mm/cache-l2x0.c - L210/L220 cache controller support
  *
  * Copyright (C) 2007 ARM Limited
+ * Copyright (c) 2009, Code Aurora Forum. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -26,6 +27,7 @@
 #define CACHE_LINE_SIZE		32
 
 static void __iomem *l2x0_base;
+static uint32_t aux_ctrl_save;
 static DEFINE_SPINLOCK(l2x0_lock);
 
 static inline void cache_wait(void __iomem *reg, unsigned long mask)
@@ -54,6 +56,13 @@ static inline void l2x0_inv_all(void)
 	spin_unlock_irqrestore(&l2x0_lock, flags);
 }
 
+static inline void l2x0_flush_all(void)
+{
+	/* clean and invalidate all ways */
+	sync_writel(0xff, L2X0_CLEAN_INV_WAY, 0xff);
+	cache_sync();
+}
+
 static void l2x0_inv_range(unsigned long start, unsigned long end)
 {
 	void __iomem *base = l2x0_base;
@@ -176,3 +185,40 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
 
 	printk(KERN_INFO "L2X0 cache controller enabled\n");
 }
+
+/*
+ * l2x0_suspend - Suspend L2 cache
+ *
+ * Call this function when L2 cache power will be collapsed (power-off):
+ * 	1) during suspend;
+ *	2) during idle.
+ */
+void l2x0_suspend(void)
+{
+	/* Save aux control register value */
+	aux_ctrl_save = readl(l2x0_base + L2X0_AUX_CTRL);
+	/* Flush all cache */
+	l2x0_flush_all();
+	/* Disable the cache */
+	writel(0, l2x0_base + L2X0_CTRL);
+}
+
+/*
+ * l2x0_resume - Resume L2 cache
+ * @collapsed:  1 -- power was collapsed
+ *              0 -- power was not collapsed
+ *
+ * Call this function with the 'collapsed' paramater to tell
+ * whether the power was really collapsed. Cache is still valid
+ * if there was no power loss, thus performance is maintained
+ * because of no need to invalidate the cache.
+ */
+void l2x0_resume(int collapsed)
+{
+	if (collapsed)
+		/* Restore aux control register value */
+		writel(aux_ctrl_save, l2x0_base + L2X0_AUX_CTRL);
+
+	/* Enable the cache */
+	writel(1, l2x0_base + L2X0_CTRL);
+}
-- 
1.6.3.3

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Pavel Machek]

On Thu 2010-01-28 14:59:29, Daniel Walker wrote:
> From: Willie Ruan <wruan@quicinc.com>
> 
> The application processor in Qualcomm's MSM chips can be powered off
> (also called power collapse) during suspend and idle time. L2 cache power
> will be collapsed too on current chips. To handle this scenario, we need
> to add a function (l2x0_suspend here) to flush L2 cache before the power
> loss and another function (l2x0_resume) to restore the cache settings
> after the power is restored.

So this fixes suspend/resume? Ack...

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Ruan, Willie]

> From: Pavel Machek [mailto:pavel at ucw.cz] 
> Sent: Thursday, January 28, 2010 10:49 PM
>
> So this fixes suspend/resume?

Yes

~Willie

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resumefunctions

[Catalin Marinas]

On Thu, 2010-01-28 at 22:59 +0000, Daniel Walker wrote:
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index cb8fc65..c746878 100644
[...]
> +static inline void l2x0_flush_all(void)
> +{
> +       /* clean and invalidate all ways */
> +       sync_writel(0xff, L2X0_CLEAN_INV_WAY, 0xff);
> +       cache_sync();
> +}

Do we need any for of locking around this code (as we do for
l2x0_inv_all) or races are not possible during suspend?

-- 
Catalin

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Ruan, Willie]

> From: Catalin Marinas [mailto:catalin.marinas at arm.com] 
> Sent: Friday, January 29, 2010 3:09 AM
>
> Do we need any for of locking around this code (as we do for
> l2x0_inv_all) or races are not possible during suspend?

Local interrupts have been disabled in suspend_ops->enter and
in arch_idle. Thus, no other locking is necessary.

~Willie

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Daniel Walker]

On Fri, 2010-01-29 at 09:55 -0800, Ruan, Willie wrote:
> > From: Catalin Marinas [mailto:catalin.marinas at arm.com] 
> > Sent: Friday, January 29, 2010 3:09 AM
> >
> > Do we need any for of locking around this code (as we do for
> > l2x0_inv_all) or races are not possible during suspend?
> 
> Local interrupts have been disabled in suspend_ops->enter and
> in arch_idle. Thus, no other locking is necessary.

What if there are multiple cpu's calling cache_sync() at the same time?
Disable interrupt wouldn't prevent it ..

Daniel

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Ruan, Willie]

> From: Daniel Walker [mailto:dwalker at codeaurora.org] 
> Sent: Friday, January 29, 2010 9:59 AM
>
> What if there are multiple cpu's calling cache_sync() at the same time?
> Disable interrupt wouldn't prevent it ..

cache_sync() is calling sync_writel(), which is using spin_lock_irqsave().
So, each call of cache_sync() and sync_writel() is SMP safe individually
in l2x0_flush_all() as in l2x0_inv_all(), unless we need to protect the
two calls together, which seems not necessary to me.

Thanks,
~Willie

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Daniel Walker]

On Fri, 2010-01-29 at 10:23 -0800, Ruan, Willie wrote:
> > From: Daniel Walker [mailto:dwalker at codeaurora.org] 
> > Sent: Friday, January 29, 2010 9:59 AM
> >
> > What if there are multiple cpu's calling cache_sync() at the same time?
> > Disable interrupt wouldn't prevent it ..
> 
> cache_sync() is calling sync_writel(), which is using spin_lock_irqsave().
> So, each call of cache_sync() and sync_writel() is SMP safe individually
> in l2x0_flush_all() as in l2x0_inv_all(), unless we need to protect the
> two calls together, which seems not necessary to me.

This is the current version,

static inline void cache_wait(void __iomem *reg, unsigned long mask)
{
        /* wait for the operation to complete */
        while (readl(reg) & mask)
                ;
}

static inline void cache_sync(void)
{
        void __iomem *base = l2x0_base;
        writel(0, base + L2X0_CACHE_SYNC);
        cache_wait(base + L2X0_CACHE_SYNC, 1);
}

Maybe cache_sync was recently changed to "writel" instead sync_writel()
due it getting called with the lock already held.

Daniel

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Russell King - ARM Linux]

On Fri, Jan 29, 2010 at 10:29:32AM -0800, Daniel Walker wrote:
> On Fri, 2010-01-29 at 10:23 -0800, Ruan, Willie wrote:
> > > From: Daniel Walker [mailto:dwalker at codeaurora.org] 
> > > Sent: Friday, January 29, 2010 9:59 AM
> > >
> > > What if there are multiple cpu's calling cache_sync() at the same time?
> > > Disable interrupt wouldn't prevent it ..
> > 
> > cache_sync() is calling sync_writel(), which is using spin_lock_irqsave().
> > So, each call of cache_sync() and sync_writel() is SMP safe individually
> > in l2x0_flush_all() as in l2x0_inv_all(), unless we need to protect the
> > two calls together, which seems not necessary to me.
> 
> This is the current version,
> 
> static inline void cache_wait(void __iomem *reg, unsigned long mask)
> {
>         /* wait for the operation to complete */
>         while (readl(reg) & mask)
>                 ;
> }
> 
> static inline void cache_sync(void)
> {
>         void __iomem *base = l2x0_base;
>         writel(0, base + L2X0_CACHE_SYNC);
>         cache_wait(base + L2X0_CACHE_SYNC, 1);
> }
> 
> Maybe cache_sync was recently changed to "writel" instead sync_writel()
> due it getting called with the lock already held.

It's pointless discussing what the current code does, because that's
not what mainline does _today_ - the locking in this file has
completely changed.

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Daniel Walker]

On Fri, 2010-01-29 at 19:03 +0000, Russell King - ARM Linux wrote:

> > 
> > Maybe cache_sync was recently changed to "writel" instead sync_writel()
> > due it getting called with the lock already held.
> 
> It's pointless discussing what the current code does, because that's
> not what mainline does _today_ - the locking in this file has
> completely changed.

Where is the code we should be looking at (what I quoted is from -next)?
Or is it in -next and we just need to look closer ? 

Daniel

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Russell King - ARM Linux]

On Fri, Jan 29, 2010 at 11:12:18AM -0800, Daniel Walker wrote:
> On Fri, 2010-01-29 at 19:03 +0000, Russell King - ARM Linux wrote:
> 
> > > 
> > > Maybe cache_sync was recently changed to "writel" instead sync_writel()
> > > due it getting called with the lock already held.
> > 
> > It's pointless discussing what the current code does, because that's
> > not what mainline does _today_ - the locking in this file has
> > completely changed.
> 
> Where is the code we should be looking at (what I quoted is from -next)?

Umm, I think whatever you're looking at is outdated by at least one
month.  What you're trying to tell me is in -next hasn't been there
for at least an entire month:

http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/sfr/linux-next.git;a=blob;f=arch/arm/mm/cache-l2x0.c;h=cb8fc6573b1b2c9dedbeeec0f9a87c491a78ba85;hb=HEAD

This is the same as what was merged into Linus' tree for 2.6.33-rc1
on 17 Dec, and was in -next well before that date.

If your tree has sync_writel() then it's out of date.

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Daniel Walker]

On Fri, 2010-01-29 at 19:43 +0000, Russell King - ARM Linux wrote:

> Umm, I think whatever you're looking at is outdated by at least one
> month.  What you're trying to tell me is in -next hasn't been there
> for at least an entire month:
> 
> http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/sfr/linux-next.git;a=blob;f=arch/arm/mm/cache-l2x0.c;h=cb8fc6573b1b2c9dedbeeec0f9a87c491a78ba85;hb=HEAD
> 
> This is the same as what was merged into Linus' tree for 2.6.33-rc1
> on 17 Dec, and was in -next well before that date.
> 
> If your tree has sync_writel() then it's out of date.

I was quoting code from next cause I was porting this patch to next (due
to your prior comments). The version of next that I have doesn't have
sync_writel() (it's next as of yesterday) ..

I'm updating the locking in this patches also.

Daniel

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Russell King - ARM Linux]

> +static inline void l2x0_flush_all(void)
> +{
> +	/* clean and invalidate all ways */
> +	sync_writel(0xff, L2X0_CLEAN_INV_WAY, 0xff);
> +	cache_sync();
> +}

Note that this patch is out of date; it won't apply to current kernels
and build.

[RFC PATCH 04/12] arm: mm: cache-l2x0: add l2x0 suspend and resume functions

[Daniel Walker]

On Fri, 2010-01-29 at 14:14 +0000, Russell King - ARM Linux wrote:
> > +static inline void l2x0_flush_all(void)
> > +{
> > +	/* clean and invalidate all ways */
> > +	sync_writel(0xff, L2X0_CLEAN_INV_WAY, 0xff);
> > +	cache_sync();
> > +}
> 
> Note that this patch is out of date; it won't apply to current kernels
> and build.

Do you generally prefer patches against current -rc kernels, or against
-next (or you development tree)?

Daniel