[PATCH v2 0/3] A few fixes to sprd watchdog driver

[PATCH v2 0/3] A few fixes to sprd watchdog driver

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

A few issues about sprd watchdog driver were found recently, this
patchset would fix them.

Changes since v1:
* Added Reviewed-by from Guenter Roeck;
* Abandon original patch 2, add a new patch to use usleep_range() instead of busy loop;
* Revised the max times of loop, also revised the comments for checking busy bit;
* Revised commit message for the whole patchset;

Chunyan Zhang (1):
  watchdog: sprd: change to use usleep_range() instead of busy loop

Lingling Xu (2):
  watchdog: sprd: remove watchdog disable from resume fail path
  watchdog: sprd: check busy bit before new loading rather than after
    that

 drivers/watchdog/sprd_wdt.c | 42 ++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] watchdog: sprd: remove watchdog disable from resume fail path

[Chunyan Zhang]

From: Lingling Xu <ling_ling.xu@unisoc.com>

sprd_wdt_start() would return fail if the loading operation is not completed
in a certain time, disabling watchdog for that case would probably cause
the kernel crash when kick watchdog later, that's too bad, so remove the
watchdog disable operation for the fail case to make sure other parts in
the kernel can run normally.

[ chunyan: Massaged changelog ]

Fixes: 477603467009 ("watchdog: Add Spreadtrum watchdog driver")
Signed-off-by: Lingling Xu <ling_ling.xu@unisoc.com>
Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/sprd_wdt.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
index 65cb55f3916f..f3c90b4afead 100644
--- a/drivers/watchdog/sprd_wdt.c
+++ b/drivers/watchdog/sprd_wdt.c
@@ -345,15 +345,10 @@ static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (watchdog_active(&wdt->wdd)) {
+	if (watchdog_active(&wdt->wdd))
 		ret = sprd_wdt_start(&wdt->wdd);
-		if (ret) {
-			sprd_wdt_disable(wdt);
-			return ret;
-		}
-	}
 
-	return 0;
+	return ret;
 }
 
 static const struct dev_pm_ops sprd_wdt_pm_ops = {
-- 
2.20.1

[PATCH v2 2/3] watchdog: sprd: check busy bit before new loading rather than after that

[Chunyan Zhang]

From: Lingling Xu <ling_ling.xu@unisoc.com>

As the specification described, users must check busy bit before start
a new loading operation to make sure that the previous loading is done
and the device is ready to accept a new one.

[ chunyan: Massaged changelog ]

Fixes: 477603467009 ("watchdog: Add Spreadtrum watchdog driver")
Signed-off-by: Lingling Xu <ling_ling.xu@unisoc.com>
Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/watchdog/sprd_wdt.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
index f3c90b4afead..b9b1daa9e2a4 100644
--- a/drivers/watchdog/sprd_wdt.c
+++ b/drivers/watchdog/sprd_wdt.c
@@ -108,18 +108,6 @@ static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
 	u32 tmr_step = timeout * SPRD_WDT_CNT_STEP;
 	u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP;
 
-	sprd_wdt_unlock(wdt->base);
-	writel_relaxed((tmr_step >> SPRD_WDT_CNT_HIGH_SHIFT) &
-		      SPRD_WDT_LOW_VALUE_MASK, wdt->base + SPRD_WDT_LOAD_HIGH);
-	writel_relaxed((tmr_step & SPRD_WDT_LOW_VALUE_MASK),
-		       wdt->base + SPRD_WDT_LOAD_LOW);
-	writel_relaxed((prtmr_step >> SPRD_WDT_CNT_HIGH_SHIFT) &
-			SPRD_WDT_LOW_VALUE_MASK,
-		       wdt->base + SPRD_WDT_IRQ_LOAD_HIGH);
-	writel_relaxed(prtmr_step & SPRD_WDT_LOW_VALUE_MASK,
-		       wdt->base + SPRD_WDT_IRQ_LOAD_LOW);
-	sprd_wdt_lock(wdt->base);
-
 	/*
 	 * Waiting the load value operation done,
 	 * it needs two or three RTC clock cycles.
@@ -134,6 +122,19 @@ static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
 
 	if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT)
 		return -EBUSY;
+
+	sprd_wdt_unlock(wdt->base);
+	writel_relaxed((tmr_step >> SPRD_WDT_CNT_HIGH_SHIFT) &
+		      SPRD_WDT_LOW_VALUE_MASK, wdt->base + SPRD_WDT_LOAD_HIGH);
+	writel_relaxed((tmr_step & SPRD_WDT_LOW_VALUE_MASK),
+		       wdt->base + SPRD_WDT_LOAD_LOW);
+	writel_relaxed((prtmr_step >> SPRD_WDT_CNT_HIGH_SHIFT) &
+			SPRD_WDT_LOW_VALUE_MASK,
+		       wdt->base + SPRD_WDT_IRQ_LOAD_HIGH);
+	writel_relaxed(prtmr_step & SPRD_WDT_LOW_VALUE_MASK,
+		       wdt->base + SPRD_WDT_IRQ_LOAD_LOW);
+	sprd_wdt_lock(wdt->base);
+
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH v2 2/3] watchdog: sprd: check busy bit before new loading rather than after that

[Guenter Roeck]

On 10/28/20 7:39 PM, Chunyan Zhang wrote:
> From: Lingling Xu <ling_ling.xu@unisoc.com>
> 
> As the specification described, users must check busy bit before start
> a new loading operation to make sure that the previous loading is done
> and the device is ready to accept a new one.
> 
> [ chunyan: Massaged changelog ]
> 
> Fixes: 477603467009 ("watchdog: Add Spreadtrum watchdog driver")
> Signed-off-by: Lingling Xu <ling_ling.xu@unisoc.com>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>

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

> ---
>  drivers/watchdog/sprd_wdt.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
> index f3c90b4afead..b9b1daa9e2a4 100644
> --- a/drivers/watchdog/sprd_wdt.c
> +++ b/drivers/watchdog/sprd_wdt.c
> @@ -108,18 +108,6 @@ static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
>  	u32 tmr_step = timeout * SPRD_WDT_CNT_STEP;
>  	u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP;
>  
> -	sprd_wdt_unlock(wdt->base);
> -	writel_relaxed((tmr_step >> SPRD_WDT_CNT_HIGH_SHIFT) &
> -		      SPRD_WDT_LOW_VALUE_MASK, wdt->base + SPRD_WDT_LOAD_HIGH);
> -	writel_relaxed((tmr_step & SPRD_WDT_LOW_VALUE_MASK),
> -		       wdt->base + SPRD_WDT_LOAD_LOW);
> -	writel_relaxed((prtmr_step >> SPRD_WDT_CNT_HIGH_SHIFT) &
> -			SPRD_WDT_LOW_VALUE_MASK,
> -		       wdt->base + SPRD_WDT_IRQ_LOAD_HIGH);
> -	writel_relaxed(prtmr_step & SPRD_WDT_LOW_VALUE_MASK,
> -		       wdt->base + SPRD_WDT_IRQ_LOAD_LOW);
> -	sprd_wdt_lock(wdt->base);
> -
>  	/*
>  	 * Waiting the load value operation done,
>  	 * it needs two or three RTC clock cycles.
> @@ -134,6 +122,19 @@ static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
>  
>  	if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT)
>  		return -EBUSY;
> +
> +	sprd_wdt_unlock(wdt->base);
> +	writel_relaxed((tmr_step >> SPRD_WDT_CNT_HIGH_SHIFT) &
> +		      SPRD_WDT_LOW_VALUE_MASK, wdt->base + SPRD_WDT_LOAD_HIGH);
> +	writel_relaxed((tmr_step & SPRD_WDT_LOW_VALUE_MASK),
> +		       wdt->base + SPRD_WDT_LOAD_LOW);
> +	writel_relaxed((prtmr_step >> SPRD_WDT_CNT_HIGH_SHIFT) &
> +			SPRD_WDT_LOW_VALUE_MASK,
> +		       wdt->base + SPRD_WDT_IRQ_LOAD_HIGH);
> +	writel_relaxed(prtmr_step & SPRD_WDT_LOW_VALUE_MASK,
> +		       wdt->base + SPRD_WDT_IRQ_LOAD_LOW);
> +	sprd_wdt_lock(wdt->base);
> +
>  	return 0;
>  }
>  
> 

[PATCH v2 3/3] watchdog: sprd: change to use usleep_range() instead of busy loop

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

After changing to check busy bit for the previous loading operation instead
of the current one, for most of cases, the busy bit is not set for the
first time of read, so there's no need to check so frequently, so this
patch use usleep_range() to replace cpu_relax() to avoid busy loop.

Also this patch change the max times to 11 which would be enough, since
according to the specification, the busy bit would be set after a new
loading operation and last 2 or 3 RTC clock cycles (about 60us~92us).

Fixes: 477603467009 ("watchdog: Add Spreadtrum watchdog driver")
Original-by: Lingling Xu <ling_ling.xu@unisoc.com>
Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/watchdog/sprd_wdt.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
index b9b1daa9e2a4..e8097551dfcd 100644
--- a/drivers/watchdog/sprd_wdt.c
+++ b/drivers/watchdog/sprd_wdt.c
@@ -53,7 +53,7 @@
 
 #define SPRD_WDT_CNT_HIGH_SHIFT		16
 #define SPRD_WDT_LOW_VALUE_MASK		GENMASK(15, 0)
-#define SPRD_WDT_LOAD_TIMEOUT		1000
+#define SPRD_WDT_LOAD_TIMEOUT		11
 
 struct sprd_wdt {
 	void __iomem *base;
@@ -109,15 +109,17 @@ static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
 	u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP;
 
 	/*
-	 * Waiting the load value operation done,
-	 * it needs two or three RTC clock cycles.
+	 * Checking busy bit to make sure the previous loading operation is
+	 * done. According to the specification, the busy bit would be set
+	 * after a new loading operation and last 2 or 3 RTC clock
+	 * cycles (about 60us~92us).
 	 */
 	do {
 		val = readl_relaxed(wdt->base + SPRD_WDT_INT_RAW);
 		if (!(val & SPRD_WDT_LD_BUSY_BIT))
 			break;
 
-		cpu_relax();
+		usleep_range(10, 100);
 	} while (delay_cnt++ < SPRD_WDT_LOAD_TIMEOUT);
 
 	if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT)
-- 
2.20.1

[PATCH v2 3/3] watchdog: sprd: change to use usleep_range() instead of busy loop

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

After changing to check busy bit for the previous loading operation instead
of the current one, for most of cases, the busy bit is not set for the
first time of read, so there's no need to check so frequently, so this
patch use usleep_range() to replace cpu_relax() to avoid busy loop.

Also this patch change the max times to 11 which would be enough, since
according to the specification, the busy bit would be set after a new
loading operation and last 2 or 3 RTC clock cycles (about 60us~92us).

Fixes: 477603467009 ("watchdog: Add Spreadtrum watchdog driver")
Original-by: Lingling Xu <ling_ling.xu@unisoc.com>
Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/watchdog/sprd_wdt.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
index b9b1daa9e2a4..4e689b6ff141 100644
--- a/drivers/watchdog/sprd_wdt.c
+++ b/drivers/watchdog/sprd_wdt.c
@@ -6,6 +6,7 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -53,7 +54,7 @@
 
 #define SPRD_WDT_CNT_HIGH_SHIFT		16
 #define SPRD_WDT_LOW_VALUE_MASK		GENMASK(15, 0)
-#define SPRD_WDT_LOAD_TIMEOUT		1000
+#define SPRD_WDT_LOAD_TIMEOUT		11
 
 struct sprd_wdt {
 	void __iomem *base;
@@ -109,15 +110,17 @@ static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
 	u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP;
 
 	/*
-	 * Waiting the load value operation done,
-	 * it needs two or three RTC clock cycles.
+	 * Checking busy bit to make sure the previous loading operation is
+	 * done. According to the specification, the busy bit would be set
+	 * after a new loading operation and last 2 or 3 RTC clock
+	 * cycles (about 60us~92us).
 	 */
 	do {
 		val = readl_relaxed(wdt->base + SPRD_WDT_INT_RAW);
 		if (!(val & SPRD_WDT_LD_BUSY_BIT))
 			break;
 
-		cpu_relax();
+		usleep_range(10, 100);
 	} while (delay_cnt++ < SPRD_WDT_LOAD_TIMEOUT);
 
 	if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT)
-- 
2.20.1

Re: [PATCH v2 3/3] watchdog: sprd: change to use usleep_range() instead of busy loop

[Guenter Roeck]

On 10/28/20 7:39 PM, Chunyan Zhang wrote:
> From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> 
> After changing to check busy bit for the previous loading operation instead
> of the current one, for most of cases, the busy bit is not set for the
> first time of read, so there's no need to check so frequently, so this
> patch use usleep_range() to replace cpu_relax() to avoid busy loop.
> 
> Also this patch change the max times to 11 which would be enough, since
> according to the specification, the busy bit would be set after a new
> loading operation and last 2 or 3 RTC clock cycles (about 60us~92us).
> 
> Fixes: 477603467009 ("watchdog: Add Spreadtrum watchdog driver")
> Original-by: Lingling Xu <ling_ling.xu@unisoc.com>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>

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

> ---
>  drivers/watchdog/sprd_wdt.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
> index b9b1daa9e2a4..e8097551dfcd 100644
> --- a/drivers/watchdog/sprd_wdt.c
> +++ b/drivers/watchdog/sprd_wdt.c
> @@ -53,7 +53,7 @@
>  
>  #define SPRD_WDT_CNT_HIGH_SHIFT		16
>  #define SPRD_WDT_LOW_VALUE_MASK		GENMASK(15, 0)
> -#define SPRD_WDT_LOAD_TIMEOUT		1000
> +#define SPRD_WDT_LOAD_TIMEOUT		11
>  
>  struct sprd_wdt {
>  	void __iomem *base;
> @@ -109,15 +109,17 @@ static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
>  	u32 prtmr_step = pretimeout * SPRD_WDT_CNT_STEP;
>  
>  	/*
> -	 * Waiting the load value operation done,
> -	 * it needs two or three RTC clock cycles.
> +	 * Checking busy bit to make sure the previous loading operation is
> +	 * done. According to the specification, the busy bit would be set
> +	 * after a new loading operation and last 2 or 3 RTC clock
> +	 * cycles (about 60us~92us).
>  	 */
>  	do {
>  		val = readl_relaxed(wdt->base + SPRD_WDT_INT_RAW);
>  		if (!(val & SPRD_WDT_LD_BUSY_BIT))
>  			break;
>  
> -		cpu_relax();
> +		usleep_range(10, 100);
>  	} while (delay_cnt++ < SPRD_WDT_LOAD_TIMEOUT);
>  
>  	if (delay_cnt >= SPRD_WDT_LOAD_TIMEOUT)
>