Re: [PATCH v2 2/2] watchdog: renesas_wwdt: add driver

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

Hi Guenter,

thanks for the fast review!

> > + * The WWDT can only be setup once after boot. Because we cannot know if this
> > + * already happened in early boot stages, it is mandated that the firmware
> > + * configures the watchdog. Linux then adapts according to the given setup.
> 
> What if it didn't happen ? Is WDTA0OVF set to a reasonable default in that case ?

It will overflow pretty fast, but it will be in a working condition,
generally.

> 
> > + * Note that this watchdog only reports an overflow to the Error Control Module.
> 
> Kind of unusual. Why not panic in that case, and why have the watchdog in the first
> place ?

We have multiple WWDTs on the SoCs, namely as much as we have cores. The
idea is to utilize one per SoC. In normal configuration, the Error
Control Module (ECM) gets notified of overflows and will act
accordingly. It has more options than a reset, it can e.g. raise
dedicated pins to trigger actions. Sadly, we don't have a driver for ECM
yet upstream, but we need to start somewhere.

> > +	struct wwdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> Maybe use container_of() ?

Ok.

> > +static irqreturn_t wwdt_error_irq(int irq, void *dev_id)
> > +{
> > +	struct device *dev = dev_id;
> > +
> > +	dev_warn(dev, "Watchdog timed out\n");
> 
> So the pretimeout may trigger a reboot (panic) if the pretimeout
> governor is set to it, but the real watchdog just says Hi.
> Does that really make sense ?

It depends on how the ECM is configured. It can make sense. If someone
misconfigures, it might not make sense. But maybe we just don't use
pretimeout here because for Linux this is system-wide while the WWDT
usually has a smaller scale?

Does this make it more clear?

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]