Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] watchdog: da9062: add power management ops
@ 2019-11-28 17:19 Marco Felsch
  2019-11-28 17:41 ` Guenter Roeck
  2019-12-02 10:04 ` Adam Thomson
  0 siblings, 2 replies; 12+ messages in thread
From: Marco Felsch @ 2019-11-28 17:19 UTC (permalink / raw)
  To: support.opensource, wim, linux; +Cc: linux-watchdog, kernel

Disable the watchdog during suspend if it is enabled and re-enable it on
resume. So we can sleep without the interruptions.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/watchdog/da9062_wdt.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
index e149e66a6ea9..2a1e7de25b71 100644
--- a/drivers/watchdog/da9062_wdt.c
+++ b/drivers/watchdog/da9062_wdt.c
@@ -212,6 +212,7 @@ static int da9062_wdt_probe(struct platform_device *pdev)
 	watchdog_set_restart_priority(&wdt->wdtdev, 128);
 
 	watchdog_set_drvdata(&wdt->wdtdev, wdt);
+	dev_set_drvdata(dev, &wdt->wdtdev);
 
 	ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
 	if (ret < 0)
@@ -220,10 +221,34 @@ static int da9062_wdt_probe(struct platform_device *pdev)
 	return da9062_wdt_ping(&wdt->wdtdev);
 }
 
+static int __maybe_unused da9062_wdt_suspend(struct device *dev)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	if (watchdog_active(wdd))
+		return da9062_wdt_stop(wdd);
+
+	return 0;
+}
+
+static int __maybe_unused da9062_wdt_resume(struct device *dev)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	if (watchdog_active(wdd))
+		return da9062_wdt_start(wdd);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(da9062_wdt_pm_ops,
+			 da9062_wdt_suspend, da9062_wdt_resume);
+
 static struct platform_driver da9062_wdt_driver = {
 	.probe = da9062_wdt_probe,
 	.driver = {
 		.name = "da9062-watchdog",
+		.pm = &da9062_wdt_pm_ops,
 		.of_match_table = da9062_compatible_id_table,
 	},
 };
-- 
2.20.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] watchdog: da9062: add power management ops
  2019-11-28 17:19 [PATCH] watchdog: da9062: add power management ops Marco Felsch
@ 2019-11-28 17:41 ` Guenter Roeck
  2019-12-02 10:04 ` Adam Thomson
  1 sibling, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2019-11-28 17:41 UTC (permalink / raw)
  To: Marco Felsch, support.opensource, wim; +Cc: linux-watchdog, kernel

On 11/28/19 9:19 AM, Marco Felsch wrote:
> Disable the watchdog during suspend if it is enabled and re-enable it on
> resume. So we can sleep without the interruptions.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

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

> ---
>   drivers/watchdog/da9062_wdt.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index e149e66a6ea9..2a1e7de25b71 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -212,6 +212,7 @@ static int da9062_wdt_probe(struct platform_device *pdev)
>   	watchdog_set_restart_priority(&wdt->wdtdev, 128);
>   
>   	watchdog_set_drvdata(&wdt->wdtdev, wdt);
> +	dev_set_drvdata(dev, &wdt->wdtdev);
>   
>   	ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
>   	if (ret < 0)
> @@ -220,10 +221,34 @@ static int da9062_wdt_probe(struct platform_device *pdev)
>   	return da9062_wdt_ping(&wdt->wdtdev);
>   }
>   
> +static int __maybe_unused da9062_wdt_suspend(struct device *dev)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(wdd))
> +		return da9062_wdt_stop(wdd);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused da9062_wdt_resume(struct device *dev)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(wdd))
> +		return da9062_wdt_start(wdd);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(da9062_wdt_pm_ops,
> +			 da9062_wdt_suspend, da9062_wdt_resume);
> +
>   static struct platform_driver da9062_wdt_driver = {
>   	.probe = da9062_wdt_probe,
>   	.driver = {
>   		.name = "da9062-watchdog",
> +		.pm = &da9062_wdt_pm_ops,
>   		.of_match_table = da9062_compatible_id_table,
>   	},
>   };
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] watchdog: da9062: add power management ops
  2019-11-28 17:19 [PATCH] watchdog: da9062: add power management ops Marco Felsch
  2019-11-28 17:41 ` Guenter Roeck
@ 2019-12-02 10:04 ` Adam Thomson
  2019-12-02 10:56   ` Marco Felsch
  1 sibling, 1 reply; 12+ messages in thread
From: Adam Thomson @ 2019-12-02 10:04 UTC (permalink / raw)
  To: Marco Felsch, Support Opensource, wim, linux; +Cc: linux-watchdog, kernel

On 28 November 2019 17:20, Marco Felsch wrote:

> Disable the watchdog during suspend if it is enabled and re-enable it on
> resume. So we can sleep without the interruptions.
> 

We actually shouldn't need these additional functions. The PMIC can be told to
suspend the watchdog timer during the PMIC's powerdown state via the CONTROL_B
register which I think should do what you want here. That could be a DT option
instead, and normally this should be configured in OTP anyway I believe.

> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/watchdog/da9062_wdt.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index e149e66a6ea9..2a1e7de25b71 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -212,6 +212,7 @@ static int da9062_wdt_probe(struct platform_device
> *pdev)
>  	watchdog_set_restart_priority(&wdt->wdtdev, 128);
> 
>  	watchdog_set_drvdata(&wdt->wdtdev, wdt);
> +	dev_set_drvdata(dev, &wdt->wdtdev);
> 
>  	ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
>  	if (ret < 0)
> @@ -220,10 +221,34 @@ static int da9062_wdt_probe(struct platform_device
> *pdev)
>  	return da9062_wdt_ping(&wdt->wdtdev);
>  }
> 
> +static int __maybe_unused da9062_wdt_suspend(struct device *dev)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(wdd))
> +		return da9062_wdt_stop(wdd);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused da9062_wdt_resume(struct device *dev)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(wdd))
> +		return da9062_wdt_start(wdd);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(da9062_wdt_pm_ops,
> +			 da9062_wdt_suspend, da9062_wdt_resume);
> +
>  static struct platform_driver da9062_wdt_driver = {
>  	.probe = da9062_wdt_probe,
>  	.driver = {
>  		.name = "da9062-watchdog",
> +		.pm = &da9062_wdt_pm_ops,
>  		.of_match_table = da9062_compatible_id_table,
>  	},
>  };
> --
> 2.20.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] watchdog: da9062: add power management ops
  2019-12-02 10:04 ` Adam Thomson
@ 2019-12-02 10:56   ` Marco Felsch
  2019-12-02 11:11     ` Adam Thomson
  0 siblings, 1 reply; 12+ messages in thread
From: Marco Felsch @ 2019-12-02 10:56 UTC (permalink / raw)
  To: Adam Thomson; +Cc: Support Opensource, wim, linux, linux-watchdog, kernel

Hi Adam,

On 19-12-02 10:04, Adam Thomson wrote:
> On 28 November 2019 17:20, Marco Felsch wrote:
> 
> > Disable the watchdog during suspend if it is enabled and re-enable it on
> > resume. So we can sleep without the interruptions.
> > 
> 
> We actually shouldn't need these additional functions. The PMIC can be told to
> suspend the watchdog timer during the PMIC's powerdown state via the CONTROL_B
> register which I think should do what you want here. That could be a DT option
> instead, and normally this should be configured in OTP anyway I believe.

This isn't always the case. My custom PCB haven't the ability to use the
sequencer powerdown/active mode becuase of a PCB bug. So without this
patch the PMIC resets my system.

Regards,
  Marco 

> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/watchdog/da9062_wdt.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> > index e149e66a6ea9..2a1e7de25b71 100644
> > --- a/drivers/watchdog/da9062_wdt.c
> > +++ b/drivers/watchdog/da9062_wdt.c
> > @@ -212,6 +212,7 @@ static int da9062_wdt_probe(struct platform_device
> > *pdev)
> >  	watchdog_set_restart_priority(&wdt->wdtdev, 128);
> > 
> >  	watchdog_set_drvdata(&wdt->wdtdev, wdt);
> > +	dev_set_drvdata(dev, &wdt->wdtdev);
> > 
> >  	ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
> >  	if (ret < 0)
> > @@ -220,10 +221,34 @@ static int da9062_wdt_probe(struct platform_device
> > *pdev)
> >  	return da9062_wdt_ping(&wdt->wdtdev);
> >  }
> > 
> > +static int __maybe_unused da9062_wdt_suspend(struct device *dev)
> > +{
> > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > +
> > +	if (watchdog_active(wdd))
> > +		return da9062_wdt_stop(wdd);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused da9062_wdt_resume(struct device *dev)
> > +{
> > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > +
> > +	if (watchdog_active(wdd))
> > +		return da9062_wdt_start(wdd);
> > +
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(da9062_wdt_pm_ops,
> > +			 da9062_wdt_suspend, da9062_wdt_resume);
> > +
> >  static struct platform_driver da9062_wdt_driver = {
> >  	.probe = da9062_wdt_probe,
> >  	.driver = {
> >  		.name = "da9062-watchdog",
> > +		.pm = &da9062_wdt_pm_ops,
> >  		.of_match_table = da9062_compatible_id_table,
> >  	},
> >  };
> > --
> > 2.20.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] watchdog: da9062: add power management ops
  2019-12-02 10:56   ` Marco Felsch
@ 2019-12-02 11:11     ` Adam Thomson
  2019-12-02 13:03       ` Marco Felsch
  0 siblings, 1 reply; 12+ messages in thread
From: Adam Thomson @ 2019-12-02 11:11 UTC (permalink / raw)
  To: Marco Felsch, Adam Thomson
  Cc: Support Opensource, wim, linux, linux-watchdog, kernel

On 02 December 2019 10:57, Marco Felsch wrote:

> On 19-12-02 10:04, Adam Thomson wrote:
> > On 28 November 2019 17:20, Marco Felsch wrote:
> >
> > > Disable the watchdog during suspend if it is enabled and re-enable it on
> > > resume. So we can sleep without the interruptions.
> > >
> >
> > We actually shouldn't need these additional functions. The PMIC can be told to
> > suspend the watchdog timer during the PMIC's powerdown state via the
> CONTROL_B
> > register which I think should do what you want here. That could be a DT option
> > instead, and normally this should be configured in OTP anyway I believe.
> 
> This isn't always the case. My custom PCB haven't the ability to use the
> sequencer powerdown/active mode becuase of a PCB bug. So without this
> patch the PMIC resets my system.

Hmmm. Wouldn't that then be a board specific fix rather than this change?
Am concerned relying on software to reenable the watchdog on resume could allow
for a hang scenario potentially if that code never gets to execute. Other
systems shouldn't need this fix, assuming they don't have issues at the HW
level, so this this seems like it could be making those systems less robust. If
we are to do this at the driver level, maybe this should be an option through DT
to help faulty systems, but I'm thinking this shouldn't be default behaviour.

> 
> Regards,
>   Marco
> 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  drivers/watchdog/da9062_wdt.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/watchdog/da9062_wdt.c
> b/drivers/watchdog/da9062_wdt.c
> > > index e149e66a6ea9..2a1e7de25b71 100644
> > > --- a/drivers/watchdog/da9062_wdt.c
> > > +++ b/drivers/watchdog/da9062_wdt.c
> > > @@ -212,6 +212,7 @@ static int da9062_wdt_probe(struct platform_device
> > > *pdev)
> > >  	watchdog_set_restart_priority(&wdt->wdtdev, 128);
> > >
> > >  	watchdog_set_drvdata(&wdt->wdtdev, wdt);
> > > +	dev_set_drvdata(dev, &wdt->wdtdev);
> > >
> > >  	ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
> > >  	if (ret < 0)
> > > @@ -220,10 +221,34 @@ static int da9062_wdt_probe(struct platform_device
> > > *pdev)
> > >  	return da9062_wdt_ping(&wdt->wdtdev);
> > >  }
> > >
> > > +static int __maybe_unused da9062_wdt_suspend(struct device *dev)
> > > +{
> > > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > > +
> > > +	if (watchdog_active(wdd))
> > > +		return da9062_wdt_stop(wdd);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __maybe_unused da9062_wdt_resume(struct device *dev)
> > > +{
> > > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > > +
> > > +	if (watchdog_active(wdd))
> > > +		return da9062_wdt_start(wdd);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static SIMPLE_DEV_PM_OPS(da9062_wdt_pm_ops,
> > > +			 da9062_wdt_suspend, da9062_wdt_resume);
> > > +
> > >  static struct platform_driver da9062_wdt_driver = {
> > >  	.probe = da9062_wdt_probe,
> > >  	.driver = {
> > >  		.name = "da9062-watchdog",
> > > +		.pm = &da9062_wdt_pm_ops,
> > >  		.of_match_table = da9062_compatible_id_table,
> > >  	},
> > >  };
> > > --
> > > 2.20.1
> >
> >
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] watchdog: da9062: add power management ops
  2019-12-02 11:11     ` Adam Thomson
@ 2019-12-02 13:03       ` Marco Felsch
  2019-12-02 13:27         ` Adam Thomson
  0 siblings, 1 reply; 12+ messages in thread
From: Marco Felsch @ 2019-12-02 13:03 UTC (permalink / raw)
  To: Adam Thomson; +Cc: Support Opensource, wim, linux, linux-watchdog, kernel

Hi Adam,

On 19-12-02 11:11, Adam Thomson wrote:
> On 02 December 2019 10:57, Marco Felsch wrote:
> 
> > On 19-12-02 10:04, Adam Thomson wrote:
> > > On 28 November 2019 17:20, Marco Felsch wrote:
> > >
> > > > Disable the watchdog during suspend if it is enabled and re-enable it on
> > > > resume. So we can sleep without the interruptions.
> > > >
> > >
> > > We actually shouldn't need these additional functions. The PMIC can be told to
> > > suspend the watchdog timer during the PMIC's powerdown state via the
> > CONTROL_B
> > > register which I think should do what you want here. That could be a DT option
> > > instead, and normally this should be configured in OTP anyway I believe.
> > 
> > This isn't always the case. My custom PCB haven't the ability to use the
> > sequencer powerdown/active mode becuase of a PCB bug. So without this
> > patch the PMIC resets my system.
> 
> Hmmm. Wouldn't that then be a board specific fix rather than this change?
> Am concerned relying on software to reenable the watchdog on resume could allow
> for a hang scenario potentially if that code never gets to execute. Other
> systems shouldn't need this fix, assuming they don't have issues at the HW
> level, so this this seems like it could be making those systems less robust. If
> we are to do this at the driver level, maybe this should be an option through DT
> to help faulty systems, but I'm thinking this shouldn't be default behaviour.

I don't think that we should rely on the OTP values. Those values are
set by Dialog, the SoM manufacturers or by the customer itself and the
time shows that this is error prone too. At least if the customer or the
SoM manufacturer don't ask the Dialog Support..

You're right with the (re-)enabling but there are other drivers using
such an approach.. IMHO it is safer to go this way rather than trust
the OTP and the PCB layout. I would rather add a dt-compatible to tell
the driver to do nothing during suspend/resume e.g.:

    - dlg,use-hw-pm

or something. Because the user needs to validate the PCB and the OTP
values.

Regards,
  Marco

> > 
> > Regards,
> >   Marco
> > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  drivers/watchdog/da9062_wdt.c | 25 +++++++++++++++++++++++++
> > > >  1 file changed, 25 insertions(+)
> > > >
> > > > diff --git a/drivers/watchdog/da9062_wdt.c
> > b/drivers/watchdog/da9062_wdt.c
> > > > index e149e66a6ea9..2a1e7de25b71 100644
> > > > --- a/drivers/watchdog/da9062_wdt.c
> > > > +++ b/drivers/watchdog/da9062_wdt.c
> > > > @@ -212,6 +212,7 @@ static int da9062_wdt_probe(struct platform_device
> > > > *pdev)
> > > >  	watchdog_set_restart_priority(&wdt->wdtdev, 128);
> > > >
> > > >  	watchdog_set_drvdata(&wdt->wdtdev, wdt);
> > > > +	dev_set_drvdata(dev, &wdt->wdtdev);
> > > >
> > > >  	ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
> > > >  	if (ret < 0)
> > > > @@ -220,10 +221,34 @@ static int da9062_wdt_probe(struct platform_device
> > > > *pdev)
> > > >  	return da9062_wdt_ping(&wdt->wdtdev);
> > > >  }
> > > >
> > > > +static int __maybe_unused da9062_wdt_suspend(struct device *dev)
> > > > +{
> > > > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > > > +
> > > > +	if (watchdog_active(wdd))
> > > > +		return da9062_wdt_stop(wdd);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int __maybe_unused da9062_wdt_resume(struct device *dev)
> > > > +{
> > > > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > > > +
> > > > +	if (watchdog_active(wdd))
> > > > +		return da9062_wdt_start(wdd);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static SIMPLE_DEV_PM_OPS(da9062_wdt_pm_ops,
> > > > +			 da9062_wdt_suspend, da9062_wdt_resume);
> > > > +
> > > >  static struct platform_driver da9062_wdt_driver = {
> > > >  	.probe = da9062_wdt_probe,
> > > >  	.driver = {
> > > >  		.name = "da9062-watchdog",
> > > > +		.pm = &da9062_wdt_pm_ops,
> > > >  		.of_match_table = da9062_compatible_id_table,
> > > >  	},
> > > >  };
> > > > --
> > > > 2.20.1
> > >
> > >
> > 
> > --
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] watchdog: da9062: add power management ops
  2019-12-02 13:03       ` Marco Felsch
@ 2019-12-02 13:27         ` Adam Thomson
  2019-12-02 13:38           ` Guenter Roeck
  0 siblings, 1 reply; 12+ messages in thread
From: Adam Thomson @ 2019-12-02 13:27 UTC (permalink / raw)
  To: Marco Felsch, Adam Thomson
  Cc: Support Opensource, wim, linux, linux-watchdog, kernel

Hi Marco,

> > Hmmm. Wouldn't that then be a board specific fix rather than this change?
> > Am concerned relying on software to reenable the watchdog on resume could
> allow
> > for a hang scenario potentially if that code never gets to execute. Other
> > systems shouldn't need this fix, assuming they don't have issues at the HW
> > level, so this this seems like it could be making those systems less robust. If
> > we are to do this at the driver level, maybe this should be an option through DT
> > to help faulty systems, but I'm thinking this shouldn't be default behaviour.
> 
> I don't think that we should rely on the OTP values. Those values are
> set by Dialog, the SoM manufacturers or by the customer itself and the
> time shows that this is error prone too. At least if the customer or the
> SoM manufacturer don't ask the Dialog Support..
> 
> You're right with the (re-)enabling but there are other drivers using
> such an approach.. IMHO it is safer to go this way rather than trust
> the OTP and the PCB layout. I would rather add a dt-compatible to tell
> the driver to do nothing during suspend/resume e.g.:
> 
>     - dlg,use-hw-pm
> 
> or something. Because the user needs to validate the PCB and the OTP
> values.

The thing is the DT FW is supposed to be fairly static so I would rather not
enforce this change on users if they adopt a kernel version with this update in.
I also still think it's safer if the HW does this for us. I would have hoped for
most designs this would be caught in early bring up where it can be rectified
with minimal impact, although I'm guessing that's not the case in your scenario.

> Regards,
>   Marco

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] watchdog: da9062: add power management ops
  2019-12-02 13:27         ` Adam Thomson
@ 2019-12-02 13:38           ` Guenter Roeck
  2019-12-02 14:44             ` Marco Felsch
  2019-12-02 14:44             ` Adam Thomson
  0 siblings, 2 replies; 12+ messages in thread
From: Guenter Roeck @ 2019-12-02 13:38 UTC (permalink / raw)
  To: Adam Thomson, Marco Felsch
  Cc: Support Opensource, wim, linux-watchdog, kernel

On 12/2/19 5:27 AM, Adam Thomson wrote:
> Hi Marco,
> 
>>> Hmmm. Wouldn't that then be a board specific fix rather than this change?
>>> Am concerned relying on software to reenable the watchdog on resume could
>> allow
>>> for a hang scenario potentially if that code never gets to execute. Other
>>> systems shouldn't need this fix, assuming they don't have issues at the HW
>>> level, so this this seems like it could be making those systems less robust. If
>>> we are to do this at the driver level, maybe this should be an option through DT
>>> to help faulty systems, but I'm thinking this shouldn't be default behaviour.
>>
>> I don't think that we should rely on the OTP values. Those values are
>> set by Dialog, the SoM manufacturers or by the customer itself and the
>> time shows that this is error prone too. At least if the customer or the
>> SoM manufacturer don't ask the Dialog Support..
>>
>> You're right with the (re-)enabling but there are other drivers using
>> such an approach.. IMHO it is safer to go this way rather than trust
>> the OTP and the PCB layout. I would rather add a dt-compatible to tell
>> the driver to do nothing during suspend/resume e.g.:
>>
>>      - dlg,use-hw-pm
>>
>> or something. Because the user needs to validate the PCB and the OTP
>> values.
> 
> The thing is the DT FW is supposed to be fairly static so I would rather not
> enforce this change on users if they adopt a kernel version with this update in.
> I also still think it's safer if the HW does this for us. I would have hoped for
> most designs this would be caught in early bring up where it can be rectified
> with minimal impact, although I'm guessing that's not the case in your scenario.
> 

dla,use-sw-pm ?
dla,hw-pm-broken ?

Guenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] watchdog: da9062: add power management ops
  2019-12-02 13:38           ` Guenter Roeck
@ 2019-12-02 14:44             ` Marco Felsch
  2019-12-02 14:44             ` Adam Thomson
  1 sibling, 0 replies; 12+ messages in thread
From: Marco Felsch @ 2019-12-02 14:44 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Adam Thomson, Support Opensource, wim, linux-watchdog, kernel

On 19-12-02 05:38, Guenter Roeck wrote:
> On 12/2/19 5:27 AM, Adam Thomson wrote:
> > Hi Marco,
> > 
> > > > Hmmm. Wouldn't that then be a board specific fix rather than this change?
> > > > Am concerned relying on software to reenable the watchdog on resume could
> > > allow
> > > > for a hang scenario potentially if that code never gets to execute. Other
> > > > systems shouldn't need this fix, assuming they don't have issues at the HW
> > > > level, so this this seems like it could be making those systems less robust. If
> > > > we are to do this at the driver level, maybe this should be an option through DT
> > > > to help faulty systems, but I'm thinking this shouldn't be default behaviour.
> > > 
> > > I don't think that we should rely on the OTP values. Those values are
> > > set by Dialog, the SoM manufacturers or by the customer itself and the
> > > time shows that this is error prone too. At least if the customer or the
> > > SoM manufacturer don't ask the Dialog Support..
> > > 
> > > You're right with the (re-)enabling but there are other drivers using
> > > such an approach.. IMHO it is safer to go this way rather than trust
> > > the OTP and the PCB layout. I would rather add a dt-compatible to tell
> > > the driver to do nothing during suspend/resume e.g.:
> > > 
> > >      - dlg,use-hw-pm
> > > 
> > > or something. Because the user needs to validate the PCB and the OTP
> > > values.
> > 
> > The thing is the DT FW is supposed to be fairly static so I would rather not
> > enforce this change on users if they adopt a kernel version with this update in.
> > I also still think it's safer if the HW does this for us. I would have hoped for
> > most designs this would be caught in early bring up where it can be rectified
> > with minimal impact, although I'm guessing that's not the case in your scenario.

Currently there is only the phytec-phycore upstream DT using the
da9061/2 PMIC... Anyway I got your concerns.

> dla,use-sw-pm ?
> dla,hw-pm-broken ?

Yes one of these. Which do you prefer Adam?

Regards,
  Marco 

> 
> Guenter
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] watchdog: da9062: add power management ops
  2019-12-02 13:38           ` Guenter Roeck
  2019-12-02 14:44             ` Marco Felsch
@ 2019-12-02 14:44             ` Adam Thomson
  2019-12-02 15:06               ` Marco Felsch
  1 sibling, 1 reply; 12+ messages in thread
From: Adam Thomson @ 2019-12-02 14:44 UTC (permalink / raw)
  To: Guenter Roeck, Adam Thomson, Marco Felsch
  Cc: Support Opensource, wim, linux-watchdog, kernel

On 02 December 2019 13:38, Adam Thomson wrote:

> On 12/2/19 5:27 AM, Adam Thomson wrote:
> > Hi Marco,
> >
> >>> Hmmm. Wouldn't that then be a board specific fix rather than this change?
> >>> Am concerned relying on software to reenable the watchdog on resume
> could
> >> allow
> >>> for a hang scenario potentially if that code never gets to execute. Other
> >>> systems shouldn't need this fix, assuming they don't have issues at the HW
> >>> level, so this this seems like it could be making those systems less robust. If
> >>> we are to do this at the driver level, maybe this should be an option through
> DT
> >>> to help faulty systems, but I'm thinking this shouldn't be default behaviour.
> >>
> >> I don't think that we should rely on the OTP values. Those values are
> >> set by Dialog, the SoM manufacturers or by the customer itself and the
> >> time shows that this is error prone too. At least if the customer or the
> >> SoM manufacturer don't ask the Dialog Support..
> >>
> >> You're right with the (re-)enabling but there are other drivers using
> >> such an approach.. IMHO it is safer to go this way rather than trust
> >> the OTP and the PCB layout. I would rather add a dt-compatible to tell
> >> the driver to do nothing during suspend/resume e.g.:
> >>
> >>      - dlg,use-hw-pm
> >>
> >> or something. Because the user needs to validate the PCB and the OTP
> >> values.
> >
> > The thing is the DT FW is supposed to be fairly static so I would rather not
> > enforce this change on users if they adopt a kernel version with this update in.
> > I also still think it's safer if the HW does this for us. I would have hoped for
> > most designs this would be caught in early bring up where it can be rectified
> > with minimal impact, although I'm guessing that's not the case in your scenario.
> >
> 
> dla,use-sw-pm ?
> dla,hw-pm-broken ?

Yes, I'd probably opt for 'dlg,use-sw-pm' or maybe 'dlg,manual-pm' to cover it.

> 
> Guenter

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] watchdog: da9062: add power management ops
  2019-12-02 14:44             ` Adam Thomson
@ 2019-12-02 15:06               ` Marco Felsch
  2019-12-02 15:17                 ` Adam Thomson
  0 siblings, 1 reply; 12+ messages in thread
From: Marco Felsch @ 2019-12-02 15:06 UTC (permalink / raw)
  To: Adam Thomson
  Cc: Guenter Roeck, wim, Support Opensource, kernel, linux-watchdog

On 19-12-02 14:44, Adam Thomson wrote:
> On 02 December 2019 13:38, Adam Thomson wrote:
> 
> > On 12/2/19 5:27 AM, Adam Thomson wrote:
> > > Hi Marco,
> > >
> > >>> Hmmm. Wouldn't that then be a board specific fix rather than this change?
> > >>> Am concerned relying on software to reenable the watchdog on resume
> > could
> > >> allow
> > >>> for a hang scenario potentially if that code never gets to execute. Other
> > >>> systems shouldn't need this fix, assuming they don't have issues at the HW
> > >>> level, so this this seems like it could be making those systems less robust. If
> > >>> we are to do this at the driver level, maybe this should be an option through
> > DT
> > >>> to help faulty systems, but I'm thinking this shouldn't be default behaviour.
> > >>
> > >> I don't think that we should rely on the OTP values. Those values are
> > >> set by Dialog, the SoM manufacturers or by the customer itself and the
> > >> time shows that this is error prone too. At least if the customer or the
> > >> SoM manufacturer don't ask the Dialog Support..
> > >>
> > >> You're right with the (re-)enabling but there are other drivers using
> > >> such an approach.. IMHO it is safer to go this way rather than trust
> > >> the OTP and the PCB layout. I would rather add a dt-compatible to tell
> > >> the driver to do nothing during suspend/resume e.g.:
> > >>
> > >>      - dlg,use-hw-pm
> > >>
> > >> or something. Because the user needs to validate the PCB and the OTP
> > >> values.
> > >
> > > The thing is the DT FW is supposed to be fairly static so I would rather not
> > > enforce this change on users if they adopt a kernel version with this update in.
> > > I also still think it's safer if the HW does this for us. I would have hoped for
> > > most designs this would be caught in early bring up where it can be rectified
> > > with minimal impact, although I'm guessing that's not the case in your scenario.
> > >
> > 
> > dla,use-sw-pm ?
> > dla,hw-pm-broken ?
> 
> Yes, I'd probably opt for 'dlg,use-sw-pm' or maybe 'dlg,manual-pm' to cover it.

Okay, so we are all agree with the dlg,use-sw-pm?

Regards,
  Marco

> 
> > 
> > Guenter

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] watchdog: da9062: add power management ops
  2019-12-02 15:06               ` Marco Felsch
@ 2019-12-02 15:17                 ` Adam Thomson
  0 siblings, 0 replies; 12+ messages in thread
From: Adam Thomson @ 2019-12-02 15:17 UTC (permalink / raw)
  To: Marco Felsch, Adam Thomson
  Cc: Guenter Roeck, wim, Support Opensource, kernel, linux-watchdog

On 02 December 2019 15:06, Marco Felsch wrote:

> On 19-12-02 14:44, Adam Thomson wrote:
> > On 02 December 2019 13:38, Adam Thomson wrote:
> >
> > > On 12/2/19 5:27 AM, Adam Thomson wrote:
> > > > Hi Marco,
> > > >
> > > >>> Hmmm. Wouldn't that then be a board specific fix rather than this
> change?
> > > >>> Am concerned relying on software to reenable the watchdog on resume
> > > could
> > > >> allow
> > > >>> for a hang scenario potentially if that code never gets to execute. Other
> > > >>> systems shouldn't need this fix, assuming they don't have issues at the
> HW
> > > >>> level, so this this seems like it could be making those systems less robust.
> If
> > > >>> we are to do this at the driver level, maybe this should be an option
> through
> > > DT
> > > >>> to help faulty systems, but I'm thinking this shouldn't be default
> behaviour.
> > > >>
> > > >> I don't think that we should rely on the OTP values. Those values are
> > > >> set by Dialog, the SoM manufacturers or by the customer itself and the
> > > >> time shows that this is error prone too. At least if the customer or the
> > > >> SoM manufacturer don't ask the Dialog Support..
> > > >>
> > > >> You're right with the (re-)enabling but there are other drivers using
> > > >> such an approach.. IMHO it is safer to go this way rather than trust
> > > >> the OTP and the PCB layout. I would rather add a dt-compatible to tell
> > > >> the driver to do nothing during suspend/resume e.g.:
> > > >>
> > > >>      - dlg,use-hw-pm
> > > >>
> > > >> or something. Because the user needs to validate the PCB and the OTP
> > > >> values.
> > > >
> > > > The thing is the DT FW is supposed to be fairly static so I would rather not
> > > > enforce this change on users if they adopt a kernel version with this update
> in.
> > > > I also still think it's safer if the HW does this for us. I would have hoped for
> > > > most designs this would be caught in early bring up where it can be rectified
> > > > with minimal impact, although I'm guessing that's not the case in your
> scenario.
> > > >
> > >
> > > dla,use-sw-pm ?
> > > dla,hw-pm-broken ?
> >
> > Yes, I'd probably opt for 'dlg,use-sw-pm' or maybe 'dlg,manual-pm' to cover it.
> 
> Okay, so we are all agree with the dlg,use-sw-pm?

Yep, that's ok for me

> 
> Regards,
>   Marco
> 
> >
> > >
> > > Guenter
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28 17:19 [PATCH] watchdog: da9062: add power management ops Marco Felsch
2019-11-28 17:41 ` Guenter Roeck
2019-12-02 10:04 ` Adam Thomson
2019-12-02 10:56   ` Marco Felsch
2019-12-02 11:11     ` Adam Thomson
2019-12-02 13:03       ` Marco Felsch
2019-12-02 13:27         ` Adam Thomson
2019-12-02 13:38           ` Guenter Roeck
2019-12-02 14:44             ` Marco Felsch
2019-12-02 14:44             ` Adam Thomson
2019-12-02 15:06               ` Marco Felsch
2019-12-02 15:17                 ` Adam Thomson

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git