Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] qcom_wdt bark irq support
@ 2019-09-06 20:54 Jorge Ramirez-Ortiz
  2019-09-06 20:54 ` [PATCH 1/2] watchdog: qcom: support pre-timeout when the bark irq is available Jorge Ramirez-Ortiz
  2019-09-06 20:54 ` [PATCH 2/2] watchdog: qcom: remove unnecessary variable from private storage Jorge Ramirez-Ortiz
  0 siblings, 2 replies; 7+ messages in thread
From: Jorge Ramirez-Ortiz @ 2019-09-06 20:54 UTC (permalink / raw)
  To: jorge.ramirez-ortiz, bjorn.andersson, linux, wim, agross
  Cc: linux-arm-msm, linux-watchdog, linux-kernel

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


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

* [PATCH 1/2] watchdog: qcom: support pre-timeout when the bark irq is available
  2019-09-06 20:54 [PATCH 0/2] qcom_wdt bark irq support Jorge Ramirez-Ortiz
@ 2019-09-06 20:54 ` Jorge Ramirez-Ortiz
  2019-09-10 18:06   ` Guenter Roeck
  2019-09-06 20:54 ` [PATCH 2/2] watchdog: qcom: remove unnecessary variable from private storage Jorge Ramirez-Ortiz
  1 sibling, 1 reply; 7+ messages in thread
From: Jorge Ramirez-Ortiz @ 2019-09-06 20:54 UTC (permalink / raw)
  To: jorge.ramirez-ortiz, bjorn.andersson, linux, wim, agross
  Cc: linux-arm-msm, linux-watchdog, linux-kernel

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


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

* [PATCH 2/2] watchdog: qcom: remove unnecessary variable from private storage
  2019-09-06 20:54 [PATCH 0/2] qcom_wdt bark irq support Jorge Ramirez-Ortiz
  2019-09-06 20:54 ` [PATCH 1/2] watchdog: qcom: support pre-timeout when the bark irq is available Jorge Ramirez-Ortiz
@ 2019-09-06 20:54 ` Jorge Ramirez-Ortiz
  2019-09-10 18:08   ` Guenter Roeck
  1 sibling, 1 reply; 7+ messages in thread
From: Jorge Ramirez-Ortiz @ 2019-09-06 20:54 UTC (permalink / raw)
  To: jorge.ramirez-ortiz, bjorn.andersson, linux, wim, agross
  Cc: linux-arm-msm, linux-watchdog, linux-kernel

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


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

* Re: [PATCH 1/2] watchdog: qcom: support pre-timeout when the bark irq is available
  2019-09-06 20:54 ` [PATCH 1/2] watchdog: qcom: support pre-timeout when the bark irq is available Jorge Ramirez-Ortiz
@ 2019-09-10 18:06   ` Guenter Roeck
  2019-09-12  7:24     ` Jorge Ramirez-Ortiz, Linaro
  0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2019-09-10 18:06 UTC (permalink / raw)
  To: Jorge Ramirez-Ortiz
  Cc: bjorn.andersson, wim, agross, linux-arm-msm, linux-watchdog,
	linux-kernel

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;

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

* Re: [PATCH 2/2] watchdog: qcom: remove unnecessary variable from private storage
  2019-09-06 20:54 ` [PATCH 2/2] watchdog: qcom: remove unnecessary variable from private storage Jorge Ramirez-Ortiz
@ 2019-09-10 18:08   ` Guenter Roeck
  0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2019-09-10 18:08 UTC (permalink / raw)
  To: Jorge Ramirez-Ortiz
  Cc: bjorn.andersson, wim, agross, linux-arm-msm, linux-watchdog,
	linux-kernel

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");

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

* Re: [PATCH 1/2] watchdog: qcom: support pre-timeout when the bark irq is available
  2019-09-10 18:06   ` Guenter Roeck
@ 2019-09-12  7:24     ` Jorge Ramirez-Ortiz, Linaro
  2019-09-12 18:34       ` Guenter Roeck
  0 siblings, 1 reply; 7+ messages in thread
From: Jorge Ramirez-Ortiz, Linaro @ 2019-09-12  7:24 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Jorge Ramirez-Ortiz, bjorn.andersson, wim, agross, linux-arm-msm,
	linux-watchdog, linux-kernel

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;

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

* Re: [PATCH 1/2] watchdog: qcom: support pre-timeout when the bark irq is available
  2019-09-12  7:24     ` Jorge Ramirez-Ortiz, Linaro
@ 2019-09-12 18:34       ` Guenter Roeck
  0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2019-09-12 18:34 UTC (permalink / raw)
  To: Jorge Ramirez-Ortiz, Linaro
  Cc: bjorn.andersson, wim, agross, linux-arm-msm, linux-watchdog,
	linux-kernel

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

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

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-06 20:54 [PATCH 0/2] qcom_wdt bark irq support Jorge Ramirez-Ortiz
2019-09-06 20:54 ` [PATCH 1/2] watchdog: qcom: support pre-timeout when the bark irq is available Jorge Ramirez-Ortiz
2019-09-10 18:06   ` Guenter Roeck
2019-09-12  7:24     ` Jorge Ramirez-Ortiz, Linaro
2019-09-12 18:34       ` Guenter Roeck
2019-09-06 20:54 ` [PATCH 2/2] watchdog: qcom: remove unnecessary variable from private storage Jorge Ramirez-Ortiz
2019-09-10 18:08   ` Guenter Roeck

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