[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[srinidhi kasagar]

The effect of cache sync operation is to drain the store
buffer and wait for all internal buffers to be empty. In
normal conditions, store buffer is able to merge the
normal memory writes within its 32-byte data buffers.
Due to this erratum present in r3p0, the effect of cache
sync operation on the store buffer still remains when
the operation completes. This means that the store buffer
is always asked to drain and this prevents it from merging
any further writes.

This can severely affect performance on the write traffic
esp. on Normal memory NC one.

The proposed workaround is to replace the normal offset of
cache sync operation(0x730) by another offset targeting an
unmapped PL310 register 0x740.

Signed-off-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
---
 arch/arm/Kconfig                           |   15 +++++++++++++++
 arch/arm/include/asm/hardware/cache-l2x0.h |    1 +
 arch/arm/mm/cache-l2x0.c                   |    6 ++++++
 3 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d5eb308..0b67a9f 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1186,6 +1186,21 @@ config ARM_ERRATA_743622
 	  visible impact on the overall performance or power consumption of the
 	  processor.
 
+config ARM_ERRATA_753970
+	bool "ARM errata: cache sync operation may be faulty"
+	depends on CACHE_PL310
+	help
+	  This option enables the workaround for the 753970 PL310 (r3p0) erratum.
+
+	  Under some condition the effect of cache sync operation on
+	  the store buffer still remains when the operation completes.
+	  This means that the store buffer is always asked to drain and
+	  this prevents it from merging any further writes. The workaround
+	  is to replace the normal offset of cache sync operation (0x730)
+	  by another offset targeting an unmapped PL310 register 0x740.
+	  This has the same effect as the cache sync operation: store buffer
+	  drain and waiting for all buffers empty.
+
 endmenu
 
 source "arch/arm/common/Kconfig"
diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
index 5aeec1e..16bd480 100644
--- a/arch/arm/include/asm/hardware/cache-l2x0.h
+++ b/arch/arm/include/asm/hardware/cache-l2x0.h
@@ -36,6 +36,7 @@
 #define L2X0_RAW_INTR_STAT		0x21C
 #define L2X0_INTR_CLEAR			0x220
 #define L2X0_CACHE_SYNC			0x730
+#define L2X0_DUMMY_REG			0x740
 #define L2X0_INV_LINE_PA		0x770
 #define L2X0_INV_WAY			0x77C
 #define L2X0_CLEAN_LINE_PA		0x7B0
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 170c9bb..8cabe09 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -49,8 +49,14 @@ static inline void cache_wait(void __iomem *reg, unsigned long mask)
 static inline void cache_sync(void)
 {
 	void __iomem *base = l2x0_base;
+
+#ifdef CONFIG_ARM_ERRATA_753970
+	/* write to an unmmapped register */
+	writel_relaxed(0, base + L2X0_DUMMY_REG);
+#else
 	writel_relaxed(0, base + L2X0_CACHE_SYNC);
 	cache_wait(base + L2X0_CACHE_SYNC, 1);
+#endif
 }
 
 static inline void l2x0_clean_line(unsigned long addr)
-- 
1.7.0.4

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[srinidhi kasagar]

The effect of cache sync operation is to drain the store
buffer and wait for all internal buffers to be empty. In
normal conditions, store buffer is able to merge the
normal memory writes within its 32-byte data buffers.
Due to this erratum present in r3p0, the effect of cache
sync operation on the store buffer still remains when
the operation completes. This means that the store buffer
is always asked to drain and this prevents it from merging
any further writes.

This can severely affect performance on the write traffic
esp. on Normal memory NC one.

The proposed workaround is to replace the normal offset of
cache sync operation(0x730) by another offset targeting an
unmapped PL310 register 0x740.

Signed-off-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
---
 arch/arm/Kconfig                           |   15 +++++++++++++++
 arch/arm/include/asm/hardware/cache-l2x0.h |    1 +
 arch/arm/mm/cache-l2x0.c                   |    6 ++++++
 3 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d5eb308..0b67a9f 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1186,6 +1186,21 @@ config ARM_ERRATA_743622
 	  visible impact on the overall performance or power consumption of the
 	  processor.
 
+config ARM_ERRATA_753970
+	bool "ARM errata: cache sync operation may be faulty"
+	depends on CACHE_PL310
+	help
+	  This option enables the workaround for the 753970 PL310 (r3p0) erratum.
+
+	  Under some condition the effect of cache sync operation on
+	  the store buffer still remains when the operation completes.
+	  This means that the store buffer is always asked to drain and
+	  this prevents it from merging any further writes. The workaround
+	  is to replace the normal offset of cache sync operation (0x730)
+	  by another offset targeting an unmapped PL310 register 0x740.
+	  This has the same effect as the cache sync operation: store buffer
+	  drain and waiting for all buffers empty.
+
 endmenu
 
 source "arch/arm/common/Kconfig"
diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
index 5aeec1e..16bd480 100644
--- a/arch/arm/include/asm/hardware/cache-l2x0.h
+++ b/arch/arm/include/asm/hardware/cache-l2x0.h
@@ -36,6 +36,7 @@
 #define L2X0_RAW_INTR_STAT		0x21C
 #define L2X0_INTR_CLEAR			0x220
 #define L2X0_CACHE_SYNC			0x730
+#define L2X0_DUMMY_REG			0x740
 #define L2X0_INV_LINE_PA		0x770
 #define L2X0_INV_WAY			0x77C
 #define L2X0_CLEAN_LINE_PA		0x7B0
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 170c9bb..f2ce38e 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -49,7 +49,13 @@ static inline void cache_wait(void __iomem *reg, unsigned long mask)
 static inline void cache_sync(void)
 {
 	void __iomem *base = l2x0_base;
+
+#ifdef CONFIG_ARM_ERRATA_753970
+	/* write to an unmmapped register */
+	writel_relaxed(0, base + L2X0_DUMMY_REG);
+#else
 	writel_relaxed(0, base + L2X0_CACHE_SYNC);
+#endif
 	cache_wait(base + L2X0_CACHE_SYNC, 1);
 }
 
-- 
1.7.0.4

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[srinidhi kasagar]

The effect of cache sync operation is to drain the store
buffer and wait for all internal buffers to be empty. In
normal conditions, store buffer is able to merge the
normal memory writes within its 32-byte data buffers.
Due to this erratum present in r3p0, the effect of cache
sync operation on the store buffer still remains when
the operation completes. This means that the store buffer
is always asked to drain and this prevents it from merging
any further writes.

This can severely affect performance on the write traffic
esp. on Normal memory NC one.

The proposed workaround is to replace the normal offset of
cache sync operation(0x730) by another offset targeting an
unmapped PL310 register 0x740.

Signed-off-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
---
 arch/arm/Kconfig         |   15 +++++++++++++++
 arch/arm/mm/cache-l2x0.c |    8 ++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d5eb308..f1946e4 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1186,6 +1186,21 @@ config ARM_ERRATA_743622
 	  visible impact on the overall performance or power consumption of the
 	  processor.
 
+config ARM_ERRATA_753970
+	bool "ARM errata: cache sync operation may be faulty"
+	depends on CACHE_PL310
+	help
+	  This option enables the workaround for the 753970 PL310 erratum.
+
+	  Under some condition the effect of cache sync operation on
+	  the store buffer still remains when the operation completes.
+	  This means that the store buffer is always asked to drain and
+	  this prevents it from merging any further writes. The workaround
+	  is to replace the normal offset of cache sync operation (0x730)
+	  by another offset targeting an unmapped PL310 register 0x740.
+	  This has the same effect as the cache sync operation: store buffer
+	  drain and waiting for all buffers empty.
+
 endmenu
 
 source "arch/arm/common/Kconfig"
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 170c9bb..998d521 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -49,8 +49,16 @@ static inline void cache_wait(void __iomem *reg, unsigned long mask)
 static inline void cache_sync(void)
 {
 	void __iomem *base = l2x0_base;
+
+#ifdef ARM_ERRATA_753970
+#define L2X0_DUMMY_REG  0x740
+	/* write to an unmmapped register */
+	writel_relaxed(0, base + L2X0_DUMMY_REG);
+	cache_wait(base + L2X0_CACHE_SYNC, 1);
+#else
 	writel_relaxed(0, base + L2X0_CACHE_SYNC);
 	cache_wait(base + L2X0_CACHE_SYNC, 1);
+#endif
 }
 
 static inline void l2x0_clean_line(unsigned long addr)
-- 
1.7.0.4

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[Russell King - ARM Linux]

On Tue, Feb 15, 2011 at 04:48:03PM +0530, srinidhi kasagar wrote:
> +#ifdef ARM_ERRATA_753970
> +#define L2X0_DUMMY_REG  0x740
> +	/* write to an unmmapped register */
> +	writel_relaxed(0, base + L2X0_DUMMY_REG);
> +	cache_wait(base + L2X0_CACHE_SYNC, 1);
> +#else
>  	writel_relaxed(0, base + L2X0_CACHE_SYNC);
>  	cache_wait(base + L2X0_CACHE_SYNC, 1);
> +#endif

So why wrap cache_wait() up in that horrible ifdef as well - and why not
put the dummy register definition along side the other register definitions?

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[Catalin Marinas]

On 15 February 2011 11:18, srinidhi kasagar
<srinidhi.kasagar@stericsson.com> wrote:
> +config ARM_ERRATA_753970
> + ? ? ? bool "ARM errata: cache sync operation may be faulty"
> + ? ? ? depends on CACHE_PL310
> + ? ? ? help
> + ? ? ? ? This option enables the workaround for the 753970 PL310 erratum.

Is this number correct? I couldn't find it in ARM's internal database.

> + ? ? ? ? Under some condition the effect of cache sync operation on
> + ? ? ? ? the store buffer still remains when the operation completes.
> + ? ? ? ? This means that the store buffer is always asked to drain and
> + ? ? ? ? this prevents it from merging any further writes. The workaround
> + ? ? ? ? is to replace the normal offset of cache sync operation (0x730)
> + ? ? ? ? by another offset targeting an unmapped PL310 register 0x740.
> + ? ? ? ? This has the same effect as the cache sync operation: store buffer
> + ? ? ? ? drain and waiting for all buffers empty.

You may want to specify the revision number this applies to so that
people to enable it if not needed.

-- 
Catalin

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[Srinidhi KASAGAR]

On Tue, Feb 15, 2011 at 17:38:52 +0100, Catalin Marinas wrote:
> On 15 February 2011 11:18, srinidhi kasagar
> <srinidhi.kasagar@stericsson.com> wrote:
> > +config ARM_ERRATA_753970
> > + ? ? ? bool "ARM errata: cache sync operation may be faulty"
> > + ? ? ? depends on CACHE_PL310
> > + ? ? ? help
> > + ? ? ? ? This option enables the workaround for the 753970 PL310 erratum.
> 
> Is this number correct? I couldn't find it in ARM's internal database.
yes, we got this errata notice from ARM recently (10-feb-11), document
revision 12.1

> 
> > + ? ? ? ? Under some condition the effect of cache sync operation on
> > + ? ? ? ? the store buffer still remains when the operation completes.
> > + ? ? ? ? This means that the store buffer is always asked to drain and
> > + ? ? ? ? this prevents it from merging any further writes. The workaround
> > + ? ? ? ? is to replace the normal offset of cache sync operation (0x730)
> > + ? ? ? ? by another offset targeting an unmapped PL310 register 0x740.
> > + ? ? ? ? This has the same effect as the cache sync operation: store buffer
> > + ? ? ? ? drain and waiting for all buffers empty.
> 
> You may want to specify the revision number this applies to so that
> people to enable it if not needed.
OK, will include this revision number

srinidhi

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[Srinidhi KASAGAR]

On Tue, Feb 15, 2011 at 12:34:22 +0100, Russell King - ARM Linux wrote:
> On Tue, Feb 15, 2011 at 04:48:03PM +0530, srinidhi kasagar wrote:
> > +#ifdef ARM_ERRATA_753970
> > +#define L2X0_DUMMY_REG  0x740
> > +	/* write to an unmmapped register */
> > +	writel_relaxed(0, base + L2X0_DUMMY_REG);
> > +	cache_wait(base + L2X0_CACHE_SYNC, 1);
> > +#else
> >  	writel_relaxed(0, base + L2X0_CACHE_SYNC);
> >  	cache_wait(base + L2X0_CACHE_SYNC, 1);
> > +#endif
> 
> So why wrap cache_wait() up in that horrible ifdef as well - and why not
> put the dummy register definition along side the other register definitions?

OK. In fact cache_wait need not have to be under ifdef
as this bug found only on PL310 where the sync operations
are atomic. I will send out a new patch.

srinidhi

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[Srinidhi KASAGAR]

On Tue, Feb 15, 2011 at 12:34:22 +0100, Russell King - ARM Linux wrote:
> On Tue, Feb 15, 2011 at 04:48:03PM +0530, srinidhi kasagar wrote:
> > +#ifdef ARM_ERRATA_753970
> > +#define L2X0_DUMMY_REG  0x740
> > +	/* write to an unmmapped register */
> > +	writel_relaxed(0, base + L2X0_DUMMY_REG);
> > +	cache_wait(base + L2X0_CACHE_SYNC, 1);
> > +#else
> >  	writel_relaxed(0, base + L2X0_CACHE_SYNC);
> >  	cache_wait(base + L2X0_CACHE_SYNC, 1);
> > +#endif
> 
> So why wrap cache_wait() up in that horrible ifdef as well - and why not
> put the dummy register definition along side the other register definitions?

updated patch below

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[Catalin Marinas]

On Wed, 2011-02-16 at 05:08 +0000, Srinidhi KASAGAR wrote:
> On Tue, Feb 15, 2011 at 17:38:52 +0100, Catalin Marinas wrote:
> > On 15 February 2011 11:18, srinidhi kasagar
> > <srinidhi.kasagar@stericsson.com> wrote:
> > > +config ARM_ERRATA_753970
> > > +       bool "ARM errata: cache sync operation may be faulty"
> > > +       depends on CACHE_PL310
> > > +       help
> > > +         This option enables the workaround for the 753970 PL310 erratum.
> >
> > Is this number correct? I couldn't find it in ARM's internal database.
> 
> yes, we got this errata notice from ARM recently (10-feb-11), document
> revision 12.1

Ah, I found it (I was looking in the wrong place).

Catalin

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[Catalin Marinas]

On 16 February 2011 06:03, Srinidhi KASAGAR
<srinidhi.kasagar@stericsson.com> wrote:
> On Tue, Feb 15, 2011 at 12:34:22 +0100, Russell King - ARM Linux wrote:
>> On Tue, Feb 15, 2011 at 04:48:03PM +0530, srinidhi kasagar wrote:
>> > +#ifdef ARM_ERRATA_753970
>> > +#define L2X0_DUMMY_REG ?0x740
>> > + ? /* write to an unmmapped register */
>> > + ? writel_relaxed(0, base + L2X0_DUMMY_REG);
>> > + ? cache_wait(base + L2X0_CACHE_SYNC, 1);
>> > +#else
>> > ? ? writel_relaxed(0, base + L2X0_CACHE_SYNC);
>> > ? ? cache_wait(base + L2X0_CACHE_SYNC, 1);
>> > +#endif
>>
>> So why wrap cache_wait() up in that horrible ifdef as well - and why not
>> put the dummy register definition along side the other register definitions?
[...]
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -49,8 +49,14 @@ static inline void cache_wait(void __iomem *reg, unsigned long mask)
> ?static inline void cache_sync(void)
> ?{
> ? ? ? ?void __iomem *base = l2x0_base;
> +
> +#ifdef CONFIG_ARM_ERRATA_753970
> + ? ? ? /* write to an unmmapped register */
> + ? ? ? writel_relaxed(0, base + L2X0_DUMMY_REG);
> +#else
> ? ? ? ?writel_relaxed(0, base + L2X0_CACHE_SYNC);
> ? ? ? ?cache_wait(base + L2X0_CACHE_SYNC, 1);
> +#endif
> ?}

You could still leave cache_wait() after #endif, even though it is a
no-op. I think it is clearer that the erratum workaround only targets
the sync.

-- 
Catalin

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[Srinidhi KASAGAR]

On Wed, Feb 16, 2011 at 11:32:12 +0100, Catalin Marinas wrote:
> On 16 February 2011 06:03, Srinidhi KASAGAR
> <srinidhi.kasagar@stericsson.com> wrote:
> > On Tue, Feb 15, 2011 at 12:34:22 +0100, Russell King - ARM Linux wrote:
> >> On Tue, Feb 15, 2011 at 04:48:03PM +0530, srinidhi kasagar wrote:
> >> > +#ifdef ARM_ERRATA_753970
> >> > +#define L2X0_DUMMY_REG ?0x740
> >> > + ? /* write to an unmmapped register */
> >> > + ? writel_relaxed(0, base + L2X0_DUMMY_REG);
> >> > + ? cache_wait(base + L2X0_CACHE_SYNC, 1);
> >> > +#else
> >> > ? ? writel_relaxed(0, base + L2X0_CACHE_SYNC);
> >> > ? ? cache_wait(base + L2X0_CACHE_SYNC, 1);
> >> > +#endif
> >>
> >> So why wrap cache_wait() up in that horrible ifdef as well - and why not
> >> put the dummy register definition along side the other register definitions?
> [...]
> > --- a/arch/arm/mm/cache-l2x0.c
> > +++ b/arch/arm/mm/cache-l2x0.c
> > @@ -49,8 +49,14 @@ static inline void cache_wait(void __iomem *reg, unsigned long mask)
> > ?static inline void cache_sync(void)
> > ?{
> > ? ? ? ?void __iomem *base = l2x0_base;
> > +
> > +#ifdef CONFIG_ARM_ERRATA_753970
> > + ? ? ? /* write to an unmmapped register */
> > + ? ? ? writel_relaxed(0, base + L2X0_DUMMY_REG);
> > +#else
> > ? ? ? ?writel_relaxed(0, base + L2X0_CACHE_SYNC);
> > ? ? ? ?cache_wait(base + L2X0_CACHE_SYNC, 1);
> > +#endif
> > ?}
> 
> You could still leave cache_wait() after #endif, even though it is a
> no-op. I think it is clearer that the erratum workaround only targets
> the sync.

updated patch below.

[PATCH] ARM: errata: pl310 cache sync operation may be faulty

[Catalin Marinas]

On Wed, 2011-02-16 at 12:34 +0000, Srinidhi KASAGAR wrote:
> From 7a1fa2f8ec106bda4f940f5c9478ec16b2de6846 Mon Sep 17 00:00:00 2001
> From: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
> Date: Mon, 14 Feb 2011 17:07:06 +0530
> Subject: [PATCH] ARM: errata: pl310 cache sync operation may be faulty
> 
> The effect of cache sync operation is to drain the store
> buffer and wait for all internal buffers to be empty. In
> normal conditions, store buffer is able to merge the
> normal memory writes within its 32-byte data buffers.
> Due to this erratum present in r3p0, the effect of cache
> sync operation on the store buffer still remains when
> the operation completes. This means that the store buffer
> is always asked to drain and this prevents it from merging
> any further writes.
> 
> This can severely affect performance on the write traffic
> esp. on Normal memory NC one.
> 
> The proposed workaround is to replace the normal offset of
> cache sync operation(0x730) by another offset targeting an
> unmapped PL310 register 0x740.
> 
> Signed-off-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
> Acked-by: Linus Walleij <linus.walleij@stericsson.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>