diff mbox series

[3/3] watchdog: sprd: check busy bit before kick watchdog

Message ID 20201026080919.28413-4-zhang.lyra@gmail.com (mailing list archive)
State Superseded
Headers show
Series A few fixes to sprd watchdog driver | expand

Commit Message

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

Comments

Guenter Roeck Oct. 26, 2020, 2:44 p.m. UTC | #1 
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;
>  }
>  
>
Chunyan Zhang Oct. 27, 2020, 9:17 a.m. UTC | #2 
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;
> >  }
> >
> >
>
diff mbox series

Patch

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;
 }