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

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

2018-03-09 Thread Stefano Babic

On 09/03/2018 11:46, Fabio Estevam wrote:
> [Adding Stefano]
> 
> On Thu, Mar 8, 2018 at 1:21 AM, Yasushi SHOJI  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:   428bcmp r3, r1
>>   8a:   f8ce 20a4   str.w   r2, [lr, #164]  ; 0xa4
>>   8e:   bf08it  eq
>>   90:   4282cmpeq   r2, r0
>>   92:   f8ce 30a0   str.w   r3, [lr, #160]  ; 0xa0
>>   96:   d3f7bcc.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   mrrc15, 0, r2, r3, cr14
>>   8e:   42abcmp r3, r5
>>   90:   f8c1 20a4   str.w   r2, [r1, #164]  ; 0xa4
>>   94:   bf08it  eq
>>   96:   42a2cmpeq   r2, r4
>>   98:   f8c1 30a0   str.w   r3, [r1, #160]  ; 0xa0
>>   9c:   d3f5bcc.n   8a <__udelay+0x8a>
>>   9e:   e8bd 8cf0   ldmia.w sp!, {r4, r5, r6, r7, sl, fp, pc}
>>   a2:   bf00nop
>>
>> 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 
>> Reviewed-by: Fabio Estevam 
> 
> Hi Stefano,
> 
> Do you think this one could be applied for the upcoming release?
> 


Applied as fix as already discussed in other thread, thanks !

Best regards,
Stefano

-- 
=
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2018-03-09 Thread Fabio Estevam

[Adding Stefano]

On Thu, Mar 8, 2018 at 1:21 AM, Yasushi SHOJI  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:   428bcmp r3, r1
>   8a:   f8ce 20a4   str.w   r2, [lr, #164]  ; 0xa4
>   8e:   bf08it  eq
>   90:   4282cmpeq   r2, r0
>   92:   f8ce 30a0   str.w   r3, [lr, #160]  ; 0xa0
>   96:   d3f7bcc.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   mrrc15, 0, r2, r3, cr14
>   8e:   42abcmp r3, r5
>   90:   f8c1 20a4   str.w   r2, [r1, #164]  ; 0xa4
>   94:   bf08it  eq
>   96:   42a2cmpeq   r2, r4
>   98:   f8c1 30a0   str.w   r3, [r1, #160]  ; 0xa0
>   9c:   d3f5bcc.n   8a <__udelay+0x8a>
>   9e:   e8bd 8cf0   ldmia.w sp!, {r4, r5, r6, r7, sl, fp, pc}
>   a2:   bf00nop
>
> 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 
> Reviewed-by: Fabio Estevam 

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, >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 & 0x);
> 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

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

2018-03-07 Thread Yasushi SHOJI

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:   428bcmp r3, r1
  8a:   f8ce 20a4   str.w   r2, [lr, #164]  ; 0xa4
  8e:   bf08it  eq
  90:   4282cmpeq   r2, r0
  92:   f8ce 30a0   str.w   r3, [lr, #160]  ; 0xa0
  96:   d3f7bcc.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   mrrc15, 0, r2, r3, cr14
  8e:   42abcmp r3, r5
  90:   f8c1 20a4   str.w   r2, [r1, #164]  ; 0xa4
  94:   bf08it  eq
  96:   42a2cmpeq   r2, r4
  98:   f8c1 30a0   str.w   r3, [r1, #160]  ; 0xa0
  9c:   d3f5bcc.n   8a <__udelay+0x8a>
  9e:   e8bd 8cf0   ldmia.w sp!, {r4, r5, r6, r7, sl, fp, pc}
  a2:   bf00nop

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 
Reviewed-by: Fabio Estevam 
---
 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, >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 & 0x);
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

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information