[PATCH] watchdog: renesas_wdt: Add a few cycles delay

[PATCH] watchdog: renesas_wdt: Add a few cycles delay

[Yoshihiro Shimoda]

According to the hardware manual of R-Car Gen2 and Gen3,
software should wait a few RLCK cycles as following:
 - Delay 2 cycles before setting watchdog counter.
 - Delay 3 cycles before disabling module clock.

So, this patch adds such delays.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 Changes from v2 (https://patchwork.kernel.org/patch/10972721/):
 - Rename the wait function name.
 - Rename the variable name in the wait function.
 - Change variable type.
 - Add Wolfram-san and Niklas-san's Reviewed-by.

 Changes from v1 (https://patchwork.kernel.org/patch/10972641/):
 - Change formula to improve accuracy.
 - Add Geert-san's Reviewed-by.

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

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 565dbc1..0cfc0e9 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -7,6 +7,7 @@
  */
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -70,6 +71,15 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
 	return 0;
 }
 
+static void rwdt_wait_cycles(struct rwdt_priv *priv, unsigned int cycles)
+{
+	unsigned int delay;
+
+	delay = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);
+
+	usleep_range(delay, 2 * delay);
+}
+
 static int rwdt_start(struct watchdog_device *wdev)
 {
 	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
@@ -80,6 +90,8 @@ static int rwdt_start(struct watchdog_device *wdev)
 	/* Stop the timer before we modify any register */
 	val = readb_relaxed(priv->base + RWTCSRA) & ~RWTCSRA_TME;
 	rwdt_write(priv, val, RWTCSRA);
+	/* Delay 2 cycles before setting watchdog counter */
+	rwdt_wait_cycles(priv, 2);
 
 	rwdt_init_timeout(wdev);
 	rwdt_write(priv, priv->cks, RWTCSRA);
@@ -98,6 +110,8 @@ static int rwdt_stop(struct watchdog_device *wdev)
 	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
 
 	rwdt_write(priv, priv->cks, RWTCSRA);
+	/* Delay 3 cycles before disabling module clock */
+	rwdt_wait_cycles(priv, 3);
 	pm_runtime_put(wdev->parent);
 
 	return 0;
-- 
2.7.4

RE: [PATCH] watchdog: renesas_wdt: Add a few cycles delay

[Yoshihiro Shimoda]

> From: Yoshihiro Shimoda, Sent: Wednesday, June 5, 2019 2:04 PM
> Subject: [PATCH] watchdog: renesas_wdt: Add a few cycles delay

Oops, I should have submitted as "PATCH v3" on the subject...

Best regards,
Yoshihiro Shimoda

Re: [PATCH v3] watchdog: renesas_wdt: Add a few cycles delay

[Guenter Roeck]

[Note: I updated the subject to "PATCH v3"]

On Wed, Jun 05, 2019 at 02:04:00PM +0900, Yoshihiro Shimoda wrote:
> According to the hardware manual of R-Car Gen2 and Gen3,
> software should wait a few RLCK cycles as following:
>  - Delay 2 cycles before setting watchdog counter.
>  - Delay 3 cycles before disabling module clock.
> 
> So, this patch adds such delays.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

> ---
>  Changes from v2 (https://patchwork.kernel.org/patch/10972721/):
>  - Rename the wait function name.
>  - Rename the variable name in the wait function.
>  - Change variable type.
>  - Add Wolfram-san and Niklas-san's Reviewed-by.
> 
>  Changes from v1 (https://patchwork.kernel.org/patch/10972641/):
>  - Change formula to improve accuracy.
>  - Add Geert-san's Reviewed-by.
> 
>  drivers/watchdog/renesas_wdt.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 565dbc1..0cfc0e9 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -7,6 +7,7 @@
>   */
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -70,6 +71,15 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
>  	return 0;
>  }
>  
> +static void rwdt_wait_cycles(struct rwdt_priv *priv, unsigned int cycles)
> +{
> +	unsigned int delay;
> +
> +	delay = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);
> +
> +	usleep_range(delay, 2 * delay);
> +}
> +
>  static int rwdt_start(struct watchdog_device *wdev)
>  {
>  	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> @@ -80,6 +90,8 @@ static int rwdt_start(struct watchdog_device *wdev)
>  	/* Stop the timer before we modify any register */
>  	val = readb_relaxed(priv->base + RWTCSRA) & ~RWTCSRA_TME;
>  	rwdt_write(priv, val, RWTCSRA);
> +	/* Delay 2 cycles before setting watchdog counter */
> +	rwdt_wait_cycles(priv, 2);
>  
>  	rwdt_init_timeout(wdev);
>  	rwdt_write(priv, priv->cks, RWTCSRA);
> @@ -98,6 +110,8 @@ static int rwdt_stop(struct watchdog_device *wdev)
>  	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
>  
>  	rwdt_write(priv, priv->cks, RWTCSRA);
> +	/* Delay 3 cycles before disabling module clock */
> +	rwdt_wait_cycles(priv, 3);
>  	pm_runtime_put(wdev->parent);
>  
>  	return 0;

Re: [PATCH v3] watchdog: renesas_wdt: Add a few cycles delay

[Simon Horman]

On Fri, Jun 07, 2019 at 10:44:11AM -0700, Guenter Roeck wrote:
> [Note: I updated the subject to "PATCH v3"]
> 
> On Wed, Jun 05, 2019 at 02:04:00PM +0900, Yoshihiro Shimoda wrote:
> > According to the hardware manual of R-Car Gen2 and Gen3,
> > software should wait a few RLCK cycles as following:
> >  - Delay 2 cycles before setting watchdog counter.
> >  - Delay 3 cycles before disabling module clock.
> > 
> > So, this patch adds such delays.
> > 
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

RE: [PATCH] watchdog: renesas_wdt: Add a few cycles delay

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Monday, June 3, 2019 8:05 PM
<snip>
> > > > +{
> > > > +       unsigned long periods, delays;
> > > > +
> > > > +       periods = DIV_ROUND_UP(priv->clk_rate, cycles);
> > >
> > > Shouldn't the above be a division with rounding down (i.e. a plain C
> > > division), instead of a division with rounding up?
> >
> > I have no idea which is the correct way (rounding down vs rounding up here).
> > At least, I tried to use rounding down before submitting patch and then
> > the result seemed the same. So, I submitted this patch with rounding up
> > (because the next step also used rounding up...).
> 
> If you round up periods, it will decrease the delay, which may become
> too small.
> If you round up delays, it will increase the delay, which doesn't hurt.

Thank you for your explanation in detail! I understood it.

Best regards,
Yoshihiro Shimoda

Re: [PATCH] watchdog: renesas_wdt: Add a few cycles delay

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Jun 3, 2019 at 12:28 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Monday, June 3, 2019 6:59 PM
> > On Mon, Jun 3, 2019 at 11:31 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > According to the hardware manual of R-Car Gen2 and Gen3,
> > > software should wait a few RLCK cycles as following:
> > >  - Delay 2 cycles before setting watchdog counter.
> > >  - Delay 3 cycles before disabling module clock.
> > >
> > > So, this patch adds such delays.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> > Thanks for your patch!
>
> Thank you for your review!
>
> > > --- a/drivers/watchdog/renesas_wdt.c
> > > +++ b/drivers/watchdog/renesas_wdt.c
> >
> > > @@ -70,6 +71,16 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
> > >         return 0;
> > >  }
> > >
> > > +static void rwdt_wait(struct rwdt_priv *priv, unsigned long cycles)
> >
> > "unsigned int" should be sufficiently large.
>
> I got it.
>
> > > +{
> > > +       unsigned long periods, delays;
> > > +
> > > +       periods = DIV_ROUND_UP(priv->clk_rate, cycles);
> >
> > Shouldn't the above be a division with rounding down (i.e. a plain C
> > division), instead of a division with rounding up?
>
> I have no idea which is the correct way (rounding down vs rounding up here).
> At least, I tried to use rounding down before submitting patch and then
> the result seemed the same. So, I submitted this patch with rounding up
> (because the next step also used rounding up...).

If you round up periods, it will decrease the delay, which may become
too small.
If you round up delays, it will increase the delay, which doesn't hurt.

> > > +       delays = DIV_ROUND_UP(1000000UL, periods);
> >
> > Given cycles is always a small number, accuracy can be improved, and one
> > division can be avoided, by calculation this as:
> >
> >     delays = DIV_ROUND_UP(cycles * 1000000 /  priv->clk_rate);
>
> Thank you for your suggest! I think so.
> It should be "s/ \//,/" like below though :)

>         delays = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);

Oops, indeed. Sorry for that silly mistake.

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] watchdog: renesas_wdt: Add a few cycles delay

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Monday, June 3, 2019 6:59 PM
> 
> Hi Shimoda-san,
> 
> On Mon, Jun 3, 2019 at 11:31 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > According to the hardware manual of R-Car Gen2 and Gen3,
> > software should wait a few RLCK cycles as following:
> >  - Delay 2 cycles before setting watchdog counter.
> >  - Delay 3 cycles before disabling module clock.
> >
> > So, this patch adds such delays.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Thanks for your patch!

Thank you for your review!

> > --- a/drivers/watchdog/renesas_wdt.c
> > +++ b/drivers/watchdog/renesas_wdt.c
> 
> > @@ -70,6 +71,16 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
> >         return 0;
> >  }
> >
> > +static void rwdt_wait(struct rwdt_priv *priv, unsigned long cycles)
> 
> "unsigned int" should be sufficiently large.

I got it.

> > +{
> > +       unsigned long periods, delays;
> > +
> > +       periods = DIV_ROUND_UP(priv->clk_rate, cycles);
> 
> Shouldn't the above be a division with rounding down (i.e. a plain C
> division), instead of a division with rounding up?

I have no idea which is the correct way (rounding down vs rounding up here).
At least, I tried to use rounding down before submitting patch and then
the result seemed the same. So, I submitted this patch with rounding up
(because the next step also used rounding up...).

> > +       delays = DIV_ROUND_UP(1000000UL, periods);
> 
> Given cycles is always a small number, accuracy can be improved, and one
> division can be avoided, by calculation this as:
> 
>     delays = DIV_ROUND_UP(cycles * 1000000 /  priv->clk_rate);

Thank you for your suggest! I think so.
It should be "s/ \//,/" like below though :)

	delays = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);

> > +
> > +       usleep_range(delays, 2 * delays);
> > +}
> 
> The rest looks good to me, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thank you for your Reviewed-by tag! I'll submit v2 patch later.

Best regards,
Yoshihiro Shimoda

> 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] watchdog: renesas_wdt: Add a few cycles delay

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Jun 3, 2019 at 11:31 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> According to the hardware manual of R-Car Gen2 and Gen3,
> software should wait a few RLCK cycles as following:
>  - Delay 2 cycles before setting watchdog counter.
>  - Delay 3 cycles before disabling module clock.
>
> So, this patch adds such delays.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks for your patch!

> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c

> @@ -70,6 +71,16 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
>         return 0;
>  }
>
> +static void rwdt_wait(struct rwdt_priv *priv, unsigned long cycles)

"unsigned int" should be sufficiently large.

> +{
> +       unsigned long periods, delays;
> +
> +       periods = DIV_ROUND_UP(priv->clk_rate, cycles);

Shouldn't the above be a division with rounding down (i.e. a plain C
division), instead of a division with rounding up?

> +       delays = DIV_ROUND_UP(1000000UL, periods);

Given cycles is always a small number, accuracy can be improved, and one
division can be avoided, by calculation this as:

    delays = DIV_ROUND_UP(cycles * 1000000 /  priv->clk_rate);

> +
> +       usleep_range(delays, 2 * delays);
> +}

The rest looks good to me, so
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

[PATCH] watchdog: renesas_wdt: Add a few cycles delay

[Yoshihiro Shimoda]

According to the hardware manual of R-Car Gen2 and Gen3,
software should wait a few RLCK cycles as following:
 - Delay 2 cycles before setting watchdog counter.
 - Delay 3 cycles before disabling module clock.

So, this patch adds such delays.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/watchdog/renesas_wdt.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 565dbc1..e632b56 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -7,6 +7,7 @@
  */
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -70,6 +71,16 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
 	return 0;
 }
 
+static void rwdt_wait(struct rwdt_priv *priv, unsigned long cycles)
+{
+	unsigned long periods, delays;
+
+	periods = DIV_ROUND_UP(priv->clk_rate, cycles);
+	delays = DIV_ROUND_UP(1000000UL, periods);
+
+	usleep_range(delays, 2 * delays);
+}
+
 static int rwdt_start(struct watchdog_device *wdev)
 {
 	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
@@ -80,6 +91,8 @@ static int rwdt_start(struct watchdog_device *wdev)
 	/* Stop the timer before we modify any register */
 	val = readb_relaxed(priv->base + RWTCSRA) & ~RWTCSRA_TME;
 	rwdt_write(priv, val, RWTCSRA);
+	/* Delay 2 cycles before setting watchdog counter */
+	rwdt_wait(priv, 2);
 
 	rwdt_init_timeout(wdev);
 	rwdt_write(priv, priv->cks, RWTCSRA);
@@ -98,6 +111,8 @@ static int rwdt_stop(struct watchdog_device *wdev)
 	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
 
 	rwdt_write(priv, priv->cks, RWTCSRA);
+	/* Delay 3 cycles before disabling module clock */
+	rwdt_wait(priv, 3);
 	pm_runtime_put(wdev->parent);
 
 	return 0;
-- 
2.7.4