[U-Boot] [PATCH] imx: syscounter: make sure asm is volatile

[Fabio Estevam]

[Adding Stefano]

On Thu, Mar 8, 2018 at 1:21 AM, Yasushi SHOJI <yasushi.shoji at gmail.com> wrote:
> 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[1] to put volatile on all asm(), so this commit also adds it
> to the asm() in timer_init().
>
> [1]: https://lists.denx.de/pipermail/u-boot/2018-March/322062.html
>
> Signed-off-by: Yasushi SHOJI <yasushi.shoji at gmail.com>
> Reviewed-by: Fabio Estevam <fabio.estevam at nxp.com>

Hi Stefano,

Do you think this one could be applied for the upcoming release?

Thanks

> ---
>  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
>