Re: [PATCH v5 3/4] wdt: Support wdt on ROHM BD9576MUF and BD9573MUF
On Wed, Nov 11, 2020 at 03:15:17PM +, Vaittinen, Matti wrote: [ ... ] > > > > + > > > > + priv->wdd.info = _wdt_ident; > > > > + priv->wdd.ops = _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
Re: [PATCH v5 3/4] wdt: Support wdt on ROHM BD9576MUF and BD9573MUF
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 > > > > > > Reviewed-by: Guenter Roeck > > > --- > > > > > > > //snip > > > > > + ret = of_property_read_variable_u32_array(np, "rohm,hw-timeout- > > > ms", > > > + _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(>wdd, priv); > > > + > > > + priv->wdd.info = _wdt_ident; > > > + priv->wdd.ops = _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
Re: [PATCH v5 3/4] wdt: Support wdt on ROHM BD9576MUF and BD9573MUF
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 >> Reviewed-by: Guenter Roeck >> --- >> > > //snip > >> +ret = of_property_read_variable_u32_array(np, "rohm,hw-timeout- >> ms", >> + _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(>wdd, priv); >> + >> +priv->wdd.info = _wdt_ident; >> +priv->wdd.ops = _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) >
Re: [PATCH v5 3/4] wdt: Support wdt on ROHM BD9576MUF and BD9573MUF
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 > Reviewed-by: Guenter Roeck > --- > //snip > + ret = of_property_read_variable_u32_array(np, "rohm,hw-timeout- > ms", > + _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(>wdd, priv); > + > + priv->wdd.info = _wdt_ident; > + priv->wdd.ops = _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)
[PATCH v5 3/4] wdt: Support wdt on ROHM BD9576MUF and BD9573MUF
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 Reviewed-by: Guenter Roeck --- drivers/watchdog/Kconfig | 13 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/bd9576_wdt.c | 290 ++ 3 files changed, 304 insertions(+) create mode 100644 drivers/watchdog/bd9576_wdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index fd7968635e6d..d5fec1bdf23e 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -172,6 +172,19 @@ config BD70528_WATCHDOG Alternatively say M to compile the driver as a module, which will be called bd70528_wdt. +config BD957XMUF_WATCHDOG + tristate "ROHM BD9576MUF and BD9573MUF PMIC Watchdog" + depends on MFD_ROHM_BD957XMUF + select WATCHDOG_CORE + help + Support for the watchdog in the ROHM BD9576 and BD9573 PMICs. + These PMIC ICs contain watchdog block which can be configured + to toggle reset line if SoC fails to ping watchdog via GPIO. + + Say Y here to include support for the ROHM BD9576 or BD9573 + watchdog. Alternatively say M to compile the driver as a module, + which will be called bd9576_wdt. + config DA9052_WATCHDOG tristate "Dialog DA9052 Watchdog" depends on PMIC_DA9052 || COMPILE_TEST diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 071a2e50be98..54f39883f3ac 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -209,6 +209,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o # Architecture Independent obj-$(CONFIG_BD70528_WATCHDOG) += bd70528_wdt.o +obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o diff --git a/drivers/watchdog/bd9576_wdt.c b/drivers/watchdog/bd9576_wdt.c new file mode 100644 index ..afef19ce01be --- /dev/null +++ b/drivers/watchdog/bd9576_wdt.c @@ -0,0 +1,290 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 ROHM Semiconductors + * + * ROHM BD9576MUF and BD9573MUF Watchdog driver + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +static bool nowayout; +module_param(nowayout, bool, 0); +MODULE_PARM_DESC(nowayout, + "Watchdog cannot be stopped once started (default=\"false\")"); + +#define HW_MARGIN_MIN 2 +#define HW_MARGIN_MAX 4416 +#define BD957X_WDT_DEFAULT_MARGIN 4416 + +struct bd9576_wdt_priv { + struct gpio_desc*gpiod_ping; + struct gpio_desc*gpiod_en; + struct device *dev; + struct regmap *regmap; + boolalways_running; + struct watchdog_device wdd; +}; + +static void bd9576_wdt_disable(struct bd9576_wdt_priv *priv) +{ + gpiod_set_value_cansleep(priv->gpiod_en, 0); +} + +static int bd9576_wdt_ping(struct watchdog_device *wdd) +{ + struct bd9576_wdt_priv *priv = watchdog_get_drvdata(wdd); + + /* Pulse */ + gpiod_set_value_cansleep(priv->gpiod_ping, 1); + gpiod_set_value_cansleep(priv->gpiod_ping, 0); + + return 0; +} + +static int bd9576_wdt_start(struct watchdog_device *wdd) +{ + struct bd9576_wdt_priv *priv = watchdog_get_drvdata(wdd); + + gpiod_set_value_cansleep(priv->gpiod_en, 1); + + return bd9576_wdt_ping(wdd); +} + +static int bd9576_wdt_stop(struct watchdog_device *wdd) +{ + struct bd9576_wdt_priv *priv = watchdog_get_drvdata(wdd); + + if (!priv->always_running) + bd9576_wdt_disable(priv); + else + set_bit(WDOG_HW_RUNNING, >status); + + return 0; +} + +static const struct watchdog_info bd957x_wdt_ident = { + .options= WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | + WDIOF_SETTIMEOUT, + .identity = "BD957x Watchdog", +}; + +static const struct watchdog_ops bd957x_wdt_ops = { + .owner = THIS_MODULE, + .start = bd9576_wdt_start, + .stop = bd9576_wdt_stop, + .ping = bd9576_wdt_ping, +}; + +/* Unit is hundreds of uS */ +#define FASTNG_MIN 23 + +static int find_closest_fast(int target, int *sel, int *val) +{ + int i; + int window = FASTNG_MIN; + + for (i = 0; i < 8 && window < target; i++) + window <<= 1; + + *val = window; + *sel = i; + + if (i == 8) + return -EINVAL; + + return 0; + +} + +static int find_closest_slow_by_fast(int fast_val, int