Re: [PATCH 5/8] power: reset: at91-reset: use at91_ramc_shutdown

Alexandre Belloni
2014-10-22T07:18:50Z

On 22/10/2014 at 09:08:10 +0200, Thomas Petazzoni wrote :
> Dear Alexandre Belloni,
>
> On Tue, 21 Oct 2014 23:55:37 +0200, Alexandre Belloni wrote:
>
> > /*
> > * unless the SDRAM is cleanly shutdown before we hit the
> > * reset register it can be left driving the data bus and
> > * killing the chance of a subsequent boot from NAND
> > */
> > -static void at91sam9260_restart(enum reboot_mode mode, const char *cmd)
> > +static void at91_restart(enum reboot_mode mode, const char *cmd)
> > {
> > - asm volatile(
> > - /* Align to cache lines */
> > - ".balign 32\n\t"
> > -
> > - /* Disable SDRAM accesses */
> > - "str %2, [%0, #" __stringify(AT91_SDRAMC_TR) "]\n\t"
> > -
> > - /* Power down SDRAM */
> > - "str %3, [%0, #" __stringify(AT91_SDRAMC_LPR) "]\n\t"
> > + if (at91_ramc_shutdown)
> > + at91_ramc_shutdown();
> >
> > + asm volatile(
> > /* Reset CPU */
> > - "str %4, [%1, #" __stringify(AT91_RSTC_CR) "]\n\t"
> > + "str %1, [%0, #" __stringify(AT91_RSTC_CR) "]\n\t"
> >
> > "b .\n\t"
>
> Are you sure this is working properly? There was a reason to have the
> SDRAM controller shutdown right before resetting the CPU: the code was
> ensuring that all those assembly instructions fitted in one cache line,
> so that even if the SDRAM controller gets shutdown, the rest of the
> code can properly execute until resetting the CPU. Now, the SDRAM
> controller shutdown code and the code resetting the CPU are in
> completely separate places, which break this assumption.
>

It it still working properly, I believe it is still fitting the cache
line.

> And also, you forgot to Cc: Maxime Ripard who did the initial work on
> this at91-reset controller.
>

Yeah, I realized after sending the patches.

--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering