* [PATCH 0/3] A few fixes to sprd watchdog driver
@ 2020-10-26 8:09 Chunyan Zhang
2020-10-26 8:09 ` [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume Chunyan Zhang
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Chunyan Zhang @ 2020-10-26 8:09 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: linux-watchdog, linux-kernel, Orson Zhai, Baolin Wang,
Chunyan Zhang, Chunyan Zhang
From: Chunyan Zhang <chunyan.zhang@unisoc.com>
A few issues about sprd watchdog driver were found recently, this
patchset would fix them.
Lingling Xu (3):
watchdog: sprd: should not disable watchdog in resume
watchdog: sprd: change timeout value from 1000 to 2000
watchdog: sprd: check busy bit before kick watchdog
drivers/watchdog/sprd_wdt.c | 38 +++++++++++++++++--------------------
1 file changed, 17 insertions(+), 21 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume 2020-10-26 8:09 [PATCH 0/3] A few fixes to sprd watchdog driver Chunyan Zhang @ 2020-10-26 8:09 ` Chunyan Zhang 2020-10-26 14:27 ` Guenter Roeck 2020-10-26 8:09 ` [PATCH 2/3] watchdog: sprd: change timeout value from 1000 to 2000 Chunyan Zhang 2020-10-26 8:09 ` [PATCH 3/3] watchdog: sprd: check busy bit before kick watchdog Chunyan Zhang 2 siblings, 1 reply; 10+ messages in thread From: Chunyan Zhang @ 2020-10-26 8:09 UTC (permalink / raw) To: Wim Van Sebroeck, Guenter Roeck Cc: linux-watchdog, linux-kernel, Orson Zhai, Baolin Wang, Chunyan Zhang, Chunyan Zhang From: Lingling Xu <ling_ling.xu@unisoc.com> Don't disable watchdog in resume process, otherwise system would crash once kick watchdog. 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 | 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume 2020-10-26 8:09 ` [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume Chunyan Zhang @ 2020-10-26 14:27 ` Guenter Roeck 2020-10-27 9:34 ` Chunyan Zhang 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2020-10-26 14:27 UTC (permalink / raw) To: Chunyan Zhang, Wim Van Sebroeck Cc: linux-watchdog, linux-kernel, Orson Zhai, Baolin Wang, Chunyan Zhang On 10/26/20 1:09 AM, Chunyan Zhang wrote: > From: Lingling Xu <ling_ling.xu@unisoc.com> > > Don't disable watchdog in resume process, otherwise system would crash > once kick watchdog. > This is a bit misleading: It is only disabled if the attempt to start it has failed. Was this observed in practice ? If so, it might make sense to identify and fix the underlying problem instead of trying to work around it (or is this addressed with the second patch of the series ?) Anyway, the patch itself is fine. Reviewed-by: Guenter Roeck <linux@roeck-us.net> Thanks, Guenter > 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 | 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 = { > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume 2020-10-26 14:27 ` Guenter Roeck @ 2020-10-27 9:34 ` Chunyan Zhang 0 siblings, 0 replies; 10+ messages in thread From: Chunyan Zhang @ 2020-10-27 9:34 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, linux-watchdog, Linux Kernel Mailing List, Orson Zhai, Baolin Wang, Chunyan Zhang On Mon, 26 Oct 2020 at 22:27, Guenter Roeck <linux@roeck-us.net> wrote: > > On 10/26/20 1:09 AM, Chunyan Zhang wrote: > > From: Lingling Xu <ling_ling.xu@unisoc.com> > > > > Don't disable watchdog in resume process, otherwise system would crash > > once kick watchdog. > > > > This is a bit misleading: It is only disabled if the attempt to start it > has failed. Was this observed in practice ? If so, it might make sense > to identify and fix the underlying problem instead of trying to work > around it (or is this addressed with the second patch of the series ?) Yes, I think the root cause of this problem was like what I explained in the 3rd patch in this series. Lingling found there was something wrong in sprd_wdt_pm_resume() when debugging that issue, then we had this patch. > > Anyway, the patch itself is fine. > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> Thanks, Chunyan > > Thanks, > Guenter > > > 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 | 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 = { > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] watchdog: sprd: change timeout value from 1000 to 2000 2020-10-26 8:09 [PATCH 0/3] A few fixes to sprd watchdog driver Chunyan Zhang 2020-10-26 8:09 ` [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume Chunyan Zhang @ 2020-10-26 8:09 ` Chunyan Zhang 2020-10-26 14:36 ` Guenter Roeck 2020-10-26 8:09 ` [PATCH 3/3] watchdog: sprd: check busy bit before kick watchdog Chunyan Zhang 2 siblings, 1 reply; 10+ messages in thread From: Chunyan Zhang @ 2020-10-26 8:09 UTC (permalink / raw) To: Wim Van Sebroeck, Guenter Roeck Cc: linux-watchdog, linux-kernel, Orson Zhai, Baolin Wang, Chunyan Zhang, Chunyan Zhang From: Lingling Xu <ling_ling.xu@unisoc.com> Because cpu_relax() takes different time on different SoCs, for some rare cases, it would take more than 1000 cycles for waitting load operation finished. The result of many times testing verified that changing the timeout value to 2000 can solve the issue. 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c index f3c90b4afead..4f2a8c6d6485 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 2000 struct sprd_wdt { void __iomem *base; -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] watchdog: sprd: change timeout value from 1000 to 2000 2020-10-26 8:09 ` [PATCH 2/3] watchdog: sprd: change timeout value from 1000 to 2000 Chunyan Zhang @ 2020-10-26 14:36 ` Guenter Roeck 2020-10-27 9:21 ` Chunyan Zhang 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2020-10-26 14:36 UTC (permalink / raw) To: Chunyan Zhang, Wim Van Sebroeck Cc: linux-watchdog, linux-kernel, Orson Zhai, Baolin Wang, Chunyan Zhang On 10/26/20 1:09 AM, Chunyan Zhang wrote: > From: Lingling Xu <ling_ling.xu@unisoc.com> > > Because cpu_relax() takes different time on different SoCs, for some rare > cases, it would take more than 1000 cycles for waitting load operation waiting > finished. The result of many times testing verified that changing the > timeout value to 2000 can solve the issue. > This is just a kludge that doesn't address the underlying problem. As the wait loop states, "Waiting the load value operation done, it needs two or three RTC clock cycles". This means the loop should wait for a maximum number of clock cycles, and not run as hot loop. If we assume that clk_get_rate() returns the clock frequency, that frequency can be used to determine how long this needs to be retried. It might also make sense - depending on how long this actually takes - to use usleep_range() instead of cpu_relax() to avoid the hot loop. Guenter > 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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c > index f3c90b4afead..4f2a8c6d6485 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 2000 > > struct sprd_wdt { > void __iomem *base; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] watchdog: sprd: change timeout value from 1000 to 2000 2020-10-26 14:36 ` Guenter Roeck @ 2020-10-27 9:21 ` Chunyan Zhang 0 siblings, 0 replies; 10+ messages in thread From: Chunyan Zhang @ 2020-10-27 9:21 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, linux-watchdog, Linux Kernel Mailing List, Orson Zhai, Baolin Wang, Chunyan Zhang, jingchao.ye, ling_ling.xu, xiaoqing.wu On Mon, 26 Oct 2020 at 22:36, Guenter Roeck <linux@roeck-us.net> wrote: > > On 10/26/20 1:09 AM, Chunyan Zhang wrote: > > From: Lingling Xu <ling_ling.xu@unisoc.com> > > > > Because cpu_relax() takes different time on different SoCs, for some rare > > cases, it would take more than 1000 cycles for waitting load operation > > waiting Ok. > > > finished. The result of many times testing verified that changing the > > timeout value to 2000 can solve the issue. > > > > This is just a kludge that doesn't address the underlying problem. > As the wait loop states, "Waiting the load value operation done, > it needs two or three RTC clock cycles". This means the loop > should wait for a maximum number of clock cycles, and not run > as hot loop. If we assume that clk_get_rate() returns the clock > frequency, that frequency can be used to determine how long this > needs to be retried. It might also make sense - depending on how > long this actually takes - to use usleep_range() instead of > cpu_relax() to avoid the hot loop. Agree, using usleep_range() instead makes more sense, I will look into that. Thanks for your review. Chunyan > > Guenter > > > 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 | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c > > index f3c90b4afead..4f2a8c6d6485 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 2000 > > > > struct sprd_wdt { > > void __iomem *base; > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] watchdog: sprd: check busy bit before kick watchdog 2020-10-26 8:09 [PATCH 0/3] A few fixes to sprd watchdog driver Chunyan Zhang 2020-10-26 8:09 ` [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume Chunyan Zhang 2020-10-26 8:09 ` [PATCH 2/3] watchdog: sprd: change timeout value from 1000 to 2000 Chunyan Zhang @ 2020-10-26 8:09 ` Chunyan Zhang 2020-10-26 14:44 ` Guenter Roeck 2 siblings, 1 reply; 10+ messages in thread From: Chunyan Zhang @ 2020-10-26 8:09 UTC (permalink / raw) To: Wim Van Sebroeck, Guenter Roeck Cc: linux-watchdog, linux-kernel, Orson Zhai, Baolin Wang, Chunyan Zhang, Chunyan Zhang From: Lingling Xu <ling_ling.xu@unisoc.com> As the specification described, checking busy bit must be done before kick watchdog. 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 | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c index 4f2a8c6d6485..14071c66ff49 100644 --- a/drivers/watchdog/sprd_wdt.c +++ b/drivers/watchdog/sprd_wdt.c @@ -108,20 +108,8 @@ 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, + * Waiting the last load value operation done, * it needs two or three RTC clock cycles. */ do { @@ -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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] watchdog: sprd: check busy bit before kick watchdog 2020-10-26 8:09 ` [PATCH 3/3] watchdog: sprd: check busy bit before kick watchdog Chunyan Zhang @ 2020-10-26 14:44 ` Guenter Roeck 2020-10-27 9:17 ` Chunyan Zhang 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2020-10-26 14:44 UTC (permalink / raw) To: Chunyan Zhang, Wim Van Sebroeck Cc: linux-watchdog, linux-kernel, Orson Zhai, Baolin Wang, Chunyan Zhang On 10/26/20 1:09 AM, Chunyan Zhang wrote: > From: Lingling Xu <ling_ling.xu@unisoc.com> > > As the specification described, checking busy bit must be done before kick > watchdog. > That is a key functional change: So far the code checked if a value was accepted after loading it. That is no longer the case. Effectively, with this change, the _next_ operation will now check if the previous operation was accepted. Is this intentional ? Also, does this really solve a problem, or is it just an optimization ? By checking for busy prior to an operation instead of after it the only real difference is that the busy check will most likely succeed immediately because enough time has passed since the last write. Ultimately it is your call how you want to handle this, but I think the impact should be spelled out. Guenter > 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 | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c > index 4f2a8c6d6485..14071c66ff49 100644 > --- a/drivers/watchdog/sprd_wdt.c > +++ b/drivers/watchdog/sprd_wdt.c > @@ -108,20 +108,8 @@ 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, > + * Waiting the last load value operation done, > * it needs two or three RTC clock cycles. > */ > do { > @@ -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; > } > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] watchdog: sprd: check busy bit before kick watchdog 2020-10-26 14:44 ` Guenter Roeck @ 2020-10-27 9:17 ` Chunyan Zhang 0 siblings, 0 replies; 10+ messages in thread From: Chunyan Zhang @ 2020-10-27 9:17 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, linux-watchdog, Linux Kernel Mailing List, Orson Zhai, Baolin Wang, Chunyan Zhang, Lingling Xu, jingchao.ye, xiaoqing.wu On Mon, 26 Oct 2020 at 22:44, Guenter Roeck <linux@roeck-us.net> wrote: > > On 10/26/20 1:09 AM, Chunyan Zhang wrote: > > From: Lingling Xu <ling_ling.xu@unisoc.com> > > > > As the specification described, checking busy bit must be done before kick > > watchdog. > > > > That is a key functional change: So far the code checked if a value > was accepted after loading it. That is no longer the case. Effectively, > with this change, the _next_ operation will now check if the previous > operation was accepted. Is this intentional ? Yes, the busy bit indicates whether the previous operation is done, so we have to make sure the last loading completed (the busy bit is not set) before new loading. The spec says that this bit is set after a new loading, and would last 2 or 3 RTC clock cycles. > > Also, does this really solve a problem, or is it just an optimization ? > By checking for busy prior to an operation instead of after it the only > real difference is that the busy check will most likely succeed immediately > because enough time has passed since the last write. > > Ultimately it is your call how you want to handle this, but I think the > impact should be spelled out. Ok, I will add more details in the commit message. Many thanks for the review! Chunyan > > Guenter > > > 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 | 27 ++++++++++++++------------- > > 1 file changed, 14 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c > > index 4f2a8c6d6485..14071c66ff49 100644 > > --- a/drivers/watchdog/sprd_wdt.c > > +++ b/drivers/watchdog/sprd_wdt.c > > @@ -108,20 +108,8 @@ 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, > > + * Waiting the last load value operation done, > > * it needs two or three RTC clock cycles. > > */ > > do { > > @@ -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; > > } > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-27 9:34 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-26 8:09 [PATCH 0/3] A few fixes to sprd watchdog driver Chunyan Zhang 2020-10-26 8:09 ` [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume Chunyan Zhang 2020-10-26 14:27 ` Guenter Roeck 2020-10-27 9:34 ` Chunyan Zhang 2020-10-26 8:09 ` [PATCH 2/3] watchdog: sprd: change timeout value from 1000 to 2000 Chunyan Zhang 2020-10-26 14:36 ` Guenter Roeck 2020-10-27 9:21 ` Chunyan Zhang 2020-10-26 8:09 ` [PATCH 3/3] watchdog: sprd: check busy bit before kick watchdog Chunyan Zhang 2020-10-26 14:44 ` Guenter Roeck 2020-10-27 9:17 ` Chunyan Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).