[2/3] watchdog: sprd: change timeout value from 1000 to 2000

Message ID 20201026080919.28413-3-zhang.lyra@gmail.com
State New
Headers show
Series
  • A few fixes to sprd watchdog driver
Related show

Commit Message

Chunyan Zhang Oct. 26, 2020, 8:09 a.m.
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(-)

Comments

Guenter Roeck Oct. 26, 2020, 2:36 p.m. | #1
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;

>
Chunyan Zhang Oct. 27, 2020, 9:21 a.m. | #2
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;

> >

>

Patch

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;