diff mbox series

[v2,1/1] watchdog: atmel: atmel-sama5d4-wdt: Disable watchdog on system suspend

Message ID 20190614125310.29458-1-ksloat@aampglobal.com (mailing list archive)
State Accepted
Headers show
Series [v2,1/1] watchdog: atmel: atmel-sama5d4-wdt: Disable watchdog on system suspend | expand

Commit Message

Ken Sloat June 14, 2019, 12:53 p.m. UTC 
From: Ken Sloat <ksloat@aampglobal.com>

Currently, the atmel-sama5d4-wdt continues to run after system suspend.
Unless the system resumes within the watchdog timeout period so the
userspace can kick it, the system will be reset. This change disables
the watchdog on suspend if it is active and re-enables on resume. These
actions occur during the late and early phases of suspend and resume
respectively to minimize chances where a lock could occur while the
watchdog is disabled.

Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
---
 Changes in v2:
 -Consolidate resume and resume early statements.

 drivers/watchdog/sama5d4_wdt.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Comments

Guenter Roeck June 14, 2019, 4:46 p.m. UTC | #1 
On Fri, Jun 14, 2019 at 12:53:22PM +0000, Ken Sloat wrote:
> From: Ken Sloat <ksloat@aampglobal.com>
> 
> Currently, the atmel-sama5d4-wdt continues to run after system suspend.
> Unless the system resumes within the watchdog timeout period so the
> userspace can kick it, the system will be reset. This change disables
> the watchdog on suspend if it is active and re-enables on resume. These
> actions occur during the late and early phases of suspend and resume
> respectively to minimize chances where a lock could occur while the
> watchdog is disabled.
> 
> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
> ---
>  Changes in v2:
>  -Consolidate resume and resume early statements.
> 
>  drivers/watchdog/sama5d4_wdt.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 111695223aae..0d123f8cbcc6 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -280,7 +280,17 @@ 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_resume(struct device *dev)
> +static int sama5d4_wdt_suspend_late(struct device *dev)
> +{
> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&wdt->wdd))
> +		sama5d4_wdt_stop(&wdt->wdd);
> +
> +	return 0;
> +}
> +
> +static int sama5d4_wdt_resume_early(struct device *dev)
>  {
>  	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>  
> @@ -291,12 +301,17 @@ static int sama5d4_wdt_resume(struct device *dev)
>  	 */
>  	sama5d4_wdt_init(wdt);
>  
> +	if (watchdog_active(&wdt->wdd))
> +		sama5d4_wdt_start(&wdt->wdd);
> +

The call to sama5d4_wdt_init() above now explicitly stops the watchdog
even if we want to (re)start it. I think this would be better handled
with an else case here

	else
		sama5d4_wdt_stop(&wdt->wdd);

Guenter

>  	return 0;
>  }
>  #endif
>  
> -static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, NULL,
> -			 sama5d4_wdt_resume);
> +static const struct dev_pm_ops sama5d4_wdt_pm_ops = {
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(sama5d4_wdt_suspend_late,
> +			sama5d4_wdt_resume_early)
> +};
>  
>  static struct platform_driver sama5d4_wdt_driver = {
>  	.probe		= sama5d4_wdt_probe,
> -- 
> 2.17.1
>
Ken Sloat June 14, 2019, 5:53 p.m. UTC | #2 
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Friday, June 14, 2019 12:46 PM
> To: Ken Sloat <KSloat@aampglobal.com>
> Cc: nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com;
> ludovic.desroches@microchip.com; wim@linux-watchdog.org; linux-arm-
> kernel@lists.infradead.org; linux-watchdog@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 1/1] watchdog: atmel: atmel-sama5d4-wdt: Disable
> watchdog on system suspend
> 
> [This is an EXTERNAL EMAIL]
> ________________________________
> 
> On Fri, Jun 14, 2019 at 12:53:22PM +0000, Ken Sloat wrote:
> > From: Ken Sloat <ksloat@aampglobal.com>
> >
> > Currently, the atmel-sama5d4-wdt continues to run after system suspend.
> > Unless the system resumes within the watchdog timeout period so the
> > userspace can kick it, the system will be reset. This change disables
> > the watchdog on suspend if it is active and re-enables on resume.
> > These actions occur during the late and early phases of suspend and
> > resume respectively to minimize chances where a lock could occur while
> > the watchdog is disabled.
> >
> > Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
> > ---
> >  Changes in v2:
> >  -Consolidate resume and resume early statements.
> >
> >  drivers/watchdog/sama5d4_wdt.c | 21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/watchdog/sama5d4_wdt.c
> > b/drivers/watchdog/sama5d4_wdt.c index 111695223aae..0d123f8cbcc6
> > 100644
> > --- a/drivers/watchdog/sama5d4_wdt.c
> > +++ b/drivers/watchdog/sama5d4_wdt.c
> > @@ -280,7 +280,17 @@ 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_resume(struct device *dev)
> > +static int sama5d4_wdt_suspend_late(struct device *dev) {
> > +     struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> > +
> > +     if (watchdog_active(&wdt->wdd))
> > +             sama5d4_wdt_stop(&wdt->wdd);
> > +
> > +     return 0;
> > +}
> > +
> > +static int sama5d4_wdt_resume_early(struct device *dev)
> >  {
> >       struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> >
> > @@ -291,12 +301,17 @@ static int sama5d4_wdt_resume(struct device
> *dev)
> >        */
> >       sama5d4_wdt_init(wdt);
> >
> > +     if (watchdog_active(&wdt->wdd))
> > +             sama5d4_wdt_start(&wdt->wdd);
> > +
> 
> The call to sama5d4_wdt_init() above now explicitly stops the watchdog
> even if we want to (re)start it. I think this would be better handled with an
> else case here
> 
>         else
>                 sama5d4_wdt_stop(&wdt->wdd);
> 

So we completely remove the sama5d4_wdt_init() call then correct?

To leave the code as it behaves today with the addition
of wdt stop/start, shouldn't we call init in the else instead?

	if (watchdog_active(&wdt->wdd))
		sama5d4_wdt_start(&wdt->wdd);
	else
		sama5d4_wdt_init();

I guess I don't really understand the purpose of having the init statement in resume
in the first place. I agree, calling this first does end up essentially resetting the wdt
it will start again if it was running before, but the count will be reset.

> Guenter
> 
> >       return 0;
> >  }
> >  #endif
> >
> > -static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, NULL,
> > -                      sama5d4_wdt_resume);
> > +static const struct dev_pm_ops sama5d4_wdt_pm_ops = {
> > +     SET_LATE_SYSTEM_SLEEP_PM_OPS(sama5d4_wdt_suspend_late,
> > +                     sama5d4_wdt_resume_early) };
> >
> >  static struct platform_driver sama5d4_wdt_driver = {
> >       .probe          = sama5d4_wdt_probe,
> > --
> > 2.17.1
> >

Thanks,
Ken Sloat
Alexandre Belloni June 14, 2019, 6:08 p.m. UTC | #3 
On 14/06/2019 17:53:01+0000, Ken Sloat wrote:
> > The call to sama5d4_wdt_init() above now explicitly stops the watchdog
> > even if we want to (re)start it. I think this would be better handled with an
> > else case here
> > 
> >         else
> >                 sama5d4_wdt_stop(&wdt->wdd);
> > 
> 
> So we completely remove the sama5d4_wdt_init() call then correct?
> 
> To leave the code as it behaves today with the addition
> of wdt stop/start, shouldn't we call init in the else instead?
> 
> 	if (watchdog_active(&wdt->wdd))
> 		sama5d4_wdt_start(&wdt->wdd);
> 	else
> 		sama5d4_wdt_init();
> 
> I guess I don't really understand the purpose of having the init statement in resume
> in the first place. I agree, calling this first does end up essentially resetting the wdt
> it will start again if it was running before, but the count will be reset.
> 

There is a nice comment explaining why ;)
Ken Sloat June 14, 2019, 6:43 p.m. UTC | #4 
> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Sent: Friday, June 14, 2019 2:08 PM
> To: Ken Sloat <KSloat@aampglobal.com>
> Cc: Guenter Roeck <linux@roeck-us.net>; nicolas.ferre@microchip.com;
> ludovic.desroches@microchip.com; wim@linux-watchdog.org; linux-arm-
> kernel@lists.infradead.org; linux-watchdog@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 1/1] watchdog: atmel: atmel-sama5d4-wdt: Disable
> watchdog on system suspend
> 
> [This is an EXTERNAL EMAIL]
> ________________________________
> 
> On 14/06/2019 17:53:01+0000, Ken Sloat wrote:
> > > The call to sama5d4_wdt_init() above now explicitly stops the
> > > watchdog even if we want to (re)start it. I think this would be
> > > better handled with an else case here
> > >
> > >         else
> > >                 sama5d4_wdt_stop(&wdt->wdd);
> > >
> >
> > So we completely remove the sama5d4_wdt_init() call then correct?
> >
> > To leave the code as it behaves today with the addition of wdt
> > stop/start, shouldn't we call init in the else instead?
> >
> >       if (watchdog_active(&wdt->wdd))
> >               sama5d4_wdt_start(&wdt->wdd);
> >       else
> >               sama5d4_wdt_init();
> >
> > I guess I don't really understand the purpose of having the init
> > statement in resume in the first place. I agree, calling this first
> > does end up essentially resetting the wdt it will start again if it was running
> before, but the count will be reset.
> >
> 
> There is a nice comment explaining why ;)

Well I'm a little confused still because there are two separate comments
in these statements. The first within resume implies that the init should
be called because we might have lost register values for some reason
unexplained. Then within the init it says that the bootloader might have
modified the registers so we should check them and then update it or
otherwise disable it. I'm not trying to pick apart the logic or anything, 
I'm just readily assuming it is good as it was already reviewed before. 

So without digging into that too much, if we don't know if any of the runtime
situations above might have occurred, then isn't it best to leave my patch
as is? Yes this has the side effect of resetting the timer count, but if 
the init call is needed and we don't have any way to know if any
of the situations occurred, then we have no choice right?

Happy to modify it either way, just didn't want to change
logic without understanding it thoroughly.

> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Thanks,
Ken Sloat
Alexandre Belloni June 14, 2019, 8:33 p.m. UTC | #5 
On 14/06/2019 18:43:22+0000, Ken Sloat wrote:
> Well I'm a little confused still because there are two separate comments
> in these statements. The first within resume implies that the init should
> be called because we might have lost register values for some reason
> unexplained.

The sama5d2 has a suspend mode where power to the core is completely
cut. Only a few IPs remain powered (in the backup power domain).
Unfortunately, the watchdog is not in that domain and may lose its
registers.

> Then within the init it says that the bootloader might have
> modified the registers so we should check them and then update it or
> otherwise disable it. I'm not trying to pick apart the logic or anything, 
> I'm just readily assuming it is good as it was already reviewed before. 
> 

The bootloaders may have started the watchdog (this makes sense if you
really care about reliability) and so we need to be careful to keep the
proper parameters.

> So without digging into that too much, if we don't know if any of the runtime
> situations above might have occurred, then isn't it best to leave my patch
> as is? Yes this has the side effect of resetting the timer count, but if 
> the init call is needed and we don't have any way to know if any
> of the situations occurred, then we have no choice right?
> 

Until we can differentiate between suspend modes, we have no other
choice.
Ken Sloat June 14, 2019, 8:45 p.m. UTC | #6 
> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Sent: Friday, June 14, 2019 4:33 PM
> To: Ken Sloat <KSloat@aampglobal.com>
> Cc: Guenter Roeck <linux@roeck-us.net>; nicolas.ferre@microchip.com;
> ludovic.desroches@microchip.com; wim@linux-watchdog.org; linux-arm-
> kernel@lists.infradead.org; linux-watchdog@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 1/1] watchdog: atmel: atmel-sama5d4-wdt: Disable
> watchdog on system suspend
> 
> [This is an EXTERNAL EMAIL]
> ________________________________
> 
> On 14/06/2019 18:43:22+0000, Ken Sloat wrote:
> > Well I'm a little confused still because there are two separate
> > comments in these statements. The first within resume implies that the
> > init should be called because we might have lost register values for
> > some reason unexplained.
> 
> The sama5d2 has a suspend mode where power to the core is completely
> cut. Only a few IPs remain powered (in the backup power domain).
> Unfortunately, the watchdog is not in that domain and may lose its registers.
> 
> > Then within the init it says that the bootloader might have modified
> > the registers so we should check them and then update it or otherwise
> > disable it. I'm not trying to pick apart the logic or anything, I'm
> > just readily assuming it is good as it was already reviewed before.
> >
> 
> The bootloaders may have started the watchdog (this makes sense if you
> really care about reliability) and so we need to be careful to keep the proper
> parameters.

Thanks for the explanation Alexandre I appreciate it.

> > So without digging into that too much, if we don't know if any of the
> > runtime situations above might have occurred, then isn't it best to
> > leave my patch as is? Yes this has the side effect of resetting the
> > timer count, but if the init call is needed and we don't have any way
> > to know if any of the situations occurred, then we have no choice right?
> >
> 
> Until we can differentiate between suspend modes, we have no other
> choice.

Ok I will leave my patch as is for now then

> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Thanks,
Ken Sloat
Guenter Roeck June 15, 2019, 2:22 p.m. UTC | #7 
On Fri, Jun 14, 2019 at 08:45:28PM +0000, Ken Sloat wrote:
> > -----Original Message-----
> > From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Sent: Friday, June 14, 2019 4:33 PM
> > To: Ken Sloat <KSloat@aampglobal.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>; nicolas.ferre@microchip.com;
> > ludovic.desroches@microchip.com; wim@linux-watchdog.org; linux-arm-
> > kernel@lists.infradead.org; linux-watchdog@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH v2 1/1] watchdog: atmel: atmel-sama5d4-wdt: Disable
> > watchdog on system suspend
> > 
> > [This is an EXTERNAL EMAIL]
> > ________________________________
> > 
> > On 14/06/2019 18:43:22+0000, Ken Sloat wrote:
> > > Well I'm a little confused still because there are two separate
> > > comments in these statements. The first within resume implies that the
> > > init should be called because we might have lost register values for
> > > some reason unexplained.
> > 
> > The sama5d2 has a suspend mode where power to the core is completely
> > cut. Only a few IPs remain powered (in the backup power domain).
> > Unfortunately, the watchdog is not in that domain and may lose its registers.
> > 
> > > Then within the init it says that the bootloader might have modified
> > > the registers so we should check them and then update it or otherwise
> > > disable it. I'm not trying to pick apart the logic or anything, I'm
> > > just readily assuming it is good as it was already reviewed before.
> > >
> > 
> > The bootloaders may have started the watchdog (this makes sense if you
> > really care about reliability) and so we need to be careful to keep the proper
> > parameters.
> 
> Thanks for the explanation Alexandre I appreciate it.
> 
> > > So without digging into that too much, if we don't know if any of the
> > > runtime situations above might have occurred, then isn't it best to
> > > leave my patch as is? Yes this has the side effect of resetting the
> > > timer count, but if the init call is needed and we don't have any way
> > > to know if any of the situations occurred, then we have no choice right?
> > >
> > 
> > Until we can differentiate between suspend modes, we have no other
> > choice.
> 
> Ok I will leave my patch as is for now then
> 

If that is what those involved in this discussion argue for, they will
need to confirm with Reviewed-by: or Acked-by: tags.

Thanks,
Guenter

> > --
> > Alexandre Belloni, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> 
> Thanks,
> Ken Sloat
Alexandre Belloni June 20, 2019, 8:33 a.m. UTC | #8 
On 14/06/2019 12:53:22+0000, Ken Sloat wrote:
> From: Ken Sloat <ksloat@aampglobal.com>
> 
> Currently, the atmel-sama5d4-wdt continues to run after system suspend.
> Unless the system resumes within the watchdog timeout period so the
> userspace can kick it, the system will be reset. This change disables
> the watchdog on suspend if it is active and re-enables on resume. These
> actions occur during the late and early phases of suspend and resume
> respectively to minimize chances where a lock could occur while the
> watchdog is disabled.
> 
> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  Changes in v2:
>  -Consolidate resume and resume early statements.
> 
>  drivers/watchdog/sama5d4_wdt.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 111695223aae..0d123f8cbcc6 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -280,7 +280,17 @@ 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_resume(struct device *dev)
> +static int sama5d4_wdt_suspend_late(struct device *dev)
> +{
> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&wdt->wdd))
> +		sama5d4_wdt_stop(&wdt->wdd);
> +
> +	return 0;
> +}
> +
> +static int sama5d4_wdt_resume_early(struct device *dev)
>  {
>  	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>  
> @@ -291,12 +301,17 @@ static int sama5d4_wdt_resume(struct device *dev)
>  	 */
>  	sama5d4_wdt_init(wdt);
>  
> +	if (watchdog_active(&wdt->wdd))
> +		sama5d4_wdt_start(&wdt->wdd);
> +
>  	return 0;
>  }
>  #endif
>  
> -static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, NULL,
> -			 sama5d4_wdt_resume);
> +static const struct dev_pm_ops sama5d4_wdt_pm_ops = {
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(sama5d4_wdt_suspend_late,
> +			sama5d4_wdt_resume_early)
> +};
>  
>  static struct platform_driver sama5d4_wdt_driver = {
>  	.probe		= sama5d4_wdt_probe,
> -- 
> 2.17.1
>
Guenter Roeck July 2, 2019, 1:40 p.m. UTC | #9 
On Fri, Jun 14, 2019 at 12:53:22PM +0000, Ken Sloat wrote:
> From: Ken Sloat <ksloat@aampglobal.com>
> 
> Currently, the atmel-sama5d4-wdt continues to run after system suspend.
> Unless the system resumes within the watchdog timeout period so the
> userspace can kick it, the system will be reset. This change disables
> the watchdog on suspend if it is active and re-enables on resume. These
> actions occur during the late and early phases of suspend and resume
> respectively to minimize chances where a lock could occur while the
> watchdog is disabled.
> 
> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

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

> ---
>  Changes in v2:
>  -Consolidate resume and resume early statements.
> 
>  drivers/watchdog/sama5d4_wdt.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 111695223aae..0d123f8cbcc6 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -280,7 +280,17 @@ 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_resume(struct device *dev)
> +static int sama5d4_wdt_suspend_late(struct device *dev)
> +{
> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&wdt->wdd))
> +		sama5d4_wdt_stop(&wdt->wdd);
> +
> +	return 0;
> +}
> +
> +static int sama5d4_wdt_resume_early(struct device *dev)
>  {
>  	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>  
> @@ -291,12 +301,17 @@ static int sama5d4_wdt_resume(struct device *dev)
>  	 */
>  	sama5d4_wdt_init(wdt);
>  
> +	if (watchdog_active(&wdt->wdd))
> +		sama5d4_wdt_start(&wdt->wdd);
> +
>  	return 0;
>  }
>  #endif
>  
> -static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, NULL,
> -			 sama5d4_wdt_resume);
> +static const struct dev_pm_ops sama5d4_wdt_pm_ops = {
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(sama5d4_wdt_suspend_late,
> +			sama5d4_wdt_resume_early)
> +};
>  
>  static struct platform_driver sama5d4_wdt_driver = {
>  	.probe		= sama5d4_wdt_probe,
diff mbox series

Patch

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 111695223aae..0d123f8cbcc6 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -280,7 +280,17 @@  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_resume(struct device *dev)
+static int sama5d4_wdt_suspend_late(struct device *dev)
+{
+	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(&wdt->wdd))
+		sama5d4_wdt_stop(&wdt->wdd);
+
+	return 0;
+}
+
+static int sama5d4_wdt_resume_early(struct device *dev)
 {
 	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
 
@@ -291,12 +301,17 @@  static int sama5d4_wdt_resume(struct device *dev)
 	 */
 	sama5d4_wdt_init(wdt);
 
+	if (watchdog_active(&wdt->wdd))
+		sama5d4_wdt_start(&wdt->wdd);
+
 	return 0;
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, NULL,
-			 sama5d4_wdt_resume);
+static const struct dev_pm_ops sama5d4_wdt_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(sama5d4_wdt_suspend_late,
+			sama5d4_wdt_resume_early)
+};
 
 static struct platform_driver sama5d4_wdt_driver = {
 	.probe		= sama5d4_wdt_probe,