ARM: errata: pl310 cache sync operation may be faulty
PATCH 1/1 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.
--- arch/arm/Kconfig | 15 +++++++++++++++ arch/arm/mm/cache-l2x0.c | 8 ++++++++ 2 files changed, 23 insertions(+), 0 deletions(-)
arch/arm/Kconfig +15 -0
@@ -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"
arch/arm/mm/cache-l2x0.c +8 -0
@@ -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)
Signature · 1 line
1.7.0.4
PATCH 1/1 ARM: errata: pl310 cache sync operation may be faulty
arch/arm/mm/cache-l2x0.c static inline void cache_wait(void __iomem *reg, unsigned long mask) · line 61
Quoted 9 lines · srinidhi kasagar
arch/arm/mm/cache-l2x0.c +7
-0+#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 1/1 ARM: errata: pl310 cache sync operation may be faulty
Quoted 13 lines · Russell King - ARM Linux
arch/arm/mm/cache-l2x0.c static inline void cache_wait(void __iomem *reg, unsigned long mask) · line 61
Quoted 9 lines · srinidhi kasagar
arch/arm/mm/cache-l2x0.c +7
-0+#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 1/1 ARM: errata: pl310 cache sync operation may be faulty
In reply to Russell King - ARM LinuxQuoted 13 lines · Russell King - ARM Linux
arch/arm/mm/cache-l2x0.c static inline void cache_wait(void __iomem *reg, unsigned long mask) · line 61
Quoted 9 lines · srinidhi kasagar
arch/arm/mm/cache-l2x0.c +7
-0+#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 1/1 ARM: errata: pl310 cache sync operation may be faulty
Quoted 14 lines · Srinidhi KASAGAR
Quoted 13 lines · Russell King - ARM Linux
Quoted 9 lines · srinidhi kasagar
(quoted diff) +6
-0+#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);
+#endifSo 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?
[...]
Quoted 5 lines
arch/arm/mm/cache-l2x0.c +0
-0@@ -49,8 +49,14 @@ static inline void cache_wait(void __iomem *reg, unsigned long mask) ?static inline void cache_sync(void) ?{
arch/arm/mm/cache-l2x0.c static inline void cache_wait(void __iomem *reg, unsigned long mask) · line 60
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +6
-0? ? ? ?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.
Signature · 1 line
Catalin
PATCH 1/1 ARM: errata: pl310 cache sync operation may be faulty
Quoted 28 lines · Catalin Marinas
Quoted 14 lines · Srinidhi KASAGAR
Quoted 13 lines · Russell King - ARM Linux
Quoted 9 lines · srinidhi kasagar
(quoted diff) +6
-0+#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);
+#endifSo 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?[...]
Quoted 11 lines
arch/arm/mm/cache-l2x0.c +5
-0@@ -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
Quoted 8 lines
arch/arm/mm/cache-l2x0.c static inline void cache_wait(void __iomem *reg, unsigned long mask) · line 60
Quoted 4 lines
arch/arm/mm/cache-l2x0.c +1
-0? ? ? ?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 1/1 ARM: errata: pl310 cache sync operation may be faulty
Quoted 18 lines · Srinidhi KASAGAR
From
7a1fa2f8ec106bda4f940f5c9478ec16b2de6846Mon Sep 17 00:00:00 2001
From: srinidhi kasagar <[email protected]>
Date: Mon, 14 Feb 2011 17:07:06 +0530
Subject: [PATCH] ARM: errata: pl310 cache sync operation may be faultyThe 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.
Quoted 6 lines
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 <[email protected]>Acked-by Linus Walleij <[email protected]>
PATCH 1/1 ARM: errata: pl310 cache sync operation may be faulty
In reply to srinidhi kasagarQuoted 5 lines · srinidhi kasagar
+config
ARM_ERRATA_753970
+ ? ? ? bool "ARM errata: cache sync operation may be faulty"
+ ? ? ? depends onCACHE_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.
Quoted 8 lines
+ ? ? ? ? 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 register0x740.
+ ? ? ? ? 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.
Signature · 1 line
Catalin
PATCH 1/1 ARM: errata: pl310 cache sync operation may be faulty
Quoted 9 lines · Catalin Marinas
Quoted 5 lines · srinidhi kasagar
+config
ARM_ERRATA_753970
+ ? ? ? bool "ARM errata: cache sync operation may be faulty"
+ ? ? ? depends onCACHE_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
Quoted 10 lines
Quoted 9 lines
+ ? ? ? ? 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 register0x740.
+ ? ? ? ? This has the same effect as the cache sync operation: store buffer
+ ? ? ? ? drain and waiting for all buffers empty.
Quoted 2 lines
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 1/1 ARM: errata: pl310 cache sync operation may be faulty
Quoted 9 lines · Srinidhi KASAGAR
Quoted 8 lines · Catalin Marinas
Quoted 5 lines · srinidhi kasagar
arch/arm/Kconfig +5
-0+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.
Quoted 4 lines
Quoted 1 line
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