Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582889 > unrolled thread

[PATCH v2 3/3] watchdog: sama5d4: Implement resume hook

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 3/3] watchdog: sama5d4: Implement resume hook Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-16 20:40 +0100
    Re: [PATCH v2 3/3] watchdog: sama5d4: Implement resume hook Guenter Roeck <linux@roeck-us.net> - 2017-02-17 15:50 +0100
      Re: [PATCH v2 3/3] watchdog: sama5d4: Implement resume hook Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-17 16:30 +0100
        Re: [PATCH v2 3/3] watchdog: sama5d4: Implement resume hook Guenter Roeck <linux@roeck-us.net> - 2017-02-19 18:10 +0100

#1582889 — [PATCH v2 3/3] watchdog: sama5d4: Implement resume hook

When resuming for the deepest state on sama5d2, it is necessary to restore
MR as the registers are lost.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
Changes in v2:
 - cache mr beofre suspending

 drivers/watchdog/sama5d4_wdt.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 2a60251806d2..5ddeb4803dc3 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -28,6 +28,7 @@
 struct sama5d4_wdt {
 	struct watchdog_device	wdd;
 	void __iomem		*reg_base;
+	u32			mr;
 };
 
 static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
@@ -248,11 +249,42 @@ static const struct of_device_id sama5d4_wdt_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
 
+#ifdef CONFIG_PM_SLEEP
+static int sama5d4_wdt_suspend(struct device *dev)
+{
+	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
+
+	wdt->mr = wdt_read(wdt, AT91_WDT_MR);
+
+	return 0;
+}
+
+static int sama5d4_wdt_resume(struct device *dev)
+{
+	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = wdt_read(wdt, AT91_WDT_MR);
+	if (reg & AT91_WDT_WDDIS)
+		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
+
+	wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
+	if (wdt->mr & AT91_WDT_WDDIS)
+		wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, sama5d4_wdt_suspend,
+			 sama5d4_wdt_resume);
+
 static struct platform_driver sama5d4_wdt_driver = {
 	.probe		= sama5d4_wdt_probe,
 	.remove		= sama5d4_wdt_remove,
 	.driver		= {
 		.name	= "sama5d4_wdt",
+		.pm	= &sama5d4_wdt_pm_ops,
 		.of_match_table = sama5d4_wdt_of_match,
 	}
 };
-- 
2.11.0

[toc] | [next] | [standalone]

#1583497

On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
> When resuming for the deepest state on sama5d2, it is necessary to restore
> MR as the registers are lost.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> Changes in v2:
>  - cache mr beofre suspending
>
>  drivers/watchdog/sama5d4_wdt.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 2a60251806d2..5ddeb4803dc3 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -28,6 +28,7 @@
>  struct sama5d4_wdt {
>  	struct watchdog_device	wdd;
>  	void __iomem		*reg_base;
> +	u32			mr;

Makes me wonder if we shouldn't just retain the original 'config'
(and maybe rename it to 'mr). After all, it _is_ used now.

>  };
>
>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> @@ -248,11 +249,42 @@ static const struct of_device_id sama5d4_wdt_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
>
> +#ifdef CONFIG_PM_SLEEP
> +static int sama5d4_wdt_suspend(struct device *dev)
> +{
> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> +
> +	wdt->mr = wdt_read(wdt, AT91_WDT_MR);
> +

Wouldn't you want to stop the watchdog here if it is running,
ie set AT91_WDT_WDDIS ?

> +	return 0;
> +}
> +
> +static int sama5d4_wdt_resume(struct device *dev)
> +{
> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = wdt_read(wdt, AT91_WDT_MR);
> +	if (reg & AT91_WDT_WDDIS)
> +		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> +
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);

Is that necessary ? Why not just write wdt->mr unconditionally ?

> +	if (wdt->mr & AT91_WDT_WDDIS)
> +		wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, sama5d4_wdt_suspend,
> +			 sama5d4_wdt_resume);
> +
>  static struct platform_driver sama5d4_wdt_driver = {
>  	.probe		= sama5d4_wdt_probe,
>  	.remove		= sama5d4_wdt_remove,
>  	.driver		= {
>  		.name	= "sama5d4_wdt",
> +		.pm	= &sama5d4_wdt_pm_ops,
>  		.of_match_table = sama5d4_wdt_of_match,
>  	}
>  };
>

[toc] | [prev] | [next] | [standalone]

#1583523

On 17/02/2017 at 06:47:03 -0800, Guenter Roeck wrote:
> On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
> > When resuming for the deepest state on sama5d2, it is necessary to restore
> > MR as the registers are lost.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > ---
> > Changes in v2:
> >  - cache mr beofre suspending
> > 
> >  drivers/watchdog/sama5d4_wdt.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> > index 2a60251806d2..5ddeb4803dc3 100644
> > --- a/drivers/watchdog/sama5d4_wdt.c
> > +++ b/drivers/watchdog/sama5d4_wdt.c
> > @@ -28,6 +28,7 @@
> >  struct sama5d4_wdt {
> >  	struct watchdog_device	wdd;
> >  	void __iomem		*reg_base;
> > +	u32			mr;
> 
> Makes me wonder if we shouldn't just retain the original 'config'
> (and maybe rename it to 'mr). After all, it _is_ used now.
> 
> >  };
> > 
> >  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> > @@ -248,11 +249,42 @@ static const struct of_device_id sama5d4_wdt_of_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
> > 
> > +#ifdef CONFIG_PM_SLEEP
> > +static int sama5d4_wdt_suspend(struct device *dev)
> > +{
> > +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> > +
> > +	wdt->mr = wdt_read(wdt, AT91_WDT_MR);
> > +
> 
> Wouldn't you want to stop the watchdog here if it is running,
> ie set AT91_WDT_WDDIS ?
> 

Some existing customers want the watchdog to continue to run while the
system is suspended.

> > +	return 0;
> > +}
> > +
> > +static int sama5d4_wdt_resume(struct device *dev)
> > +{
> > +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> > +	u32 reg;
> > +
> > +	reg = wdt_read(wdt, AT91_WDT_MR);
> > +	if (reg & AT91_WDT_WDDIS)
> > +		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> > +
> > +	wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> 
> Is that necessary ? Why not just write wdt->mr unconditionally ?
> 

Because you can change WDV and WDD *OR* WDDIS but not both at the same
time.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1584188

On 02/17/2017 07:22 AM, Alexandre Belloni wrote:
> On 17/02/2017 at 06:47:03 -0800, Guenter Roeck wrote:
>> On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
>>> When resuming for the deepest state on sama5d2, it is necessary to restore
>>> MR as the registers are lost.
>>>
>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>> ---
>>> Changes in v2:
>>>  - cache mr beofre suspending
>>>
>>>  drivers/watchdog/sama5d4_wdt.c | 32 ++++++++++++++++++++++++++++++++
>>>  1 file changed, 32 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
>>> index 2a60251806d2..5ddeb4803dc3 100644
>>> --- a/drivers/watchdog/sama5d4_wdt.c
>>> +++ b/drivers/watchdog/sama5d4_wdt.c
>>> @@ -28,6 +28,7 @@
>>>  struct sama5d4_wdt {
>>>  	struct watchdog_device	wdd;
>>>  	void __iomem		*reg_base;
>>> +	u32			mr;
>>
>> Makes me wonder if we shouldn't just retain the original 'config'
>> (and maybe rename it to 'mr). After all, it _is_ used now.
>>
>>>  };
>>>
>>>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
>>> @@ -248,11 +249,42 @@ static const struct of_device_id sama5d4_wdt_of_match[] = {
>>>  };
>>>  MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
>>>
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int sama5d4_wdt_suspend(struct device *dev)
>>> +{
>>> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>>> +
>>> +	wdt->mr = wdt_read(wdt, AT91_WDT_MR);
>>> +
>>
>> Wouldn't you want to stop the watchdog here if it is running,
>> ie set AT91_WDT_WDDIS ?
>>
>
> Some existing customers want the watchdog to continue to run while the
> system is suspended.
>
>>> +	return 0;
>>> +}
>>> +
>>> +static int sama5d4_wdt_resume(struct device *dev)
>>> +{
>>> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>>> +	u32 reg;
>>> +
>>> +	reg = wdt_read(wdt, AT91_WDT_MR);
>>> +	if (reg & AT91_WDT_WDDIS)
>>> +		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
>>> +
>>> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
>>
>> Is that necessary ? Why not just write wdt->mr unconditionally ?
>>
>
> Because you can change WDV and WDD *OR* WDDIS but not both at the same
> time.
>

Odd hardware ;-). Please add a comment explaining why the watchdog isn't
stopped on suspend, and why the AT91_WDT_MR has to be reprogrammed on resume.

Thanks,
Guenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web