[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines

[Santosh Shilimkar]

This patch implements the work-around for the errata 588369. The secure API
is used to alter L2 debug regsiter because of trust-zone.

v3 is rebased on the latest 2.6.33-rc1 which has Russell's "pending-l2x0"
changes merged. The patch is tested on OMAP4430 SDP.

Signed-off-by: Woodruff Richard <r-woodruff2@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
cc: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/Kconfig         |   13 ++++++++++++
 arch/arm/mm/cache-l2x0.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 233a222..3891b77 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -917,6 +917,19 @@ config ARM_ERRATA_460075
 	  ACTLR register. Note that setting specific bits in the ACTLR register
 	  may not be available in non-secure mode.
 
+config PL310_ERRATA_588369
+	bool "Clean & Invalidate maintenance operations do not invalidate clean lines"
+	depends on CACHE_L2X0 && ARCH_OMAP4
+	help
+	   The PL310 L2 cache controller implements three types of Clean &
+	   Invalidate maintenance operations: by Physical Address
+	   (offset 0x7F0), by Index/Way (0x7F8) and by Way (0x7FC).
+	   They are architecturally defined to behave as the execution of a
+	   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.
 endmenu
 
 source "arch/arm/common/Kconfig"
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index cb8fc65..5443c0d 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -28,6 +28,24 @@
 static void __iomem *l2x0_base;
 static DEFINE_SPINLOCK(l2x0_lock);
 
+#ifdef CONFIG_PL310_ERRATA_588369
+static void debug_writel(unsigned long val)
+{
+	register unsigned long r0 asm("r0") = val;
+	/*
+	 * Texas Instrument secure monitor api to modify the PL310
+	 * Debug Control Register.
+	 */
+	__asm__ __volatile__(
+		__asmeq("%0", "r0")
+		"ldr r12, =0x100\n"
+		"dsb\n"
+		"smc\n"
+		: : "r" (r0)
+		: "r4", "r5", "r6", "r7", "r8");
+}
+#endif
+
 static inline void cache_wait(void __iomem *reg, unsigned long mask)
 {
 	/* wait for the operation to complete */
@@ -62,15 +80,33 @@ static void l2x0_inv_range(unsigned long start, unsigned long end)
 	spin_lock_irqsave(&l2x0_lock, flags);
 	if (start & (CACHE_LINE_SIZE - 1)) {
 		start &= ~(CACHE_LINE_SIZE - 1);
+#ifdef CONFIG_PL310_ERRATA_588369
+		debug_writel(0x03);
+		cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
+		writel(start, base + L2X0_CLEAN_LINE_PA);
+		cache_wait(base + L2X0_INV_LINE_PA, 1);
+		writel(start, base + L2X0_INV_LINE_PA);
+		debug_writel(0x00);
+#else
 		cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
 		writel(start, base + L2X0_CLEAN_INV_LINE_PA);
+#endif
 		start += CACHE_LINE_SIZE;
 	}
 
 	if (end & (CACHE_LINE_SIZE - 1)) {
 		end &= ~(CACHE_LINE_SIZE - 1);
+#ifdef CONFIG_PL310_ERRATA_588369
+		debug_writel(0x03);
+		cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
+		writel(end, base + L2X0_CLEAN_LINE_PA);
+		cache_wait(base + L2X0_INV_LINE_PA, 1);
+		writel(end, base + L2X0_INV_LINE_PA);
+			debug_writel(0x00);
+#else
 		cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
 		writel(end, base + L2X0_CLEAN_INV_LINE_PA);
+#endif
 	}
 
 	while (start < end) {
@@ -129,8 +165,17 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
 		unsigned long blk_end = start + min(end - start, 4096UL);
 
 		while (start < blk_end) {
+#ifdef CONFIG_PL310_ERRATA_588369
+			debug_writel(0x03);
+			/* Clean by PA followed by Invalidate by PA */
+			cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
+			writel(start, base + L2X0_CLEAN_LINE_PA);
+			cache_wait(base + L2X0_INV_LINE_PA, 1);
+			writel(start, base + L2X0_INV_LINE_PA);
+#else
 			cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
 			writel(start, base + L2X0_CLEAN_INV_LINE_PA);
+#endif
 			start += CACHE_LINE_SIZE;
 		}
 
@@ -139,7 +184,12 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
 			spin_lock_irqsave(&l2x0_lock, flags);
 		}
 	}
+#ifdef CONFIG_PL310_ERRATA_588369
+	cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
+	cache_wait(base + L2X0_INV_LINE_PA, 1);
+#else
 	cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
+#endif
 	cache_sync();
 	spin_unlock_irqrestore(&l2x0_lock, flags);
 }
-- 
1.6.0.4

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines

[Shilimkar, Santosh]

Russell/Catalin,

Can this make way to patch system if you are ok with this version ?
> -----Original Message-----
> From: Shilimkar, Santosh
> Sent: Monday, December 21, 2009 3:39 PM
> To: linux at arm.linux.org.uk
> Cc: linux-arm-kernel at lists.infradead.org; linux-omap at vger.kernel.org; tony at atomide.com; Shilimkar,
> Santosh; Woodruff, Richard; Catalin Marinas
> Subject: [PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines
> 
> This patch implements the work-around for the errata 588369. The secure API
> is used to alter L2 debug regsiter because of trust-zone.
> 
> v3 is rebased on the latest 2.6.33-rc1 which has Russell's "pending-l2x0"
> changes merged. The patch is tested on OMAP4430 SDP.
> 
> Signed-off-by: Woodruff Richard <r-woodruff2@ti.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> cc: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm/Kconfig         |   13 ++++++++++++
>  arch/arm/mm/cache-l2x0.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 233a222..3891b77 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -917,6 +917,19 @@ config ARM_ERRATA_460075
>  	  ACTLR register. Note that setting specific bits in the ACTLR register
>  	  may not be available in non-secure mode.
> 
> +config PL310_ERRATA_588369
> +	bool "Clean & Invalidate maintenance operations do not invalidate clean lines"
> +	depends on CACHE_L2X0 && ARCH_OMAP4
> +	help
> +	   The PL310 L2 cache controller implements three types of Clean &
> +	   Invalidate maintenance operations: by Physical Address
> +	   (offset 0x7F0), by Index/Way (0x7F8) and by Way (0x7FC).
> +	   They are architecturally defined to behave as the execution of a
> +	   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.
>  endmenu
> 
>  source "arch/arm/common/Kconfig"
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index cb8fc65..5443c0d 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -28,6 +28,24 @@
>  static void __iomem *l2x0_base;
>  static DEFINE_SPINLOCK(l2x0_lock);
> 
> +#ifdef CONFIG_PL310_ERRATA_588369
> +static void debug_writel(unsigned long val)
> +{
> +	register unsigned long r0 asm("r0") = val;
> +	/*
> +	 * Texas Instrument secure monitor api to modify the PL310
> +	 * Debug Control Register.
> +	 */
> +	__asm__ __volatile__(
> +		__asmeq("%0", "r0")
> +		"ldr r12, =0x100\n"
> +		"dsb\n"
> +		"smc\n"
> +		: : "r" (r0)
> +		: "r4", "r5", "r6", "r7", "r8");
> +}
> +#endif
> +
>  static inline void cache_wait(void __iomem *reg, unsigned long mask)
>  {
>  	/* wait for the operation to complete */
> @@ -62,15 +80,33 @@ static void l2x0_inv_range(unsigned long start, unsigned long end)
>  	spin_lock_irqsave(&l2x0_lock, flags);
>  	if (start & (CACHE_LINE_SIZE - 1)) {
>  		start &= ~(CACHE_LINE_SIZE - 1);
> +#ifdef CONFIG_PL310_ERRATA_588369
> +		debug_writel(0x03);
> +		cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
> +		writel(start, base + L2X0_CLEAN_LINE_PA);
> +		cache_wait(base + L2X0_INV_LINE_PA, 1);
> +		writel(start, base + L2X0_INV_LINE_PA);
> +		debug_writel(0x00);
> +#else
>  		cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
>  		writel(start, base + L2X0_CLEAN_INV_LINE_PA);
> +#endif
>  		start += CACHE_LINE_SIZE;
>  	}
> 
>  	if (end & (CACHE_LINE_SIZE - 1)) {
>  		end &= ~(CACHE_LINE_SIZE - 1);
> +#ifdef CONFIG_PL310_ERRATA_588369
> +		debug_writel(0x03);
> +		cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
> +		writel(end, base + L2X0_CLEAN_LINE_PA);
> +		cache_wait(base + L2X0_INV_LINE_PA, 1);
> +		writel(end, base + L2X0_INV_LINE_PA);
> +			debug_writel(0x00);
> +#else
>  		cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
>  		writel(end, base + L2X0_CLEAN_INV_LINE_PA);
> +#endif
>  	}
> 
>  	while (start < end) {
> @@ -129,8 +165,17 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
>  		unsigned long blk_end = start + min(end - start, 4096UL);
> 
>  		while (start < blk_end) {
> +#ifdef CONFIG_PL310_ERRATA_588369
> +			debug_writel(0x03);
> +			/* Clean by PA followed by Invalidate by PA */
> +			cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
> +			writel(start, base + L2X0_CLEAN_LINE_PA);
> +			cache_wait(base + L2X0_INV_LINE_PA, 1);
> +			writel(start, base + L2X0_INV_LINE_PA);
> +#else
>  			cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
>  			writel(start, base + L2X0_CLEAN_INV_LINE_PA);
> +#endif
>  			start += CACHE_LINE_SIZE;
>  		}
> 
> @@ -139,7 +184,12 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
>  			spin_lock_irqsave(&l2x0_lock, flags);
>  		}
>  	}
> +#ifdef CONFIG_PL310_ERRATA_588369
> +	cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
> +	cache_wait(base + L2X0_INV_LINE_PA, 1);
> +#else
>  	cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
> +#endif
>  	cache_sync();
>  	spin_unlock_irqrestore(&l2x0_lock, flags);
>  }
> --
> 1.6.0.4

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines

[Catalin Marinas]

On Mon, 2009-12-21 at 10:09 +0000, Santosh Shilimkar wrote:
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index cb8fc65..5443c0d 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -28,6 +28,24 @@
>  static void __iomem *l2x0_base;
>  static DEFINE_SPINLOCK(l2x0_lock);
> 
> +#ifdef CONFIG_PL310_ERRATA_588369
> +static void debug_writel(unsigned long val)
> +{
> +       register unsigned long r0 asm("r0") = val;
> +       /*
> +        * Texas Instrument secure monitor api to modify the PL310
> +        * Debug Control Register.
> +        */
> +       __asm__ __volatile__(
> +               __asmeq("%0", "r0")
> +               "ldr r12, =0x100\n"
> +               "dsb\n"
> +               "smc\n"
> +               : : "r" (r0)
> +               : "r4", "r5", "r6", "r7", "r8");

Does your secure monitor corrupt r4-r8? Maybe you could add a comment
with a few words on this API.

Do you need to specify "r12" as well? What about "cc", are they
preserved by the secure monitor?

> @@ -139,7 +184,12 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
>                         spin_lock_irqsave(&l2x0_lock, flags);
>                 }
>         }
> +#ifdef CONFIG_PL310_ERRATA_588369
> +       cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
> +       cache_wait(base + L2X0_INV_LINE_PA, 1);
> +#else
>         cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
> +#endif

I don't think we need to way on two separate registers here. AFAICT, bit
1 of those registers is shared for all the operations.

As a general comment, maybe an inline function called something like
wait_writel(before/after) would be better than a lot of ifdefs in the
code, especially if one has a different workaround other than using TI's
secure monitor.

-- 
Catalin

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines

[Russell King - ARM Linux]

On Mon, Jan 11, 2010 at 02:37:08PM +0000, Catalin Marinas wrote:
> On Mon, 2009-12-21 at 10:09 +0000, Santosh Shilimkar wrote:
> > @@ -139,7 +184,12 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
> >                         spin_lock_irqsave(&l2x0_lock, flags);
> >                 }
> >         }
> > +#ifdef CONFIG_PL310_ERRATA_588369
> > +       cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
> > +       cache_wait(base + L2X0_INV_LINE_PA, 1);
> > +#else
> >         cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
> > +#endif
> 
> I don't think we need to way on two separate registers here. AFAICT, bit
> 1 of those registers is shared for all the operations.
> 
> As a general comment, maybe an inline function called something like
> wait_writel(before/after) would be better than a lot of ifdefs in the
> code, especially if one has a different workaround other than using TI's
> secure monitor.

Since the bit is shared between all 'R7' registers, the ifdef above makes
no sense.  We can just wait on any R7 register - I suggest that bit of
code is left as-is.

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines

[Shilimkar, Santosh]

Thanks for cooments !!
> -----Original Message-----
> From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> Sent: Monday, January 11, 2010 8:07 PM
> To: Shilimkar, Santosh
> Cc: linux at arm.linux.org.uk; linux-arm-kernel at lists.infradead.org; linux-omap at vger.kernel.org;
> tony at atomide.com; Woodruff, Richard
> Subject: Re: [PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines
> 
> On Mon, 2009-12-21 at 10:09 +0000, Santosh Shilimkar wrote:
> > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> > index cb8fc65..5443c0d 100644
> > --- a/arch/arm/mm/cache-l2x0.c
> > +++ b/arch/arm/mm/cache-l2x0.c
> > @@ -28,6 +28,24 @@
> >  static void __iomem *l2x0_base;
> >  static DEFINE_SPINLOCK(l2x0_lock);
> >
> > +#ifdef CONFIG_PL310_ERRATA_588369
> > +static void debug_writel(unsigned long val)
> > +{
> > +       register unsigned long r0 asm("r0") = val;
> > +       /*
> > +        * Texas Instrument secure monitor api to modify the PL310
> > +        * Debug Control Register.
> > +        */
> > +       __asm__ __volatile__(
> > +               __asmeq("%0", "r0")
> > +               "ldr r12, =0x100\n"
> > +               "dsb\n"
> > +               "smc\n"
> > +               : : "r" (r0)
> > +               : "r4", "r5", "r6", "r7", "r8");
> 
> Does your secure monitor corrupt r4-r8? Maybe you could add a comment
> with a few words on this API.
Yes they are corrupted. I will add some comments on the API.
> Do you need to specify "r12" as well? What about "cc", are they
> preserved by the secure monitor?
r12 and reset of the registers are preserved. Lr needs to be saved but
because of function call, the compiler saves/restores it.
I didn't get your this comment "What about "cc", are they preserved
by the secure monitor ?  Do you mean rest of the register. If yes then
the secure monitor don't tamper those registers.
 
> > @@ -139,7 +184,12 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
> >                         spin_lock_irqsave(&l2x0_lock, flags);
> >                 }
> >         }
> > +#ifdef CONFIG_PL310_ERRATA_588369
> > +       cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
> > +       cache_wait(base + L2X0_INV_LINE_PA, 1);
> > +#else
> >         cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
> > +#endif
> 
> I don't think we need to way on two separate registers here. AFAICT, bit
> 1 of those registers is shared for all the operations.
> 
> As a general comment, maybe an inline function called something like
> wait_writel(before/after) would be better than a lot of ifdefs in the
> code, especially if one has a different workaround other than using TI's
> secure monitor.
I agree. As suggested by Russell, we don't need above #ifdef and can 
be removed 

Regards,
Santosh

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines

[Shilimkar, Santosh]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Monday, January 11, 2010 8:15 PM
> To: Catalin Marinas
> Cc: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org; linux-omap at vger.kernel.org;
> tony at atomide.com; Woodruff, Richard
> Subject: Re: [PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines
> 
> On Mon, Jan 11, 2010 at 02:37:08PM +0000, Catalin Marinas wrote:
> > On Mon, 2009-12-21 at 10:09 +0000, Santosh Shilimkar wrote:
> > > @@ -139,7 +184,12 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
> > >                         spin_lock_irqsave(&l2x0_lock, flags);
> > >                 }
> > >         }
> > > +#ifdef CONFIG_PL310_ERRATA_588369
> > > +       cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
> > > +       cache_wait(base + L2X0_INV_LINE_PA, 1);
> > > +#else
> > >         cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
> > > +#endif
> >
> > I don't think we need to way on two separate registers here. AFAICT, bit
> > 1 of those registers is shared for all the operations.
> >
> > As a general comment, maybe an inline function called something like
> > wait_writel(before/after) would be better than a lot of ifdefs in the
> > code, especially if one has a different workaround other than using TI's
> > secure monitor.
> 
> Since the bit is shared between all 'R7' registers, the ifdef above makes
> no sense.  We can just wait on any R7 register - I suggest that bit of
> code is left as-is.
Ok. will update the patch.

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines

[Russell King - ARM Linux]

On Mon, Jan 11, 2010 at 08:48:37PM +0530, Shilimkar, Santosh wrote:
> Thanks for cooments !!
> > -----Original Message-----
> > From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> > Sent: Monday, January 11, 2010 8:07 PM
> > To: Shilimkar, Santosh
> > Cc: linux at arm.linux.org.uk; linux-arm-kernel at lists.infradead.org; linux-omap at vger.kernel.org;
> > tony at atomide.com; Woodruff, Richard
> > Subject: Re: [PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines
> >
> > Does your secure monitor corrupt r4-r8? Maybe you could add a comment
> > with a few words on this API.
>
> Yes they are corrupted. I will add some comments on the API.
>
> > Do you need to specify "r12" as well? What about "cc", are they
> > preserved by the secure monitor?
>
> r12 and reset of the registers are preserved. Lr needs to be saved but
> because of function call, the compiler saves/restores it.

That's not guaranteed; the compiler can re-use lr for its own purposes
within a function.  You need to add lr to the list of clobbered registers.

> I didn't get your this comment "What about "cc", are they preserved
> by the secure monitor ?  Do you mean rest of the register. If yes then
> the secure monitor don't tamper those registers.

No - Catalin means are the NZCV flags changed by the secure monitor, or
are they preserved?  If they're preserved, you don't need a "cc" clobber.
If they're not preserved, you do.

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines

[Shilimkar, Santosh]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Monday, January 11, 2010 8:54 PM
> To: Shilimkar, Santosh
> Cc: Catalin Marinas; linux-arm-kernel at lists.infradead.org; linux-omap at vger.kernel.org;
> tony at atomide.com; Woodruff, Richard
> Subject: Re: [PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines
> 
> On Mon, Jan 11, 2010 at 08:48:37PM +0530, Shilimkar, Santosh wrote:
> > Thanks for cooments !!
> > > -----Original Message-----
> > > From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> > > Sent: Monday, January 11, 2010 8:07 PM
> > > To: Shilimkar, Santosh
> > > Cc: linux at arm.linux.org.uk; linux-arm-kernel at lists.infradead.org; linux-omap at vger.kernel.org;
> > > tony at atomide.com; Woodruff, Richard
> > > Subject: Re: [PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines
> > >
> > > Does your secure monitor corrupt r4-r8? Maybe you could add a comment
> > > with a few words on this API.
> >
> > Yes they are corrupted. I will add some comments on the API.
> >
> > > Do you need to specify "r12" as well? What about "cc", are they
> > > preserved by the secure monitor?
> >
> > r12 and reset of the registers are preserved. Lr needs to be saved but
> > because of function call, the compiler saves/restores it.
> 
> That's not guaranteed; the compiler can re-use lr for its own purposes
> within a function.  You need to add lr to the list of clobbered registers.
Ok will add that.
> > I didn't get your this comment "What about "cc", are they preserved
> > by the secure monitor ?  Do you mean rest of the register. If yes then
> > the secure monitor don't tamper those registers.
> 
> No - Catalin means are the NZCV flags changed by the secure monitor, or
> are they preserved?  If they're preserved, you don't need a "cc" clobber.
> If they're not preserved, you do.
I will double check this but IIRC the flags are preserved.

Regards,
Santosh

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do notinvalidate clean lines

[Catalin Marinas]

On Mon, 2010-01-11 at 15:23 +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 11, 2010 at 08:48:37PM +0530, Shilimkar, Santosh wrote:
> > > Do you need to specify "r12" as well? What about "cc", are they
> > > preserved by the secure monitor?
> >
> > r12 and reset of the registers are preserved. Lr needs to be saved but
> > because of function call, the compiler saves/restores it.
> 
> That's not guaranteed; the compiler can re-use lr for its own purposes
> within a function.  You need to add lr to the list of clobbered registers.

Do we need to specify "r12" in the list of clobbered registers as the
inline asm explicitly modifies it? Or the compiler doesn't touch it.

-- 
Catalin

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do notinvalidate clean lines

[Russell King - ARM Linux]

On Mon, Jan 11, 2010 at 04:26:30PM +0000, Catalin Marinas wrote:
> On Mon, 2010-01-11 at 15:23 +0000, Russell King - ARM Linux wrote:
> > On Mon, Jan 11, 2010 at 08:48:37PM +0530, Shilimkar, Santosh wrote:
> > > > Do you need to specify "r12" as well? What about "cc", are they
> > > > preserved by the secure monitor?
> > >
> > > r12 and reset of the registers are preserved. Lr needs to be saved but
> > > because of function call, the compiler saves/restores it.
> > 
> > That's not guaranteed; the compiler can re-use lr for its own purposes
> > within a function.  You need to add lr to the list of clobbered registers.
> 
> Do we need to specify "r12" in the list of clobbered registers as the
> inline asm explicitly modifies it? Or the compiler doesn't touch it.

Yes - again, the compiler should be told about everything that
potentially could change in an asm statement.

The compiler could decide to inline debug_writel, and use things
like r12 and lr to store temporary values within the parent function
across these calls.

[PATCH v3] ARM: L2 : Errata 588369: Clean & Invalidate do not invalidate clean lines

[Shilimkar, Santosh]

<snip>
> > No - Catalin means are the NZCV flags changed by the secure monitor, or
> > are they preserved?  If they're preserved, you don't need a "cc" clobber.
> > If they're not preserved, you do.
> I will double check this but IIRC the flags are preserved.
I confirm that the flags are restored. Will repost the patch with 
suggested changes.