Re: [PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories

[Jean-Jacques Hiblot]

2016-10-13 15:47 GMT+02:00 Alexandre Belloni
<alexandre.belloni@xxxxxxxxxxxxxxxxxx>:
> On 13/10/2016 at 14:27:15 +0200, Jean-Jacques Hiblot wrote :
>> 2016-10-13 13:03 GMT+02:00 Alexandre Belloni
>> <alexandre.belloni@xxxxxxxxxxxxxxxxxx>:
>> > On 12/10/2016 at 14:48:27 +0200, Jean-Jacques Hiblot wrote :
>> >> > +static void at91_lpddr_poweroff(void)
>> >> > +{
>> >> > +       asm volatile(
>> >> > +               /* Align to cache lines */
>> >> > +               ".balign 32\n\t"
>> >> > +
>> >> > +               "       ldr     r6, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
>> >> At first sight, it looks useless. I assume it's used to preload the
>> >> TLB before the LPDDR is turned off.
>> >> A comment to explain why this line is useful would prevent its removal.
>> >
>> > Yes, this is the case. I can add a comment.
>> >
>> > Anyway, I would prefer the whole thing to run from SRAM, as a PIE
>> > instead of relying on the cache.
>>
>> Instead of copying into the SRAM, you can make the cache reliable by
>> preloading it, much like the TLB.
>> LDI is probably not available for most of atmel's SOC, so the only way
>> I can think of, is to execute code from the targeted area. here is an
>> example:
>> +               /*
>> +                * Jump to the end of the sequence to preload instruction cache
>> +                * It only works because the sequence is short enough not to
>> +                * sit accross more than 2 cache lines
>> +                */
>> +               "       b end_of_sequence\n\t"
>> +               "start_of_sequence:\n\t"
>> +
>>                 /* Power down SDRAM0 */
>>                 "       str     %1, [%0, #"
>> __stringify(AT91_DDRSDRC_LPR) "]\n\t"
>>                 /* Shutdown CPU */
>>                 "       str     %3, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t"
>>
>>                 "       b       .\n\t"
>> +
>> +               /*
>> +                * we're now 100% sure that the code to shutdown the LPDDR and
>> +                * the CPU is in cache, go back to do the actual job
>> +                */
>> +               "end_of_sequence:\n\t"
>> +               "       b start_of_sequence\n\t"
>>                 :
>>
>
> I don't think this is necessary. By aligning the instructions properly,
> we are already sure the whole code is loaded into the cache.
right I didn't see the align directive.
>
> My plan is to get rid of the assembly and use PIE so it is written in C
> and we can properly separate the RAM stuff in the ddrc driver.
>
> The mpddrc driver could load its shutdown function in SRAM. The reset
> controller driver would load the reset function in SRAM and the shutdown
> controller would load the poweroff function in SRAM. It would e quite
> cleaner than what we have here.
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com