On 01/31/2018 04:13 AM, Geert Uytterhoeven wrote:
Hi Fabrizio,
On Wed, Jan 31, 2018 at 11:47 AM, Fabrizio Castro
<fabrizio.castro@xxxxxxxxxxxxxx> wrote:
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:
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 <fabrizio.castro@xxxxxxxxxxxxxx>
Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@xxxxxxxxxxxxxx>
---
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.
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.
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.
Guenter
The only way to reboot iWave's boards iwg20d and iwg22d is to use the internal watchdog,
that's why the restart implementation was merged into this patch.
I can split this patch in two, one for PM support and one for restart support for the next
version, what do you think about this?
Splitting in individual logical changes sounds like a good idea to me.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
