[RFC v3 11/25] watchdog: renesas_wdt: Add R-Car Gen2 support

Hi G?nter,

On Wed, Jan 31, 2018 at 3:48 PM, Guenter Roeck [off-list ref] wrote:

On 01/31/2018 04:13 AM, Geert Uytterhoeven wrote:

quoted

On Wed, Jan 31, 2018 at 11:47 AM, Fabrizio Castro
[off-list ref] wrote:

quoted

quoted

Subject: Re: [RFC v3 11/25] watchdog: renesas_wdt: Add R-Car Gen2
support
On Tue, Jan 30, 2018 at 08:22:44PM +0000, Fabrizio Castro wrote:

quoted

On R-Car Gen2 and RZ/G1 the rwdt clock needs to be always ON, therefore
when suspending to RAM we need to explicitly disable the counting by
clearing TME from RWTCSRA.
Also, on some systems RWDT is the only piece of HW that allows the SoC
to be restarted, therefore this patch implements the restart callback.

Signed-off-by: Fabrizio Castro <redacted>
Signed-off-by: Ramesh Shanmugasundaram
[off-list ref]
---
Wolfram, was the restart callback implementation missing for a reason?
Is its implementation going to break any Gen3 platform?

The changes clearly are way more than claimed in the subject. Adding
restart handler and PM support may be prerequisites for Gen2, but the
changes apply to Gen3 as well. What happened to "one patch per logical
change" ?

The PM implementation should be ok for Gen3 too, without this patch
series the kernel
would disable the rwdt clock when suspending to RAM, this would "pause"
the counting.
This patch series declares the rwdt clock as critical for Gen2 and RZ/G1,
which means we
need to explicitly disable the counting to keep the same behaviour in
place.

Note that if the kernel crashes after rwdt_suspend(), the watchdog won't
restart the system. But that's an issue common to many watchdog driver,
right?

If so, that would be buggy.

All watchdog drivers implementing a system suspend handler stop the watchdog
before suspending the system. I guess if they wouldn't do that, the watchdog
would restart the system while asleep?

Exceptions are:
  - atlas7_wdt, sirfsoc_wdt: these seem to rely on another driver stopping
    the watchdog clock, so it behaves the same as all above,
  - diag288_wdt: this returns -EBUSY when trying to suspend the system
    while the watchdog is enabled, to put the burden of disabling the
    watchdog on the user,
  - ux500_wdt: this seems to use a hardware feature to automatically
    disable the watchdog during suspend.

So only the last one seems to protect against kernel crashes after the
watchdog's suspend method is called...

Note that two drivers (iTCO_wdt and wdat_wdt) implements suspend_noirq
instead of suspend, which decreases the window of running without a watchdog
a bit.

quoted

quoted

With respect to the restart callback implementation, that may well have
consequences on
Gen3, hopefully Wolfram will give us a feedback on this.
In particular I would like to know if we should be installing the restart
callback only when
we use "renesas,rcar-gen2-wdt" as compatible string, or it's ok to make
it available for
Gen3 as well.

IIRC, the reason we don't have the restart callback on R-Car Gen3 is the
arm64 maintainers insisting on using PSCI, and vetoing other means,
to restart the system.

You could just give it lower priority than PSCI.

quoted

Which leaves us with a few boards where we have to use the watchdog, and
wait until the timeout triggers...

Which means the veto is counter-productive and thus meaningless.

I do welcome that point of view ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds