* [PATCH v5] watchdog: da9063: Fix setting/changing timeout
@ 2018-05-18 16:38 Marco Felsch
2018-05-18 17:11 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Marco Felsch @ 2018-05-18 16:38 UTC (permalink / raw)
To: support.opensource, wim, linux; +Cc: linux-watchdog, kernel
If the timeout value is set more than once the DA9063 watchdog triggers
a reset signal which reset the system.
The DA9063 watchdog timeout register field TWDSCALE combines two functions:
setting the timeout value scale and enabling the watchdog. If the
watchdog is enabled we have to disable the watchdog, wait some time due
to a HW issue and set the new timeout value. Setting the new timeout
value reenables the watchdog.
We have to buffer the timeout value because the DA9063 can't set a
timeout without enabling the watchdog (as described above). The buffered
value will then set by the next wdt_start() call.
The patch is based on Philipp Zabel's previous patch.
Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
drivers/watchdog/da9063_wdt.c | 54 ++++++++++++++++++++++++++++++-----
1 file changed, 47 insertions(+), 7 deletions(-)
diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index b17ac1bb1f28..1e2ecb76dcb1 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -45,10 +45,51 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
return DA9063_TWDSCALE_MAX;
}
-static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
+/*
+ * Writing a '1' to the self-clearing WATCHDOG bit resets the watchdog counter
+ * value.
+ */
+static int _da9063_wdt_reset_timer(struct da9063 *da9063)
+{
+ return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
+ DA9063_WATCHDOG);
+}
+
+static int da9063_wdt_stop(struct watchdog_device *wdd);
+static int
+_da9063_wdt_set_timeout(struct watchdog_device *wdd, unsigned int regval)
{
- return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
- DA9063_TWDSCALE_MASK, regval);
+ struct da9063 *da9063 = watchdog_get_drvdata(wdd);
+ int ret = 0;
+
+ /*
+ * The watchdog triggers a reboot if a timeout value is already
+ * programmed because the timeout value combines two functions
+ * in one: indicating the counter limit and starting the watchdog.
+ * The watchdog must be disabled to be able to change the timeout
+ * value if the watchdog is already running. Then we can set the
+ * new timeout value which enables the watchdog again.
+ *
+ * We have to cache the timeout value since we can't update the value
+ * without enabling the watchdog. This case may happen if the watchdog
+ * is disabled and we only want to update the timeout value.
+ */
+
+ if (watchdog_hw_running(wdd)) {
+ /* Don't try to update the value if disabling fails */
+ ret = da9063_wdt_stop(wdd);
+ if (ret)
+ goto out;
+
+ usleep_range(150, 300);
+
+ ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
+ DA9063_TWDSCALE_MASK, regval);
+ }
+
+ wdd->timeout = regval;
+out:
+ return ret;
}
static int da9063_wdt_start(struct watchdog_device *wdd)
@@ -58,7 +99,7 @@ static int da9063_wdt_start(struct watchdog_device *wdd)
int ret;
selector = da9063_wdt_timeout_to_sel(wdd->timeout);
- ret = _da9063_wdt_set_timeout(da9063, selector);
+ ret = _da9063_wdt_set_timeout(wdd, selector);
if (ret)
dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n",
ret);
@@ -85,8 +126,7 @@ static int da9063_wdt_ping(struct watchdog_device *wdd)
struct da9063 *da9063 = watchdog_get_drvdata(wdd);
int ret;
- ret = regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
- DA9063_WATCHDOG);
+ ret = _da9063_wdt_reset_timer(da9063);
if (ret)
dev_alert(da9063->dev, "Failed to ping the watchdog (err = %d)\n",
ret);
@@ -102,7 +142,7 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
int ret;
selector = da9063_wdt_timeout_to_sel(timeout);
- ret = _da9063_wdt_set_timeout(da9063, selector);
+ ret = _da9063_wdt_set_timeout(wdd, selector);
if (ret)
dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
ret);
--
2.17.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v5] watchdog: da9063: Fix setting/changing timeout 2018-05-18 16:38 [PATCH v5] watchdog: da9063: Fix setting/changing timeout Marco Felsch @ 2018-05-18 17:11 ` Guenter Roeck 2018-05-22 8:18 ` Marco Felsch 0 siblings, 1 reply; 4+ messages in thread From: Guenter Roeck @ 2018-05-18 17:11 UTC (permalink / raw) To: Marco Felsch; +Cc: support.opensource, wim, linux-watchdog, kernel On Fri, May 18, 2018 at 06:38:50PM +0200, Marco Felsch wrote: > If the timeout value is set more than once the DA9063 watchdog triggers > a reset signal which reset the system. > > The DA9063 watchdog timeout register field TWDSCALE combines two functions: > setting the timeout value scale and enabling the watchdog. If the > watchdog is enabled we have to disable the watchdog, wait some time due > to a HW issue and set the new timeout value. Setting the new timeout > value reenables the watchdog. > > We have to buffer the timeout value because the DA9063 can't set a > timeout without enabling the watchdog (as described above). The buffered > value will then set by the next wdt_start() call. > > The patch is based on Philipp Zabel's previous patch. > > Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.") > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/watchdog/da9063_wdt.c | 54 ++++++++++++++++++++++++++++++----- > 1 file changed, 47 insertions(+), 7 deletions(-) > > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c > index b17ac1bb1f28..1e2ecb76dcb1 100644 > --- a/drivers/watchdog/da9063_wdt.c > +++ b/drivers/watchdog/da9063_wdt.c > @@ -45,10 +45,51 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs) > return DA9063_TWDSCALE_MAX; > } > > -static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval) > +/* > + * Writing a '1' to the self-clearing WATCHDOG bit resets the watchdog counter > + * value. > + */ > +static int _da9063_wdt_reset_timer(struct da9063 *da9063) > +{ > + return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, > + DA9063_WATCHDOG); > +} > + > +static int da9063_wdt_stop(struct watchdog_device *wdd); > +static int > +_da9063_wdt_set_timeout(struct watchdog_device *wdd, unsigned int regval) > { > - return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, > - DA9063_TWDSCALE_MASK, regval); > + struct da9063 *da9063 = watchdog_get_drvdata(wdd); > + int ret = 0; > + > + /* > + * The watchdog triggers a reboot if a timeout value is already > + * programmed because the timeout value combines two functions > + * in one: indicating the counter limit and starting the watchdog. > + * The watchdog must be disabled to be able to change the timeout > + * value if the watchdog is already running. Then we can set the > + * new timeout value which enables the watchdog again. > + * > + * We have to cache the timeout value since we can't update the value > + * without enabling the watchdog. This case may happen if the watchdog > + * is disabled and we only want to update the timeout value. > + */ > + > + if (watchdog_hw_running(wdd)) { > + /* Don't try to update the value if disabling fails */ > + ret = da9063_wdt_stop(wdd); > + if (ret) > + goto out; > + > + usleep_range(150, 300); > + > + ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, > + DA9063_TWDSCALE_MASK, regval); > + } > + > + wdd->timeout = regval; Sorry, this is public (ie reported back to userspace) and has to be in seconds, and is already set in da9063_wdt_set_timeout(). If you want to cache the to-be-written register value, you will indeed have to do that locally. Guenter > +out: > + return ret; > } > > static int da9063_wdt_start(struct watchdog_device *wdd) > @@ -58,7 +99,7 @@ static int da9063_wdt_start(struct watchdog_device *wdd) > int ret; > > selector = da9063_wdt_timeout_to_sel(wdd->timeout); > - ret = _da9063_wdt_set_timeout(da9063, selector); > + ret = _da9063_wdt_set_timeout(wdd, selector); > if (ret) > dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n", > ret); > @@ -85,8 +126,7 @@ static int da9063_wdt_ping(struct watchdog_device *wdd) > struct da9063 *da9063 = watchdog_get_drvdata(wdd); > int ret; > > - ret = regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, > - DA9063_WATCHDOG); > + ret = _da9063_wdt_reset_timer(da9063); > if (ret) > dev_alert(da9063->dev, "Failed to ping the watchdog (err = %d)\n", > ret); > @@ -102,7 +142,7 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd, > int ret; > > selector = da9063_wdt_timeout_to_sel(timeout); > - ret = _da9063_wdt_set_timeout(da9063, selector); > + ret = _da9063_wdt_set_timeout(wdd, selector); > if (ret) > dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n", > ret); > -- > 2.17.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] watchdog: da9063: Fix setting/changing timeout 2018-05-18 17:11 ` Guenter Roeck @ 2018-05-22 8:18 ` Marco Felsch 2018-05-22 13:48 ` Guenter Roeck 0 siblings, 1 reply; 4+ messages in thread From: Marco Felsch @ 2018-05-22 8:18 UTC (permalink / raw) To: Guenter Roeck; +Cc: wim, support.opensource, kernel, linux-watchdog Hi Guenter, On 18-05-18 10:11, Guenter Roeck wrote: > On Fri, May 18, 2018 at 06:38:50PM +0200, Marco Felsch wrote: > > If the timeout value is set more than once the DA9063 watchdog triggers > > a reset signal which reset the system. > > > > The DA9063 watchdog timeout register field TWDSCALE combines two functions: > > setting the timeout value scale and enabling the watchdog. If the > > watchdog is enabled we have to disable the watchdog, wait some time due > > to a HW issue and set the new timeout value. Setting the new timeout > > value reenables the watchdog. > > > > We have to buffer the timeout value because the DA9063 can't set a > > timeout without enabling the watchdog (as described above). The buffered > > value will then set by the next wdt_start() call. > > > > The patch is based on Philipp Zabel's previous patch. > > > > Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.") > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/watchdog/da9063_wdt.c | 54 ++++++++++++++++++++++++++++++----- > > 1 file changed, 47 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c > > index b17ac1bb1f28..1e2ecb76dcb1 100644 > > --- a/drivers/watchdog/da9063_wdt.c > > +++ b/drivers/watchdog/da9063_wdt.c > > @@ -45,10 +45,51 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs) > > return DA9063_TWDSCALE_MAX; > > } > > > > -static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval) > > +/* > > + * Writing a '1' to the self-clearing WATCHDOG bit resets the watchdog counter > > + * value. > > + */ > > +static int _da9063_wdt_reset_timer(struct da9063 *da9063) > > +{ > > + return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, > > + DA9063_WATCHDOG); > > +} > > + > > +static int da9063_wdt_stop(struct watchdog_device *wdd); > > +static int > > +_da9063_wdt_set_timeout(struct watchdog_device *wdd, unsigned int regval) > > { > > - return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, > > - DA9063_TWDSCALE_MASK, regval); > > + struct da9063 *da9063 = watchdog_get_drvdata(wdd); > > + int ret = 0; > > + > > + /* > > + * The watchdog triggers a reboot if a timeout value is already > > + * programmed because the timeout value combines two functions > > + * in one: indicating the counter limit and starting the watchdog. > > + * The watchdog must be disabled to be able to change the timeout > > + * value if the watchdog is already running. Then we can set the > > + * new timeout value which enables the watchdog again. > > + * > > + * We have to cache the timeout value since we can't update the value > > + * without enabling the watchdog. This case may happen if the watchdog > > + * is disabled and we only want to update the timeout value. > > + */ > > + > > + if (watchdog_hw_running(wdd)) { > > + /* Don't try to update the value if disabling fails */ > > + ret = da9063_wdt_stop(wdd); > > + if (ret) > > + goto out; > > + > > + usleep_range(150, 300); > > + > > + ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, > > + DA9063_TWDSCALE_MASK, regval); > > + } > > + > > + wdd->timeout = regval; > > Sorry, this is public (ie reported back to userspace) and has to be in > seconds, and is already set in da9063_wdt_set_timeout(). If you want to > cache the to-be-written register value, you will indeed have to do that > locally. > > Guenter Yes I saw it to late. However, should I buffer the value a second time? In my v6 I made the following changes to avoid enabling the wdt during updating the timeout if the watchdog is off. da9063_wdt_set_timeout() { [...] ret = 0; selector = da9063_wdt_timeout_to_sel(timeout); if (watchdog_hw_running(wdd)) ret = da9063_wdt_update_timeout(); if (ret) dev_err(); else wdd->timeout = wdt_timeout[selector]; } Note: I renamed the _da9063_wdt_set_timeout() to da9063_wdt_update_timeout() in a separate patch. Anyway, this way we store/buffer the timeout value if the watchdog is off and can be set later by da9063_wdt_start(). If the watchdog is enabled we update the timeout immediately. Is that okay? Regards, Marco > > +out: > > + return ret; > > } > > > > static int da9063_wdt_start(struct watchdog_device *wdd) > > @@ -58,7 +99,7 @@ static int da9063_wdt_start(struct watchdog_device *wdd) > > int ret; > > > > selector = da9063_wdt_timeout_to_sel(wdd->timeout); > > - ret = _da9063_wdt_set_timeout(da9063, selector); > > + ret = _da9063_wdt_set_timeout(wdd, selector); > > if (ret) > > dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n", > > ret); > > @@ -85,8 +126,7 @@ static int da9063_wdt_ping(struct watchdog_device *wdd) > > struct da9063 *da9063 = watchdog_get_drvdata(wdd); > > int ret; > > > > - ret = regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, > > - DA9063_WATCHDOG); > > + ret = _da9063_wdt_reset_timer(da9063); > > if (ret) > > dev_alert(da9063->dev, "Failed to ping the watchdog (err = %d)\n", > > ret); > > @@ -102,7 +142,7 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd, > > int ret; > > > > selector = da9063_wdt_timeout_to_sel(timeout); > > - ret = _da9063_wdt_set_timeout(da9063, selector); > > + ret = _da9063_wdt_set_timeout(wdd, selector); > > if (ret) > > dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n", > > ret); > > -- > > 2.17.0 > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] watchdog: da9063: Fix setting/changing timeout 2018-05-22 8:18 ` Marco Felsch @ 2018-05-22 13:48 ` Guenter Roeck 0 siblings, 0 replies; 4+ messages in thread From: Guenter Roeck @ 2018-05-22 13:48 UTC (permalink / raw) To: Marco Felsch; +Cc: wim, support.opensource, kernel, linux-watchdog On 05/22/2018 01:18 AM, Marco Felsch wrote: > Hi Guenter, > > On 18-05-18 10:11, Guenter Roeck wrote: >> On Fri, May 18, 2018 at 06:38:50PM +0200, Marco Felsch wrote: >>> If the timeout value is set more than once the DA9063 watchdog triggers >>> a reset signal which reset the system. >>> >>> The DA9063 watchdog timeout register field TWDSCALE combines two functions: >>> setting the timeout value scale and enabling the watchdog. If the >>> watchdog is enabled we have to disable the watchdog, wait some time due >>> to a HW issue and set the new timeout value. Setting the new timeout >>> value reenables the watchdog. >>> >>> We have to buffer the timeout value because the DA9063 can't set a >>> timeout without enabling the watchdog (as described above). The buffered >>> value will then set by the next wdt_start() call. >>> >>> The patch is based on Philipp Zabel's previous patch. >>> >>> Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.") >>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> >>> --- >>> drivers/watchdog/da9063_wdt.c | 54 ++++++++++++++++++++++++++++++----- >>> 1 file changed, 47 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c >>> index b17ac1bb1f28..1e2ecb76dcb1 100644 >>> --- a/drivers/watchdog/da9063_wdt.c >>> +++ b/drivers/watchdog/da9063_wdt.c >>> @@ -45,10 +45,51 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs) >>> return DA9063_TWDSCALE_MAX; >>> } >>> >>> -static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval) >>> +/* >>> + * Writing a '1' to the self-clearing WATCHDOG bit resets the watchdog counter >>> + * value. >>> + */ >>> +static int _da9063_wdt_reset_timer(struct da9063 *da9063) >>> +{ >>> + return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, >>> + DA9063_WATCHDOG); >>> +} >>> + >>> +static int da9063_wdt_stop(struct watchdog_device *wdd); >>> +static int >>> +_da9063_wdt_set_timeout(struct watchdog_device *wdd, unsigned int regval) >>> { >>> - return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, >>> - DA9063_TWDSCALE_MASK, regval); >>> + struct da9063 *da9063 = watchdog_get_drvdata(wdd); >>> + int ret = 0; >>> + >>> + /* >>> + * The watchdog triggers a reboot if a timeout value is already >>> + * programmed because the timeout value combines two functions >>> + * in one: indicating the counter limit and starting the watchdog. >>> + * The watchdog must be disabled to be able to change the timeout >>> + * value if the watchdog is already running. Then we can set the >>> + * new timeout value which enables the watchdog again. >>> + * >>> + * We have to cache the timeout value since we can't update the value >>> + * without enabling the watchdog. This case may happen if the watchdog >>> + * is disabled and we only want to update the timeout value. >>> + */ >>> + >>> + if (watchdog_hw_running(wdd)) { >>> + /* Don't try to update the value if disabling fails */ >>> + ret = da9063_wdt_stop(wdd); >>> + if (ret) >>> + goto out; >>> + >>> + usleep_range(150, 300); >>> + >>> + ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, >>> + DA9063_TWDSCALE_MASK, regval); >>> + } >>> + >>> + wdd->timeout = regval; >> >> Sorry, this is public (ie reported back to userspace) and has to be in >> seconds, and is already set in da9063_wdt_set_timeout(). If you want to >> cache the to-be-written register value, you will indeed have to do that >> locally. >> >> Guenter > > Yes I saw it to late. However, should I buffer the value a second time? > In my v6 I made the following changes to avoid enabling the wdt during > updating the timeout if the watchdog is off. > I did not say you _should_ cache it twice; if anything, you would cache the register value. > da9063_wdt_set_timeout() { > [...] > ret = 0; > > selector = da9063_wdt_timeout_to_sel(timeout); > > if (watchdog_hw_running(wdd)) > ret = da9063_wdt_update_timeout(); > > if (ret) > dev_err(); > else > wdd->timeout = wdt_timeout[selector]; I would use this approach, though I would drop the dev_err() since 1) I am not a friend of noisy kernel drivers and 2) the error is reported to user space. In general, problems like this tend to be persistent, and this is one of the kernel drivers which will fill the log with noise if/when that happens. Guenter > } > > Note: I renamed the _da9063_wdt_set_timeout() to > da9063_wdt_update_timeout() in a separate patch. Anyway, this way we > store/buffer the timeout value if the watchdog is off and can be set > later by da9063_wdt_start(). If the watchdog is enabled we update the > timeout immediately. > > Is that okay? > > Regards, > Marco > >>> +out: >>> + return ret; >>> } >>> >>> static int da9063_wdt_start(struct watchdog_device *wdd) >>> @@ -58,7 +99,7 @@ static int da9063_wdt_start(struct watchdog_device *wdd) >>> int ret; >>> >>> selector = da9063_wdt_timeout_to_sel(wdd->timeout); >>> - ret = _da9063_wdt_set_timeout(da9063, selector); >>> + ret = _da9063_wdt_set_timeout(wdd, selector); >>> if (ret) >>> dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n", >>> ret); >>> @@ -85,8 +126,7 @@ static int da9063_wdt_ping(struct watchdog_device *wdd) >>> struct da9063 *da9063 = watchdog_get_drvdata(wdd); >>> int ret; >>> >>> - ret = regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, >>> - DA9063_WATCHDOG); >>> + ret = _da9063_wdt_reset_timer(da9063); >>> if (ret) >>> dev_alert(da9063->dev, "Failed to ping the watchdog (err = %d)\n", >>> ret); >>> @@ -102,7 +142,7 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd, >>> int ret; >>> >>> selector = da9063_wdt_timeout_to_sel(timeout); >>> - ret = _da9063_wdt_set_timeout(da9063, selector); >>> + ret = _da9063_wdt_set_timeout(wdd, selector); >>> if (ret) >>> dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n", >>> ret); >>> -- >>> 2.17.0 >>> >> >> > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-05-22 13:48 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-18 16:38 [PATCH v5] watchdog: da9063: Fix setting/changing timeout Marco Felsch 2018-05-18 17:11 ` Guenter Roeck 2018-05-22 8:18 ` Marco Felsch 2018-05-22 13:48 ` Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.