Quoting Ezequiel Garcia <ezequiel.garcia@xxxxxxxxxxxxxxxxxx>:
On Fri, Feb 07, 2014 at 10:43:14AM -0700, Jason Gunthorpe wrote:
On Fri, Feb 07, 2014 at 07:40:45AM -0300, Ezequiel Garcia wrote:
> Well, this is related to the discussion about the bootloader not
> reseting the watchdog properly, provoking spurious watchdog triggering.
>
> Jason Gunthorpe explained [1] that we needed a particular sequence:
>
> 1. Disable WDT
> 2. Clear bridge
> 3. Enable WDT
>
> We added the irq handling to satisfy (2), and the watchdog stop for (1).
The issue here is the driver configures two 'machine kill' elements:
the PANIC IRQ and the RstOut setup.
Before configuring either of those the driver needs to ensure that any
old watchdog events are cleared out of the HW. We must not get a
spurious event.
I agree not disabling an already functional and properly configured
counter from the bootloader is desirable.
So lets break it down a bit..
1) The IRQ:
It looks like the cause bit latches high on watchdog timer
expiration but has no side effect unless it is unmasked.
The new IRQ flow code ensures the bit is cleared during request_irq
so no old events can trigger the IRQ. Thus it is solved now.
Agreed.
3) The timer itself:
The WDT is just a general timer with an optional hookup to the
rst control. If it is harmlessly counting but not resetting we need
to stop that before enabling rst out.
Actually, the current flow is to:
1. Disable rst out and then disable the counter, in probe().
2. Enable the counter, and then enable rst out, in start().
So, how about this for psuedo-code in probe:
if (readl(RSTOUTn) & WDRstOutEn)
{
/* Watchdog is configured and may be down counting,
don't touch it */
request_irq(..);
}
else
{
/* Watchdog is not configured, fully disable the timer
and configure for watchdog operation. */
disable_watchdog();
request_irq();
writel(RSTOUTn), .. WDRstOutEn);
}
Sounds good, although it seems to me it's actually simpler:
/* Let's make sure the watchdog is fully stopped, unless
* it's explicitly enabled and running
*/
if ( !(wdt_rst_out_en && wdt_timer_enabled) ) {
watchdog_stop();
}
if (!wdt_rst_out_en || !wdt_timer_enabled)
watchdog_stop();
seems to be a bit easier to understand.
Also watch out for coding style issues. Above code
snippet has multiple violations ;-).
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
