[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 11 Nov 2020 07:47:09 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
Cc: "mazziesaccount@...il.com" <mazziesaccount@...il.com>,
"wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
linux-power <linux-power@...rohmeurope.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"lee.jones@...aro.org" <lee.jones@...aro.org>,
"linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>
Subject: Re: [PATCH v5 3/4] wdt: Support wdt on ROHM BD9576MUF and BD9573MUF
On Wed, Nov 11, 2020 at 03:15:17PM +0000, Vaittinen, Matti wrote:
[ ... ]
> > > > +
> > > > + 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 :)
Yes, exactly, that is the idea. Various drivers used to implement this
within the driver, so it just made sense to pull the functionality into
the watchdog core.
Thanks,
Guenter
Powered by blists - more mailing lists