| Message ID | 20211211212617.19639-3-biju.das.jz@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] watchdog: rzg2l_wdt: Fix 32bit overflow issue | expand |
On 12/11/21 1:26 PM, Biju Das wrote: > Add support for set_timeout() callback. > This needs an explanation. WDIOF_SETTIMEOUT is, after all, already supported. I can see that 'count' is not recalculated, so that is one of the reasons. However, it also needs to be explained why it is necessary to stop and restart the watchdog. > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > drivers/watchdog/rzg2l_wdt.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > index 58fe4efd9a89..c81b9dd05e63 100644 > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -117,6 +117,15 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) > return 0; > } > > +static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout) > +{ > + wdev->timeout = timeout; > + rzg2l_wdt_stop(wdev); > + rzg2l_wdt_start(wdev); Is it necessary to stop and restart the timeout, or would it be sufficient to call rza_wdt_calc_timeout() ? If it is necessary, please add a comment describing the reason. Either case, calling rzg2l_wdt_start() unconditionally is wrong because the watchdog might be stopped. Guenter > + > + return 0; > +} > + > static int rzg2l_wdt_restart(struct watchdog_device *wdev, > unsigned long action, void *data) > { > @@ -160,6 +169,7 @@ static const struct watchdog_ops rzg2l_wdt_ops = { > .start = rzg2l_wdt_start, > .stop = rzg2l_wdt_stop, > .ping = rzg2l_wdt_ping, > + .set_timeout = rzg2l_wdt_set_timeout, > .restart = rzg2l_wdt_restart, > }; > >
On 12/11/21 1:38 PM, Guenter Roeck wrote: > On 12/11/21 1:26 PM, Biju Das wrote: >> Add support for set_timeout() callback. >> > This needs an explanation. WDIOF_SETTIMEOUT is, after all, > already supported. I can see that 'count' is not recalculated, > so that is one of the reasons. However, it also needs to be explained > why it is necessary to stop and restart the watchdog. > >> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >> --- >> drivers/watchdog/rzg2l_wdt.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c >> index 58fe4efd9a89..c81b9dd05e63 100644 >> --- a/drivers/watchdog/rzg2l_wdt.c >> +++ b/drivers/watchdog/rzg2l_wdt.c >> @@ -117,6 +117,15 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) >> return 0; >> } >> +static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout) >> +{ >> + wdev->timeout = timeout; >> + rzg2l_wdt_stop(wdev); >> + rzg2l_wdt_start(wdev); > > Is it necessary to stop and restart the timeout, or would it be sufficient > to call rza_wdt_calc_timeout() ? If it is necessary, please add a comment That should have been rzg2l_wdt_init_timeout(). Also, as mentioned in the second patch of the series, the return value of rzg2l_wdt_start() needs to be checked if the watchdog needs to be stopped and restarted. Thanks, Guenter > describing the reason. > > Either case, calling rzg2l_wdt_start() unconditionally is wrong because > the watchdog might be stopped. > > Guenter > >> + >> + return 0; >> +} >> + >> static int rzg2l_wdt_restart(struct watchdog_device *wdev, >> unsigned long action, void *data) >> { >> @@ -160,6 +169,7 @@ static const struct watchdog_ops rzg2l_wdt_ops = { >> .start = rzg2l_wdt_start, >> .stop = rzg2l_wdt_stop, >> .ping = rzg2l_wdt_ping, >> + .set_timeout = rzg2l_wdt_set_timeout, >> .restart = rzg2l_wdt_restart, >> }; >> >
Hi Guenter Roeck, > Subject: Re: [PATCH 3/4] watchdog: rzg2l_wdt: Add set_timeout callback > > On 12/11/21 1:38 PM, Guenter Roeck wrote: > > On 12/11/21 1:26 PM, Biju Das wrote: > >> Add support for set_timeout() callback. > >> > > This needs an explanation. WDIOF_SETTIMEOUT is, after all, already > > supported. I can see that 'count' is not recalculated, so that is one > > of the reasons. However, it also needs to be explained why it is > > necessary to stop and restart the watchdog. Will add explanation in next revision. Basically once watchdog is started, WDT cycle setting register(WDTSET) cannot be changed. However we can stop WDT by issuing a reset and then reconfigure the new value for WDSET. So module reset is needed here. As you stated below, restarting watchdog unconditionally is not correct. May be after module reset, if watchdog timer is active(test_bit(WDOG_ACTIVE)), then will call start function. If it is in stopped state, when it calls start next time, it will pick new values. So both conditions are taken care here. > > > >> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >> --- > >> drivers/watchdog/rzg2l_wdt.c | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > >> > >> diff --git a/drivers/watchdog/rzg2l_wdt.c > >> b/drivers/watchdog/rzg2l_wdt.c index 58fe4efd9a89..c81b9dd05e63 > >> 100644 > >> --- a/drivers/watchdog/rzg2l_wdt.c > >> +++ b/drivers/watchdog/rzg2l_wdt.c > >> @@ -117,6 +117,15 @@ static int rzg2l_wdt_stop(struct watchdog_device > >> *wdev) > >> return 0; > >> } > >> +static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, > >> +unsigned int timeout) { > >> + wdev->timeout = timeout; > >> + rzg2l_wdt_stop(wdev); > >> + rzg2l_wdt_start(wdev); > > > > Is it necessary to stop and restart the timeout, or would it be > > sufficient to call rza_wdt_calc_timeout() ? If it is necessary, please > > add a comment OK, will do. Basically we need to do a module reset. Then only we can change the WDTSET register. On the next version, instead of stop/start, will issue a module reset, and If wdt is active then will call start. > > That should have been rzg2l_wdt_init_timeout(). Also, as mentioned in the > second patch of the series, the return value of rzg2l_wdt_start() needs to > be checked if the watchdog needs to be stopped and restarted. On the second patch, I will add the changes as you suggested for rzg2l_wdt_init_timeout(). and rzg2l_wdt_start will check return value of rzg2l_wdt_init_timeout. Regards, Biju > > Thanks, > Guenter > > > describing the reason. > > > > Either case, calling rzg2l_wdt_start() unconditionally is wrong > > because the watchdog might be stopped. > > > > Guenter > > > >> + > >> + return 0; > >> +} > >> + > >> static int rzg2l_wdt_restart(struct watchdog_device *wdev, > >> unsigned long action, void *data) > >> { > >> @@ -160,6 +169,7 @@ static const struct watchdog_ops rzg2l_wdt_ops = > >> { > >> .start = rzg2l_wdt_start, > >> .stop = rzg2l_wdt_stop, > >> .ping = rzg2l_wdt_ping, > >> + .set_timeout = rzg2l_wdt_set_timeout, > >> .restart = rzg2l_wdt_restart, > >> }; > >> > >
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index 58fe4efd9a89..c81b9dd05e63 100644 --- a/drivers/watchdog/rzg2l_wdt.c +++ b/drivers/watchdog/rzg2l_wdt.c @@ -117,6 +117,15 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) return 0; } +static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout) +{ + wdev->timeout = timeout; + rzg2l_wdt_stop(wdev); + rzg2l_wdt_start(wdev); + + return 0; +} + static int rzg2l_wdt_restart(struct watchdog_device *wdev, unsigned long action, void *data) { @@ -160,6 +169,7 @@ static const struct watchdog_ops rzg2l_wdt_ops = { .start = rzg2l_wdt_start, .stop = rzg2l_wdt_stop, .ping = rzg2l_wdt_ping, + .set_timeout = rzg2l_wdt_set_timeout, .restart = rzg2l_wdt_restart, };
Add support for set_timeout() callback. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/watchdog/rzg2l_wdt.c | 10 ++++++++++ 1 file changed, 10 insertions(+)