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@xxxxxxxxxxxxxx>
> > ---
> > 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
> >
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
