RE: [PATCH v6 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Fabrizio Castro]

Hi Wolfram,

thank you for your feedback!

> Subject: Re: [PATCH v6 1/3] watchdog: renesas_wdt: Add suspend/resume support
>
>
> > +unsigned int timeleft;
>
> What about using
> u16 time_left;
>
> ...
>
> > +static void rwdt_set_timeleft(struct rwdt_priv *priv, unsigned int 
> > timeleft)
> > +{
> > +rwdt_write(priv, 65536 - MUL_BY_CLKS_PER_SEC(priv, timeleft), RWTCNT);
> > +}
> > +
>
> ... skipping this ...
>
> >  static int rwdt_init_timeout(struct watchdog_device *wdev)
> >  {
> >  struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> >
> > -rwdt_write(priv, 65536 - MUL_BY_CLKS_PER_SEC(priv, wdev->timeout), RWTCNT);
> > -
> > +rwdt_set_timeleft(priv, wdev->timeout);
>
> ... and this ...
>
> >  return 0;
> >  }
> >
> > @@ -203,6 +208,30 @@ static int rwdt_remove(struct platform_device *pdev)
> >  return 0;
> >  }
> >
> > +static int __maybe_unused rwdt_suspend(struct device *dev)
> > +{
> > +struct rwdt_priv *priv = dev_get_drvdata(dev);
> > +
> > +if (watchdog_active(>wdev)) {
> > +priv->timeleft = rwdt_get_timeleft(>wdev);
>
> ...and read the register value directly here...
>
> > +rwdt_stop(>wdev);
> > +}
> > +return 0;
> > +}
> > +
> > +static int __maybe_unused rwdt_resume(struct device *dev)
> > +{
> > +struct rwdt_priv *priv = dev_get_drvdata(dev);
> > +
> > +if (watchdog_active(>wdev)) {
> > +rwdt_start(>wdev);
> > +rwdt_set_timeleft(priv, priv->timeleft);
>
> ... and put it back here?

It works for me, I'll send a new version with your comments applied.

Thanks,
Fab

>
> That would save calling the conversion macros which may introduce
> rounding errors. It is also a tad faster and smaller. And less lines of
> code.




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, 
Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered 
No. 04586709.

Re: [PATCH v6 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Wolfram Sang]

> + unsigned int timeleft;

What about using
u16 time_left;

...

> +static void rwdt_set_timeleft(struct rwdt_priv *priv, unsigned int timeleft)
> +{
> + rwdt_write(priv, 65536 - MUL_BY_CLKS_PER_SEC(priv, timeleft), RWTCNT);
> +}
> +

... skipping this ...

>  static int rwdt_init_timeout(struct watchdog_device *wdev)
>  {
>   struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
>  
> - rwdt_write(priv, 65536 - MUL_BY_CLKS_PER_SEC(priv, wdev->timeout), 
> RWTCNT);
> -
> + rwdt_set_timeleft(priv, wdev->timeout);

... and this ...

>   return 0;
>  }
>  
> @@ -203,6 +208,30 @@ static int rwdt_remove(struct platform_device *pdev)
>   return 0;
>  }
>  
> +static int __maybe_unused rwdt_suspend(struct device *dev)
> +{
> + struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> + if (watchdog_active(>wdev)) {
> + priv->timeleft = rwdt_get_timeleft(>wdev);

...and read the register value directly here...

> + rwdt_stop(>wdev);
> + }
> + return 0;
> +}
> +
> +static int __maybe_unused rwdt_resume(struct device *dev)
> +{
> + struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> + if (watchdog_active(>wdev)) {
> + rwdt_start(>wdev);
> + rwdt_set_timeleft(priv, priv->timeleft);

... and put it back here?

That would save calling the conversion macros which may introduce
rounding errors. It is also a tad faster and smaller. And less lines of
code.



signature.asc
Description: PGP signature

[PATCH v6 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Fabrizio Castro]

On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
on R-Car Gen3 we power the IP down during suspend.

This commit adds suspend/resume support, so that the watchdog counting
"pauses" during suspend on all of the SoCs compatible with this driver
and on those we are now adding support for (R-Car Gen2 and RZ/G1).

Signed-off-by: Fabrizio Castro 
Signed-off-by: Ramesh Shanmugasundaram 
---
v5->v6:
* pausing the watchdog during suspend

Geert,

you marked v5 as:
Reviewed-and-Tested-by: Geert Uytterhoeven 

but I believe this patch deserves a fresh review and fresh testing.

Thanks,
Fab

 drivers/watchdog/renesas_wdt.c | 34 --
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 831ef83..cbf8e4d 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -49,6 +49,7 @@ struct rwdt_priv {
void __iomem *base;
struct watchdog_device wdev;
unsigned long clk_rate;
+   unsigned int timeleft;
u8 cks;
 };
 
@@ -62,12 +63,16 @@ static void rwdt_write(struct rwdt_priv *priv, u32 val, 
unsigned int reg)
writel_relaxed(val, priv->base + reg);
 }
 
+static void rwdt_set_timeleft(struct rwdt_priv *priv, unsigned int timeleft)
+{
+   rwdt_write(priv, 65536 - MUL_BY_CLKS_PER_SEC(priv, timeleft), RWTCNT);
+}
+
 static int rwdt_init_timeout(struct watchdog_device *wdev)
 {
struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
 
-   rwdt_write(priv, 65536 - MUL_BY_CLKS_PER_SEC(priv, wdev->timeout), 
RWTCNT);
-
+   rwdt_set_timeleft(priv, wdev->timeout);
return 0;
 }
 
@@ -203,6 +208,30 @@ static int rwdt_remove(struct platform_device *pdev)
return 0;
 }
 
+static int __maybe_unused rwdt_suspend(struct device *dev)
+{
+   struct rwdt_priv *priv = dev_get_drvdata(dev);
+
+   if (watchdog_active(>wdev)) {
+   priv->timeleft = rwdt_get_timeleft(>wdev);
+   rwdt_stop(>wdev);
+   }
+   return 0;
+}
+
+static int __maybe_unused rwdt_resume(struct device *dev)
+{
+   struct rwdt_priv *priv = dev_get_drvdata(dev);
+
+   if (watchdog_active(>wdev)) {
+   rwdt_start(>wdev);
+   rwdt_set_timeleft(priv, priv->timeleft);
+   }
+   return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(rwdt_pm_ops, rwdt_suspend, rwdt_resume);
+
 /*
  * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for SMP
  * to work there, one also needs a RESET (RST) driver which does not exist yet
@@ -218,6 +247,7 @@ static struct platform_driver rwdt_driver = {
.driver = {
.name = "renesas_wdt",
.of_match_table = rwdt_ids,
+   .pm = _pm_ops,
},
.probe = rwdt_probe,
.remove = rwdt_remove,
-- 
2.7.4