watchdog: renesas_wdt: adapt timer setup to recommended procedure
diff mbox

Message ID 20180226214905.24516-1-wsa+renesas@sang-engineering.com
State Under Review
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Wolfram Sang Feb. 26, 2018, 9:49 p.m. UTC
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(-)

Comments

Guenter Roeck Feb. 26, 2018, 10:38 p.m. UTC | #1
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
Wolfram Sang Feb. 27, 2018, 1 p.m. UTC | #2
> > -	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.
Guenter Roeck Feb. 27, 2018, 3:03 p.m. UTC | #3
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
Wolfram Sang Feb. 27, 2018, 4:22 p.m. UTC | #4
> 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.
Guenter Roeck Feb. 27, 2018, 5:19 p.m. UTC | #5
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
Wolfram Sang Feb. 27, 2018, 8:36 p.m. UTC | #6
> 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!

Patch
diff mbox

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