| Message ID | cover.1604574431.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| Headers | show |
| Series | Support ROHM BD9576MUF and BD9573MUF PMICs | expand |
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@fi.rohmeurope.com> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > //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. I need to do some testing when I get the HW at my hands - please don't apply this patch just yet. I will respin this after some testing - or if other patches are applied then I will just send this one alone. Sorry for the hassle... --Matti -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon)
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@fi.rohmeurope.com> >> Reviewed-by: Guenter Roeck <linux@roeck-us.net> >> --- >> > > //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. > I need to do some testing when I get the HW at my hands - please don't > apply this patch just yet. I will respin this after some testing - or > if other patches are applied then I will just send this one alone. > > Sorry for the hassle... > No worries. Thanks for noticing. Guenter > --Matti > > -- > Matti Vaittinen, Linux device drivers > ROHM Semiconductors, Finland SWDC > Kiviharjunlenkki 1E > 90220 OULU > FINLAND > > ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ > > Simon says - in Latin please. > "non cogito me" dixit Rene Descarte, deinde evanescavit > > (Thanks for the translation Simon) >
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@fi.rohmeurope.com > > > > > > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > > --- > > > > > > > //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
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