[RFC PATCH 0/3] watchdog: renesas_wdt: use standard clock handling

[RFC PATCH 0/3] watchdog: renesas_wdt: use standard clock handling

[Wolfram Sang]

As mentioned in a response to a previous patch, clock handling for Renesas
R-Car is done via RuntimePM. Patch 1 implements that consequently. Patch 2
is a cleanup then possible and patch 3 is a trivial copyright update.

This is RFC because I'd like Geert's comments on these patches first which
might take a few days...

Looking forward to other comments, too, of course :)

These patches are on top of the already reviewed 'better-precision' series for
this watchdog. A branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/watchdog-fixes

These patches have been tested on a Renesas Salvator-X / r8a7796 (R-Car M3-W)

Thanks and kind regards,

   Wolfram


Wolfram Sang (3):
  watchdog: renesas_wdt: consistently use RuntimePM for clock management
  watchdog: renesas_wdt: make 'clk' a variable local to probe()
  watchdog: renesas_wdt: update copyright dates

 drivers/watchdog/renesas_wdt.c | 47 +++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

-- 
2.11.0

[RFC PATCH 1/3] watchdog: renesas_wdt: consistently use RuntimePM for clock management

[Wolfram Sang]

On Renesas R-Car archs, RuntimePM does all the clock handling. So, use
it consistently to enable/disable the clocks. Also make sure that clocks
are really enabled around clk_get_rate(). clk_summary looks proper now:

		clock	enable_cnt	prepare_cnt	rate ...
Before this commit:

At boot:	rwdt	1		1		32768 0 0
WDT running:	rwdt	2		2		32768 0 0

After this commit:

At boot:	rwdt	0		1		32768 0 0
WDT running	rwdt	1		1		32768 0 0

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/renesas_wdt.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index e3f204bb8802aa..a03997b418ba9c 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -76,7 +76,7 @@ static int rwdt_start(struct watchdog_device *wdev)
 {
 	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
 
-	clk_prepare_enable(priv->clk);
+	pm_runtime_get_sync(wdev->parent);
 
 	rwdt_write(priv, 0, RWTCSRB);
 	rwdt_write(priv, priv->cks, RWTCSRA);
@@ -95,7 +95,7 @@ static int rwdt_stop(struct watchdog_device *wdev)
 	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
 
 	rwdt_write(priv, priv->cks, RWTCSRA);
-	clk_disable_unprepare(priv->clk);
+	pm_runtime_put(wdev->parent);
 
 	return 0;
 }
@@ -141,9 +141,16 @@ static int rwdt_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->clk))
 		return PTR_ERR(priv->clk);
 
+	pm_runtime_enable(&pdev->dev);
+
+	pm_runtime_get_sync(&pdev->dev);
 	priv->clk_rate = clk_get_rate(priv->clk);
-	if (!priv->clk_rate)
-		return -ENOENT;
+	pm_runtime_put(&pdev->dev);
+
+	if (!priv->clk_rate) {
+		ret = -ENOENT;
+		goto out_pm_disable;
+	}
 
 	for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
 		clks_per_sec = priv->clk_rate / clk_divs[i];
@@ -155,12 +162,10 @@ static int rwdt_probe(struct platform_device *pdev)
 
 	if (i < 0) {
 		dev_err(&pdev->dev, "Can't find suitable clock divider\n");
-		return -ERANGE;
+		ret = -ERANGE;
+		goto out_pm_disable;
 	}
 
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
-
 	priv->wdev.info = &rwdt_ident,
 	priv->wdev.ops = &rwdt_ops,
 	priv->wdev.parent = &pdev->dev;
@@ -178,13 +183,14 @@ static int rwdt_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "Specified timeout value invalid, using default\n");
 
 	ret = watchdog_register_device(&priv->wdev);
-	if (ret < 0) {
-		pm_runtime_put(&pdev->dev);
-		pm_runtime_disable(&pdev->dev);
-		return ret;
-	}
+	if (ret < 0)
+		goto out_pm_disable;
 
 	return 0;
+
+ out_pm_disable:
+	pm_runtime_disable(&pdev->dev);
+	return ret;
 }
 
 static int rwdt_remove(struct platform_device *pdev)
@@ -192,7 +198,6 @@ static int rwdt_remove(struct platform_device *pdev)
 	struct rwdt_priv *priv = platform_get_drvdata(pdev);
 
 	watchdog_unregister_device(&priv->wdev);
-	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	return 0;
-- 
2.11.0

[RFC PATCH 2/3] watchdog: renesas_wdt: make 'clk' a variable local to probe()

[Wolfram Sang]

It is not needed outside probe() anymore.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/renesas_wdt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index a03997b418ba9c..2927b5086e156e 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -48,7 +48,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 struct rwdt_priv {
 	void __iomem *base;
 	struct watchdog_device wdev;
-	struct clk *clk;
 	unsigned long clk_rate;
 	u8 cks;
 };
@@ -125,6 +124,7 @@ static int rwdt_probe(struct platform_device *pdev)
 {
 	struct rwdt_priv *priv;
 	struct resource *res;
+	struct clk *clk;
 	unsigned long clks_per_sec;
 	int ret, i;
 
@@ -137,14 +137,14 @@ static int rwdt_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
-	priv->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(priv->clk))
-		return PTR_ERR(priv->clk);
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
 	pm_runtime_enable(&pdev->dev);
 
 	pm_runtime_get_sync(&pdev->dev);
-	priv->clk_rate = clk_get_rate(priv->clk);
+	priv->clk_rate = clk_get_rate(clk);
 	pm_runtime_put(&pdev->dev);
 
 	if (!priv->clk_rate) {
-- 
2.11.0

[RFC PATCH 3/3] watchdog: renesas_wdt: update copyright dates

[Wolfram Sang]

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/renesas_wdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 2927b5086e156e..831ef83f6de15b 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -1,8 +1,8 @@
 /*
  * Watchdog driver for Renesas WDT watchdog
  *
- * Copyright (C) 2015-16 Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
- * Copyright (C) 2015-16 Renesas Electronics Corporation
+ * Copyright (C) 2015-17 Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
+ * Copyright (C) 2015-17 Renesas Electronics Corporation
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 as published by
-- 
2.11.0

Re: [RFC PATCH 0/3] watchdog: renesas_wdt: use standard clock handling

[Guenter Roeck]

On Wed, Jul 26, 2017 at 11:54:36PM +0200, Wolfram Sang wrote:
> As mentioned in a response to a previous patch, clock handling for Renesas
> R-Car is done via RuntimePM. Patch 1 implements that consequently. Patch 2
> is a cleanup then possible and patch 3 is a trivial copyright update.
> 
> This is RFC because I'd like Geert's comments on these patches first which
> might take a few days...
> 
Was there any feedback from Geert ? I don't recall seeing any.

Guenter

> Looking forward to other comments, too, of course :)
> 
> These patches are on top of the already reviewed 'better-precision' series for
> this watchdog. A branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/watchdog-fixes
> 
> These patches have been tested on a Renesas Salvator-X / r8a7796 (R-Car M3-W)
> 
> Thanks and kind regards,
> 
>    Wolfram
> 
> 
> Wolfram Sang (3):
>   watchdog: renesas_wdt: consistently use RuntimePM for clock management
>   watchdog: renesas_wdt: make 'clk' a variable local to probe()
>   watchdog: renesas_wdt: update copyright dates
> 
>  drivers/watchdog/renesas_wdt.c | 47 +++++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> -- 
> 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

Re: [RFC PATCH 0/3] watchdog: renesas_wdt: use standard clock handling

[Wolfram Sang]

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

On Mon, Aug 14, 2017 at 08:32:30AM -0700, Guenter Roeck wrote:
> On Wed, Jul 26, 2017 at 11:54:36PM +0200, Wolfram Sang wrote:
> > As mentioned in a response to a previous patch, clock handling for Renesas
> > R-Car is done via RuntimePM. Patch 1 implements that consequently. Patch 2
> > is a cleanup then possible and patch 3 is a trivial copyright update.
> > 
> > This is RFC because I'd like Geert's comments on these patches first which
> > might take a few days...
> > 
> Was there any feedback from Geert ? I don't recall seeing any.

He just got back from holidays and is bravely working through his mail
queue :)


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

Re: [RFC PATCH 0/3] watchdog: renesas_wdt: use standard clock handling

[Guenter Roeck]

On Mon, Aug 14, 2017 at 05:47:28PM +0200, Wolfram Sang wrote:
> On Mon, Aug 14, 2017 at 08:32:30AM -0700, Guenter Roeck wrote:
> > On Wed, Jul 26, 2017 at 11:54:36PM +0200, Wolfram Sang wrote:
> > > As mentioned in a response to a previous patch, clock handling for Renesas
> > > R-Car is done via RuntimePM. Patch 1 implements that consequently. Patch 2
> > > is a cleanup then possible and patch 3 is a trivial copyright update.
> > > 
> > > This is RFC because I'd like Geert's comments on these patches first which
> > > might take a few days...
> > > 
> > Was there any feedback from Geert ? I don't recall seeing any.
> 
> He just got back from holidays and is bravely working through his mail
> queue :)
> 

Good to hear that I am not the only one with that problem :-).

Guenter

Re: [RFC PATCH 1/3] watchdog: renesas_wdt: consistently use RuntimePM for clock management

[Geert Uytterhoeven]

On Wed, Jul 26, 2017 at 11:54 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> On Renesas R-Car archs, RuntimePM does all the clock handling. So, use
> it consistently to enable/disable the clocks. Also make sure that clocks
> are really enabled around clk_get_rate(). clk_summary looks proper now:
>
>                 clock   enable_cnt      prepare_cnt     rate ...
> Before this commit:
>
> At boot:        rwdt    1               1               32768 0 0
> WDT running:    rwdt    2               2               32768 0 0
>
> After this commit:
>
> At boot:        rwdt    0               1               32768 0 0
> WDT running     rwdt    1               1               32768 0 0
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 2/3] watchdog: renesas_wdt: make 'clk' a variable local to probe()

[Geert Uytterhoeven]

On Wed, Jul 26, 2017 at 11:54 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> It is not needed outside probe() anymore.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC, 1/3] watchdog: renesas_wdt: consistently use RuntimePM for clock management

[Guenter Roeck]

On Wed, Jul 26, 2017 at 11:54:37PM +0200, Wolfram Sang wrote:
> On Renesas R-Car archs, RuntimePM does all the clock handling. So, use
> it consistently to enable/disable the clocks. Also make sure that clocks
> are really enabled around clk_get_rate(). clk_summary looks proper now:
> 
> 		clock	enable_cnt	prepare_cnt	rate ...
> Before this commit:
> 
> At boot:	rwdt	1		1		32768 0 0
> WDT running:	rwdt	2		2		32768 0 0
> 
> After this commit:
> 
> At boot:	rwdt	0		1		32768 0 0
> WDT running	rwdt	1		1		32768 0 0
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/renesas_wdt.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index e3f204bb8802aa..a03997b418ba9c 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -76,7 +76,7 @@ static int rwdt_start(struct watchdog_device *wdev)
>  {
>  	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
>  
> -	clk_prepare_enable(priv->clk);
> +	pm_runtime_get_sync(wdev->parent);
>  
>  	rwdt_write(priv, 0, RWTCSRB);
>  	rwdt_write(priv, priv->cks, RWTCSRA);
> @@ -95,7 +95,7 @@ static int rwdt_stop(struct watchdog_device *wdev)
>  	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
>  
>  	rwdt_write(priv, priv->cks, RWTCSRA);
> -	clk_disable_unprepare(priv->clk);
> +	pm_runtime_put(wdev->parent);
>  
>  	return 0;
>  }
> @@ -141,9 +141,16 @@ static int rwdt_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->clk))
>  		return PTR_ERR(priv->clk);
>  
> +	pm_runtime_enable(&pdev->dev);
> +
> +	pm_runtime_get_sync(&pdev->dev);
>  	priv->clk_rate = clk_get_rate(priv->clk);
> -	if (!priv->clk_rate)
> -		return -ENOENT;
> +	pm_runtime_put(&pdev->dev);
> +
> +	if (!priv->clk_rate) {
> +		ret = -ENOENT;
> +		goto out_pm_disable;
> +	}
>  
>  	for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
>  		clks_per_sec = priv->clk_rate / clk_divs[i];
> @@ -155,12 +162,10 @@ static int rwdt_probe(struct platform_device *pdev)
>  
>  	if (i < 0) {
>  		dev_err(&pdev->dev, "Can't find suitable clock divider\n");
> -		return -ERANGE;
> +		ret = -ERANGE;
> +		goto out_pm_disable;
>  	}
>  
> -	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_get_sync(&pdev->dev);
> -
>  	priv->wdev.info = &rwdt_ident,
>  	priv->wdev.ops = &rwdt_ops,
>  	priv->wdev.parent = &pdev->dev;
> @@ -178,13 +183,14 @@ static int rwdt_probe(struct platform_device *pdev)
>  		dev_warn(&pdev->dev, "Specified timeout value invalid, using default\n");
>  
>  	ret = watchdog_register_device(&priv->wdev);
> -	if (ret < 0) {
> -		pm_runtime_put(&pdev->dev);
> -		pm_runtime_disable(&pdev->dev);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto out_pm_disable;
>  
>  	return 0;
> +
> + out_pm_disable:
> +	pm_runtime_disable(&pdev->dev);
> +	return ret;
>  }
>  
>  static int rwdt_remove(struct platform_device *pdev)
> @@ -192,7 +198,6 @@ static int rwdt_remove(struct platform_device *pdev)
>  	struct rwdt_priv *priv = platform_get_drvdata(pdev);
>  
>  	watchdog_unregister_device(&priv->wdev);
> -	pm_runtime_put(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
>  	return 0;

Re: [RFC, 2/3] watchdog: renesas_wdt: make 'clk' a variable local to probe()

[Guenter Roeck]

On Wed, Jul 26, 2017 at 11:54:38PM +0200, Wolfram Sang wrote:
> It is not needed outside probe() anymore.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/renesas_wdt.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index a03997b418ba9c..2927b5086e156e 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -48,7 +48,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>  struct rwdt_priv {
>  	void __iomem *base;
>  	struct watchdog_device wdev;
> -	struct clk *clk;
>  	unsigned long clk_rate;
>  	u8 cks;
>  };
> @@ -125,6 +124,7 @@ static int rwdt_probe(struct platform_device *pdev)
>  {
>  	struct rwdt_priv *priv;
>  	struct resource *res;
> +	struct clk *clk;
>  	unsigned long clks_per_sec;
>  	int ret, i;
>  
> @@ -137,14 +137,14 @@ static int rwdt_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->base))
>  		return PTR_ERR(priv->base);
>  
> -	priv->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(priv->clk))
> -		return PTR_ERR(priv->clk);
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
>  
>  	pm_runtime_enable(&pdev->dev);
>  
>  	pm_runtime_get_sync(&pdev->dev);
> -	priv->clk_rate = clk_get_rate(priv->clk);
> +	priv->clk_rate = clk_get_rate(clk);
>  	pm_runtime_put(&pdev->dev);
>  
>  	if (!priv->clk_rate) {

Re: [RFC,3/3] watchdog: renesas_wdt: update copyright dates

[Guenter Roeck]

On Wed, Jul 26, 2017 at 11:54:39PM +0200, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/renesas_wdt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 2927b5086e156e..831ef83f6de15b 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -1,8 +1,8 @@
>  /*
>   * Watchdog driver for Renesas WDT watchdog
>   *
> - * Copyright (C) 2015-16 Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
> - * Copyright (C) 2015-16 Renesas Electronics Corporation
> + * Copyright (C) 2015-17 Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
> + * Copyright (C) 2015-17 Renesas Electronics Corporation
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU General Public License version 2 as published by