| Message ID | 1559558161-31244-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] watchdog: renesas_wdt: Add a few cycles delay | expand |
On Mon, Jun 03, 2019 at 07:36:01PM +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> Despite minor nits: Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > +static void rwdt_wait(struct rwdt_priv *priv, unsigned int cycles) I'd think 'rwdt_wait_cycles' would be a more precise name for this function. > + unsigned long delays; Why not just 'delay'? > + /* Delay 3 cycles before disabling module clock */ I like the comments explaining why it is needed.
Hi Shimoda-san, Thanks for your work. On 2019-06-03 19:36:01 +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> Small nit bellow, with or without that addressed. Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > 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..525a1fe 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(struct rwdt_priv *priv, unsigned int cycles) > +{ > + unsigned long delays; Could this be unsigned int? It would still fit for a cycles number around 2000 and this change use 2 and 3 cycles. > + > + delays = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate); > + > + usleep_range(delays, 2 * delays); > +} > + > 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(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(priv, 3); > pm_runtime_put(wdev->parent); > > return 0; > -- > 2.7.4 >
Hi Wolfram-san, > From: Wolfram Sang, Sent: Monday, June 3, 2019 10:04 PM > On Mon, Jun 03, 2019 at 07:36:01PM +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> > > Despite minor nits: > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thank you for your review! > > +static void rwdt_wait(struct rwdt_priv *priv, unsigned int cycles) > > I'd think 'rwdt_wait_cycles' would be a more precise name for this > function. > > > + unsigned long delays; > > Why not just 'delay'? Ah, in v1, periods value existed. So, for consistence, I named "delay". But, I think we should just "delay" in this case. I'll fix it. > > + /* Delay 3 cycles before disabling module clock */ > > I like the comments explaining why it is needed. I think so! Best regards, Yoshihiro Shimoda
Hi Niklas-san, > From: Niklas Söderlund, Sent: Tuesday, June 4, 2019 1:23 AM > > Hi Shimoda-san, > > Thanks for your work. > > On 2019-06-03 19:36:01 +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> > > Small nit bellow, with or without that addressed. > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Thank you for your review! <snip> > > +static void rwdt_wait(struct rwdt_priv *priv, unsigned int cycles) > > +{ > > + unsigned long delays; > > Could this be unsigned int? It would still fit for a cycles number > around 2000 and this change use 2 and 3 cycles. I think so. I'll fix it on v3. Best regards, Yoshihiro Shimoda
diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c index 565dbc1..525a1fe 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(struct rwdt_priv *priv, unsigned int cycles) +{ + unsigned long delays; + + delays = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate); + + usleep_range(delays, 2 * delays); +} + 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(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(priv, 3); pm_runtime_put(wdev->parent); return 0;