[PATCH 3/5] ARM: l2x0: Errata fix for flush by Way operation can cause data corruption
Andrei Warkentin
andreiw at motorola.com
Tue Feb 15 04:10:28 EST 2011
On Tue, Feb 15, 2011 at 1:14 AM, Santosh Shilimkar
<santosh.shilimkar at ti.com> wrote:
>> -----Original Message-----
>> From: Santosh Shilimkar [mailto:santosh.shilimkar at ti.com]
>> Sent: Monday, February 14, 2011 10:39 AM
>> To: Andrei Warkentin
>> Cc: linux-omap at vger.kernel.org; Kevin Hilman; tony at atomide.com;
>> linux-arm-kernel at lists.infradead.org; Catalin Marinas
>> Subject: RE: [PATCH 3/5] ARM: l2x0: Errata fix for flush by Way
>> operation can cause data corruption
>>
>
> [....]
>
>> > ...
>> I understood that from first comment. But I am not in favor
>> of polluting common ARM files with SOC specific #ifdeffery.
>> We have gone over this when first errata support
>> was added for PL310
>>
>> I have a better way to handle this scenario.
>> Expect an updated patch for this.
>>
>
> Below is the updated version which should remove any
> OMAP dependency on these errata's. Attached same.
>
> ----
> From: Santosh Shilimkar <santosh.shilimkar at ti.com>
> Date: Fri, 14 Jan 2011 14:16:04 +0530
> Subject: [v2 PATCH 3/5] 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)
>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar at ti.com>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> ---
> arch/arm/Kconfig | 13 ++++++++++++-
> arch/arm/include/asm/outercache.h | 1 +
> arch/arm/mach-omap2/Kconfig | 3 +++
> arch/arm/mach-omap2/omap4-common.c | 7 +++++++
> arch/arm/mm/cache-l2x0.c | 28 +++++++++++++++-------------
> 5 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 5cff165..ebadd95 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -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 && CACHE_PL310
> help
> The PL310 L2 cache controller implements three types of Clean &
> Invalidate maintenance operations: by Physical Address
> @@ -1177,6 +1177,17 @@ 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 && CACHE_PL310
> + 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 Note that this errata uses Texas
> + Instrument's secure monitor api to implement the work around.
> endmenu
>
> source "arch/arm/common/Kconfig"
> diff --git a/arch/arm/include/asm/outercache.h
> b/arch/arm/include/asm/outercache.h
> index fc19009..348d513 100644
> --- a/arch/arm/include/asm/outercache.h
> +++ b/arch/arm/include/asm/outercache.h
> @@ -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
> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
> index f285dd7..fd11ab4 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -45,7 +45,10 @@ config ARCH_OMAP4
> select CPU_V7
> select ARM_GIC
> select LOCAL_TIMERS
> + select CACHE_L2X0
> + select CACHE_PL310
> select PL310_ERRATA_588369
> + select PL310_ERRATA_727915
> select ARM_ERRATA_720789
> select ARCH_HAS_OPP
> select PM_OPP if PM
> diff --git a/arch/arm/mach-omap2/omap4-common.c
> b/arch/arm/mach-omap2/omap4-common.c
> index 1926864..9ef8c29 100644
> --- a/arch/arm/mach-omap2/omap4-common.c
> +++ b/arch/arm/mach-omap2/omap4-common.c
> @@ -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;
> }
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index 170c9bb..a8caee4 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -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",
> --
> 1.6.0.4
>
Why set by default to NULL, why not have a normal l2x0_set_debug which
just does a write to L2X0_DEBUG_CTRL, and then just invoke the
function pointer when you wish to manipulate the DCR? That way you
don't need the runtime check, and it's just clearer, I think.
Also, why not do something like -
....
do_stuff();
#ifdef CONFIG_NEED_ERRATA_1234
do_errata_stuff();
#endif
do_more_stuff();
...
instead of -
...
#ifdef CONFIG_NEED_ERRATA_1234
do_some_stuf() {
bar();
}
#else
{
do_some_stuff() {
}
// nothing
}
do_stuff();
do_some_stuff();
do_more_stuff();
It's not exactly clear otherwise what's happening and why there are
these extra calls that sometimes are compiled in, and sometimes
aren't. The way I am suggesting, you would just look at a function
block and you can easily tell what is different in the errata case,
and you don't need to jump back and forth to figure that out. I think
it makes for cleaner code.
And it would be nice if the errata had some revision data attached to
them, as in my suggestion patch I sent earlier. It's a nuisance to
look at Kconfig wondering which errata you might be interested in,
especially if your platform spans several generations and revs of
different parts. Then you need to dig up an errata sheet and scan it
yourself. The alternative is to be able to select a known rev for
PL310, for example, which would just pick the errata that apply to
that rev, or at least mention the affected revs in the documentation
for the option.
More information about the linux-arm-kernel
mailing list