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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
>
> 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
