[PATCH 0/2] qcom_wdt bark irq support

[PATCH 0/2] qcom_wdt bark irq support

[Jorge Ramirez-Ortiz]

Support pre-timeout when the bark irq is avaible.

This is the fifth version of the patchset addressing all the review
issues to date:

 v5:
    include linux/bits.h
    pretimeout only enables IRQs if value != 0
    remove unnecessary subtract operation
    add clarity to the conditional in the probe function
    revert the irq registration changes
        
 v4:
    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
    modify the irq registration as per pm8916_wdt
    
 v3
    remove unecessary variable from the driver's private storage
 
 v2:
     register the pre-timeout notifier.

With the second patch in the set, I took the oportunity to do some
cleanup in the same code base removing an unnecesary variable from the
driver's private storage.

Jorge Ramirez-Ortiz (2):
  watchdog: qcom: support pre-timeout when the bark irq is available
  watchdog: qcom: remove unnecessary variable from private storage

 drivers/watchdog/qcom-wdt.c | 85 +++++++++++++++++++++++++++++++------
 1 file changed, 72 insertions(+), 13 deletions(-)

-- 
2.23.0

[PATCH 1/2] 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>
---
 drivers/watchdog/qcom-wdt.c | 70 ++++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 7be7f87be28f..935c78a882a3 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (c) 2014, The Linux Foundation. All rights reserved.
  */
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -19,6 +21,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 +59,35 @@ 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->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 - 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 +114,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 +137,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_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
 
 	/*
 	 * Actually make sure the above sequence hits hardware before sleeping.
@@ -121,6 +153,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 +166,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 +188,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 +246,25 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	wdt->wdd.info = &qcom_wdt_info;
+	/* check if there is pretimeout support */
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0) {
+		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
+				       IRQF_TRIGGER_RISING,
+				       "wdt_bark", &wdt->wdd);
+		if (ret)
+			return ret;
+
+		wdt->wdd.info = &qcom_wdt_pt_info;
+		wdt->wdd.pretimeout = 1;
+	} else {
+		if (irq == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		wdt->wdd.info = &qcom_wdt_info;
+		wdt->wdd.pretimeout = 0;
+	}
+
 	wdt->wdd.ops = &qcom_wdt_ops;
 	wdt->wdd.min_timeout = 1;
 	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
-- 
2.23.0

[PATCH 2/2] watchdog: qcom: remove unnecessary variable from private storage

[Jorge Ramirez-Ortiz]

there is no need to continue keeping the clock in private storage.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 drivers/watchdog/qcom-wdt.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 935c78a882a3..e98f5a3d83ea 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -42,7 +42,6 @@ static const u32 reg_offset_data_kpss[] = {
 
 struct qcom_wdt {
 	struct watchdog_device	wdd;
-	struct clk		*clk;
 	unsigned long		rate;
 	void __iomem		*base;
 	const u32		*layout;
@@ -189,6 +188,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 	const u32 *regs;
 	u32 percpu_offset;
 	int irq, ret;
+	struct clk *clk;
 
 	regs = of_device_get_match_data(dev);
 	if (!regs) {
@@ -215,19 +215,18 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(wdt->base))
 		return PTR_ERR(wdt->base);
 
-	wdt->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(wdt->clk)) {
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk)) {
 		dev_err(dev, "failed to get input clock\n");
-		return PTR_ERR(wdt->clk);
+		return PTR_ERR(clk);
 	}
 
-	ret = clk_prepare_enable(wdt->clk);
+	ret = clk_prepare_enable(clk);
 	if (ret) {
 		dev_err(dev, "failed to setup clock\n");
 		return ret;
 	}
-	ret = devm_add_action_or_reset(dev, qcom_clk_disable_unprepare,
-				       wdt->clk);
+	ret = devm_add_action_or_reset(dev, qcom_clk_disable_unprepare, clk);
 	if (ret)
 		return ret;
 
@@ -239,7 +238,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 	 * that it would bite before a second elapses it's usefulness is
 	 * limited.  Bail if this is the case.
 	 */
-	wdt->rate = clk_get_rate(wdt->clk);
+	wdt->rate = clk_get_rate(clk);
 	if (wdt->rate == 0 ||
 	    wdt->rate > 0x10000000U) {
 		dev_err(dev, "invalid clock rate\n");
-- 
2.23.0

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

[Guenter Roeck]

On Fri, Sep 06, 2019 at 10:54:10PM +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>

Nitpick below, otherwise:

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

> ---
>  drivers/watchdog/qcom-wdt.c | 70 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 65 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 7be7f87be28f..935c78a882a3 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright (c) 2014, The Linux Foundation. All rights reserved.
>   */
> +#include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -19,6 +21,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 +59,35 @@ 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->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 - 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 +114,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 +137,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_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
>  
>  	/*
>  	 * Actually make sure the above sequence hits hardware before sleeping.
> @@ -121,6 +153,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 +166,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 +188,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 +246,25 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	wdt->wdd.info = &qcom_wdt_info;
> +	/* check if there is pretimeout support */
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq > 0) {
> +		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
> +				       IRQF_TRIGGER_RISING,
> +				       "wdt_bark", &wdt->wdd);
> +		if (ret)
> +			return ret;
> +
> +		wdt->wdd.info = &qcom_wdt_pt_info;
> +		wdt->wdd.pretimeout = 1;
> +	} else {
> +		if (irq == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		wdt->wdd.info = &qcom_wdt_info;
> +		wdt->wdd.pretimeout = 0;

It is not necessary to set pretimeout to 0; the data structure was
allocated with devm_kzalloc(). The compiler won't know that and
generate unnecessary code otherwise.

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

Re: [PATCH 2/2] watchdog: qcom: remove unnecessary variable from private storage

[Guenter Roeck]

On Fri, Sep 06, 2019 at 10:54:11PM +0200, Jorge Ramirez-Ortiz wrote:
> there is no need to continue keeping the clock in private storage.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

Good catch.

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

> ---
>  drivers/watchdog/qcom-wdt.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 935c78a882a3..e98f5a3d83ea 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -42,7 +42,6 @@ static const u32 reg_offset_data_kpss[] = {
>  
>  struct qcom_wdt {
>  	struct watchdog_device	wdd;
> -	struct clk		*clk;
>  	unsigned long		rate;
>  	void __iomem		*base;
>  	const u32		*layout;
> @@ -189,6 +188,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  	const u32 *regs;
>  	u32 percpu_offset;
>  	int irq, ret;
> +	struct clk *clk;
>  
>  	regs = of_device_get_match_data(dev);
>  	if (!regs) {
> @@ -215,19 +215,18 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  	if (IS_ERR(wdt->base))
>  		return PTR_ERR(wdt->base);
>  
> -	wdt->clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(wdt->clk)) {
> +	clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(clk)) {
>  		dev_err(dev, "failed to get input clock\n");
> -		return PTR_ERR(wdt->clk);
> +		return PTR_ERR(clk);
>  	}
>  
> -	ret = clk_prepare_enable(wdt->clk);
> +	ret = clk_prepare_enable(clk);
>  	if (ret) {
>  		dev_err(dev, "failed to setup clock\n");
>  		return ret;
>  	}
> -	ret = devm_add_action_or_reset(dev, qcom_clk_disable_unprepare,
> -				       wdt->clk);
> +	ret = devm_add_action_or_reset(dev, qcom_clk_disable_unprepare, clk);
>  	if (ret)
>  		return ret;
>  
> @@ -239,7 +238,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  	 * that it would bite before a second elapses it's usefulness is
>  	 * limited.  Bail if this is the case.
>  	 */
> -	wdt->rate = clk_get_rate(wdt->clk);
> +	wdt->rate = clk_get_rate(clk);
>  	if (wdt->rate == 0 ||
>  	    wdt->rate > 0x10000000U) {
>  		dev_err(dev, "invalid clock rate\n");

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

[Jorge Ramirez-Ortiz, Linaro]

On 10/09/19 11:06:55, Guenter Roeck wrote:
> On Fri, Sep 06, 2019 at 10:54:10PM +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>
> 
> Nitpick below, otherwise:
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> > ---
> >  drivers/watchdog/qcom-wdt.c | 70 ++++++++++++++++++++++++++++++++++---
> >  1 file changed, 65 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> > index 7be7f87be28f..935c78a882a3 100644
> > --- a/drivers/watchdog/qcom-wdt.c
> > +++ b/drivers/watchdog/qcom-wdt.c
> > @@ -1,8 +1,10 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> >  /* Copyright (c) 2014, The Linux Foundation. All rights reserved.
> >   */
> > +#include <linux/bits.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > @@ -19,6 +21,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 +59,35 @@ 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->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 - 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 +114,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 +137,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_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
> >  
> >  	/*
> >  	 * Actually make sure the above sequence hits hardware before sleeping.
> > @@ -121,6 +153,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 +166,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 +188,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 +246,25 @@ static int qcom_wdt_probe(struct platform_device *pdev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	wdt->wdd.info = &qcom_wdt_info;
> > +	/* check if there is pretimeout support */
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq > 0) {
> > +		ret = devm_request_irq(dev, irq, qcom_wdt_isr,
> > +				       IRQF_TRIGGER_RISING,
> > +				       "wdt_bark", &wdt->wdd);
> > +		if (ret)
> > +			return ret;
> > +
> > +		wdt->wdd.info = &qcom_wdt_pt_info;
> > +		wdt->wdd.pretimeout = 1;
> > +	} else {
> > +		if (irq == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		wdt->wdd.info = &qcom_wdt_info;
> > +		wdt->wdd.pretimeout = 0;
> 
> It is not necessary to set pretimeout to 0; the data structure was
> allocated with devm_kzalloc(). The compiler won't know that and
> generate unnecessary code otherwise.

will you need me to send another version or could you pick it up as is?

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

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

[Guenter Roeck]

On Thu, Sep 12, 2019 at 09:24:54AM +0200, Jorge Ramirez-Ortiz, Linaro wrote:
> On 10/09/19 11:06:55, Guenter Roeck wrote:
> > On Fri, Sep 06, 2019 at 10:54:10PM +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>
> > 
> > Nitpick below, otherwise:
> > 
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > 
> > > +		wdt->wdd.pretimeout = 0;
> > 
> > It is not necessary to set pretimeout to 0; the data structure was
> > allocated with devm_kzalloc(). The compiler won't know that and
> > generate unnecessary code otherwise.
> 
> will you need me to send another version or could you pick it up as is?
> 

I applied the patch to my watchdog-next branch, with the line removed.
Let's assume that Wim will pick it up from there. If not, and
the line stays in, no real damage.

In short, no need to resubmit.

Guenter