| Message ID | 20220223160100.23543-6-biju.das.jz@bp.renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RZG2L_WDT Fixes and Improvements | expand |
Hi Biju, On Wed, Feb 23, 2022 at 5:01 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(ie, Generate Reset (WDTRSTB) Signal on > parity error)is faster compared to the overflow method for triggering > watchdog reset. > > Overflow method: > reboot: Restarting system > Reboot failed -- System halted > NOTICE: BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c > > Parity error method: > reboot: Restarting system > NOTICE: BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c > > 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 > @@ -117,17 +120,14 @@ 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); > - I think this part belongs in patch 4/6. > clk_prepare_enable(priv->pclk); > clk_prepare_enable(priv->osc_clk); > > - /* 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 parity error */ > + rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > > return 0; The rest LGTM, to 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
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v4 5/6] watchdog: rzg2l_wdt: Use force reset for WDT > reset > > Hi Biju, > > On Wed, Feb 23, 2022 at 5:01 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(ie, Generate Reset (WDTRSTB) Signal on > > parity error)is faster compared to the overflow method for triggering > > watchdog reset. > > > > Overflow method: > > reboot: Restarting system > > Reboot failed -- System halted > > NOTICE: BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c > > > > Parity error method: > > reboot: Restarting system > > NOTICE: BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c > > > > 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 > > > @@ -117,17 +120,14 @@ 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); > > - > > I think this part belongs in patch 4/6. For Overflow method we need to reset the module, so that we can update WDTSET register to change the timeout value from 60sec to 43.69 msec, so that reboot can occur after 43.69 msec which corresponds to counter value of 1. Where as on parity error case, Generating parity error immediately trigger the reboot. Here we don't need to reset the module, for Generating parity error, that is the reason it got removed in this patch. Regards, Biju > > > clk_prepare_enable(priv->pclk); > > clk_prepare_enable(priv->osc_clk); > > > > - /* 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 parity error */ > > + rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > > > > return 0; > > The rest LGTM, to > 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
Hi Biju, On Thu, Feb 24, 2022 at 10:51 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v4 5/6] watchdog: rzg2l_wdt: Use force reset for WDT > > reset > > On Wed, Feb 23, 2022 at 5:01 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(ie, Generate Reset (WDTRSTB) Signal on > > > parity error)is faster compared to the overflow method for triggering > > > watchdog reset. > > > > > > Overflow method: > > > reboot: Restarting system > > > Reboot failed -- System halted > > > NOTICE: BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c > > > > > > Parity error method: > > > reboot: Restarting system > > > NOTICE: BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c > > > > > > 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 > > > > > @@ -117,17 +120,14 @@ 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); > > > - > > > > I think this part belongs in patch 4/6. > > For Overflow method we need to reset the module, so that we can > update WDTSET register to change the timeout value from 60sec to 43.69 msec, > so that reboot can occur after 43.69 msec which corresponds to counter value of 1. > > Where as on parity error case, Generating parity error > immediately trigger the reboot. Here we don't need to reset the module, > for Generating parity error, that is the reason it got removed in this patch. Right, so please ignore my comment. 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
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index 73b667ed3e99..4e7107655cc2 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 BIT(0) #define WDT_DEFAULT_TIMEOUT 60U @@ -117,17 +120,14 @@ 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); - clk_prepare_enable(priv->pclk); clk_prepare_enable(priv->osc_clk); - /* 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 parity error */ + rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); return 0; }
This patch uses the force reset(WDTRSTB) for triggering WDT reset for restart callback. This method(ie, Generate Reset (WDTRSTB) Signal on parity error)is faster compared to the overflow method for triggering watchdog reset. Overflow method: reboot: Restarting system Reboot failed -- System halted NOTICE: BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c Parity error method: reboot: Restarting system NOTICE: BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- V3->V4: * Renamed PEEN_FORCE_RST->PEEN_FORCE * Updated comments with parity error description * Updated commit description V2->v3: * Patch reordering from patch 4->patch 2 * Updated the commit description. V1->V2: * Updated the commit description. --- drivers/watchdog/rzg2l_wdt.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)