On 8/12/22 02:56, Kegl Rohit wrote:
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.
It is sent as attachment and thus unusable. In addition to that, it is not a proper patch.
Is the rn5t618_wdt.c driver maintained? Strange that such issue was never noticed.
I'd rather assume that your usage pattern is one that no one ever tried before because it is quite unusual. Why enable NOWAYOUT just to violate it continuously ? Anyway, I think you are a bit off track here. The register 0x13 should probably be volatile. However, register 0x0b is not updated by the chip and thus does not need to be volatile. Replacing the various regmap_update_bits() with regmap_write() is just papering over the problem. To force the write to the chip even if nothing changed, regmap_write_bits() should be used instead of regmap_update_bits() where appropriate. That is probably also the core of the problem: rn5t618_wdt_start() calls regmap_update_bits(). Since no bit was changed, there is actually no write to the chip. Ultimately this causes a timeout since the watchdog is never stopped and the counter is never reset. To avoid that and to make sure that the counter is reset, it is probably sufficient to replace the regmap_update_bits() to enable the watchdog with regmap_write_bits(). The read operation in rn5t618_wdt_ping() doesn't really do anything because the register value is cached. The read only happens to have a value to write. I can not follow your logic that that write could somehow be dangerous. It writes the value that is expected for the register. There is nothing dangerous about it. If there was indeed a problem, you would have to provide specifics; you can not just make such a claim without basis in fact. The interrupt status register at 0x13 should probably be declared volatile to avoid "loss" of interrupt status for other drivers. Clearing the interrupt bit is only necessary to ensure that an expired interrupt is caught if the 'ping' comes late. The same is true for the start function. In normal operation it should not be needed. Either case, if you want your code to be considered, you'll have to split it into two patches, one for mfd and one for watchdog, and you'll have to submit proper patches as outlined in Documentation/process/submitting-patches.rst. Thanks, Guenter
