* [PATCH 0/5] watchdog: renesas_wdt: improve precision
@ 2017-07-17 15:12 Wolfram Sang
2017-07-17 15:12 ` [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow Wolfram Sang
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Wolfram Sang @ 2017-07-17 15:12 UTC (permalink / raw)
To: linux-watchdog; +Cc: linux-renesas-soc, Takeshi Kihara, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow
2017-07-17 15:12 [PATCH 0/5] watchdog: renesas_wdt: improve precision Wolfram Sang
@ 2017-07-17 15:12 ` Wolfram Sang
2017-07-18 7:00 ` Geert Uytterhoeven
2017-07-17 15:12 ` [PATCH 2/5] watchdog: renesas_wdt: check rate also for upper limit Wolfram Sang
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Wolfram Sang @ 2017-07-17 15:12 UTC (permalink / raw)
To: linux-watchdog; +Cc: linux-renesas-soc, Takeshi Kihara, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/5] watchdog: renesas_wdt: check rate also for upper limit
2017-07-17 15:12 [PATCH 0/5] watchdog: renesas_wdt: improve precision Wolfram Sang
2017-07-17 15:12 ` [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow Wolfram Sang
@ 2017-07-17 15:12 ` Wolfram Sang
2017-07-17 15:12 ` [PATCH 3/5] watchdog: renesas_wdt: don't round closest with get_timeleft Wolfram Sang
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2017-07-17 15:12 UTC (permalink / raw)
To: linux-watchdog; +Cc: linux-renesas-soc, Takeshi Kihara, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/5] watchdog: renesas_wdt: don't round closest with get_timeleft
2017-07-17 15:12 [PATCH 0/5] watchdog: renesas_wdt: improve precision Wolfram Sang
2017-07-17 15:12 ` [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow Wolfram Sang
2017-07-17 15:12 ` [PATCH 2/5] watchdog: renesas_wdt: check rate also for upper limit Wolfram Sang
@ 2017-07-17 15:12 ` Wolfram Sang
2017-07-17 15:12 ` [PATCH 4/5] watchdog: renesas_wdt: apply better precision Wolfram Sang
2017-07-17 15:12 ` [PATCH 5/5] watchdog: renesas_wdt: add another divider option Wolfram Sang
4 siblings, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2017-07-17 15:12 UTC (permalink / raw)
To: linux-watchdog; +Cc: linux-renesas-soc, Takeshi Kihara, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/5] watchdog: renesas_wdt: apply better precision
2017-07-17 15:12 [PATCH 0/5] watchdog: renesas_wdt: improve precision Wolfram Sang
` (2 preceding siblings ...)
2017-07-17 15:12 ` [PATCH 3/5] watchdog: renesas_wdt: don't round closest with get_timeleft Wolfram Sang
@ 2017-07-17 15:12 ` Wolfram Sang
2017-07-17 15:12 ` [PATCH 5/5] watchdog: renesas_wdt: add another divider option Wolfram Sang
4 siblings, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2017-07-17 15:12 UTC (permalink / raw)
To: linux-watchdog; +Cc: linux-renesas-soc, Takeshi Kihara, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 5/5] watchdog: renesas_wdt: add another divider option
2017-07-17 15:12 [PATCH 0/5] watchdog: renesas_wdt: improve precision Wolfram Sang
` (3 preceding siblings ...)
2017-07-17 15:12 ` [PATCH 4/5] watchdog: renesas_wdt: apply better precision Wolfram Sang
@ 2017-07-17 15:12 ` Wolfram Sang
4 siblings, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2017-07-17 15:12 UTC (permalink / raw)
To: linux-watchdog; +Cc: linux-renesas-soc, Takeshi Kihara, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow
2017-07-17 15:12 ` [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow Wolfram Sang
@ 2017-07-18 7:00 ` Geert Uytterhoeven
2017-07-18 7:21 ` Wolfram Sang
0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2017-07-18 7:00 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Linux Watchdog Mailing List, Linux-Renesas, Takeshi Kihara
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow
2017-07-18 7:00 ` Geert Uytterhoeven
@ 2017-07-18 7:21 ` Wolfram Sang
2017-07-19 2:57 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: Wolfram Sang @ 2017-07-18 7:21 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Wolfram Sang, Linux Watchdog Mailing List, Linux-Renesas,
Takeshi Kihara
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow
2017-07-18 7:21 ` Wolfram Sang
@ 2017-07-19 2:57 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2017-07-19 2:57 UTC (permalink / raw)
To: Wolfram Sang, Geert Uytterhoeven
Cc: Wolfram Sang, Linux Watchdog Mailing List, Linux-Renesas,
Takeshi Kihara
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-07-19 2:57 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-17 15:12 [PATCH 0/5] watchdog: renesas_wdt: improve precision Wolfram Sang
2017-07-17 15:12 ` [PATCH 1/5] watchdog: renesas_wdt: avoid (theoretical) type overflow Wolfram Sang
2017-07-18 7:00 ` Geert Uytterhoeven
2017-07-18 7:21 ` Wolfram Sang
2017-07-19 2:57 ` Guenter Roeck
2017-07-17 15:12 ` [PATCH 2/5] watchdog: renesas_wdt: check rate also for upper limit Wolfram Sang
2017-07-17 15:12 ` [PATCH 3/5] watchdog: renesas_wdt: don't round closest with get_timeleft Wolfram Sang
2017-07-17 15:12 ` [PATCH 4/5] watchdog: renesas_wdt: apply better precision Wolfram Sang
2017-07-17 15:12 ` [PATCH 5/5] watchdog: renesas_wdt: add another divider option Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).