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

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

[Wolfram Sang]

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);
+	/* 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

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

[Guenter Roeck]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

> > -	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.

Yes. The request came from a group doing some (safety?) audit who didn't
like the implicit handling which might change if the code in probe()
might change somewhen. And the datasheet explicitly says to disable the
timer first before doing anything with the clock dividers. Given that, I
can agree to this change, too.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Guenter Roeck]

On 02/27/2018 05:00 AM, Wolfram Sang wrote:
>>> -	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.
> 
> Yes. The request came from a group doing some (safety?) audit who didn't
> like the implicit handling which might change if the code in probe()
> might change somewhen. And the datasheet explicitly says to disable the
> timer first before doing anything with the clock dividers. Given that, I
> can agree to this change, too.
> 

Defensive code is fine, bit that is a bit too defensive. We might as well
start checking value ranges in the drivers' set_timeout functions just in case
the core gets it wrong. Just add a note to the probe function where cks is
initialized that RWTCSRA_TME must not be set, and that RWTCSRA_TME must be
cleared before changing the divider.

Guenter

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]


> the core gets it wrong. Just add a note to the probe function where cks is
> initialized that RWTCSRA_TME must not be set, and that RWTCSRA_TME must be
> cleared before changing the divider.

But we would still need to disable TME sperately if the caclulated cks
is not the same value as the current register value (be it power-on
default or something the firmware did set). Because we shall not disable
TME and modify cks at the same time.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Guenter Roeck]

On Tue, Feb 27, 2018 at 05:22:56PM +0100, Wolfram Sang wrote:
> 
> > the core gets it wrong. Just add a note to the probe function where cks is
> > initialized that RWTCSRA_TME must not be set, and that RWTCSRA_TME must be
> > cleared before changing the divider.
> 
> But we would still need to disable TME sperately if the caclulated cks
> is not the same value as the current register value (be it power-on
> default or something the firmware did set). Because we shall not disable
> TME and modify cks at the same time.
> 
AFAICS this only applies if the watchdog is already running at boot,
which is not currently supported to start with.

Guenter

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]


> AFAICS this only applies if the watchdog is already running at boot,
> which is not currently supported to start with.

Hmm, probably this is a good occasion to add support for it. I'll check.

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]