[PATCH 0/4] ACPI: Add support for WDAT (Watchdog Action Table)

[PATCH 0/4] ACPI: Add support for WDAT (Watchdog Action Table)

[Mika Westerberg]

Hi all,

The WDAT (Watchdog Action Table) is a special ACPI table introduced by
Microsoft [1] that abstracts the watchdog hardware from the OS. Windows
uses this table for its watchdog implementation instead of a native iTCO
driver.

Microsoft re-licensed the WDAT specification to be under Microsoft
Community Promise license [2] so it should be fine to use it in Linux.

This series brings WDAT table support to Linux.

When the driver is enabled and we find out that there is a WDAT table, the
driver will take over the native iTCO watchdog driver. Main advantage in
this is that we do not need to change the native iTCO driver whenever the
hardware changes. For example in Skylake iTCO moved to sit behind SMBus and
the NO_REBOOT bit was hidden behind P2SB (Primary to Sideband). In addition
we can expect this to be tested much better by OEMs who typically validate
that Windows works fine on their hardware/firmware.

Patch [1/4] adds ACPI enumeration support and the driver itself. It also
introduces acpi_has_watchdog() which can be used to check if we should use
ACPI watchdog or native one.

Patches [2-4/4] prevent creation of the native iTCO platform device if we
detect that the ACPI watchdog (WDAT) should be used instead.

[1] http://msdn.microsoft.com/en-us/windows/hardware/gg463320.aspx
[2] https://msdn.microsoft.com/en-us/openspecifications/dn646766.aspx

Mika Westerberg (4):
  ACPI / watchdog: Add support for WDAT hardware watchdog
  mfd: lpc_ich: Do not create iTCO watchdog when WDAT table exists
  i2c: i801: Do not create iTCO watchdog when WDAT table exists
  platform/x86: intel_pmc_ipc: Do not create iTCO watchdog when WDAT table exists

 drivers/acpi/Kconfig                 |  16 ++
 drivers/acpi/Makefile                |   2 +
 drivers/acpi/acpi_watchdog.c         | 123 ++++++++
 drivers/acpi/internal.h              |  10 +
 drivers/acpi/scan.c                  |   1 +
 drivers/acpi/wdat_wdt.c              | 524 +++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-i801.c        |   4 +-
 drivers/mfd/lpc_ich.c                |   4 +
 drivers/platform/x86/intel_pmc_ipc.c |  12 +-
 include/linux/acpi.h                 |   6 +
 10 files changed, 697 insertions(+), 5 deletions(-)
 create mode 100644 drivers/acpi/acpi_watchdog.c
 create mode 100644 drivers/acpi/wdat_wdt.c

-- 
2.9.3

[PATCH 1/4] ACPI / watchdog: Add support for WDAT hardware watchdog

[Mika Westerberg]

Starting from Intel Skylake the iTCO watchdog timer registers were moved to
reside in the same register space with SMBus host controller.  Not all
needed registers are available though and we need to unhide P2SB (Primary
to Sideband) device briefly to be able to read status of required NO_REBOOT
bit. The i2c-i801.c SMBus driver used to handle this and creation of the
iTCO watchdog platform device.

Windows, on the other hand, does not use the iTCO watchdog hardware
directly even if it is available. Instead it relies on ACPI Watchdog Action
Table (WDAT) table to describe the watchdog hardware to the OS. This table
contains necessary information about the the hardware and also set of
actions which are executed by a driver as needed.

This patch implements a new watchdog driver that takes advantage of the
ACPI WDAT table. We split the functionality into two parts: first part
enumerates the WDAT table and if found, populates resources and creates
platform device for the actual driver. The second part is the driver
itself.

The reason for the split is that this way we can make the driver itself to
be a module and loaded automatically if the WDAT table is found. Otherwise
the module is not loaded.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/Kconfig         |  16 ++
 drivers/acpi/Makefile        |   2 +
 drivers/acpi/acpi_watchdog.c | 123 ++++++++++
 drivers/acpi/internal.h      |  10 +
 drivers/acpi/scan.c          |   1 +
 drivers/acpi/wdat_wdt.c      | 524 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h         |   6 +
 7 files changed, 682 insertions(+)
 create mode 100644 drivers/acpi/acpi_watchdog.c
 create mode 100644 drivers/acpi/wdat_wdt.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 445ce28475b3..f6dc7885c35f 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -462,6 +462,22 @@ source "drivers/acpi/nfit/Kconfig"
 source "drivers/acpi/apei/Kconfig"
 source "drivers/acpi/dptf/Kconfig"
 
+config ACPI_WATCHDOG
+	bool
+
+config ACPI_WDAT_WDT
+	tristate "ACPI Watchdog Action Table (WDAT)"
+	depends on WATCHDOG_CORE
+	select ACPI_WATCHDOG
+	help
+	  This driver adds support for systems with ACPI Watchdog Action
+	  Table (WDAT) table. Servers typically have this but it can be
+	  found on some desktop machines as well. This driver will take
+	  over the native iTCO watchdog driver found on many Intel CPUs.
+
+	  To compile this driver as module, choose M here: the module will
+	  be called wdat_wdt.
+
 config ACPI_EXTLOG
 	tristate "Extended Error Log support"
 	depends on X86_MCE && X86_LOCAL_APIC
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 5ae9d85c5159..b9568a954090 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -56,6 +56,7 @@ acpi-$(CONFIG_ACPI_NUMA)	+= numa.o
 acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
 acpi-y				+= acpi_lpat.o
 acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
+acpi-$(CONFIG_ACPI_WATCHDOG)	+= acpi_watchdog.o
 
 # These are (potentially) separate modules
 
@@ -82,6 +83,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
 obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
 obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
+obj-$(CONFIG_ACPI_WDAT_WDT)	+= wdat_wdt.o
 
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o
diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
new file mode 100644
index 000000000000..13caebd679f5
--- /dev/null
+++ b/drivers/acpi/acpi_watchdog.c
@@ -0,0 +1,123 @@
+/*
+ * ACPI watchdog table parsing support.
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "ACPI: watchdog: " fmt
+
+#include <linux/acpi.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+
+#include "internal.h"
+
+/**
+ * Returns true if this system should prefer ACPI based watchdog instead of
+ * the native one (which are typically the same hardware).
+ */
+bool acpi_has_watchdog(void)
+{
+	struct acpi_table_header hdr;
+
+	if (acpi_disabled)
+		return false;
+
+	return ACPI_SUCCESS(acpi_get_table_header(ACPI_SIG_WDAT, 0, &hdr));
+}
+EXPORT_SYMBOL_GPL(acpi_has_watchdog);
+
+void __init acpi_watchdog_init(void)
+{
+	const struct acpi_wdat_entry *entries;
+	const struct acpi_table_wdat *wdat;
+	struct list_head resource_list;
+	struct resource_entry *rentry;
+	struct platform_device *pdev;
+	struct resource *resources;
+	size_t nresources = 0;
+	acpi_status status;
+	int i;
+
+	status = acpi_get_table(ACPI_SIG_WDAT, 0,
+				(struct acpi_table_header **)&wdat);
+	if (ACPI_FAILURE(status)) {
+		/* It is fine if there is no WDAT */
+		return;
+	}
+
+	/* Watchdog disabled by BIOS */
+	if (!(wdat->flags & ACPI_WDAT_ENABLED))
+		return;
+
+	/* Skip legacy PCI WDT devices */
+	if (wdat->pci_segment != 0xff || wdat->pci_bus != 0xff ||
+	    wdat->pci_device != 0xff || wdat->pci_function != 0xff)
+		return;
+
+	INIT_LIST_HEAD(&resource_list);
+
+	entries = (struct acpi_wdat_entry *)(wdat + 1);
+	for (i = 0; i < wdat->entries; i++) {
+		const struct acpi_generic_address *gas;
+		struct resource_entry *rentry;
+		struct resource res;
+		bool found;
+
+		gas = &entries[i].register_region;
+
+		res.start = gas->address;
+		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+			res.flags = IORESOURCE_MEM;
+			res.end = res.start + ALIGN(gas->access_width, 4);
+		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
+			res.flags = IORESOURCE_IO;
+			res.end = res.start + gas->access_width;
+		} else {
+			pr_warn("Unsupported address space: %u\n",
+				gas->space_id);
+			goto fail_free_resource_list;
+		}
+
+		found = false;
+		resource_list_for_each_entry(rentry, &resource_list) {
+			if (resource_contains(rentry->res, &res)) {
+				found = true;
+				break;
+			}
+		}
+
+		if (!found) {
+			rentry = resource_list_create_entry(NULL, 0);
+			if (!rentry)
+				goto fail_free_resource_list;
+
+			*rentry->res = res;
+			resource_list_add_tail(rentry, &resource_list);
+			nresources++;
+		}
+	}
+
+	resources = kcalloc(nresources, sizeof(*resources), GFP_KERNEL);
+	if (!resources)
+		goto fail_free_resource_list;
+
+	i = 0;
+	resource_list_for_each_entry(rentry, &resource_list)
+		resources[i++] = *rentry->res;
+
+	pdev = platform_device_register_simple("wdat_wdt", PLATFORM_DEVID_NONE,
+					       resources, nresources);
+	if (IS_ERR(pdev))
+		pr_err("Failed to create platform device\n");
+
+	kfree(resources);
+
+fail_free_resource_list:
+	resource_list_free(&resource_list);
+}
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 940218ff0193..fb9a7ad96756 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -225,4 +225,14 @@ static inline void suspend_nvs_restore(void) {}
 void acpi_init_properties(struct acpi_device *adev);
 void acpi_free_properties(struct acpi_device *adev);
 
+/*--------------------------------------------------------------------------
+				Watchdog
+  -------------------------------------------------------------------------- */
+
+#ifdef CONFIG_ACPI_WATCHDOG
+void acpi_watchdog_init(void);
+#else
+static inline void acpi_watchdog_init(void) {}
+#endif
+
 #endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index e878fc799af7..3b85d87021da 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2002,6 +2002,7 @@ int __init acpi_scan_init(void)
 	acpi_pnp_init();
 	acpi_int340x_thermal_init();
 	acpi_amba_init();
+	acpi_watchdog_init();
 
 	acpi_scan_add_handler(&generic_device_handler);
 
diff --git a/drivers/acpi/wdat_wdt.c b/drivers/acpi/wdat_wdt.c
new file mode 100644
index 000000000000..a541753a3fc1
--- /dev/null
+++ b/drivers/acpi/wdat_wdt.c
@@ -0,0 +1,524 @@
+/*
+ * ACPI Hardware Watchdog (WDAT) driver.
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/acpi.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/watchdog.h>
+
+#define MAX_WDAT_ACTIONS ACPI_WDAT_ACTION_RESERVED
+
+/**
+ * struct wdat_instruction - Single ACPI WDAT instruction
+ * @entry: Copy of the ACPI table instruction
+ * @reg: Register the instruction is accessing
+ * @node: Next instruction in action sequence
+ */
+struct wdat_instruction {
+	struct acpi_wdat_entry entry;
+	void __iomem *reg;
+	struct list_head node;
+};
+
+/**
+ * struct wdat_wdt - ACPI WDAT watchdog device
+ * @pdev: Parent platform device
+ * @wdd: Watchdog core device
+ * @period: How long is one watchdog period in ms
+ * @stopped_in_sleep: Is this watchdog stopped by the firmware in S1-S5
+ * @stopped: Was the watchdog stopped by the driver in suspend
+ * @actions: An array of instruction lists indexed by an action number from
+ *           the WDAT table. There can be %NULL entries for not implemented
+ *           actions.
+ */
+struct wdat_wdt {
+	struct platform_device *pdev;
+	struct watchdog_device wdd;
+	unsigned int period;
+	bool stopped_in_sleep;
+	bool stopped;
+	struct list_head *instructions[MAX_WDAT_ACTIONS];
+};
+
+#define to_wdat_wdt(wdd) container_of(wdd, struct wdat_wdt, wdd)
+
+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 wdat_wdt_read(struct wdat_wdt *wdat,
+	 const struct wdat_instruction *instr, u32 *value)
+{
+	const struct acpi_generic_address *gas = &instr->entry.register_region;
+
+	switch (gas->access_width) {
+	case 1:
+		*value = ioread8(instr->reg);
+		break;
+	case 2:
+		*value = ioread16(instr->reg);
+		break;
+	case 3:
+		*value = ioread32(instr->reg);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	dev_dbg(&wdat->pdev->dev, "Read %#x from 0x%08llx\n", *value,
+		gas->address);
+
+	return 0;
+}
+
+static int wdat_wdt_write(struct wdat_wdt *wdat,
+	const struct wdat_instruction *instr, u32 value)
+{
+	const struct acpi_generic_address *gas = &instr->entry.register_region;
+
+	switch (gas->access_width) {
+	case 1:
+		iowrite8((u8)value, instr->reg);
+		break;
+	case 2:
+		iowrite16((u16)value, instr->reg);
+		break;
+	case 3:
+		iowrite32(value, instr->reg);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	dev_dbg(&wdat->pdev->dev, "Wrote %#x to 0x%08llx\n", value,
+		gas->address);
+
+	return 0;
+}
+
+static int wdat_wdt_run_action(struct wdat_wdt *wdat, unsigned int action,
+			       u32 param, u32 *retval)
+{
+	struct wdat_instruction *instr;
+
+	if (action >= ARRAY_SIZE(wdat->instructions))
+		return -EINVAL;
+
+	if (!wdat->instructions[action])
+		return -EOPNOTSUPP;
+
+	dev_dbg(&wdat->pdev->dev, "Running action %#x\n", action);
+
+	/* Run each instruction sequentially */
+	list_for_each_entry(instr, wdat->instructions[action], node) {
+		const struct acpi_wdat_entry *entry = &instr->entry;
+		const struct acpi_generic_address *gas;
+		u32 flags, value, mask, x, y;
+		bool preserve;
+		int ret;
+
+		gas = &entry->register_region;
+
+		preserve = entry->instruction & ACPI_WDAT_PRESERVE_REGISTER;
+		flags = entry->instruction & ~ACPI_WDAT_PRESERVE_REGISTER;
+		value = entry->value;
+		mask = entry->mask;
+
+		switch (flags) {
+		case ACPI_WDAT_READ_VALUE:
+			ret = wdat_wdt_read(wdat, instr, &x);
+			if (ret)
+				return ret;
+			x >>= gas->bit_offset;
+			x &= mask;
+			if (retval)
+				*retval = x == value;
+			break;
+
+		case ACPI_WDAT_READ_COUNTDOWN:
+			ret = wdat_wdt_read(wdat, instr, &x);
+			if (ret)
+				return ret;
+			x >>= gas->bit_offset;
+			x &= mask;
+			if (retval)
+				*retval = x;
+			break;
+
+		case ACPI_WDAT_WRITE_VALUE:
+			x = value & mask;
+			x <<= gas->bit_offset;
+			if (preserve) {
+				ret = wdat_wdt_read(wdat, instr, &y);
+				if (ret)
+					return ret;
+				y = y & ~(mask << gas->bit_offset);
+				x |= y;
+			}
+			ret = wdat_wdt_write(wdat, instr, x);
+			if (ret)
+				return ret;
+			break;
+
+		case ACPI_WDAT_WRITE_COUNTDOWN:
+			x = param;
+			x &= mask;
+			x <<= gas->bit_offset;
+			if (preserve) {
+				ret = wdat_wdt_read(wdat, instr, &y);
+				if (ret)
+					return ret;
+				y = y & ~(mask << gas->bit_offset);
+				x |= y;
+			}
+			ret = wdat_wdt_write(wdat, instr, x);
+			if (ret)
+				return ret;
+			break;
+
+		default:
+			dev_err(&wdat->pdev->dev, "Unknown instruction: %u\n",
+				flags);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int wdat_wdt_enable_reboot(struct wdat_wdt *wdat)
+{
+	int ret;
+
+	/*
+	 * WDAT specification says that the watchdog is required to reboot
+	 * the system when it fires. However, it also states that it is
+	 * recommeded to make it configurable through hardware register. We
+	 * enable reboot now if it is configrable, just in case.
+	 */
+	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_SET_REBOOT, 0, 0);
+	if (ret && ret != -EOPNOTSUPP) {
+		dev_err(&wdat->pdev->dev,
+			"Failed to enable reboot when watchdog triggers\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void wdat_wdt_boot_status(struct wdat_wdt *wdat)
+{
+	u32 boot_status = 0;
+	int ret;
+
+	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_GET_STATUS, 0, &boot_status);
+	if (ret && ret != -EOPNOTSUPP) {
+		dev_err(&wdat->pdev->dev, "Failed to read boot status\n");
+		return;
+	}
+
+	if (boot_status)
+		wdat->wdd.bootstatus = WDIOF_CARDRESET;
+
+	/* Clear the boot status in case BIOS did not do it */
+	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_SET_STATUS, 0, 0);
+	if (ret && ret != -EOPNOTSUPP)
+		dev_err(&wdat->pdev->dev, "Failed to clear boot status\n");
+}
+
+static int wdat_wdt_start(struct watchdog_device *wdd)
+{
+	return wdat_wdt_run_action(to_wdat_wdt(wdd),
+				   ACPI_WDAT_SET_RUNNING_STATE, 0, NULL);
+}
+
+static int wdat_wdt_stop(struct watchdog_device *wdd)
+{
+	return wdat_wdt_run_action(to_wdat_wdt(wdd),
+				   ACPI_WDAT_SET_STOPPED_STATE, 0, NULL);
+}
+
+static int wdat_wdt_ping(struct watchdog_device *wdd)
+{
+	return wdat_wdt_run_action(to_wdat_wdt(wdd), ACPI_WDAT_RESET, 0, NULL);
+}
+
+static int wdat_wdt_set_timeout(struct watchdog_device *wdd,
+				unsigned int timeout)
+{
+	struct wdat_wdt *wdat = to_wdat_wdt(wdd);
+	unsigned int periods;
+	int ret;
+
+	periods = timeout * 1000 / wdat->period;
+	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_SET_COUNTDOWN, periods, NULL);
+	if (!ret)
+		wdd->timeout = timeout;
+	return ret;
+}
+
+static unsigned int wdat_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct wdat_wdt *wdat = to_wdat_wdt(wdd);
+	u32 periods = 0;
+
+	wdat_wdt_run_action(wdat, ACPI_WDAT_GET_COUNTDOWN, 0, &periods);
+	return periods * wdat->period / 1000;
+}
+
+static const struct watchdog_info wdat_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.firmware_version = 0,
+	.identity = "wdat_wdt",
+};
+
+static const struct watchdog_ops wdat_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = wdat_wdt_start,
+	.stop = wdat_wdt_stop,
+	.ping = wdat_wdt_ping,
+	.set_timeout = wdat_wdt_set_timeout,
+	.get_timeleft = wdat_wdt_get_timeleft,
+};
+
+static int wdat_wdt_probe(struct platform_device *pdev)
+{
+	const struct acpi_wdat_entry *entries;
+	const struct acpi_table_wdat *tbl;
+	struct wdat_wdt *wdat;
+	struct resource *res;
+	void __iomem **regs;
+	acpi_status status;
+	int i, ret;
+
+	status = acpi_get_table(ACPI_SIG_WDAT, 0,
+				(struct acpi_table_header **)&tbl);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	wdat = devm_kzalloc(&pdev->dev, sizeof(*wdat), GFP_KERNEL);
+	if (!wdat)
+		return -ENOMEM;
+
+	regs = devm_kcalloc(&pdev->dev, pdev->num_resources, sizeof(*regs),
+			    GFP_KERNEL);
+	if (!regs)
+		return -ENOMEM;
+
+	wdat->period = tbl->timer_period;
+	wdat->wdd.max_timeout = wdat->period * tbl->max_count / 1000;
+	wdat->wdd.min_timeout = wdat->period * tbl->min_count / 1000;
+	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
+	wdat->wdd.info = &wdat_wdt_info;
+	wdat->wdd.ops = &wdat_wdt_ops;
+	wdat->pdev = pdev;
+
+	/* Request and map all resources */
+	for (i = 0; i < pdev->num_resources; i++) {
+		void __iomem *reg;
+
+		res = &pdev->resource[i];
+		if (resource_type(res) == IORESOURCE_MEM) {
+			reg = devm_ioremap_resource(&pdev->dev, res);
+		} else if (resource_type(res) == IORESOURCE_IO) {
+			reg = devm_ioport_map(&pdev->dev, res->start, 1);
+		} else {
+			dev_err(&pdev->dev, "Unsupported resource\n");
+			return -EINVAL;
+		}
+
+		if (!reg)
+			return -ENOMEM;
+
+		regs[i] = reg;
+	}
+
+	entries = (struct acpi_wdat_entry *)(tbl + 1);
+	for (i = 0; i < tbl->entries; i++) {
+		const struct acpi_generic_address *gas;
+		struct wdat_instruction *instr;
+		struct list_head *instructions;
+		unsigned int action;
+		struct resource r;
+		int j;
+
+		action = entries[i].action;
+		if (action >= MAX_WDAT_ACTIONS) {
+			dev_dbg(&pdev->dev, "Skipping unknown action: %u\n",
+				action);
+			continue;
+		}
+
+		instr = devm_kzalloc(&pdev->dev, sizeof(*instr), GFP_KERNEL);
+		if (!instr)
+			return -ENOMEM;
+
+		INIT_LIST_HEAD(&instr->node);
+		instr->entry = entries[i];
+
+		gas = &entries[i].register_region;
+
+		memset(&r, 0, sizeof(r));
+		r.start = gas->address;
+		r.end = r.start + gas->access_width;
+		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+			r.flags = IORESOURCE_MEM;
+		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
+			r.flags = IORESOURCE_IO;
+		} else {
+			dev_dbg(&pdev->dev, "Unsupported address space: %d\n",
+				gas->space_id);
+			continue;
+		}
+
+		/* Find the matching resource */
+		for (j = 0; j < pdev->num_resources; j++) {
+			res = &pdev->resource[j];
+			if (resource_contains(res, &r)) {
+				instr->reg = regs[j] + r.start - res->start;
+				break;
+			}
+		}
+
+		if (!instr->reg) {
+			dev_err(&pdev->dev, "I/O resource not found\n");
+			return -EINVAL;
+		}
+
+		instructions = wdat->instructions[action];
+		if (!instructions) {
+			instructions = devm_kzalloc(&pdev->dev,
+					sizeof(*instructions), GFP_KERNEL);
+			if (!instructions)
+				return -ENOMEM;
+
+			INIT_LIST_HEAD(instructions);
+			wdat->instructions[action] = instructions;
+		}
+
+		list_add_tail(&instr->node, instructions);
+	}
+
+	/* Make sure it is stopped now */
+	ret = wdat_wdt_stop(&wdat->wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to stop watchdog\n");
+		return ret;
+	}
+
+	wdat_wdt_boot_status(wdat);
+
+	ret = wdat_wdt_enable_reboot(wdat);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, wdat);
+
+	watchdog_set_nowayout(&wdat->wdd, nowayout);
+	return watchdog_register_device(&wdat->wdd);
+}
+
+static int wdat_wdt_remove(struct platform_device *pdev)
+{
+	struct wdat_wdt *wdat = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&wdat->wdd);
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int wdat_wdt_suspend_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct wdat_wdt *wdat = platform_get_drvdata(pdev);
+	int ret;
+
+	if (!watchdog_active(&wdat->wdd))
+		return 0;
+
+	/*
+	 * We need to stop the watchdog if firmare is not doing it or if we
+	 * are going suspend to idle (where firmware is not involved). If
+	 * firmware is stopping the watchdog we kick it here one more time
+	 * to give it some time.
+	 */
+	wdat->stopped = false;
+	if (acpi_target_system_state() == ACPI_STATE_S0 ||
+	    !wdat->stopped_in_sleep) {
+		ret = wdat_wdt_stop(&wdat->wdd);
+		if (!ret)
+			wdat->stopped = true;
+	} else {
+		ret = wdat_wdt_ping(&wdat->wdd);
+	}
+
+	return ret;
+}
+
+static int wdat_wdt_resume_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct wdat_wdt *wdat = platform_get_drvdata(pdev);
+	int ret;
+
+	if (!watchdog_active(&wdat->wdd))
+		return 0;
+
+	if (!wdat->stopped) {
+		/*
+		 * Looks like the boot firmware reinitializes the watchdog
+		 * before it hands off to the OS on resume from sleep so we
+		 * stop and reprogram the watchdog here.
+		 */
+		ret = wdat_wdt_stop(&wdat->wdd);
+		if (ret)
+			return ret;
+
+		ret = wdat_wdt_set_timeout(&wdat->wdd, wdat->wdd.timeout);
+		if (ret)
+			return ret;
+
+		ret = wdat_wdt_enable_reboot(wdat);
+		if (ret)
+			return ret;
+
+		ret = wdat_wdt_ping(&wdat->wdd);
+		if (ret)
+			return ret;
+	}
+
+	return wdat_wdt_start(&wdat->wdd);
+}
+#endif
+
+static const struct dev_pm_ops wdat_wdt_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(wdat_wdt_suspend_noirq,
+				      wdat_wdt_resume_noirq)
+};
+
+static struct platform_driver wdat_wdt_driver = {
+	.probe = wdat_wdt_probe,
+	.remove = wdat_wdt_remove,
+	.driver = {
+		.name = "wdat_wdt",
+		.pm = &wdat_wdt_pm_ops,
+	},
+};
+
+module_platform_driver(wdat_wdt_driver);
+
+MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
+MODULE_DESCRIPTION("ACPI Hardware Watchdog (WDAT) driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:wdat_wdt");
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index c5eaf2f80a4c..8ff6ca4a2639 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1074,4 +1074,10 @@ void acpi_table_upgrade(void);
 static inline void acpi_table_upgrade(void) { }
 #endif
 
+#if defined(CONFIG_ACPI) && defined(CONFIG_ACPI_WATCHDOG)
+extern bool acpi_has_watchdog(void);
+#else
+static inline bool acpi_has_watchdog(void) { return false; }
+#endif
+
 #endif	/*_LINUX_ACPI_H*/
-- 
2.9.3

Re: [PATCH 1/4] ACPI / watchdog: Add support for WDAT hardware watchdog

[Guenter Roeck]

On 09/13/2016 08:23 AM, Mika Westerberg wrote:
> Starting from Intel Skylake the iTCO watchdog timer registers were moved to
> reside in the same register space with SMBus host controller.  Not all
> needed registers are available though and we need to unhide P2SB (Primary
> to Sideband) device briefly to be able to read status of required NO_REBOOT
> bit. The i2c-i801.c SMBus driver used to handle this and creation of the
> iTCO watchdog platform device.
>
> Windows, on the other hand, does not use the iTCO watchdog hardware
> directly even if it is available. Instead it relies on ACPI Watchdog Action
> Table (WDAT) table to describe the watchdog hardware to the OS. This table
> contains necessary information about the the hardware and also set of
> actions which are executed by a driver as needed.
>
> This patch implements a new watchdog driver that takes advantage of the
> ACPI WDAT table. We split the functionality into two parts: first part
> enumerates the WDAT table and if found, populates resources and creates
> platform device for the actual driver. The second part is the driver
> itself.
>
> The reason for the split is that this way we can make the driver itself to
> be a module and loaded automatically if the WDAT table is found. Otherwise
> the module is not loaded.
>
What I don't really like is that the driver won't be in the watchdog directory,
and will thus easily be overlooked in the future by watchdog maintainers.

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/acpi/Kconfig         |  16 ++
>  drivers/acpi/Makefile        |   2 +
>  drivers/acpi/acpi_watchdog.c | 123 ++++++++++
>  drivers/acpi/internal.h      |  10 +
>  drivers/acpi/scan.c          |   1 +
>  drivers/acpi/wdat_wdt.c      | 524 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h         |   6 +
>  7 files changed, 682 insertions(+)
>  create mode 100644 drivers/acpi/acpi_watchdog.c
>  create mode 100644 drivers/acpi/wdat_wdt.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 445ce28475b3..f6dc7885c35f 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -462,6 +462,22 @@ source "drivers/acpi/nfit/Kconfig"
>  source "drivers/acpi/apei/Kconfig"
>  source "drivers/acpi/dptf/Kconfig"
>
> +config ACPI_WATCHDOG
> +	bool
> +
> +config ACPI_WDAT_WDT
> +	tristate "ACPI Watchdog Action Table (WDAT)"
> +	depends on WATCHDOG_CORE
> +	select ACPI_WATCHDOG
> +	help
> +	  This driver adds support for systems with ACPI Watchdog Action
> +	  Table (WDAT) table. Servers typically have this but it can be
> +	  found on some desktop machines as well. This driver will take
> +	  over the native iTCO watchdog driver found on many Intel CPUs.
> +
> +	  To compile this driver as module, choose M here: the module will
> +	  be called wdat_wdt.
> +
>  config ACPI_EXTLOG
>  	tristate "Extended Error Log support"
>  	depends on X86_MCE && X86_LOCAL_APIC
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 5ae9d85c5159..b9568a954090 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -56,6 +56,7 @@ acpi-$(CONFIG_ACPI_NUMA)	+= numa.o
>  acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
>  acpi-y				+= acpi_lpat.o
>  acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
> +acpi-$(CONFIG_ACPI_WATCHDOG)	+= acpi_watchdog.o
>
>  # These are (potentially) separate modules
>
> @@ -82,6 +83,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>  obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
>  obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
>  obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
> +obj-$(CONFIG_ACPI_WDAT_WDT)	+= wdat_wdt.o
>
>  # processor has its own "processor." module_param namespace
>  processor-y			:= processor_driver.o
> diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
> new file mode 100644
> index 000000000000..13caebd679f5
> --- /dev/null
> +++ b/drivers/acpi/acpi_watchdog.c
> @@ -0,0 +1,123 @@
> +/*
> + * ACPI watchdog table parsing support.
> + *
Not just parsing. Looks to me like an ACPI watchdog driver.

> + * Copyright (C) 2016, Intel Corporation
> + * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define pr_fmt(fmt) "ACPI: watchdog: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/ioport.h>
> +#include <linux/platform_device.h>
> +
> +#include "internal.h"
> +
> +/**
> + * Returns true if this system should prefer ACPI based watchdog instead of
> + * the native one (which are typically the same hardware).
> + */
> +bool acpi_has_watchdog(void)
> +{
> +	struct acpi_table_header hdr;
> +
> +	if (acpi_disabled)
> +		return false;
> +
> +	return ACPI_SUCCESS(acpi_get_table_header(ACPI_SIG_WDAT, 0, &hdr));
> +}
> +EXPORT_SYMBOL_GPL(acpi_has_watchdog);
> +
> +void __init acpi_watchdog_init(void)
> +{
> +	const struct acpi_wdat_entry *entries;
> +	const struct acpi_table_wdat *wdat;
> +	struct list_head resource_list;
> +	struct resource_entry *rentry;
> +	struct platform_device *pdev;
> +	struct resource *resources;
> +	size_t nresources = 0;
> +	acpi_status status;
> +	int i;
> +
> +	status = acpi_get_table(ACPI_SIG_WDAT, 0,
> +				(struct acpi_table_header **)&wdat);
> +	if (ACPI_FAILURE(status)) {
> +		/* It is fine if there is no WDAT */
> +		return;
> +	}
> +
> +	/* Watchdog disabled by BIOS */
> +	if (!(wdat->flags & ACPI_WDAT_ENABLED))
> +		return;
> +
> +	/* Skip legacy PCI WDT devices */
> +	if (wdat->pci_segment != 0xff || wdat->pci_bus != 0xff ||
> +	    wdat->pci_device != 0xff || wdat->pci_function != 0xff)
> +		return;
> +
> +	INIT_LIST_HEAD(&resource_list);
> +
> +	entries = (struct acpi_wdat_entry *)(wdat + 1);
> +	for (i = 0; i < wdat->entries; i++) {
> +		const struct acpi_generic_address *gas;
> +		struct resource_entry *rentry;
> +		struct resource res;
> +		bool found;
> +
> +		gas = &entries[i].register_region;
> +
> +		res.start = gas->address;
> +		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +			res.flags = IORESOURCE_MEM;
> +			res.end = res.start + ALIGN(gas->access_width, 4);
> +		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> +			res.flags = IORESOURCE_IO;
> +			res.end = res.start + gas->access_width;
> +		} else {
> +			pr_warn("Unsupported address space: %u\n",
> +				gas->space_id);
> +			goto fail_free_resource_list;
> +		}
> +
> +		found = false;
> +		resource_list_for_each_entry(rentry, &resource_list) {
> +			if (resource_contains(rentry->res, &res)) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found) {
> +			rentry = resource_list_create_entry(NULL, 0);
> +			if (!rentry)
> +				goto fail_free_resource_list;
> +
> +			*rentry->res = res;
> +			resource_list_add_tail(rentry, &resource_list);
> +			nresources++;
> +		}
> +	}
> +
> +	resources = kcalloc(nresources, sizeof(*resources), GFP_KERNEL);
> +	if (!resources)
> +		goto fail_free_resource_list;
> +
> +	i = 0;
> +	resource_list_for_each_entry(rentry, &resource_list)
> +		resources[i++] = *rentry->res;
> +
> +	pdev = platform_device_register_simple("wdat_wdt", PLATFORM_DEVID_NONE,
> +					       resources, nresources);
> +	if (IS_ERR(pdev))
> +		pr_err("Failed to create platform device\n");
> +
> +	kfree(resources);
> +
> +fail_free_resource_list:
> +	resource_list_free(&resource_list);
> +}
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 940218ff0193..fb9a7ad96756 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -225,4 +225,14 @@ static inline void suspend_nvs_restore(void) {}
>  void acpi_init_properties(struct acpi_device *adev);
>  void acpi_free_properties(struct acpi_device *adev);
>
> +/*--------------------------------------------------------------------------
> +				Watchdog
> +  -------------------------------------------------------------------------- */
> +
> +#ifdef CONFIG_ACPI_WATCHDOG
> +void acpi_watchdog_init(void);
> +#else
> +static inline void acpi_watchdog_init(void) {}
> +#endif
> +
>  #endif /* _ACPI_INTERNAL_H_ */
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index e878fc799af7..3b85d87021da 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2002,6 +2002,7 @@ int __init acpi_scan_init(void)
>  	acpi_pnp_init();
>  	acpi_int340x_thermal_init();
>  	acpi_amba_init();
> +	acpi_watchdog_init();
>
>  	acpi_scan_add_handler(&generic_device_handler);
>
> diff --git a/drivers/acpi/wdat_wdt.c b/drivers/acpi/wdat_wdt.c
> new file mode 100644
> index 000000000000..a541753a3fc1
> --- /dev/null
> +++ b/drivers/acpi/wdat_wdt.c
> @@ -0,0 +1,524 @@
> +/*
> + * ACPI Hardware Watchdog (WDAT) driver.
> + *
> + * Copyright (C) 2016, Intel Corporation
> + * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/ioport.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/watchdog.h>
> +
> +#define MAX_WDAT_ACTIONS ACPI_WDAT_ACTION_RESERVED
> +
> +/**
> + * struct wdat_instruction - Single ACPI WDAT instruction
> + * @entry: Copy of the ACPI table instruction
> + * @reg: Register the instruction is accessing
> + * @node: Next instruction in action sequence
> + */
> +struct wdat_instruction {
> +	struct acpi_wdat_entry entry;
> +	void __iomem *reg;
> +	struct list_head node;
> +};
> +
> +/**
> + * struct wdat_wdt - ACPI WDAT watchdog device
> + * @pdev: Parent platform device
> + * @wdd: Watchdog core device
> + * @period: How long is one watchdog period in ms
> + * @stopped_in_sleep: Is this watchdog stopped by the firmware in S1-S5
> + * @stopped: Was the watchdog stopped by the driver in suspend
> + * @actions: An array of instruction lists indexed by an action number from
> + *           the WDAT table. There can be %NULL entries for not implemented
> + *           actions.
> + */
> +struct wdat_wdt {
> +	struct platform_device *pdev;
> +	struct watchdog_device wdd;
> +	unsigned int period;
> +	bool stopped_in_sleep;
> +	bool stopped;
> +	struct list_head *instructions[MAX_WDAT_ACTIONS];
> +};
> +
> +#define to_wdat_wdt(wdd) container_of(wdd, struct wdat_wdt, wdd)
> +
> +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 wdat_wdt_read(struct wdat_wdt *wdat,
> +	 const struct wdat_instruction *instr, u32 *value)
> +{
> +	const struct acpi_generic_address *gas = &instr->entry.register_region;
> +
> +	switch (gas->access_width) {
> +	case 1:
> +		*value = ioread8(instr->reg);
> +		break;
> +	case 2:
> +		*value = ioread16(instr->reg);
> +		break;
> +	case 3:
> +		*value = ioread32(instr->reg);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(&wdat->pdev->dev, "Read %#x from 0x%08llx\n", *value,
> +		gas->address);
> +
> +	return 0;
> +}
> +
> +static int wdat_wdt_write(struct wdat_wdt *wdat,
> +	const struct wdat_instruction *instr, u32 value)
> +{
> +	const struct acpi_generic_address *gas = &instr->entry.register_region;
> +
> +	switch (gas->access_width) {
> +	case 1:
> +		iowrite8((u8)value, instr->reg);
> +		break;
> +	case 2:
> +		iowrite16((u16)value, instr->reg);
> +		break;
> +	case 3:
> +		iowrite32(value, instr->reg);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(&wdat->pdev->dev, "Wrote %#x to 0x%08llx\n", value,
> +		gas->address);
> +
> +	return 0;
> +}
> +
> +static int wdat_wdt_run_action(struct wdat_wdt *wdat, unsigned int action,
> +			       u32 param, u32 *retval)
> +{
> +	struct wdat_instruction *instr;
> +
> +	if (action >= ARRAY_SIZE(wdat->instructions))
> +		return -EINVAL;
> +
> +	if (!wdat->instructions[action])
> +		return -EOPNOTSUPP;
> +
> +	dev_dbg(&wdat->pdev->dev, "Running action %#x\n", action);
> +
> +	/* Run each instruction sequentially */
> +	list_for_each_entry(instr, wdat->instructions[action], node) {
> +		const struct acpi_wdat_entry *entry = &instr->entry;
> +		const struct acpi_generic_address *gas;
> +		u32 flags, value, mask, x, y;
> +		bool preserve;
> +		int ret;
> +
> +		gas = &entry->register_region;
> +
> +		preserve = entry->instruction & ACPI_WDAT_PRESERVE_REGISTER;
> +		flags = entry->instruction & ~ACPI_WDAT_PRESERVE_REGISTER;
> +		value = entry->value;
> +		mask = entry->mask;
> +
> +		switch (flags) {
> +		case ACPI_WDAT_READ_VALUE:
> +			ret = wdat_wdt_read(wdat, instr, &x);
> +			if (ret)
> +				return ret;
> +			x >>= gas->bit_offset;
> +			x &= mask;
> +			if (retval)
> +				*retval = x == value;
> +			break;
> +
> +		case ACPI_WDAT_READ_COUNTDOWN:
> +			ret = wdat_wdt_read(wdat, instr, &x);
> +			if (ret)
> +				return ret;
> +			x >>= gas->bit_offset;
> +			x &= mask;
> +			if (retval)
> +				*retval = x;
> +			break;
> +
> +		case ACPI_WDAT_WRITE_VALUE:
> +			x = value & mask;
> +			x <<= gas->bit_offset;
> +			if (preserve) {
> +				ret = wdat_wdt_read(wdat, instr, &y);
> +				if (ret)
> +					return ret;
> +				y = y & ~(mask << gas->bit_offset);
> +				x |= y;
> +			}
> +			ret = wdat_wdt_write(wdat, instr, x);
> +			if (ret)
> +				return ret;
> +			break;
> +
> +		case ACPI_WDAT_WRITE_COUNTDOWN:
> +			x = param;
> +			x &= mask;
> +			x <<= gas->bit_offset;
> +			if (preserve) {
> +				ret = wdat_wdt_read(wdat, instr, &y);
> +				if (ret)
> +					return ret;
> +				y = y & ~(mask << gas->bit_offset);
> +				x |= y;
> +			}
> +			ret = wdat_wdt_write(wdat, instr, x);
> +			if (ret)
> +				return ret;
> +			break;
> +
> +		default:
> +			dev_err(&wdat->pdev->dev, "Unknown instruction: %u\n",
> +				flags);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int wdat_wdt_enable_reboot(struct wdat_wdt *wdat)
> +{
> +	int ret;
> +
> +	/*
> +	 * WDAT specification says that the watchdog is required to reboot
> +	 * the system when it fires. However, it also states that it is
> +	 * recommeded to make it configurable through hardware register. We
> +	 * enable reboot now if it is configrable, just in case.
> +	 */
> +	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_SET_REBOOT, 0, 0);
> +	if (ret && ret != -EOPNOTSUPP) {
> +		dev_err(&wdat->pdev->dev,
> +			"Failed to enable reboot when watchdog triggers\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void wdat_wdt_boot_status(struct wdat_wdt *wdat)
> +{
> +	u32 boot_status = 0;
> +	int ret;
> +
> +	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_GET_STATUS, 0, &boot_status);
> +	if (ret && ret != -EOPNOTSUPP) {
> +		dev_err(&wdat->pdev->dev, "Failed to read boot status\n");
> +		return;
> +	}
> +
> +	if (boot_status)
> +		wdat->wdd.bootstatus = WDIOF_CARDRESET;
> +
> +	/* Clear the boot status in case BIOS did not do it */
> +	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_SET_STATUS, 0, 0);
> +	if (ret && ret != -EOPNOTSUPP)
> +		dev_err(&wdat->pdev->dev, "Failed to clear boot status\n");
> +}
> +
> +static int wdat_wdt_start(struct watchdog_device *wdd)
> +{
> +	return wdat_wdt_run_action(to_wdat_wdt(wdd),
> +				   ACPI_WDAT_SET_RUNNING_STATE, 0, NULL);
> +}
> +
> +static int wdat_wdt_stop(struct watchdog_device *wdd)
> +{
> +	return wdat_wdt_run_action(to_wdat_wdt(wdd),
> +				   ACPI_WDAT_SET_STOPPED_STATE, 0, NULL);
> +}
> +
> +static int wdat_wdt_ping(struct watchdog_device *wdd)
> +{
> +	return wdat_wdt_run_action(to_wdat_wdt(wdd), ACPI_WDAT_RESET, 0, NULL);
> +}
> +
> +static int wdat_wdt_set_timeout(struct watchdog_device *wdd,
> +				unsigned int timeout)
> +{
> +	struct wdat_wdt *wdat = to_wdat_wdt(wdd);
> +	unsigned int periods;
> +	int ret;
> +
> +	periods = timeout * 1000 / wdat->period;
> +	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_SET_COUNTDOWN, periods, NULL);
> +	if (!ret)
> +		wdd->timeout = timeout;
> +	return ret;
> +}
> +
> +static unsigned int wdat_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct wdat_wdt *wdat = to_wdat_wdt(wdd);
> +	u32 periods = 0;
> +
> +	wdat_wdt_run_action(wdat, ACPI_WDAT_GET_COUNTDOWN, 0, &periods);
> +	return periods * wdat->period / 1000;
> +}
> +
> +static const struct watchdog_info wdat_wdt_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +	.firmware_version = 0,
> +	.identity = "wdat_wdt",
> +};
> +
> +static const struct watchdog_ops wdat_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = wdat_wdt_start,
> +	.stop = wdat_wdt_stop,
> +	.ping = wdat_wdt_ping,
> +	.set_timeout = wdat_wdt_set_timeout,
> +	.get_timeleft = wdat_wdt_get_timeleft,
> +};
> +
> +static int wdat_wdt_probe(struct platform_device *pdev)
> +{
> +	const struct acpi_wdat_entry *entries;
> +	const struct acpi_table_wdat *tbl;
> +	struct wdat_wdt *wdat;
> +	struct resource *res;
> +	void __iomem **regs;
> +	acpi_status status;
> +	int i, ret;
> +
> +	status = acpi_get_table(ACPI_SIG_WDAT, 0,
> +				(struct acpi_table_header **)&tbl);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	wdat = devm_kzalloc(&pdev->dev, sizeof(*wdat), GFP_KERNEL);
> +	if (!wdat)
> +		return -ENOMEM;
> +
> +	regs = devm_kcalloc(&pdev->dev, pdev->num_resources, sizeof(*regs),
> +			    GFP_KERNEL);
> +	if (!regs)
> +		return -ENOMEM;
> +
> +	wdat->period = tbl->timer_period;

Is this guaranteed to never be 0 ?

> +	wdat->wdd.max_timeout = wdat->period * tbl->max_count / 1000;
> +	wdat->wdd.min_timeout = wdat->period * tbl->min_count / 1000;

Are those guaranteed to be correct, ie max_timeout >= min_timeout
and both valid ?

Reason for asking is that the core will accept any timeouts if, for
example, max_timeout is 0.

> +	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
> +	wdat->wdd.info = &wdat_wdt_info;
> +	wdat->wdd.ops = &wdat_wdt_ops;
> +	wdat->pdev = pdev;
> +
> +	/* Request and map all resources */
> +	for (i = 0; i < pdev->num_resources; i++) {
> +		void __iomem *reg;
> +
> +		res = &pdev->resource[i];
> +		if (resource_type(res) == IORESOURCE_MEM) {
> +			reg = devm_ioremap_resource(&pdev->dev, res);
> +		} else if (resource_type(res) == IORESOURCE_IO) {
> +			reg = devm_ioport_map(&pdev->dev, res->start, 1);
> +		} else {
> +			dev_err(&pdev->dev, "Unsupported resource\n");
> +			return -EINVAL;
> +		}
> +
> +		if (!reg)
> +			return -ENOMEM;
> +
> +		regs[i] = reg;
> +	}
> +
> +	entries = (struct acpi_wdat_entry *)(tbl + 1);
> +	for (i = 0; i < tbl->entries; i++) {
> +		const struct acpi_generic_address *gas;
> +		struct wdat_instruction *instr;
> +		struct list_head *instructions;
> +		unsigned int action;
> +		struct resource r;
> +		int j;
> +
> +		action = entries[i].action;
> +		if (action >= MAX_WDAT_ACTIONS) {
> +			dev_dbg(&pdev->dev, "Skipping unknown action: %u\n",
> +				action);
> +			continue;
> +		}
> +
> +		instr = devm_kzalloc(&pdev->dev, sizeof(*instr), GFP_KERNEL);
> +		if (!instr)
> +			return -ENOMEM;
> +
> +		INIT_LIST_HEAD(&instr->node);
> +		instr->entry = entries[i];
> +
> +		gas = &entries[i].register_region;
> +
> +		memset(&r, 0, sizeof(r));
> +		r.start = gas->address;
> +		r.end = r.start + gas->access_width;
> +		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +			r.flags = IORESOURCE_MEM;
> +		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> +			r.flags = IORESOURCE_IO;
> +		} else {
> +			dev_dbg(&pdev->dev, "Unsupported address space: %d\n",
> +				gas->space_id);
> +			continue;
> +		}
> +
> +		/* Find the matching resource */
> +		for (j = 0; j < pdev->num_resources; j++) {
> +			res = &pdev->resource[j];
> +			if (resource_contains(res, &r)) {
> +				instr->reg = regs[j] + r.start - res->start;
> +				break;
> +			}
> +		}
> +
> +		if (!instr->reg) {
> +			dev_err(&pdev->dev, "I/O resource not found\n");
> +			return -EINVAL;
> +		}
> +
> +		instructions = wdat->instructions[action];
> +		if (!instructions) {
> +			instructions = devm_kzalloc(&pdev->dev,
> +					sizeof(*instructions), GFP_KERNEL);
> +			if (!instructions)
> +				return -ENOMEM;
> +
> +			INIT_LIST_HEAD(instructions);
> +			wdat->instructions[action] = instructions;
> +		}
> +
> +		list_add_tail(&instr->node, instructions);
> +	}
> +
> +	/* Make sure it is stopped now */
> +	ret = wdat_wdt_stop(&wdat->wdd);

Why ? You could set max_hw_heartbeat_ms instead of max_timeout and
inform the watchdog core that the watchdog is running (by setting
the WDOG_HW_RUNNING status flag).

> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to stop watchdog\n");
> +		return ret;
> +	}
> +
> +	wdat_wdt_boot_status(wdat);
> +
> +	ret = wdat_wdt_enable_reboot(wdat);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, wdat);
> +
> +	watchdog_set_nowayout(&wdat->wdd, nowayout);
> +	return watchdog_register_device(&wdat->wdd);
> +}
> +
> +static int wdat_wdt_remove(struct platform_device *pdev)
> +{
> +	struct wdat_wdt *wdat = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&wdat->wdd);

You could also use devm_watchdog_register_device()
and drop the remove function.

> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP

I personally prefer to drop the #ifdef and use __maybe_unused instead.

> +static int wdat_wdt_suspend_noirq(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct wdat_wdt *wdat = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	if (!watchdog_active(&wdat->wdd))
> +		return 0;
> +
> +	/*
> +	 * We need to stop the watchdog if firmare is not doing it or if we
> +	 * are going suspend to idle (where firmware is not involved). If
> +	 * firmware is stopping the watchdog we kick it here one more time
> +	 * to give it some time.
> +	 */
> +	wdat->stopped = false;

Could the watchdog already be stopped here ? If so, what would be the purpose
and benefit of stopping it again ? And if it is already stopped, could there
be conditions where it would not restart in suspend because the stopped flag
is 0 ?

> +	if (acpi_target_system_state() == ACPI_STATE_S0 ||
> +	    !wdat->stopped_in_sleep) {
> +		ret = wdat_wdt_stop(&wdat->wdd);
> +		if (!ret)
> +			wdat->stopped = true;
> +	} else {
> +		ret = wdat_wdt_ping(&wdat->wdd);
> +	}
> +
> +	return ret;
> +}
> +
> +static int wdat_wdt_resume_noirq(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct wdat_wdt *wdat = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	if (!watchdog_active(&wdat->wdd))
> +		return 0;
> +
> +	if (!wdat->stopped) {
> +		/*
> +		 * Looks like the boot firmware reinitializes the watchdog
> +		 * before it hands off to the OS on resume from sleep so we
> +		 * stop and reprogram the watchdog here.

And the firmware reprograms it as it sees fit ? Brr ... :-(

> +		 */
> +		ret = wdat_wdt_stop(&wdat->wdd);
> +		if (ret)
> +			return ret;
> +
> +		ret = wdat_wdt_set_timeout(&wdat->wdd, wdat->wdd.timeout);
> +		if (ret)
> +			return ret;
> +
> +		ret = wdat_wdt_enable_reboot(wdat);
> +		if (ret)
> +			return ret;
> +
> +		ret = wdat_wdt_ping(&wdat->wdd);
> +		if (ret)
> +			return ret;

The watchdog is already running here. Why start it again ?

> +	}
> +
> +	return wdat_wdt_start(&wdat->wdd);
> +}
> +#endif
> +
> +static const struct dev_pm_ops wdat_wdt_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(wdat_wdt_suspend_noirq,
> +				      wdat_wdt_resume_noirq)
> +};
> +
> +static struct platform_driver wdat_wdt_driver = {
> +	.probe = wdat_wdt_probe,
> +	.remove = wdat_wdt_remove,
> +	.driver = {
> +		.name = "wdat_wdt",
> +		.pm = &wdat_wdt_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(wdat_wdt_driver);
> +
> +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
> +MODULE_DESCRIPTION("ACPI Hardware Watchdog (WDAT) driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:wdat_wdt");
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index c5eaf2f80a4c..8ff6ca4a2639 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1074,4 +1074,10 @@ void acpi_table_upgrade(void);
>  static inline void acpi_table_upgrade(void) { }
>  #endif
>
> +#if defined(CONFIG_ACPI) && defined(CONFIG_ACPI_WATCHDOG)
> +extern bool acpi_has_watchdog(void);
> +#else
> +static inline bool acpi_has_watchdog(void) { return false; }
> +#endif
> +
>  #endif	/*_LINUX_ACPI_H*/
>

Re: [PATCH 1/4] ACPI / watchdog: Add support for WDAT hardware watchdog

[Mika Westerberg]

On Tue, Sep 13, 2016 at 02:00:25PM -0700, Guenter Roeck wrote:
> On 09/13/2016 08:23 AM, Mika Westerberg wrote:
> > Starting from Intel Skylake the iTCO watchdog timer registers were moved to
> > reside in the same register space with SMBus host controller.  Not all
> > needed registers are available though and we need to unhide P2SB (Primary
> > to Sideband) device briefly to be able to read status of required NO_REBOOT
> > bit. The i2c-i801.c SMBus driver used to handle this and creation of the
> > iTCO watchdog platform device.
> > 
> > Windows, on the other hand, does not use the iTCO watchdog hardware
> > directly even if it is available. Instead it relies on ACPI Watchdog Action
> > Table (WDAT) table to describe the watchdog hardware to the OS. This table
> > contains necessary information about the the hardware and also set of
> > actions which are executed by a driver as needed.
> > 
> > This patch implements a new watchdog driver that takes advantage of the
> > ACPI WDAT table. We split the functionality into two parts: first part
> > enumerates the WDAT table and if found, populates resources and creates
> > platform device for the actual driver. The second part is the driver
> > itself.
> > 
> > The reason for the split is that this way we can make the driver itself to
> > be a module and loaded automatically if the WDAT table is found. Otherwise
> > the module is not loaded.
> > 
> What I don't really like is that the driver won't be in the watchdog directory,
> and will thus easily be overlooked in the future by watchdog maintainers.

It is in ACPI directory because we expose the functionality to users as
"ACPI Watchdog Action Table (WDAT)" which works with other similar table
options in drivers/acpi/Kconfig.

I'm fine moving the driver itself (wdat_wdt.c) under drivers/watchdog
but at least the enumeration part should be part of drivers/acpi and I
would still like to have only one user selectable option.

> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/acpi/Kconfig         |  16 ++
> >  drivers/acpi/Makefile        |   2 +
> >  drivers/acpi/acpi_watchdog.c | 123 ++++++++++
> >  drivers/acpi/internal.h      |  10 +
> >  drivers/acpi/scan.c          |   1 +
> >  drivers/acpi/wdat_wdt.c      | 524 +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/acpi.h         |   6 +
> >  7 files changed, 682 insertions(+)
> >  create mode 100644 drivers/acpi/acpi_watchdog.c
> >  create mode 100644 drivers/acpi/wdat_wdt.c
> > 
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index 445ce28475b3..f6dc7885c35f 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -462,6 +462,22 @@ source "drivers/acpi/nfit/Kconfig"
> >  source "drivers/acpi/apei/Kconfig"
> >  source "drivers/acpi/dptf/Kconfig"
> > 
> > +config ACPI_WATCHDOG
> > +	bool
> > +
> > +config ACPI_WDAT_WDT
> > +	tristate "ACPI Watchdog Action Table (WDAT)"
> > +	depends on WATCHDOG_CORE
> > +	select ACPI_WATCHDOG
> > +	help
> > +	  This driver adds support for systems with ACPI Watchdog Action
> > +	  Table (WDAT) table. Servers typically have this but it can be
> > +	  found on some desktop machines as well. This driver will take
> > +	  over the native iTCO watchdog driver found on many Intel CPUs.
> > +
> > +	  To compile this driver as module, choose M here: the module will
> > +	  be called wdat_wdt.
> > +
> >  config ACPI_EXTLOG
> >  	tristate "Extended Error Log support"
> >  	depends on X86_MCE && X86_LOCAL_APIC
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index 5ae9d85c5159..b9568a954090 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -56,6 +56,7 @@ acpi-$(CONFIG_ACPI_NUMA)	+= numa.o
> >  acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
> >  acpi-y				+= acpi_lpat.o
> >  acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
> > +acpi-$(CONFIG_ACPI_WATCHDOG)	+= acpi_watchdog.o
> > 
> >  # These are (potentially) separate modules
> > 
> > @@ -82,6 +83,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
> >  obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
> >  obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
> >  obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
> > +obj-$(CONFIG_ACPI_WDAT_WDT)	+= wdat_wdt.o
> > 
> >  # processor has its own "processor." module_param namespace
> >  processor-y			:= processor_driver.o
> > diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
> > new file mode 100644
> > index 000000000000..13caebd679f5
> > --- /dev/null
> > +++ b/drivers/acpi/acpi_watchdog.c
> > @@ -0,0 +1,123 @@
> > +/*
> > + * ACPI watchdog table parsing support.
> > + *
> Not just parsing. Looks to me like an ACPI watchdog driver.

No this is really just parsing ;-) The driver lives in drivers/acpi/wdat_wdt.c.

> > + * Copyright (C) 2016, Intel Corporation
> > + * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#define pr_fmt(fmt) "ACPI: watchdog: " fmt
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/ioport.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "internal.h"
> > +
> > +/**
> > + * Returns true if this system should prefer ACPI based watchdog instead of
> > + * the native one (which are typically the same hardware).
> > + */
> > +bool acpi_has_watchdog(void)
> > +{
> > +	struct acpi_table_header hdr;
> > +
> > +	if (acpi_disabled)
> > +		return false;
> > +
> > +	return ACPI_SUCCESS(acpi_get_table_header(ACPI_SIG_WDAT, 0, &hdr));
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_has_watchdog);
> > +
> > +void __init acpi_watchdog_init(void)
> > +{
> > +	const struct acpi_wdat_entry *entries;
> > +	const struct acpi_table_wdat *wdat;
> > +	struct list_head resource_list;
> > +	struct resource_entry *rentry;
> > +	struct platform_device *pdev;
> > +	struct resource *resources;
> > +	size_t nresources = 0;
> > +	acpi_status status;
> > +	int i;
> > +
> > +	status = acpi_get_table(ACPI_SIG_WDAT, 0,
> > +				(struct acpi_table_header **)&wdat);
> > +	if (ACPI_FAILURE(status)) {
> > +		/* It is fine if there is no WDAT */
> > +		return;
> > +	}
> > +
> > +	/* Watchdog disabled by BIOS */
> > +	if (!(wdat->flags & ACPI_WDAT_ENABLED))
> > +		return;
> > +
> > +	/* Skip legacy PCI WDT devices */
> > +	if (wdat->pci_segment != 0xff || wdat->pci_bus != 0xff ||
> > +	    wdat->pci_device != 0xff || wdat->pci_function != 0xff)
> > +		return;
> > +
> > +	INIT_LIST_HEAD(&resource_list);
> > +
> > +	entries = (struct acpi_wdat_entry *)(wdat + 1);
> > +	for (i = 0; i < wdat->entries; i++) {
> > +		const struct acpi_generic_address *gas;
> > +		struct resource_entry *rentry;
> > +		struct resource res;
> > +		bool found;
> > +
> > +		gas = &entries[i].register_region;
> > +
> > +		res.start = gas->address;
> > +		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> > +			res.flags = IORESOURCE_MEM;
> > +			res.end = res.start + ALIGN(gas->access_width, 4);
> > +		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> > +			res.flags = IORESOURCE_IO;
> > +			res.end = res.start + gas->access_width;
> > +		} else {
> > +			pr_warn("Unsupported address space: %u\n",
> > +				gas->space_id);
> > +			goto fail_free_resource_list;
> > +		}
> > +
> > +		found = false;
> > +		resource_list_for_each_entry(rentry, &resource_list) {
> > +			if (resource_contains(rentry->res, &res)) {
> > +				found = true;
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (!found) {
> > +			rentry = resource_list_create_entry(NULL, 0);
> > +			if (!rentry)
> > +				goto fail_free_resource_list;
> > +
> > +			*rentry->res = res;
> > +			resource_list_add_tail(rentry, &resource_list);
> > +			nresources++;
> > +		}
> > +	}
> > +
> > +	resources = kcalloc(nresources, sizeof(*resources), GFP_KERNEL);
> > +	if (!resources)
> > +		goto fail_free_resource_list;
> > +
> > +	i = 0;
> > +	resource_list_for_each_entry(rentry, &resource_list)
> > +		resources[i++] = *rentry->res;
> > +
> > +	pdev = platform_device_register_simple("wdat_wdt", PLATFORM_DEVID_NONE,
> > +					       resources, nresources);
> > +	if (IS_ERR(pdev))
> > +		pr_err("Failed to create platform device\n");
> > +
> > +	kfree(resources);
> > +
> > +fail_free_resource_list:
> > +	resource_list_free(&resource_list);
> > +}
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 940218ff0193..fb9a7ad96756 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -225,4 +225,14 @@ static inline void suspend_nvs_restore(void) {}
> >  void acpi_init_properties(struct acpi_device *adev);
> >  void acpi_free_properties(struct acpi_device *adev);
> > 
> > +/*--------------------------------------------------------------------------
> > +				Watchdog
> > +  -------------------------------------------------------------------------- */
> > +
> > +#ifdef CONFIG_ACPI_WATCHDOG
> > +void acpi_watchdog_init(void);
> > +#else
> > +static inline void acpi_watchdog_init(void) {}
> > +#endif
> > +
> >  #endif /* _ACPI_INTERNAL_H_ */
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index e878fc799af7..3b85d87021da 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -2002,6 +2002,7 @@ int __init acpi_scan_init(void)
> >  	acpi_pnp_init();
> >  	acpi_int340x_thermal_init();
> >  	acpi_amba_init();
> > +	acpi_watchdog_init();
> > 
> >  	acpi_scan_add_handler(&generic_device_handler);
> > 
> > diff --git a/drivers/acpi/wdat_wdt.c b/drivers/acpi/wdat_wdt.c
> > new file mode 100644
> > index 000000000000..a541753a3fc1
> > --- /dev/null
> > +++ b/drivers/acpi/wdat_wdt.c
> > @@ -0,0 +1,524 @@
> > +/*
> > + * ACPI Hardware Watchdog (WDAT) driver.
> > + *
> > + * Copyright (C) 2016, Intel Corporation
> > + * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/ioport.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define MAX_WDAT_ACTIONS ACPI_WDAT_ACTION_RESERVED
> > +
> > +/**
> > + * struct wdat_instruction - Single ACPI WDAT instruction
> > + * @entry: Copy of the ACPI table instruction
> > + * @reg: Register the instruction is accessing
> > + * @node: Next instruction in action sequence
> > + */
> > +struct wdat_instruction {
> > +	struct acpi_wdat_entry entry;
> > +	void __iomem *reg;
> > +	struct list_head node;
> > +};
> > +
> > +/**
> > + * struct wdat_wdt - ACPI WDAT watchdog device
> > + * @pdev: Parent platform device
> > + * @wdd: Watchdog core device
> > + * @period: How long is one watchdog period in ms
> > + * @stopped_in_sleep: Is this watchdog stopped by the firmware in S1-S5
> > + * @stopped: Was the watchdog stopped by the driver in suspend
> > + * @actions: An array of instruction lists indexed by an action number from
> > + *           the WDAT table. There can be %NULL entries for not implemented
> > + *           actions.
> > + */
> > +struct wdat_wdt {
> > +	struct platform_device *pdev;
> > +	struct watchdog_device wdd;
> > +	unsigned int period;
> > +	bool stopped_in_sleep;
> > +	bool stopped;
> > +	struct list_head *instructions[MAX_WDAT_ACTIONS];
> > +};
> > +
> > +#define to_wdat_wdt(wdd) container_of(wdd, struct wdat_wdt, wdd)
> > +
> > +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 wdat_wdt_read(struct wdat_wdt *wdat,
> > +	 const struct wdat_instruction *instr, u32 *value)
> > +{
> > +	const struct acpi_generic_address *gas = &instr->entry.register_region;
> > +
> > +	switch (gas->access_width) {
> > +	case 1:
> > +		*value = ioread8(instr->reg);
> > +		break;
> > +	case 2:
> > +		*value = ioread16(instr->reg);
> > +		break;
> > +	case 3:
> > +		*value = ioread32(instr->reg);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(&wdat->pdev->dev, "Read %#x from 0x%08llx\n", *value,
> > +		gas->address);
> > +
> > +	return 0;
> > +}
> > +
> > +static int wdat_wdt_write(struct wdat_wdt *wdat,
> > +	const struct wdat_instruction *instr, u32 value)
> > +{
> > +	const struct acpi_generic_address *gas = &instr->entry.register_region;
> > +
> > +	switch (gas->access_width) {
> > +	case 1:
> > +		iowrite8((u8)value, instr->reg);
> > +		break;
> > +	case 2:
> > +		iowrite16((u16)value, instr->reg);
> > +		break;
> > +	case 3:
> > +		iowrite32(value, instr->reg);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(&wdat->pdev->dev, "Wrote %#x to 0x%08llx\n", value,
> > +		gas->address);
> > +
> > +	return 0;
> > +}
> > +
> > +static int wdat_wdt_run_action(struct wdat_wdt *wdat, unsigned int action,
> > +			       u32 param, u32 *retval)
> > +{
> > +	struct wdat_instruction *instr;
> > +
> > +	if (action >= ARRAY_SIZE(wdat->instructions))
> > +		return -EINVAL;
> > +
> > +	if (!wdat->instructions[action])
> > +		return -EOPNOTSUPP;
> > +
> > +	dev_dbg(&wdat->pdev->dev, "Running action %#x\n", action);
> > +
> > +	/* Run each instruction sequentially */
> > +	list_for_each_entry(instr, wdat->instructions[action], node) {
> > +		const struct acpi_wdat_entry *entry = &instr->entry;
> > +		const struct acpi_generic_address *gas;
> > +		u32 flags, value, mask, x, y;
> > +		bool preserve;
> > +		int ret;
> > +
> > +		gas = &entry->register_region;
> > +
> > +		preserve = entry->instruction & ACPI_WDAT_PRESERVE_REGISTER;
> > +		flags = entry->instruction & ~ACPI_WDAT_PRESERVE_REGISTER;
> > +		value = entry->value;
> > +		mask = entry->mask;
> > +
> > +		switch (flags) {
> > +		case ACPI_WDAT_READ_VALUE:
> > +			ret = wdat_wdt_read(wdat, instr, &x);
> > +			if (ret)
> > +				return ret;
> > +			x >>= gas->bit_offset;
> > +			x &= mask;
> > +			if (retval)
> > +				*retval = x == value;
> > +			break;
> > +
> > +		case ACPI_WDAT_READ_COUNTDOWN:
> > +			ret = wdat_wdt_read(wdat, instr, &x);
> > +			if (ret)
> > +				return ret;
> > +			x >>= gas->bit_offset;
> > +			x &= mask;
> > +			if (retval)
> > +				*retval = x;
> > +			break;
> > +
> > +		case ACPI_WDAT_WRITE_VALUE:
> > +			x = value & mask;
> > +			x <<= gas->bit_offset;
> > +			if (preserve) {
> > +				ret = wdat_wdt_read(wdat, instr, &y);
> > +				if (ret)
> > +					return ret;
> > +				y = y & ~(mask << gas->bit_offset);
> > +				x |= y;
> > +			}
> > +			ret = wdat_wdt_write(wdat, instr, x);
> > +			if (ret)
> > +				return ret;
> > +			break;
> > +
> > +		case ACPI_WDAT_WRITE_COUNTDOWN:
> > +			x = param;
> > +			x &= mask;
> > +			x <<= gas->bit_offset;
> > +			if (preserve) {
> > +				ret = wdat_wdt_read(wdat, instr, &y);
> > +				if (ret)
> > +					return ret;
> > +				y = y & ~(mask << gas->bit_offset);
> > +				x |= y;
> > +			}
> > +			ret = wdat_wdt_write(wdat, instr, x);
> > +			if (ret)
> > +				return ret;
> > +			break;
> > +
> > +		default:
> > +			dev_err(&wdat->pdev->dev, "Unknown instruction: %u\n",
> > +				flags);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int wdat_wdt_enable_reboot(struct wdat_wdt *wdat)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * WDAT specification says that the watchdog is required to reboot
> > +	 * the system when it fires. However, it also states that it is
> > +	 * recommeded to make it configurable through hardware register. We
> > +	 * enable reboot now if it is configrable, just in case.
> > +	 */
> > +	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_SET_REBOOT, 0, 0);
> > +	if (ret && ret != -EOPNOTSUPP) {
> > +		dev_err(&wdat->pdev->dev,
> > +			"Failed to enable reboot when watchdog triggers\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void wdat_wdt_boot_status(struct wdat_wdt *wdat)
> > +{
> > +	u32 boot_status = 0;
> > +	int ret;
> > +
> > +	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_GET_STATUS, 0, &boot_status);
> > +	if (ret && ret != -EOPNOTSUPP) {
> > +		dev_err(&wdat->pdev->dev, "Failed to read boot status\n");
> > +		return;
> > +	}
> > +
> > +	if (boot_status)
> > +		wdat->wdd.bootstatus = WDIOF_CARDRESET;
> > +
> > +	/* Clear the boot status in case BIOS did not do it */
> > +	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_SET_STATUS, 0, 0);
> > +	if (ret && ret != -EOPNOTSUPP)
> > +		dev_err(&wdat->pdev->dev, "Failed to clear boot status\n");
> > +}
> > +
> > +static int wdat_wdt_start(struct watchdog_device *wdd)
> > +{
> > +	return wdat_wdt_run_action(to_wdat_wdt(wdd),
> > +				   ACPI_WDAT_SET_RUNNING_STATE, 0, NULL);
> > +}
> > +
> > +static int wdat_wdt_stop(struct watchdog_device *wdd)
> > +{
> > +	return wdat_wdt_run_action(to_wdat_wdt(wdd),
> > +				   ACPI_WDAT_SET_STOPPED_STATE, 0, NULL);
> > +}
> > +
> > +static int wdat_wdt_ping(struct watchdog_device *wdd)
> > +{
> > +	return wdat_wdt_run_action(to_wdat_wdt(wdd), ACPI_WDAT_RESET, 0, NULL);
> > +}
> > +
> > +static int wdat_wdt_set_timeout(struct watchdog_device *wdd,
> > +				unsigned int timeout)
> > +{
> > +	struct wdat_wdt *wdat = to_wdat_wdt(wdd);
> > +	unsigned int periods;
> > +	int ret;
> > +
> > +	periods = timeout * 1000 / wdat->period;
> > +	ret = wdat_wdt_run_action(wdat, ACPI_WDAT_SET_COUNTDOWN, periods, NULL);
> > +	if (!ret)
> > +		wdd->timeout = timeout;
> > +	return ret;
> > +}
> > +
> > +static unsigned int wdat_wdt_get_timeleft(struct watchdog_device *wdd)
> > +{
> > +	struct wdat_wdt *wdat = to_wdat_wdt(wdd);
> > +	u32 periods = 0;
> > +
> > +	wdat_wdt_run_action(wdat, ACPI_WDAT_GET_COUNTDOWN, 0, &periods);
> > +	return periods * wdat->period / 1000;
> > +}
> > +
> > +static const struct watchdog_info wdat_wdt_info = {
> > +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> > +	.firmware_version = 0,
> > +	.identity = "wdat_wdt",
> > +};
> > +
> > +static const struct watchdog_ops wdat_wdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = wdat_wdt_start,
> > +	.stop = wdat_wdt_stop,
> > +	.ping = wdat_wdt_ping,
> > +	.set_timeout = wdat_wdt_set_timeout,
> > +	.get_timeleft = wdat_wdt_get_timeleft,
> > +};
> > +
> > +static int wdat_wdt_probe(struct platform_device *pdev)
> > +{
> > +	const struct acpi_wdat_entry *entries;
> > +	const struct acpi_table_wdat *tbl;
> > +	struct wdat_wdt *wdat;
> > +	struct resource *res;
> > +	void __iomem **regs;
> > +	acpi_status status;
> > +	int i, ret;
> > +
> > +	status = acpi_get_table(ACPI_SIG_WDAT, 0,
> > +				(struct acpi_table_header **)&tbl);
> > +	if (ACPI_FAILURE(status))
> > +		return -ENODEV;
> > +
> > +	wdat = devm_kzalloc(&pdev->dev, sizeof(*wdat), GFP_KERNEL);
> > +	if (!wdat)
> > +		return -ENOMEM;
> > +
> > +	regs = devm_kcalloc(&pdev->dev, pdev->num_resources, sizeof(*regs),
> > +			    GFP_KERNEL);
> > +	if (!regs)
> > +		return -ENOMEM;
> > +
> > +	wdat->period = tbl->timer_period;
> 
> Is this guaranteed to never be 0 ?

According to WDAT spec it must be >= 1ms.

> > +	wdat->wdd.max_timeout = wdat->period * tbl->max_count / 1000;
> > +	wdat->wdd.min_timeout = wdat->period * tbl->min_count / 1000;
> 
> Are those guaranteed to be correct, ie max_timeout >= min_timeout
> and both valid ?

The WDAT spec says nothing about those. I'll add sanity check here and
return -EINVAL if the values cannot be used. While there I think I can
do the same for tbl->timer_period, just in case.

> Reason for asking is that the core will accept any timeouts if, for
> example, max_timeout is 0.
> 
> > +	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
> > +	wdat->wdd.info = &wdat_wdt_info;
> > +	wdat->wdd.ops = &wdat_wdt_ops;
> > +	wdat->pdev = pdev;
> > +
> > +	/* Request and map all resources */
> > +	for (i = 0; i < pdev->num_resources; i++) {
> > +		void __iomem *reg;
> > +
> > +		res = &pdev->resource[i];
> > +		if (resource_type(res) == IORESOURCE_MEM) {
> > +			reg = devm_ioremap_resource(&pdev->dev, res);
> > +		} else if (resource_type(res) == IORESOURCE_IO) {
> > +			reg = devm_ioport_map(&pdev->dev, res->start, 1);
> > +		} else {
> > +			dev_err(&pdev->dev, "Unsupported resource\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (!reg)
> > +			return -ENOMEM;
> > +
> > +		regs[i] = reg;
> > +	}
> > +
> > +	entries = (struct acpi_wdat_entry *)(tbl + 1);
> > +	for (i = 0; i < tbl->entries; i++) {
> > +		const struct acpi_generic_address *gas;
> > +		struct wdat_instruction *instr;
> > +		struct list_head *instructions;
> > +		unsigned int action;
> > +		struct resource r;
> > +		int j;
> > +
> > +		action = entries[i].action;
> > +		if (action >= MAX_WDAT_ACTIONS) {
> > +			dev_dbg(&pdev->dev, "Skipping unknown action: %u\n",
> > +				action);
> > +			continue;
> > +		}
> > +
> > +		instr = devm_kzalloc(&pdev->dev, sizeof(*instr), GFP_KERNEL);
> > +		if (!instr)
> > +			return -ENOMEM;
> > +
> > +		INIT_LIST_HEAD(&instr->node);
> > +		instr->entry = entries[i];
> > +
> > +		gas = &entries[i].register_region;
> > +
> > +		memset(&r, 0, sizeof(r));
> > +		r.start = gas->address;
> > +		r.end = r.start + gas->access_width;
> > +		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> > +			r.flags = IORESOURCE_MEM;
> > +		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> > +			r.flags = IORESOURCE_IO;
> > +		} else {
> > +			dev_dbg(&pdev->dev, "Unsupported address space: %d\n",
> > +				gas->space_id);
> > +			continue;
> > +		}
> > +
> > +		/* Find the matching resource */
> > +		for (j = 0; j < pdev->num_resources; j++) {
> > +			res = &pdev->resource[j];
> > +			if (resource_contains(res, &r)) {
> > +				instr->reg = regs[j] + r.start - res->start;
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (!instr->reg) {
> > +			dev_err(&pdev->dev, "I/O resource not found\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		instructions = wdat->instructions[action];
> > +		if (!instructions) {
> > +			instructions = devm_kzalloc(&pdev->dev,
> > +					sizeof(*instructions), GFP_KERNEL);
> > +			if (!instructions)
> > +				return -ENOMEM;
> > +
> > +			INIT_LIST_HEAD(instructions);
> > +			wdat->instructions[action] = instructions;
> > +		}
> > +
> > +		list_add_tail(&instr->node, instructions);
> > +	}
> > +
> > +	/* Make sure it is stopped now */
> > +	ret = wdat_wdt_stop(&wdat->wdd);
> 
> Why ? You could set max_hw_heartbeat_ms instead of max_timeout and
> inform the watchdog core that the watchdog is running (by setting
> the WDOG_HW_RUNNING status flag).

Hmm is the watchdog core then kicking it? 

It is stopped here to make sure system does not reboot until userspace
explicitly opens the device and starts kicking the watchdog.

> 
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to stop watchdog\n");
> > +		return ret;
> > +	}
> > +
> > +	wdat_wdt_boot_status(wdat);
> > +
> > +	ret = wdat_wdt_enable_reboot(wdat);
> > +	if (ret)
> > +		return ret;
> > +
> > +	platform_set_drvdata(pdev, wdat);
> > +
> > +	watchdog_set_nowayout(&wdat->wdd, nowayout);
> > +	return watchdog_register_device(&wdat->wdd);
> > +}
> > +
> > +static int wdat_wdt_remove(struct platform_device *pdev)
> > +{
> > +	struct wdat_wdt *wdat = platform_get_drvdata(pdev);
> > +
> > +	watchdog_unregister_device(&wdat->wdd);
> 
> You could also use devm_watchdog_register_device()
> and drop the remove function.

Good point.

> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> 
> I personally prefer to drop the #ifdef and use __maybe_unused instead.
> 
> > +static int wdat_wdt_suspend_noirq(struct device *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct wdat_wdt *wdat = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	if (!watchdog_active(&wdat->wdd))
> > +		return 0;
> > +
> > +	/*
> > +	 * We need to stop the watchdog if firmare is not doing it or if we
> > +	 * are going suspend to idle (where firmware is not involved). If
> > +	 * firmware is stopping the watchdog we kick it here one more time
> > +	 * to give it some time.
> > +	 */
> > +	wdat->stopped = false;
> 
> Could the watchdog already be stopped here ? If so, what would be the purpose
> and benefit of stopping it again ? And if it is already stopped, could there
> be conditions where it would not restart in suspend because the stopped flag
> is 0 ?

If it is stopped watchdog_active() check above will skip stopping it
again.

We only set ->stopped if we explictly stopped the watchdog here in
suspend. If the firmware stops it (->stopped_in_sleep != 0) we do
nothing here.

> > +	if (acpi_target_system_state() == ACPI_STATE_S0 ||
> > +	    !wdat->stopped_in_sleep) {
> > +		ret = wdat_wdt_stop(&wdat->wdd);
> > +		if (!ret)
> > +			wdat->stopped = true;
> > +	} else {
> > +		ret = wdat_wdt_ping(&wdat->wdd);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int wdat_wdt_resume_noirq(struct device *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct wdat_wdt *wdat = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	if (!watchdog_active(&wdat->wdd))
> > +		return 0;
> > +
> > +	if (!wdat->stopped) {
> > +		/*
> > +		 * Looks like the boot firmware reinitializes the watchdog
> > +		 * before it hands off to the OS on resume from sleep so we
> > +		 * stop and reprogram the watchdog here.
> 
> And the firmware reprograms it as it sees fit ? Brr ... :-(

Yes.

The below steps were the only ones which worked accross all my test
machines.

> > +		 */
> > +		ret = wdat_wdt_stop(&wdat->wdd);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = wdat_wdt_set_timeout(&wdat->wdd, wdat->wdd.timeout);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = wdat_wdt_enable_reboot(wdat);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = wdat_wdt_ping(&wdat->wdd);
> > +		if (ret)
> > +			return ret;
> 
> The watchdog is already running here. Why start it again ?

No it's not. We stopped it few lines above before we reprogram it.

Re: [PATCH 1/4] ACPI / watchdog: Add support for WDAT hardware watchdog

[Guenter Roeck]

On 09/14/2016 01:06 AM, Mika Westerberg wrote:
> On Tue, Sep 13, 2016 at 02:00:25PM -0700, Guenter Roeck wrote:
>> On 09/13/2016 08:23 AM, Mika Westerberg wrote:
>>> Starting from Intel Skylake the iTCO watchdog timer registers were moved to
>>> reside in the same register space with SMBus host controller.  Not all
>>> needed registers are available though and we need to unhide P2SB (Primary
>>> to Sideband) device briefly to be able to read status of required NO_REBOOT
>>> bit. The i2c-i801.c SMBus driver used to handle this and creation of the
>>> iTCO watchdog platform device.
>>>
>>> Windows, on the other hand, does not use the iTCO watchdog hardware
>>> directly even if it is available. Instead it relies on ACPI Watchdog Action
>>> Table (WDAT) table to describe the watchdog hardware to the OS. This table
>>> contains necessary information about the the hardware and also set of
>>> actions which are executed by a driver as needed.
>>>
>>> This patch implements a new watchdog driver that takes advantage of the
>>> ACPI WDAT table. We split the functionality into two parts: first part
>>> enumerates the WDAT table and if found, populates resources and creates
>>> platform device for the actual driver. The second part is the driver
>>> itself.
>>>
>>> The reason for the split is that this way we can make the driver itself to
>>> be a module and loaded automatically if the WDAT table is found. Otherwise
>>> the module is not loaded.
>>>
>> What I don't really like is that the driver won't be in the watchdog directory,
>> and will thus easily be overlooked in the future by watchdog maintainers.
>
> It is in ACPI directory because we expose the functionality to users as
> "ACPI Watchdog Action Table (WDAT)" which works with other similar table
> options in drivers/acpi/Kconfig.
>
> I'm fine moving the driver itself (wdat_wdt.c) under drivers/watchdog
> but at least the enumeration part should be part of drivers/acpi and I
> would still like to have only one user selectable option.
>

SGTM, but up to you and Wim to decide, really.

>>> +	wdat->wdd.max_timeout = wdat->period * tbl->max_count / 1000;
>>> +	wdat->wdd.min_timeout = wdat->period * tbl->min_count / 1000;
>>
>> Are those guaranteed to be correct, ie max_timeout >= min_timeout
>> and both valid ?
>
> The WDAT spec says nothing about those. I'll add sanity check here and
> return -EINVAL if the values cannot be used. While there I think I can
> do the same for tbl->timer_period, just in case.
>
Using max_hw_heartbeat_ms would help a bit here. Then the actual timeout
is not limited by the hardware maximum, and the watchdog core will ping
the watchdog if the selected timeout is larger than the maximum hardware
timeout.

Not sure how well the core would handle a maximum timeout of 1ms, though,
so some basic sanity checking might still make sense.

>> Reason for asking is that the core will accept any timeouts if, for
>> example, max_timeout is 0.
>>
>>> +	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
>>> +	wdat->wdd.info = &wdat_wdt_info;
>>> +	wdat->wdd.ops = &wdat_wdt_ops;
>>> +	wdat->pdev = pdev;
>>> +
>>> +	/* Request and map all resources */
>>> +	for (i = 0; i < pdev->num_resources; i++) {
>>> +		void __iomem *reg;
>>> +
>>> +		res = &pdev->resource[i];
>>> +		if (resource_type(res) == IORESOURCE_MEM) {
>>> +			reg = devm_ioremap_resource(&pdev->dev, res);
>>> +		} else if (resource_type(res) == IORESOURCE_IO) {
>>> +			reg = devm_ioport_map(&pdev->dev, res->start, 1);
>>> +		} else {
>>> +			dev_err(&pdev->dev, "Unsupported resource\n");
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		if (!reg)
>>> +			return -ENOMEM;
>>> +
>>> +		regs[i] = reg;
>>> +	}
>>> +
>>> +	entries = (struct acpi_wdat_entry *)(tbl + 1);
>>> +	for (i = 0; i < tbl->entries; i++) {
>>> +		const struct acpi_generic_address *gas;
>>> +		struct wdat_instruction *instr;
>>> +		struct list_head *instructions;
>>> +		unsigned int action;
>>> +		struct resource r;
>>> +		int j;
>>> +
>>> +		action = entries[i].action;
>>> +		if (action >= MAX_WDAT_ACTIONS) {
>>> +			dev_dbg(&pdev->dev, "Skipping unknown action: %u\n",
>>> +				action);
>>> +			continue;
>>> +		}
>>> +
>>> +		instr = devm_kzalloc(&pdev->dev, sizeof(*instr), GFP_KERNEL);
>>> +		if (!instr)
>>> +			return -ENOMEM;
>>> +
>>> +		INIT_LIST_HEAD(&instr->node);
>>> +		instr->entry = entries[i];
>>> +
>>> +		gas = &entries[i].register_region;
>>> +
>>> +		memset(&r, 0, sizeof(r));
>>> +		r.start = gas->address;
>>> +		r.end = r.start + gas->access_width;
>>> +		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>>> +			r.flags = IORESOURCE_MEM;
>>> +		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
>>> +			r.flags = IORESOURCE_IO;
>>> +		} else {
>>> +			dev_dbg(&pdev->dev, "Unsupported address space: %d\n",
>>> +				gas->space_id);
>>> +			continue;
>>> +		}
>>> +
>>> +		/* Find the matching resource */
>>> +		for (j = 0; j < pdev->num_resources; j++) {
>>> +			res = &pdev->resource[j];
>>> +			if (resource_contains(res, &r)) {
>>> +				instr->reg = regs[j] + r.start - res->start;
>>> +				break;
>>> +			}
>>> +		}
>>> +
>>> +		if (!instr->reg) {
>>> +			dev_err(&pdev->dev, "I/O resource not found\n");
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		instructions = wdat->instructions[action];
>>> +		if (!instructions) {
>>> +			instructions = devm_kzalloc(&pdev->dev,
>>> +					sizeof(*instructions), GFP_KERNEL);
>>> +			if (!instructions)
>>> +				return -ENOMEM;
>>> +
>>> +			INIT_LIST_HEAD(instructions);
>>> +			wdat->instructions[action] = instructions;
>>> +		}
>>> +
>>> +		list_add_tail(&instr->node, instructions);
>>> +	}
>>> +
>>> +	/* Make sure it is stopped now */
>>> +	ret = wdat_wdt_stop(&wdat->wdd);
>>
>> Why ? You could set max_hw_heartbeat_ms instead of max_timeout and
>> inform the watchdog core that the watchdog is running (by setting
>> the WDOG_HW_RUNNING status flag).
>
> Hmm is the watchdog core then kicking it?
>
> It is stopped here to make sure system does not reboot until userspace
> explicitly opens the device and starts kicking the watchdog.
>

Yes, that functionality was recently added to the watchdog core.

>
>>> +		 */
>>> +		ret = wdat_wdt_stop(&wdat->wdd);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		ret = wdat_wdt_set_timeout(&wdat->wdd, wdat->wdd.timeout);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		ret = wdat_wdt_enable_reboot(wdat);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		ret = wdat_wdt_ping(&wdat->wdd);
>>> +		if (ret)
>>> +			return ret;
>>
>> The watchdog is already running here. Why start it again ?
>
> No it's not. We stopped it few lines above before we reprogram it.
>
But you start it below, don't you ? And it is stopped here, so why ping it ?

If it is necessary to ping the watchdog before starting it,
maybe the start code should do it ?

Thanks,
Guenter

Re: [PATCH 1/4] ACPI / watchdog: Add support for WDAT hardware watchdog

[Mika Westerberg]

On Wed, Sep 14, 2016 at 07:54:34AM -0700, Guenter Roeck wrote:
> On 09/14/2016 01:06 AM, Mika Westerberg wrote:
> > On Tue, Sep 13, 2016 at 02:00:25PM -0700, Guenter Roeck wrote:
> > > On 09/13/2016 08:23 AM, Mika Westerberg wrote:
> > > > Starting from Intel Skylake the iTCO watchdog timer registers were moved to
> > > > reside in the same register space with SMBus host controller.  Not all
> > > > needed registers are available though and we need to unhide P2SB (Primary
> > > > to Sideband) device briefly to be able to read status of required NO_REBOOT
> > > > bit. The i2c-i801.c SMBus driver used to handle this and creation of the
> > > > iTCO watchdog platform device.
> > > > 
> > > > Windows, on the other hand, does not use the iTCO watchdog hardware
> > > > directly even if it is available. Instead it relies on ACPI Watchdog Action
> > > > Table (WDAT) table to describe the watchdog hardware to the OS. This table
> > > > contains necessary information about the the hardware and also set of
> > > > actions which are executed by a driver as needed.
> > > > 
> > > > This patch implements a new watchdog driver that takes advantage of the
> > > > ACPI WDAT table. We split the functionality into two parts: first part
> > > > enumerates the WDAT table and if found, populates resources and creates
> > > > platform device for the actual driver. The second part is the driver
> > > > itself.
> > > > 
> > > > The reason for the split is that this way we can make the driver itself to
> > > > be a module and loaded automatically if the WDAT table is found. Otherwise
> > > > the module is not loaded.
> > > > 
> > > What I don't really like is that the driver won't be in the watchdog directory,
> > > and will thus easily be overlooked in the future by watchdog maintainers.
> > 
> > It is in ACPI directory because we expose the functionality to users as
> > "ACPI Watchdog Action Table (WDAT)" which works with other similar table
> > options in drivers/acpi/Kconfig.
> > 
> > I'm fine moving the driver itself (wdat_wdt.c) under drivers/watchdog
> > but at least the enumeration part should be part of drivers/acpi and I
> > would still like to have only one user selectable option.
> > 
> 
> SGTM, but up to you and Wim to decide, really.
> 
> > > > +	wdat->wdd.max_timeout = wdat->period * tbl->max_count / 1000;
> > > > +	wdat->wdd.min_timeout = wdat->period * tbl->min_count / 1000;
> > > 
> > > Are those guaranteed to be correct, ie max_timeout >= min_timeout
> > > and both valid ?
> > 
> > The WDAT spec says nothing about those. I'll add sanity check here and
> > return -EINVAL if the values cannot be used. While there I think I can
> > do the same for tbl->timer_period, just in case.
> > 
> Using max_hw_heartbeat_ms would help a bit here. Then the actual timeout
> is not limited by the hardware maximum, and the watchdog core will ping
> the watchdog if the selected timeout is larger than the maximum hardware
> timeout.
> 
> Not sure how well the core would handle a maximum timeout of 1ms, though,
> so some basic sanity checking might still make sense.

OK

> > > Reason for asking is that the core will accept any timeouts if, for
> > > example, max_timeout is 0.
> > > 
> > > > +	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
> > > > +	wdat->wdd.info = &wdat_wdt_info;
> > > > +	wdat->wdd.ops = &wdat_wdt_ops;
> > > > +	wdat->pdev = pdev;
> > > > +
> > > > +	/* Request and map all resources */
> > > > +	for (i = 0; i < pdev->num_resources; i++) {
> > > > +		void __iomem *reg;
> > > > +
> > > > +		res = &pdev->resource[i];
> > > > +		if (resource_type(res) == IORESOURCE_MEM) {
> > > > +			reg = devm_ioremap_resource(&pdev->dev, res);
> > > > +		} else if (resource_type(res) == IORESOURCE_IO) {
> > > > +			reg = devm_ioport_map(&pdev->dev, res->start, 1);
> > > > +		} else {
> > > > +			dev_err(&pdev->dev, "Unsupported resource\n");
> > > > +			return -EINVAL;
> > > > +		}
> > > > +
> > > > +		if (!reg)
> > > > +			return -ENOMEM;
> > > > +
> > > > +		regs[i] = reg;
> > > > +	}
> > > > +
> > > > +	entries = (struct acpi_wdat_entry *)(tbl + 1);
> > > > +	for (i = 0; i < tbl->entries; i++) {
> > > > +		const struct acpi_generic_address *gas;
> > > > +		struct wdat_instruction *instr;
> > > > +		struct list_head *instructions;
> > > > +		unsigned int action;
> > > > +		struct resource r;
> > > > +		int j;
> > > > +
> > > > +		action = entries[i].action;
> > > > +		if (action >= MAX_WDAT_ACTIONS) {
> > > > +			dev_dbg(&pdev->dev, "Skipping unknown action: %u\n",
> > > > +				action);
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		instr = devm_kzalloc(&pdev->dev, sizeof(*instr), GFP_KERNEL);
> > > > +		if (!instr)
> > > > +			return -ENOMEM;
> > > > +
> > > > +		INIT_LIST_HEAD(&instr->node);
> > > > +		instr->entry = entries[i];
> > > > +
> > > > +		gas = &entries[i].register_region;
> > > > +
> > > > +		memset(&r, 0, sizeof(r));
> > > > +		r.start = gas->address;
> > > > +		r.end = r.start + gas->access_width;
> > > > +		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> > > > +			r.flags = IORESOURCE_MEM;
> > > > +		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> > > > +			r.flags = IORESOURCE_IO;
> > > > +		} else {
> > > > +			dev_dbg(&pdev->dev, "Unsupported address space: %d\n",
> > > > +				gas->space_id);
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		/* Find the matching resource */
> > > > +		for (j = 0; j < pdev->num_resources; j++) {
> > > > +			res = &pdev->resource[j];
> > > > +			if (resource_contains(res, &r)) {
> > > > +				instr->reg = regs[j] + r.start - res->start;
> > > > +				break;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		if (!instr->reg) {
> > > > +			dev_err(&pdev->dev, "I/O resource not found\n");
> > > > +			return -EINVAL;
> > > > +		}
> > > > +
> > > > +		instructions = wdat->instructions[action];
> > > > +		if (!instructions) {
> > > > +			instructions = devm_kzalloc(&pdev->dev,
> > > > +					sizeof(*instructions), GFP_KERNEL);
> > > > +			if (!instructions)
> > > > +				return -ENOMEM;
> > > > +
> > > > +			INIT_LIST_HEAD(instructions);
> > > > +			wdat->instructions[action] = instructions;
> > > > +		}
> > > > +
> > > > +		list_add_tail(&instr->node, instructions);
> > > > +	}
> > > > +
> > > > +	/* Make sure it is stopped now */
> > > > +	ret = wdat_wdt_stop(&wdat->wdd);
> > > 
> > > Why ? You could set max_hw_heartbeat_ms instead of max_timeout and
> > > inform the watchdog core that the watchdog is running (by setting
> > > the WDOG_HW_RUNNING status flag).
> > 
> > Hmm is the watchdog core then kicking it?
> > 
> > It is stopped here to make sure system does not reboot until userspace
> > explicitly opens the device and starts kicking the watchdog.
> > 
> 
> Yes, that functionality was recently added to the watchdog core.

Cool. So then I'll just set WDOG_HW_RUNNING and drop stopping the
watchdog here.

> > > > +		 */
> > > > +		ret = wdat_wdt_stop(&wdat->wdd);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		ret = wdat_wdt_set_timeout(&wdat->wdd, wdat->wdd.timeout);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		ret = wdat_wdt_enable_reboot(wdat);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		ret = wdat_wdt_ping(&wdat->wdd);
> > > > +		if (ret)
> > > > +			return ret;
> > > 
> > > The watchdog is already running here. Why start it again ?
> > 
> > No it's not. We stopped it few lines above before we reprogram it.
> > 
> But you start it below, don't you ? And it is stopped here, so why ping it ?
> 
> If it is necessary to ping the watchdog before starting it,
> maybe the start code should do it ?

Now that you mentioned, I don't immediately remember why we ping it
here. It should not be necessary at this point. I'll remove that call in
next revision.

[PATCH 2/4] mfd: lpc_ich: Do not create iTCO watchdog when WDAT table exists

[Mika Westerberg]

ACPI WDAT table is the preferred way to use hardware watchdog over the
native iTCO_wdt. Windows only uses this table for its hardware watchdog
implementation so we should be relatively safe to trust it has been
validated by OEMs

Prevent iTCO watchdog creation if we detect that there is ACPI WDAT table.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/mfd/lpc_ich.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index bd3aa4578346..c8dee47b45d9 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -984,6 +984,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
 	int ret;
 	struct resource *res;
 
+	/* If we have ACPI based watchdog use that instead */
+	if (acpi_has_watchdog())
+		return -ENODEV;
+
 	/* Setup power management base register */
 	pci_read_config_dword(dev, priv->abase, &base_addr_cfg);
 	base_addr = base_addr_cfg & 0x0000ff80;
-- 
2.9.3

Re: [PATCH 2/4] mfd: lpc_ich: Do not create iTCO watchdog when WDAT table exists

[Guenter Roeck]

On Tue, Sep 13, 2016 at 06:23:34PM +0300, Mika Westerberg wrote:
> ACPI WDAT table is the preferred way to use hardware watchdog over the
> native iTCO_wdt. Windows only uses this table for its hardware watchdog
> implementation so we should be relatively safe to trust it has been
> validated by OEMs
> 
> Prevent iTCO watchdog creation if we detect that there is ACPI WDAT table.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

> ---
>  drivers/mfd/lpc_ich.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index bd3aa4578346..c8dee47b45d9 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -984,6 +984,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  	int ret;
>  	struct resource *res;
>  
> +	/* If we have ACPI based watchdog use that instead */
> +	if (acpi_has_watchdog())
> +		return -ENODEV;
> +
>  	/* Setup power management base register */
>  	pci_read_config_dword(dev, priv->abase, &base_addr_cfg);
>  	base_addr = base_addr_cfg & 0x0000ff80;
> -- 
> 2.9.3
> 

[PATCH 3/4] i2c: i801: Do not create iTCO watchdog when WDAT table exists

[Mika Westerberg]

ACPI WDAT table is the preferred way to use hardware watchdog over the
native iTCO_wdt. Windows only uses this table for its hardware watchdog
implementation so we should be relatively safe to trust it has been
validated by OEMs

Prevent iTCO watchdog creation if we detect that there is ACPI WDAT table.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-i801.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ef9b733d153..26298af73232 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1486,7 +1486,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		priv->features |= FEATURE_IRQ;
 		priv->features |= FEATURE_SMBUS_PEC;
 		priv->features |= FEATURE_BLOCK_BUFFER;
-		priv->features |= FEATURE_TCO;
+		/* If we have ACPI based watchdog use that instead */
+		if (!acpi_has_watchdog())
+			priv->features |= FEATURE_TCO;
 		priv->features |= FEATURE_HOST_NOTIFY;
 		break;
 
-- 
2.9.3

Re: [PATCH 3/4] i2c: i801: Do not create iTCO watchdog when WDAT table exists

[Guenter Roeck]

On Tue, Sep 13, 2016 at 06:23:35PM +0300, Mika Westerberg wrote:
> ACPI WDAT table is the preferred way to use hardware watchdog over the
> native iTCO_wdt. Windows only uses this table for its hardware watchdog
> implementation so we should be relatively safe to trust it has been
> validated by OEMs
> 
> Prevent iTCO watchdog creation if we detect that there is ACPI WDAT table.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

> ---
>  drivers/i2c/busses/i2c-i801.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 5ef9b733d153..26298af73232 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1486,7 +1486,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		priv->features |= FEATURE_IRQ;
>  		priv->features |= FEATURE_SMBUS_PEC;
>  		priv->features |= FEATURE_BLOCK_BUFFER;
> -		priv->features |= FEATURE_TCO;
> +		/* If we have ACPI based watchdog use that instead */
> +		if (!acpi_has_watchdog())
> +			priv->features |= FEATURE_TCO;
>  		priv->features |= FEATURE_HOST_NOTIFY;
>  		break;
>  
> -- 
> 2.9.3
> 

[PATCH 4/4] platform/x86: intel_pmc_ipc: Do not create iTCO watchdog when WDAT table exists

[Mika Westerberg]

ACPI WDAT table is the preferred way to use hardware watchdog over the
native iTCO_wdt. Windows only uses this table for its hardware watchdog
implementation so we should be relatively safe to trust it has been
validated by OEMs.

Prevent iTCO watchdog creation if we detect that there is an ACPI WDAT
table.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_ipc.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index b86e1bcaa055..a511d518206b 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -651,11 +651,15 @@ static int ipc_create_pmc_devices(void)
 {
 	int ret;
 
-	ret = ipc_create_tco_device();
-	if (ret) {
-		dev_err(ipcdev.dev, "Failed to add tco platform device\n");
-		return ret;
+	/* If we have ACPI based watchdog use that instead */
+	if (!acpi_has_watchdog()) {
+		ret = ipc_create_tco_device();
+		if (ret) {
+			dev_err(ipcdev.dev, "Failed to add tco platform device\n");
+			return ret;
+		}
 	}
+
 	ret = ipc_create_punit_device();
 	if (ret) {
 		dev_err(ipcdev.dev, "Failed to add punit platform device\n");
-- 
2.9.3

Re: [PATCH 4/4] platform/x86: intel_pmc_ipc: Do not create iTCO watchdog when WDAT table exists

[Guenter Roeck]

On Tue, Sep 13, 2016 at 06:23:36PM +0300, Mika Westerberg wrote:
> ACPI WDAT table is the preferred way to use hardware watchdog over the
> native iTCO_wdt. Windows only uses this table for its hardware watchdog
> implementation so we should be relatively safe to trust it has been
> validated by OEMs.
> 
> Prevent iTCO watchdog creation if we detect that there is an ACPI WDAT
> table.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

> ---
>  drivers/platform/x86/intel_pmc_ipc.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index b86e1bcaa055..a511d518206b 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -651,11 +651,15 @@ static int ipc_create_pmc_devices(void)
>  {
>  	int ret;
>  
> -	ret = ipc_create_tco_device();
> -	if (ret) {
> -		dev_err(ipcdev.dev, "Failed to add tco platform device\n");
> -		return ret;
> +	/* If we have ACPI based watchdog use that instead */
> +	if (!acpi_has_watchdog()) {
> +		ret = ipc_create_tco_device();
> +		if (ret) {
> +			dev_err(ipcdev.dev, "Failed to add tco platform device\n");
> +			return ret;
> +		}
>  	}
> +
>  	ret = ipc_create_punit_device();
>  	if (ret) {
>  		dev_err(ipcdev.dev, "Failed to add punit platform device\n");
> -- 
> 2.9.3
>