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@xxxxxxxxxxxx>
> >>---
> >> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
