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

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

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@bp.renesas.com> 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@bp.renesas.com>
>>>> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
>>>> ---
>>>> 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@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
>