| Message ID | 1559553957-5764-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: renesas_wdt: Add a few cycles delay | expand |
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
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
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
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
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;
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(+)