From: Chunyan Zhang <[email protected]>
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
From: Lingling Xu <[email protected]>
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 <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
---
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
From: Chunyan Zhang <[email protected]>
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 <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
---
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
From: Lingling Xu <[email protected]>
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 <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
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
From: Chunyan Zhang <[email protected]>
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 <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
---
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
On 10/28/20 7:39 PM, Chunyan Zhang wrote:
> From: Lingling Xu <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Chunyan Zhang <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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;
> }
>
>
On 10/28/20 7:39 PM, Chunyan Zhang wrote:
> From: Chunyan Zhang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Chunyan Zhang <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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)
>