From: Werner Fischer <devlists@wefi.net>
To: Guenter Roeck <linux@roeck-us.net>,
Hanspeter Portner <dev@open-music-kontrollers.ch>,
linux-watchdog@vger.kernel.org
Subject: [RFC] improve it87_wdt (IT8784/IT8786) / keeping WDTCTRL unchanged / deactivate watchdog by setting WDTVALLSB/WDTVALMSB 0
Date: Mon, 16 Oct 2023 15:16:54 +0200 [thread overview]
Message-ID: <87b1c97177c43eeec640483cc2f83f5f4d7b1060.camel@wefi.net> (raw)
Hi Guenter, hi Hanspeter,
I currently testing two devices with IT8784 and IT8786 watchdog timers.
Although the chips are supported by it87_wdt.c after Hanspeter's
patches back in 2020, the watchdog functionality does not work in my
following test:
- Debian 12 using Kernel 6.1.58 or current 6.6-rc
- loading module it87_wdt
- starting wd_keepalive Deamon
- killing wd_keepalive using signal 9
-> system keeps on running even after the configured watchdog timeout
For debugging purposes, I have used the patch below to report the
content of the watchdog registers 0x71 (WDTCTRL), 0x72 (WDTCFG), 0x73
(WDTVALLSB), and 0x74 (WDTVALMSB) to the system log.
It turned out, that 0x71 (WDTCTRL) has initially the following value
set (before the module changes it to 0x00):
- 8 decimal (IT8784 / IT8786)
- 4 decimal (IT8613)
I figured out, that the following code line makes the watchdog of
IT8784 and IT8786 non-functional for me:
superio_outb(0x00, WDTCTRL);
I have removed this code in my patch below, then the watchdog works for
IT8784 and IT8786.
I'm not sure, why the WDTCTRL register is set to 0x00 in the code. As
it seems, the register can have different meanings for differnt IT8xxx
chips. Accoring to [1] it seems sufficient to set both WDTVALLSB and
WDTVALMSB to 0x00 to deactivate the watchdog timer: "When the WDT Time-
out Value register is set to a non-zero value, the WDT loads the value
and begin counting down from the value."
This happens e.g. also when wd_keepalive is stopped cleanly.
I am open to support to improve the it87_wdt code.
But before I'm writing and sending a patch, I have the following
question:
* What is the reason, why WDTCTRL is set to 0x00 in the code? and
* Could we think about removing this (at least for IT8784/8786)?
Best regards,
Werner
[1] https://www.ite.com.tw/uploads/product_download/IT8781_A_V0.2.2_121712.pdf
---
diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
index bb1122909396..4ea5505b1771 100644
--- a/drivers/watchdog/it87_wdt.c
+++ b/drivers/watchdog/it87_wdt.c
@@ -50,6 +50,7 @@
/* Chip Id numbers */
#define NO_DEV_ID 0xffff
#define IT8607_ID 0x8607
+#define IT8613_ID 0x8613
#define IT8620_ID 0x8620
#define IT8622_ID 0x8622
#define IT8625_ID 0x8625
@@ -152,9 +153,26 @@ static inline int superio_inw(int reg)
return val;
}
+static void wdt_debug_registers(unsigned int debugstage)
+{
+ unsigned int wdt_ctrl;
+ unsigned int wdt_cfg;
+ unsigned int wdt_vallsb;
+ unsigned int wdt_valmsb;
+ wdt_ctrl = superio_inb(WDTCTRL);
+ wdt_cfg = superio_inb(WDTCFG);
+ wdt_vallsb = superio_inb(WDTVALLSB);
+ wdt_valmsb = superio_inb(WDTVALMSB);
+ pr_info("DEBUG at stage %d: wdt_ctrl=%d; wdt_cfg=%d; wdt_vallsb=%d, wdt_valmsb=%d\n", debugstage, wdt_ctrl, wdt_cfg, wdt_vallsb, wdt_valmsb);
+}
+
/* Internal function, should be called after superio_select(GPIO) */
static void _wdt_update_timeout(unsigned int t)
{
+ wdt_debug_registers(3);
+ /* test werner reset */
+ /* superio_outb(0x00, WDTCTRL); */
+ wdt_debug_registers(9);
unsigned char cfg = WDT_KRST;
if (testmode)
@@ -172,6 +190,7 @@ static void _wdt_update_timeout(unsigned int t)
superio_outb(t, WDTVALLSB);
if (max_units > 255)
superio_outb(t >> 8, WDTVALMSB);
+ wdt_debug_registers(4);
}
static int wdt_update_timeout(unsigned int t)
@@ -258,11 +277,13 @@ static int __init it87_wdt_init(void)
int rc;
rc = superio_enter();
+ wdt_debug_registers(0);
if (rc)
return rc;
chip_type = superio_inw(CHIPID);
chip_rev = superio_inb(CHIPREV) & 0x0f;
+ wdt_debug_registers(1);
superio_exit();
switch (chip_type) {
@@ -274,9 +295,8 @@ static int __init it87_wdt_init(void)
break;
case IT8716_ID:
case IT8726_ID:
- max_units = 65535;
- break;
case IT8607_ID:
+ case IT8613_ID:
case IT8620_ID:
case IT8622_ID:
case IT8625_ID:
@@ -310,10 +330,12 @@ static int __init it87_wdt_init(void)
rc = superio_enter();
if (rc)
return rc;
-
+ wdt_debug_registers(10);
superio_select(GPIO);
+ wdt_debug_registers(11);
superio_outb(WDT_TOV1, WDTCFG);
- superio_outb(0x00, WDTCTRL);
+ /* superio_outb(0x00, WDTCTRL); */
+ wdt_debug_registers(2);
superio_exit();
if (timeout < 1 || timeout > max_units * 60) {
next reply other threads:[~2023-10-16 13:17 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-16 13:16 Werner Fischer [this message]
2023-11-07 13:18 ` [RFC] improve it87_wdt (IT8784/IT8786) / keeping WDTCTRL unchanged / deactivate watchdog by setting WDTVALLSB/WDTVALMSB 0 Werner Fischer
2023-11-07 14:34 ` Guenter Roeck
2023-11-08 9:05 ` Werner Fischer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87b1c97177c43eeec640483cc2f83f5f4d7b1060.camel@wefi.net \
--to=devlists@wefi.net \
--cc=dev@open-music-kontrollers.ch \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).