l2x0: Add PL310 Errata 727915

PATCH In review Santosh Shilimkar 4 participants 13 messages Feb 18, 2011 – Mar 8, 2011

Subsystems (3)

Arm (32-bit) Arm sub-architectures OMAP2+ support

lore

Share & tools

lore

Santosh Shilimkar

author

2011-02-18 12:35 UTC #0 lore raw

cover l2x0: Add PL310 Errata 727915

Based on feedback from Catalin on the L2X0 ERRATA 742231 patch, I have split the original patch into two separate patches. First one adds errata and also removes OMAP dependency from it so that other SOCs can use them. Second patch populates the set_debug() pointer for OMAP using secure SMC call to alter the debug registers.

Mail thread on this:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg44571.html

The following changes since commit 85e2efbb1db9a18d218006706d6e4fbeb0216213:
Linus Torvalds (1):

        Linux 2.6.38-rc5

Santosh Shilimkar (2):
ARM: l2x0: Errata fix for flush by Way operation can cause data
corruption
omap4: l2x0: Populate set_debug() function and enable Errata 727915

 arch/arm/Kconfig                   |   15 ++++++++++++---
 arch/arm/include/asm/outercache.h  |    1 +
 arch/arm/mach-omap2/Kconfig        |    1 +
 arch/arm/mach-omap2/omap4-common.c |    7 +++++++
 arch/arm/mm/cache-l2x0.c           |   28 +++++++++++++++-------------
 5 files changed, 36 insertions(+), 16 deletions(-)

Santosh Shilimkar

author

2011-02-18 12:35 UTC #1 lore raw

PATCH 1/2 ARM: l2x0: Errata fix for flush by Way operation can cause data corruption

PL310 implements the Clean & Invalidate by Way L2 cache maintenance operation (offset 0x7FC). This operation runs in background so that PL310 can handle normal accesses while it is in progress. Under very rare circumstances, due to this erratum, write data can be lost when PL310 treats a cacheable write transaction during a Clean & Invalidate by Way operation.

Workaround:
Disable Write-Back and Cache Linefill (Debug Control Register) Clean & Invalidate by Way (0x7FC)
Re-enable Write-Back and Cache Linefill (Debug Control Register)

This patch also removes any OMAP dependency on PL310 Errata's

Signed-off-by Santosh Shilimkar <[email protected]>

Reviewed-by Catalin Marinas <[email protected]>

---
 arch/arm/Kconfig                  |   15 ++++++++++++---
 arch/arm/include/asm/outercache.h |    1 +
 arch/arm/mm/cache-l2x0.c          |   28 +++++++++++++++-------------
 3 files changed, 28 insertions(+), 16 deletions(-)

arch/arm/Kconfig +12 -3

index 26d45e5..b6eeab2 100644
@@ -1140,7 +1140,7 @@ config ARM_ERRATA_742231
 
 config PL310_ERRATA_588369
 	bool "Clean & Invalidate maintenance operations do not invalidate clean lines"
-	depends on CACHE_L2X0 && ARCH_OMAP4
+	depends on CACHE_L2X0
 	help
 	   The PL310 L2 cache controller implements three types of Clean &
 	   Invalidate maintenance operations: by Physical Address

@@ -1149,8 +1149,7 @@ config PL310_ERRATA_588369
 	   clean operation followed immediately by an invalidate operation,
 	   both performing to the same memory location. This functionality
 	   is not correctly implemented in PL310 as clean lines are not
-	   invalidated as a result of these operations. Note that this errata
-	   uses Texas Instrument's secure monitor api.
+	   invalidated as a result of these operations.
 
 config ARM_ERRATA_720789
 	bool "ARM errata: TLBIASIDIS and TLBIMVAIS operations can broadcast a faulty ASID"

@@ -1177,6 +1176,16 @@ config ARM_ERRATA_743622
 	  visible impact on the overall performance or power consumption of the
 	  processor.
 
+config PL310_ERRATA_727915
+	bool "Background Clean & Invalidate by Way operation can cause data corruption"
+	depends on CACHE_L2X0
+	help
+	  PL310 implements the Clean & Invalidate by Way L2 cache maintenance
+	  operation (offset 0x7FC). This operation runs in background so that
+	  PL310 can handle normal accesses while it is in progress. Under very
+	  rare circumstances, due to this erratum, write data can be lost when
+	  PL310 treats a cacheable write transaction during a Clean &
+	  Invalidate by Way operation.
 endmenu
 
 source "arch/arm/common/Kconfig"

arch/arm/include/asm/outercache.h +1 -0

index fc19009..348d513 100644
@@ -31,6 +31,7 @@ struct outer_cache_fns {
 #ifdef CONFIG_OUTER_CACHE_SYNC
 	void (*sync)(void);
 #endif
+	void (*set_debug)(unsigned long);
 };
 
 #ifdef CONFIG_OUTER_CACHE

arch/arm/mm/cache-l2x0.c +15 ~~-13~~

index 170c9bb..a8caee4 100644
@@ -67,18 +67,22 @@ static inline void l2x0_inv_line(unsigned long addr)
 	writel_relaxed(addr, base + L2X0_INV_LINE_PA);
 }
 
-#ifdef CONFIG_PL310_ERRATA_588369
+#if defined(CONFIG_PL310_ERRATA_588369) || defined(CONFIG_PL310_ERRATA_727915)
 static void debug_writel(unsigned long val)
 {
-	extern void omap_smc1(u32 fn, u32 arg);
-
-	/*
-	 * Texas Instrument secure monitor api to modify the
-	 * PL310 Debug Control Register.
-	 */
-	omap_smc1(0x100, val);
+	if (outer_cache.set_debug)
+		outer_cache.set_debug(val);
+	else
+		writel(val, l2x0_base + L2X0_DEBUG_CTRL);
+}
+#else
+/* Optimised out for non-errata case */
+static inline void debug_writel(unsigned long val)
+{
 }
+#endif
 
+#ifdef CONFIG_PL310_ERRATA_588369
 static inline void l2x0_flush_line(unsigned long addr)
 {
 	void __iomem *base = l2x0_base;

@@ -91,11 +95,6 @@ static inline void l2x0_flush_line(unsigned long addr)
 }
 #else
 
-/* Optimised out for non-errata case */
-static inline void debug_writel(unsigned long val)
-{
-}
-
 static inline void l2x0_flush_line(unsigned long addr)
 {
 	void __iomem *base = l2x0_base;

@@ -119,9 +118,11 @@ static void l2x0_flush_all(void)
 
 	/* clean all ways */
 	spin_lock_irqsave(&l2x0_lock, flags);
+	debug_writel(0x03);
 	writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_CLEAN_INV_WAY);
 	cache_wait_way(l2x0_base + L2X0_CLEAN_INV_WAY, l2x0_way_mask);
 	cache_sync();
+	debug_writel(0x00);
 	spin_unlock_irqrestore(&l2x0_lock, flags);
 }

@@ -329,6 +330,7 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
 	outer_cache.flush_all = l2x0_flush_all;
 	outer_cache.inv_all = l2x0_inv_all;
 	outer_cache.disable = l2x0_disable;
+	outer_cache.set_debug = NULL;
 
 	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",

Signature · 1 line

1.6.0.4

Catalin Marinas

2011-02-21 09:46 UTC #2 lore raw

PATCH 1/2 ARM: l2x0: Errata fix for flush by Way operation can cause data corruption

Quoted 7 lines · Santosh Shilimkar

PL310 implements the Clean & Invalidate by Way L2 cache maintenance operation (offset 0x7FC). This operation runs in background so that PL310 can handle normal accesses while it is in progress. Under very rare circumstances, due to this erratum, write data can be lost when PL310 treats a cacheable write transaction during a Clean & Invalidate by Way operation.

Quoted 8 lines

Workaround:
Disable Write-Back and Cache Linefill (Debug Control Register) Clean & Invalidate by Way (0x7FC)
Re-enable Write-Back and Cache Linefill (Debug Control Register)
This patch also removes any OMAP dependency on PL310 Errata's
Signed-off-by Santosh Shilimkar <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Russell King - ARM Linux

2011-02-27 12:00 UTC #3 lore raw

PATCH 1/2 ARM: l2x0: Errata fix for flush by Way operation can cause data corruption

In reply to Santosh Shilimkar

Quoted 14 lines · Santosh Shilimkar

arch/arm/mm/cache-l2x0.c +1 -3

index 170c9bb..a8caee4 100644
@@ -67,18 +67,22 @@ static inline void l2x0_inv_line(unsigned long addr)
 	writel_relaxed(addr, base + L2X0_INV_LINE_PA);
 }
 
-#ifdef CONFIG_PL310_ERRATA_588369
+#if defined(CONFIG_PL310_ERRATA_588369) || defined(CONFIG_PL310_ERRATA_727915)
 static void debug_writel(unsigned long val)
 {
-	extern void omap_smc1(u32 fn, u32 arg);
-

arch/arm/mm/cache-l2x0.c static inline void l2x0_inv_line(unsigned long addr) · line 77

Quoted 10 lines

arch/arm/mm/cache-l2x0.c +5 -5

-	/*
-	 * Texas Instrument secure monitor api to modify the
-	 * PL310 Debug Control Register.
-	 */
-	omap_smc1(0x100, val);
+	if (outer_cache.set_debug)
+		outer_cache.set_debug(val);
+	else
+		writel(val, l2x0_base + L2X0_DEBUG_CTRL);
+}

I think changing this to:

#define debug_writel(val) outer_cache.set_debug(val)

static void l2x0_set_debug(unsigned long val)

{
	writel(val, l2x0_base + L2X0_DEBUG_CTRL);
}

arch/arm/mm/cache-l2x0.c static inline void l2x0_inv_line(unsigned long addr) · line 82

Quoted 5 lines

arch/arm/mm/cache-l2x0.c +4 -0

+#else
+/* Optimised out for non-errata case */
+static inline void debug_writel(unsigned long val)
+{
 }

#define l2x0_set_debug NULL

arch/arm/mm/cache-l2x0.c void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask) · line 333

Quoted 7 lines

arch/arm/mm/cache-l2x0.c +2 -0

+#endif

@@ -329,6 +330,7 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
 	outer_cache.flush_all = l2x0_flush_all;
 	outer_cache.inv_all = l2x0_inv_all;
 	outer_cache.disable = l2x0_disable;
+	outer_cache.set_debug = NULL;

	outer_cache.set_debug = l2x0_set_debug;

may result in more efficient code as we're avoiding having to test the value of outer_cache.set_debug each time we want to call it and branch appropriately.

Santosh Shilimkar

author

2011-02-28 03:45 UTC #4 lore raw

PATCH 1/2 ARM: l2x0: Errata fix for flush by Way operationcan cause data corruption

Quoted 10 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Sunday, February 27, 2011 5:30 PM
To: Santosh Shilimkar
Cc: linux-arm-kernel at lists.infradead.org; tony at atomide.com; catalin.marinas at arm.com; linux-omap at vger.kernel.org
Subject: Re: [PATCH 1/2] ARM: l2x0: Errata fix for flush by Way operationcan cause data corruption
On Fri, Feb 18, 2011 at 06:05:24PM +0530, Santosh Shilimkar wrote:

[...]

Quoted 10 lines

Review void __init l2x0_init(void __iomem *base,
Quoted 2 lines
(quoted diff) +0 -0
@@ -329,6 +330,7 @@ void __init l2x0_init(void __iomem *base,
__u32 aux_val, __u32 aux_mask)
arch/arm/mm/cache-l2x0.c void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask) · line 333
Quoted 4 lines
arch/arm/mm/cache-l2x0.c +1 -0
 	outer_cache.flush_all = l2x0_flush_all;
 	outer_cache.inv_all = l2x0_inv_all;
 	outer_cache.disable = l2x0_disable;
+	outer_cache.set_debug = NULL;
	outer_cache.set_debug = l2x0_set_debug;

Quoted 3 lines

may result in more efficient code as we're avoiding having to test the value of outer_cache.set_debug each time we want to call it and branch appropriately.

Ok. Will do the necessary change and submit it to patch System

Regards,
Santosh

Russell King - ARM Linux

2011-02-28 10:13 UTC #5 lore raw

PATCH 1/2 ARM: l2x0: Errata fix for flush by Way operation can cause data corruption

In reply to Russell King - ARM Linux

Quoted 9 lines · Russell King - ARM Linux

arch/arm/mm/cache-l2x0.c static inline void l2x0_inv_line(unsigned long addr) · line 82
Quoted 5 lines
arch/arm/mm/cache-l2x0.c +4 -0
+#else
+/* Optimised out for non-errata case */
+static inline void debug_writel(unsigned long val)
+{
 }
#define l2x0_set_debug NULL
Quoted 1 line
+#endif

I notice you got rid of the inline function. Have you tried building this without the errata enabled?

Santosh Shilimkar

author

2011-02-28 12:47 UTC #6 lore raw

PATCH 1/2 ARM: l2x0: Errata fix for flush by Way operationcan cause data corruption

Quoted 19 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Monday, February 28, 2011 3:44 PM
To: Santosh Shilimkar
Cc: tony at atomide.com; catalin.marinas at arm.com; linux- omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/2] ARM: l2x0: Errata fix for flush by Way operationcan cause data corruption
Quoted 8 lines · Russell King - ARM Linux
arch/arm/mm/cache-l2x0.c static inline void l2x0_inv_line(unsigned long addr) · line 82
Quoted 5 lines
arch/arm/mm/cache-l2x0.c +4 -0
+#else
+/* Optimised out for non-errata case */
+static inline void debug_writel(unsigned long val)
+{
 }
#define l2x0_set_debug NULL

Quoted 4 lines

Quoted 1 line
Quoted 1 line
+#endif
I notice you got rid of the inline function. Have you tried building this without the errata enabled?

I accidently dropped the inline function while
incorporating the comment from you. :(

Fixed it. Updated version # 6770/1

Regards,
Santosh

Will Deacon

2011-03-07 12:07 UTC #7 lore raw

PATCH 1/2 ARM: l2x0: Errata fix for flush by Way operationcan cause data corruption

Hi Santosh,

Quoted 12 lines

Quoted 12 lines
Quoted 9 lines · Russell King - ARM Linux
arch/arm/mm/cache-l2x0.c static inline void l2x0_inv_line(unsigned long addr) · line 82
Quoted 5 lines
arch/arm/mm/cache-l2x0.c +4 -0
+#else
+/* Optimised out for non-errata case */
+static inline void debug_writel(unsigned long val)
+{
 }
#define l2x0_set_debug NULL
Quoted 1 line
+#endif

Quoted 7 lines

Quoted 2 lines
I notice you got rid of the inline function. Have you tried building this without the errata enabled?
I accidently dropped the inline function while
incorporating the comment from you. :(
Fixed it. Updated version # 6770/1

This version of the patch (as it appears in -next) is broken:

+#define debug_writel(val) outer_cache.set_debug(val)
+
+static void l2x0_set_debug(unsigned long val)
+{
+ writel(val, l2x0_base + L2X0_DEBUG_CTRL);
}

[...]

@@ -119,9 +120,11 @@ static void l2x0_flush_all(void)

/* clean all ways */
spin_lock_irqsave(&l2x0_lock, flags);
+ debug_writel(0x03);
writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_CLEAN_INV_WAY); cache_wait_way(l2x0_base + L2X0_CLEAN_INV_WAY, l2x0_way_mask); cache_sync();
+ debug_writel(0x00);
spin_unlock_irqrestore(&l2x0_lock, flags);
}

This deadlocks because the writel forces an outer cache sync, which then tries to acquire the spinlock which is held by the calling function.

If you change l2x0_set_debug to use writel_relaxed then you can avoid the problem.

Will

Santosh Shilimkar

author

2011-03-07 12:25 UTC #8 lore raw

PATCH 1/2 ARM: l2x0: Errata fix for flush by Wayoperationcan cause data corruption

Thanks Will for reporting it.

Quoted 36 lines

-----Original Message-----
From: linux-arm-kernel-bounces at lists.infradead.org [mailto:linux- arm-kernel-bounces at lists.infradead.org] On Behalf Of Will Deacon
Sent: Monday, March 07, 2011 5:38 PM
To: 'Santosh Shilimkar'; Russell King - ARM Linux
Cc: tony at atomide.com; Catalin Marinas; linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: RE: [PATCH 1/2] ARM: l2x0: Errata fix for flush by Wayoperationcan cause data corruption
Hi Santosh,
Quoted 1 line
Quoted 1 line
On Sun, Feb 27, 2011 at 12:00:21PM +0000, Russell King - ARM
Linux
Quoted 11 lines
Quoted 11 lines
Quoted 9 lines · wrote:
arch/arm/mm/cache-l2x0.c static inline void l2x0_inv_line(unsigned long addr) · line 82
Quoted 5 lines
arch/arm/mm/cache-l2x0.c +4 -0
+#else
+/* Optimised out for non-errata case */
+static inline void debug_writel(unsigned long val)
+{
 }
#define l2x0_set_debug NULL
Quoted 1 line
+#endif
Quoted 7 lines
Quoted 2 lines
I notice you got rid of the inline function. Have you tried building this without the errata enabled?
I accidently dropped the inline function while
incorporating the comment from you. :(
Fixed it. Updated version # 6770/1
This version of the patch (as it appears in -next) is broken:

Review

Quoted 5 lines

(quoted diff) +5 -0

+#define debug_writel(val)	outer_cache.set_debug(val)
+
+static void l2x0_set_debug(unsigned long val)
+{
+	writel(val, l2x0_base + L2X0_DEBUG_CTRL);

This should have been "writel_relaxed()" to avoid the cache sync.

Quoted 17 lines

}

[...]

arch/arm/mm/cache-l2x0.c +2 -0

@@ -119,9 +120,11 @@ static void l2x0_flush_all(void)
 
 	/* clean all ways */
 	spin_lock_irqsave(&l2x0_lock, flags);
+	debug_writel(0x03);
 	writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_CLEAN_INV_WAY);
 	cache_wait_way(l2x0_base + L2X0_CLEAN_INV_WAY, l2x0_way_mask);
 	cache_sync();
+	debug_writel(0x00);
 	spin_unlock_irqrestore(&l2x0_lock, flags);
 }

Quoted 7 lines

This deadlocks because the writel forces an outer cache sync, which then tries to acquire the spinlock which is held by the calling function.
If you change l2x0_set_debug to use writel_relaxed then you can avoid the problem.

Ya understood. I couldn't test non-errata case because direct right doesn't work because of security and henced missed it.

Below is the updated version. Also attached.

Russell,
Do you want me to push this to patch system or you can apply this one?

Regards,
Santosh
------

Russell King - ARM Linux

2011-03-07 19:10 UTC #9 lore raw

PATCH 1/2 ARM: l2x0: Errata fix for flush by Wayoperationcan cause data corruption

Quoted 5 lines · Santosh Shilimkar

Below is the updated version. Also attached.
Russell,
Do you want me to push this to patch system or you can apply this one?

Patch system please.

Santosh Shilimkar

author

2011-03-08 06:00 UTC #10 lore raw

PATCH 1/2 ARM: l2x0: Errata fix for flush by Wayoperationcancause data corruption

Quoted 9 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Tuesday, March 08, 2011 12:40 AM
To: Santosh Shilimkar
Cc: Will Deacon; tony at atomide.com; Catalin Marinas; linux- omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/2] ARM: l2x0: Errata fix for flush by Wayoperationcancause data corruption

Quoted 8 lines

Quoted 5 lines · Santosh Shilimkar
Below is the updated version. Also attached.
Russell,
Do you want me to push this to patch system or you can apply this one?
Patch system please.

Done.
patch 6795/1

Regards,
Santosh

Will Deacon

2011-03-07 12:07 UTC #11 lore raw

RE: [PATCH 1/2] ARM: l2x0: Errata fix for flush by Way operationcan cause data corruption

In reply to Santosh Shilimkar

Hi Santosh,

Quoted 12 lines

Quoted 12 lines
Quoted 9 lines · Russell King - ARM Linux
arch/arm/mm/cache-l2x0.c static inline void l2x0_inv_line(unsigned long addr) · line 82
Quoted 5 lines
arch/arm/mm/cache-l2x0.c +4 -0
+#else
+/* Optimised out for non-errata case */
+static inline void debug_writel(unsigned long val)
+{
 }
#define l2x0_set_debug NULL
Quoted 1 line
+#endif

Quoted 7 lines

Quoted 2 lines
I notice you got rid of the inline function. Have you tried building this without the errata enabled?
I accidently dropped the inline function while
incorporating the comment from you. :(
Fixed it. Updated version # 6770/1

This version of the patch (as it appears in -next) is broken:

+#define debug_writel(val) outer_cache.set_debug(val)
+
+static void l2x0_set_debug(unsigned long val)
+{
+ writel(val, l2x0_base + L2X0_DEBUG_CTRL);
}

[...]

@@ -119,9 +120,11 @@ static void l2x0_flush_all(void)

This deadlocks because the writel forces an outer cache sync, which then tries to acquire the spinlock which is held by the calling function.

If you change l2x0_set_debug to use writel_relaxed then you can avoid the problem.

Will

Santosh Shilimkar

author

2011-02-18 12:35 UTC #12 lore raw

PATCH 2/2 omap4: l2x0: Populate set_debug() function and enable Errata 727915

In reply to Santosh Shilimkar

Populate the l2x0 set_debug function pointer with OMAP secure call and enable the PL310 Errata 727915

Signed-off-by Santosh Shilimkar <[email protected]>

Reviewed-by Catalin Marinas <[email protected]>

Cc Tony Lindgren <[email protected]>

---
 arch/arm/mach-omap2/Kconfig        |    1 +
 arch/arm/mach-omap2/omap4-common.c |    7 +++++++
 2 files changed, 8 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/Kconfig +1 -0

index 1a2cf62..b69fa0a 100644
@@ -45,6 +45,7 @@ config ARCH_OMAP4
 	select CPU_V7
 	select ARM_GIC
 	select PL310_ERRATA_588369
+	select PL310_ERRATA_727915
 	select ARM_ERRATA_720789
 	select ARCH_HAS_OPP
 	select PM_OPP if PM

arch/arm/mach-omap2/omap4-common.c +7 -0

index 1926864..9ef8c29 100644
@@ -52,6 +52,12 @@ static void omap4_l2x0_disable(void)
 	omap_smc1(0x102, 0x0);
 }
 
+static void omap4_l2x0_set_debug(unsigned long val)
+{
+	/* Program PL310 L2 Cache controller debug register */
+	omap_smc1(0x100, val);
+}
+
 static int __init omap_l2_cache_init(void)
 {
 	u32 aux_ctrl = 0;

@@ -99,6 +105,7 @@ static int __init omap_l2_cache_init(void)
 	 * specific one
 	*/
 	outer_cache.disable = omap4_l2x0_disable;
+	outer_cache.set_debug = omap4_l2x0_set_debug;
 
 	return 0;
 }

Signature · 1 line

1.6.0.4