On Tue, Mar 6, 2018 at 10:11 PM, Fabio Estevam <feste...@gmail.com> wrote:
> On Tue, Mar 6, 2018 at 9:31 AM, Lothar Waßmann <l...@karo-electronics.de> 
> wrote:
>> Without the 'volatile' attribute the compiler is entitled to move the
>> asm code around or optimize it out.
>> So, your patch is the correct fix independent from the gcc version
>> used.
> Yes, but then it would be better to fix all the places where asm is
> used without volatile.

That'd be a quite big patch.  A quick grep shows me that
there is over 100 asm() without volatile.

git grep -e '\basm *(' | grep  -v -e volatile -e nop | wc -l

Do you guys really want to put volatile on all of these now?
We are at rc4 and Tom is planing to cut the release
March 12th.

I'm attaching a tentative patch to fix only syscounter.c.
If it looks good, I'l resend it by git-send-email.

From 8772a9b5b532da4c1d0656ffa21f324e72d369e8 Mon Sep 17 00:00:00 2001
From: Yasushi SHOJI <ya...@atmark-techno.com>
Date: Wed, 7 Mar 2018 14:38:05 +0900
Subject: [PATCH] imx: syscounter: make sure asm is volatile

Without the volatile attribute, compilers are entitled to optimize out
the same asm().  In the case of __udelay() in syscounter.c, it calls
`get_ticks()` twice, one for the starting time and the second in the
loop to check the current time.  When compilers inline `get_ticks()`
they see the same `mrrc` instructions and optimize out the second one.
This leads to infinite loop since we don't get updated value from the
system counter.

Here is a portion of the disassembly of __udelay:

  88:	428b      	cmp	r3, r1
  8a:	f8ce 20a4 	str.w	r2, [lr, #164]	; 0xa4
  8e:	bf08      	it	eq
  90:	4282      	cmpeq	r2, r0
  92:	f8ce 30a0 	str.w	r3, [lr, #160]	; 0xa0
  96:	d3f7      	bcc.n	88 <__udelay+0x88>
  98:	e8bd 8cf0 	ldmia.w	sp!, {r4, r5, r6, r7, sl, fp, pc}

Note that final jump / loop at 96 to 88, we don't have any `mrrc`.

With a volatile attribute, the above changes to this:

  8a:	ec53 2f0e 	mrrc	15, 0, r2, r3, cr14
  8e:	42ab      	cmp	r3, r5
  90:	f8c1 20a4 	str.w	r2, [r1, #164]	; 0xa4
  94:	bf08      	it	eq
  96:	42a2      	cmpeq	r2, r4
  98:	f8c1 30a0 	str.w	r3, [r1, #160]	; 0xa0
  9c:	d3f5      	bcc.n	8a <__udelay+0x8a>
  9e:	e8bd 8cf0 	ldmia.w	sp!, {r4, r5, r6, r7, sl, fp, pc}
  a2:	bf00      	nop

I'm advised to put volatile on all asm(), so this commit also adds it
to the asm() in timer_init().
 arch/arm/mach-imx/syscounter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/syscounter.c b/arch/arm/mach-imx/syscounter.c
index 9290918dca..1d4ebfe343 100644
--- a/arch/arm/mach-imx/syscounter.c
+++ b/arch/arm/mach-imx/syscounter.c
@@ -62,7 +62,7 @@ int timer_init(void)
 	unsigned long val, freq;
-	asm("mcr p15, 0, %0, c14, c0, 0" : : "r" (freq));
+	asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (freq));
 	writel(freq, &sctr->cntfid0);
@@ -82,7 +82,7 @@ unsigned long long get_ticks(void)
 	unsigned long long now;
-	asm("mrrc p15, 0, %Q0, %R0, c14" : "=r" (now));
+	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (now));
 	gd->arch.tbl = (unsigned long)(now & 0xffffffff);
 	gd->arch.tbu = (unsigned long)(now >> 32);

U-Boot mailing list

Reply via email to