diff mbox

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

Message ID 1519919048-21884-2-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Fabrizio Castro March 1, 2018, 3:44 p.m. UTC 
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 <fabrizio.castro@bp.renesas.com>
Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
---
v5->v6:
* pausing the watchdog during suspend

Geert,

you marked v5 as:
Reviewed-and-Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Comments

Wolfram Sang March 1, 2018, 5:18 p.m. UTC | #1 
> +	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(&priv->wdev)) {
> +		priv->timeleft = rwdt_get_timeleft(&priv->wdev);

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

> +		rwdt_stop(&priv->wdev);
> +	}
> +	return 0;
> +}
> +
> +static int __maybe_unused rwdt_resume(struct device *dev)
> +{
> +	struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&priv->wdev)) {
> +		rwdt_start(&priv->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.
Fabrizio Castro March 1, 2018, 6:05 p.m. UTC | #2 
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(&priv->wdev)) {
> > +priv->timeleft = rwdt_get_timeleft(&priv->wdev);
>
> ...and read the register value directly here...
>
> > +rwdt_stop(&priv->wdev);
> > +}
> > +return 0;
> > +}
> > +
> > +static int __maybe_unused rwdt_resume(struct device *dev)
> > +{
> > +struct rwdt_priv *priv = dev_get_drvdata(dev);
> > +
> > +if (watchdog_active(&priv->wdev)) {
> > +rwdt_start(&priv->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.
diff mbox

Patch

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(&priv->wdev)) {
+		priv->timeleft = rwdt_get_timeleft(&priv->wdev);
+		rwdt_stop(&priv->wdev);
+	}
+	return 0;
+}
+
+static int __maybe_unused rwdt_resume(struct device *dev)
+{
+	struct rwdt_priv *priv = dev_get_drvdata(dev);
+
+	if (watchdog_active(&priv->wdev)) {
+		rwdt_start(&priv->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 = &rwdt_pm_ops,
 	},
 	.probe = rwdt_probe,
 	.remove = rwdt_remove,