| Message ID | 20200120091729.16256-1-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | watchdog: da9062: do not ping the hw during stop() | expand |
On Mon, Jan 20, 2020 at 10:17:29AM +0100, Marco Felsch wrote: > The da9062 hw has a minimum ping cool down phase of at least 200ms. The > driver takes that into account by setting the min_hw_heartbeat_ms to > 300ms and the core guarantees that the hw limit is observed for the > ping() calls. But the core can't guarantees the required minimum ping guarantee > cool down phase if a stop() command is send immediately after the ping() > command. So it is not allowed to ping the watchdog within the stop() > command as the driver do. Remove the ping can be done without doubts does > because the watchdog gets disabled anyway and a (re)start reset the resets > watchdog counter too. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/da9062_wdt.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c > index 77b6b5336067..0ad15d55071c 100644 > --- a/drivers/watchdog/da9062_wdt.c > +++ b/drivers/watchdog/da9062_wdt.c > @@ -97,13 +97,6 @@ static int da9062_wdt_stop(struct watchdog_device *wdd) > struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); > int ret; > > - ret = da9062_reset_watchdog_timer(wdt); > - if (ret) { > - dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n", > - ret); > - return ret; > - } > - > ret = regmap_update_bits(wdt->hw->regmap, > DA9062AA_CONTROL_D, > DA9062AA_TWDSCALE_MASK,
On 20 January 2020 09:17, Marco Felsch wrote: > The da9062 hw has a minimum ping cool down phase of at least 200ms. The > driver takes that into account by setting the min_hw_heartbeat_ms to > 300ms and the core guarantees that the hw limit is observed for the > ping() calls. But the core can't guarantees the required minimum ping > cool down phase if a stop() command is send immediately after the ping() > command. So it is not allowed to ping the watchdog within the stop() > command as the driver do. Remove the ping can be done without doubts > because the watchdog gets disabled anyway and a (re)start reset the > watchdog counter too. > Good spot. Thanks for this. Am wondering if this might also be an issue for da9062_wdt_set_timeout() which calls down to da9062_wdt_update_timeout_register() which in turn pings the watchdog first before disabling it and then setting the new timeout value. I think it makes sense to remove the ping from da9062_wdt_update_timeout_register() as well to avoid any possible issues where a ping() is called and then immediately after there's a call to adjust the timeout. If you can double check my hypothesis that would be good though. > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/watchdog/da9062_wdt.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c > index 77b6b5336067..0ad15d55071c 100644 > --- a/drivers/watchdog/da9062_wdt.c > +++ b/drivers/watchdog/da9062_wdt.c > @@ -97,13 +97,6 @@ static int da9062_wdt_stop(struct watchdog_device *wdd) > struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); > int ret; > > - ret = da9062_reset_watchdog_timer(wdt); > - if (ret) { > - dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = > %d)\n", > - ret); > - return ret; > - } > - > ret = regmap_update_bits(wdt->hw->regmap, > DA9062AA_CONTROL_D, > DA9062AA_TWDSCALE_MASK, > -- > 2.20.1
diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c index 77b6b5336067..0ad15d55071c 100644 --- a/drivers/watchdog/da9062_wdt.c +++ b/drivers/watchdog/da9062_wdt.c @@ -97,13 +97,6 @@ static int da9062_wdt_stop(struct watchdog_device *wdd) struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); int ret; - ret = da9062_reset_watchdog_timer(wdt); - if (ret) { - dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n", - ret); - return ret; - } - ret = regmap_update_bits(wdt->hw->regmap, DA9062AA_CONTROL_D, DA9062AA_TWDSCALE_MASK,
The da9062 hw has a minimum ping cool down phase of at least 200ms. The driver takes that into account by setting the min_hw_heartbeat_ms to 300ms and the core guarantees that the hw limit is observed for the ping() calls. But the core can't guarantees the required minimum ping cool down phase if a stop() command is send immediately after the ping() command. So it is not allowed to ping the watchdog within the stop() command as the driver do. Remove the ping can be done without doubts because the watchdog gets disabled anyway and a (re)start reset the watchdog counter too. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/watchdog/da9062_wdt.c | 7 ------- 1 file changed, 7 deletions(-)