* [PATCH] watchdog: it87_wdt: add quirks for some Qotom IT8786 boards
@ 2024-10-18 3:09 James Hilliard
2024-10-18 3:59 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: James Hilliard @ 2024-10-18 3:09 UTC (permalink / raw)
To: linux-watchdog
Cc: James Hilliard, Wim Van Sebroeck, Guenter Roeck, linux-kernel
For the watchdog timer to work properly on the QCML04 board we need to
set PWRGD enable in the Environment Controller Configuration Registers
Special Configuration Register 1 when it is not already set, this may
be the case when the watchdog is not enabled from within the BIOS.
For the Qotom QGLK02 board the vendor indicates that the IT8786
watchdog hardware is not functional due to a conflict with the BIOS
power-on function, with PWRGD set the watchdog will trigger but the
board will poweroff rather than restart as expected. Disable the
it87 driver on this broken hardware.
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/watchdog/it87_wdt.c | 55 +++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
index 3e8c15138edd..dec501c21fd3 100644
--- a/drivers/watchdog/it87_wdt.c
+++ b/drivers/watchdog/it87_wdt.c
@@ -20,6 +20,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/dmi.h>
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
@@ -40,6 +41,7 @@
#define VAL 0x2f
/* Logical device Numbers LDN */
+#define EC 0x04
#define GPIO 0x07
/* Configuration Registers and Functions */
@@ -73,6 +75,12 @@
#define IT8784_ID 0x8784
#define IT8786_ID 0x8786
+/* Environment Controller Configuration Registers LDN=0x04 */
+#define SCR1 0xfa
+
+/* Environment Controller Bits SCR1 */
+#define WDT_PWRGD 0x20
+
/* GPIO Configuration Registers LDN=0x07 */
#define WDTCTRL 0x71
#define WDTCFG 0x72
@@ -240,6 +248,28 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int t)
return ret;
}
+enum {
+ IT87_WDT_BROKEN = BIT(0),
+ IT87_WDT_OUTPUT_THROUGH_PWRGD = BIT(1),
+};
+
+static const struct dmi_system_id it8786_quirks[] = {
+ {
+ /* Qotom Q730P/Q8XXG2-P */
+ .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "QGLK02"),
+ },
+ .driver_data = (void *)IT87_WDT_BROKEN,
+ },
+ {
+ /* Qotom Q30900P */
+ .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "QCML04"),
+ },
+ .driver_data = (void *)IT87_WDT_OUTPUT_THROUGH_PWRGD,
+ }
+};
+
static const struct watchdog_info ident = {
.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
.firmware_version = 1,
@@ -261,8 +291,10 @@ static struct watchdog_device wdt_dev = {
static int __init it87_wdt_init(void)
{
+ const struct dmi_system_id *dmi_id;
u8 chip_rev;
u8 ctrl;
+ int quirks = 0;
int rc;
rc = superio_enter();
@@ -273,6 +305,20 @@ static int __init it87_wdt_init(void)
chip_rev = superio_inb(CHIPREV) & 0x0f;
superio_exit();
+ switch (chip_type) {
+ case IT8786_ID:
+ dmi_id = dmi_first_match(it8786_quirks);
+ break;
+ default:
+ dmi_id = NULL;
+ }
+
+ if (dmi_id)
+ quirks = (long)dmi_id->driver_data;
+
+ if (quirks & IT87_WDT_BROKEN)
+ return -ENODEV;
+
switch (chip_type) {
case IT8702_ID:
max_units = 255;
@@ -333,6 +379,15 @@ static int __init it87_wdt_init(void)
superio_outb(0x00, WDTCTRL);
}
+ if (quirks & IT87_WDT_OUTPUT_THROUGH_PWRGD) {
+ superio_select(EC);
+ ctrl = superio_inb(SCR1);
+ if (!(ctrl & WDT_PWRGD)) {
+ ctrl |= WDT_PWRGD;
+ superio_outb(ctrl, SCR1);
+ }
+ }
+
superio_exit();
if (timeout < 1 || timeout > max_units * 60) {
--
2.34.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: it87_wdt: add quirks for some Qotom IT8786 boards
2024-10-18 3:09 [PATCH] watchdog: it87_wdt: add quirks for some Qotom IT8786 boards James Hilliard
@ 2024-10-18 3:59 ` Guenter Roeck
2024-10-18 4:29 ` James Hilliard
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2024-10-18 3:59 UTC (permalink / raw)
To: James Hilliard, linux-watchdog; +Cc: Wim Van Sebroeck, linux-kernel
On 10/17/24 20:09, James Hilliard wrote:
> For the watchdog timer to work properly on the QCML04 board we need to
> set PWRGD enable in the Environment Controller Configuration Registers
> Special Configuration Register 1 when it is not already set, this may
> be the case when the watchdog is not enabled from within the BIOS.
>
> For the Qotom QGLK02 board the vendor indicates that the IT8786
> watchdog hardware is not functional due to a conflict with the BIOS
> power-on function, with PWRGD set the watchdog will trigger but the
> board will poweroff rather than restart as expected. Disable the
> it87 driver on this broken hardware.
>
This shouldn't be done in drivers, and it doesn't scale. The driver needs
to be disabled with the mechanism supported by the distribution, for example
in /etc/modprobe.d/blacklist-watchdog.conf, or by whatever other mechanism
the distribution supports for that purpose.
Guenter
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
> drivers/watchdog/it87_wdt.c | 55 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
> index 3e8c15138edd..dec501c21fd3 100644
> --- a/drivers/watchdog/it87_wdt.c
> +++ b/drivers/watchdog/it87_wdt.c
> @@ -20,6 +20,7 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> +#include <linux/dmi.h>
> #include <linux/init.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> @@ -40,6 +41,7 @@
> #define VAL 0x2f
>
> /* Logical device Numbers LDN */
> +#define EC 0x04
> #define GPIO 0x07
>
> /* Configuration Registers and Functions */
> @@ -73,6 +75,12 @@
> #define IT8784_ID 0x8784
> #define IT8786_ID 0x8786
>
> +/* Environment Controller Configuration Registers LDN=0x04 */
> +#define SCR1 0xfa
> +
> +/* Environment Controller Bits SCR1 */
> +#define WDT_PWRGD 0x20
> +
> /* GPIO Configuration Registers LDN=0x07 */
> #define WDTCTRL 0x71
> #define WDTCFG 0x72
> @@ -240,6 +248,28 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int t)
> return ret;
> }
>
> +enum {
> + IT87_WDT_BROKEN = BIT(0),
> + IT87_WDT_OUTPUT_THROUGH_PWRGD = BIT(1),
> +};
> +
> +static const struct dmi_system_id it8786_quirks[] = {
> + {
> + /* Qotom Q730P/Q8XXG2-P */
> + .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "QGLK02"),
> + },
> + .driver_data = (void *)IT87_WDT_BROKEN,
> + },
> + {
> + /* Qotom Q30900P */
> + .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "QCML04"),
> + },
> + .driver_data = (void *)IT87_WDT_OUTPUT_THROUGH_PWRGD,
> + }
> +};
> +
> static const struct watchdog_info ident = {
> .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
> .firmware_version = 1,
> @@ -261,8 +291,10 @@ static struct watchdog_device wdt_dev = {
>
> static int __init it87_wdt_init(void)
> {
> + const struct dmi_system_id *dmi_id;
> u8 chip_rev;
> u8 ctrl;
> + int quirks = 0;
> int rc;
>
> rc = superio_enter();
> @@ -273,6 +305,20 @@ static int __init it87_wdt_init(void)
> chip_rev = superio_inb(CHIPREV) & 0x0f;
> superio_exit();
>
> + switch (chip_type) {
> + case IT8786_ID:
> + dmi_id = dmi_first_match(it8786_quirks);
> + break;
> + default:
> + dmi_id = NULL;
> + }
> +
> + if (dmi_id)
> + quirks = (long)dmi_id->driver_data;
> +
> + if (quirks & IT87_WDT_BROKEN)
> + return -ENODEV;
> +
> switch (chip_type) {
> case IT8702_ID:
> max_units = 255;
> @@ -333,6 +379,15 @@ static int __init it87_wdt_init(void)
> superio_outb(0x00, WDTCTRL);
> }
>
> + if (quirks & IT87_WDT_OUTPUT_THROUGH_PWRGD) {
> + superio_select(EC);
> + ctrl = superio_inb(SCR1);
> + if (!(ctrl & WDT_PWRGD)) {
> + ctrl |= WDT_PWRGD;
> + superio_outb(ctrl, SCR1);
> + }
> + }
> +
> superio_exit();
>
> if (timeout < 1 || timeout > max_units * 60) {
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: it87_wdt: add quirks for some Qotom IT8786 boards
2024-10-18 3:59 ` Guenter Roeck
@ 2024-10-18 4:29 ` James Hilliard
2024-10-18 10:09 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: James Hilliard @ 2024-10-18 4:29 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-watchdog, Wim Van Sebroeck, linux-kernel
On Thu, Oct 17, 2024 at 9:59 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/17/24 20:09, James Hilliard wrote:
> > For the watchdog timer to work properly on the QCML04 board we need to
> > set PWRGD enable in the Environment Controller Configuration Registers
> > Special Configuration Register 1 when it is not already set, this may
> > be the case when the watchdog is not enabled from within the BIOS.
> >
> > For the Qotom QGLK02 board the vendor indicates that the IT8786
> > watchdog hardware is not functional due to a conflict with the BIOS
> > power-on function, with PWRGD set the watchdog will trigger but the
> > board will poweroff rather than restart as expected. Disable the
> > it87 driver on this broken hardware.
> >
>
> This shouldn't be done in drivers, and it doesn't scale. The driver needs
> to be disabled with the mechanism supported by the distribution, for example
> in /etc/modprobe.d/blacklist-watchdog.conf, or by whatever other mechanism
> the distribution supports for that purpose.
There isn't really a good way that I've found with my setup since I use common
images for both of these boards. I'm also worried that it's much easier to mess
something critical like this up if user space is involved in hardware detection.
Many other watchdog drivers do this sort of thing so I'm a bit confused why we
would want to not do that here as well, for example:
https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/renesas_wdt.c#L176-L207
https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/ebc-c384_wdt.c#L125
https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/lenovo_se10_wdt.c#L242-L285
https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/sbc_fitpc2_wdt.c#L203-L206
>
> Guenter
>
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> > drivers/watchdog/it87_wdt.c | 55 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 55 insertions(+)
> >
> > diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
> > index 3e8c15138edd..dec501c21fd3 100644
> > --- a/drivers/watchdog/it87_wdt.c
> > +++ b/drivers/watchdog/it87_wdt.c
> > @@ -20,6 +20,7 @@
> >
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > +#include <linux/dmi.h>
> > #include <linux/init.h>
> > #include <linux/io.h>
> > #include <linux/kernel.h>
> > @@ -40,6 +41,7 @@
> > #define VAL 0x2f
> >
> > /* Logical device Numbers LDN */
> > +#define EC 0x04
> > #define GPIO 0x07
> >
> > /* Configuration Registers and Functions */
> > @@ -73,6 +75,12 @@
> > #define IT8784_ID 0x8784
> > #define IT8786_ID 0x8786
> >
> > +/* Environment Controller Configuration Registers LDN=0x04 */
> > +#define SCR1 0xfa
> > +
> > +/* Environment Controller Bits SCR1 */
> > +#define WDT_PWRGD 0x20
> > +
> > /* GPIO Configuration Registers LDN=0x07 */
> > #define WDTCTRL 0x71
> > #define WDTCFG 0x72
> > @@ -240,6 +248,28 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int t)
> > return ret;
> > }
> >
> > +enum {
> > + IT87_WDT_BROKEN = BIT(0),
> > + IT87_WDT_OUTPUT_THROUGH_PWRGD = BIT(1),
> > +};
> > +
> > +static const struct dmi_system_id it8786_quirks[] = {
> > + {
> > + /* Qotom Q730P/Q8XXG2-P */
> > + .matches = {
> > + DMI_EXACT_MATCH(DMI_BOARD_NAME, "QGLK02"),
> > + },
> > + .driver_data = (void *)IT87_WDT_BROKEN,
> > + },
> > + {
> > + /* Qotom Q30900P */
> > + .matches = {
> > + DMI_EXACT_MATCH(DMI_BOARD_NAME, "QCML04"),
> > + },
> > + .driver_data = (void *)IT87_WDT_OUTPUT_THROUGH_PWRGD,
> > + }
> > +};
> > +
> > static const struct watchdog_info ident = {
> > .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
> > .firmware_version = 1,
> > @@ -261,8 +291,10 @@ static struct watchdog_device wdt_dev = {
> >
> > static int __init it87_wdt_init(void)
> > {
> > + const struct dmi_system_id *dmi_id;
> > u8 chip_rev;
> > u8 ctrl;
> > + int quirks = 0;
> > int rc;
> >
> > rc = superio_enter();
> > @@ -273,6 +305,20 @@ static int __init it87_wdt_init(void)
> > chip_rev = superio_inb(CHIPREV) & 0x0f;
> > superio_exit();
> >
> > + switch (chip_type) {
> > + case IT8786_ID:
> > + dmi_id = dmi_first_match(it8786_quirks);
> > + break;
> > + default:
> > + dmi_id = NULL;
> > + }
> > +
> > + if (dmi_id)
> > + quirks = (long)dmi_id->driver_data;
> > +
> > + if (quirks & IT87_WDT_BROKEN)
> > + return -ENODEV;
> > +
> > switch (chip_type) {
> > case IT8702_ID:
> > max_units = 255;
> > @@ -333,6 +379,15 @@ static int __init it87_wdt_init(void)
> > superio_outb(0x00, WDTCTRL);
> > }
> >
> > + if (quirks & IT87_WDT_OUTPUT_THROUGH_PWRGD) {
> > + superio_select(EC);
> > + ctrl = superio_inb(SCR1);
> > + if (!(ctrl & WDT_PWRGD)) {
> > + ctrl |= WDT_PWRGD;
> > + superio_outb(ctrl, SCR1);
> > + }
> > + }
> > +
> > superio_exit();
> >
> > if (timeout < 1 || timeout > max_units * 60) {
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: it87_wdt: add quirks for some Qotom IT8786 boards
2024-10-18 4:29 ` James Hilliard
@ 2024-10-18 10:09 ` Guenter Roeck
2024-10-18 15:18 ` James Hilliard
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2024-10-18 10:09 UTC (permalink / raw)
To: James Hilliard; +Cc: linux-watchdog, Wim Van Sebroeck, linux-kernel
On 10/17/24 21:29, James Hilliard wrote:
> On Thu, Oct 17, 2024 at 9:59 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 10/17/24 20:09, James Hilliard wrote:
>>> For the watchdog timer to work properly on the QCML04 board we need to
>>> set PWRGD enable in the Environment Controller Configuration Registers
>>> Special Configuration Register 1 when it is not already set, this may
>>> be the case when the watchdog is not enabled from within the BIOS.
>>>
>>> For the Qotom QGLK02 board the vendor indicates that the IT8786
>>> watchdog hardware is not functional due to a conflict with the BIOS
>>> power-on function, with PWRGD set the watchdog will trigger but the
>>> board will poweroff rather than restart as expected. Disable the
>>> it87 driver on this broken hardware.
>>>
>>
>> This shouldn't be done in drivers, and it doesn't scale. The driver needs
>> to be disabled with the mechanism supported by the distribution, for example
>> in /etc/modprobe.d/blacklist-watchdog.conf, or by whatever other mechanism
>> the distribution supports for that purpose.
>
> There isn't really a good way that I've found with my setup since I use common
> images for both of these boards. I'm also worried that it's much easier to mess
> something critical like this up if user space is involved in hardware detection.
>
> Many other watchdog drivers do this sort of thing so I'm a bit confused why we
> would want to not do that here as well, for example:
> https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/renesas_wdt.c#L176-L207
> https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/ebc-c384_wdt.c#L125
> https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/lenovo_se10_wdt.c#L242-L285
> https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/sbc_fitpc2_wdt.c#L203-L206
>
Those are specialty watchdog drivers, which only work on a very limited number of boards
to start with. For the most part they use DMI data to determine if the watchdog is supported
on a board, not to determine if a watchdog isn't supported.
The it87 driver works on thousands of boards, and is not wired up on a substantial percentage
of them. In many cases, systems with ITE Super-IO chips have two Super-IO chips installed
(one of them typically being an IT8786), and only one of those (or none) will have the watchdog
wired up. Many boards with Intel CPUs use the iTCO watchdog and don't have the Super-IO
watchdog wired up at all. Trying to maintain a deny-list for all boards where the watchdog
isn't wired up would not scale.
Guenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: it87_wdt: add quirks for some Qotom IT8786 boards
2024-10-18 10:09 ` Guenter Roeck
@ 2024-10-18 15:18 ` James Hilliard
0 siblings, 0 replies; 5+ messages in thread
From: James Hilliard @ 2024-10-18 15:18 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-watchdog, Wim Van Sebroeck, linux-kernel
On Fri, Oct 18, 2024 at 4:09 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/17/24 21:29, James Hilliard wrote:
> > On Thu, Oct 17, 2024 at 9:59 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 10/17/24 20:09, James Hilliard wrote:
> >>> For the watchdog timer to work properly on the QCML04 board we need to
> >>> set PWRGD enable in the Environment Controller Configuration Registers
> >>> Special Configuration Register 1 when it is not already set, this may
> >>> be the case when the watchdog is not enabled from within the BIOS.
> >>>
> >>> For the Qotom QGLK02 board the vendor indicates that the IT8786
> >>> watchdog hardware is not functional due to a conflict with the BIOS
> >>> power-on function, with PWRGD set the watchdog will trigger but the
> >>> board will poweroff rather than restart as expected. Disable the
> >>> it87 driver on this broken hardware.
> >>>
> >>
> >> This shouldn't be done in drivers, and it doesn't scale. The driver needs
> >> to be disabled with the mechanism supported by the distribution, for example
> >> in /etc/modprobe.d/blacklist-watchdog.conf, or by whatever other mechanism
> >> the distribution supports for that purpose.
> >
> > There isn't really a good way that I've found with my setup since I use common
> > images for both of these boards. I'm also worried that it's much easier to mess
> > something critical like this up if user space is involved in hardware detection.
> >
> > Many other watchdog drivers do this sort of thing so I'm a bit confused why we
> > would want to not do that here as well, for example:
> > https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/renesas_wdt.c#L176-L207
> > https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/ebc-c384_wdt.c#L125
> > https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/lenovo_se10_wdt.c#L242-L285
> > https://github.com/torvalds/linux/blob/v6.11/drivers/watchdog/sbc_fitpc2_wdt.c#L203-L206
> >
>
> Those are specialty watchdog drivers, which only work on a very limited number of boards
> to start with. For the most part they use DMI data to determine if the watchdog is supported
> on a board, not to determine if a watchdog isn't supported.
>
> The it87 driver works on thousands of boards, and is not wired up on a substantial percentage
> of them. In many cases, systems with ITE Super-IO chips have two Super-IO chips installed
> (one of them typically being an IT8786), and only one of those (or none) will have the watchdog
> wired up. Many boards with Intel CPUs use the iTCO watchdog and don't have the Super-IO
> watchdog wired up at all. Trying to maintain a deny-list for all boards where the watchdog
> isn't wired up would not scale.
Hmm, so what would scale then? I mean obviously having every user manually
configure watchdog drivers scales even worse than trying to maintain a deny-list
as users are generally going to expect drivers to work properly without manual
configuration.
Maybe something like hid-quirks would work better here for matching the
correct watchdog driver on systems where multiple watchdog drivers
otherwise detect a watchdog as being present?:
https://github.com/torvalds/linux/blob/v6.11/drivers/hid/hid-quirks.c
>
> Guenter
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-10-18 15:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-18 3:09 [PATCH] watchdog: it87_wdt: add quirks for some Qotom IT8786 boards James Hilliard
2024-10-18 3:59 ` Guenter Roeck
2024-10-18 4:29 ` James Hilliard
2024-10-18 10:09 ` Guenter Roeck
2024-10-18 15:18 ` James Hilliard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®