lkml.org 
[lkml]   [2010]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
From
SubjectRe: [PATCH 2/2] AT91: pm: make sure that r0 is 0 when dealing with cache operations
Hi Russell,

Le 22/10/2010 18:42, Russell King - ARM Linux :
> On Fri, Oct 22, 2010 at 07:29:00PM +0200, Nicolas Ferre wrote:
>> When using CP15 cache operations (c7), we make sure that Rd (r0)
>> is actually 0 as ARM 926 TRM is saying.
>
> Err, no. From the GCC manual:
>
> | Note that even a volatile `asm' instruction can be moved in ways
> | that appear insignificant to the compiler, such as across jump
> | instructions. You can't expect a sequence of volatile `asm'
> | instructions to remain perfectly consecutive. If you want consecutive
> | output, use a single `asm'. Also, GCC will perform some optimizations
> | across a volatile `asm' instruction; GCC does not "forget everything"
> | when it encounters a volatile `asm' instruction the way some other
> | compilers do.
>
> So:
>
> asm volatile("foo");
> asm volatile("bar");
>
> is not guaranteed to produce the following uninterrupted code sequence:
>
> foo
> bar
>
> The compiler is free to insert other instructions inbetween these two
> asm statements.
>
> It's also not permitted to modify registers without telling gcc that
> they've been modified.
>
>> + asm("mov r0, #0"); /* clear r0 for CP15 accesses */
>> asm("b 1f; .align 5; 1:");
>> asm("mcr p15, 0, r0, c7, c10, 4"); /* drain write buffer */
>
> That means the above is completely unacceptable. It should be:
>
> asm("mov r0, #0;"
> "b 1f;"
> ".align 5;"
> "1: mcr p15, 0, r0, c7, c10, 4" : : : "r0");


Thanks a lot for your detailed explanation.

I modify my patch according to your comments:

--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -261,8 +261,13 @@ static int at91_pm_enter(suspend_state_t state)
* For ARM 926 based chips, this requirement is weaker
* as at91sam9 can access a RAM in self-refresh mode.
*/
- asm("b 1f; .align 5; 1:");
- asm("mcr p15, 0, r0, c7, c10, 4"); /* drain write buffer */
+ asm volatile ( "mov r0, #0\n\t"
+ "b 1f\n\t"
+ ".align 5\n\t"
+ "1: mcr p15, 0, r0, c7, c10, 4\n\t"
+ : /* no output */
+ : /* no input */
+ : "r0");
saved_lpr = sdram_selfrefresh_enable();
wait_for_interrupt_enable();
sdram_selfrefresh_disable(saved_lpr);

>> --- a/arch/arm/mach-at91/pm.h
>> +++ b/arch/arm/mach-at91/pm.h
>> @@ -21,7 +21,7 @@ static inline u32 sdram_selfrefresh_enable(void)
>> }
>>
>> #define sdram_selfrefresh_disable(saved_lpr) at91_sys_write(AT91_SDRAMC_LPR, saved_lpr)
>> -#define wait_for_interrupt_enable() asm("mcr p15, 0, r0, c7, c0, 4")
>> +#define wait_for_interrupt_enable() asm("mcr p15, 0, r0, c7, c0, 4") /* r0 is 0 here */
>
> No, r0 is not guaranteed to be zero here. Use dsb() here instead which
> gets it right.

In fact it is Wait For Interrupt and not dsb... but anyway you are right: I modify it like this:

--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -21,7 +21,8 @@ static inline u32 sdram_selfrefresh_enable(void)
}

#define sdram_selfrefresh_disable(saved_lpr) at91_sys_write(AT91_SDRAMC_LPR, saved_lpr)
-#define wait_for_interrupt_enable() asm("mcr p15, 0, r0, c7, c0, 4")
+#define wait_for_interrupt_enable() asm volatile ("mcr p15, 0, %0, c7, c0, 4" \
+ : : "r" (0))

#elif defined(CONFIG_ARCH_AT91CAP9)
#include <mach/at91cap9_ddrsdr.h>
I repost this modified patch now...

Bye,
--
Nicolas Ferre



\
 
 \ /
  Last update: 2010-10-25 11:59    [W:0.106 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site