Hello!
Our board uses the RN5T567 as /dev/watchdog source via i2c communication.
RN5T567 is using the rn5t618_wdt.c driver
Our kernel has CONFIG_WATCHDOG_NOWAYOUT=y enabled
# Starting the wdt works as expected
echo -n '1' > /dev/watchdog
# Stopping the wdt works as expected no reboot will be issued
echo -n 'V' > /dev/watchdog
# Starting the wdt again will enable the wdt again
# BUT while the wdt is triggered every second the system reboots
while true; do echo -n '1' > /dev/watchdog; sleep 1; done
Digging deeper into the issue I could find out that the remap is
initialized to cache the register accesses RN5T618_WATCHDOG (0x0b) and
RN5T618_PWRIRQ (0x13).
So I expect that because of this caching the IRQ status bit was never reset.
The status register must not be cached because its set by the RN5T567.
Also it is not ideal to cache the access to the watchdog register
which resets the counter via read write cycle.
debugfs shows the regmap setting for these registers:
[root@imx7d /sys/kernel/debug]# cat regmap/0-0033/access
// third column means volatile yes or no
…
0b: y y n n
…
13: y y n n
After marking these registers volatile, stopping the wdt and starting
again seems to work.
Furthermore it is not necessary to do a RN5T618_WATCHDOG read AND
write cycle to reset the wdt counter.
The source code states:
/* The counter is restarted after a R/W access to watchdog register */
The RN5T567 datasheet states:
“The count value of watchdog timer is cleared by accessing (R/W) to
this register.”
Tests showed that a single read is enough. I did not check other chip
variants which use the same driver.
In my opinion a write cycle is even dangerous if there is some strange
situation and the write cycle disables the wdt or changes the wdt
settings stored in this register.
I still don't know why the irq status bit is cleared on every ping()
but I kept it there.
Attached is my patch tested with RN5T567.
Is the rn5t618_wdt.c driver maintained? Strange that such issue was
never noticed.
diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
index 652a5e60067f..b414abcdfecb 100644
--- a/drivers/mfd/rn5t618.c
+++ b/drivers/mfd/rn5t618.c
@@ -47,8 +47,10 @@ static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg)
case RN5T618_RTC_SECONDS ... RN5T618_RTC_YEAR:
case RN5T618_CHGSTATE:
case RN5T618_CHGCTRL_IRR ... RN5T618_CHGERR_MONI:
case RN5T618_CONTROL ... RN5T618_CC_AVEREG0:
+ case RN5T618_WATCHDOG: // should not be cached because of r/w to reset counter
+ case RN5T618_PWRIRQ: // should not be cached because its set by hardware
return true;
default:
return false;
}
diff --git a/drivers/watchdog/rn5t618_wdt.c b/drivers/watchdog/rn5t618_wdt.c
index 6e524c8e26a8..83d3f006f812 100644
--- a/drivers/watchdog/rn5t618_wdt.c
+++ b/drivers/watchdog/rn5t618_wdt.c
@@ -75,8 +75,13 @@ static int rn5t618_wdt_start(struct watchdog_device *wdt_dev)
ret = rn5t618_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
if (ret)
return ret;
+ /* Clear pending watchdog interrupt */
+ ret = regmap_write(wdt->rn5t618->regmap, RN5T618_PWRIRQ, ~RN5T618_PWRIRQ_IR_WDOG);
+ if (ret)
+ return ret;
+
/* enable repower-on */
ret = regmap_update_bits(wdt->rn5t618->regmap, RN5T618_REPCNT,
RN5T618_REPCNT_REPWRON,
RN5T618_REPCNT_REPWRON);
@@ -114,15 +119,10 @@ static int rn5t618_wdt_ping(struct watchdog_device *wdt_dev)
ret = regmap_read(wdt->rn5t618->regmap, RN5T618_WATCHDOG, &val);
if (ret)
return ret;
- ret = regmap_write(wdt->rn5t618->regmap, RN5T618_WATCHDOG, val);
- if (ret)
- return ret;
-
/* Clear pending watchdog interrupt */
- return regmap_update_bits(wdt->rn5t618->regmap, RN5T618_PWRIRQ,
- RN5T618_PWRIRQ_IR_WDOG, 0);
+ return regmap_write(wdt->rn5t618->regmap, RN5T618_PWRIRQ, ~RN5T618_PWRIRQ_IR_WDOG);
}
static const struct watchdog_info rn5t618_wdt_info = {
.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
--
2.30.2
