| Message ID | 1517423070-24236-14-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On 01/31/2018 10:24 AM, Fabrizio Castro wrote: > On iWave's boards iwg20d and iwg22d the only way to reboot the system is > by means of the watchdog. > This patch adds a restart handler to rwdt_ops, and also makes sure we > keep its priority to a medium level, in order to not override other more > effective handlers. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > v3->v4: > * New patch spawn out from patch 12/16. The restart handler on Gen3 is > controversial, hopefully this patch will help finalizing the discussion. > > drivers/watchdog/renesas_wdt.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c > index 0a1a402..6d1c4b9 100644 > --- a/drivers/watchdog/renesas_wdt.c > +++ b/drivers/watchdog/renesas_wdt.c > @@ -107,6 +107,24 @@ static unsigned int rwdt_get_timeleft(struct watchdog_device *wdev) > return DIV_BY_CLKS_PER_SEC(priv, 65536 - val); > } > > +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action, > + void *data) > +{ > + struct rwdt_priv *priv = watchdog_get_drvdata(wdev); > + > + pm_runtime_get_sync(wdev->parent); > + > + rwdt_write(priv, 0x00, RWTCSRB); > + rwdt_write(priv, 0x00, RWTCSRA); > + rwdt_write(priv, 0xffff, RWTCNT); > + > + while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG) > + cpu_relax(); > + > + rwdt_write(priv, 0x80, RWTCSRA); > + return 0; > +} > + > static const struct watchdog_info rwdt_ident = { > .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, > .identity = "Renesas WDT Watchdog", > @@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = { > .stop = rwdt_stop, > .ping = rwdt_init_timeout, > .get_timeleft = rwdt_get_timeleft, > + .restart = rwdt_restart, > }; > > static int rwdt_probe(struct platform_device *pdev) > @@ -176,6 +195,7 @@ static int rwdt_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, priv); > watchdog_set_drvdata(&priv->wdev, priv); > watchdog_set_nowayout(&priv->wdev, nowayout); > + watchdog_set_restart_priority(&priv->wdev, 128); > > /* This overrides the default timeout only if DT configuration was found */ > ret = watchdog_init_timeout(&priv->wdev, 0, &pdev->dev); >
Hi Fabrizio, On Wed, Jan 31, 2018 at 7:24 PM, Fabrizio Castro <fabrizio.castro@bp.renesas.com> wrote: > On iWave's boards iwg20d and iwg22d the only way to reboot the system is > by means of the watchdog. > This patch adds a restart handler to rwdt_ops, and also makes sure we > keep its priority to a medium level, in order to not override other more > effective handlers. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> > --- a/drivers/watchdog/renesas_wdt.c > +++ b/drivers/watchdog/renesas_wdt.c > @@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = { > .stop = rwdt_stop, > .ping = rwdt_init_timeout, > .get_timeleft = rwdt_get_timeleft, > + .restart = rwdt_restart, > }; > > static int rwdt_probe(struct platform_device *pdev) > @@ -176,6 +195,7 @@ static int rwdt_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, priv); > watchdog_set_drvdata(&priv->wdev, priv); > watchdog_set_nowayout(&priv->wdev, nowayout); > + watchdog_set_restart_priority(&priv->wdev, 128); Given we want to reboot R-Car Gen2 boards equipped with a suitable PMIC (e.g. DA9063) using that PMIC, I think the priority should be lower (0?), cfr. * 0: use watchdog's restart function as last resort, has limited restart * capabilies * 128: default restart handler, use if no other handler is expected to be * available and/or if restart is sufficient to restart the entire system * 255: preempt all other handlers 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
Hello Geert, > Subject: Re: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler > > Hi Fabrizio, Thank you for your feedback! > > On Wed, Jan 31, 2018 at 7:24 PM, Fabrizio Castro > <fabrizio.castro@bp.renesas.com> wrote: > > On iWave's boards iwg20d and iwg22d the only way to reboot the system is > > by means of the watchdog. > > This patch adds a restart handler to rwdt_ops, and also makes sure we > > keep its priority to a medium level, in order to not override other more > > effective handlers. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> > > > --- a/drivers/watchdog/renesas_wdt.c > > +++ b/drivers/watchdog/renesas_wdt.c > > @@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = { > > .stop = rwdt_stop, > > .ping = rwdt_init_timeout, > > .get_timeleft = rwdt_get_timeleft, > > + .restart = rwdt_restart, > > }; > > > > static int rwdt_probe(struct platform_device *pdev) > > @@ -176,6 +195,7 @@ static int rwdt_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, priv); > > watchdog_set_drvdata(&priv->wdev, priv); > > watchdog_set_nowayout(&priv->wdev, nowayout); > > + watchdog_set_restart_priority(&priv->wdev, 128); > > Given we want to reboot R-Car Gen2 boards equipped with a suitable PMIC > (e.g. DA9063) using that PMIC, I think the priority should be lower (0?), > cfr. Yes, can do, I have no strong opinion about this. I'll use priority 0 for the next submission. Thanks, Fab > > * 0: use watchdog's restart function as last resort, has limited restart > * capabilies > * 128: default restart handler, use if no other handler is expected to be > * available and/or if restart is sufficient to restart the entire system > * 255: preempt all other handlers > > 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 Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
> +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action, > + void *data) > +{ > + struct rwdt_priv *priv = watchdog_get_drvdata(wdev); > + > + pm_runtime_get_sync(wdev->parent); > + > + rwdt_write(priv, 0x00, RWTCSRB); > + rwdt_write(priv, 0x00, RWTCSRA); > + rwdt_write(priv, 0xffff, RWTCNT); > + > + while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG) > + cpu_relax(); > + > + rwdt_write(priv, 0x80, RWTCSRA); > + return 0; > +} I used to have this simpler version (back then for the UP case): + rwdt_start(wdev); + rwdt_write(priv, 0xffff, RWTCNT); + return 0; It should still work probably...
Hello Wolfram, thank you for your feedback! > Subject: Re: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler > > > > +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action, > > +void *data) > > +{ > > +struct rwdt_priv *priv = watchdog_get_drvdata(wdev); > > + > > +pm_runtime_get_sync(wdev->parent); > > + > > +rwdt_write(priv, 0x00, RWTCSRB); > > +rwdt_write(priv, 0x00, RWTCSRA); > > +rwdt_write(priv, 0xffff, RWTCNT); > > + > > +while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG) > > +cpu_relax(); > > + > > +rwdt_write(priv, 0x80, RWTCSRA); > > +return 0; > > +} > > I used to have this simpler version (back then for the UP case): > > +rwdt_start(wdev); > +rwdt_write(priv, 0xffff, RWTCNT); > +return 0; > > It should still work probably... It should still work, but it would be slower, I would stick with the version I have submitted if you don't mind. Thanks, Fabrizio Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
> > I used to have this simpler version (back then for the UP case): > > > > +rwdt_start(wdev); > > +rwdt_write(priv, 0xffff, RWTCNT); > > +return 0; > > > > It should still work probably... > > It should still work, but it would be slower, I would stick with the > version I have submitted if you don't mind. Actually, I do mind. Because of code duplication reasons. We have a fix in the works for rwdt_start() and it would be too easy to overlook the same fix will be needed for a restart.
> > > I used to have this simpler version (back then for the UP case): > > > > > > +rwdt_start(wdev); > > > +rwdt_write(priv, 0xffff, RWTCNT); > > > +return 0; > > > > > > It should still work probably... > > > > It should still work, but it would be slower, I would stick with the > > version I have submitted if you don't mind. > > Actually, I do mind. Because of code duplication reasons. We have a fix > in the works for rwdt_start() and it would be too easy to overlook the > same fix will be needed for a restart. The code is very similar indeed, but not exactly duplicated as it sets the registers in order to reset the system as quickly as possible. But since you have strong feelings about this I'll reuse this other version for the next submission. Thanks, Fabrizio Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c index 0a1a402..6d1c4b9 100644 --- a/drivers/watchdog/renesas_wdt.c +++ b/drivers/watchdog/renesas_wdt.c @@ -107,6 +107,24 @@ static unsigned int rwdt_get_timeleft(struct watchdog_device *wdev) return DIV_BY_CLKS_PER_SEC(priv, 65536 - val); } +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action, + void *data) +{ + struct rwdt_priv *priv = watchdog_get_drvdata(wdev); + + pm_runtime_get_sync(wdev->parent); + + rwdt_write(priv, 0x00, RWTCSRB); + rwdt_write(priv, 0x00, RWTCSRA); + rwdt_write(priv, 0xffff, RWTCNT); + + while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG) + cpu_relax(); + + rwdt_write(priv, 0x80, RWTCSRA); + return 0; +} + static const struct watchdog_info rwdt_ident = { .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, .identity = "Renesas WDT Watchdog", @@ -118,6 +136,7 @@ static const struct watchdog_ops rwdt_ops = { .stop = rwdt_stop, .ping = rwdt_init_timeout, .get_timeleft = rwdt_get_timeleft, + .restart = rwdt_restart, }; static int rwdt_probe(struct platform_device *pdev) @@ -176,6 +195,7 @@ static int rwdt_probe(struct platform_device *pdev) platform_set_drvdata(pdev, priv); watchdog_set_drvdata(&priv->wdev, priv); watchdog_set_nowayout(&priv->wdev, nowayout); + watchdog_set_restart_priority(&priv->wdev, 128); /* This overrides the default timeout only if DT configuration was found */ ret = watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);