On Wed, 2020-11-11 at 06:41 -0800, Guenter Roeck wrote:
> On 11/11/20 6:01 AM, Vaittinen, Matti wrote:
> > On Thu, 2020-11-05 at 13:38 +0200, Matti Vaittinen wrote:
> > > Add Watchdog support for ROHM BD9576MUF and BD9573MUF PMICs which
> > > are
> > > mainly used to power the R-Car series processors. The watchdog is
> > > pinged using a GPIO and enabled using another GPIO. Additionally
> > > watchdog time-out can be configured to HW prior starting the
> > > watchdog.
> > > Watchdog timeout can be configured to detect only delayed ping or
> > > in
> > > a window mode where also too fast pings are detected.
> > >
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx
> > > >
> > > Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > > ---
> > >
> >
> > //snip
> >
> > > + ret = of_property_read_variable_u32_array(np, "rohm,hw-timeout-
> > > ms",
> > > + &hw_margin[0], 1, 2);
> > > + if (ret < 0 && ret != -EINVAL)
> > > + return ret;
> > > +
> > > + if (ret == 1)
> > > + hw_margin_max = hw_margin[0];
> > > +
> > > + if (ret == 2) {
> > > + hw_margin_max = hw_margin[1];
> > > + hw_margin_min = hw_margin[0];
> > > + }
> > > +
> > > + ret = bd957x_set_wdt_mode(priv, hw_margin_max, hw_margin_min);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + priv->always_running = of_property_read_bool(np, "always-
> > > running");
> > > +
> > > + watchdog_set_drvdata(&priv->wdd, priv);
> > > +
> > > + priv->wdd.info = &bd957x_wdt_ident;
> > > + priv->wdd.ops = &bd957x_wdt_ops;
> > > + priv->wdd.min_hw_heartbeat_ms = hw_margin_min;
> > > + priv->wdd.max_hw_heartbeat_ms = hw_margin_max;
> > > + priv->wdd.parent = dev;
> > > + priv->wdd.timeout = (hw_margin_max / 2) * 1000;
> >
> > Hmm. Just noticed this value does not make sense, right?
> > Maximum hw_margin is 4416 ms. If I read this correctly timeout
> > should
> > be in seconds - so result is around 2 000 000 seconds here. I
> > think it
> > is useless value...
> >
> > Perhaps
> > priv->wdd.timeout = (hw_margin_max / 2) / 1000;
> > if (!priv->wdd.timeout)
> > priv->wdd.timeout = 1;
> > would be more appropriate.
> >
>
> Yes. Good catch. Actually, since max_hw_heartbeat_ms is specified,
> it can and should be a reasonable constant (like the usual 30
> seconds).
> It does not and should not be bound by max_hw_heartbeat_ms.
Thanks for confirming this Guenter. I'd better admit I didn't
understand how the max_hw_heartbeat_ms works.
If I now read the code correctly, the "watchdog worker" takes care of
feeding for shorter periods than the "timeout" - and only stops
feeding max_hw_heartbeat_ms before timeout expires if userland has not
been feedin wdg. This is really cool approach for short(ish)
max_hw_heartbeat_ms configurations as user-space does not need to meet
"RT requirements". WDG framework is much more advanced that I knew :)
It's nice to learn!
--Matti
