| Message ID | 20231213094525.11849-4-devlists@wefi.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] watchdog: it87_wdt: add blank line after variable declaration | expand |
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 >
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 >
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
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 >
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
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 >
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
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 >
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
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 >
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) {
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(-)