[PATCH] watchdog: da9062: do not ping the hw during stop()

[PATCH] watchdog: da9062: do not ping the hw during stop()

[Marco Felsch]

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(-)

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

Re: [PATCH] watchdog: da9062: do not ping the hw during stop()

[Guenter Roeck]

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,

RE: [PATCH] watchdog: da9062: do not ping the hw during stop()

[Adam Thomson]

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