[PATCH v2] drivers/watchdog: add amdfch_wdt watchdog timer driver for AMD

[Arsalan H. Awan]

This adds support for AMD FCH Watchdog in efibootguard.

This watchdog is known to exist on the following AMD platforms:

* AMD Ryzen Embedded V1000 (V-Series APU)
* AMD Ryzen Embedded R1000 (R-Series APU)
* AMD EPYC Embedded E3000 (E-Series CPU)

Signed-off-by: Arsalan H. Awan <Arsala...@mentor.com>
---
Makefile.am | 1 +
drivers/watchdog/amdfch_wdt.c | 290 ++++++++++++++++++++++++++++++++++
2 files changed, 291 insertions(+)
create mode 100644 drivers/watchdog/amdfch_wdt.c

diff --git a/Makefile.am b/Makefile.am
index 74fa7f0..0496384 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -123,6 +123,7 @@ efi_loadername = efibootguard$(MACHINE_TYPE_NAME).efi

efi_sources = \
drivers/watchdog/init_array_start.S \
+ drivers/watchdog/amdfch_wdt.c \
drivers/watchdog/i6300esb.c \
drivers/watchdog/atom-quark.c \
drivers/watchdog/itco.c \
diff --git a/drivers/watchdog/amdfch_wdt.c b/drivers/watchdog/amdfch_wdt.c
new file mode 100644
index 0000000..e9b7451
--- /dev/null
+++ b/drivers/watchdog/amdfch_wdt.c
@@ -0,0 +1,290 @@
+/*
+ * EFI Boot Guard
+ *
+ * Copyright (C) 2020 Mentor Graphics, A Siemens business
+ *
+ * Authors:
+ * Arsalan H. Awan <Arsala...@mentor.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <efi.h>
+#include <efilib.h>
+#include <pci/header.h>
+
+/* #define AMDFCH_WDT_DEBUG */
+
+#define AMDFCH_WDT_MIN_TIMEOUT 0x0001 /* minimum timeout value */
+#define AMDFCH_WDT_MAX_TIMEOUT 0xFFFF /* maximum timeout value */
+
+/* Watchdog register definitions */
+#define AMD_ACPI_MMIO_BASE 0xFED80000
+#define AMDFCH_WDT_MEM_MAP_OFFSET 0xB00
+#define AMDFCH_WDT_MEM_MAP_SIZE 0x100
+
+#define AMDFCH_WDT_CONTROL(base) ((base) + 0x00) /* Watchdog Control */
+ #define AMDFCH_WDT_START_STOP_BIT (1 << 0)
+ #define AMDFCH_WDT_FIRED_BIT (1 << 1)
+ #define AMDFCH_WDT_ACTION_RESET_BIT (1 << 2)
+ #define AMDFCH_WDT_DISABLE_BIT (1 << 3)
+ /* 6:4 bits Reserved */
+ #define AMDFCH_WDT_TRIGGER_BIT (1 << 7)
+#define AMDFCH_WDT_COUNT(base) ((base) + 0x04) /* Watchdog Count */
+ #define AMDFCH_WDT_COUNT_MASK 0xFFFF
+
+#define AMD_PM_WATCHDOG_EN_REG 0x00
+ #define AMD_PM_WATCHDOG_TIMER_EN (0x01 << 7)
+
+#define AMD_PM_WATCHDOG_CONFIG_REG 0x03
+ #define AMD_PM_WATCHDOG_32USEC_RES 0x0
+ #define AMD_PM_WATCHDOG_10MSEC_RES 0x1
+ #define AMD_PM_WATCHDOG_100MSEC_RES 0x2
+ #define AMD_PM_WATCHDOG_1SEC_RES 0x3
+#define AMD_PM_WATCHDOG_CONFIG_MASK 0x3
+
+/* IO port address for indirect access using ACPI PM registers */
+#define AMD_IO_PM_INDEX_REG 0xCD6
+#define AMD_IO_PM_DATA_REG 0xCD7
+
+#define PCI_DEVICE_ID_AMD_CARRIZO_SMBUS 0x790B
+#define PCI_VENDOR_ID_AMD 0x1022
+
+static struct
+{
+ UINTN base;
+ EFI_PCI_IO *pci_io;
+} watchdog;
+
+static EFI_STATUS writel (UINT32 val, UINT32 addr)
+{
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"\n- writel (%X, %X) ", val, addr);
+#endif
+ EFI_STATUS status;
+
+ status = uefi_call_wrapper(watchdog.pci_io->Mem.Write, 6, watchdog.pci_io,
+ EfiPciIoWidthUint32,
+ EFI_PCI_IO_PASS_THROUGH_BAR,
+ addr, 1, &val);
+
+ if (EFI_ERROR(status)) {
+ Print(L"Error while writel (%X, %X): %r", val, addr, status);
+ }
+
+ return status;
+}
+
+static UINT32 readl (UINT32 addr)
+{
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"\n- readl (%X) ", addr);
+#endif
+ UINT32 val = 0;
+ EFI_STATUS status;
+
+ status = uefi_call_wrapper(watchdog.pci_io->Mem.Read, 6, watchdog.pci_io,
+ EfiPciIoWidthUint32,
+ EFI_PCI_IO_PASS_THROUGH_BAR,
+ addr, 1, &val);
+
+ if (EFI_ERROR(status)) {
+ Print(L"Error while readl (%X): %r", addr, status);
+ }
+
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"= %X ", val);
+#endif
+ return val;
+}
+
+static EFI_STATUS outb (UINT8 val, UINT16 reg)
+{
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"\n- outb (%X, %X) ", val, reg);
+#endif
+ EFI_STATUS status;
+
+ status = uefi_call_wrapper(watchdog.pci_io->Io.Write, 6, watchdog.pci_io,
+ EfiPciIoWidthUint8,
+ EFI_PCI_IO_PASS_THROUGH_BAR,
+ reg, 1, &val);
+ if (EFI_ERROR(status)) {
+ Print(L"Error while outb (%X, %X): %r", val, reg, status);
+ }
+
+ return status;
+}
+
+static UINT8 inb (UINT16 reg)
+{
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"\n- inb (%X) ", reg);
+#endif
+ EFI_STATUS status;
+ UINT8 val = 0;
+
+ status = uefi_call_wrapper(watchdog.pci_io->Io.Read, 6, watchdog.pci_io,
+ EfiPciIoWidthUint8,
+ EFI_PCI_IO_PASS_THROUGH_BAR,
+ reg, 1, &val);
+ if (EFI_ERROR(status)) {
+ Print(L"Error while inb (%X): %r", reg, status);
+ }
+
+#ifdef AMDFCH_WDT_DEBUG
+ Print(L"= %X ", val);
+#endif
+ return val;
+}
+
+static EFI_STATUS amdfch_wdt_enable (VOID)
+{
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"\n-- amdfch_wdt_enable() ");
+#endif
+ UINT8 val;
+ /* Enable watchdog timer */
+ outb(AMD_PM_WATCHDOG_EN_REG, AMD_IO_PM_INDEX_REG);
+ val = inb(AMD_IO_PM_DATA_REG);
+ val |= AMD_PM_WATCHDOG_TIMER_EN;
+ return outb(val, AMD_IO_PM_DATA_REG);
+}
+
+static EFI_STATUS amdfch_wdt_set_resolution (UINT8 freq)
+{
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"\n-- amdfch_wdt_set_resolution(%d) ", freq);
+#endif
+ UINT8 val;
+ /* Set the watchdog timer resolution */
+ outb(AMD_PM_WATCHDOG_CONFIG_REG, AMD_IO_PM_INDEX_REG);
+ val = inb(AMD_IO_PM_DATA_REG);
+ /* Clear the previous frequency setting, if any */
+ val &= ~AMD_PM_WATCHDOG_CONFIG_MASK;
+ /* Set the new frequency value */
+ val |= freq;
+ return outb(val, AMD_IO_PM_DATA_REG);
+}
+
+static EFI_STATUS amdfch_wdt_set_timeout_action (CONST CHAR16 * action)
+{
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"\n-- amdfch_wdt_set_timeout_action(%s) ", action);
+#endif
+ UINT32 val;
+ val = readl (AMDFCH_WDT_CONTROL(watchdog.base));
+
+ /*
+ * Set the watchdog timeout action.
+ *
+ * If action is specified anything other than reboot or shutdown,
+ * we default it to reboot.
+ */
+ if (StrnCmp(action, L"shutdown", 8) == 0)
+ val |= AMDFCH_WDT_ACTION_RESET_BIT;
+ else
+ val &= ~AMDFCH_WDT_ACTION_RESET_BIT;
+
+ return writel (val, AMDFCH_WDT_CONTROL(watchdog.base));
+}
+
+static EFI_STATUS amdfch_wdt_set_time (UINT32 t)
+{
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"\n-- amdfch_wdt_set_time(%d) ", t);
+#endif
+ if (t < AMDFCH_WDT_MIN_TIMEOUT)
+ t = AMDFCH_WDT_MIN_TIMEOUT;
+ else if (t > AMDFCH_WDT_MAX_TIMEOUT)
+ t = AMDFCH_WDT_MAX_TIMEOUT;
+
+ /* Write new timeout value to watchdog COUNT register */
+ return writel (t, AMDFCH_WDT_COUNT(watchdog.base));
+}
+
+static EFI_STATUS amdfch_wdt_start (VOID)
+{
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"\n-- amdfch_wdt_start() ");
+#endif
+ UINT32 val;
+ /* Start the watchdog timer */
+ val = readl (AMDFCH_WDT_CONTROL(watchdog.base));
+ val |= AMDFCH_WDT_START_STOP_BIT;
+ return writel (val, AMDFCH_WDT_CONTROL(watchdog.base));
+}
+
+static EFI_STATUS amdfch_wdt_ping (VOID)
+{
+#ifdef AMDFCH_WDT_DEBUG
+ Print (L"\n-- amdfch_wdt_ping() ");
+#endif
+ UINT32 val;
+ /* Trigger/Ping watchdog timer */
+ val = readl (AMDFCH_WDT_CONTROL(watchdog.base));
+ val |= AMDFCH_WDT_TRIGGER_BIT;
+ return writel (val, AMDFCH_WDT_CONTROL(watchdog.base));
+}
+
+static EFI_STATUS __attribute__((constructor))
+init (EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
+ UINTN timeout)
+{
+ EFI_STATUS status;
+
+ if (!pci_io || pci_vendor_id != PCI_VENDOR_ID_AMD ||
+ pci_device_id != PCI_DEVICE_ID_AMD_CARRIZO_SMBUS) {
+ return EFI_UNSUPPORTED;
+ }
+
+ watchdog.base = AMD_ACPI_MMIO_BASE + AMDFCH_WDT_MEM_MAP_OFFSET;
+ watchdog.pci_io = pci_io;
+
+ Print(L"Detected AMD FCH Watchdog Timer\n");
+
+#ifdef AMDFCH_WDT_DEBUG
+ Print(L"\nwatchdog.base = %X\n", watchdog.base);
+#endif
+
+ status = amdfch_wdt_enable ();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_enable () failed.");
+ return status;
+ }
+
+ status = amdfch_wdt_set_resolution (AMD_PM_WATCHDOG_1SEC_RES);
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_set_resolution (%d) failed.", AMD_PM_WATCHDOG_1SEC_RES);
+ return status;
+ }
+
+ status = amdfch_wdt_set_timeout_action (L"reboot");
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_set_timeout_action (\"reboot\") failed.");
+ return status;
+ }
+
+ status = amdfch_wdt_set_time (timeout);
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_set_time (%d) failed.", timeout);
+ return status;
+ }
+
+ status = amdfch_wdt_start ();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_start () failed.");
+ return status;
+ }
+
+ status = amdfch_wdt_ping ();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_ping () failed.");
+ return status;
+ }
+
+ return status;
+}
--
2.17.1

[Jan Kiszka]

Thanks for the update, but I'm not seeing all my comments addressed, e.g.

- coding style (no "function (...)", rather "function(...)")
- DebugPrintf, rather than #ifdef'ery
- unclear relationship to other hardward under 0x790B, see upstream
kernel driver
- no device revision ID match

possibly more. Please double-check all what I wrote.

Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[Arsalan H. Awan]

This adds support for AMD FCH Watchdog in efibootguard.

This watchdog is known to exist on the following AMD platforms:

* AMD Ryzen Embedded V1000 (V-Series APU)
* AMD Ryzen Embedded R1000 (R-Series APU)
* AMD EPYC Embedded E3000 (E-Series CPU)

Signed-off-by: Arsalan H. Awan <Arsala...@mentor.com>
---
Makefile.am | 1 +

drivers/watchdog/amdfch_wdt.c | 288 ++++++++++++++++++++++++++++++++++
2 files changed, 289 insertions(+)

create mode 100644 drivers/watchdog/amdfch_wdt.c

diff --git a/Makefile.am b/Makefile.am
index 74fa7f0..0496384 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -123,6 +123,7 @@ efi_loadername = efibootguard$(MACHINE_TYPE_NAME).efi

efi_sources = \
drivers/watchdog/init_array_start.S \
+ drivers/watchdog/amdfch_wdt.c \
drivers/watchdog/i6300esb.c \
drivers/watchdog/atom-quark.c \
drivers/watchdog/itco.c \
diff --git a/drivers/watchdog/amdfch_wdt.c b/drivers/watchdog/amdfch_wdt.c
new file mode 100644

index 0000000..0956a25
--- /dev/null
+++ b/drivers/watchdog/amdfch_wdt.c
@@ -0,0 +1,288 @@

+/*
+ * EFI Boot Guard
+ *
+ * Copyright (C) 2020 Mentor Graphics, A Siemens business
+ *
+ * Authors:
+ * Arsalan H. Awan <Arsala...@mentor.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <efi.h>
+#include <efilib.h>
+#include <pci/header.h>
+
+/* #define AMDFCH_WDT_DEBUG */
+

+#ifdef AMDFCH_WDT_DEBUG
+#define DebugPrint(fmt, ...) Print(fmt, ##__VA_ARGS__)
+#else
+#define DebugPrint(fmt, ...) {}
+#endif

+#define PCI_REVISION_ID_REG 0x8

+
+#define PCI_DEVICE_ID_AMD_CARRIZO_SMBUS 0x790B
+#define PCI_VENDOR_ID_AMD 0x1022
+
+static struct
+{
+ UINTN base;
+ EFI_PCI_IO *pci_io;
+} watchdog;
+
+static EFI_STATUS writel(UINT32 val, UINT32 addr)
+{

+ DebugPrint(L"\n- writel(0x%X, 0x%X) ", val, addr);

+ EFI_STATUS status;
+
+ status = uefi_call_wrapper(watchdog.pci_io->Mem.Write, 6, watchdog.pci_io,
+ EfiPciIoWidthUint32,
+ EFI_PCI_IO_PASS_THROUGH_BAR,
+ addr, 1, &val);
+
+ if (EFI_ERROR(status)) {

+ Print(L"Error while writel(0x%X, 0x%X): %r", val, addr, status);

+ }
+
+ return status;
+}
+
+static UINT32 readl(UINT32 addr)
+{

+ DebugPrint(L"\n- readl(0x%X) ", addr);

+ UINT32 val = 0;
+ EFI_STATUS status;
+
+ status = uefi_call_wrapper(watchdog.pci_io->Mem.Read, 6, watchdog.pci_io,
+ EfiPciIoWidthUint32,
+ EFI_PCI_IO_PASS_THROUGH_BAR,
+ addr, 1, &val);
+
+ if (EFI_ERROR(status)) {

+ Print(L"Error while readl(0x%X): %r", addr, status);
+ }
+
+ DebugPrint(L"= 0x%X ", val);

+ return val;
+}
+
+static EFI_STATUS outb(UINT8 val, UINT16 reg)
+{

+ DebugPrint(L"\n- outb(0x%X, 0x%X) ", val, reg);

+ EFI_STATUS status;
+
+ status = uefi_call_wrapper(watchdog.pci_io->Io.Write, 6, watchdog.pci_io,
+ EfiPciIoWidthUint8,
+ EFI_PCI_IO_PASS_THROUGH_BAR,
+ reg, 1, &val);
+ if (EFI_ERROR(status)) {

+ Print(L"Error while outb(0x%X, 0x%X): %r", val, reg, status);

+ }
+
+ return status;
+}
+
+static UINT8 inb(UINT16 reg)
+{

+ DebugPrint(L"\n- inb(0x%X) ", reg);

+ EFI_STATUS status;
+ UINT8 val = 0;
+
+ status = uefi_call_wrapper(watchdog.pci_io->Io.Read, 6, watchdog.pci_io,
+ EfiPciIoWidthUint8,
+ EFI_PCI_IO_PASS_THROUGH_BAR,
+ reg, 1, &val);
+ if (EFI_ERROR(status)) {

+ Print(L"Error while inb(0x%X): %r", reg, status);
+ }
+
+ DebugPrint(L"= 0x%X ", val);

+ return val;
+}
+

+static EFI_STATUS amdfch_wdt_enable(VOID)
+{
+ DebugPrint(L"\n-- amdfch_wdt_enable() ");

+ UINT8 val;
+ /* Enable watchdog timer */
+ outb(AMD_PM_WATCHDOG_EN_REG, AMD_IO_PM_INDEX_REG);
+ val = inb(AMD_IO_PM_DATA_REG);
+ val |= AMD_PM_WATCHDOG_TIMER_EN;
+ return outb(val, AMD_IO_PM_DATA_REG);
+}
+
+static EFI_STATUS amdfch_wdt_set_resolution(UINT8 freq)
+{

+ DebugPrint(L"\n-- amdfch_wdt_set_resolution(%d) ", freq);

+ UINT8 val;
+ /* Set the watchdog timer resolution */
+ outb(AMD_PM_WATCHDOG_CONFIG_REG, AMD_IO_PM_INDEX_REG);
+ val = inb(AMD_IO_PM_DATA_REG);
+ /* Clear the previous frequency setting, if any */
+ val &= ~AMD_PM_WATCHDOG_CONFIG_MASK;
+ /* Set the new frequency value */
+ val |= freq;
+ return outb(val, AMD_IO_PM_DATA_REG);
+}
+
+static EFI_STATUS amdfch_wdt_set_timeout_action(CONST CHAR16 * action)
+{

+ DebugPrint(L"\n-- amdfch_wdt_set_timeout_action(%s) ", action);

+ UINT32 val;
+ val = readl(AMDFCH_WDT_CONTROL(watchdog.base));
+
+ /*
+ * Set the watchdog timeout action.
+ *
+ * If action is specified anything other than reboot or shutdown,
+ * we default it to reboot.
+ */
+ if (StrnCmp(action, L"shutdown", 8) == 0)
+ val |= AMDFCH_WDT_ACTION_RESET_BIT;
+ else
+ val &= ~AMDFCH_WDT_ACTION_RESET_BIT;
+

+ return writel(val, AMDFCH_WDT_CONTROL(watchdog.base));

+}
+
+static EFI_STATUS amdfch_wdt_set_time(UINT32 t)
+{

+ DebugPrint(L"\n-- amdfch_wdt_set_time(%d) ", t);

+ if (t < AMDFCH_WDT_MIN_TIMEOUT)
+ t = AMDFCH_WDT_MIN_TIMEOUT;
+ else if (t > AMDFCH_WDT_MAX_TIMEOUT)
+ t = AMDFCH_WDT_MAX_TIMEOUT;
+
+ /* Write new timeout value to watchdog COUNT register */

+ return writel(t, AMDFCH_WDT_COUNT(watchdog.base));

+}
+
+static EFI_STATUS amdfch_wdt_start(VOID)
+{

+ DebugPrint(L"\n-- amdfch_wdt_start() ");

+ UINT32 val;
+ /* Start the watchdog timer */
+ val = readl(AMDFCH_WDT_CONTROL(watchdog.base));
+ val |= AMDFCH_WDT_START_STOP_BIT;

+ return writel(val, AMDFCH_WDT_CONTROL(watchdog.base));
+}
+
+static EFI_STATUS amdfch_wdt_ping(VOID)
+{
+ DebugPrint(L"\n-- amdfch_wdt_ping() ");

+ UINT32 val;
+ /* Trigger/Ping watchdog timer */
+ val = readl(AMDFCH_WDT_CONTROL(watchdog.base));
+ val |= AMDFCH_WDT_TRIGGER_BIT;

+ return writel(val, AMDFCH_WDT_CONTROL(watchdog.base));

+}
+
+static EFI_STATUS __attribute__((constructor))

+init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,

+ UINTN timeout)
+{
+ EFI_STATUS status;

+ UINT8 pci_revision_id = 0;

+
+ if (!pci_io || pci_vendor_id != PCI_VENDOR_ID_AMD ||
+ pci_device_id != PCI_DEVICE_ID_AMD_CARRIZO_SMBUS) {
+ return EFI_UNSUPPORTED;
+ }
+

+ status = uefi_call_wrapper(pci_io->Pci.Read, 5, pci_io,
+ EfiPciIoWidthUint8, PCI_REVISION_ID_REG,
+ 1, &pci_revision_id);
+ if (EFI_ERROR(status)) {

+ return EFI_UNSUPPORTED;
+ }
+

+ switch (pci_revision_id) {
+ case 0x61:
+ case 0x59:
+ Print(L"\nDetected AMD FCH Watchdog Timer (rev %X)\n", pci_revision_id);
+ break;
+ default:
+ Print(L"\nError: Detected Unknown AMD FCH Watchdog Timer\n");

+ return EFI_UNSUPPORTED;
+ }
+
+ watchdog.base = AMD_ACPI_MMIO_BASE + AMDFCH_WDT_MEM_MAP_OFFSET;
+ watchdog.pci_io = pci_io;
+

+ DebugPrint(L"\nwatchdog.base = 0x%X\n", watchdog.base);
+
+ status = amdfch_wdt_enable();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_enable() failed.");

+ return status;
+ }
+
+ status = amdfch_wdt_set_resolution(AMD_PM_WATCHDOG_1SEC_RES);

+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_set_resolution(%d) failed.", AMD_PM_WATCHDOG_1SEC_RES);

+ return status;
+ }
+

+ status = amdfch_wdt_set_timeout_action(L"reboot");
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_set_timeout_action(\"reboot\") failed.");

+ return status;
+ }
+
+ status = amdfch_wdt_set_time(timeout);

+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_set_time(%d) failed.", timeout);

+ return status;
+ }
+
+ status = amdfch_wdt_start();

+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_start() failed.");

+ return status;
+ }
+

+ status = amdfch_wdt_ping();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_ping() failed.");

+ return status;
+ }
+
+ return status;
+}

--
2.17.1

[Jan Kiszka]

Just realized that you nicely propagate write errors down to the init
handler, and such errors will then cause the core to try another driver
or fail loudly there. However, read errors are only printed, not
propagated. That's because you kept the original readl signature (while
changing that of writel).

Same here.

BTW, not for this patch, can be done at a later point: The in/out and
read/write wrappers are actually generic and could probably be reused by
other drivers. We could factor them out. Again, later.

Why are you making this configurable? There is no use case for
"shutdown" here (and, as mentioned in the first review, strings would be
a weird API).

Otherwise, this looks good now.

Tomorrow, I will receive an R1505G-based mini-itx board, and on Friday
also the RAM modules... Then I will be able test as well (not only EBG).
I'm still curious what may prevent the mainline watchdog driver from
working. Will be important for real designs, when you are using standard
mainline or distro kernels (which is rather common on x86) to pick up
the running watchdog.

[Arsalan H. Awan]

This adds support for AMD FCH Watchdog in efibootguard.

This watchdog is known to exist on the following AMD platforms:

* AMD Ryzen Embedded V1000 (V-Series APU)
* AMD Ryzen Embedded R1000 (R-Series APU)
* AMD EPYC Embedded E3000 (E-Series CPU)

Signed-off-by: Arsalan H. Awan <Arsala...@mentor.com>
---
Makefile.am | 1 +

drivers/watchdog/amdfch_wdt.c | 278 ++++++++++++++++++++++++++++++++++
2 files changed, 279 insertions(+)

create mode 100644 drivers/watchdog/amdfch_wdt.c

diff --git a/Makefile.am b/Makefile.am
index 74fa7f0..0496384 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -123,6 +123,7 @@ efi_loadername = efibootguard$(MACHINE_TYPE_NAME).efi

efi_sources = \
drivers/watchdog/init_array_start.S \
+ drivers/watchdog/amdfch_wdt.c \
drivers/watchdog/i6300esb.c \
drivers/watchdog/atom-quark.c \
drivers/watchdog/itco.c \
diff --git a/drivers/watchdog/amdfch_wdt.c b/drivers/watchdog/amdfch_wdt.c
new file mode 100644

index 0000000..5b1cec0
--- /dev/null
+++ b/drivers/watchdog/amdfch_wdt.c
@@ -0,0 +1,278 @@

+static EFI_STATUS amdfch_wdt_set_timeout_action_reboot(VOID)
+{
+ DebugPrint(L"\n-- amdfch_wdt_set_timeout_action_reboot() ");
+ UINT32 val;
+ /* Set the watchdog timeout action to reboot */
+ val = readl(AMDFCH_WDT_CONTROL(watchdog.base));
+ val &= ~AMDFCH_WDT_ACTION_RESET_BIT;

+ case 0x59:
+ case 0x61:

+ Print(L"\nDetected AMD FCH Watchdog Timer (rev %X)\n", pci_revision_id);
+ break;
+ default:
+ Print(L"\nError: Detected Unknown AMD FCH Watchdog Timer\n");
+ return EFI_UNSUPPORTED;
+ }
+
+ watchdog.base = AMD_ACPI_MMIO_BASE + AMDFCH_WDT_MEM_MAP_OFFSET;
+ watchdog.pci_io = pci_io;
+
+ DebugPrint(L"\nwatchdog.base = 0x%X\n", watchdog.base);
+
+ status = amdfch_wdt_enable();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_enable() failed.");
+ return status;
+ }
+
+ status = amdfch_wdt_set_resolution(AMD_PM_WATCHDOG_1SEC_RES);
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_set_resolution(%d) failed.", AMD_PM_WATCHDOG_1SEC_RES);
+ return status;
+ }
+

+ status = amdfch_wdt_set_timeout_action_reboot();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_set_timeout_action_reboot() failed.");

+ return status;
+ }
+
+ status = amdfch_wdt_set_time(timeout);
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_set_time(%d) failed.", timeout);
+ return status;
+ }
+
+ status = amdfch_wdt_start();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_start() failed.");
+ return status;
+ }
+
+ status = amdfch_wdt_ping();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amdfch_wdt_ping() failed.");
+ return status;
+ }
+
+ return status;
+}

--
2.17.1

[Jan Kiszka]

OK, just missed my first review comment. But we can sort out read/in
error handling while factoring out those helpers in-tree.

Applied to next.

Thanks,

[Awan, Arsalan]

> OK, just missed my first review comment. But we can sort out read/in
> error handling while factoring out those helpers in-tree.
>
> Applied to next.
>

Arsalan:
Appreciate that! Thanks!

-
Arsalan
Mentor Graphics - a SIemens business
Mentor Embedded Linux

[Jan Kiszka]

On 02.09.20 12:00, Awan, Arsalan wrote:
>> OK, just missed my first review comment. But we can sort out read/in
>> error handling while factoring out those helpers in-tree.
>>
>> Applied to next.
>>
> Arsalan:
> Appreciate that! Thanks!
>

Some update after trying out the driver on my new R1505G. The good news:

- efibootguard detects and configures the watchdog correctly

- the upstream kernel driver picks up the watchdog - if not, the system
resets after the timeout

The bad ones:

- there is no lock-down of the watchdog, and a distro kernel will
simply turn it off until some watchdog service may pick it up again

- the upstream driver only works after efibootguard enabled the
watchdog - likely fixable, though, and likely the reason why the
misunderstanding arose that the kernel does not support this
hardware

Did you check the specifications for a potential hw-assisted locking /
no-way-out mode? Not having that significantly decreases the value of
this watchdog unfortunately.

It could be partially mitigated by changing the kernel driver to detect
a running watchdog and avoid turning it off unless explicitly requested.
That what w83627hf_wdt.c does e.g. I have that one on my board as well,
and it also lacks any lock-down support.

Jan

[Awan, Arsalan]

>>> OK, just missed my first review comment. But we can sort out read/in
>>> error handling while factoring out those helpers in-tree.
>>>
>>> Applied to next.
>>>
>> Arsalan:
>> Appreciate that! Thanks!
>>
>
>Some update after trying out the driver on my new R1505G. The good news:
>
> - efibootguard detects and configures the watchdog correctly
>
> - the upstream kernel driver picks up the watchdog - if not, the system
> resets after the timeout
>

Arsalan:
I noticed that the other day. The kernel was picking sp5100 driver + our amd_wdt kernel module to get things working.
Without the amd_wdt kernel module OR the amdfch_wdt efibootguard driver, the WDT won't work.

>The bad ones:
>
> - there is no lock-down of the watchdog, and a distro kernel will
> simply turn it off until some watchdog service may pick it up again
>

Arsalan:
Yes you're right.
However, the kernel driver sp5100 has this nowayout functionality implemented though. Probably a software implementation. So user should be able to enable nowayout using kernel module args. Right?

> - the upstream driver only works after efibootguard enabled the
> watchdog - likely fixable, though, and likely the reason why the
> misunderstanding arose that the kernel does not support this
> hardware
>
>Did you check the specifications for a potential hw-assisted locking /
>no-way-out mode? Not having that significantly decreases the value of
>this watchdog unfortunately.
>

Arsalan:
I had a conversation with AMD requesting from them the WDT docs, WDT name and upstream status. They said that their team is working on upstreaming this amdfch_wdt driver as we speak. They will not be able to share the docs. They suggested this name "amdfch_wdt" for this WDT. They said that you can use the current code for the driver that AMD has shared with Mentor, and you can merge the latest code once publicly available. The code is expected to be available in Q4 2020.

Since I could not get hands on the design docs or manuals for this WDT, unfortunately we would not be able to implement the HW-assisted nowayout at the moment. But workarounds exist! And having this amdfch_wdt implementation in the efibootguard has enough value for now, than not having it at all.

>It could be partially mitigated by changing the kernel driver to detect
>a running watchdog and avoid turning it off unless explicitly requested.
>That what w83627hf_wdt.c does e.g. I have that one on my board as well,
>and it also lacks any lock-down support.
>

Arsalan:
I did fix this by removing that line in the amd_wdt kernel module that stops the wdt. With that workaround, things worked fine!

[Jan Kiszka]

On 07.09.20 11:08, Awan, Arsalan wrote:
>
>>>> OK, just missed my first review comment. But we can sort out read/in
>>>> error handling while factoring out those helpers in-tree.
>>>>
>>>> Applied to next.
>>>>
>>> Arsalan:
>>> Appreciate that! Thanks!
>>>
>>
>> Some update after trying out the driver on my new R1505G. The good news:
>>
>> - efibootguard detects and configures the watchdog correctly
>>
>> - the upstream kernel driver picks up the watchdog - if not, the system
>> resets after the timeout
>>
> Arsalan:
> I noticed that the other day. The kernel was picking sp5100 driver + our amd_wdt kernel module to get things working.
> Without the amd_wdt kernel module OR the amdfch_wdt efibootguard driver, the WDT won't work.
>

The difference must be in the watchdog enabling bits. I didn't look into
the details, but they can't be many.

>> The bad ones:
>>
>> - there is no lock-down of the watchdog, and a distro kernel will
>> simply turn it off until some watchdog service may pick it up again
>>
> Arsalan:
> Yes you're right.
> However, the kernel driver sp5100 has this nowayout functionality implemented though. Probably a software implementation. So user should be able to enable nowayout using kernel module args. Right?

The generic nowayout mode of the watchdog subsystem only works for the
case of opening the device from userspace. Once that happened, the
kernel prevents in software that the watchdog is disabled again.

However, this has nothing to do with hardware-assisted locking and also
the default behavior when loading the driver, i.e. before the first
userspace access. Here, the given kernel driver disables the hardware
unconditionally.

>
>> - the upstream driver only works after efibootguard enabled the
>> watchdog - likely fixable, though, and likely the reason why the
>> misunderstanding arose that the kernel does not support this
>> hardware
>>
>> Did you check the specifications for a potential hw-assisted locking /
>> no-way-out mode? Not having that significantly decreases the value of
>> this watchdog unfortunately.
>>
> Arsalan:
> I had a conversation with AMD requesting from them the WDT docs, WDT name and upstream status. They said that their team is working on upstreaming this amdfch_wdt driver as we speak. They will not be able to share the docs. They suggested this name "amdfch_wdt" for this WDT. They said that you can use the current code for the driver that AMD has shared with Mentor, and you can merge the latest code once publicly available. The code is expected to be available in Q4 2020.

I doubt they will be successful with proposing a new driver, rather than
adding the three missing lines to the existing one. From my experience,
the wdt maintainer is very accurate.

>
> Since I could not get hands on the design docs or manuals for this WDT, unfortunately we would not be able to implement the HW-assisted nowayout at the moment. But workarounds exist! And having this amdfch_wdt implementation in the efibootguard has enough value for now, than not having it at all.
>
>> It could be partially mitigated by changing the kernel driver to detect
>> a running watchdog and avoid turning it off unless explicitly requested.
>> That what w83627hf_wdt.c does e.g. I have that one on my board as well,
>> and it also lacks any lock-down support.
>>
> Arsalan:
> I did fix this by removing that line in the amd_wdt kernel module that stops the wdt. With that workaround, things worked fine!
>

To make that effective (unless the hardware has some hidden lock-down
mode), it needs to go upstream.

Jan

[Jan Kiszka]

All what is missing is a single(!) line:

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 85e9664318c9..5482154fde42 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -193,7 +193,8 @@ static void tco_timer_enable(struct sp5100_tco *tco)
/* Set the Watchdog timer resolution to 1 sec and enable */
sp5100_tco_update_pm_reg8(EFCH_PM_DECODEEN3,
~EFCH_PM_WATCHDOG_DISABLE,
- EFCH_PM_DECODEEN_SECOND_RES);
+ EFCH_PM_DECODEEN_SECOND_RES |
+ EFCH_PM_DECODEEN_WDT_TMREN);
break;
}
}

[ 11.957371] sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver
[ 11.957473] sp5100-tco sp5100-tco: Using 0xfed80b00 for watchdog MMIO address
[ 11.957555] sp5100-tco sp5100-tco: initialized. heartbeat=60 sec (nowayout=0)
(while EBG was disabled)

I'm not sure if that is unconditionally correct, also for the older
chipsets supported by this driver, but maybe you can poke your AMD
contacts. Will also save them "a lot" of their time. Then I can propose
the result for upstream.

Jan

[Jan Kiszka]

Already posted, you were on CC, because Guenter was the one to add efch
support upstream. He possibly knows as well and need to decide anyway.

Jan