RE: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler

[Fabrizio Castro]

> > > I used to have this simpler version (back then for the UP case):
> > >
> > > +rwdt_start(wdev);
> > > +rwdt_write(priv, 0x, 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.

Re: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler

[Wolfram Sang]

> > I used to have this simpler version (back then for the UP case):
> >
> > +rwdt_start(wdev);
> > +rwdt_write(priv, 0x, 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.



signature.asc
Description: PGP signature

RE: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler

[Fabrizio Castro]

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, 0x, 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, 0x, 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.

Re: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler

[Wolfram Sang]

> +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, 0x, 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, 0x, RWTCNT);
+   return 0;

It should still work probably...



signature.asc
Description: PGP signature

RE: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler

[Fabrizio Castro]

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.cas...@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.cas...@bp.renesas.com>
> > Signed-off-by: Ramesh Shanmugasundaram 
> > <ramesh.shanmugasunda...@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(>wdev, priv);
> > watchdog_set_nowayout(>wdev, nowayout);
> > +   watchdog_set_restart_priority(>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 -- 
> ge...@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.

Re: [RFC v4 13/26] watchdog: renesas_wdt: Add restart handler

[Guenter Roeck]

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 
Signed-off-by: Ramesh Shanmugasundaram 


Reviewed-by: Guenter Roeck 


---
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, 0x, 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(>wdev, priv);
watchdog_set_nowayout(>wdev, nowayout);
+   watchdog_set_restart_priority(>wdev, 128);
  
  	/* This overrides the default timeout only if DT configuration was found */

ret = watchdog_init_timeout(>wdev, 0, >dev);