[1/1] watchdog: sp805: ping fails to abort wdt reset

Message ID 20160119065313.GA22443@vireshk
State New
Headers show

Commit Message

Viresh Kumar Jan. 19, 2016, 6:53 a.m.
On 19-01-16, 11:34, Sandeep Tripathy wrote:
>  sp805 wdt asserts interrupt for the first expiry and


Why an extra space before sp805 ?

> reloads the counter. If wdt interrupt is set and count

> reaches zero then wdt reset event is generated.To get


Add space after full-stop.

> wdt reset at t timeout the driver loads wdt counter


Keep t and t/2 in single quotes, its hardly readable otherwise.

> with t/2. A ping before t second *should* prevent wdt

> reset. Currently if ping is done after t/2 sec then

> wdt interrupt condition gets set. On the next countdown

> of loadval wdt reset event occurs eventhough wdt was

> reloaded before the set timeout t.

> 

>  This patch clears the interrupt condition on ping.


Why extra spaces before 'This'? 

> 

> Signed-off-by: Sandeep Tripathy <tripathy@broadcom.com>

> ---

>  drivers/watchdog/sp805_wdt.c |    5 ++---

>  1 file changed, 2 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c

> index 01d8162..e7a715e 100644

> --- a/drivers/watchdog/sp805_wdt.c

> +++ b/drivers/watchdog/sp805_wdt.c

> @@ -139,12 +139,11 @@ static int wdt_config(struct watchdog_device *wdd, bool ping)

>  

>  	writel_relaxed(UNLOCK, wdt->base + WDTLOCK);

>  	writel_relaxed(wdt->load_val, wdt->base + WDTLOAD);

> +	writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);

>  

> -	if (!ping) {

> -		writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);

> +	if (!ping)

>  		writel_relaxed(INT_ENABLE | RESET_ENABLE, wdt->base +

>  				WDTCONTROL);

> -	}

>  

>  	writel_relaxed(LOCK, wdt->base + WDTLOCK);


If I read the TRM properly, then you don't need to load the WDTLOAD
register on ping and so your change can be written as:



Please try if this works or not.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Viresh Kumar Jan. 19, 2016, 7:17 a.m. | #1
On 18-01-16, 23:11, Guenter Roeck wrote:
> On 01/18/2016 10:53 PM, Viresh Kumar wrote:

> >On 19-01-16, 11:34, Sandeep Tripathy wrote:

> >>  sp805 wdt asserts interrupt for the first expiry and

> >

> >Why an extra space before sp805 ?

> >

> >>reloads the counter. If wdt interrupt is set and count

> >>reaches zero then wdt reset event is generated.To get

> >

> >Add space after full-stop.

> >

> >>wdt reset at t timeout the driver loads wdt counter

> >

> >Keep t and t/2 in single quotes, its hardly readable otherwise.

> >

> >>with t/2. A ping before t second *should* prevent wdt

> >>reset. Currently if ping is done after t/2 sec then

> >>wdt interrupt condition gets set. On the next countdown

> >>of loadval wdt reset event occurs eventhough wdt was

> >>reloaded before the set timeout t.

> >>

> >>  This patch clears the interrupt condition on ping.

> >

> >Why extra spaces before 'This'?

> >

> >>

> >>Signed-off-by: Sandeep Tripathy <tripathy@broadcom.com>

> >>---

> >>  drivers/watchdog/sp805_wdt.c |    5 ++---

> >>  1 file changed, 2 insertions(+), 3 deletions(-)

> >>

> >>diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c

> >>index 01d8162..e7a715e 100644

> >>--- a/drivers/watchdog/sp805_wdt.c

> >>+++ b/drivers/watchdog/sp805_wdt.c

> >>@@ -139,12 +139,11 @@ static int wdt_config(struct watchdog_device *wdd, bool ping)

> >>

> >>  	writel_relaxed(UNLOCK, wdt->base + WDTLOCK);

> >>  	writel_relaxed(wdt->load_val, wdt->base + WDTLOAD);

> >>+	writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);

> >>

> >>-	if (!ping) {

> >>-		writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);

> >>+	if (!ping)

> >>  		writel_relaxed(INT_ENABLE | RESET_ENABLE, wdt->base +

> >>  				WDTCONTROL);

> >>-	}

> >>

> >>  	writel_relaxed(LOCK, wdt->base + WDTLOCK);

> >

> >If I read the TRM properly, then you don't need to load the WDTLOAD

> >register on ping and so your change can be written as:

> >

> >diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c

> >index 01d816251302..42e1ec86532d 100644

> >--- a/drivers/watchdog/sp805_wdt.c

> >+++ b/drivers/watchdog/sp805_wdt.c

> >@@ -138,10 +138,10 @@ static int wdt_config(struct watchdog_device *wdd, bool ping)

> >         spin_lock(&wdt->lock);

> >

> >         writel_relaxed(UNLOCK, wdt->base + WDTLOCK);

> >-       writel_relaxed(wdt->load_val, wdt->base + WDTLOAD);

> >+       writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);

> >

> >         if (!ping) {

> >-               writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);

> >+               writel_relaxed(wdt->load_val, wdt->base + WDTLOAD);

> >                 writel_relaxed(INT_ENABLE | RESET_ENABLE, wdt->base +

> >                                 WDTCONTROL);

> >         }

> >

> 

> That would require additional changes. Reason is that the load value is not immediately

> written after a timeout change, but only with the next ping (which happens immediately

> afterwards). With your proposed change, the load value would never be updated.

> 

> Besides, that would be a separate change, and would require a separate patch.


Hmm, maybe then we can keep the original patch as is :)

@Sandeep: Please resend your patch after fixing the commit log and not
the diff.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index 01d816251302..42e1ec86532d 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -138,10 +138,10 @@  static int wdt_config(struct watchdog_device *wdd, bool ping)
        spin_lock(&wdt->lock);
 
        writel_relaxed(UNLOCK, wdt->base + WDTLOCK);
-       writel_relaxed(wdt->load_val, wdt->base + WDTLOAD);
+       writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);
 
        if (!ping) {
-               writel_relaxed(INT_MASK, wdt->base + WDTINTCLR);
+               writel_relaxed(wdt->load_val, wdt->base + WDTLOAD);
                writel_relaxed(INT_ENABLE | RESET_ENABLE, wdt->base +
                                WDTCONTROL);
        }