linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] watchdog: renesas_wdt: Add a few cycles delay
@ 2019-06-03  9:25 Yoshihiro Shimoda
  2019-06-03  9:58 ` Geert Uytterhoeven
  0 siblings, 1 reply; 7+ messages in thread
From: Yoshihiro Shimoda @ 2019-06-03  9:25 UTC (permalink / raw)
  To: wim, linux; +Cc: linux-watchdog, linux-renesas-soc, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] watchdog: renesas_wdt: Add a few cycles delay
  2019-06-03  9:25 [PATCH] watchdog: renesas_wdt: Add a few cycles delay Yoshihiro Shimoda
@ 2019-06-03  9:58 ` Geert Uytterhoeven
  2019-06-03 10:28   ` Yoshihiro Shimoda
  0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2019-06-03  9:58 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Wim Van Sebroeck, Guenter Roeck, Linux Watchdog Mailing List,
	Linux-Renesas

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] watchdog: renesas_wdt: Add a few cycles delay
  2019-06-03  9:58 ` Geert Uytterhoeven
@ 2019-06-03 10:28   ` Yoshihiro Shimoda
  2019-06-03 11:04     ` Geert Uytterhoeven
  0 siblings, 1 reply; 7+ messages in thread
From: Yoshihiro Shimoda @ 2019-06-03 10:28 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Wim Van Sebroeck, Guenter Roeck, Linux Watchdog Mailing List,
	Linux-Renesas

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] watchdog: renesas_wdt: Add a few cycles delay
  2019-06-03 10:28   ` Yoshihiro Shimoda
@ 2019-06-03 11:04     ` Geert Uytterhoeven
  2019-06-05  4:56       ` Yoshihiro Shimoda
  0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2019-06-03 11:04 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Wim Van Sebroeck, Guenter Roeck, Linux Watchdog Mailing List,
	Linux-Renesas

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] watchdog: renesas_wdt: Add a few cycles delay
  2019-06-03 11:04     ` Geert Uytterhoeven
@ 2019-06-05  4:56       ` Yoshihiro Shimoda
  0 siblings, 0 replies; 7+ messages in thread
From: Yoshihiro Shimoda @ 2019-06-05  4:56 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Wim Van Sebroeck, Guenter Roeck, Linux Watchdog Mailing List,
	Linux-Renesas

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] watchdog: renesas_wdt: Add a few cycles delay
  2019-06-05  5:04 Yoshihiro Shimoda
@ 2019-06-05  5:21 ` Yoshihiro Shimoda
  0 siblings, 0 replies; 7+ messages in thread
From: Yoshihiro Shimoda @ 2019-06-05  5:21 UTC (permalink / raw)
  To: Yoshihiro Shimoda, wim, linux; +Cc: linux-watchdog, linux-renesas-soc

> 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] watchdog: renesas_wdt: Add a few cycles delay
@ 2019-06-05  5:04 Yoshihiro Shimoda
  2019-06-05  5:21 ` Yoshihiro Shimoda
  0 siblings, 1 reply; 7+ messages in thread
From: Yoshihiro Shimoda @ 2019-06-05  5:04 UTC (permalink / raw)
  To: wim, linux; +Cc: linux-watchdog, linux-renesas-soc, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-06-05  5:21 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-03  9:25 [PATCH] watchdog: renesas_wdt: Add a few cycles delay Yoshihiro Shimoda
2019-06-03  9:58 ` Geert Uytterhoeven
2019-06-03 10:28   ` Yoshihiro Shimoda
2019-06-03 11:04     ` Geert Uytterhoeven
2019-06-05  4:56       ` Yoshihiro Shimoda
2019-06-05  5:04 Yoshihiro Shimoda
2019-06-05  5:21 ` Yoshihiro Shimoda

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 NNTP newsgroup(s).