Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Anson Huang <anson.huang@nxp.com>
To: Guenter Roeck <linux@roeck-us.net>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"wim@linux-watchdog.org" <wim@linux-watchdog.org>,
	Aisheng Dong <aisheng.dong@nxp.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>
Cc: dl-linux-imx <linux-imx@nxp.com>
Subject: RE: [PATCH 2/2] watchdog: imx_sc: Add pretimeout support
Date: Thu, 25 Apr 2019 05:44:45 +0000
Message-ID: <DB3PR0402MB3916867C62AEB8999EC9EBD4F53D0@DB3PR0402MB3916.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <21bd7656-541e-5f77-af8e-a5ea40b904a6@roeck-us.net>

Hi, Guenter

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Thursday, April 25, 2019 12:04 PM
> To: Anson Huang <anson.huang@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> wim@linux-watchdog.org; Aisheng Dong <aisheng.dong@nxp.com>; linux-
> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> watchdog@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 2/2] watchdog: imx_sc: Add pretimeout support
> 
> On 4/24/19 6:14 PM, Anson Huang wrote:
> > i.MX system controller watchdog can support pretimeout IRQ via general
> > SCU MU IRQ, it depends on IMX_SCU and driver MUST be probed after SCU
> > IPC ready, then enable corresponding SCU IRQ group and register SCU
> > IRQ notifier, when watchdog pretimeout IRQ fires, SCU MU IRQ will be
> > handled and watchdog pretimeout notifier will be called to handle the
> > event.
> >
> 
> Ah, here is the missing patch.
> 
> As mentioned in my other reply, the watchdog driver does now depend on
> the SCU IPC handle and should be instantiated accordingly.
> Using -EPROBE_DEFER to work around bad dependencies is not a solution.

So, I have to move the i.MX system controller watchdog node into the i.MX SCU
node in DT file now? As it depends on i.MX SCU firmware. If so, should I remove the
previous i.MX system controller binding doc (fsl-imx-sc-wdt.txt) and add binding doc
to (Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt) ?  

> 
> Additional comment below.
> 
> Guenter
> 
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >   drivers/watchdog/Kconfig      |  1 +
> >   drivers/watchdog/imx_sc_wdt.c | 65
> +++++++++++++++++++++++++++++++++++++++----
> >   2 files changed, 61 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > 44a3158..f2c2c1a 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -644,6 +644,7 @@ config IMX2_WDT
> >   config IMX_SC_WDT
> >   	tristate "IMX SC Watchdog"
> >   	depends on HAVE_ARM_SMCCC
> > +	depends on IMX_SCU
> >   	select WATCHDOG_CORE
> >   	help
> >   	  This is the driver for the system controller watchdog diff --git
> > a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c index
> > 49848b6..f45ed10 100644
> > --- a/drivers/watchdog/imx_sc_wdt.c
> > +++ b/drivers/watchdog/imx_sc_wdt.c
> > @@ -4,6 +4,7 @@
> >    */
> >
> >   #include <linux/arm-smccc.h>
> > +#include <linux/firmware/imx/sci.h>
> >   #include <linux/io.h>
> >   #include <linux/init.h>
> >   #include <linux/kernel.h>
> > @@ -33,11 +34,16 @@
> >
> >   #define SC_TIMER_WDOG_ACTION_PARTITION	0
> >
> > +#define SC_IRQ_WDOG			1
> > +#define SC_IRQ_GROUP_WDOG		1
> > +
> >   static bool nowayout = WATCHDOG_NOWAYOUT;
> >   module_param(nowayout, bool, 0000);
> >   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once
> started (default="
> >   		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >
> > +struct watchdog_device *imx_sc_wdd;
> > +
> >   static int imx_sc_wdt_ping(struct watchdog_device *wdog)
> >   {
> >   	struct arm_smccc_res res;
> > @@ -85,12 +91,42 @@ static int imx_sc_wdt_set_timeout(struct
> watchdog_device *wdog,
> >   	return res.a0 ? -EACCES : 0;
> >   }
> >
> > +static int imx_sc_wdt_set_pretimeout(struct watchdog_device *wdog,
> > +				     unsigned int pretimeout)
> > +{
> > +	struct arm_smccc_res res;
> > +
> > +	wdog->pretimeout = pretimeout;
> > +	arm_smccc_smc(IMX_SIP_TIMER,
> IMX_SIP_TIMER_SET_PRETIME_WDOG,
> > +		      pretimeout * 1000, 0, 0, 0, 0, 0, &res);
> > +
> > +	return res.a0 ? -EACCES : 0;
> 
> If this function returns an error, why does it set wdog->pretimeout
> unconditionally ? That seems wrong.

You are right, I will fix it in V2, but looks like some other watchdog drivers also has such issue, such
as below:

drivers/watchdog/pm8916_wdt.c
drivers/watchdog/sprd_wdt.c

> 
> > +}
> > +
> > +static int imx_sc_wdt_notify(struct notifier_block *nb,
> > +			     unsigned long event, void *group) {
> > +	/* ignore other irqs */
> > +	if (!(event & SC_IRQ_WDOG &&
> > +		(*(u8 *)group == SC_IRQ_GROUP_WDOG)))
> 
> 	if (!(event & SC_IRQ_WDO) || *(u8 *)group !=
> SC_IRQ_GROUP_WDOG)
> 
> would be easier to understand. Either case, the second part of the
> expression has an unnecessary (), and multi-line alignment seems to be off.

Will improve it and fix the line alignment in V2.

> 
> > +		return 0;
> > +
> > +	watchdog_notify_pretimeout(imx_sc_wdd);
> 
> The notifier block should be embedded in a local data structure, which would
> include strict watchdog_device and struct notifier_block.
> This would avoid the need for a static variable.

OK, I will add a local data structure to do it in V2.

> 
> > +
> > +	return 0;
> 
> If the function always returns 0, why not the following ?
> Above code seems unnecessary complex for no good reason.
> 
> 	if (event & SC_IRQ_WDOG &&
> 	    *(u8 *)group == SC_IRQ_GROUP_WDOG)
> 		watchdog_notify_pretimeout(imx_sc_wdd);
> 
> 	return 0;

OK.

> 
> > +}
> > +
> > +static struct notifier_block imx_sc_wdt_notifier = {
> > +	.notifier_call = imx_sc_wdt_notify,
> > +};
> > +
> >   static const struct watchdog_ops imx_sc_wdt_ops = {
> >   	.owner = THIS_MODULE,
> >   	.start = imx_sc_wdt_start,
> >   	.stop  = imx_sc_wdt_stop,
> >   	.ping  = imx_sc_wdt_ping,
> >   	.set_timeout = imx_sc_wdt_set_timeout,
> > +	.set_pretimeout = imx_sc_wdt_set_pretimeout,
> >   };
> >
> >   static const struct watchdog_info imx_sc_wdt_info = { @@ -102,9
> > +138,15 @@ static const struct watchdog_info imx_sc_wdt_info = {
> >   static int imx_sc_wdt_probe(struct platform_device *pdev)
> >   {
> >   	struct device *dev = &pdev->dev;
> > -	struct watchdog_device *imx_sc_wdd;
> >   	int ret;
> >
> > +	/* wait until i.MX SCU IPC ready */
> > +	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> > +				       SC_IRQ_WDOG,
> > +				       true);
> > +	if (ret == -EPROBE_DEFER)
> > +		return ret;
> > +
> 
> And if the error is anything else it is ignored ?
> Also, what happens if the interrupt triggers before imx_sc_wdd is set ?

Other error ONLY means the IPC failed, the IRQ WDOG group will NOT be enabled,
it does NOT impact other wdog functions, so I did NOT handle it, maybe I can add
some warning message to tell user that pretimeout function is failed if other error
occurs and also disable the pretimeout function in wdog info?

If interrupt triggers before imx_sc_wdd is set, since the notifier is NOT registered yet,
so the wdog interrupt will be ignored.  

> 
> >   	imx_sc_wdd = devm_kzalloc(dev, sizeof(*imx_sc_wdd), GFP_KERNEL);
> >   	if (!imx_sc_wdd)
> >   		return -ENOMEM;
> > @@ -117,6 +159,7 @@ static int imx_sc_wdt_probe(struct platform_device
> *pdev)
> >   	imx_sc_wdd->max_timeout = MAX_TIMEOUT;
> >   	imx_sc_wdd->parent = dev;
> >   	imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
> > +	imx_sc_wdd->pretimeout = 0;
> 
> Unnecessary.

OK.

> 
> >
> >   	watchdog_init_timeout(imx_sc_wdd, 0, dev);
> >   	watchdog_stop_on_reboot(imx_sc_wdd);
> > @@ -128,13 +171,26 @@ static int imx_sc_wdt_probe(struct
> platform_device *pdev)
> >   		return ret;
> >   	}
> >
> > +	ret = imx_scu_irq_register_notifier(&imx_sc_wdt_notifier);
> > +	if (ret)
> > +		dev_warn(&pdev->dev,
> > +			 "Failed to register watchdog irq notifier\n");
> 
> pretimeout support doesn't work in this case, and any claim to support it
> seems inappropriate.

So how to disable pretimeout function in this case, just overwrite the watchdog_info
to remove the WDIOF_PRETIMEOUT?

> 
> > +
> >   	return 0;
> >   }
> >
> > -static int __maybe_unused imx_sc_wdt_suspend(struct device *dev)
> > +static int imx_sc_wdt_remove(struct platform_device *pdev)
> >   {
> > -	struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> > +	imx_scu_irq_unregister_notifier(&imx_sc_wdt_notifier);
> > +	imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> > +				 SC_IRQ_WDOG,
> > +				 false);
> >
> I would prefer to see devm_add_action() calls.

Ah, agreed, will do it in V2.

Thanks,
Anson.

> 
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused imx_sc_wdt_suspend(struct device *dev) {
> >   	if (watchdog_active(imx_sc_wdd))
> >   		imx_sc_wdt_stop(imx_sc_wdd);
> >
> > @@ -143,8 +199,6 @@ static int __maybe_unused
> > imx_sc_wdt_suspend(struct device *dev)
> >
> >   static int __maybe_unused imx_sc_wdt_resume(struct device *dev)
> >   {
> > -	struct watchdog_device *imx_sc_wdd = dev_get_drvdata(dev);
> > -
> >   	if (watchdog_active(imx_sc_wdd))
> >   		imx_sc_wdt_start(imx_sc_wdd);
> >
> > @@ -162,6 +216,7 @@ MODULE_DEVICE_TABLE(of, imx_sc_wdt_dt_ids);
> >
> >   static struct platform_driver imx_sc_wdt_driver = {
> >   	.probe		= imx_sc_wdt_probe,
> > +	.remove		= imx_sc_wdt_remove,
> >   	.driver		= {
> >   		.name	= "imx-sc-wdt",
> >   		.of_match_table = imx_sc_wdt_dt_ids,
> >


  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-25  1:14 [PATCH 1/2] firmware: imx: SCU irq should ONLY be enabled after SCU IPC is ready Anson Huang
2019-04-25  1:14 ` [PATCH 2/2] watchdog: imx_sc: Add pretimeout support Anson Huang
2019-04-25  4:04   ` Guenter Roeck
2019-04-25  5:44     ` Anson Huang [this message]
2019-04-26 20:17       ` Guenter Roeck
2019-04-28  3:29         ` Anson Huang
2019-04-25  2:15 ` [PATCH 1/2] firmware: imx: SCU irq should ONLY be enabled after SCU IPC is ready Guenter Roeck
2019-04-25  6:51   ` Anson Huang
2019-04-26 20:14     ` Guenter Roeck

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=DB3PR0402MB3916867C62AEB8999EC9EBD4F53D0@DB3PR0402MB3916.eurprd04.prod.outlook.com \
    --to=anson.huang@nxp.com \
    --cc=aisheng.dong@nxp.com \
    --cc=festevam@gmail.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    --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 linux-watchdog@archiver.kernel.org
	public-inbox-index linux-watchdog


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