Re: [PATCH] watchdog: renesas_wdt: adapt timer setup to recommended procedure

[Guenter Roeck <linux@roeck-us.net>]

On Mon, Feb 26, 2018 at 10:49:05PM +0100, Wolfram Sang wrote:
> The datasheet says we should stop the timer before changing the clock
> divider. So, let's meet that recommendation.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/watchdog/renesas_wdt.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 831ef83f6de15b..c4a17d72d02506 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -74,12 +74,17 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
>  static int rwdt_start(struct watchdog_device *wdev)
>  {
>  	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> +	u8 val;
>  
>  	pm_runtime_get_sync(wdev->parent);
>  
> -	rwdt_write(priv, 0, RWTCSRB);
> -	rwdt_write(priv, priv->cks, RWTCSRA);

Isn't this done implicitly with the above already ?
After all, priv->cks won't have RWTCSRA_TME set.

The only exception I can think of is if the watchdog is 
already running during boot, but that situation isn't handled
anyway.

Thanks,
Guenter

> +	/* Stop the timer before we modify any register */
> +	val = readb_relaxed(priv->base + RWTCSRA) & ~RWTCSRA_TME;
> +	rwdt_write(priv, val, RWTCSRA);
> +
>  	rwdt_init_timeout(wdev);
> +	rwdt_write(priv, priv->cks, RWTCSRA);
> +	rwdt_write(priv, 0, RWTCSRB);
>  
>  	while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
>  		cpu_relax();
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html