| Message ID | 20180226214905.24516-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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
> > - 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.
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
> 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.
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
> 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!
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();
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(-)