Hi, 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 153 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. Best, -- yashi
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; freq = CONFIG_SC_TIMER_CLK; - 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); -- 2.16.2
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot