RE: [PATCH V1 5/6] watchdog: da9062: DA9062 watchdog driver
From: Opensource [Steve Twiss] <hidden>
Date: 2015-05-07 17:45:32
Also in:
linux-devicetree, linux-rtc, linux-watchdog, lkml
Subsystem:
the rest · Maintainer:
Linus Torvalds
On 06 May 2015 21:07 Guenter Roeck wrote:
quoted
quoted
quoted
The DA9062 watchdog ping (register CONTROL_F) is "windowed" for protection against spurious writes -- i.e. the ping function cannot be called within a 250ms time limit or the PMIC will reset. This windowing protection also extends to altering the timeout scale in the CONTROL_D register -- in which case if the timeout register is altered and the ping() function is called within the 250ms limit, the PMIC will reset. The delay is there to stop that from happening. I realised my previous patch was over-sanitised: by putting the time delay into the ping() function I was protecting CONTROL_D in stop() and update_timeout_register(), but I was being too over-protective of the ping() function. Therefore if there was an "incorrect trigger signal", the watchdog would not be allowed to fail because the driver would have filtered out the errors.Hi Steve, From your description, it sounds like the protection is only necessary if there was a previous write to the same register(s).
Hi Guenter, A clarification from me. It is not the CONTROL_D register that needs protecting, but when the CONTROL_D register is altered the function call also performs a CONTROL_F watchdog ping. Too many pings close together would cause the PMIC reset.
quoted
quoted
If so, it might make sense to record the time of such writes, and only add the delay if necessary, and only for the remainder of the time.
I've tried it several ways, but my previous suggestion of putting the delays in the stop() and update_timeout_register() functions just cause even more lengthy delays. So, I've followed your suggestion and used a variable delay inside the ping() function instead: this seems to cause a lot less delay. A debug message can be used to notify the user if the watchdog is trying to be kicked too quickly -- that would be more preferable than just shutting the PMIC down and still provide a notification that something wasn't quite right.
quoted
quoted
Would this be possible ?
I'll run the tests overnight. I'm going to do something like this:
diff --git a/linux-next/v4.0/drivers/watchdog/da9062_wdt.c b/linux-next/v4.0/drivers/watchdog/da9062_wdt.c
index ad80261..d596910 100644
--- a/gp_sparse/linux-next/v4.0/drivers/watchdog/da9062_wdt.c
+++ b/gp_sparse/linux-next/v4.0/drivers/watchdog/da9062_wdt.c
@@ -32,12 +33,37 @@ static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 }; #define DA9062_WDT_MIN_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MIN] #define DA9062_WDT_MAX_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MAX] #define DA9062_WDG_DEFAULT_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MAX-1] +#define DA9062_RESET_PROTECTION_MS 300 struct da9062_watchdog { struct da9062 *hw; struct watchdog_device wdtdev; + unsigned long j_time_stamp; }; +static void da9062_set_window_start(struct da9062_watchdog *wdt) +{ + wdt->j_time_stamp = jiffies; +} + +static void da9062_apply_window_protection(struct da9062_watchdog *wdt) +{ + unsigned long delay = msecs_to_jiffies(DA9062_RESET_PROTECTION_MS); + unsigned long timeout = wdt->j_time_stamp + delay; + unsigned long now = jiffies; + unsigned int diff_ms; + + /* if time-limit has not elapsed then wait for remainder */ + if (time_before(now, timeout)) { + diff_ms = jiffies_to_msecs(timeout-now); + dev_dbg(wdt->hw->dev, + "Delaying watchdog ping by %u msecs\n", diff_ms); + mdelay(diff_ms); + } + + return; +} + static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs) { unsigned int i;
@@ -50,26 +76,29 @@ static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs) return DA9062_TWDSCALE_MAX; } -static int da9062_reset_watchdog_timer(struct da9062 *hw) +static int da9062_reset_watchdog_timer(struct da9062_watchdog *wdt) { int ret; - ret = regmap_update_bits(hw->regmap, + da9062_apply_window_protection(wdt); + + ret = regmap_update_bits(wdt->hw->regmap, DA9062AA_CONTROL_F, DA9062AA_WATCHDOG_MASK, DA9062AA_WATCHDOG_MASK); - mdelay(300); + da9062_set_window_start(wdt); return ret; }
[...]
@@ -216,6 +245,8 @@ static int da9062_wdt_probe(struct platform_device *pdev) dev_err(wdt->hw->dev, "watchdog registration incomplete (%d)\n", ret); + da9062_set_window_start(wdt); + da9062_wdt_ping(&wdt->wdtdev); if (ret < 0) dev_err(wdt->hw->dev,