Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
To: Marco Felsch <m.felsch@pengutronix.de>,
	Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Cc: Support Opensource <Support.Opensource@diasemi.com>,
	"wim@linux-watchdog.org" <wim@linux-watchdog.org>,
	"linux@roeck-us.net" <linux@roeck-us.net>,
	"linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>
Subject: RE: [PATCH] watchdog: da9062: add power management ops
Date: Mon, 2 Dec 2019 11:11:46 +0000
Message-ID: <AM5PR1001MB0994B329C5D4B8846FC7A71880430@AM5PR1001MB0994.EURPRD10.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20191202105646.xzqzlu2jnel4qbuv@pengutronix.de>

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 |

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-28 17:19 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 [this message]
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

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM5PR1001MB0994B329C5D4B8846FC7A71880430@AM5PR1001MB0994.EURPRD10.PROD.OUTLOOK.COM \
    --to=adam.thomson.opensource@diasemi.com \
    --cc=Support.Opensource@diasemi.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=m.felsch@pengutronix.de \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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