[PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available

[PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available

[Jorge Ramirez-Ortiz]

Use the bark interrupt as the pre-timeout notifier whenever this
interrupt is available.

By default, the pretimeout notification shall occur one second earlier
than the timeout.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 v4:
     address Guenter Roeck comments as follows:
       remove unnecessary include and private variable
       provide macro for WDT EN register values
       use pretimeout as per its API intent
       handle EPROBE_DEFER on get_irq
     also:
       handle the irq registration as done in pm8916_wdt.c
 v3:
    remove unnecesary variable added to private.

 v2:
    register the pre-timeout notifier instead.

 v1:
 drivers/watchdog/qcom-wdt.c | 64 ++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 7be7f87be28f..0f1d29eeb81d 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -10,6 +10,7 @@
 #include <linux/platform_device.h>
 #include <linux/watchdog.h>
 #include <linux/of_device.h>
+#include <linux/interrupt.h>
 
 enum wdt_reg {
 	WDT_RST,
@@ -19,6 +20,9 @@ enum wdt_reg {
 	WDT_BITE_TIME,
 };
 
+#define QCOM_WDT_ENABLE		BIT(0)
+#define QCOM_WDT_ENABLE_IRQ	BIT(1)
+
 static const u32 reg_offset_data_apcs_tmr[] = {
 	[WDT_RST] = 0x38,
 	[WDT_EN] = 0x40,
@@ -54,15 +58,38 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
 	return container_of(wdd, struct qcom_wdt, wdd);
 }
 
+static inline int qcom_get_enable(struct watchdog_device *wdd)
+{
+	int enable = QCOM_WDT_ENABLE;
+
+	if (wdd->info->options & WDIOF_PRETIMEOUT)
+		enable |= QCOM_WDT_ENABLE_IRQ;
+
+	return enable;
+}
+
+static irqreturn_t qcom_wdt_isr(int irq, void *arg)
+{
+	struct watchdog_device *wdd = arg;
+
+	watchdog_notify_pretimeout(wdd);
+
+	return IRQ_HANDLED;
+}
+
 static int qcom_wdt_start(struct watchdog_device *wdd)
 {
 	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+	unsigned int bark = wdd->timeout;
+
+	if (wdd->pretimeout)
+		bark = bark - wdd->pretimeout;
 
 	writel(0, wdt_addr(wdt, WDT_EN));
 	writel(1, wdt_addr(wdt, WDT_RST));
-	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
+	writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
 	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
-	writel(1, wdt_addr(wdt, WDT_EN));
+	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
 	return 0;
 }
 
@@ -89,6 +116,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
 	return qcom_wdt_start(wdd);
 }
 
+static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
+				   unsigned int timeout)
+{
+	wdd->pretimeout = timeout;
+	return qcom_wdt_start(wdd);
+}
+
 static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
 			    void *data)
 {
@@ -105,7 +139,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
 	writel(1, wdt_addr(wdt, WDT_RST));
 	writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
 	writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
-	writel(1, wdt_addr(wdt, WDT_EN));
+	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
 
 	/*
 	 * Actually make sure the above sequence hits hardware before sleeping.
@@ -121,6 +155,7 @@ static const struct watchdog_ops qcom_wdt_ops = {
 	.stop		= qcom_wdt_stop,
 	.ping		= qcom_wdt_ping,
 	.set_timeout	= qcom_wdt_set_timeout,
+	.set_pretimeout	= qcom_wdt_set_pretimeout,
 	.restart        = qcom_wdt_restart,
 	.owner		= THIS_MODULE,
 };
@@ -133,6 +168,15 @@ static const struct watchdog_info qcom_wdt_info = {
 	.identity	= KBUILD_MODNAME,
 };
 
+static const struct watchdog_info qcom_wdt_pt_info = {
+	.options	= WDIOF_KEEPALIVEPING
+			| WDIOF_MAGICCLOSE
+			| WDIOF_SETTIMEOUT
+			| WDIOF_PRETIMEOUT
+			| WDIOF_CARDRESET,
+	.identity	= KBUILD_MODNAME,
+};
+
 static void qcom_clk_disable_unprepare(void *data)
 {
 	clk_disable_unprepare(data);
@@ -146,7 +190,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	const u32 *regs;
 	u32 percpu_offset;
-	int ret;
+	int irq, ret;
 
 	regs = of_device_get_match_data(dev);
 	if (!regs) {
@@ -204,7 +248,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	wdt->wdd.info = &qcom_wdt_info;
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0) {
+		if (devm_request_irq(dev, irq, qcom_wdt_isr,
+				     IRQF_TRIGGER_RISING, "wdt_bark",
+				     &wdt->wdd))
+			irq = 0;
+	} else if (irq == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	wdt->wdd.info = irq > 0 ? &qcom_wdt_pt_info : &qcom_wdt_info;
+	wdt->wdd.pretimeout = irq > 0 ? 1 : 0;
 	wdt->wdd.ops = &qcom_wdt_ops;
 	wdt->wdd.min_timeout = 1;
 	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
-- 
2.23.0

Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available

[Guenter Roeck]

On Thu, Sep 05, 2019 at 11:00:35PM +0200, Jorge Ramirez-Ortiz wrote:
> Use the bark interrupt as the pre-timeout notifier whenever this
> interrupt is available.
> 
> By default, the pretimeout notification shall occur one second earlier
> than the timeout.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  v4:
>      address Guenter Roeck comments as follows:
>        remove unnecessary include and private variable
>        provide macro for WDT EN register values
>        use pretimeout as per its API intent
>        handle EPROBE_DEFER on get_irq
>      also:
>        handle the irq registration as done in pm8916_wdt.c
>  v3:
>     remove unnecesary variable added to private.
> 
>  v2:
>     register the pre-timeout notifier instead.
> 
>  v1:
>  drivers/watchdog/qcom-wdt.c | 64 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 59 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 7be7f87be28f..0f1d29eeb81d 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -10,6 +10,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/watchdog.h>
>  #include <linux/of_device.h>
> +#include <linux/interrupt.h>
>  
>  enum wdt_reg {
>  	WDT_RST,
> @@ -19,6 +20,9 @@ enum wdt_reg {
>  	WDT_BITE_TIME,
>  };
>  
> +#define QCOM_WDT_ENABLE		BIT(0)
> +#define QCOM_WDT_ENABLE_IRQ	BIT(1)
> +

Using BIT() requires "#include <linux/bits.h>".

>  static const u32 reg_offset_data_apcs_tmr[] = {
>  	[WDT_RST] = 0x38,
>  	[WDT_EN] = 0x40,
> @@ -54,15 +58,38 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
>  	return container_of(wdd, struct qcom_wdt, wdd);
>  }
>  
> +static inline int qcom_get_enable(struct watchdog_device *wdd)
> +{
> +	int enable = QCOM_WDT_ENABLE;
> +
> +	if (wdd->info->options & WDIOF_PRETIMEOUT)
> +		enable |= QCOM_WDT_ENABLE_IRQ;
> +

Again, the condition needs to be that pretimeout != 0,
not that it is supported.

> +	return enable;
> +}
> +
> +static irqreturn_t qcom_wdt_isr(int irq, void *arg)
> +{
> +	struct watchdog_device *wdd = arg;
> +
> +	watchdog_notify_pretimeout(wdd);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int qcom_wdt_start(struct watchdog_device *wdd)
>  {
>  	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> +	unsigned int bark = wdd->timeout;
> +
> +	if (wdd->pretimeout)
> +		bark = bark - wdd->pretimeout;

The if() just adds code and doesn't otherwise do any good.

>  
>  	writel(0, wdt_addr(wdt, WDT_EN));
>  	writel(1, wdt_addr(wdt, WDT_RST));
> -	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
> +	writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>  	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
> -	writel(1, wdt_addr(wdt, WDT_EN));
> +	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
>  	return 0;
>  }
>  
> @@ -89,6 +116,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
>  	return qcom_wdt_start(wdd);
>  }
>  
> +static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
> +				   unsigned int timeout)
> +{
> +	wdd->pretimeout = timeout;
> +	return qcom_wdt_start(wdd);
> +}
> +
>  static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>  			    void *data)
>  {
> @@ -105,7 +139,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>  	writel(1, wdt_addr(wdt, WDT_RST));
>  	writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
>  	writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
> -	writel(1, wdt_addr(wdt, WDT_EN));
> +	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
>  
>  	/*
>  	 * Actually make sure the above sequence hits hardware before sleeping.
> @@ -121,6 +155,7 @@ static const struct watchdog_ops qcom_wdt_ops = {
>  	.stop		= qcom_wdt_stop,
>  	.ping		= qcom_wdt_ping,
>  	.set_timeout	= qcom_wdt_set_timeout,
> +	.set_pretimeout	= qcom_wdt_set_pretimeout,
>  	.restart        = qcom_wdt_restart,
>  	.owner		= THIS_MODULE,
>  };
> @@ -133,6 +168,15 @@ static const struct watchdog_info qcom_wdt_info = {
>  	.identity	= KBUILD_MODNAME,
>  };
>  
> +static const struct watchdog_info qcom_wdt_pt_info = {
> +	.options	= WDIOF_KEEPALIVEPING
> +			| WDIOF_MAGICCLOSE
> +			| WDIOF_SETTIMEOUT
> +			| WDIOF_PRETIMEOUT
> +			| WDIOF_CARDRESET,
> +	.identity	= KBUILD_MODNAME,
> +};
> +
>  static void qcom_clk_disable_unprepare(void *data)
>  {
>  	clk_disable_unprepare(data);
> @@ -146,7 +190,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  	struct device_node *np = dev->of_node;
>  	const u32 *regs;
>  	u32 percpu_offset;
> -	int ret;
> +	int irq, ret;
>  
>  	regs = of_device_get_match_data(dev);
>  	if (!regs) {
> @@ -204,7 +248,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	wdt->wdd.info = &qcom_wdt_info;
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq > 0) {
> +		if (devm_request_irq(dev, irq, qcom_wdt_isr,
> +				     IRQF_TRIGGER_RISING, "wdt_bark",
> +				     &wdt->wdd))
> +			irq = 0;
> +	} else if (irq == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	wdt->wdd.info = irq > 0 ? &qcom_wdt_pt_info : &qcom_wdt_info;
> +	wdt->wdd.pretimeout = irq > 0 ? 1 : 0;

Why repeat the conditional ? It seems to me that something like

	wdt->wdd.info = &qcom_wdt_info;
	...
	if (irq > 0) {
		wdt->wdd.info = &qcom_wdt_pt_info;
		wdt->wdd.pretimeout = 1;
		...
	}

would be much easier and avoid the repeated conditionals.

>  	wdt->wdd.ops = &qcom_wdt_ops;
>  	wdt->wdd.min_timeout = 1;
>  	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
> -- 
> 2.23.0
> 

Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available

[Jorge Ramirez]

On 9/5/19 23:19, Guenter Roeck wrote:
> On Thu, Sep 05, 2019 at 11:00:35PM +0200, Jorge Ramirez-Ortiz wrote:
>> Use the bark interrupt as the pre-timeout notifier whenever this
>> interrupt is available.
>>
>> By default, the pretimeout notification shall occur one second earlier
>> than the timeout.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> ---
>>  v4:
>>      address Guenter Roeck comments as follows:
>>        remove unnecessary include and private variable
>>        provide macro for WDT EN register values
>>        use pretimeout as per its API intent
>>        handle EPROBE_DEFER on get_irq
>>      also:
>>        handle the irq registration as done in pm8916_wdt.c
>>  v3:
>>     remove unnecesary variable added to private.
>>
>>  v2:
>>     register the pre-timeout notifier instead.
>>
>>  v1:
>>  drivers/watchdog/qcom-wdt.c | 64 ++++++++++++++++++++++++++++++++++---
>>  1 file changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
>> index 7be7f87be28f..0f1d29eeb81d 100644
>> --- a/drivers/watchdog/qcom-wdt.c
>> +++ b/drivers/watchdog/qcom-wdt.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/watchdog.h>
>>  #include <linux/of_device.h>
>> +#include <linux/interrupt.h>
>>  
>>  enum wdt_reg {
>>  	WDT_RST,
>> @@ -19,6 +20,9 @@ enum wdt_reg {
>>  	WDT_BITE_TIME,
>>  };
>>  
>> +#define QCOM_WDT_ENABLE		BIT(0)
>> +#define QCOM_WDT_ENABLE_IRQ	BIT(1)
>> +
> 
> Using BIT() requires "#include <linux/bits.h>".

do you want it explicitly in the file even if it builds?

> 
>>  static const u32 reg_offset_data_apcs_tmr[] = {
>>  	[WDT_RST] = 0x38,
>>  	[WDT_EN] = 0x40,
>> @@ -54,15 +58,38 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
>>  	return container_of(wdd, struct qcom_wdt, wdd);
>>  }
>>  
>> +static inline int qcom_get_enable(struct watchdog_device *wdd)
>> +{
>> +	int enable = QCOM_WDT_ENABLE;
>> +
>> +	if (wdd->info->options & WDIOF_PRETIMEOUT)
>> +		enable |= QCOM_WDT_ENABLE_IRQ;
>> +
> 
> Again, the condition needs to be that pretimeout != 0,
> not that it is supported.

no I dont think so. doing that would propagate a possible error in some
pretimeout setup code which would end up enabling an interrupt when it
shouldnt. so I dont think that doing that would be correct.

The interrupt should only be enabled if WDIOF_PRETIMEOUT is configured
(independently of the pretimeout value); as a matter of fact, if
pretimeout is 0, the interrupt will trigger at the same time than bark
(which is what the original code used to do).

so I'd rather keep this condition unless you strongly oppose to it.

> 
>> +	return enable;
>> +}
>> +
>> +static irqreturn_t qcom_wdt_isr(int irq, void *arg)
>> +{
>> +	struct watchdog_device *wdd = arg;
>> +
>> +	watchdog_notify_pretimeout(wdd);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static int qcom_wdt_start(struct watchdog_device *wdd)
>>  {
>>  	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>> +	unsigned int bark = wdd->timeout;
>> +
>> +	if (wdd->pretimeout)
>> +		bark = bark - wdd->pretimeout;
> 
> The if() just adds code and doesn't otherwise do any good.

yeah, was just for clarity and it is surely removed by the compiler. but
sure will remove

> 
>>  
>>  	writel(0, wdt_addr(wdt, WDT_EN));
>>  	writel(1, wdt_addr(wdt, WDT_RST));
>> -	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>> +	writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>>  	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
>> -	writel(1, wdt_addr(wdt, WDT_EN));
>> +	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
>>  	return 0;
>>  }
>>  
>> @@ -89,6 +116,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
>>  	return qcom_wdt_start(wdd);
>>  }
>>  
>> +static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
>> +				   unsigned int timeout)
>> +{
>> +	wdd->pretimeout = timeout;
>> +	return qcom_wdt_start(wdd);
>> +}
>> +
>>  static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>>  			    void *data)
>>  {
>> @@ -105,7 +139,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>>  	writel(1, wdt_addr(wdt, WDT_RST));
>>  	writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
>>  	writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
>> -	writel(1, wdt_addr(wdt, WDT_EN));
>> +	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
>>  
>>  	/*
>>  	 * Actually make sure the above sequence hits hardware before sleeping.
>> @@ -121,6 +155,7 @@ static const struct watchdog_ops qcom_wdt_ops = {
>>  	.stop		= qcom_wdt_stop,
>>  	.ping		= qcom_wdt_ping,
>>  	.set_timeout	= qcom_wdt_set_timeout,
>> +	.set_pretimeout	= qcom_wdt_set_pretimeout,
>>  	.restart        = qcom_wdt_restart,
>>  	.owner		= THIS_MODULE,
>>  };
>> @@ -133,6 +168,15 @@ static const struct watchdog_info qcom_wdt_info = {
>>  	.identity	= KBUILD_MODNAME,
>>  };
>>  
>> +static const struct watchdog_info qcom_wdt_pt_info = {
>> +	.options	= WDIOF_KEEPALIVEPING
>> +			| WDIOF_MAGICCLOSE
>> +			| WDIOF_SETTIMEOUT
>> +			| WDIOF_PRETIMEOUT
>> +			| WDIOF_CARDRESET,
>> +	.identity	= KBUILD_MODNAME,
>> +};
>> +
>>  static void qcom_clk_disable_unprepare(void *data)
>>  {
>>  	clk_disable_unprepare(data);
>> @@ -146,7 +190,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>>  	struct device_node *np = dev->of_node;
>>  	const u32 *regs;
>>  	u32 percpu_offset;
>> -	int ret;
>> +	int irq, ret;
>>  
>>  	regs = of_device_get_match_data(dev);
>>  	if (!regs) {
>> @@ -204,7 +248,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>>  		return -EINVAL;
>>  	}
>>  
>> -	wdt->wdd.info = &qcom_wdt_info;
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq > 0) {
>> +		if (devm_request_irq(dev, irq, qcom_wdt_isr,
>> +				     IRQF_TRIGGER_RISING, "wdt_bark",
>> +				     &wdt->wdd))
>> +			irq = 0;
>> +	} else if (irq == -EPROBE_DEFER)
>> +		return -EPROBE_DEFER;
>> +
>> +	wdt->wdd.info = irq > 0 ? &qcom_wdt_pt_info : &qcom_wdt_info;
>> +	wdt->wdd.pretimeout = irq > 0 ? 1 : 0;
> 
> Why repeat the conditional ? It seems to me that something like
> 
> 	wdt->wdd.info = &qcom_wdt_info;
> 	...
> 	if (irq > 0) {
> 		wdt->wdd.info = &qcom_wdt_pt_info;
> 		wdt->wdd.pretimeout = 1;
> 		...
> 	}
> 
> would be much easier and avoid the repeated conditionals.

I agree. will change.

> 
>>  	wdt->wdd.ops = &qcom_wdt_ops;
>>  	wdt->wdd.min_timeout = 1;
>>  	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
>> -- 
>> 2.23.0
>>
> 

Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available

[Guenter Roeck]

On Thu, Sep 05, 2019 at 11:34:03PM +0200, Jorge Ramirez wrote:
> On 9/5/19 23:19, Guenter Roeck wrote:
> > On Thu, Sep 05, 2019 at 11:00:35PM +0200, Jorge Ramirez-Ortiz wrote:
> >> Use the bark interrupt as the pre-timeout notifier whenever this
> >> interrupt is available.
> >>
> >> By default, the pretimeout notification shall occur one second earlier
> >> than the timeout.
> >>
> >> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >> ---
> >>  v4:
> >>      address Guenter Roeck comments as follows:
> >>        remove unnecessary include and private variable
> >>        provide macro for WDT EN register values
> >>        use pretimeout as per its API intent
> >>        handle EPROBE_DEFER on get_irq
> >>      also:
> >>        handle the irq registration as done in pm8916_wdt.c
> >>  v3:
> >>     remove unnecesary variable added to private.
> >>
> >>  v2:
> >>     register the pre-timeout notifier instead.
> >>
> >>  v1:
> >>  drivers/watchdog/qcom-wdt.c | 64 ++++++++++++++++++++++++++++++++++---
> >>  1 file changed, 59 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> >> index 7be7f87be28f..0f1d29eeb81d 100644
> >> --- a/drivers/watchdog/qcom-wdt.c
> >> +++ b/drivers/watchdog/qcom-wdt.c
> >> @@ -10,6 +10,7 @@
> >>  #include <linux/platform_device.h>
> >>  #include <linux/watchdog.h>
> >>  #include <linux/of_device.h>
> >> +#include <linux/interrupt.h>
> >>  
> >>  enum wdt_reg {
> >>  	WDT_RST,
> >> @@ -19,6 +20,9 @@ enum wdt_reg {
> >>  	WDT_BITE_TIME,
> >>  };
> >>  
> >> +#define QCOM_WDT_ENABLE		BIT(0)
> >> +#define QCOM_WDT_ENABLE_IRQ	BIT(1)
> >> +
> > 
> > Using BIT() requires "#include <linux/bits.h>".
> 
> do you want it explicitly in the file even if it builds?
> 

May I kindly suggest to read Documentation/process/submit-checklist.rst ?

> > 
> >>  static const u32 reg_offset_data_apcs_tmr[] = {
> >>  	[WDT_RST] = 0x38,
> >>  	[WDT_EN] = 0x40,
> >> @@ -54,15 +58,38 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
> >>  	return container_of(wdd, struct qcom_wdt, wdd);
> >>  }
> >>  
> >> +static inline int qcom_get_enable(struct watchdog_device *wdd)
> >> +{
> >> +	int enable = QCOM_WDT_ENABLE;
> >> +
> >> +	if (wdd->info->options & WDIOF_PRETIMEOUT)
> >> +		enable |= QCOM_WDT_ENABLE_IRQ;
> >> +
> > 
> > Again, the condition needs to be that pretimeout != 0,
> > not that it is supported.
> 
> no I dont think so. doing that would propagate a possible error in some
> pretimeout setup code which would end up enabling an interrupt when it
> shouldnt. so I dont think that doing that would be correct.
> 
If the pretimeout setup code is buggy, it needs to be fixed.

> The interrupt should only be enabled if WDIOF_PRETIMEOUT is configured
> (independently of the pretimeout value); as a matter of fact, if
> pretimeout is 0, the interrupt will trigger at the same time than bark
> (which is what the original code used to do).
> 
The original code did not set bit 1 of the WDT_EN register,
and it did not set the bark time.

> so I'd rather keep this condition unless you strongly oppose to it.
> 

Please feel free to petition  to Wim.

> > 
> >> +	return enable;
> >> +}
> >> +
> >> +static irqreturn_t qcom_wdt_isr(int irq, void *arg)
> >> +{
> >> +	struct watchdog_device *wdd = arg;
> >> +
> >> +	watchdog_notify_pretimeout(wdd);
> >> +
> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >>  static int qcom_wdt_start(struct watchdog_device *wdd)
> >>  {
> >>  	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> >> +	unsigned int bark = wdd->timeout;
> >> +
> >> +	if (wdd->pretimeout)
> >> +		bark = bark - wdd->pretimeout;
> > 
> > The if() just adds code and doesn't otherwise do any good.
> 
> yeah, was just for clarity and it is surely removed by the compiler. but
> sure will remove
> 
> > 
> >>  
> >>  	writel(0, wdt_addr(wdt, WDT_EN));
> >>  	writel(1, wdt_addr(wdt, WDT_RST));
> >> -	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
> >> +	writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
> >>  	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
> >> -	writel(1, wdt_addr(wdt, WDT_EN));
> >> +	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
> >>  	return 0;
> >>  }
> >>  
> >> @@ -89,6 +116,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
> >>  	return qcom_wdt_start(wdd);
> >>  }
> >>  
> >> +static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
> >> +				   unsigned int timeout)
> >> +{
> >> +	wdd->pretimeout = timeout;
> >> +	return qcom_wdt_start(wdd);
> >> +}
> >> +
> >>  static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
> >>  			    void *data)
> >>  {
> >> @@ -105,7 +139,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
> >>  	writel(1, wdt_addr(wdt, WDT_RST));
> >>  	writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
> >>  	writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
> >> -	writel(1, wdt_addr(wdt, WDT_EN));
> >> +	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
> >>  
> >>  	/*
> >>  	 * Actually make sure the above sequence hits hardware before sleeping.
> >> @@ -121,6 +155,7 @@ static const struct watchdog_ops qcom_wdt_ops = {
> >>  	.stop		= qcom_wdt_stop,
> >>  	.ping		= qcom_wdt_ping,
> >>  	.set_timeout	= qcom_wdt_set_timeout,
> >> +	.set_pretimeout	= qcom_wdt_set_pretimeout,
> >>  	.restart        = qcom_wdt_restart,
> >>  	.owner		= THIS_MODULE,
> >>  };
> >> @@ -133,6 +168,15 @@ static const struct watchdog_info qcom_wdt_info = {
> >>  	.identity	= KBUILD_MODNAME,
> >>  };
> >>  
> >> +static const struct watchdog_info qcom_wdt_pt_info = {
> >> +	.options	= WDIOF_KEEPALIVEPING
> >> +			| WDIOF_MAGICCLOSE
> >> +			| WDIOF_SETTIMEOUT
> >> +			| WDIOF_PRETIMEOUT
> >> +			| WDIOF_CARDRESET,
> >> +	.identity	= KBUILD_MODNAME,
> >> +};
> >> +
> >>  static void qcom_clk_disable_unprepare(void *data)
> >>  {
> >>  	clk_disable_unprepare(data);
> >> @@ -146,7 +190,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
> >>  	struct device_node *np = dev->of_node;
> >>  	const u32 *regs;
> >>  	u32 percpu_offset;
> >> -	int ret;
> >> +	int irq, ret;
> >>  
> >>  	regs = of_device_get_match_data(dev);
> >>  	if (!regs) {
> >> @@ -204,7 +248,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> -	wdt->wdd.info = &qcom_wdt_info;
> >> +	irq = platform_get_irq(pdev, 0);
> >> +	if (irq > 0) {
> >> +		if (devm_request_irq(dev, irq, qcom_wdt_isr,
> >> +				     IRQF_TRIGGER_RISING, "wdt_bark",
> >> +				     &wdt->wdd))
> >> +			irq = 0;
> >> +	} else if (irq == -EPROBE_DEFER)
> >> +		return -EPROBE_DEFER;
> >> +
> >> +	wdt->wdd.info = irq > 0 ? &qcom_wdt_pt_info : &qcom_wdt_info;
> >> +	wdt->wdd.pretimeout = irq > 0 ? 1 : 0;
> > 
> > Why repeat the conditional ? It seems to me that something like
> > 
> > 	wdt->wdd.info = &qcom_wdt_info;
> > 	...
> > 	if (irq > 0) {
> > 		wdt->wdd.info = &qcom_wdt_pt_info;
> > 		wdt->wdd.pretimeout = 1;
> > 		...
> > 	}
> > 
> > would be much easier and avoid the repeated conditionals.
> 
> I agree. will change.
> 
> > 
> >>  	wdt->wdd.ops = &qcom_wdt_ops;
> >>  	wdt->wdd.min_timeout = 1;
> >>  	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
> >> -- 
> >> 2.23.0
> >>
> > 
> 

Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available

[Jorge Ramirez]

>>>
>>>>  static const u32 reg_offset_data_apcs_tmr[] = {
>>>>  	[WDT_RST] = 0x38,
>>>>  	[WDT_EN] = 0x40,
>>>> @@ -54,15 +58,38 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
>>>>  	return container_of(wdd, struct qcom_wdt, wdd);
>>>>  }
>>>>  
>>>> +static inline int qcom_get_enable(struct watchdog_device *wdd)
>>>> +{
>>>> +	int enable = QCOM_WDT_ENABLE;
>>>> +
>>>> +	if (wdd->info->options & WDIOF_PRETIMEOUT)
>>>> +		enable |= QCOM_WDT_ENABLE_IRQ;
>>>> +
>>>
>>> Again, the condition needs to be that pretimeout != 0,
>>> not that it is supported.
>>
>> no I dont think so. doing that would propagate a possible error in some
>> pretimeout setup code which would end up enabling an interrupt when it
>> shouldnt. so I dont think that doing that would be correct.
>>
> If the pretimeout setup code is buggy, it needs to be fixed.

the condition whether to enable the HW interrupts (IMO) should be
controlled by the DTS as part of the static configuration.

> 
>> The interrupt should only be enabled if WDIOF_PRETIMEOUT is configured
>> (independently of the pretimeout value); as a matter of fact, if
>> pretimeout is 0, the interrupt will trigger at the same time than bark
>> (which is what the original code used to do).
>>
> The original code did not set bit 1 of the WDT_EN register,

sure, this is true

> and it did not set the bark time.

actually no, unless we are looking at different files, the original code
did set the bark time even though PRETIMEOUT was not enabled... so yes
bark was being set to bite. Maybe I am misunderstanding your point.

> 
>> so I'd rather keep this condition unless you strongly oppose to it.
>>
> 
> Please feel free to petition  to Wim.

I'll change to your recomendation and repost v5 - I thought
WDIOF_PRETIMEOUT was formally correct but functionally there is little
difference (if the hardware works as expected)

thanks for all your comments Guenter.

Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available

[Bjorn Andersson]

On Thu 05 Sep 14:00 PDT 2019, Jorge Ramirez-Ortiz wrote:
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
[..]
> +static inline int qcom_get_enable(struct watchdog_device *wdd)
> +{
> +	int enable = QCOM_WDT_ENABLE;
> +
> +	if (wdd->info->options & WDIOF_PRETIMEOUT)
> +		enable |= QCOM_WDT_ENABLE_IRQ;

Looking at downstream they conditionally write 3 to WDT_EN during
initialization, but during suspend/resume they just set it to back to 1.

So I don't think you should touch BIT(1) (which name doesn't match
downstream or the register documentation)

> +
> +	return enable;
> +}
> +
> +static irqreturn_t qcom_wdt_isr(int irq, void *arg)
> +{
> +	struct watchdog_device *wdd = arg;
> +
> +	watchdog_notify_pretimeout(wdd);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int qcom_wdt_start(struct watchdog_device *wdd)
>  {
>  	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> +	unsigned int bark = wdd->timeout;
> +
> +	if (wdd->pretimeout)
> +		bark = bark - wdd->pretimeout;

As Guenter points out, writing wdd->timeout - wdt->pretimeout to
WDT_BARK_TIME unconditionally should do the trick.

>  
>  	writel(0, wdt_addr(wdt, WDT_EN));
>  	writel(1, wdt_addr(wdt, WDT_RST));
> -	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
> +	writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>  	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
> -	writel(1, wdt_addr(wdt, WDT_EN));
> +	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
>  	return 0;
>  }
[..]
> @@ -204,7 +248,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	wdt->wdd.info = &qcom_wdt_info;
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq > 0) {
> +		if (devm_request_irq(dev, irq, qcom_wdt_isr,
> +				     IRQF_TRIGGER_RISING, "wdt_bark",
> +				     &wdt->wdd))

A failure here means that a irq was specified in DT (platform_get_irq()
returned > 0) but you failed to acquire request it, you should fail your
probe() when this happens.

> +			irq = 0;
> +	} else if (irq == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

Some {} around this block please.

Regards,
Bjorn

> +
> +	wdt->wdd.info = irq > 0 ? &qcom_wdt_pt_info : &qcom_wdt_info;
> +	wdt->wdd.pretimeout = irq > 0 ? 1 : 0;
>  	wdt->wdd.ops = &qcom_wdt_ops;
>  	wdt->wdd.min_timeout = 1;
>  	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
> -- 
> 2.23.0
> 

Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available

[Guenter Roeck]

On Fri, Sep 06, 2019 at 10:40:09AM -0700, Bjorn Andersson wrote:
> On Thu 05 Sep 14:00 PDT 2019, Jorge Ramirez-Ortiz wrote:
> > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> [..]
> > +static inline int qcom_get_enable(struct watchdog_device *wdd)
> > +{
> > +	int enable = QCOM_WDT_ENABLE;
> > +
> > +	if (wdd->info->options & WDIOF_PRETIMEOUT)
> > +		enable |= QCOM_WDT_ENABLE_IRQ;
> 
> Looking at downstream they conditionally write 3 to WDT_EN during
> initialization, but during suspend/resume they just set it to back to 1.
> 
Looks like a bug to me.

Either case, per API, pretimeout is enabled with the value of pretimeout,
not with interrupt information from DT. I am not inclined to accept the
above condition for enabling it.

> So I don't think you should touch BIT(1) (which name doesn't match
> downstream or the register documentation)
> 

You mean touching bit 1 is wrong to start with, and it is not a bit used
to enable the interrupt (and thus pretimeout) ?

Guess I'll need to dig out the manual myself.

Guenter

Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available

[Jorge Ramirez]

On 9/6/19 19:40, Bjorn Andersson wrote:
> On Thu 05 Sep 14:00 PDT 2019, Jorge Ramirez-Ortiz wrote:
>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> [..]
>> +static inline int qcom_get_enable(struct watchdog_device *wdd)
>> +{
>> +	int enable = QCOM_WDT_ENABLE;
>> +
>> +	if (wdd->info->options & WDIOF_PRETIMEOUT)
>> +		enable |= QCOM_WDT_ENABLE_IRQ;
> 
> Looking at downstream they conditionally write 3 to WDT_EN during
> initialization, but during suspend/resume they just set it to back to 1.
> 
> So I don't think you should touch BIT(1) (which name doesn't match
> downstream or the register documentation)

writing BIT(1) on the enable register is necessary to get the interrupt
and therefore to be notified of the bark event. this can not be avoided.

> 
>> +
>> +	return enable;
>> +}
>> +
>> +static irqreturn_t qcom_wdt_isr(int irq, void *arg)
>> +{
>> +	struct watchdog_device *wdd = arg;
>> +
>> +	watchdog_notify_pretimeout(wdd);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static int qcom_wdt_start(struct watchdog_device *wdd)
>>  {
>>  	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>> +	unsigned int bark = wdd->timeout;
>> +
>> +	if (wdd->pretimeout)
>> +		bark = bark - wdd->pretimeout;
> 
> As Guenter points out, writing wdd->timeout - wdt->pretimeout to
> WDT_BARK_TIME unconditionally should do the trick.

yes

> 
>>  
>>  	writel(0, wdt_addr(wdt, WDT_EN));
>>  	writel(1, wdt_addr(wdt, WDT_RST));
>> -	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>> +	writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>>  	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
>> -	writel(1, wdt_addr(wdt, WDT_EN));
>> +	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
>>  	return 0;
>>  }
> [..]
>> @@ -204,7 +248,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>>  		return -EINVAL;
>>  	}
>>  
>> -	wdt->wdd.info = &qcom_wdt_info;
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq > 0) {
>> +		if (devm_request_irq(dev, irq, qcom_wdt_isr,
>> +				     IRQF_TRIGGER_RISING, "wdt_bark",
>> +				     &wdt->wdd))
> 
> A failure here means that a irq was specified in DT (platform_get_irq()
> returned > 0) but you failed to acquire request it, you should fail your
> probe() when this happens.

yeah that is what I thought but since pm8916-wdt.c has recently been
merged exactly like I copied above I chose to follow to avoid arguing
about this.

anyway I'll send a patch to fix pm8916-wdt.c and then will do it that
same way on this driver.

> 
>> +			irq = 0;
>> +	} else if (irq == -EPROBE_DEFER)
>> +		return -EPROBE_DEFER;
> 
> Some {} around this block please.

um, checkpatch didnt complain. anyway sure, will do

> 
> Regards,
> Bjorn
> 
>> +
>> +	wdt->wdd.info = irq > 0 ? &qcom_wdt_pt_info : &qcom_wdt_info;
>> +	wdt->wdd.pretimeout = irq > 0 ? 1 : 0;
>>  	wdt->wdd.ops = &qcom_wdt_ops;
>>  	wdt->wdd.min_timeout = 1;
>>  	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
>> -- 
>> 2.23.0
>>
>