[PATCH 1/4] watchdog: it87_wdt: add blank line after variable declaration

[PATCH 1/4] watchdog: it87_wdt: add blank line after variable declaration

[Werner Fischer]

This patch fixes the following checkpatch.pl warning:

WARNING: Missing a blank line after declarations

Signed-off-by: Werner Fischer <devlists@wefi.net>
---
 drivers/watchdog/it87_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
index e888b1bdd1f2..6b7f7ec03633 100644
--- a/drivers/watchdog/it87_wdt.c
+++ b/drivers/watchdog/it87_wdt.c
@@ -146,6 +146,7 @@ static inline void superio_outb(int val, int reg)
 static inline int superio_inw(int reg)
 {
 	int val;
+
 	outb(reg++, REG);
 	val = inb(VAL) << 8;
 	outb(reg, REG);
-- 
2.39.2

[PATCH 2/4] watchdog: it87_wdt: Remove redundant max_units setting

[Werner Fischer]

Commit 893dc8b5c978 ("watchdog: it87: Drop support for resetting watchdog
though CIR and Game port") removed the try_gameport variable, and left
max_units setting redundant.

To clean up the code, this patch removes this redundant setting.

Signed-off-by: Werner Fischer <devlists@wefi.net>
---
 drivers/watchdog/it87_wdt.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
index 6b7f7ec03633..ca377096bdd7 100644
--- a/drivers/watchdog/it87_wdt.c
+++ b/drivers/watchdog/it87_wdt.c
@@ -274,10 +274,6 @@ static int __init it87_wdt_init(void)
 	case IT8712_ID:
 		max_units = (chip_rev < 8) ? 255 : 65535;
 		break;
-	case IT8716_ID:
-	case IT8726_ID:
-		max_units = 65535;
-		break;
 	case IT8607_ID:
 	case IT8613_ID:
 	case IT8620_ID:
@@ -287,9 +283,11 @@ static int __init it87_wdt_init(void)
 	case IT8655_ID:
 	case IT8665_ID:
 	case IT8686_ID:
+	case IT8716_ID:
 	case IT8718_ID:
 	case IT8720_ID:
 	case IT8721_ID:
+	case IT8726_ID:
 	case IT8728_ID:
 	case IT8772_ID:
 	case IT8783_ID:
-- 
2.39.2

Re: [PATCH 2/4] watchdog: it87_wdt: Remove redundant max_units setting

[Guenter Roeck]

On Wed, Dec 13, 2023 at 10:45:23AM +0100, Werner Fischer wrote:
> Commit 893dc8b5c978 ("watchdog: it87: Drop support for resetting watchdog
> though CIR and Game port") removed the try_gameport variable, and left
> max_units setting redundant.
> 
> To clean up the code, this patch removes this redundant setting.
> 
> Signed-off-by: Werner Fischer <devlists@wefi.net>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/it87_wdt.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
> index 6b7f7ec03633..ca377096bdd7 100644
> --- a/drivers/watchdog/it87_wdt.c
> +++ b/drivers/watchdog/it87_wdt.c
> @@ -274,10 +274,6 @@ static int __init it87_wdt_init(void)
>  	case IT8712_ID:
>  		max_units = (chip_rev < 8) ? 255 : 65535;
>  		break;
> -	case IT8716_ID:
> -	case IT8726_ID:
> -		max_units = 65535;
> -		break;
>  	case IT8607_ID:
>  	case IT8613_ID:
>  	case IT8620_ID:
> @@ -287,9 +283,11 @@ static int __init it87_wdt_init(void)
>  	case IT8655_ID:
>  	case IT8665_ID:
>  	case IT8686_ID:
> +	case IT8716_ID:
>  	case IT8718_ID:
>  	case IT8720_ID:
>  	case IT8721_ID:
> +	case IT8726_ID:
>  	case IT8728_ID:
>  	case IT8772_ID:
>  	case IT8783_ID:
> -- 
> 2.39.2
> 
> 

[PATCH 3/4] watchdog: it87_wdt: Add IT8659 ID

[Werner Fischer]

This patch adds watchdog support for the ITE IT8659 watchdog.
IT8659 watchdog works in the same way as the other watchdogs supported
by it87_wdt.

Before this patch, IT8659 watchdog is not supported. After a modprobe,
dmesg reports:
  it87_wdt: Unknown Chip found, Chip 8659 Revision 0007

With this patch, modprobe it87_wdt recognizes the watchdog as the dmesg
output shows:
  it87_wdt: Chip IT8659 revision 7 initialized. timeout=60 sec (nowayout=0
  testmode=0)

Watchdog tests on a YANLING YL-ALP3L2C-1235U system have been successful,
the watchdog works as expected with this patch.

Signed-off-by: Werner Fischer <devlists@wefi.net>
---
 drivers/watchdog/it87_wdt.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
index ca377096bdd7..f6a344c002af 100644
--- a/drivers/watchdog/it87_wdt.c
+++ b/drivers/watchdog/it87_wdt.c
@@ -13,9 +13,9 @@
  *		    http://www.ite.com.tw/
  *
  *	Support of the watchdog timers, which are available on
- *	IT8607, IT8613, IT8620, IT8622, IT8625, IT8628, IT8655, IT8665,
- *	IT8686, IT8702, IT8712, IT8716, IT8718, IT8720, IT8721, IT8726,
- *	IT8728, IT8772, IT8783 and IT8784.
+ *	IT8607, IT8613, IT8620, IT8622, IT8625, IT8628, IT8655, IT8659,
+ *	IT8665, IT8686, IT8702, IT8712, IT8716, IT8718, IT8720, IT8721,
+ *	IT8726,	IT8728, IT8772, IT8783, IT8784 and IT8786.
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -56,6 +56,7 @@
 #define IT8625_ID	0x8625
 #define IT8628_ID	0x8628
 #define IT8655_ID	0x8655
+#define IT8659_ID	0x8659
 #define IT8665_ID	0x8665
 #define IT8686_ID	0x8686
 #define IT8702_ID	0x8702
@@ -281,6 +282,7 @@ static int __init it87_wdt_init(void)
 	case IT8625_ID:
 	case IT8628_ID:
 	case IT8655_ID:
+	case IT8659_ID:
 	case IT8665_ID:
 	case IT8686_ID:
 	case IT8716_ID:
-- 
2.39.2

Re: [PATCH 3/4] watchdog: it87_wdt: Add IT8659 ID

[Guenter Roeck]

On Wed, Dec 13, 2023 at 10:45:24AM +0100, Werner Fischer wrote:
> This patch adds watchdog support for the ITE IT8659 watchdog.
> IT8659 watchdog works in the same way as the other watchdogs supported
> by it87_wdt.
> 
> Before this patch, IT8659 watchdog is not supported. After a modprobe,
> dmesg reports:
>   it87_wdt: Unknown Chip found, Chip 8659 Revision 0007
> 
> With this patch, modprobe it87_wdt recognizes the watchdog as the dmesg
> output shows:
>   it87_wdt: Chip IT8659 revision 7 initialized. timeout=60 sec (nowayout=0
>   testmode=0)
> 
> Watchdog tests on a YANLING YL-ALP3L2C-1235U system have been successful,
> the watchdog works as expected with this patch.
> 
> Signed-off-by: Werner Fischer <devlists@wefi.net>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/it87_wdt.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
> index ca377096bdd7..f6a344c002af 100644
> --- a/drivers/watchdog/it87_wdt.c
> +++ b/drivers/watchdog/it87_wdt.c
> @@ -13,9 +13,9 @@
>   *		    http://www.ite.com.tw/
>   *
>   *	Support of the watchdog timers, which are available on
> - *	IT8607, IT8613, IT8620, IT8622, IT8625, IT8628, IT8655, IT8665,
> - *	IT8686, IT8702, IT8712, IT8716, IT8718, IT8720, IT8721, IT8726,
> - *	IT8728, IT8772, IT8783 and IT8784.
> + *	IT8607, IT8613, IT8620, IT8622, IT8625, IT8628, IT8655, IT8659,
> + *	IT8665, IT8686, IT8702, IT8712, IT8716, IT8718, IT8720, IT8721,
> + *	IT8726,	IT8728, IT8772, IT8783, IT8784 and IT8786.
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -56,6 +56,7 @@
>  #define IT8625_ID	0x8625
>  #define IT8628_ID	0x8628
>  #define IT8655_ID	0x8655
> +#define IT8659_ID	0x8659
>  #define IT8665_ID	0x8665
>  #define IT8686_ID	0x8686
>  #define IT8702_ID	0x8702
> @@ -281,6 +282,7 @@ static int __init it87_wdt_init(void)
>  	case IT8625_ID:
>  	case IT8628_ID:
>  	case IT8655_ID:
> +	case IT8659_ID:
>  	case IT8665_ID:
>  	case IT8686_ID:
>  	case IT8716_ID:
> -- 
> 2.39.2
> 
> 

[PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[Werner Fischer]

WDTCTRL bit 3 sets the mode choice for the clock input of IT8784/IT8786.
Some motherboards require this bit to be set to 1 (= PCICLK mode),
otherwise the watchdog functionality gets broken. The BIOS of those
motherboards sets WDTCTRL bit 3 already to 1.

Instead of setting all bits of WDTCTRL to 0 by writing 0x00 to it, keep
bit 3 of it unchanged for IT8784/IT8786 chips. In this way, bit 3 keeps
the status as set by the BIOS of the motherboard.

Watchdog tests have been successful with this patch with the following
systems:
  IT8784: Thomas-Krenn LES plus v2 (YANLING YL-KBRL2 V2)
  IT8786: Thomas-Krenn LES plus v3 (YANLING YL-CLU L2)
  IT8786: Thomas-Krenn LES network 6L v2 (YANLING YL-CLU6L)

Link: https://lore.kernel.org/all/140b264d-341f-465b-8715-dacfe84b3f71@roeck-us.net/

Signed-off-by: Werner Fischer <devlists@wefi.net>
---
 drivers/watchdog/it87_wdt.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
index f6a344c002af..9297a5891912 100644
--- a/drivers/watchdog/it87_wdt.c
+++ b/drivers/watchdog/it87_wdt.c
@@ -258,6 +258,7 @@ static struct watchdog_device wdt_dev = {
 static int __init it87_wdt_init(void)
 {
 	u8  chip_rev;
+	u8 ctrl;
 	int rc;
 
 	rc = superio_enter();
@@ -316,7 +317,18 @@ static int __init it87_wdt_init(void)
 
 	superio_select(GPIO);
 	superio_outb(WDT_TOV1, WDTCFG);
-	superio_outb(0x00, WDTCTRL);
+
+	switch (chip_type) {
+	case IT8784_ID:
+	case IT8786_ID:
+		ctrl = superio_inb(WDTCTRL);
+		ctrl &= 0x08;
+		superio_outb(ctrl, WDTCTRL);
+		break;
+	default:
+		superio_outb(0x00, WDTCTRL);
+	}
+
 	superio_exit();
 
 	if (timeout < 1 || timeout > max_units * 60) {
-- 
2.39.2

Re: [PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[Guenter Roeck]

On Wed, Dec 13, 2023 at 10:45:25AM +0100, Werner Fischer wrote:
> WDTCTRL bit 3 sets the mode choice for the clock input of IT8784/IT8786.
> Some motherboards require this bit to be set to 1 (= PCICLK mode),
> otherwise the watchdog functionality gets broken. The BIOS of those
> motherboards sets WDTCTRL bit 3 already to 1.
> 
> Instead of setting all bits of WDTCTRL to 0 by writing 0x00 to it, keep
> bit 3 of it unchanged for IT8784/IT8786 chips. In this way, bit 3 keeps
> the status as set by the BIOS of the motherboard.
> 
> Watchdog tests have been successful with this patch with the following
> systems:
>   IT8784: Thomas-Krenn LES plus v2 (YANLING YL-KBRL2 V2)
>   IT8786: Thomas-Krenn LES plus v3 (YANLING YL-CLU L2)
>   IT8786: Thomas-Krenn LES network 6L v2 (YANLING YL-CLU6L)
> 
> Link: https://lore.kernel.org/all/140b264d-341f-465b-8715-dacfe84b3f71@roeck-us.net/
> 
> Signed-off-by: Werner Fischer <devlists@wefi.net>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/it87_wdt.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
> index f6a344c002af..9297a5891912 100644
> --- a/drivers/watchdog/it87_wdt.c
> +++ b/drivers/watchdog/it87_wdt.c
> @@ -258,6 +258,7 @@ static struct watchdog_device wdt_dev = {
>  static int __init it87_wdt_init(void)
>  {
>  	u8  chip_rev;
> +	u8 ctrl;
>  	int rc;
>  
>  	rc = superio_enter();
> @@ -316,7 +317,18 @@ static int __init it87_wdt_init(void)
>  
>  	superio_select(GPIO);
>  	superio_outb(WDT_TOV1, WDTCFG);
> -	superio_outb(0x00, WDTCTRL);
> +
> +	switch (chip_type) {
> +	case IT8784_ID:
> +	case IT8786_ID:
> +		ctrl = superio_inb(WDTCTRL);
> +		ctrl &= 0x08;
> +		superio_outb(ctrl, WDTCTRL);
> +		break;
> +	default:
> +		superio_outb(0x00, WDTCTRL);
> +	}
> +
>  	superio_exit();
>  
>  	if (timeout < 1 || timeout > max_units * 60) {
> -- 
> 2.39.2
> 

Re: [PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[James Hilliard]

On Wed, Dec 13, 2023 at 1:45 AM Werner Fischer <devlists@wefi.net> wrote:
>
> WDTCTRL bit 3 sets the mode choice for the clock input of IT8784/IT8786.
> Some motherboards require this bit to be set to 1 (= PCICLK mode),
> otherwise the watchdog functionality gets broken. The BIOS of those
> motherboards sets WDTCTRL bit 3 already to 1.
>
> Instead of setting all bits of WDTCTRL to 0 by writing 0x00 to it, keep
> bit 3 of it unchanged for IT8784/IT8786 chips. In this way, bit 3 keeps
> the status as set by the BIOS of the motherboard.

I have a board(https://qotom.net/product/94.html) with an IT8786
revision 4 which is recognized but doesn't seem to ever trigger. Did
your IT8786 based boards show revision 4 like mine do?

[    1.607590] it87_wdt: Chip IT8786 revision 4 initialized.
timeout=60 sec (nowayout=0 testmode=0)
[    2.367608] systemd[1]: Using hardware watchdog 'IT87 WDT', version
1, device /dev/watchdog0

Docs I have from the vendor just show bit 3 as reserved:

https://qotom.us/download/SuperIO/IT8786_B_V0.2_industrial_111412.pdf

8.10.8 Watch Dog Timer Control Register (Index=71h, Default=00h)

Bit      Description
7        WDT is reset upon a CIR interrupt.
6        WDT is reset upon a KBC(Mouse) interrupt.
5        WDT is reset upon a KBC(Keyboard) interrupt.
4        WDT Status will not be cleared by VCCOK or LRESET#, and only
be cleared while write one to WDT Status
         1: Enable
         0: Disable
3-2      Reserved
1        Force Time-out
         This bit is self-cleared.
0        WDT Status
         1: WDT value is equal to 0.
         0: WDT value is not is equal to 0.

Any idea why the docs I have would just show bit 3 as reserved?

Did you have any information from your vendor under what conditions
bit 3 should be set?

>
> Watchdog tests have been successful with this patch with the following
> systems:
>   IT8784: Thomas-Krenn LES plus v2 (YANLING YL-KBRL2 V2)
>   IT8786: Thomas-Krenn LES plus v3 (YANLING YL-CLU L2)
>   IT8786: Thomas-Krenn LES network 6L v2 (YANLING YL-CLU6L)
>
> Link: https://lore.kernel.org/all/140b264d-341f-465b-8715-dacfe84b3f71@roeck-us.net/
>
> Signed-off-by: Werner Fischer <devlists@wefi.net>
> ---
>  drivers/watchdog/it87_wdt.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
> index f6a344c002af..9297a5891912 100644
> --- a/drivers/watchdog/it87_wdt.c
> +++ b/drivers/watchdog/it87_wdt.c
> @@ -258,6 +258,7 @@ static struct watchdog_device wdt_dev = {
>  static int __init it87_wdt_init(void)
>  {
>         u8  chip_rev;
> +       u8 ctrl;
>         int rc;
>
>         rc = superio_enter();
> @@ -316,7 +317,18 @@ static int __init it87_wdt_init(void)
>
>         superio_select(GPIO);
>         superio_outb(WDT_TOV1, WDTCFG);
> -       superio_outb(0x00, WDTCTRL);
> +
> +       switch (chip_type) {
> +       case IT8784_ID:
> +       case IT8786_ID:
> +               ctrl = superio_inb(WDTCTRL);

If I print this value out like this:
pr_warn("WDTCTRL initial: %02x\n", ctrl);

I get 0x00:
[    1.607480] it87_wdt: WDTCTRL initial: 00

Do you think it's required that the kernel set bit 3 for some boards for
the watchdog to work correctly if not set by the BIOS?

Or maybe it's required to configure additional registers?

For my board I don't see options in BIOS for configuring the watchdog.

> +               ctrl &= 0x08;
> +               superio_outb(ctrl, WDTCTRL);
> +               break;
> +       default:
> +               superio_outb(0x00, WDTCTRL);
> +       }
> +
>         superio_exit();
>
>         if (timeout < 1 || timeout > max_units * 60) {
> --
> 2.39.2
>

Re: [PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[Guenter Roeck]

On 7/6/24 12:06, James Hilliard wrote:
> On Wed, Dec 13, 2023 at 1:45 AM Werner Fischer <devlists@wefi.net> wrote:
>>
>> WDTCTRL bit 3 sets the mode choice for the clock input of IT8784/IT8786.
>> Some motherboards require this bit to be set to 1 (= PCICLK mode),
>> otherwise the watchdog functionality gets broken. The BIOS of those
>> motherboards sets WDTCTRL bit 3 already to 1.
>>
>> Instead of setting all bits of WDTCTRL to 0 by writing 0x00 to it, keep
>> bit 3 of it unchanged for IT8784/IT8786 chips. In this way, bit 3 keeps
>> the status as set by the BIOS of the motherboard.
> 
> I have a board(https://qotom.net/product/94.html) with an IT8786
> revision 4 which is recognized but doesn't seem to ever trigger. Did
> your IT8786 based boards show revision 4 like mine do?
> 
> [    1.607590] it87_wdt: Chip IT8786 revision 4 initialized.
> timeout=60 sec (nowayout=0 testmode=0)
> [    2.367608] systemd[1]: Using hardware watchdog 'IT87 WDT', version
> 1, device /dev/watchdog0
> 
> Docs I have from the vendor just show bit 3 as reserved:
> 
> https://qotom.us/download/SuperIO/IT8786_B_V0.2_industrial_111412.pdf
> 
> 8.10.8 Watch Dog Timer Control Register (Index=71h, Default=00h)
> 
> Bit      Description
> 7        WDT is reset upon a CIR interrupt.
> 6        WDT is reset upon a KBC(Mouse) interrupt.
> 5        WDT is reset upon a KBC(Keyboard) interrupt.
> 4        WDT Status will not be cleared by VCCOK or LRESET#, and only
> be cleared while write one to WDT Status
>           1: Enable
>           0: Disable
> 3-2      Reserved
> 1        Force Time-out
>           This bit is self-cleared.
> 0        WDT Status
>           1: WDT value is equal to 0.
>           0: WDT value is not is equal to 0.
> 
> Any idea why the docs I have would just show bit 3 as reserved?
> 
> Did you have any information from your vendor under what conditions
> bit 3 should be set?
> 

On ITE8784E bit 3 is "External CLK_IN Select".

>>
>> Watchdog tests have been successful with this patch with the following
>> systems:
>>    IT8784: Thomas-Krenn LES plus v2 (YANLING YL-KBRL2 V2)
>>    IT8786: Thomas-Krenn LES plus v3 (YANLING YL-CLU L2)
>>    IT8786: Thomas-Krenn LES network 6L v2 (YANLING YL-CLU6L)
>>
>> Link: https://lore.kernel.org/all/140b264d-341f-465b-8715-dacfe84b3f71@roeck-us.net/
>>
>> Signed-off-by: Werner Fischer <devlists@wefi.net>
>> ---
>>   drivers/watchdog/it87_wdt.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
>> index f6a344c002af..9297a5891912 100644
>> --- a/drivers/watchdog/it87_wdt.c
>> +++ b/drivers/watchdog/it87_wdt.c
>> @@ -258,6 +258,7 @@ static struct watchdog_device wdt_dev = {
>>   static int __init it87_wdt_init(void)
>>   {
>>          u8  chip_rev;
>> +       u8 ctrl;
>>          int rc;
>>
>>          rc = superio_enter();
>> @@ -316,7 +317,18 @@ static int __init it87_wdt_init(void)
>>
>>          superio_select(GPIO);
>>          superio_outb(WDT_TOV1, WDTCFG);
>> -       superio_outb(0x00, WDTCTRL);
>> +
>> +       switch (chip_type) {
>> +       case IT8784_ID:
>> +       case IT8786_ID:
>> +               ctrl = superio_inb(WDTCTRL);
> 
> If I print this value out like this:
> pr_warn("WDTCTRL initial: %02x\n", ctrl);
> 
> I get 0x00:
> [    1.607480] it87_wdt: WDTCTRL initial: 00
> 
> Do you think it's required that the kernel set bit 3 for some boards for
> the watchdog to work correctly if not set by the BIOS?
> 
That is done for none of the boards. The code in question does not _clear_ the bit,
but it is never set.

> Or maybe it's required to configure additional registers?
> 
I would suspect that to be the case. You might want to check register 0x72.

Guenter

Re: [PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[James Hilliard]

On Sat, Jul 6, 2024 at 1:47 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/6/24 12:06, James Hilliard wrote:
> > On Wed, Dec 13, 2023 at 1:45 AM Werner Fischer <devlists@wefi.net> wrote:
> >>
> >> WDTCTRL bit 3 sets the mode choice for the clock input of IT8784/IT8786.
> >> Some motherboards require this bit to be set to 1 (= PCICLK mode),
> >> otherwise the watchdog functionality gets broken. The BIOS of those
> >> motherboards sets WDTCTRL bit 3 already to 1.
> >>
> >> Instead of setting all bits of WDTCTRL to 0 by writing 0x00 to it, keep
> >> bit 3 of it unchanged for IT8784/IT8786 chips. In this way, bit 3 keeps
> >> the status as set by the BIOS of the motherboard.
> >
> > I have a board(https://qotom.net/product/94.html) with an IT8786
> > revision 4 which is recognized but doesn't seem to ever trigger. Did
> > your IT8786 based boards show revision 4 like mine do?
> >
> > [    1.607590] it87_wdt: Chip IT8786 revision 4 initialized.
> > timeout=60 sec (nowayout=0 testmode=0)
> > [    2.367608] systemd[1]: Using hardware watchdog 'IT87 WDT', version
> > 1, device /dev/watchdog0
> >
> > Docs I have from the vendor just show bit 3 as reserved:
> >
> > https://qotom.us/download/SuperIO/IT8786_B_V0.2_industrial_111412.pdf
> >
> > 8.10.8 Watch Dog Timer Control Register (Index=71h, Default=00h)
> >
> > Bit      Description
> > 7        WDT is reset upon a CIR interrupt.
> > 6        WDT is reset upon a KBC(Mouse) interrupt.
> > 5        WDT is reset upon a KBC(Keyboard) interrupt.
> > 4        WDT Status will not be cleared by VCCOK or LRESET#, and only
> > be cleared while write one to WDT Status
> >           1: Enable
> >           0: Disable
> > 3-2      Reserved
> > 1        Force Time-out
> >           This bit is self-cleared.
> > 0        WDT Status
> >           1: WDT value is equal to 0.
> >           0: WDT value is not is equal to 0.
> >
> > Any idea why the docs I have would just show bit 3 as reserved?
> >
> > Did you have any information from your vendor under what conditions
> > bit 3 should be set?
> >
>
> On ITE8784E bit 3 is "External CLK_IN Select".
>
> >>
> >> Watchdog tests have been successful with this patch with the following
> >> systems:
> >>    IT8784: Thomas-Krenn LES plus v2 (YANLING YL-KBRL2 V2)
> >>    IT8786: Thomas-Krenn LES plus v3 (YANLING YL-CLU L2)
> >>    IT8786: Thomas-Krenn LES network 6L v2 (YANLING YL-CLU6L)
> >>
> >> Link: https://lore.kernel.org/all/140b264d-341f-465b-8715-dacfe84b3f71@roeck-us.net/
> >>
> >> Signed-off-by: Werner Fischer <devlists@wefi.net>
> >> ---
> >>   drivers/watchdog/it87_wdt.c | 14 +++++++++++++-
> >>   1 file changed, 13 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
> >> index f6a344c002af..9297a5891912 100644
> >> --- a/drivers/watchdog/it87_wdt.c
> >> +++ b/drivers/watchdog/it87_wdt.c
> >> @@ -258,6 +258,7 @@ static struct watchdog_device wdt_dev = {
> >>   static int __init it87_wdt_init(void)
> >>   {
> >>          u8  chip_rev;
> >> +       u8 ctrl;
> >>          int rc;
> >>
> >>          rc = superio_enter();
> >> @@ -316,7 +317,18 @@ static int __init it87_wdt_init(void)
> >>
> >>          superio_select(GPIO);
> >>          superio_outb(WDT_TOV1, WDTCFG);
> >> -       superio_outb(0x00, WDTCTRL);
> >> +
> >> +       switch (chip_type) {
> >> +       case IT8784_ID:
> >> +       case IT8786_ID:
> >> +               ctrl = superio_inb(WDTCTRL);
> >
> > If I print this value out like this:
> > pr_warn("WDTCTRL initial: %02x\n", ctrl);
> >
> > I get 0x00:
> > [    1.607480] it87_wdt: WDTCTRL initial: 00
> >
> > Do you think it's required that the kernel set bit 3 for some boards for
> > the watchdog to work correctly if not set by the BIOS?
> >
> That is done for none of the boards. The code in question does not _clear_ the bit,
> but it is never set.
>
> > Or maybe it's required to configure additional registers?
> >
> I would suspect that to be the case. You might want to check register 0x72.

So it turns out using the wdat_wdt driver works on this board.

If I log register 0xF1 I get a value of 0x44 which the IT8786 docs
indicate for bit 2:
This bit is to enable the generation of an SMI# due to WDT’s IRQ (EN_WDT).

If SMI is enabled for the WDT IRQ does that mean one can't use the it87_wdt
driver and instead must use wdat_wdt?

I noticed there's some ACPI resource conflict detection in the hwmon IT8786
driver(although the hwmon driver doesn't indicate a resource conflict on this
board for me. I wonder if there is a resource conflict here with the watchdog
that should be detected?

>
> Guenter
>

Re: [PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[Guenter Roeck]

On 7/11/24 10:43, James Hilliard wrote:
> On Sat, Jul 6, 2024 at 1:47 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 7/6/24 12:06, James Hilliard wrote:
>>> On Wed, Dec 13, 2023 at 1:45 AM Werner Fischer <devlists@wefi.net> wrote:
>>>>
>>>> WDTCTRL bit 3 sets the mode choice for the clock input of IT8784/IT8786.
>>>> Some motherboards require this bit to be set to 1 (= PCICLK mode),
>>>> otherwise the watchdog functionality gets broken. The BIOS of those
>>>> motherboards sets WDTCTRL bit 3 already to 1.
>>>>
>>>> Instead of setting all bits of WDTCTRL to 0 by writing 0x00 to it, keep
>>>> bit 3 of it unchanged for IT8784/IT8786 chips. In this way, bit 3 keeps
>>>> the status as set by the BIOS of the motherboard.
>>>
>>> I have a board(https://qotom.net/product/94.html) with an IT8786
>>> revision 4 which is recognized but doesn't seem to ever trigger. Did
>>> your IT8786 based boards show revision 4 like mine do?
>>>
>>> [    1.607590] it87_wdt: Chip IT8786 revision 4 initialized.
>>> timeout=60 sec (nowayout=0 testmode=0)
>>> [    2.367608] systemd[1]: Using hardware watchdog 'IT87 WDT', version
>>> 1, device /dev/watchdog0
>>>
>>> Docs I have from the vendor just show bit 3 as reserved:
>>>
>>> https://qotom.us/download/SuperIO/IT8786_B_V0.2_industrial_111412.pdf
>>>
>>> 8.10.8 Watch Dog Timer Control Register (Index=71h, Default=00h)
>>>
>>> Bit      Description
>>> 7        WDT is reset upon a CIR interrupt.
>>> 6        WDT is reset upon a KBC(Mouse) interrupt.
>>> 5        WDT is reset upon a KBC(Keyboard) interrupt.
>>> 4        WDT Status will not be cleared by VCCOK or LRESET#, and only
>>> be cleared while write one to WDT Status
>>>            1: Enable
>>>            0: Disable
>>> 3-2      Reserved
>>> 1        Force Time-out
>>>            This bit is self-cleared.
>>> 0        WDT Status
>>>            1: WDT value is equal to 0.
>>>            0: WDT value is not is equal to 0.
>>>
>>> Any idea why the docs I have would just show bit 3 as reserved?
>>>
>>> Did you have any information from your vendor under what conditions
>>> bit 3 should be set?
>>>
>>
>> On ITE8784E bit 3 is "External CLK_IN Select".
>>
>>>>
>>>> Watchdog tests have been successful with this patch with the following
>>>> systems:
>>>>     IT8784: Thomas-Krenn LES plus v2 (YANLING YL-KBRL2 V2)
>>>>     IT8786: Thomas-Krenn LES plus v3 (YANLING YL-CLU L2)
>>>>     IT8786: Thomas-Krenn LES network 6L v2 (YANLING YL-CLU6L)
>>>>
>>>> Link: https://lore.kernel.org/all/140b264d-341f-465b-8715-dacfe84b3f71@roeck-us.net/
>>>>
>>>> Signed-off-by: Werner Fischer <devlists@wefi.net>
>>>> ---
>>>>    drivers/watchdog/it87_wdt.c | 14 +++++++++++++-
>>>>    1 file changed, 13 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
>>>> index f6a344c002af..9297a5891912 100644
>>>> --- a/drivers/watchdog/it87_wdt.c
>>>> +++ b/drivers/watchdog/it87_wdt.c
>>>> @@ -258,6 +258,7 @@ static struct watchdog_device wdt_dev = {
>>>>    static int __init it87_wdt_init(void)
>>>>    {
>>>>           u8  chip_rev;
>>>> +       u8 ctrl;
>>>>           int rc;
>>>>
>>>>           rc = superio_enter();
>>>> @@ -316,7 +317,18 @@ static int __init it87_wdt_init(void)
>>>>
>>>>           superio_select(GPIO);
>>>>           superio_outb(WDT_TOV1, WDTCFG);
>>>> -       superio_outb(0x00, WDTCTRL);
>>>> +
>>>> +       switch (chip_type) {
>>>> +       case IT8784_ID:
>>>> +       case IT8786_ID:
>>>> +               ctrl = superio_inb(WDTCTRL);
>>>
>>> If I print this value out like this:
>>> pr_warn("WDTCTRL initial: %02x\n", ctrl);
>>>
>>> I get 0x00:
>>> [    1.607480] it87_wdt: WDTCTRL initial: 00
>>>
>>> Do you think it's required that the kernel set bit 3 for some boards for
>>> the watchdog to work correctly if not set by the BIOS?
>>>
>> That is done for none of the boards. The code in question does not _clear_ the bit,
>> but it is never set.
>>
>>> Or maybe it's required to configure additional registers?
>>>
>> I would suspect that to be the case. You might want to check register 0x72.
> 
> So it turns out using the wdat_wdt driver works on this board.
> 
> If I log register 0xF1 I get a value of 0x44 which the IT8786 docs
> indicate for bit 2:
> This bit is to enable the generation of an SMI# due to WDT’s IRQ (EN_WDT).
> 

Interesting find. I looked into some other ITE datasheets; they all have this bit.

> If SMI is enabled for the WDT IRQ does that mean one can't use the it87_wdt
> driver and instead must use wdat_wdt?
> 
Looks like it.

> I noticed there's some ACPI resource conflict detection in the hwmon IT8786
> driver(although the hwmon driver doesn't indicate a resource conflict on this
> board for me. I wonder if there is a resource conflict here with the watchdog
> that should be detected?
> 

The hwmon driver detects the conflict on the hwmon register space, not the
Super-IO configuration address space. I would suspect that pretty much all
systems would show a resource conflict on the Super-IO configuration space.

The best we could possibly do might be to add a check for the bit in register
0xf1 and warn the user that they might have to use the ACPI driver if the bit
is set. I am not sure if that would be helpful or just add noise, though.

Guenter

Re: [PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[James Hilliard]

On Thu, Jul 11, 2024 at 1:17 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/11/24 10:43, James Hilliard wrote:
> > On Sat, Jul 6, 2024 at 1:47 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 7/6/24 12:06, James Hilliard wrote:
> >>> On Wed, Dec 13, 2023 at 1:45 AM Werner Fischer <devlists@wefi.net> wrote:
> >>>>
> >>>> WDTCTRL bit 3 sets the mode choice for the clock input of IT8784/IT8786.
> >>>> Some motherboards require this bit to be set to 1 (= PCICLK mode),
> >>>> otherwise the watchdog functionality gets broken. The BIOS of those
> >>>> motherboards sets WDTCTRL bit 3 already to 1.
> >>>>
> >>>> Instead of setting all bits of WDTCTRL to 0 by writing 0x00 to it, keep
> >>>> bit 3 of it unchanged for IT8784/IT8786 chips. In this way, bit 3 keeps
> >>>> the status as set by the BIOS of the motherboard.
> >>>
> >>> I have a board(https://qotom.net/product/94.html) with an IT8786
> >>> revision 4 which is recognized but doesn't seem to ever trigger. Did
> >>> your IT8786 based boards show revision 4 like mine do?
> >>>
> >>> [    1.607590] it87_wdt: Chip IT8786 revision 4 initialized.
> >>> timeout=60 sec (nowayout=0 testmode=0)
> >>> [    2.367608] systemd[1]: Using hardware watchdog 'IT87 WDT', version
> >>> 1, device /dev/watchdog0
> >>>
> >>> Docs I have from the vendor just show bit 3 as reserved:
> >>>
> >>> https://qotom.us/download/SuperIO/IT8786_B_V0.2_industrial_111412.pdf
> >>>
> >>> 8.10.8 Watch Dog Timer Control Register (Index=71h, Default=00h)
> >>>
> >>> Bit      Description
> >>> 7        WDT is reset upon a CIR interrupt.
> >>> 6        WDT is reset upon a KBC(Mouse) interrupt.
> >>> 5        WDT is reset upon a KBC(Keyboard) interrupt.
> >>> 4        WDT Status will not be cleared by VCCOK or LRESET#, and only
> >>> be cleared while write one to WDT Status
> >>>            1: Enable
> >>>            0: Disable
> >>> 3-2      Reserved
> >>> 1        Force Time-out
> >>>            This bit is self-cleared.
> >>> 0        WDT Status
> >>>            1: WDT value is equal to 0.
> >>>            0: WDT value is not is equal to 0.
> >>>
> >>> Any idea why the docs I have would just show bit 3 as reserved?
> >>>
> >>> Did you have any information from your vendor under what conditions
> >>> bit 3 should be set?
> >>>
> >>
> >> On ITE8784E bit 3 is "External CLK_IN Select".
> >>
> >>>>
> >>>> Watchdog tests have been successful with this patch with the following
> >>>> systems:
> >>>>     IT8784: Thomas-Krenn LES plus v2 (YANLING YL-KBRL2 V2)
> >>>>     IT8786: Thomas-Krenn LES plus v3 (YANLING YL-CLU L2)
> >>>>     IT8786: Thomas-Krenn LES network 6L v2 (YANLING YL-CLU6L)
> >>>>
> >>>> Link: https://lore.kernel.org/all/140b264d-341f-465b-8715-dacfe84b3f71@roeck-us.net/
> >>>>
> >>>> Signed-off-by: Werner Fischer <devlists@wefi.net>
> >>>> ---
> >>>>    drivers/watchdog/it87_wdt.c | 14 +++++++++++++-
> >>>>    1 file changed, 13 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
> >>>> index f6a344c002af..9297a5891912 100644
> >>>> --- a/drivers/watchdog/it87_wdt.c
> >>>> +++ b/drivers/watchdog/it87_wdt.c
> >>>> @@ -258,6 +258,7 @@ static struct watchdog_device wdt_dev = {
> >>>>    static int __init it87_wdt_init(void)
> >>>>    {
> >>>>           u8  chip_rev;
> >>>> +       u8 ctrl;
> >>>>           int rc;
> >>>>
> >>>>           rc = superio_enter();
> >>>> @@ -316,7 +317,18 @@ static int __init it87_wdt_init(void)
> >>>>
> >>>>           superio_select(GPIO);
> >>>>           superio_outb(WDT_TOV1, WDTCFG);
> >>>> -       superio_outb(0x00, WDTCTRL);
> >>>> +
> >>>> +       switch (chip_type) {
> >>>> +       case IT8784_ID:
> >>>> +       case IT8786_ID:
> >>>> +               ctrl = superio_inb(WDTCTRL);
> >>>
> >>> If I print this value out like this:
> >>> pr_warn("WDTCTRL initial: %02x\n", ctrl);
> >>>
> >>> I get 0x00:
> >>> [    1.607480] it87_wdt: WDTCTRL initial: 00
> >>>
> >>> Do you think it's required that the kernel set bit 3 for some boards for
> >>> the watchdog to work correctly if not set by the BIOS?
> >>>
> >> That is done for none of the boards. The code in question does not _clear_ the bit,
> >> but it is never set.
> >>
> >>> Or maybe it's required to configure additional registers?
> >>>
> >> I would suspect that to be the case. You might want to check register 0x72.
> >
> > So it turns out using the wdat_wdt driver works on this board.
> >
> > If I log register 0xF1 I get a value of 0x44 which the IT8786 docs
> > indicate for bit 2:
> > This bit is to enable the generation of an SMI# due to WDT’s IRQ (EN_WDT).
> >
>
> Interesting find. I looked into some other ITE datasheets; they all have this bit.
>
> > If SMI is enabled for the WDT IRQ does that mean one can't use the it87_wdt
> > driver and instead must use wdat_wdt?
> >
> Looks like it.
>
> > I noticed there's some ACPI resource conflict detection in the hwmon IT8786
> > driver(although the hwmon driver doesn't indicate a resource conflict on this
> > board for me. I wonder if there is a resource conflict here with the watchdog
> > that should be detected?
> >
>
> The hwmon driver detects the conflict on the hwmon register space, not the
> Super-IO configuration address space. I would suspect that pretty much all
> systems would show a resource conflict on the Super-IO configuration space.
>
> The best we could possibly do might be to add a check for the bit in register
> 0xf1 and warn the user that they might have to use the ACPI driver if the bit
> is set. I am not sure if that would be helpful or just add noise, though.

Do your systems which work with the it87_wdt driver have that 0xF1 bit not set?

I'm thinking we should check for that bit and prevent loading the
it87_wdt driver if
it's set(maybe along with an override param). That way the wdat_wdt driver I
think should end up as the primary watchdog(systemd only seems to properly
handle one watchdog).

>
> Guenter
>

Re: [PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[Guenter Roeck]

On 7/11/24 14:09, James Hilliard wrote:

>> The best we could possibly do might be to add a check for the bit in register
>> 0xf1 and warn the user that they might have to use the ACPI driver if the bit
>> is set. I am not sure if that would be helpful or just add noise, though.
> 
> Do your systems which work with the it87_wdt driver have that 0xF1 bit not set?
> 

I only have one such system left, and the bit is not set on that system.
I avoid buying hardware with ITE Super-IO chips nowadays since their support
for Linux is non-existent.

> I'm thinking we should check for that bit and prevent loading the
> it87_wdt driver if

No. That would create the risk of no longer loading the driver on systems where
it currently works.

> it's set(maybe along with an override param). That way the wdat_wdt driver I

I prefer the less invasive version of logging a message. The user can then
block the it87_wdt driver if it doesn't work.

Guenter

Re: [PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[James Hilliard]

On Thu, Jul 11, 2024 at 3:42 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/11/24 14:09, James Hilliard wrote:
>
> >> The best we could possibly do might be to add a check for the bit in register
> >> 0xf1 and warn the user that they might have to use the ACPI driver if the bit
> >> is set. I am not sure if that would be helpful or just add noise, though.
> >
> > Do your systems which work with the it87_wdt driver have that 0xF1 bit not set?
> >
>
> I only have one such system left, and the bit is not set on that system.
> I avoid buying hardware with ITE Super-IO chips nowadays since their support
> for Linux is non-existent.

Yeah, I got stuck with a fleet of these boards, trying to make the best of it.

>
> > I'm thinking we should check for that bit and prevent loading the
> > it87_wdt driver if
>
> No. That would create the risk of no longer loading the driver on systems where
> it currently works.

Hmm, any idea how likely it would be that the bit could be set on a board
which the driver works on?

Or maybe best to have a quirks table with dmi matching to disable the
driver on known broken systems?

>
> > it's set(maybe along with an override param). That way the wdat_wdt driver I
>
> I prefer the less invasive version of logging a message. The user can then
> block the it87_wdt driver if it doesn't work.

Hmm, I build multiple watchdog drivers into the same kernel and somewhat
rely on the autodetection working correctly as I support multiple boards
with the same kernel build. It's not exactly trivial to conditionally prevent
drivers from loading when built into the kernel AFAIU.

>
> Guenter
>

Re: [PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[Guenter Roeck]

On 7/11/24 15:14, James Hilliard wrote:
> On Thu, Jul 11, 2024 at 3:42 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 7/11/24 14:09, James Hilliard wrote:
>>
>>>> The best we could possibly do might be to add a check for the bit in register
>>>> 0xf1 and warn the user that they might have to use the ACPI driver if the bit
>>>> is set. I am not sure if that would be helpful or just add noise, though.
>>>
>>> Do your systems which work with the it87_wdt driver have that 0xF1 bit not set?
>>>
>>
>> I only have one such system left, and the bit is not set on that system.
>> I avoid buying hardware with ITE Super-IO chips nowadays since their support
>> for Linux is non-existent.
> 
> Yeah, I got stuck with a fleet of these boards, trying to make the best of it.
> 
>>
>>> I'm thinking we should check for that bit and prevent loading the
>>> it87_wdt driver if
>>
>> No. That would create the risk of no longer loading the driver on systems where
>> it currently works.
> 
> Hmm, any idea how likely it would be that the bit could be set on a board
> which the driver works on?
> 

No idea, but I would not want to disable it just to find out with a flurry
of angry e-mails.

> Or maybe best to have a quirks table with dmi matching to disable the
> driver on known broken systems?
> 
>>
>>> it's set(maybe along with an override param). That way the wdat_wdt driver I
>>
>> I prefer the less invasive version of logging a message. The user can then
>> block the it87_wdt driver if it doesn't work.
> 
> Hmm, I build multiple watchdog drivers into the same kernel and somewhat
> rely on the autodetection working correctly as I support multiple boards
> with the same kernel build. It's not exactly trivial to conditionally prevent
> drivers from loading when built into the kernel AFAIU.
> 

Those drivers should never be built into the kernel; they should be built
as modules, and module load instructions in /etc/modprobe.d (or whatever the
distribution uses) should be used to determine which drivers to load. I really
would not want to rely on a bit such as the smi interrupt bit to determine
if the watchdog is used by ACPI.

This is actually a multi-level problem: Even if there is an ACPI watchdog,
that does not mean that ACPI uses the Super-IO chip for its watchdog implementation.
It might as well using the ICH watchdog on Intel systems or the TCO watchdog on
AMD systems. Similar, even if the SMI interrupt bit is not set, it is essentially
unknown if the it87_wdt driver actually works, because its reset pins might not
be connected. Or, of course, both watchdogs might work.

Assuming the wdat_wdt driver auto-loads on your system (I think it should),
can you write a little script which loads the it87_wdt driver only if the
wdat_wdt driver is not loaded ?

Actually, just building the wdat_wdt driver into the kernel and it87_wdt as
module (and loading it via modules.d) should work since the wdat_wdt driver
would then be instantiated first, and the first watchdog is all that systemd
cares about.

Thanks,
Guenter

Re: [PATCH 4/4] watchdog: it87_wdt: Keep WDTCTRL bit 3 unmodified for IT8784/IT8786

[James Hilliard]

On Thu, Jul 11, 2024 at 5:48 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/11/24 15:14, James Hilliard wrote:
> > On Thu, Jul 11, 2024 at 3:42 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 7/11/24 14:09, James Hilliard wrote:
> >>
> >>>> The best we could possibly do might be to add a check for the bit in register
> >>>> 0xf1 and warn the user that they might have to use the ACPI driver if the bit
> >>>> is set. I am not sure if that would be helpful or just add noise, though.
> >>>
> >>> Do your systems which work with the it87_wdt driver have that 0xF1 bit not set?
> >>>
> >>
> >> I only have one such system left, and the bit is not set on that system.
> >> I avoid buying hardware with ITE Super-IO chips nowadays since their support
> >> for Linux is non-existent.
> >
> > Yeah, I got stuck with a fleet of these boards, trying to make the best of it.
> >
> >>
> >>> I'm thinking we should check for that bit and prevent loading the
> >>> it87_wdt driver if
> >>
> >> No. That would create the risk of no longer loading the driver on systems where
> >> it currently works.
> >
> > Hmm, any idea how likely it would be that the bit could be set on a board
> > which the driver works on?
> >
>
> No idea, but I would not want to disable it just to find out with a flurry
> of angry e-mails.
>
> > Or maybe best to have a quirks table with dmi matching to disable the
> > driver on known broken systems?
> >
> >>
> >>> it's set(maybe along with an override param). That way the wdat_wdt driver I
> >>
> >> I prefer the less invasive version of logging a message. The user can then
> >> block the it87_wdt driver if it doesn't work.
> >
> > Hmm, I build multiple watchdog drivers into the same kernel and somewhat
> > rely on the autodetection working correctly as I support multiple boards
> > with the same kernel build. It's not exactly trivial to conditionally prevent
> > drivers from loading when built into the kernel AFAIU.
> >
>
> Those drivers should never be built into the kernel; they should be built
> as modules, and module load instructions in /etc/modprobe.d (or whatever the
> distribution uses) should be used to determine which drivers to load. I really
> would not want to rely on a bit such as the smi interrupt bit to determine
> if the watchdog is used by ACPI.
>
> This is actually a multi-level problem: Even if there is an ACPI watchdog,
> that does not mean that ACPI uses the Super-IO chip for its watchdog implementation.
> It might as well using the ICH watchdog on Intel systems or the TCO watchdog on
> AMD systems. Similar, even if the SMI interrupt bit is not set, it is essentially
> unknown if the it87_wdt driver actually works, because its reset pins might not
> be connected. Or, of course, both watchdogs might work.
>
> Assuming the wdat_wdt driver auto-loads on your system (I think it should),
> can you write a little script which loads the it87_wdt driver only if the
> wdat_wdt driver is not loaded ?

I'm a bit wary of using scripts to manually choose drivers like this, I
suppose it could work but I'm thinking it may be somewhat brittle.

So I ended up just disabling the it87_wdt driver, but a different batch of
boards also with the it8786 wdt chip we found doesn't appear to have a
functional wdat_wdt and does have working it87_wdt support so I'm now
looking at this again.

I messaged the vendor and they sent me some wdt sample code(that
I still need to test) which is supposed to be for the board that I'm having to
use wdat_wdt on.

It's setting some additional bits(like bit 5) which is interesting.

#include <sys/io.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/io.h>
#include <string.h>

#define BIT0 0x01
#define BIT1 0x02
#define BIT2 0x04
#define BIT3 0x08
#define BIT4 0x10
#define BIT5 0x20
#define BIT6 0x40
#define BIT7 0x80

#define BYTE unsigned char

main(int argc, char* argv[])
{

int TValue;
BYTE data =0;

ioperm(0x2e,1,1);
ioperm(0x2f,1,1);

//Enter SIO  Configuration Mode
outb(0x87,0x2e);
outb(0x01,0x2e);
outb(0x55,0x2e);
outb(0x55,0x2e);

//Select Logic Device 7
outb(0x07,0x2e);
outb(0x07,0x2f);

//Setting Default Watch Dog is Disabled
outb(0x71,0x2e);
outb(0x00,0x2f);

//Watchdog Reset Code
outb(0x72,0x2e);
outb(0x80,0x2f);


if(argc == 3)
{
    if(strcmp(argv[1], "-M") == 0)
    {
        outb(0x72,0x2e);
        data = inb(0x2f);
        data &= ~BIT7;
        outb(data,0x2f);

        TValue = atoi(argv[2]);
        outb(0x73,0x2e);
        outb((BYTE)TValue,0x2f);
    }

    if(strcmp(argv[1], "-S") == 0)
    {
       outb(0x72,0x2e);
        data = inb(0x2f);
        data |= BIT7;
        outb(data,0x2f);

        TValue = atoi(argv[2]);
        outb(0x73,0x2e);
        outb((BYTE)TValue,0x2f);
    }

    outb(0x07,0x2e);
    outb(0x04,0x2f);

    outb(0xFA,0x2e);
    data = inb(0x2f);
    data |= BIT5;
    outb(data,0x2f);

}
}


>
> Actually, just building the wdat_wdt driver into the kernel and it87_wdt as
> module (and loading it via modules.d) should work since the wdat_wdt driver
> would then be instantiated first, and the first watchdog is all that systemd
> cares about.
>
> Thanks,
> Guenter
>

Re: [PATCH 1/4] watchdog: it87_wdt: add blank line after variable declaration

[Guenter Roeck]

On Wed, Dec 13, 2023 at 10:45:22AM +0100, Werner Fischer wrote:
> This patch fixes the following checkpatch.pl warning:
> 
> WARNING: Missing a blank line after declarations
> 
> Signed-off-by: Werner Fischer <devlists@wefi.net>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/it87_wdt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
> index e888b1bdd1f2..6b7f7ec03633 100644
> --- a/drivers/watchdog/it87_wdt.c
> +++ b/drivers/watchdog/it87_wdt.c
> @@ -146,6 +146,7 @@ static inline void superio_outb(int val, int reg)
>  static inline int superio_inw(int reg)
>  {
>  	int val;
> +
>  	outb(reg++, REG);
>  	val = inb(VAL) << 8;
>  	outb(reg, REG);
> -- 
> 2.39.2
>