[PATCH v6 0/3] Add Gen2 support to rwdt driver

[PATCH v6 0/3] Add Gen2 support to rwdt driver

[Fabrizio Castro]

Dear All,

the changes from this series originally come from another series:
"Fix watchdog on Renesas R-Car Gen2 and RZ/G1"
However, the watchdog driver related changes are the only changes
still under open discussion, that's why I am moving on with a
smaller series now.

Geert highlighted problems on R-Car Gen3, and he also preferred
splitting commit "watchdog: renesas_wdt: Add R-Car Gen2 support"
into two commits, one for adding suspend/resume support, and
another one for adding the R-Car Gen2 compatible string.
This version of the series tackles those two open points.

Thanks,

Fabrizio Castro (3):
  watchdog: renesas_wdt: Add suspend/resume support
  watchdog: renesas_wdt: Add R-Car Gen2 support
  watchdog: renesas_wdt: Add restart handler

 drivers/watchdog/renesas_wdt.c | 52 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 7 deletions(-)

-- 
2.7.4

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

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

[PATCH v6 2/3] watchdog: renesas_wdt: Add R-Car Gen2 support

[Fabrizio Castro]

Due to commits:
* "ARM: shmobile: Add watchdog support",
* "ARM: shmobile: rcar-gen2: Add watchdog support", and
* "soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2",
we now have everything we needed for the watchdog to work on Gen2 and
RZ/G1.

This commit adds "renesas,rcar-gen2-wdt" as compatible string 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:
* stripped suspend/resume functionality
* now gen2 compatible string comes before gen3

Geert,

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

but since I have flipped the order, I am not including it as you may
disagree with the change.

Thanks,
Fab

 drivers/watchdog/renesas_wdt.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index cbf8e4d..609056f 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -232,12 +232,8 @@ static int __maybe_unused rwdt_resume(struct device *dev)
 
 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
- * due to HW issues. This needs to be solved before adding compatibles here.
- */
 static const struct of_device_id rwdt_ids[] = {
+	{ .compatible = "renesas,rcar-gen2-wdt", },
 	{ .compatible = "renesas,rcar-gen3-wdt", },
 	{ /* sentinel */ }
 };
-- 
2.7.4

[PATCH v6 3/3] watchdog: renesas_wdt: Add restart handler

[Fabrizio Castro]

On iWave's boards iwg20d and iwg22d the only way to reboot the system is
by means of the watchdog.
This patch adds a restart handler to rwdt_ops, and also makes sure we
keep its priority to a medium level, in order to not override other more
effective handlers.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
v5->v6:
* no change

 drivers/watchdog/renesas_wdt.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 609056f..ce36efc 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -112,6 +112,16 @@ static unsigned int rwdt_get_timeleft(struct watchdog_device *wdev)
 	return DIV_BY_CLKS_PER_SEC(priv, 65536 - val);
 }
 
+static int rwdt_restart(struct watchdog_device *wdev, unsigned long action,
+			void *data)
+{
+	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	rwdt_start(wdev);
+	rwdt_write(priv, 0xffff, RWTCNT);
+	return 0;
+}
+
 static const struct watchdog_info rwdt_ident = {
 	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
 	.identity = "Renesas WDT Watchdog",
@@ -123,6 +133,7 @@ static const struct watchdog_ops rwdt_ops = {
 	.stop = rwdt_stop,
 	.ping = rwdt_init_timeout,
 	.get_timeleft = rwdt_get_timeleft,
+	.restart = rwdt_restart,
 };
 
 static int rwdt_probe(struct platform_device *pdev)
@@ -181,6 +192,7 @@ static int rwdt_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, priv);
 	watchdog_set_drvdata(&priv->wdev, priv);
 	watchdog_set_nowayout(&priv->wdev, nowayout);
+	watchdog_set_restart_priority(&priv->wdev, 0);
 
 	/* This overrides the default timeout only if DT configuration was found */
 	ret = watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
-- 
2.7.4

Re: [PATCH v6 3/3] watchdog: renesas_wdt: Add restart handler

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

On Thu, Mar 01, 2018 at 03:44:08PM +0000, Fabrizio Castro wrote:
> On iWave's boards iwg20d and iwg22d the only way to reboot the system is
> by means of the watchdog.
> This patch adds a restart handler to rwdt_ops, and also makes sure we
> keep its priority to a medium level, in order to not override other more

the lowest level

> effective handlers.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

With that fixed:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 2/3] watchdog: renesas_wdt: Add R-Car Gen2 support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]

On Thu, Mar 01, 2018 at 03:44:07PM +0000, Fabrizio Castro wrote:
> Due to commits:
> * "ARM: shmobile: Add watchdog support",
> * "ARM: shmobile: rcar-gen2: Add watchdog support", and
> * "soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2",
> we now have everything we needed for the watchdog to work on Gen2 and
> RZ/G1.
> 
> This commit adds "renesas,rcar-gen2-wdt" as compatible string 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>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]


> +	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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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(&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.