mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH v2 0/3] arm/cpu/lowlevel cleanups
@ 2014-12-11 20:51 Uwe Kleine-König
  2014-12-11 20:51 ` [PATCH v2 1/3] arm/cpu/lowlevel: invalidate i-cache before enabling Uwe Kleine-König
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2014-12-11 20:51 UTC (permalink / raw)
  To: barebox

Hello,

compared to (implicit) v1 this changes the order to make the i-cache fix the
first patch to address Sascha's review remarks. I also removed the explicit
dropping of branch predictor entries which Lucas pointed out to be unnecessary.

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

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

-- 
2.1.3


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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2 1/3] arm/cpu/lowlevel: invalidate i-cache before enabling
  2014-12-11 20:51 [PATCH v2 0/3] arm/cpu/lowlevel cleanups Uwe Kleine-König
@ 2014-12-11 20:51 ` Uwe Kleine-König
  2014-12-18  8:40   ` Uwe Kleine-König
  2014-12-11 20:51 ` [PATCH v2 2/3] arm/cpu/lowlevel: add and fix comments for CPSR and SCTLR accesses Uwe Kleine-König
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Uwe Kleine-König @ 2014-12-11 20:51 UTC (permalink / raw)
  To: barebox

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). For the already existing mcr instruction see
the newly added comment.

This patch also unifies handling of ARMv6 and ARMv7, the isb instruction
can also be done on the latter via mcr which simplifies the code a bit.

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

diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
index c615d5b58160..e000cd8eae4c 100644
--- a/arch/arm/cpu/lowlevel.S
+++ b/arch/arm/cpu/lowlevel.S
@@ -11,9 +11,26 @@ ENTRY(arm_cpu_lowlevel_init)
 	orr	r12, r12, #0xd3
 	msr	cpsr, r12
 
-#if __LINUX_ARM_ARCH__ >= 7
-	isb
-#elif __LINUX_ARM_ARCH__ == 6
+#if __LINUX_ARM_ARCH__ >= 6
+	/*
+	 * ICIALLU: Invalidate all instruction caches to PoU,
+	 * includes flushing of branch predictors.
+	 * Even if the i-cache is off it might contain stale entries
+	 * that are better discarded before enabling the cache.
+	 * Architectually this is even possible after a cold reset.
+	 */
+	mcr	p15, 0, r12, c7, c5, 0
+	/* 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
+	 * 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
 #endif
 
-- 
2.1.3


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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2 2/3] arm/cpu/lowlevel: add and fix comments for CPSR and SCTLR accesses
  2014-12-11 20:51 [PATCH v2 0/3] arm/cpu/lowlevel cleanups Uwe Kleine-König
  2014-12-11 20:51 ` [PATCH v2 1/3] arm/cpu/lowlevel: invalidate i-cache before enabling Uwe Kleine-König
@ 2014-12-11 20:51 ` Uwe Kleine-König
  2014-12-11 20:51 ` [PATCH v2 3/3] arm/cpu/lowlevel: Don't save the return address in another register Uwe Kleine-König
  2014-12-15  9:58 ` [PATCH v2 0/3] arm/cpu/lowlevel cleanups Sascha Hauer
  3 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2014-12-11 20:51 UTC (permalink / raw)
  To: barebox

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 e000cd8eae4c..de7afba2c317 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
@@ -34,10 +34,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
@@ -51,7 +53,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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2 3/3] arm/cpu/lowlevel: Don't save the return address in another register
  2014-12-11 20:51 [PATCH v2 0/3] arm/cpu/lowlevel cleanups Uwe Kleine-König
  2014-12-11 20:51 ` [PATCH v2 1/3] arm/cpu/lowlevel: invalidate i-cache before enabling Uwe Kleine-König
  2014-12-11 20:51 ` [PATCH v2 2/3] arm/cpu/lowlevel: add and fix comments for CPSR and SCTLR accesses Uwe Kleine-König
@ 2014-12-11 20:51 ` Uwe Kleine-König
  2014-12-15  9:58 ` [PATCH v2 0/3] arm/cpu/lowlevel cleanups Sascha Hauer
  3 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2014-12-11 20:51 UTC (permalink / raw)
  To: barebox

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 de7afba2c317..b76222d8f3c0 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
@@ -55,5 +54,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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/3] arm/cpu/lowlevel cleanups
  2014-12-11 20:51 [PATCH v2 0/3] arm/cpu/lowlevel cleanups Uwe Kleine-König
                   ` (2 preceding siblings ...)
  2014-12-11 20:51 ` [PATCH v2 3/3] arm/cpu/lowlevel: Don't save the return address in another register Uwe Kleine-König
@ 2014-12-15  9:58 ` Sascha Hauer
  3 siblings, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2014-12-15  9:58 UTC (permalink / raw)
  To: Uwe Kleine-König; +Cc: barebox

On Thu, Dec 11, 2014 at 09:51:30PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> compared to (implicit) v1 this changes the order to make the i-cache fix the
> first patch to address Sascha's review remarks. I also removed the explicit
> dropping of branch predictor entries which Lucas pointed out to be unnecessary.
> 
> Uwe Kleine-König (3):
>   arm/cpu/lowlevel: invalidate i-cache before enabling
>   arm/cpu/lowlevel: add and fix comments for CPSR and SCTLR accesses
>   arm/cpu/lowlevel: Don't save the return address in another register
> 
>  arch/arm/cpu/lowlevel.S | 38 ++++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)

Applied, thanks

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/3] arm/cpu/lowlevel: invalidate i-cache before enabling
  2014-12-11 20:51 ` [PATCH v2 1/3] arm/cpu/lowlevel: invalidate i-cache before enabling Uwe Kleine-König
@ 2014-12-18  8:40   ` Uwe Kleine-König
  0 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2014-12-18  8:40 UTC (permalink / raw)
  To: barebox

Hello,

On Thu, Dec 11, 2014 at 09:51:31PM +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). For the already existing mcr instruction see
> the newly added comment.
> 
> This patch also unifies handling of ARMv6 and ARMv7, the isb instruction
> can also be done on the latter via mcr which simplifies the code a bit.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/cpu/lowlevel.S | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
> index c615d5b58160..e000cd8eae4c 100644
> --- a/arch/arm/cpu/lowlevel.S
> +++ b/arch/arm/cpu/lowlevel.S
> @@ -11,9 +11,26 @@ ENTRY(arm_cpu_lowlevel_init)
>  	orr	r12, r12, #0xd3
>  	msr	cpsr, r12
>  
> -#if __LINUX_ARM_ARCH__ >= 7
> -	isb
> -#elif __LINUX_ARM_ARCH__ == 6
> +#if __LINUX_ARM_ARCH__ >= 6
> +	/*
> +	 * ICIALLU: Invalidate all instruction caches to PoU,
> +	 * includes flushing of branch predictors.
> +	 * Even if the i-cache is off it might contain stale entries
> +	 * that are better discarded before enabling the cache.
> +	 * Architectually this is even possible after a cold reset.
> +	 */
> +	mcr	p15, 0, r12, c7, c5, 0
> +	/* 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
> +	 * 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
>  #endif
I just noticed that this is not optimal here. E.g. for mvebu_defconfig
builds this code is not compiled in because

	CONFIG_ARCH_ARMADA_370=y
	CONFIG_ARCH_ARMADA_XP=y
	CONFIG_ARCH_DOVE=y
	CONFIG_ARCH_KIRKWOOD=y

and so __LINUX_ARM_ARCH__ = 5. That means it's not worse than before my
patch, but still not optimal. I'll check how to fix that.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-18  8:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-11 20:51 [PATCH v2 0/3] arm/cpu/lowlevel cleanups Uwe Kleine-König
2014-12-11 20:51 ` [PATCH v2 1/3] arm/cpu/lowlevel: invalidate i-cache before enabling Uwe Kleine-König
2014-12-18  8:40   ` Uwe Kleine-König
2014-12-11 20:51 ` [PATCH v2 2/3] arm/cpu/lowlevel: add and fix comments for CPSR and SCTLR accesses Uwe Kleine-König
2014-12-11 20:51 ` [PATCH v2 3/3] arm/cpu/lowlevel: Don't save the return address in another register Uwe Kleine-König
2014-12-15  9:58 ` [PATCH v2 0/3] arm/cpu/lowlevel cleanups Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox