Re: [PATCH v2 5/4] watchdog: omap: assert the counter being stopped before reprogramming

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

On 04/29/2015 11:38 AM, Uwe Kleine-König wrote:
> The omap watchdog has the annoying behaviour that writes to most
> registers don't have any effect when the watchdog is already running.
> Quoting the AM335x reference manual:
>
> 	To modify the timer counter value (the WDT_WCRR register),
> 	prescaler ratio (the WDT_WCLR[4:2] PTV bit field), delay
> 	configuration value (the WDT_WDLY[31:0] DLY_VALUE bit field), or
> 	the load value (the WDT_WLDR[31:0] TIMER_LOAD bit field), the
> 	watchdog timer must be disabled by using the start/stop sequence
> 	(the WDT_WSPR register).
>
> Currently the timer is stopped in the .probe callback but still there
> are possibilities that yield to a situation where omap_wdt_start is
> entered with the timer running (e.g. when /dev/watchdog is closed
> without stopping and then reopened). In such a case programming the
> timeout silently fails!
>
> To circumvent this stop the timer before reprogramming.
>
> Assuming one of the first things the watchdog user does is setting the
> timeout explicitly nothing too bad should happen because this explicit
> setting works fine.
>
> Fixes: 7768a13c252a ("[PATCH] OMAP: Add Watchdog driver support")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Annoying, but it looks like it is necessary.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/omap_wdt.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 0421c06a6cf0..d7619dd7c1ca 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -135,6 +135,13 @@ static int omap_wdt_start(struct watchdog_device *wdog)
>
>   	pm_runtime_get_sync(wdev->dev);
>
> +	/*
> +	 * Make sure the watchdog is disabled. This is unfortunately required
> +	 * because writing to various registers with the watchdog running has no
> +	 * effect.
> +	 */
> +	omap_wdt_disable(wdev);
> +
>   	/* initialize prescaler */
>   	while (readl_relaxed(base + OMAP_WATCHDOG_WPS) & 0x01)
>   		cpu_relax();
>