RE: [PATCH v6 1/3] watchdog: renesas_wdt: Add suspend/resume support
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.
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? 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
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(&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, -- 2.7.4