Messages in this thread | | | From | "Lad, Prabhakar" <> | Date | Mon, 14 Nov 2022 19:45:38 +0000 | Subject | Re: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks |
| |
HI Biju,
Thank you for the review.
On Mon, Nov 14, 2022 at 6:42 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > -----Original Message----- > > From: Prabhakar <prabhakar.csengg@gmail.com> > > Sent: 14 November 2022 18:09 > > To: Geert Uytterhoeven <geert+renesas@glider.be>; Wim Van Sebroeck > > <wim@linux-watchdog.org>; Guenter Roeck <linux@roeck-us.net>; Philipp Zabel > > <p.zabel@pengutronix.de>; linux-watchdog@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; > > Prabhakar <prabhakar.csengg@gmail.com>; Biju Das > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro > > <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev- > > lad.rj@bp.renesas.com> > > Subject: [PATCH] watchdog: rzg2l_wdt: Issue a reset before we put the PM > > clocks > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > On RZ/Five SoC it was observed that setting timeout (to say 1 sec) wouldn't > > reset the system. To fix this we make sure we issue a reset before putting > > the PM clocks to make sure the registers have been cleared. > > > > While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as we were > > calling the same functions here. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > Note, > > - This patch has been tested on RZ/G2L, RZ/V2M and RZ/Five. > > - My initial investigation showed adding the delay after > > pm_runtime_get_sync() > > also fixed this issue. > > - Do I need add the fixes tag ? what should be the operation PUT- > > >RESET/RESET->PUT? > > It looks like timing issue, None of the previous devices are affected by this. > yep.
> > in case we need the tag is: > > Fixes: 4055ee81009e6 ("watchdog: rzg2l_wdt: Add set_timeout callback") > > --- > > drivers/watchdog/rzg2l_wdt.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > > index 00438ceed17a..d1271cc7750f 100644 > > --- a/drivers/watchdog/rzg2l_wdt.c > > +++ b/drivers/watchdog/rzg2l_wdt.c > > @@ -115,16 +115,14 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) > > { > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > - pm_runtime_put(wdev->parent); > > reset_control_reset(priv->rstc); > > + pm_runtime_put(wdev->parent); > > > > return 0; > > } > > > > static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int > > timeout) { > > - struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > - > > wdev->timeout = timeout; > > > > /* > > @@ -132,8 +130,7 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device > > *wdev, unsigned int time > > * register so that it is updated with new timeout values. > > */ > > > Maybe update the comment above with new code change. > /* * If the watchdog is active, reset the module for updating the WDTSET * register so that it is updated with new timeout values. */
The above existing comment holds good with this code change. If you prefer something else please let me know I'll update it accordingly.
Cheers, Prabhakar
| |