Messages in this thread | | | From | Chunyan Zhang <> | Date | Tue, 27 Oct 2020 17:21:06 +0800 | Subject | Re: [PATCH 2/3] watchdog: sprd: change timeout value from 1000 to 2000 |
| |
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; > > >
| |