Re: [PATCH] watchdog: renesas_wdt: adapt timer setup to recommended procedure
> 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! signature.asc Description: PGP signature
Re: [PATCH] watchdog: renesas_wdt: adapt timer setup to recommended procedure
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
> 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. signature.asc Description: PGP signature
Re: [PATCH] watchdog: renesas_wdt: adapt timer setup to recommended procedure
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
> > - 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. signature.asc Description: PGP signature
Re: [PATCH] watchdog: renesas_wdt: adapt timer setup to recommended procedure
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> --- > 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] watchdog: renesas_wdt: adapt timer setup to recommended procedure
The datasheet says we should stop the timer before changing the clock divider. So, let's meet that recommendation. Signed-off-by: Wolfram Sang--- 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