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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxx>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> 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).



> > 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.


Chris

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux