[PATCH v7 0/3] Add Gen2 support to rwdt driver

[PATCH v7 0/3] Add Gen2 support to rwdt driver

[Fabrizio Castro]

Dear All,

this is to merge Wolfram's comments he made on the previous version.

Thanks,

Fabrizio Castro (3):
  watchdog: renesas_wdt: Add suspend/resume support
  watchdog: renesas_wdt: Add R-Car Gen2 support
  watchdog: renesas_wdt: Add restart handler

 drivers/watchdog/renesas_wdt.c | 44 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Fabrizio Castro]

On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
on R-Car Gen3 we power the IP down during suspend.

This commit adds suspend/resume support, so that the watchdog counting
"pauses" during suspend on all of the SoCs compatible with this driver
and on those we are now adding support for (R-Car Gen2 and RZ/G1).

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
---
v6->v7:
* backup and restore register RWTCNT instead of using rwdt_get_timeleft and
  rwdt_set_timeleft

 drivers/watchdog/renesas_wdt.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 831ef83..024d54e 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -49,6 +49,7 @@ struct rwdt_priv {
 	void __iomem *base;
 	struct watchdog_device wdev;
 	unsigned long clk_rate;
+	u16 time_left;
 	u8 cks;
 };
 
@@ -203,6 +204,30 @@ static int rwdt_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused rwdt_suspend(struct device *dev)
+{
+	struct rwdt_priv *priv = dev_get_drvdata(dev);
+
+	if (watchdog_active(&priv->wdev)) {
+		priv->time_left = readw(priv->base + RWTCNT);
+		rwdt_stop(&priv->wdev);
+	}
+	return 0;
+}
+
+static int __maybe_unused rwdt_resume(struct device *dev)
+{
+	struct rwdt_priv *priv = dev_get_drvdata(dev);
+
+	if (watchdog_active(&priv->wdev)) {
+		rwdt_start(&priv->wdev);
+		rwdt_write(priv, priv->time_left, RWTCNT);
+	}
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(rwdt_pm_ops, rwdt_suspend, rwdt_resume);
+
 /*
  * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for SMP
  * to work there, one also needs a RESET (RST) driver which does not exist yet
@@ -218,6 +243,7 @@ static struct platform_driver rwdt_driver = {
 	.driver = {
 		.name = "renesas_wdt",
 		.of_match_table = rwdt_ids,
+		.pm = &rwdt_pm_ops,
 	},
 	.probe = rwdt_probe,
 	.remove = rwdt_remove,
-- 
2.7.4

Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

On Thu, Mar 01, 2018 at 06:17:21PM +0000, Fabrizio Castro wrote:
> On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
> on R-Car Gen3 we power the IP down during suspend.
> 
> This commit adds suspend/resume support, so that the watchdog counting
> "pauses" during suspend on all of the SoCs compatible with this driver
> and on those we are now adding support for (R-Car Gen2 and RZ/G1).
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>

I like it:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for keeping at this topic!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Guenter Roeck]

On Thu, Mar 01, 2018 at 06:17:21PM +0000, Fabrizio Castro wrote:
> On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
> on R-Car Gen3 we power the IP down during suspend.
> 
> This commit adds suspend/resume support, so that the watchdog counting
> "pauses" during suspend on all of the SoCs compatible with this driver
> and on those we are now adding support for (R-Car Gen2 and RZ/G1).
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>

Usually, on resume, we just restart the watchdog, with the expectation in mind
that there may be some delay in userspace before it gets to send the next ping.
Presumably that is not a concern here, so

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> v6->v7:
> * backup and restore register RWTCNT instead of using rwdt_get_timeleft and
>   rwdt_set_timeleft
> 
>  drivers/watchdog/renesas_wdt.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 831ef83..024d54e 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -49,6 +49,7 @@ struct rwdt_priv {
>  	void __iomem *base;
>  	struct watchdog_device wdev;
>  	unsigned long clk_rate;
> +	u16 time_left;
>  	u8 cks;
>  };
>  
> @@ -203,6 +204,30 @@ static int rwdt_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int __maybe_unused rwdt_suspend(struct device *dev)
> +{
> +	struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&priv->wdev)) {
> +		priv->time_left = readw(priv->base + RWTCNT);
> +		rwdt_stop(&priv->wdev);
> +	}
> +	return 0;
> +}
> +
> +static int __maybe_unused rwdt_resume(struct device *dev)
> +{
> +	struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&priv->wdev)) {
> +		rwdt_start(&priv->wdev);
> +		rwdt_write(priv, priv->time_left, RWTCNT);
> +	}
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rwdt_pm_ops, rwdt_suspend, rwdt_resume);
> +
>  /*
>   * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for SMP
>   * to work there, one also needs a RESET (RST) driver which does not exist yet
> @@ -218,6 +243,7 @@ static struct platform_driver rwdt_driver = {
>  	.driver = {
>  		.name = "renesas_wdt",
>  		.of_match_table = rwdt_ids,
> +		.pm = &rwdt_pm_ops,
>  	},
>  	.probe = rwdt_probe,
>  	.remove = rwdt_remove,
> -- 
> 2.7.4
> 

Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2954 bytes --]

On Thu, Mar 01, 2018 at 11:41:01AM -0800, Guenter Roeck wrote:
> On Thu, Mar 01, 2018 at 06:17:21PM +0000, Fabrizio Castro wrote:
> > On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
> > on R-Car Gen3 we power the IP down during suspend.
> > 
> > This commit adds suspend/resume support, so that the watchdog counting
> > "pauses" during suspend on all of the SoCs compatible with this driver
> > and on those we are now adding support for (R-Car Gen2 and RZ/G1).
> > 
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> 
> Usually, on resume, we just restart the watchdog, with the expectation in mind
> that there may be some delay in userspace before it gets to send the next ping.
> Presumably that is not a concern here, so

I didn't know that. Actually, I'd prefer that. Even less complexity. And
if it is even more consistent with the rest of the drivers...

> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> > ---
> > v6->v7:
> > * backup and restore register RWTCNT instead of using rwdt_get_timeleft and
> >   rwdt_set_timeleft
> > 
> >  drivers/watchdog/renesas_wdt.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> > index 831ef83..024d54e 100644
> > --- a/drivers/watchdog/renesas_wdt.c
> > +++ b/drivers/watchdog/renesas_wdt.c
> > @@ -49,6 +49,7 @@ struct rwdt_priv {
> >  	void __iomem *base;
> >  	struct watchdog_device wdev;
> >  	unsigned long clk_rate;
> > +	u16 time_left;
> >  	u8 cks;
> >  };
> >  
> > @@ -203,6 +204,30 @@ static int rwdt_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +static int __maybe_unused rwdt_suspend(struct device *dev)
> > +{
> > +	struct rwdt_priv *priv = dev_get_drvdata(dev);
> > +
> > +	if (watchdog_active(&priv->wdev)) {
> > +		priv->time_left = readw(priv->base + RWTCNT);
> > +		rwdt_stop(&priv->wdev);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused rwdt_resume(struct device *dev)
> > +{
> > +	struct rwdt_priv *priv = dev_get_drvdata(dev);
> > +
> > +	if (watchdog_active(&priv->wdev)) {
> > +		rwdt_start(&priv->wdev);
> > +		rwdt_write(priv, priv->time_left, RWTCNT);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(rwdt_pm_ops, rwdt_suspend, rwdt_resume);
> > +
> >  /*
> >   * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for SMP
> >   * to work there, one also needs a RESET (RST) driver which does not exist yet
> > @@ -218,6 +243,7 @@ static struct platform_driver rwdt_driver = {
> >  	.driver = {
> >  		.name = "renesas_wdt",
> >  		.of_match_table = rwdt_ids,
> > +		.pm = &rwdt_pm_ops,
> >  	},
> >  	.probe = rwdt_probe,
> >  	.remove = rwdt_remove,
> > -- 
> > 2.7.4
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Geert Uytterhoeven]

Hi Fabrizio,

On Thu, Mar 1, 2018 at 7:17 PM, Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
> on R-Car Gen3 we power the IP down during suspend.
>
> This commit adds suspend/resume support, so that the watchdog counting
> "pauses" during suspend on all of the SoCs compatible with this driver
> and on those we are now adding support for (R-Car Gen2 and RZ/G1).
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> ---
> v6->v7:
> * backup and restore register RWTCNT instead of using rwdt_get_timeleft and
>   rwdt_set_timeleft

Thanks for the update (v6 and v7)!

>
>  drivers/watchdog/renesas_wdt.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 831ef83..024d54e 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -49,6 +49,7 @@ struct rwdt_priv {
>         void __iomem *base;
>         struct watchdog_device wdev;
>         unsigned long clk_rate;
> +       u16 time_left;
>         u8 cks;
>  };
>
> @@ -203,6 +204,30 @@ static int rwdt_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static int __maybe_unused rwdt_suspend(struct device *dev)
> +{
> +       struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> +       if (watchdog_active(&priv->wdev)) {
> +               priv->time_left = readw(priv->base + RWTCNT);
> +               rwdt_stop(&priv->wdev);
> +       }
> +       return 0;
> +}
> +
> +static int __maybe_unused rwdt_resume(struct device *dev)
> +{
> +       struct rwdt_priv *priv = dev_get_drvdata(dev);
> +
> +       if (watchdog_active(&priv->wdev)) {
> +               rwdt_start(&priv->wdev);
> +               rwdt_write(priv, priv->time_left, RWTCNT);

Upon given it more thought, I'm a bit worried about restoring the
original time left.
In my experiments, it may take a few seconds before userspace fully resumes.
If time_left was a small value, the system may reboot before userspace has
a chance to send its next ping.
This was with NFS root, so heavily impacted by the delays introduced by the
PHY link getting up again.

So just using rwdt_stop()/rwdt_start() may be the safest option.

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

RE: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Fabrizio Castro]

Dear All,

perhaps someone from this email thread could explain to me what's the actual
(general) expectation from a system perspective (at resume) from the watchdog,
because I can see pitfalls whether 1) we simply start the watchdog at resume or
2) we pick up from where we left.

If we have a system that goes to sleep quite a bit, option 1) may cause the watchdog
to never fire, even though user space is not explicitly pinging the watchdog. As Geert
has pointed out, going to sleep and waking up adds a delay, therefore with option 2)
you may miss the opportunity to ping the watchdog and therefore the system may
restart even when it shouldn't. However, with option 2) user space can make
arrangements to compensate for the delay, and when user space compensates for
that it means the system is probably sane. With option 1) instead we are basically
pinging the watchdog without explicitly doing so from user space, which I don't think
is what we want here, but I may be wrong.

Could someone please shed some light here?

Thanks,
Fab


> Subject: Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support
>
> Hi Fabrizio,
>
> On Thu, Mar 1, 2018 at 7:17 PM, Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
> > on R-Car Gen3 we power the IP down during suspend.
> >
> > This commit adds suspend/resume support, so that the watchdog counting
> > "pauses" during suspend on all of the SoCs compatible with this driver
> > and on those we are now adding support for (R-Car Gen2 and RZ/G1).
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> > ---
> > v6->v7:
> > * backup and restore register RWTCNT instead of using rwdt_get_timeleft and
> >   rwdt_set_timeleft
>
> Thanks for the update (v6 and v7)!
>
> >
> >  drivers/watchdog/renesas_wdt.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> > index 831ef83..024d54e 100644
> > --- a/drivers/watchdog/renesas_wdt.c
> > +++ b/drivers/watchdog/renesas_wdt.c
> > @@ -49,6 +49,7 @@ struct rwdt_priv {
> >         void __iomem *base;
> >         struct watchdog_device wdev;
> >         unsigned long clk_rate;
> > +       u16 time_left;
> >         u8 cks;
> >  };
> >
> > @@ -203,6 +204,30 @@ static int rwdt_remove(struct platform_device *pdev)
> >         return 0;
> >  }
> >
> > +static int __maybe_unused rwdt_suspend(struct device *dev)
> > +{
> > +       struct rwdt_priv *priv = dev_get_drvdata(dev);
> > +
> > +       if (watchdog_active(&priv->wdev)) {
> > +               priv->time_left = readw(priv->base + RWTCNT);
> > +               rwdt_stop(&priv->wdev);
> > +       }
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused rwdt_resume(struct device *dev)
> > +{
> > +       struct rwdt_priv *priv = dev_get_drvdata(dev);
> > +
> > +       if (watchdog_active(&priv->wdev)) {
> > +               rwdt_start(&priv->wdev);
> > +               rwdt_write(priv, priv->time_left, RWTCNT);
>
> Upon given it more thought, I'm a bit worried about restoring the
> original time left.
> In my experiments, it may take a few seconds before userspace fully resumes.
> If time_left was a small value, the system may reboot before userspace has
> a chance to send its next ping.
> This was with NFS root, so heavily impacted by the delays introduced by the
> PHY link getting up again.
>
> So just using rwdt_stop()/rwdt_start() may be the safest option.
>
> 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.

Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Guenter Roeck]

On 03/02/2018 02:45 AM, Fabrizio Castro wrote:
> Dear All,
> 
> perhaps someone from this email thread could explain to me what's the actual
> (general) expectation from a system perspective (at resume) from the watchdog,
> because I can see pitfalls whether 1) we simply start the watchdog at resume or
> 2) we pick up from where we left.
> 
> If we have a system that goes to sleep quite a bit, option 1) may cause the watchdog
> to never fire, even though user space is not explicitly pinging the watchdog. As Geert
> has pointed out, going to sleep and waking up adds a delay, therefore with option 2)
> you may miss the opportunity to ping the watchdog and therefore the system may
> restart even when it shouldn't. However, with option 2) user space can make
> arrangements to compensate for the delay, and when user space compensates for
> that it means the system is probably sane. With option 1) instead we are basically
> pinging the watchdog without explicitly doing so from user space, which I don't think
> is what we want here, but I may be wrong.
> 
> Could someone please shed some light here?
> 
If the system goes to sleep so often that the watchdog never triggers just because of
that, it must either be in pretty good shape, in which case the watchdog doesn't need
to fire, or it is in bad shape, and the repeated stopping/restarting of the watchdog
would ultimately cause the system to die with the watchdog stopped anyway.

Overall, just the fact that the watchdog has to be stopped during suspend is a weak spot.
Bad luck if the system hangs after the watchdog was stopped. Since suspend is a critical
operation )in the sense that if anything goes wrong, that is the time for it), that is
a _real_ weak spot. If anything, we should be concerned about that, not about the exact
timing of watchdog pings.

Sure, you can leave it to user space to adjust for the resume time. Let's hope that the
watchdog daemon does that, and that it gets to run fast enough to actually do it.
I do wonder though how it would know. Are processes informed about a resume event ?

Personally I rather play it safe, meaning I rather give the watchdog a bit of additional
slack during resume. Having said that, as mentioned before, I am willing to accept
the patch as is, in the assumption that the authors know what they are doing.

Guenter

> Thanks,
> Fab
> 
> 
>> Subject: Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support
>>
>> Hi Fabrizio,
>>
>> On Thu, Mar 1, 2018 at 7:17 PM, Fabrizio Castro
>> <fabrizio.castro@bp.renesas.com> wrote:
>>> On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
>>> on R-Car Gen3 we power the IP down during suspend.
>>>
>>> This commit adds suspend/resume support, so that the watchdog counting
>>> "pauses" during suspend on all of the SoCs compatible with this driver
>>> and on those we are now adding support for (R-Car Gen2 and RZ/G1).
>>>
>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>>> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
>>> ---
>>> v6->v7:
>>> * backup and restore register RWTCNT instead of using rwdt_get_timeleft and
>>>    rwdt_set_timeleft
>>
>> Thanks for the update (v6 and v7)!
>>
>>>
>>>   drivers/watchdog/renesas_wdt.c | 26 ++++++++++++++++++++++++++
>>>   1 file changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
>>> index 831ef83..024d54e 100644
>>> --- a/drivers/watchdog/renesas_wdt.c
>>> +++ b/drivers/watchdog/renesas_wdt.c
>>> @@ -49,6 +49,7 @@ struct rwdt_priv {
>>>          void __iomem *base;
>>>          struct watchdog_device wdev;
>>>          unsigned long clk_rate;
>>> +       u16 time_left;
>>>          u8 cks;
>>>   };
>>>
>>> @@ -203,6 +204,30 @@ static int rwdt_remove(struct platform_device *pdev)
>>>          return 0;
>>>   }
>>>
>>> +static int __maybe_unused rwdt_suspend(struct device *dev)
>>> +{
>>> +       struct rwdt_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +       if (watchdog_active(&priv->wdev)) {
>>> +               priv->time_left = readw(priv->base + RWTCNT);
>>> +               rwdt_stop(&priv->wdev);
>>> +       }
>>> +       return 0;
>>> +}
>>> +
>>> +static int __maybe_unused rwdt_resume(struct device *dev)
>>> +{
>>> +       struct rwdt_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +       if (watchdog_active(&priv->wdev)) {
>>> +               rwdt_start(&priv->wdev);
>>> +               rwdt_write(priv, priv->time_left, RWTCNT);
>>
>> Upon given it more thought, I'm a bit worried about restoring the
>> original time left.
>> In my experiments, it may take a few seconds before userspace fully resumes.
>> If time_left was a small value, the system may reboot before userspace has
>> a chance to send its next ping.
>> This was with NFS root, so heavily impacted by the delays introduced by the
>> PHY link getting up again.
>>
>> So just using rwdt_stop()/rwdt_start() may be the safest option.
>>
>> 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.
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{���\�� �{ay�\x1d
ʇڙ�,j\a��f���h���z�\x1e
�w���\f
���j:+v���w�j�m����\a����zZ+�����ݢj"��!tml=
> 

RE: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Fabrizio Castro]

Hi Guenter,

thank you for your feedback!

> Subject: Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support
>
> On 03/02/2018 02:45 AM, Fabrizio Castro wrote:
> > Dear All,
> >
> > perhaps someone from this email thread could explain to me what's the actual
> > (general) expectation from a system perspective (at resume) from the watchdog,
> > because I can see pitfalls whether 1) we simply start the watchdog at resume or
> > 2) we pick up from where we left.
> >
> > If we have a system that goes to sleep quite a bit, option 1) may cause the watchdog
> > to never fire, even though user space is not explicitly pinging the watchdog. As Geert
> > has pointed out, going to sleep and waking up adds a delay, therefore with option 2)
> > you may miss the opportunity to ping the watchdog and therefore the system may
> > restart even when it shouldn't. However, with option 2) user space can make
> > arrangements to compensate for the delay, and when user space compensates for
> > that it means the system is probably sane. With option 1) instead we are basically
> > pinging the watchdog without explicitly doing so from user space, which I don't think
> > is what we want here, but I may be wrong.
> >
> > Could someone please shed some light here?
> >
> If the system goes to sleep so often that the watchdog never triggers just because of
> that, it must either be in pretty good shape, in which case the watchdog doesn't need
> to fire, or it is in bad shape, and the repeated stopping/restarting of the watchdog
> would ultimately cause the system to die with the watchdog stopped anyway.

yeah, in some cases if the system isn't restarted user space may do something it isn't supposed
to do , that's why I don't particularly like the idea of implicitly pinging the watchdog on resume,
it should be left to user space alone as this is policy related, but that is my personal opinion.

>
> Overall, just the fact that the watchdog has to be stopped during suspend is a weak spot.
> Bad luck if the system hangs after the watchdog was stopped. Since suspend is a critical
> operation )in the sense that if anything goes wrong, that is the time for it), that is
> a _real_ weak spot. If anything, we should be concerned about that, not about the exact
> timing of watchdog pings.

so true!

>
> Sure, you can leave it to user space to adjust for the resume time. Let's hope that the
> watchdog daemon does that, and that it gets to run fast enough to actually do it.
> I do wonder though how it would know. Are processes informed about a resume event ?

I was thinking more about making adjustments before going to sleep.

>
> Personally I rather play it safe, meaning I rather give the watchdog a bit of additional
> slack during resume. Having said that, as mentioned before, I am willing to accept
> the patch as is, in the assumption that the authors know what they are doing.

Thank you for this.

Cheers,
Fab

>
> Guenter
>
> > Thanks,
> > Fab
> >
> >
> >> Subject: Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support
> >>
> >> Hi Fabrizio,
> >>
> >> On Thu, Mar 1, 2018 at 7:17 PM, Fabrizio Castro
> >> <fabrizio.castro@bp.renesas.com> wrote:
> >>> On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
> >>> on R-Car Gen3 we power the IP down during suspend.
> >>>
> >>> This commit adds suspend/resume support, so that the watchdog counting
> >>> "pauses" during suspend on all of the SoCs compatible with this driver
> >>> and on those we are now adding support for (R-Car Gen2 and RZ/G1).
> >>>
> >>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >>> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> >>> ---
> >>> v6->v7:
> >>> * backup and restore register RWTCNT instead of using rwdt_get_timeleft and
> >>>    rwdt_set_timeleft
> >>
> >> Thanks for the update (v6 and v7)!
> >>
> >>>
> >>>   drivers/watchdog/renesas_wdt.c | 26 ++++++++++++++++++++++++++
> >>>   1 file changed, 26 insertions(+)
> >>>
> >>> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> >>> index 831ef83..024d54e 100644
> >>> --- a/drivers/watchdog/renesas_wdt.c
> >>> +++ b/drivers/watchdog/renesas_wdt.c
> >>> @@ -49,6 +49,7 @@ struct rwdt_priv {
> >>>          void __iomem *base;
> >>>          struct watchdog_device wdev;
> >>>          unsigned long clk_rate;
> >>> +       u16 time_left;
> >>>          u8 cks;
> >>>   };
> >>>
> >>> @@ -203,6 +204,30 @@ static int rwdt_remove(struct platform_device *pdev)
> >>>          return 0;
> >>>   }
> >>>
> >>> +static int __maybe_unused rwdt_suspend(struct device *dev)
> >>> +{
> >>> +       struct rwdt_priv *priv = dev_get_drvdata(dev);
> >>> +
> >>> +       if (watchdog_active(&priv->wdev)) {
> >>> +               priv->time_left = readw(priv->base + RWTCNT);
> >>> +               rwdt_stop(&priv->wdev);
> >>> +       }
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +static int __maybe_unused rwdt_resume(struct device *dev)
> >>> +{
> >>> +       struct rwdt_priv *priv = dev_get_drvdata(dev);
> >>> +
> >>> +       if (watchdog_active(&priv->wdev)) {
> >>> +               rwdt_start(&priv->wdev);
> >>> +               rwdt_write(priv, priv->time_left, RWTCNT);
> >>
> >> Upon given it more thought, I'm a bit worried about restoring the
> >> original time left.
> >> In my experiments, it may take a few seconds before userspace fully resumes.
> >> If time_left was a small value, the system may reboot before userspace has
> >> a chance to send its next ping.
> >> This was with NFS root, so heavily impacted by the delays introduced by the
> >> PHY link getting up again.
> >>
> >> So just using rwdt_stop()/rwdt_start() may be the safest option.
> >>
> >> 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.
> > N�����r��y���b�X��ǧv�^�)޺{.n�+����{���\�� �{ay�\x1dʇڙ�,j
��f���h���z�\x1e�w���
>
> ���j:+v���w�j�m����
����zZ+�����ݢj"��!tml=
> >




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Sergei Shtylyov]

Hello!

On 3/1/2018 9:17 PM, Fabrizio Castro wrote:

> On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
> on R-Car Gen3 we power the IP down during suspend.
> 
> This commit adds suspend/resume support, so that the watchdog counting
> "pauses" during suspend on all of the SoCs compatible with this driver
> and on those we are now adding support for (R-Car Gen2 and RZ/G1).

    Why these parens here?

> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
[...]

MBR, Sergei

RE: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Fabrizio Castro]

Hello Sergei,

thank you for your feedback!

> Subject: Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support
>
> Hello!
>
> On 3/1/2018 9:17 PM, Fabrizio Castro wrote:
>
> > On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
> > on R-Car Gen3 we power the IP down during suspend.
> >
> > This commit adds suspend/resume support, so that the watchdog counting
> > "pauses" during suspend on all of the SoCs compatible with this driver
> > and on those we are now adding support for (R-Car Gen2 and RZ/G1).
>
>     Why these parens here?

it's due to the context, this patch belongs to the watchdog support series
but it comes before the commit where we add driver support for R-Car Gen2
and RZ/G1. Anyway, what we do works for R-Car Gen3 as well.

Would you prefer a different message?

Thanks,
Fab

>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> [...]
>
> MBR, Sergei



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v7 1/3] watchdog: renesas_wdt: Add suspend/resume support

[Sergei Shtylyov]

On 03/05/2018 05:08 PM, Fabrizio Castro wrote:

>>> On R-Car Gen2 and RZ/G1 the watchdog IP clock needs to be always ON,
>>> on R-Car Gen3 we power the IP down during suspend.
>>>
>>> This commit adds suspend/resume support, so that the watchdog counting
>>> "pauses" during suspend on all of the SoCs compatible with this driver
>>> and on those we are now adding support for (R-Car Gen2 and RZ/G1).
>>
>>     Why these parens here?
> 
> it's due to the context, this patch belongs to the watchdog support series
> but it comes before the commit where we add driver support for R-Car Gen2
> and RZ/G1. Anyway, what we do works for R-Car Gen3 as well.
> 
> Would you prefer a different message?

   No -- looks like I had just misread the message.

> Thanks,
> Fab

[...]

MBR, Sergei

[PATCH v7 2/3] watchdog: renesas_wdt: Add R-Car Gen2 support

[Fabrizio Castro]

Due to commits:
* "ARM: shmobile: Add watchdog support",
* "ARM: shmobile: rcar-gen2: Add watchdog support", and
* "soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2",
we now have everything we needed for the watchdog to work on Gen2 and
RZ/G1.

This commit adds "renesas,rcar-gen2-wdt" as compatible string for R-Car
Gen2 and RZ/G1.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
v6->v7:
* no change

 drivers/watchdog/renesas_wdt.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 024d54e..9fc4c78 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -228,12 +228,8 @@ static int __maybe_unused rwdt_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(rwdt_pm_ops, rwdt_suspend, rwdt_resume);
 
-/*
- * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for SMP
- * to work there, one also needs a RESET (RST) driver which does not exist yet
- * due to HW issues. This needs to be solved before adding compatibles here.
- */
 static const struct of_device_id rwdt_ids[] = {
+	{ .compatible = "renesas,rcar-gen2-wdt", },
 	{ .compatible = "renesas,rcar-gen3-wdt", },
 	{ /* sentinel */ }
 };
-- 
2.7.4

Re: [PATCH v7 2/3] watchdog: renesas_wdt: Add R-Car Gen2 support

[Guenter Roeck]

On Thu, Mar 01, 2018 at 06:17:22PM +0000, Fabrizio Castro wrote:
> Due to commits:
> * "ARM: shmobile: Add watchdog support",
> * "ARM: shmobile: rcar-gen2: Add watchdog support", and
> * "soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2",
> we now have everything we needed for the watchdog to work on Gen2 and
> RZ/G1.
> 
> This commit adds "renesas,rcar-gen2-wdt" as compatible string for R-Car
> Gen2 and RZ/G1.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> v6->v7:
> * no change
> 
>  drivers/watchdog/renesas_wdt.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 024d54e..9fc4c78 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -228,12 +228,8 @@ static int __maybe_unused rwdt_resume(struct device *dev)
>  
>  static SIMPLE_DEV_PM_OPS(rwdt_pm_ops, rwdt_suspend, rwdt_resume);
>  
> -/*
> - * This driver does also fit for R-Car Gen2 (r8a779[0-4]) WDT. However, for SMP
> - * to work there, one also needs a RESET (RST) driver which does not exist yet
> - * due to HW issues. This needs to be solved before adding compatibles here.
> - */
>  static const struct of_device_id rwdt_ids[] = {
> +	{ .compatible = "renesas,rcar-gen2-wdt", },
>  	{ .compatible = "renesas,rcar-gen3-wdt", },
>  	{ /* sentinel */ }
>  };
> -- 
> 2.7.4
> 

Re: [PATCH v7 2/3] watchdog: renesas_wdt: Add R-Car Gen2 support

[Geert Uytterhoeven]

Hi Fabrizio,

On Thu, Mar 1, 2018 at 7:17 PM, Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> Due to commits:
> * "ARM: shmobile: Add watchdog support",
> * "ARM: shmobile: rcar-gen2: Add watchdog support", and
> * "soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2",
> we now have everything we needed for the watchdog to work on Gen2 and
> RZ/G1.
>
> This commit adds "renesas,rcar-gen2-wdt" as compatible string for R-Car
> Gen2 and RZ/G1.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

To avoid nasty surprises on early R-Car Gen2 SoCs, "[PATCH] watchdog:
renesas_wdt: Blacklist early R-Car Gen2 SoCs"
(https://patchwork.kernel.org/patch/10233297/) should be folded into this
patch.

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

RE: [PATCH v7 2/3] watchdog: renesas_wdt: Add R-Car Gen2 support

[Fabrizio Castro]

Hello Geert,

> Subject: Re: [PATCH v7 2/3] watchdog: renesas_wdt: Add R-Car Gen2 support
>
> Hi Fabrizio,
>
> On Thu, Mar 1, 2018 at 7:17 PM, Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > Due to commits:
> > * "ARM: shmobile: Add watchdog support",
> > * "ARM: shmobile: rcar-gen2: Add watchdog support", and
> > * "soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2",
> > we now have everything we needed for the watchdog to work on Gen2 and
> > RZ/G1.
> >
> > This commit adds "renesas,rcar-gen2-wdt" as compatible string for R-Car
> > Gen2 and RZ/G1.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> To avoid nasty surprises on early R-Car Gen2 SoCs, "[PATCH] watchdog:
> renesas_wdt: Blacklist early R-Car Gen2 SoCs"
> (https://patchwork.kernel.org/patch/10233297/) should be folded into this
> patch.

I agree, I'll send a new version with your blacklisting patch folded into this patch.

Thanks,
Fab

>
> 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.

[PATCH v7 3/3] watchdog: renesas_wdt: Add restart handler

[Fabrizio Castro]

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 the lowest 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>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
v6->v7:
* fix changelog

 drivers/watchdog/renesas_wdt.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 9fc4c78..d287854 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -108,6 +108,16 @@ 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);
+
+	rwdt_start(wdev);
+	rwdt_write(priv, 0xffff, RWTCNT);
+	return 0;
+}
+
 static const struct watchdog_info rwdt_ident = {
 	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
 	.identity = "Renesas WDT Watchdog",
@@ -119,6 +129,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)
@@ -177,6 +188,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, 0);
 
 	/* This overrides the default timeout only if DT configuration was found */
 	ret = watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
-- 
2.7.4

Re: [PATCH v7 3/3] watchdog: renesas_wdt: Add restart handler

[Geert Uytterhoeven]

On Thu, Mar 1, 2018 at 7:17 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 the lowest 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>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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