[PATCH 0/5] watchdog: renesas_wdt: improve precision

[PATCH 0/5] watchdog: renesas_wdt: improve precision

[Wolfram Sang]

Currently, the renesas-rwdt driver is not precise with input clocks which have
a remainder after the clock divisors are applied. This series should fix the
situation and also pays attention to ensure variables have proper types and are
divided properly. As a cherry on top, we also get a new divider value to allow
for higher max_timeout :)

Originally brought to attention by this BSP patch:
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=7d56256a4d189a83e278ec2013eb1f3648bc7411

Tested with a Salvator-X (R-Car M3-W) and the clock driver hacked to supply the
internal clock of 32552 Hz instead of the externally supplied 32768 Hz.

A branch can be found here:

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

Please comment or apply...

Thanks,

   Wolfram


Wolfram Sang (5):
  watchdog: renesas_wdt: avoid (theoretical) type overflow
  watchdog: renesas_wdt: check rate also for upper limit
  watchdog: renesas_wdt: don't round closest with get_timeleft
  watchdog: renesas_wdt: apply better precision
  watchdog: renesas_wdt: add another divider option

 drivers/watchdog/renesas_wdt.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

-- 
2.11.0

[PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow

[Wolfram Sang]

Because the smallest clock divider we can select is 1, 'clks_per_sec'
must be the same type as 'rate'.

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

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index cf61c92f7ecd63..4f8a3563b6aa53 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -112,8 +112,7 @@ static int rwdt_probe(struct platform_device *pdev)
 {
 	struct rwdt_priv *priv;
 	struct resource *res;
-	unsigned long rate;
-	unsigned int clks_per_sec;
+	unsigned long rate, clks_per_sec;
 	int ret, i;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-- 
2.11.0

[PATCH 2/5] watchdog: renesas_wdt: check rate also for upper limit

[Wolfram Sang]

When checking the clock rate, ensure also that counting all 16 bits
takes at least one second to match the granularity of the framework.

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 4f8a3563b6aa53..0b9e8a9b1dd14c 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -134,14 +134,14 @@ static int rwdt_probe(struct platform_device *pdev)
 
 	for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
 		clks_per_sec = DIV_ROUND_UP(rate, clk_divs[i]);
-		if (clks_per_sec) {
+		if (clks_per_sec && clks_per_sec < 65536) {
 			priv->clks_per_sec = clks_per_sec;
 			priv->cks = i;
 			break;
 		}
 	}
 
-	if (!clks_per_sec) {
+	if (i < 0) {
 		dev_err(&pdev->dev, "Can't find suitable clock divider\n");
 		return -ERANGE;
 	}
-- 
2.11.0

[PATCH 3/5] watchdog: renesas_wdt: don't round closest with get_timeleft

[Wolfram Sang]

We should never return more time left than there actually is. So, switch
to a plain divider instead of DIV_ROUND_CLOSEST.

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

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 0b9e8a9b1dd14c..d2390695d3453f 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -92,7 +92,7 @@ static unsigned int rwdt_get_timeleft(struct watchdog_device *wdev)
 	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
 	u16 val = readw_relaxed(priv->base + RWTCNT);
 
-	return DIV_ROUND_CLOSEST(65536 - val, priv->clks_per_sec);
+	return (65536 - val) / priv->clks_per_sec;
 }
 
 static const struct watchdog_info rwdt_ident = {
-- 
2.11.0

[PATCH 4/5] watchdog: renesas_wdt: apply better precision

[Wolfram Sang]

The error margin of the clks_per_second variable was too large and
caused offsets when used with clock frequencies which left a remainder
after applying the dividers. Now we always calculate directly using the
clock rate and the divider using some helper macros. That also means
that DIV_ROUND_UP moves from probe to the multiplication macro. In
probe, we don't need to ensure anymore that 'clks_per_sec' would go too
fast but rather ensure that the lower limit is really at least 1 to
certainly get a full cycle.

Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/renesas_wdt.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index d2390695d3453f..599ba5aaa0536f 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -26,6 +26,17 @@
 
 #define RWDT_DEFAULT_TIMEOUT 60U
 
+/*
+ * In probe, clk_rate is checked to be not more than 16 bit * biggest clock
+ * divider (10 bits). d is only a factor to fully utilize the WDT counter and
+ * will not exceed its 16 bits. Thus, no overflow, we stay below 32 bits.
+ */
+#define MUL_BY_CLKS_PER_SEC(p, d) \
+	DIV_ROUND_UP((d) * (p)->clk_rate, clk_divs[(p)->cks])
+
+/* d is 16 bit, clk_divs 10 bit -> no 32 bit overflow */
+#define DIV_BY_CLKS_PER_SEC(p, d) ((d) * clk_divs[(p)->cks] / (p)->clk_rate)
+
 static const unsigned int clk_divs[] = { 1, 4, 16, 32, 64, 128, 1024 };
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
@@ -37,7 +48,7 @@ struct rwdt_priv {
 	void __iomem *base;
 	struct watchdog_device wdev;
 	struct clk *clk;
-	unsigned int clks_per_sec;
+	unsigned long clk_rate;
 	u8 cks;
 };
 
@@ -55,7 +66,7 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
 {
 	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
 
-	rwdt_write(priv, 65536 - wdev->timeout * priv->clks_per_sec, RWTCNT);
+	rwdt_write(priv, 65536 - MUL_BY_CLKS_PER_SEC(priv, wdev->timeout), RWTCNT);
 
 	return 0;
 }
@@ -92,7 +103,7 @@ static unsigned int rwdt_get_timeleft(struct watchdog_device *wdev)
 	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
 	u16 val = readw_relaxed(priv->base + RWTCNT);
 
-	return (65536 - val) / priv->clks_per_sec;
+	return DIV_BY_CLKS_PER_SEC(priv, 65536 - val);
 }
 
 static const struct watchdog_info rwdt_ident = {
@@ -112,7 +123,7 @@ static int rwdt_probe(struct platform_device *pdev)
 {
 	struct rwdt_priv *priv;
 	struct resource *res;
-	unsigned long rate, clks_per_sec;
+	unsigned long clks_per_sec;
 	int ret, i;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -128,14 +139,13 @@ static int rwdt_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->clk))
 		return PTR_ERR(priv->clk);
 
-	rate = clk_get_rate(priv->clk);
-	if (!rate)
+	priv->clk_rate = clk_get_rate(priv->clk);
+	if (!priv->clk_rate)
 		return -ENOENT;
 
 	for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
-		clks_per_sec = DIV_ROUND_UP(rate, clk_divs[i]);
+		clks_per_sec = priv->clk_rate / clk_divs[i];
 		if (clks_per_sec && clks_per_sec < 65536) {
-			priv->clks_per_sec = clks_per_sec;
 			priv->cks = i;
 			break;
 		}
@@ -153,7 +163,7 @@ static int rwdt_probe(struct platform_device *pdev)
 	priv->wdev.ops = &rwdt_ops,
 	priv->wdev.parent = &pdev->dev;
 	priv->wdev.min_timeout = 1;
-	priv->wdev.max_timeout = 65536 / clks_per_sec;
+	priv->wdev.max_timeout = DIV_BY_CLKS_PER_SEC(priv, 65536);
 	priv->wdev.timeout = min(priv->wdev.max_timeout, RWDT_DEFAULT_TIMEOUT);
 
 	platform_set_drvdata(pdev, priv);
-- 
2.11.0

[PATCH 5/5] watchdog: renesas_wdt: add another divider option

[Wolfram Sang]

If we set RWTCSRB to 0, we can gain 4096 as another divider value. This
is supported by all R-Car Gen2 and Gen3 devices which we aim to support.

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

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 599ba5aaa0536f..e3f204bb8802aa 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -23,21 +23,22 @@
 #define RWTCSRA_WOVF	BIT(4)
 #define RWTCSRA_WRFLG	BIT(5)
 #define RWTCSRA_TME	BIT(7)
+#define RWTCSRB		8
 
 #define RWDT_DEFAULT_TIMEOUT 60U
 
 /*
  * In probe, clk_rate is checked to be not more than 16 bit * biggest clock
- * divider (10 bits). d is only a factor to fully utilize the WDT counter and
+ * divider (12 bits). d is only a factor to fully utilize the WDT counter and
  * will not exceed its 16 bits. Thus, no overflow, we stay below 32 bits.
  */
 #define MUL_BY_CLKS_PER_SEC(p, d) \
 	DIV_ROUND_UP((d) * (p)->clk_rate, clk_divs[(p)->cks])
 
-/* d is 16 bit, clk_divs 10 bit -> no 32 bit overflow */
+/* d is 16 bit, clk_divs 12 bit -> no 32 bit overflow */
 #define DIV_BY_CLKS_PER_SEC(p, d) ((d) * clk_divs[(p)->cks] / (p)->clk_rate)
 
-static const unsigned int clk_divs[] = { 1, 4, 16, 32, 64, 128, 1024 };
+static const unsigned int clk_divs[] = { 1, 4, 16, 32, 64, 128, 1024, 4096 };
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
@@ -77,6 +78,7 @@ static int rwdt_start(struct watchdog_device *wdev)
 
 	clk_prepare_enable(priv->clk);
 
+	rwdt_write(priv, 0, RWTCSRB);
 	rwdt_write(priv, priv->cks, RWTCSRA);
 	rwdt_init_timeout(wdev);
 
-- 
2.11.0

Re: [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Jul 17, 2017 at 5:12 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Because the smallest clock divider we can select is 1, 'clks_per_sec'
> must be the same type as 'rate'.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -112,8 +112,7 @@ static int rwdt_probe(struct platform_device *pdev)
>  {
>         struct rwdt_priv *priv;
>         struct resource *res;
> -       unsigned long rate;
> -       unsigned int clks_per_sec;
> +       unsigned long rate, clks_per_sec;

If you make this change, you should also update rwdt_priv.clks_per_sec
(yes I know it's removed in a later patch in this series).

>         int ret, i;
>
>         priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);

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: [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow

[Wolfram Sang]

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

> > -       unsigned long rate;
> > -       unsigned int clks_per_sec;
> > +       unsigned long rate, clks_per_sec;
> 
> If you make this change, you should also update rwdt_priv.clks_per_sec
> (yes I know it's removed in a later patch in this series).

Right. I will change but also wait a bit for more comments.

Thanks!


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

Re: [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow

[Guenter Roeck]

On 07/18/2017 12:21 AM, Wolfram Sang wrote:
>>> -       unsigned long rate;
>>> -       unsigned int clks_per_sec;
>>> +       unsigned long rate, clks_per_sec;
>>
>> If you make this change, you should also update rwdt_priv.clks_per_sec
>> (yes I know it's removed in a later patch in this series).
> 
> Right. I will change but also wait a bit for more comments.
> 

I for my part don't have any. Feel free to add

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

to the series when you resend.

Thanks,
Guenter