[PATCH 0/4] arm/cpu/lowlevel cleanups

[PATCH 0/4] arm/cpu/lowlevel cleanups

[Uwe Kleine-König]

Hello,

similar to how the d-cache is cleaned before enabling the same should be
done for the i-cache.

I'd not consider this series as urgent because of patch 4. I created
this series because there was a problem that could have happend because
of stale entries in the i-cache, but this proved to be wrong. Still for
correctness sake the patch should applied.

The other three patches are just minor cleanups that I did on the way to
create patch 4.

Uwe Kleine-König (4):
  arm/cpu/lowlevel: add and fix comments for CPSR and SCTLR accesses
  arm/cpu/lowlevel: Use coprocessor instruction for ARMv7, too
  arm/cpu/lowlevel: Don't save the return address in another register
  arm/cpu/lowlevel: invalidate i-cache before enabling

 arch/arm/cpu/lowlevel.S | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

-- 
2.1.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/4] arm/cpu/lowlevel: add and fix comments for CPSR and SCTLR accesses

[Uwe Kleine-König]

A part of the existing comments was incomplete or missleading.
Adding the register name to mcr/mrc instructions helps finding the
corresponding documentation in the manuals.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/cpu/lowlevel.S | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
index c615d5b58160..7cc14b954f1f 100644
--- a/arch/arm/cpu/lowlevel.S
+++ b/arch/arm/cpu/lowlevel.S
@@ -5,7 +5,7 @@
 .section ".text_bare_init_","ax"
 ENTRY(arm_cpu_lowlevel_init)
 	mov	r2, lr
-	/* set the cpu to SVC32 mode */
+	/* set the cpu to SVC32 mode, mask irq and fiq */
 	mrs	r12, cpsr
 	bic	r12, r12, #0x1f
 	orr	r12, r12, #0xd3
@@ -17,10 +17,12 @@ ENTRY(arm_cpu_lowlevel_init)
 	mcr	p15, 0, r12, c7, c5, 4
 #endif
 
-	/* disable MMU stuff and caches */
-	mrc	p15, 0, r12, c1, c0, 0
-	bic	r12, r12 , #(CR_M | CR_C | CR_B)
+	/* disable MMU stuff and data/unified caches */
+	mrc	p15, 0, r12, c1, c0, 0		/* SCTLR */
+	bic	r12, r12, #(CR_M | CR_C | CR_B)
 	bic	r12, r12, #(CR_S | CR_R | CR_V)
+
+	/* enable instruction cache */
 	orr	r12, r12, #CR_I
 
 #if __LINUX_ARM_ARCH__ >= 6
@@ -34,7 +36,7 @@ ENTRY(arm_cpu_lowlevel_init)
 	orr	r12, r12, #CR_B
 #endif
 
-	mcr	p15, 0, r12, c1, c0, 0
+	mcr	p15, 0, r12, c1, c0, 0		/* SCTLR */
 
 	mov	pc, r2
 ENDPROC(arm_cpu_lowlevel_init)
-- 
2.1.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/4] arm/cpu/lowlevel: Use coprocessor instruction for ARMv7, too

[Uwe Kleine-König]

ARMv7 also supports the mcr syntax for the isb instructions, so use that
one to simplify the code a bit. The Linux kernel does the same, for
example in the decompressor's __armv7_mmu_cache_on function.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/cpu/lowlevel.S | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
index 7cc14b954f1f..7c99ecaad9dd 100644
--- a/arch/arm/cpu/lowlevel.S
+++ b/arch/arm/cpu/lowlevel.S
@@ -11,10 +11,16 @@ ENTRY(arm_cpu_lowlevel_init)
 	orr	r12, r12, #0xd3
 	msr	cpsr, r12
 
-#if __LINUX_ARM_ARCH__ >= 7
-	isb
-#elif __LINUX_ARM_ARCH__ == 6
-	mcr	p15, 0, r12, c7, c5, 4
+#if __LINUX_ARM_ARCH__ >= 6
+	/*
+	 * Note that the ARM Architecture Reference Manual, ARMv7-A and ARMv7-R
+	 * edition (ARM DDI 0406C.c) doesn't define this instruction in the
+	 * ARMv6 part (D12.7.10). It only has: "Support of additional
+	 * operations is IMPLEMENTATION DEFINED".
+	 * But an earlier version of the ARMARM (ARM DDI 0100I) does define it
+	 * as "Flush prefetch buffer (PrefetchFlush)".
+	 */
+	mcr	p15, 0, r12, c7, c5, 4		/* ISB */
 #endif
 
 	/* disable MMU stuff and data/unified caches */
-- 
2.1.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/4] arm/cpu/lowlevel: Don't save the return address in another register

[Uwe Kleine-König]

The corresponding code doesn't use the lr register (neither explicitly
nor implicitly by the bl instruction), so there is no gain in using r2
here.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/cpu/lowlevel.S | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
index 7c99ecaad9dd..dd0f75a8802a 100644
--- a/arch/arm/cpu/lowlevel.S
+++ b/arch/arm/cpu/lowlevel.S
@@ -4,7 +4,6 @@
 
 .section ".text_bare_init_","ax"
 ENTRY(arm_cpu_lowlevel_init)
-	mov	r2, lr
 	/* set the cpu to SVC32 mode, mask irq and fiq */
 	mrs	r12, cpsr
 	bic	r12, r12, #0x1f
@@ -44,5 +43,5 @@ ENTRY(arm_cpu_lowlevel_init)
 
 	mcr	p15, 0, r12, c1, c0, 0		/* SCTLR */
 
-	mov	pc, r2
+	mov	pc, lr
 ENDPROC(arm_cpu_lowlevel_init)
-- 
2.1.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/4] arm/cpu/lowlevel: invalidate i-cache before enabling

[Uwe Kleine-König]

Architecturally the cache contents are undefined so it might well
contain stale data at reset. So better be save than sorry.

I verifyed that the added instructions are defined for both, ARMv6 and
ARMv7, using the ARM Architecture Reference Manual, ARMv7-A and ARMv7-R
edition (ARM DDI 0406C.c).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/cpu/lowlevel.S | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
index dd0f75a8802a..af2b0a8ac93a 100644
--- a/arch/arm/cpu/lowlevel.S
+++ b/arch/arm/cpu/lowlevel.S
@@ -11,7 +11,19 @@ ENTRY(arm_cpu_lowlevel_init)
 	msr	cpsr, r12
 
 #if __LINUX_ARM_ARCH__ >= 6
+/*
+ * Invalidate instruction cache and branch predictor. Even if the
+ * i-cache is off it might contain stale entries that are better
+ * discarded before enabling the cache.
+ */
+	/* ICIALLU: Invalidate all instruction caches to PoU */
+	mcr	p15, 0, r12, c7, c5, 0
+	/* BPIALL: Invalidate all branch predictors */
+	mcr	p15, 0, r12, c7, c5, 6
+	/* DSB, ensure completion of the invalidation */
+	mcr	p15, 0, r12, c7, c10, 4
 	/*
+	 * ISB, ensure instruction fetch path is in sync.
 	 * Note that the ARM Architecture Reference Manual, ARMv7-A and ARMv7-R
 	 * edition (ARM DDI 0406C.c) doesn't define this instruction in the
 	 * ARMv6 part (D12.7.10). It only has: "Support of additional
@@ -19,7 +31,7 @@ ENTRY(arm_cpu_lowlevel_init)
 	 * But an earlier version of the ARMARM (ARM DDI 0100I) does define it
 	 * as "Flush prefetch buffer (PrefetchFlush)".
 	 */
-	mcr	p15, 0, r12, c7, c5, 4		/* ISB */
+	mcr	p15, 0, r12, c7, c5, 4
 #endif
 
 	/* disable MMU stuff and data/unified caches */
-- 
2.1.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] arm/cpu/lowlevel: invalidate i-cache before enabling

[Lucas Stach]

Am Donnerstag, den 11.12.2014, 10:15 +0100 schrieb Uwe Kleine-König:
> Architecturally the cache contents are undefined so it might well
> contain stale data at reset. So better be save than sorry.
> 
> I verifyed that the added instructions are defined for both, ARMv6 and
> ARMv7, using the ARM Architecture Reference Manual, ARMv7-A and ARMv7-R
> edition (ARM DDI 0406C.c).
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/cpu/lowlevel.S | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
> index dd0f75a8802a..af2b0a8ac93a 100644
> --- a/arch/arm/cpu/lowlevel.S
> +++ b/arch/arm/cpu/lowlevel.S
> @@ -11,7 +11,19 @@ ENTRY(arm_cpu_lowlevel_init)
>  	msr	cpsr, r12
>  
>  #if __LINUX_ARM_ARCH__ >= 6
> +/*
> + * Invalidate instruction cache and branch predictor. Even if the
> + * i-cache is off it might contain stale entries that are better
> + * discarded before enabling the cache.
> + */
> +	/* ICIALLU: Invalidate all instruction caches to PoU */
> +	mcr	p15, 0, r12, c7, c5, 0
> +	/* BPIALL: Invalidate all branch predictors */
> +	mcr	p15, 0, r12, c7, c5, 6

I don't think the above operation is needed as it is already implicitly
done in the ICIALLU op.

Citing the ARM ARM about ICIALLU/ICIALLUIS: "These instructions
invalidate the entire instruction cache or caches, and, if branch
predictors are architecturally-visible, all branch predictors."

Otherwise I think this series looks good. It makes the code more
readable and while the I-Cache _should_ be clean when the core comes out
of reset I can well see the use-case where we drop into barebox from an
unclean state.

So with the above fixed:
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] arm/cpu/lowlevel: invalidate i-cache before enabling

[Sascha Hauer]

On Thu, Dec 11, 2014 at 10:15:27AM +0100, Uwe Kleine-König wrote:
> Architecturally the cache contents are undefined so it might well
> contain stale data at reset. So better be save than sorry.
> 
> I verifyed that the added instructions are defined for both, ARMv6 and
> ARMv7, using the ARM Architecture Reference Manual, ARMv7-A and ARMv7-R
> edition (ARM DDI 0406C.c).
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/cpu/lowlevel.S | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
> index dd0f75a8802a..af2b0a8ac93a 100644
> --- a/arch/arm/cpu/lowlevel.S
> +++ b/arch/arm/cpu/lowlevel.S
> @@ -11,7 +11,19 @@ ENTRY(arm_cpu_lowlevel_init)
>  	msr	cpsr, r12
>  
>  #if __LINUX_ARM_ARCH__ >= 6
> +/*
> + * Invalidate instruction cache and branch predictor. Even if the
> + * i-cache is off it might contain stale entries that are better
> + * discarded before enabling the cache.
> + */

Please indent like the other comments

> +	/* ICIALLU: Invalidate all instruction caches to PoU */
> +	mcr	p15, 0, r12, c7, c5, 0
> +	/* BPIALL: Invalidate all branch predictors */
> +	mcr	p15, 0, r12, c7, c5, 6
> +	/* DSB, ensure completion of the invalidation */
> +	mcr	p15, 0, r12, c7, c10, 4
>  	/*
> +	 * ISB, ensure instruction fetch path is in sync.

Should this comment be in 2/4?

>  	 * Note that the ARM Architecture Reference Manual, ARMv7-A and ARMv7-R
>  	 * edition (ARM DDI 0406C.c) doesn't define this instruction in the
>  	 * ARMv6 part (D12.7.10). It only has: "Support of additional
> @@ -19,7 +31,7 @@ ENTRY(arm_cpu_lowlevel_init)
>  	 * But an earlier version of the ARMARM (ARM DDI 0100I) does define it
>  	 * as "Flush prefetch buffer (PrefetchFlush)".
>  	 */
> -	mcr	p15, 0, r12, c7, c5, 4		/* ISB */
> +	mcr	p15, 0, r12, c7, c5, 4

This comment was just introduced in 2/4.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] arm/cpu/lowlevel: invalidate i-cache before enabling

[Uwe Kleine-König]

On Thu, Dec 11, 2014 at 01:05:53PM +0100, Sascha Hauer wrote:
> On Thu, Dec 11, 2014 at 10:15:27AM +0100, Uwe Kleine-König wrote:
> > Architecturally the cache contents are undefined so it might well
> > contain stale data at reset. So better be save than sorry.
> > 
> > I verifyed that the added instructions are defined for both, ARMv6 and
> > ARMv7, using the ARM Architecture Reference Manual, ARMv7-A and ARMv7-R
> > edition (ARM DDI 0406C.c).
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  arch/arm/cpu/lowlevel.S | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
> > index dd0f75a8802a..af2b0a8ac93a 100644
> > --- a/arch/arm/cpu/lowlevel.S
> > +++ b/arch/arm/cpu/lowlevel.S
> > @@ -11,7 +11,19 @@ ENTRY(arm_cpu_lowlevel_init)
> >  	msr	cpsr, r12
> >  
> >  #if __LINUX_ARM_ARCH__ >= 6
> > +/*
> > + * Invalidate instruction cache and branch predictor. Even if the
> > + * i-cache is off it might contain stale entries that are better
> > + * discarded before enabling the cache.
> > + */
> 
> Please indent like the other comments
I don't care much, my in[dt]ention was to start at column 0 for comments
that affect >1 instruction, and at column 8 for a single instruction
comment. Can rework if you care.
 
> > +	/* ICIALLU: Invalidate all instruction caches to PoU */
> > +	mcr	p15, 0, r12, c7, c5, 0
> > +	/* BPIALL: Invalidate all branch predictors */
> > +	mcr	p15, 0, r12, c7, c5, 6
> > +	/* DSB, ensure completion of the invalidation */
> > +	mcr	p15, 0, r12, c7, c10, 4
> >  	/*
> > +	 * ISB, ensure instruction fetch path is in sync.
> 
> Should this comment be in 2/4?
I don't know what the intention of the stand-alone isb is, so I just
added a note that it is an isb. Without the flushing above "ensure
instruction fetch path is in sync" sounds wrong to me.
 
> >  	 * Note that the ARM Architecture Reference Manual, ARMv7-A and ARMv7-R
> >  	 * edition (ARM DDI 0406C.c) doesn't define this instruction in the
> >  	 * ARMv6 part (D12.7.10). It only has: "Support of additional
> > @@ -19,7 +31,7 @@ ENTRY(arm_cpu_lowlevel_init)
> >  	 * But an earlier version of the ARMARM (ARM DDI 0100I) does define it
> >  	 * as "Flush prefetch buffer (PrefetchFlush)".
> >  	 */
> > -	mcr	p15, 0, r12, c7, c5, 4		/* ISB */
> > +	mcr	p15, 0, r12, c7, c5, 4
> 
> This comment was just introduced in 2/4.
See above.

I can reorder the series to have the added comments at the end.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox