Re: [PATCH] watchdog: Add driver for Intel OC WDT

From: Diogo Ivo
Date: Tue Mar 11 2025 - 12:49:28 EST

Next message: Dave Kleikamp: "Re: [PATCH] jfs: jfs_xtree: replace XT_GETPAGE macro with xt_getpage() function"
Previous message: Suzuki K Poulose: "Re: [PATCH v2] coresight: Add a KUnit test for coresight_find_default_sink()"
In reply to: Guenter Roeck: "Re: [PATCH] watchdog: Add driver for Intel OC WDT"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH] watchdog: Add driver for Intel OC WDT"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 3/11/25 4:06 PM, Guenter Roeck wrote:

On 3/11/25 07:59, Diogo Ivo wrote:

On 3/11/25 2:10 PM, Guenter Roeck wrote:

On 3/11/25 06:18, Diogo Ivo wrote:

Add a driver for the Intel Over-Clocking Watchdog found in Intel
Platform Controller (PCH) chipsets. This watchdog is controlled
via a simple single-register interface and would otherwise be
standard except for the presence of a LOCK bit that can only be
set once per power cycle, needing extra handling around it.

Signed-off-by: Diogo Ivo <diogo.ivo@xxxxxxxxxxx>
---
drivers/acpi/acpi_pnp.c | 2 +
drivers/watchdog/Kconfig | 11 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/intel_oc_wdt.c | 235 ++++++++++++++++++++++++++++ ++ ++++++++++
4 files changed, 249 insertions(+)

diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
index 01abf26764b00c86f938dea2ed138424f041f880..3f5a1840f573303c71f5d579e32963a5b29d2587 100644
--- a/drivers/acpi/acpi_pnp.c
+++ b/drivers/acpi/acpi_pnp.c
@@ -355,8 +355,10 @@ static bool acpi_pnp_match(const char *idstr, const struct acpi_device_id **matc
* device represented by it.
*/
static const struct acpi_device_id acpi_nonpnp_device_ids[] = {
+ {"INT3F0D"},
{"INTC1080"},
{"INTC1081"},
+ {"INTC1099"},
{""},
};

This needs to be a separate patch.

I will split it for v2.

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f81705f8539aa0b12d156a86aae521aa40b4527d..16e6df441bb344c0f91b40bd76b6322ad3016e72 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1350,6 +1350,17 @@ config INTEL_MID_WATCHDOG
To compile this driver as a module, choose M here.
+config INTEL_OC_WATCHDOG
+ tristate "Intel OC Watchdog"
+ depends on X86 && ACPI
+ select WATCHDOG_CORE
+ help
+ Hardware driver for Intel Over-Clocking watchdog present in
+ Platform Controller Hub (PCH) chipsets.
+
+ To compile this driver as a module, choose M here: the
+ module will be called intel_oc_wdt.
+
config ITCO_WDT
tristate "Intel TCO Timer/Watchdog"
depends on X86 && PCI
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 8411626fa162268e8ccd06349f7193b15a9d281a..3a13f3e80a0f460b99b4f1592fcf17cc6428876b 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -149,6 +149,7 @@ obj-$(CONFIG_W83977F_WDT) += w83977f_wdt.o
obj-$(CONFIG_MACHZ_WDT) += machzwd.o
obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
+obj-$(CONFIG_INTEL_OC_WATCHDOG) += intel_oc_wdt.o
obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
diff --git a/drivers/watchdog/intel_oc_wdt.c b/drivers/watchdog/ intel_oc_wdt.c
new file mode 100644
index 0000000000000000000000000000000000000000..0a2df3440024090f7e342fe7da895a7930ac09b2
--- /dev/null
+++ b/drivers/watchdog/intel_oc_wdt.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel OC Watchdog driver
+ *
+ * Copyright (C) 2025, Siemens SA
+ * Author: Diogo Ivo <diogo.ivo@xxxxxxxxxxx>
+ */
+
+#define DRV_NAME "intel_oc_wdt"
+
+#include <linux/acpi.h>
+#include <linux/bits.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#define INTEL_OC_WDT_TOV GENMASK(9, 0)
+#define INTEL_OC_WDT_MIN_TOV 1
+#define INTEL_OC_WDT_MAX_TOV 1024
+
+/*
+ * One-time writable lock bit. If set forbids
+ * modification of itself, _TOV and _EN until
+ * next reboot.
+ */
+#define INTEL_OC_WDT_CTL_LCK BIT(12)
+
+#define INTEL_OC_WDT_EN BIT(14)
+#define INTEL_OC_WDT_NO_ICCSURV_STS BIT(24)
+#define INTEL_OC_WDT_ICCSURV_STS BIT(25)
+#define INTEL_OC_WDT_RLD BIT(31)
+
+#define INTEL_OC_WDT_STS_BITS (INTEL_OC_WDT_NO_ICCSURV_STS | \
+ INTEL_OC_WDT_ICCSURV_STS)
+
+#define INTEL_OC_WDT_CTRL_REG(wdt) ((wdt)->ctrl_res->start)
+
+struct intel_oc_wdt {
+ struct watchdog_device wdd;
+ struct resource *ctrl_res;
+ bool locked;
+};
+
+#define WDT_HEARTBEAT 60
+static int heartbeat = WDT_HEARTBEAT;

Normally this is set to 0 and the default timeout is initialized together
with min_timeout and max_timeout. This lets the watchdog core override it
with devicetree data (if that is available).

Ok, thank you for the insight. I will address this for v2.
It is unlikely that this driver will have devicetree data but it's
better to follow best practice.

I just submitted a patch to have the watchdog core also look into firmware data,
which would include data provided by ACPI.

Great to know, and all the more reason to apply your suggested change.

+module_param(heartbeat, uint, 0);
+MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds. (default="
+ __MODULE_STRING(WDT_HEARTBEAT) ")");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static int intel_oc_wdt_start(struct watchdog_device *wdd)
+{
+ struct intel_oc_wdt *oc_wdt = watchdog_get_drvdata(wdd);
+
+ if (oc_wdt->locked)
+ return 0;
+
+ outl(inl(INTEL_OC_WDT_CTRL_REG(oc_wdt)) | INTEL_OC_WDT_EN,
+ INTEL_OC_WDT_CTRL_REG(oc_wdt));
+
+ return 0;
+}
+
+static int intel_oc_wdt_stop(struct watchdog_device *wdd)
+{
+ struct intel_oc_wdt *oc_wdt = watchdog_get_drvdata(wdd);
+
+ outl(inl(INTEL_OC_WDT_CTRL_REG(oc_wdt)) & ~INTEL_OC_WDT_EN,
+ INTEL_OC_WDT_CTRL_REG(oc_wdt));
+
+ return 0;
+}
+
+static int intel_oc_wdt_ping(struct watchdog_device *wdd)
+{
+ struct intel_oc_wdt *oc_wdt = watchdog_get_drvdata(wdd);
+
+ outl(inl(INTEL_OC_WDT_CTRL_REG(oc_wdt)) | INTEL_OC_WDT_RLD,
+ INTEL_OC_WDT_CTRL_REG(oc_wdt));
+
+ return 0;
+}
+
+static int intel_oc_wdt_set_timeout(struct watchdog_device *wdd,
+ unsigned int t)
+{
+ struct intel_oc_wdt *oc_wdt = watchdog_get_drvdata(wdd);
+
+ outl((inl(INTEL_OC_WDT_CTRL_REG(oc_wdt)) & ~INTEL_OC_WDT_TOV) | (t - 1),
+ INTEL_OC_WDT_CTRL_REG(oc_wdt));
+
+ wdd->timeout = t;
+
+ return 0;
+}
+
+static const struct watchdog_info intel_oc_wdt_info = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
+ .identity = DRV_NAME,
+};
+
+static const struct watchdog_ops intel_oc_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = intel_oc_wdt_start,
+ .stop = intel_oc_wdt_stop,
+ .ping = intel_oc_wdt_ping,
+ .set_timeout = intel_oc_wdt_set_timeout,
+};
+
+static int intel_oc_wdt_setup(struct intel_oc_wdt *oc_wdt)
+{
+ struct watchdog_info *info;
+ unsigned long val;
+
+ val = inl(INTEL_OC_WDT_CTRL_REG(oc_wdt));
+
+ if (val & INTEL_OC_WDT_STS_BITS)
+ oc_wdt->wdd.bootstatus |= WDIOF_CARDRESET;
+
+ oc_wdt->locked = !!(val & INTEL_OC_WDT_CTL_LCK);
+
+ if (val & INTEL_OC_WDT_EN) {
+ /*
+ * No need to issue a ping here to "commit" the new timeout
+ * value to hardware as the watchdog core schedules one
+ * immediately when registering the watchdog.
+ */
+ set_bit(WDOG_HW_RUNNING, &oc_wdt->wdd.status);
+
+ if (oc_wdt->locked) {
+ info = (struct watchdog_info *)&intel_oc_wdt_info;
+ /*
+ * Set nowayout unconditionally as we cannot stop
+ * the watchdog and read the current timeout.
+ */

But the timeout is read below ? Do you mean "change the current timeout" ?

In this case where the BIOS both enabled the watchdog and set the LOCK
bit we cannot change the timeout anymore, meaning that we have to read
the value currently in the register and pass it to the watchdog core,
which is what is done below.

Yes, but the comment says " we cannot stop the watchdog and _read_ the
current timeout" (emphasis added), suggesting that the current timeout
can not be _read_ if locked is true.

+ nowayout = true;
+
+ oc_wdt->wdd.timeout = (val & INTEL_OC_WDT_TOV) + 1;

However, this code does read the timeout even if locked is set. That
suggests that it is not possible to _change_ the timeout if locked is set,
but that it is possible to read the current timeout.

Sorry for the confusing wording. Yes, according to the documentation [1]
this is exactly the case, we cannot change the timeout but it is possible
to read it. I will word this better in v2 to avoid confusion.

Best regards,
Diogo

[1]: https://edc.intel.com/content/www/us/en/design/publications/14th-generation-core-processors-ioe-p-registers/over-clocking-wdt-control-oc-wdt-ctl-offset-54/

Next message: Dave Kleikamp: "Re: [PATCH] jfs: jfs_xtree: replace XT_GETPAGE macro with xt_getpage() function"
Previous message: Suzuki K Poulose: "Re: [PATCH v2] coresight: Add a KUnit test for coresight_find_default_sink()"
In reply to: Guenter Roeck: "Re: [PATCH] watchdog: Add driver for Intel OC WDT"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH] watchdog: Add driver for Intel OC WDT"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]