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

Jean-Jacques Hiblot
2016-10-13T15:04:06Z

2016-10-13 15:47 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> 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@free-electrons.com>:
>> > 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