| Message ID | 20220104181249.3174-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/4] watchdog: rzg2l_wdt: Fix 32bit overflow issue | expand |
Hi Biju, On Tue, Jan 4, 2022 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > This patch uses the force reset(WDTRSTB) for triggering WDT reset for > restart callback. This method is faster compared to the overflow method > for triggering watchdog reset. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -21,8 +21,11 @@ > #define WDTSET 0x04 > #define WDTTIM 0x08 > #define WDTINT 0x0C > +#define PECR 0x10 > +#define PEEN 0x14 > #define WDTCNT_WDTEN BIT(0) > #define WDTINT_INTDISP BIT(0) > +#define PEEN_FORCE_RST BIT(0) PEEN_FORCE, as this can trigger either a reset or interrupt? > > #define WDT_DEFAULT_TIMEOUT 60U > > @@ -116,15 +119,11 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, > { > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > - /* Reset the module before we modify any register */ > - reset_control_reset(priv->rstc); > - pm_runtime_get_sync(wdev->parent); Why are these no longer needed? Because .probe() takes care of that? Then why do .start() and .stop() have reset and Runtime PM handling, too? > - > - /* smallest counter value to reboot soon */ > - rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET); > + /* Generate Reset (WDTRSTB) Signal */ ... on parity error > + rzg2l_wdt_write(priv, 0, PECR); > > - /* Enable watchdog timer*/ > - rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT); > + /* Force reset (WDTRSTB) */ s/reset/parity error/ > + rzg2l_wdt_write(priv, PEEN_FORCE_RST, PEEN); > > return 0; > } 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 Geert, Thanks for the feedback. > Subject: Re: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT > reset > > Hi Biju, > > On Tue, Jan 4, 2022 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > This patch uses the force reset(WDTRSTB) for triggering WDT reset for > > restart callback. This method is faster compared to the overflow > > method for triggering watchdog reset. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/watchdog/rzg2l_wdt.c > > +++ b/drivers/watchdog/rzg2l_wdt.c > > @@ -21,8 +21,11 @@ > > #define WDTSET 0x04 > > #define WDTTIM 0x08 > > #define WDTINT 0x0C > > +#define PECR 0x10 > > +#define PEEN 0x14 > > #define WDTCNT_WDTEN BIT(0) > > #define WDTINT_INTDISP BIT(0) > > +#define PEEN_FORCE_RST BIT(0) > > PEEN_FORCE, as this can trigger either a reset or interrupt? Yes you are correct, it should be PEEN_FORCE. 1 --> Force reset 0 --> Based on operation of parity error register it can trigger a reset or interrupt. > > > > > #define WDT_DEFAULT_TIMEOUT 60U > > > > @@ -116,15 +119,11 @@ static int rzg2l_wdt_restart(struct > > watchdog_device *wdev, { > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > - /* Reset the module before we modify any register */ > > - reset_control_reset(priv->rstc); > > - pm_runtime_get_sync(wdev->parent); > > Why are these no longer needed? Because .probe() takes care of that? This code is added to modify WDTSET register. Once watchdog is started, On the fly, we won't be able to update WDTSET register. By resetting(assert/deassert) the module we can update the WDTSET register. It takes 34 millisec to trigger reset. But with PEEN_FORCE, on the fly we can update register and it immediately triggers reset. Then why do .start() and .stop() have > reset and Runtime PM handling, too? .start-> turns on the Clocks using Runtime PM. We cannot Update WDTSET/WDTEN registers, once watchdog is started. Adding reset and Runtime PM handling in .stop, allows to stop the watchdog. .stop-> turns off the clock using Runtime PM and does the reset(assert/deassert) of the module in order to modify WDTSET/WDTEN registers after stop operation. May be I should keep pm_runtime_get_sync(wdev->parent) in restart callback, To turn on the clocks, If someone calls stop followed by restart Regards, Biju > > > - > > - /* smallest counter value to reboot soon */ > > - rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET); > > + /* Generate Reset (WDTRSTB) Signal */ > > ... on parity error You are correct, Force parity error causes reset generation. OK will update the comment. > > > + rzg2l_wdt_write(priv, 0, PECR); > > > > - /* Enable watchdog timer*/ > > - rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT); > > + /* Force reset (WDTRSTB) */ > > s/reset/parity error/ OK. Will update the comment. Regards, Biju > > > + rzg2l_wdt_write(priv, PEEN_FORCE_RST, PEEN); > > > > return 0; > > } > > 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 Biju, On Mon, Jan 10, 2022 at 11:51 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT > > reset > > On Tue, Jan 4, 2022 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > This patch uses the force reset(WDTRSTB) for triggering WDT reset for > > > restart callback. This method is faster compared to the overflow > > > method for triggering watchdog reset. > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- a/drivers/watchdog/rzg2l_wdt.c > > > +++ b/drivers/watchdog/rzg2l_wdt.c > > > #define WDT_DEFAULT_TIMEOUT 60U > > > > > > @@ -116,15 +119,11 @@ static int rzg2l_wdt_restart(struct > > > watchdog_device *wdev, { > > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > > > - /* Reset the module before we modify any register */ > > > - reset_control_reset(priv->rstc); > > > - pm_runtime_get_sync(wdev->parent); > > > > Why are these no longer needed? Because .probe() takes care of that? > > This code is added to modify WDTSET register. > Once watchdog is started, On the fly, we won't be able to > update WDTSET register. By resetting(assert/deassert) the module > we can update the WDTSET register. It takes 34 millisec to trigger reset. > > But with PEEN_FORCE, on the fly we can update register and it immediately triggers reset. > > Then why do .start() and .stop() have > > reset and Runtime PM handling, too? > > .start-> turns on the Clocks using Runtime PM. > > We cannot Update WDTSET/WDTEN registers, once watchdog is started. > Adding reset and Runtime PM handling in .stop, allows to stop the watchdog. > > .stop-> turns off the clock using Runtime PM and does the reset(assert/deassert) of the module > in order to modify WDTSET/WDTEN registers after stop operation. > > May be I should keep pm_runtime_get_sync(wdev->parent) in restart callback, > To turn on the clocks, If someone calls stop followed by restart I'm still confused: .probe() turns the clock on through Runtime PM, so it's always running. Calling .start() merely increases the usage count, and a subsequent .stop() will decrease it again. But the clock keeps on running? What am I missing? 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 Geert, > Subject: Re: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT > reset > > Hi Biju, > > On Mon, Jan 10, 2022 at 11:51 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: Re: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for > > > WDT reset On Tue, Jan 4, 2022 at 7:12 PM Biju Das > > > <biju.das.jz@bp.renesas.com> > > > wrote: > > > > This patch uses the force reset(WDTRSTB) for triggering WDT reset > > > > for restart callback. This method is faster compared to the > > > > overflow method for triggering watchdog reset. > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > > --- a/drivers/watchdog/rzg2l_wdt.c > > > > +++ b/drivers/watchdog/rzg2l_wdt.c > > > > > #define WDT_DEFAULT_TIMEOUT 60U > > > > > > > > @@ -116,15 +119,11 @@ static int rzg2l_wdt_restart(struct > > > > watchdog_device *wdev, { > > > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > > > > > - /* Reset the module before we modify any register */ > > > > - reset_control_reset(priv->rstc); > > > > - pm_runtime_get_sync(wdev->parent); > > > > > > Why are these no longer needed? Because .probe() takes care of that? > > > > This code is added to modify WDTSET register. > > Once watchdog is started, On the fly, we won't be able to update > > WDTSET register. By resetting(assert/deassert) the module we can > > update the WDTSET register. It takes 34 millisec to trigger reset. > > > > But with PEEN_FORCE, on the fly we can update register and it > immediately triggers reset. > > > > Then why do .start() and .stop() have > > > reset and Runtime PM handling, too? > > > > .start-> turns on the Clocks using Runtime PM. > > > > We cannot Update WDTSET/WDTEN registers, once watchdog is started. > > Adding reset and Runtime PM handling in .stop, allows to stop the > watchdog. > > > > .stop-> turns off the clock using Runtime PM and does the > reset(assert/deassert) of the module > > in order to modify WDTSET/WDTEN registers after stop operation. > > > > May be I should keep pm_runtime_get_sync(wdev->parent) in restart > > callback, To turn on the clocks, If someone calls stop followed by > > restart > > I'm still confused: .probe() turns the clock on through Runtime PM, so > it's always running. Calling .start() merely increases the usage count, > and a subsequent .stop() will decrease it again. But the clock keeps on > running? What am I missing? Sorry for the confusion. On the next patch #3, I am removing clock on through Runtime PM from probe, In order to get balanced PM usage count with start/stop operation. So after patch#3, .start() turns on the clock and .stop() turns off the clock. Regards, Biju
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index 96f2a018ab62..b02ecfff5299 100644 --- a/drivers/watchdog/rzg2l_wdt.c +++ b/drivers/watchdog/rzg2l_wdt.c @@ -21,8 +21,11 @@ #define WDTSET 0x04 #define WDTTIM 0x08 #define WDTINT 0x0C +#define PECR 0x10 +#define PEEN 0x14 #define WDTCNT_WDTEN BIT(0) #define WDTINT_INTDISP BIT(0) +#define PEEN_FORCE_RST BIT(0) #define WDT_DEFAULT_TIMEOUT 60U @@ -116,15 +119,11 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, { struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); - /* Reset the module before we modify any register */ - reset_control_reset(priv->rstc); - pm_runtime_get_sync(wdev->parent); - - /* smallest counter value to reboot soon */ - rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET); + /* Generate Reset (WDTRSTB) Signal */ + rzg2l_wdt_write(priv, 0, PECR); - /* Enable watchdog timer*/ - rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT); + /* Force reset (WDTRSTB) */ + rzg2l_wdt_write(priv, PEEN_FORCE_RST, PEEN); return 0; }
This patch uses the force reset(WDTRSTB) for triggering WDT reset for restart callback. This method is faster compared to the overflow method for triggering watchdog reset. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- V2->v3: * Patch reordering from patch 4->patch 2 * Updated the commit description. V1->V2: * Updated the commit description. --- drivers/watchdog/rzg2l_wdt.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)