Re: [PATCH v2 1/3] power: reset: Add Renesas reset driver

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Feb 16, 2017 at 06:40:05PM +0000, Chris Brandt wrote:
> On Thursday, February 16, 2017, Guenter Roeck wrote:
> > On Thu, Feb 16, 2017 at 12:23:18PM -0500, Chris Brandt wrote:
> > > Some Renesas SoCs do not have a reset register and the only way to do a SW
> > > controlled reset is to use the watchdog timer. Additionally, since all the
> > > WDT timeout options are so quick, a system reset is about the only thing
> > > it's good for.
> > >
> > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Reviewed-by: Geert
> > > Uytterhoeven <geert+renesas@glider.be>
> > 
> > Wondering - why not use the watchdog driver and the infrastructure provided
> > by the watchdog core (ie the restart callback) instead ?
> 
> That was Geert's first suggestion, but then he said:
> 
> On Friday, February 10, 2017, Geert Uytterhoeven wrote:
> > On Fri, Feb 10, 2017 at 3:59 PM, Chris Brandt <Chris.Brandt@renesas.com>
> > wrote:
> > > On Friday, February 10, 2017, Geert Uytterhoeven wrote:
> > >> >  static const char *const r7s72100_boards_compat_dt[] __initconst = {
> > >> >  "renesas,r7s72100", NULL, @@ -29,4 +58,5 @@
> > >> >  DT_MACHINE_START(R7S72100_DT, "Generic R7S72100
> > >> (Flattened Device Tree)")
> > >> >         .init_early     = shmobile_init_delay, .init_late      =
> > >> >         shmobile_init_late, .dt_compat      =
> > >> >         r7s72100_boards_compat_dt, +       .restart        =
> > >> >         r7s72100_restart, MACHINE_END
> > >>
> > >> Perhaps unsurprisingly, I'd recommend writing a watchdog driver instead.
> > >> drivers/watchdog/renesas_wdt.c (currently supporting R-Car Gen3 only) may
> > >> serve as an example.
> > >
> > > Why do you guys always make things more difficult for me... ;)
> > >
> > > To be clear, you are recommending I make a WDT timer driver (and not use
> > > .restart) and that will reset the system?
> > >
> > > Or, make a WDT driver just so I can steal the base address?
> > 
> > I meant a WDT timer driver. If the watchdog driver provides a .restart
> > callback, it will be used for system reset (hmm, renesas_wdt.c no longer has
> > the .restart callback, as per arm64 "system reset must be implemented using
> > PSCI"-policy).
> 

Hmm, ok. Guess I don't have to understand that you can not use the watchdog
driver because of the above, but implementing exactly the same functionality
in a separate driver is ok.

[ I am sure I am missing something here, so just ignore what I am saying ]

> 
> 
> > > v2: * changed DT bindings from 'wdt-reset' to 'rza-wdt' * changed hard
> > > coded register values to defines * added msleep to while(1) loop
> > 
> > Sure you can sleep here ?
> 
> As per Geert's review:
> 
> On Wednesday, February 15, 2017, Geert Uytterhoeven wrote:
> > > + +       /* Wait for WDT overflow */ +       while (1) +               ;
> > 
> > This burns CPU, and thus power, albeit for a very short time.  Perhaps add
> > an msleep() to the loop body?
> 
> Note that you only have 7.7us before the restart occurs, so probably not much
> sleeping will be going on.
> 
Let me rephrase my question. Is it guaranteed that you _can_ sleep in this
function, or in other words that it is guaranteed that interrupts are enabled ?

Thanks,
Guenter