[PATCH v6 05/19] watchdog: orion: Make sure the watchdog is initially stopped

On Mon, Feb 10, 2014 at 10:48:50AM -0600, linux at roeck-us.net wrote:

Quoting Ezequiel Garcia [off-list ref]:

quoted

On Fri, Feb 07, 2014 at 10:43:14AM -0700, Jason Gunthorpe wrote:

quoted

On Fri, Feb 07, 2014 at 07:40:45AM -0300, Ezequiel Garcia wrote:

quoted

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.

quoted

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().

quoted

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.

Yeah, I was actually planning to have a orion_wdt_enabled() function
to get the running and enabled status. Looks cleaner I think.

	if (!orion_wdt_enabled())
		watchdog_stop();

So the idea is OK?

I'll push the new series in a short while. Unfortunately, this change
means I have to rebase almost all the series, because I introduce the
orion watchdog struct :-(
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com