On Thu, Sep 18, 2014 at 07:47:54PM -0700, Guenter Roeck wrote:

On 09/18/2014 03:27 PM, Josh Cartwright wrote:

quoted

The WDT's BITE_TIME warm-reset behavior can be leveraged as a last
resort mechanism for triggering chip reset.  Usually, other restart
methods (such as PS_HOLD) are preferrable for issuing a more complete
reset of the chip.  As such, keep the priority of the watchdog notifier
low.

Signed-off-by: Josh Cartwright <redacted>

[..]

quoted

+static int qcom_wdt_restart(struct notifier_block *nb, unsigned long action,
+			    void *data)
+{
+	struct qcom_wdt *wdt = container_of(nb, struct qcom_wdt, restart_nb);
+
+	/*
+	 * Trigger watchdog bite:
+	 *    Setup BITE_TIME to be very low, and enable WDT.
+	 */
+	mutex_lock(&wdt->wdd.lock);

At this time you don't need to worry about locks.

Actually, this might be dangerous if the lock happens to be taken,
as it won't be released (there is no other code running anymore
when this function is called).

Ah, great.  I'll drop the locking.

quoted

+	writel_relaxed(0, wdt->base + WDT_EN);
+	writel_relaxed(1, wdt->base + WDT_RST);
+	writel_relaxed(0x31F3, wdt->base + WDT_BITE_TIME);

What is the magic here, ie what does 0x31F3 stand for ?

Nothing magic, it's just a reasonably low value to set the bite time
counter at.  It also happens to be the value at reset.

quoted

+	writel_relaxed(1, wdt->base + WDT_EN);
+	mutex_unlock(&wdt->wdd.lock);
+	return NOTIFY_DONE;
+}
+
 static int qcom_watchdog_probe(struct platform_device *pdev)
 {
 	struct qcom_wdt *wdt;

@@ -121,6 +141,17 @@ static int qcom_watchdog_probe(struct platform_device *pdev)
 		return ret;
 	}

+	/*
+	 * WDT restart notifier has priority 0 (use as a last resort)
+	 */
+	wdt->restart_nb.notifier_call = qcom_wdt_restart;
+	ret = register_restart_handler(&wdt->restart_nb);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to setup restart handler\n");
+		watchdog_unregister_device(&wdt->wdd);
+		return ret;

Sure you want to return an error here ? The watchdog itself is still working,
and this is supposed to be a restart method of last resort. Causing the driver
to fail loading because it can not register its restart handler seems to be
a bit aggressive.

It is a bit aggressive.  I'll at least drop it to a dev_warn(); even
though it is a "last resort", on some boards it's the only available
mechanism for reliable restart.

Thanks!
  Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation