| Message ID | 20241018154859.2543595-1-james.hilliard1@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] watchdog: it87_wdt: add PWRGD enable quirk for Qotom QCML04 | expand |
On 10/18/24 08:48, 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. > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > Changes v1 -> v2: > - remove QGLK02/IT87_WDT_BROKEN > --- > drivers/watchdog/it87_wdt.c | 44 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c > index 3e8c15138edd..b8be9af065b2 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 The IT8786 documentation I have states that this bit is reserved. Do you have information suggesting otherwise ? > + > /* GPIO Configuration Registers LDN=0x07 */ > #define WDTCTRL 0x71 > #define WDTCFG 0x72 > @@ -240,6 +248,20 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int t) > return ret; > } > > +enum { > + IT87_WDT_OUTPUT_THROUGH_PWRGD = BIT(0), I don't mind starting to use BIT(), but then <linux/bits.h> needs to be included as well. > +}; > + > +static const struct dmi_system_id it8786_quirks[] = { I see that bit 5 of EC register 0xfa _is_ documented for this purpose on at least one other chip supported by this driver, so the flag should be made generic, and not be IT8786 specific. Please name the quirks it87_quirks or similar and check it for all chips. Thanks, Guenter > + { > + /* 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 +283,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 +297,17 @@ 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; > + > switch (chip_type) { > case IT8702_ID: > max_units = 255; > @@ -333,6 +368,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) {
On Sat, Oct 19, 2024 at 8:33 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 10/18/24 08:48, 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. > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > --- > > Changes v1 -> v2: > > - remove QGLK02/IT87_WDT_BROKEN > > --- > > drivers/watchdog/it87_wdt.c | 44 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c > > index 3e8c15138edd..b8be9af065b2 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 > > The IT8786 documentation I have states that this bit is reserved. > Do you have information suggesting otherwise ? Yes, if you clone this repo you'll see the docs in the .rar archive: https://gitcode.com/open-source-toolkit/c602e.git > > > + > > /* GPIO Configuration Registers LDN=0x07 */ > > #define WDTCTRL 0x71 > > #define WDTCFG 0x72 > > @@ -240,6 +248,20 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int t) > > return ret; > > } > > > > +enum { > > + IT87_WDT_OUTPUT_THROUGH_PWRGD = BIT(0), > > I don't mind starting to use BIT(), but then <linux/bits.h> needs to be > included as well. Ok, will add in v3. > > > +}; > > + > > +static const struct dmi_system_id it8786_quirks[] = { > > I see that bit 5 of EC register 0xfa _is_ documented for this purpose on > at least one other chip supported by this driver, so the flag should be made > generic, and not be IT8786 specific. Please name the quirks it87_quirks > or similar and check it for all chips. So, the enum is generic but I wanted to separate out the quirks by chip since from my understanding each board only uses one chip model and thus doing the DMI check for chips with no known boards that need quirks applied would be unnecessary. This also helps to document in the code which chip a specific board uses which I think is potentially useful information to have. If quirks for additional chips end up being needed one can simply add another quirks table for that chip. I'm also trying to be as specific as possible for the DMI match since these Qotom boards only set board names and have no other unique DMI attributes we can match against like vendor names. Maybe I'm being a bit overly paranoid here but I figured it would be best to minimize the probability of a bad match as much as possible. > > Thanks, > Guenter > > > + { > > + /* 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 +283,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 +297,17 @@ 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; > > + > > switch (chip_type) { > > case IT8702_ID: > > max_units = 255; > > @@ -333,6 +368,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) { >
On 10/19/24 12:55, James Hilliard wrote: > On Sat, Oct 19, 2024 at 8:33 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 10/18/24 08:48, 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. >>> >>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >>> --- >>> Changes v1 -> v2: >>> - remove QGLK02/IT87_WDT_BROKEN >>> --- >>> drivers/watchdog/it87_wdt.c | 44 +++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 44 insertions(+) >>> >>> diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c >>> index 3e8c15138edd..b8be9af065b2 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 >> >> The IT8786 documentation I have states that this bit is reserved. >> Do you have information suggesting otherwise ? > > Yes, if you clone this repo you'll see the docs in the .rar archive: > https://gitcode.com/open-source-toolkit/c602e.git > >> >>> + >>> /* GPIO Configuration Registers LDN=0x07 */ >>> #define WDTCTRL 0x71 >>> #define WDTCFG 0x72 >>> @@ -240,6 +248,20 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int t) >>> return ret; >>> } >>> >>> +enum { >>> + IT87_WDT_OUTPUT_THROUGH_PWRGD = BIT(0), >> >> I don't mind starting to use BIT(), but then <linux/bits.h> needs to be >> included as well. > > Ok, will add in v3. > >> >>> +}; >>> + >>> +static const struct dmi_system_id it8786_quirks[] = { >> >> I see that bit 5 of EC register 0xfa _is_ documented for this purpose on >> at least one other chip supported by this driver, so the flag should be made >> generic, and not be IT8786 specific. Please name the quirks it87_quirks >> or similar and check it for all chips. > > So, the enum is generic but I wanted to separate out the quirks by chip > since from my understanding each board only uses one chip model and > thus doing the DMI check for chips with no known boards that need > quirks applied would be unnecessary. This also helps to document in the > code which chip a specific board uses which I think is potentially useful > information to have. If quirks for additional chips end up being needed > one can simply add another quirks table for that chip. > > I'm also trying to be as specific as possible for the DMI match since these > Qotom boards only set board names and have no other unique DMI attributes > we can match against like vendor names. > > Maybe I'm being a bit overly paranoid here but I figured it would be best > to minimize the probability of a bad match as much as possible. > I really think you are, and I really do not want to have to maintain multiple quirks tables, and I do not want to see a case match per chip type. I also really do not care if dmi_first_match() is called for each chip. It is called exactly once. Please let's stick with one table and a single call to dmi_first_match(). That is much simpler and much less error prone. If that ever turns out to be insufficient, we can talk about it then. Thanks, Guenter
diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c index 3e8c15138edd..b8be9af065b2 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,20 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int t) return ret; } +enum { + IT87_WDT_OUTPUT_THROUGH_PWRGD = BIT(0), +}; + +static const struct dmi_system_id it8786_quirks[] = { + { + /* 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 +283,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 +297,17 @@ 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; + switch (chip_type) { case IT8702_ID: max_units = 255; @@ -333,6 +368,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) {
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. Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- Changes v1 -> v2: - remove QGLK02/IT87_WDT_BROKEN --- drivers/watchdog/it87_wdt.c | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)