| Message ID | 20250129-da9052-v1-1-aa0dfc2ad595@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: da9052_wdt: respect TWDMIN | expand |
On 1/29/25 01:24, Marcus Folkesson wrote: > From: Marcus Folkesson <marcus.folkesson@combitech.com> > > We have to wait at least the minimium time for the watchdog window > (TWDMIN) before writings to the wdt register after the > watchdog is activated. > Otherwise the chip will assert TWD_ERROR and power down to reset mode. > This is missing a key explanation: Why does setting @min_hw_heartbeat_ms in struct watchdog_device not work for this driver ? Guenter > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> > --- > The current implementation does not respect the time window for all > cases not it handle when the jiffie wrap around. > E.g. setting a new timeout and immediately perform a 'ping' causes the > chip to assert TWD_ERROR. > --- > drivers/watchdog/da9052_wdt.c | 33 +++++++++++++++++++++++---------- > 1 file changed, 23 insertions(+), 10 deletions(-) > > diff --git a/drivers/watchdog/da9052_wdt.c b/drivers/watchdog/da9052_wdt.c > index 77039f2f0be54273df1666fe40c413b6c89285a1..5ca957a6c2219d99cb3e653205e3f556f7cfb56f 100644 > --- a/drivers/watchdog/da9052_wdt.c > +++ b/drivers/watchdog/da9052_wdt.c > @@ -45,6 +45,21 @@ static const struct { > { 7, 131 }, > }; > > +static void da9052_wdt_wait_for_twdmin(struct watchdog_device *wdt_dev) > +{ > + struct da9052_wdt_data *driver_data = watchdog_get_drvdata(wdt_dev); > + unsigned long msecs, twdmin, jnow = jiffies; > + > + /* > + * The host must wait at least TWDMIN ms between writings to the watchdog > + * register or the DA9052 will assert TWD_ERROR and power down to RESET mode. > + */ > + twdmin = driver_data->jpast + msecs_to_jiffies(DA9052_TWDMIN); > + if (time_before(jnow, twdmin)) { > + msecs = jiffies_to_msecs(twdmin - jnow); > + mdelay(msecs); > + } > +} > > static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev, > unsigned int timeout) > @@ -53,6 +68,7 @@ static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev, > struct da9052 *da9052 = driver_data->da9052; > int ret, i; > > + da9052_wdt_wait_for_twdmin(wdt_dev); > /* > * Disable the Watchdog timer before setting > * new time out. > @@ -89,8 +105,8 @@ static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev, > } > > wdt_dev->timeout = timeout; > - driver_data->jpast = jiffies; > } > + driver_data->jpast = jiffies; > > return 0; > } > @@ -109,16 +125,9 @@ static int da9052_wdt_ping(struct watchdog_device *wdt_dev) > { > struct da9052_wdt_data *driver_data = watchdog_get_drvdata(wdt_dev); > struct da9052 *da9052 = driver_data->da9052; > - unsigned long msec, jnow = jiffies; > int ret; > > - /* > - * We have a minimum time for watchdog window called TWDMIN. A write > - * to the watchdog before this elapsed time should cause an error. > - */ > - msec = (jnow - driver_data->jpast) * 1000/HZ; > - if (msec < DA9052_TWDMIN) > - mdelay(msec); > + da9052_wdt_wait_for_twdmin(wdt_dev); > > /* Reset the watchdog timer */ > ret = da9052_reg_update(da9052, DA9052_CONTROL_D_REG, > @@ -130,8 +139,11 @@ static int da9052_wdt_ping(struct watchdog_device *wdt_dev) > * FIXME: Reset the watchdog core, in general PMIC > * is supposed to do this > */ > - return da9052_reg_update(da9052, DA9052_CONTROL_D_REG, > + ret = da9052_reg_update(da9052, DA9052_CONTROL_D_REG, > DA9052_CONTROLD_WATCHDOG, 0 << 7); > + > + driver_data->jpast = jiffies; > + return ret; > } > > static const struct watchdog_info da9052_wdt_info = { > @@ -187,6 +199,7 @@ static int da9052_wdt_probe(struct platform_device *pdev) > return ret; > } > > + driver_data->jpast = jiffies; > return devm_watchdog_register_device(dev, &driver_data->wdt); > } > > > --- > base-commit: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5 > change-id: 20250129-da9052-661dfbb7de38 > > Best regards,
Hello, On Wed, Jan 29, 2025 at 06:48:04AM -0800, Guenter Roeck wrote: > On 1/29/25 01:24, Marcus Folkesson wrote: > > From: Marcus Folkesson <marcus.folkesson@combitech.com> > > > > We have to wait at least the minimium time for the watchdog window > > (TWDMIN) before writings to the wdt register after the > > watchdog is activated. > > Otherwise the chip will assert TWD_ERROR and power down to reset mode. > > > > This is missing a key explanation: > > Why does setting @min_hw_heartbeat_ms in struct watchdog_device > not work for this driver ? Thank you Guenter, I wasn't aware of min_hw_heartbeat_ms. I will see if it helps. I've also noticed that the same problem occur when, for example, I set the timeout with wdctl and .stop is called too soon as the application terminates. > > Guenter > Best regards, Marcus Folkesson
diff --git a/drivers/watchdog/da9052_wdt.c b/drivers/watchdog/da9052_wdt.c index 77039f2f0be54273df1666fe40c413b6c89285a1..5ca957a6c2219d99cb3e653205e3f556f7cfb56f 100644 --- a/drivers/watchdog/da9052_wdt.c +++ b/drivers/watchdog/da9052_wdt.c @@ -45,6 +45,21 @@ static const struct { { 7, 131 }, }; +static void da9052_wdt_wait_for_twdmin(struct watchdog_device *wdt_dev) +{ + struct da9052_wdt_data *driver_data = watchdog_get_drvdata(wdt_dev); + unsigned long msecs, twdmin, jnow = jiffies; + + /* + * The host must wait at least TWDMIN ms between writings to the watchdog + * register or the DA9052 will assert TWD_ERROR and power down to RESET mode. + */ + twdmin = driver_data->jpast + msecs_to_jiffies(DA9052_TWDMIN); + if (time_before(jnow, twdmin)) { + msecs = jiffies_to_msecs(twdmin - jnow); + mdelay(msecs); + } +} static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev, unsigned int timeout) @@ -53,6 +68,7 @@ static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev, struct da9052 *da9052 = driver_data->da9052; int ret, i; + da9052_wdt_wait_for_twdmin(wdt_dev); /* * Disable the Watchdog timer before setting * new time out. @@ -89,8 +105,8 @@ static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev, } wdt_dev->timeout = timeout; - driver_data->jpast = jiffies; } + driver_data->jpast = jiffies; return 0; } @@ -109,16 +125,9 @@ static int da9052_wdt_ping(struct watchdog_device *wdt_dev) { struct da9052_wdt_data *driver_data = watchdog_get_drvdata(wdt_dev); struct da9052 *da9052 = driver_data->da9052; - unsigned long msec, jnow = jiffies; int ret; - /* - * We have a minimum time for watchdog window called TWDMIN. A write - * to the watchdog before this elapsed time should cause an error. - */ - msec = (jnow - driver_data->jpast) * 1000/HZ; - if (msec < DA9052_TWDMIN) - mdelay(msec); + da9052_wdt_wait_for_twdmin(wdt_dev); /* Reset the watchdog timer */ ret = da9052_reg_update(da9052, DA9052_CONTROL_D_REG, @@ -130,8 +139,11 @@ static int da9052_wdt_ping(struct watchdog_device *wdt_dev) * FIXME: Reset the watchdog core, in general PMIC * is supposed to do this */ - return da9052_reg_update(da9052, DA9052_CONTROL_D_REG, + ret = da9052_reg_update(da9052, DA9052_CONTROL_D_REG, DA9052_CONTROLD_WATCHDOG, 0 << 7); + + driver_data->jpast = jiffies; + return ret; } static const struct watchdog_info da9052_wdt_info = { @@ -187,6 +199,7 @@ static int da9052_wdt_probe(struct platform_device *pdev) return ret; } + driver_data->jpast = jiffies; return devm_watchdog_register_device(dev, &driver_data->wdt); }