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

[Arsalan H. Awan]

This adds support for AMD Watchdog in efibootguard.

Signed-off-by: Arsalan H. Awan <Arsala...@mentor.com>
---
Makefile.am | 1 +
drivers/watchdog/amd_wdt.c | 296 +++++++++++++++++++++++++++++++++++++
drivers/watchdog/amd_wdt.h | 76 ++++++++++
3 files changed, 373 insertions(+)
create mode 100644 drivers/watchdog/amd_wdt.c
create mode 100644 drivers/watchdog/amd_wdt.h

diff --git a/Makefile.am b/Makefile.am
index 74fa7f0..da4c7c1 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/amd_wdt.c \
drivers/watchdog/i6300esb.c \
drivers/watchdog/atom-quark.c \
drivers/watchdog/itco.c \
diff --git a/drivers/watchdog/amd_wdt.c b/drivers/watchdog/amd_wdt.c
new file mode 100644
index 0000000..2e194ea
--- /dev/null
+++ b/drivers/watchdog/amd_wdt.c
@@ -0,0 +1,296 @@
+/*
+ * 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>
+
+#include "amd_wdt.h"
+
+#define PCI_VENDOR_ID_AMD 0x1022
+
+/* #define AMD_WDT_DEBUG */
+
+static struct
+{
+ UINT8 fired;
+ UINTN base;
+ EFI_PCI_IO *pci_io;
+} watchdog;
+
+static EFI_STATUS
+writel (UINT32 val, UINT32 addr)
+{
+#ifdef AMD_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 AMD_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 AMD_WDT_DEBUG
+ Print (L"= %X ", val);
+#endif
+ return val;
+}
+
+static EFI_STATUS
+outb (UINT8 val, UINT16 reg)
+{
+#ifdef AMD_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 AMD_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 AMD_WDT_DEBUG
+ Print(L"= %X ", val);
+#endif
+ return val;
+}
+
+static UINT8
+amd_wdt_check_fired (VOID)
+{
+#ifdef AMD_WDT_DEBUG
+ Print (L"\n-- amd_wdt_check_fired() ");
+#endif
+ UINT32 val;
+ /* Read watchdog fired bit */
+ val = readl (AMD_WDT_CONTROL(watchdog.base));
+ return val & AMD_WDT_FIRED_BIT;
+}
+
+static EFI_STATUS
+amd_wdt_enable (VOID)
+{
+#ifdef AMD_WDT_DEBUG
+ Print (L"\n-- amd_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
+amd_wdt_set_resolution (UINT8 freq)
+{
+#ifdef AMD_WDT_DEBUG
+ Print (L"\n-- amd_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
+amd_wdt_set_timeout_action (CONST CHAR16 * action)
+{
+#ifdef AMD_WDT_DEBUG
+ Print (L"\n-- amd_wdt_set_timeout_action(%s) ", action);
+#endif
+ UINT32 val;
+ val = readl (AMD_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 |= AMD_WDT_ACTION_RESET_BIT;
+ else
+ val &= ~AMD_WDT_ACTION_RESET_BIT;
+
+ return writel (val, AMD_WDT_CONTROL(watchdog.base));
+}
+
+static EFI_STATUS
+amd_wdt_set_time (UINT32 t)
+{
+#ifdef AMD_WDT_DEBUG
+ Print (L"\n-- amd_wdt_set_time(%d) ", t);
+#endif
+ if (t < AMD_WDT_MIN_TIMEOUT)
+ t = AMD_WDT_MIN_TIMEOUT;
+ else if (t > AMD_WDT_MAX_TIMEOUT)
+ t = AMD_WDT_MAX_TIMEOUT;
+
+ /* Write new timeout value to watchdog COUNT register */
+ return writel (t, AMD_WDT_COUNT(watchdog.base));
+}
+
+static EFI_STATUS
+amd_wdt_start (VOID)
+{
+#ifdef AMD_WDT_DEBUG
+ Print (L"\n-- amd_wdt_start() ");
+#endif
+ UINT32 val;
+ /* Start the watchdog timer */
+ val = readl (AMD_WDT_CONTROL(watchdog.base));
+ val |= AMD_WDT_START_STOP_BIT;
+ return writel (val, AMD_WDT_CONTROL(watchdog.base));
+}
+
+static EFI_STATUS
+amd_wdt_ping (VOID)
+{
+#ifdef AMD_WDT_DEBUG
+ Print (L"\n-- amd_wdt_ping() ");
+#endif
+ UINT32 val;
+ /* Trigger/Ping watchdog timer */
+ val = readl (AMD_WDT_CONTROL(watchdog.base));
+ val |= AMD_WDT_TRIGGER_BIT;
+ return writel (val, AMD_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 + AMD_WDT_MEM_MAP_OFFSET;
+ watchdog.pci_io = pci_io;
+
+ Print(L"Detected AMD Carrizo SMBus Watchdog Timer\n");
+
+ watchdog.fired = amd_wdt_check_fired();
+
+#ifdef AMD_WDT_DEBUG
+ Print(L"\nwatchdog.base = %X\n", watchdog.base);
+ Print(L"watchdog.fired = %X\n", watchdog.fired);
+#endif
+
+ status = amd_wdt_enable ();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amd_wdt_enable () failed.");
+ return status;
+ }
+
+ status = amd_wdt_set_resolution (AMD_PM_WATCHDOG_1SEC_RES);
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amd_wdt_set_resolution (%d) failed.", AMD_PM_WATCHDOG_1SEC_RES);
+ return status;
+ }
+
+ status = amd_wdt_set_timeout_action (L"reboot");
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amd_wdt_set_timeout_action ("reboot") failed.");
+ return status;
+ }
+
+ status = amd_wdt_set_time (timeout);
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amd_wdt_set_time (%d) failed.", timeout);
+ return status;
+ }
+
+ status = amd_wdt_start ();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amd_wdt_start () failed.");
+ return status;
+ }
+
+ status = amd_wdt_ping ();
+ if (EFI_ERROR(status)) {
+ Print(L"Error: amd_wdt_ping () failed.");
+ return status;
+ }
+
+/*
+ * Following is an alternate easy way to set the watchdog
+ * using the UEFI API.
+ * This works only if watchdog driver is implemented in the
+ * UEFI firmware on the machine.
+ *
+ * status = uefi_call_wrapper(BS->SetWatchdogTimer, 4,
+ * timeout, 0, 0, NULL);
+ */
+
+ Print(L"\nAMD Watchdog setup complete!\n");
+ return status;
+}
diff --git a/drivers/watchdog/amd_wdt.h b/drivers/watchdog/amd_wdt.h
new file mode 100644
index 0000000..db7c49a
--- /dev/null
+++ b/drivers/watchdog/amd_wdt.h
@@ -0,0 +1,76 @@
+/*****************************************************************************
+*
+* Copyright (c) 2014, Advanced Micro Devices, Inc.
+* All rights reserved.
+*
+* Redistribution and use in source and binary forms, with or without
+* modification, are permitted provided that the following conditions are met:
+* * Redistributions of source code must retain the above copyright
+* notice, this list of conditions and the following disclaimer.
+* * Redistributions in binary form must reproduce the above copyright
+* notice, this list of conditions and the following disclaimer in the
+* documentation and/or other materials provided with the distribution.
+* * Neither the name of Advanced Micro Devices, Inc. nor the names of
+* its contributors may be used to endorse or promote products derived
+* from this software without specific prior written permission.
+*
+* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+* DISCLAIMED. IN NO EVENT SHALL ADVANCED MICRO DEVICES, INC. BE LIABLE FOR ANY
+* DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+* ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+*
+*
+***************************************************************************/
+
+#ifndef _AMD_WDT_H_
+#define _AMD_WDT_H_
+
+/* Module and version information */
+#define WDT_VERSION "1.0"
+#define WDT_MODULE_NAME "AMD watchdog timer"
+#define WDT_DRIVER_NAME WDT_MODULE_NAME ", v" WDT_VERSION
+
+#define AMD_WDT_DEFAULT_TIMEOUT 60 /* 60 units default heartbeat. */
+#define AMD_WDT_MIN_TIMEOUT 0x0001 /* minimum timeout value */
+#define AMD_WDT_MAX_TIMEOUT 0xFFFF /* maximum timeout value */
+#define MAX_LENGTH (8 + 1) /* shutdown has 8 characters + NULL character */
+
+/* Watchdog register definitions */
+#define AMD_ACPI_MMIO_BASE 0xFED80000
+#define AMD_WDT_MEM_MAP_OFFSET 0xB00
+#define AMD_WDT_MEM_MAP_SIZE 0x100
+
+#define AMD_WDT_CONTROL(base) ((base) + 0x00) /* Watchdog Control */
+ #define AMD_WDT_START_STOP_BIT (1 << 0)
+ #define AMD_WDT_FIRED_BIT (1 << 1)
+ #define AMD_WDT_ACTION_RESET_BIT (1 << 2)
+ #define AMD_WDT_DISABLE_BIT (1 << 3)
+ /* 6:4 bits Reserved */
+ #define AMD_WDT_TRIGGER_BIT (1 << 7)
+#define AMD_WDT_COUNT(base) ((base) + 0x04) /* Watchdog Count */
+ #define AMD_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 AMD_ACPI_MMIO_ADDR_MASK ~0x1FFF
+#define PCI_DEVICE_ID_AMD_CARRIZO_SMBUS 0x790B
+
+#endif /* _AMD_WDT_H_ */
--
2.17.1

[Jan Kiszka]

On 21.08.20 17:46, Arsalan H. Awan wrote:
> This adds support for AMD Watchdog in efibootguard.
>

The driver does not look like it's derived from the authoritative
linux/drivers/watchdog/sp5100_tco.c, is it? Did you double-check that
you are not missing anything from that driver? If you are in line with
Linux, you may also add the other PCI IDs. But I just started to compare.

That linux driver suggests you are implementing the SB800 (and newer
compatible ones). Maybe avoid such a generic name and use that instead.

Please move the used defines here.

> +
> +#define PCI_VENDOR_ID_AMD 0x1022
> +
> +/* #define AMD_WDT_DEBUG */
> +
> +static struct
> +{
> + UINT8 fired;
> + UINTN base;
> + EFI_PCI_IO *pci_io;
> +} watchdog;
> +
> +static EFI_STATUS
> +writel (UINT32 val, UINT32 addr)

static EFI_STATUS writel(...)

Please align your coding style to the rest of the project, specifically
the watchdog drivers.

> +{
> +#ifdef AMD_WDT_DEBUG
> + Print (L"\n- writel (%X, %X) ", val, addr);
> +#endif

Still needed? If so, wrap properly and have a single ifdef there and a
DebugPrint or so in the code. But I suppose your code is beyond that
point already.

Unneed abstraction with a rather unusual (string-based) interface. We
only use "reboot".

If you want to factor out a common read-modify-write cycle, look at
sp5100_tco_update_pm_reg8.

That comment is not correct.

> + val = readl (AMD_WDT_CONTROL(watchdog.base));
> + val |= AMD_WDT_START_STOP_BIT;

I think the kernel does more, please cross-check.

Ok, this hard-coding actually limits us to embedded FCH chipset. Fine
with me, but then we have a different scope and should reflect that in
the driver name (amd-efch?).

> + watchdog.pci_io = pci_io;
> +
> + Print(L"Detected AMD Carrizo SMBus Watchdog Timer\n");

And here you actually call Carrizo, though I'm not sure if that covers
it better. It's just that "AMD" is too generic.

> +
> + watchdog.fired = amd_wdt_check_fired();
> +
> +#ifdef AMD_WDT_DEBUG
> + Print(L"\nwatchdog.base = %X\n", watchdog.base);
> + Print(L"watchdog.fired = %X\n", watchdog.fired);
> +#endif

Still needed? What for? If not, drop all the dead fired-related code.

Is that ping actually needed? The kernel doesn't issue it.

> + if (EFI_ERROR(status)) {
> + Print(L"Error: amd_wdt_ping () failed.");
> + return status;
> + }
> +
> +/*
> + * Following is an alternate easy way to set the watchdog
> + * using the UEFI API.
> + * This works only if watchdog driver is implemented in the
> + * UEFI firmware on the machine.
> + *
> + * status = uefi_call_wrapper(BS->SetWatchdogTimer, 4,
> + * timeout, 0, 0, NULL);
> + */

What is this?

> +
> + Print(L"\nAMD Watchdog setup complete!\n");

Not strictly needed - we will see error messages if that stage is not
reached.

Undesirable additional license. Just derive from the kernel, and we stay
GPL-only.

Thanks,
Jan

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

[Awan, Arsalan]

> On 21.08.20 17:46, Arsalan H. Awan wrote:
> > This adds support for AMD Watchdog in efibootguard.
> >
>
> The driver does not look like it's derived from the authoritative
> linux/drivers/watchdog/sp5100_tco.c, is it? Did you double-check that
> you are not missing anything from that driver? If you are in line with
> Linux, you may also add the other PCI IDs. But I just started to compare.
>
> That linux driver suggests you are implementing the SB800 (and newer
> compatible ones). Maybe avoid such a generic name and use that instead.
>

Arsalan:
No, it's not! This driver is for the latest AMD platforms e.g.:

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

See: https://www.amd.com/en/products/embedded-ryzen-series
See: https://www.amd.com/en/products/embedded-epyc-3000-series

The driver that you mentioned does not drive the watchdog on these platforms, and the driver I am submitting
does not exist in the upstream linux kernel. However linux implementation of this driver was shared by AMD
with us (Mentor).

See: https://git.yoctoproject.org/cgit/cgit.cgi/meta-amd/tree/meta-amd-bsp/recipes-kernel/amd-wdt/files

...and I implemented this driver for grub-efi bootloader.

See: https://git.yoctoproject.org/cgit/cgit.cgi/meta-amd/tree/common/mentor-swupdate/recipes-bsp/grub/amd-wdt?h=sumo

...and I basically ported that over to efibootguard bootloader.

And yes, you're right. I should change the name to something more specific targetting these platforms.
But I'd need your suggestion on this for what you'd like for the efibootguard project.

Arsalan:
I'm on it!

> > +
> > +#define PCI_VENDOR_ID_AMD 0x1022
> > +
> > +/* #define AMD_WDT_DEBUG */
> > +
> > +static struct
> > +{
> > + UINT8 fired;
> > + UINTN base;
> > + EFI_PCI_IO *pci_io;
> > +} watchdog;
> > +
> > +static EFI_STATUS
> > +writel (UINT32 val, UINT32 addr)
>
> static EFI_STATUS writel(...)
>
> Please align your coding style to the rest of the project, specifically
> the watchdog drivers.
>
> > +{
> > +#ifdef AMD_WDT_DEBUG
> > + Print (L"\n- writel (%X, %X) ", val, addr);
> > +#endif
>
> Still needed? If so, wrap properly and have a single ifdef there and a
> DebugPrint or so in the code. But I suppose your code is beyond that
> point already.
>

Arsalan:
I found this extremely helpful in debugging.

Arsalan:
I did what AMD did for their kernel implementation of this watchdog driver.

Arsalan:
AMD themselves think that it is.

> > + val = readl (AMD_WDT_CONTROL(watchdog.base));
> > + val |= AMD_WDT_START_STOP_BIT;
>
> I think the kernel does more, please cross-check.
>

Arsalan:
I did what AMD did in their kernel implementation of this driver.

Arsalan:
Again, It's not for EFCH chipset. And it targets at least 3 different latest series of AMD chips at the moment
that I have personally tested this driver on, and we know 2 more series of AMD APUs/CPUs coming soon
that this driver will be potentially applicable to... but I'm not sure.

So what would you suggest as a name for this driver?

> > + watchdog.pci_io = pci_io;
> > +
> > + Print(L"Detected AMD Carrizo SMBus Watchdog Timer\n");
>
> And here you actually call Carrizo, though I'm not sure if that covers
> it better. It's just that "AMD" is too generic.
>

Arsalan:
Please suggest a name. I'm only copying AMD here: ---> #define PCI_DEVICE_ID_AMD_CARRIZO_SMBUS 0x790B

> > +
> > + watchdog.fired = amd_wdt_check_fired();
> > +
> > +#ifdef AMD_WDT_DEBUG
> > + Print(L"\nwatchdog.base = %X\n", watchdog.base);
> > + Print(L"watchdog.fired = %X\n", watchdog.fired);
> > +#endif
>

> Still needed? Wlhat for? If not, drop all the dead fired-related code.
>
Arsalan:
In grub, we used the fired status to allow the user to take many decissions based on watchdog.fired status in the grub.cfg.
So, it can be useful in the cfg files. Besides I just copied the kernel implementation of this driver... so...
I can remove this if you guys don't have any plans of having such functionality.

Arsalan:
Well, I'm pretty sure the watchdog does not start the countdown unless it is pingged after start cmd.
Tested this driver with and without the ping on 3 different AMD platforms... so...

Also, here's how AMD did it in their kernel implementation of this driver:

static int amd_wdt_start(struct watchdog_device *wdt_dev)
{
u32 val;
unsigned long flags;

/* Enable the watchdog timer */
spin_lock_irqsave(&wdt_lock, flags);

val = readl(AMD_WDT_CONTROL(wdtbase));
val |= AMD_WDT_START_STOP_BIT;
writel(val, AMD_WDT_CONTROL(wdtbase));

spin_unlock_irqrestore(&wdt_lock, flags);

/* Trigger the watchdog timer */
amd_wdt_ping(wdt_dev);

return 0;
}

> > + if (EFI_ERROR(status)) {
> > + Print(L"Error: amd_wdt_ping () failed.");
> > + return status;
> > + }
> > +
> > +/*
> > + * Following is an alternate easy way to set the watchdog
> > + * using the UEFI API.
> > + * This works only if watchdog driver is implemented in the
> > + * UEFI firmware on the machine.
> > + *
> > + * status = uefi_call_wrapper(BS->SetWatchdogTimer, 4,
> > + * timeout, 0, 0, NULL);
> > + */
>
> What is this?
>

Arsalan:
It's the holy grail of watchdogs in UEFI. We don't need this whole implemntation of watchdog
drivers if the driver is already implemented in the UEFI firmware... we can simply do this!
And the UEFI sets the watchdog up for is... I can remove it if you think it's not useful.

> > +
> > + Print(L"\nAMD Watchdog setup complete!\n");
>
> Not strictly needed - we will see error messages if that stage is not
> reached.
>

Arsalan:
Sure, I'll remove this.

Arsalan:
OK!

-
Arsalan

Mentor, a Siemens business.
Mentor Embedded Linux.

[Jan Kiszka]

On 22.08.20 09:37, Awan, Arsalan wrote:
>> On 21.08.20 17:46, Arsalan H. Awan wrote:
>>> This adds support for AMD Watchdog in efibootguard.
>>>
>>
>> The driver does not look like it's derived from the authoritative
>> linux/drivers/watchdog/sp5100_tco.c, is it? Did you double-check that
>> you are not missing anything from that driver? If you are in line with
>> Linux, you may also add the other PCI IDs. But I just started to compare.
>>
>> That linux driver suggests you are implementing the SB800 (and newer
>> compatible ones). Maybe avoid such a generic name and use that instead.
>>
> Arsalan:
> No, it's not! This driver is for the latest AMD platforms e.g.:
>
> * AMD Ryzen Embedded V1000 (V-Series APU)
> * AMD Ryzen Embedded R1000 (R-Series APU)
> * AMD EPYC Embedded E3000 (E-Series CPU)
>
> See: https://www.amd.com/en/products/embedded-ryzen-series
> See: https://www.amd.com/en/products/embedded-epyc-3000-series
>
> The driver that you mentioned does not drive the watchdog on these platforms, and the driver I am submitting

It matches the same PCI ID as your driver
(PCI_DEVICE_ID_AMD_KERNCZ_SMBUS vs. PCI_DEVICE_ID_AMD_CARRIZO_SMBUS). It
shares a lot of the magic variables. It is the same base.

Where do I find the manuals for the implemented logic?

> does not exist in the upstream linux kernel. However linux implementation of this driver was shared by AMD
> with us (Mentor).

What? It's not upstream?

>
> See: https://git.yoctoproject.org/cgit/cgit.cgi/meta-amd/tree/meta-amd-bsp/recipes-kernel/amd-wdt/files
>

A yocto layer... Folks, properly work towards upstream. This kernel
driver apparently sits there for a year. This is no proper way of
maintaining such code. If AMD didn't do their homework, fix it.

I didn't look into all the details of what is missing/diverging, but I
predict from what I've seen they will be minimal. It should take you day
to sort it out for good.

> ...and I implemented this driver for grub-efi bootloader.
>
> See: https://git.yoctoproject.org/cgit/cgit.cgi/meta-amd/tree/common/mentor-swupdate/recipes-bsp/grub/amd-wdt?h=sumo
>
> ...and I basically ported that over to efibootguard bootloader.
>
> And yes, you're right. I should change the name to something more specific targetting these platforms.
> But I'd need your suggestion on this for what you'd like for the efibootguard project.

For EFI Boot Guard, I'm fine with a driver that has a similar, modern
scope like your downstream kernel module plus what the kernel drives
under the very same PCI ID. Give it a name that reflects that scope.

Their driver is for the Linux watchdog subsystem with all its runtime
features. We just implement the setup function here. So you can fold a
lot of things to make the code more compact.

Ah, sorry, I was off the track.

>
>>> + val = readl (AMD_WDT_CONTROL(watchdog.base));
>>> + val |= AMD_WDT_START_STOP_BIT;
>>
>> I think the kernel does more, please cross-check.
>>
> Arsalan:
> I did what AMD did in their kernel implementation of this driver.

That's why I'm asking for the documentation.

>
>>> + return writel (val, AMD_WDT_CONTROL(watchdog.base));
>>> +}
>>> +
>>> +static EFI_STATUS
>>> +amd_wdt_ping (VOID)
>>> +{
>>> +#ifdef AMD_WDT_DEBUG
>>> + Print (L"\n-- amd_wdt_ping() ");
>>> +#endif
>>> + UINT32 val;
>>> + /* Trigger/Ping watchdog timer */
>>> + val = readl (AMD_WDT_CONTROL(watchdog.base));
>>> + val |= AMD_WDT_TRIGGER_BIT;
>>> + return writel (val, AMD_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) {

BTW, you will likely need to check the revision as well. I suspect it
will be the key to differentiate from existing EFCH (if there are
relevant differences in the end), but that already requires >= 0x49, see
tco_reg_layout.

>>> + return EFI_UNSUPPORTED;
>>> + }
>>> +
>>> + watchdog.base = AMD_ACPI_MMIO_BASE + AMD_WDT_MEM_MAP_OFFSET;
>>
>> Ok, this hard-coding actually limits us to embedded FCH chipset. Fine
>> with me, but then we have a different scope and should reflect that in
>> the driver name (amd-efch?).
>>
> Arsalan:
> Again, It's not for EFCH chipset. And it targets at least 3 different latest series of AMD chips at the moment

It *is* EFCH-derived, just compare the code.

> that I have personally tested this driver on, and we know 2 more series of AMD APUs/CPUs coming soon
> that this driver will be potentially applicable to... but I'm not sure.
>
> So what would you suggest as a name for this driver?

Give me the docs and I can likely tell you.

>
>>> + watchdog.pci_io = pci_io;
>>> +
>>> + Print(L"Detected AMD Carrizo SMBus Watchdog Timer\n");
>>
>> And here you actually call Carrizo, though I'm not sure if that covers
>> it better. It's just that "AMD" is too generic.
>>
> Arsalan:
> Please suggest a name. I'm only copying AMD here: ---> #define PCI_DEVICE_ID_AMD_CARRIZO_SMBUS 0x790B
>
>>> +
>>> + watchdog.fired = amd_wdt_check_fired();
>>> +
>>> +#ifdef AMD_WDT_DEBUG
>>> + Print(L"\nwatchdog.base = %X\n", watchdog.base);
>>> + Print(L"watchdog.fired = %X\n", watchdog.fired);
>>> +#endif
>>
>> Still needed? Wlhat for? If not, drop all the dead fired-related code.
>>
> Arsalan:
> In grub, we used the fired status to allow the user to take many decissions based on watchdog.fired status in the grub.cfg.
> So, it can be useful in the cfg files. Besides I just copied the kernel implementation of this driver... so...
> I can remove this if you guys don't have any plans of having such functionality.
>

There is no use case in EFI Boot Guard for it. We robustly track the
state internally, and it does not matter if we restarted due to a
watchdog timeout or an erroneous reboot/power-cycle - we will always
declare an active trial boot failed if that happens. In fact, if you
based your state machine in grub solely on the fired bit, you would have
a serious bug.

So, please drop this dead code.

Then this might be a concrete deviation from older EFCH designs.

WDAT? That would be a different driver for EBG, nothing that belongs
here because it is not specific to this hardware.

Or is this the UEFI boot services watchdog that terminates early during
Linux boot? That's unfortunately a completely useless thing when it
comes to A/B update monitoring.

[Awan, Arsalan]

>On 22.08.20 09:37, Awan, Arsalan wrote:
>>> On 21.08.20 17:46, Arsalan H. Awan wrote:
>>>> This adds support for AMD Watchdog in efibootguard.
>>>>
>>>
>>> The driver does not look like it's derived from the authoritative
>>> linux/drivers/watchdog/sp5100_tco.c, is it? Did you double-check that
>>> you are not missing anything from that driver? If you are in line with
>>> Linux, you may also add the other PCI IDs. But I just started to compare.
>>>
>>> That linux driver suggests you are implementing the SB800 (and newer
>>> compatible ones). Maybe avoid such a generic name and use that instead.
>>>
>> Arsalan:
>> No, it's not! This driver is for the latest AMD platforms e.g.:
>>
>> * AMD Ryzen Embedded V1000 (V-Series APU)
>> * AMD Ryzen Embedded R1000 (R-Series APU)
>> * AMD EPYC Embedded E3000 (E-Series CPU)
>>
>> See: https://www.amd.com/en/products/embedded-ryzen-series
>> See: https://www.amd.com/en/products/embedded-epyc-3000-series
>>
>> The driver that you mentioned does not drive the watchdog on these platforms, and the driver I am submitting
>
>It matches the same PCI ID as your driver
>(PCI_DEVICE_ID_AMD_KERNCZ_SMBUS vs. PCI_DEVICE_ID_AMD_CARRIZO_SMBUS). It
>shares a lot of the magic variables. It is the same base.
>
>Where do I find the manuals for the implemented logic?
>

Arsalan:
Let me check if I can get hands on the docs.

>> does not exist in the upstream linux kernel. However linux implementation of this driver was shared by AMD
>> with us (Mentor).
>
>What? It's not upstream?
>
>>
>> See: https://git.yoctoproject.org/cgit/cgit.cgi/meta-amd/tree/meta-amd-bsp/recipes-kernel/amd-wdt/files
>>
>
>A yocto layer... Folks, properly work towards upstream. This kernel
>driver apparently sits there for a year. This is no proper way of
>maintaining such code. If AMD didn't do their homework, fix it.
>

Arsalan:
Well that's not all... anyway, there is no point in discussing that...

>I didn't look into all the details of what is missing/diverging, but I
>predict from what I've seen they will be minimal. It should take you day
>to sort it out for good.
>

Arsalan:
I can only submit what I have worked on... something I can test on a physical machine.
I don't know EFCH or SB800 or SP5100... and I know EFCH driver does *not* work on the
platforms I mentioned earlier.

>> ...and I implemented this driver for grub-efi bootloader.
>>
>> See: https://git.yoctoproject.org/cgit/cgit.cgi/meta-amd/tree/common/mentor-swupdate/recipes-bsp/grub/amd-wdt?h=sumo
>>
>> ...and I basically ported that over to efibootguard bootloader.
>>
>> And yes, you're right. I should change the name to something more specific targetting these platforms.
>> But I'd need your suggestion on this for what you'd like for the efibootguard project.
>
>For EFI Boot Guard, I'm fine with a driver that has a similar, modern
>scope like your downstream kernel module plus what the kernel drives
>under the very same PCI ID. Give it a name that reflects that scope.
>

Arsalan:
I'll try to talk to AMD to find out what they call this watchdog, but in
case I can't, would it be OK if I renamed it to "amd_carrizo_smbus_wdt"?

Arsalan:
If you look at the code, you'll see a lot of calls to inb, outb, readl and writel functions.
If I remove the functions and use the code inside instead, the code will get only bigger.
Everything else is necessary apart from watchdog.fired bits. I'll remove that. It may not
be as compact as it can get, but it is clean.

Arsalan:
How do I access the revision in efibootguard watchdog driver init (constructor) to compare against?

>>>> + return EFI_UNSUPPORTED;
>>>> + }
>>>> +
>>>> + watchdog.base = AMD_ACPI_MMIO_BASE + AMD_WDT_MEM_MAP_OFFSET;
>>>
>>> Ok, this hard-coding actually limits us to embedded FCH chipset. Fine
>>> with me, but then we have a different scope and should reflect that in
>>> the driver name (amd-efch?).
>>>
>> Arsalan:
>> Again, It's not for EFCH chipset. And it targets at least 3 different latest series of AMD chips at the moment
>
>It *is* EFCH-derived, just compare the code.
>

Arsalan:
Sure! But I don't know EFCH and I know that that driver does not work on these latest platforms.
And I don't want to write a driver for an EFCH because I cannot test it.

>> that I have personally tested this driver on, and we know 2 more series of AMD APUs/CPUs coming soon
>> that this driver will be potentially applicable to... but I'm not sure.
>>
>> So what would you suggest as a name for this driver?
>
>Give me the docs and I can likely tell you.
>

Arsalan:
Working on it...

Arsalan:
Consider it done!

Arsalan:
I guess...

Arsalan:
Consider it gone!

>Or is this the UEFI boot services watchdog that terminates early during
>Linux boot? That's unfortunately a completely useless thing when it
>comes to A/B update monitoring.
>

Arsalan:
I'm not sure if I exactly got you here, but I want to share my experience with this
method that the underlying hardware wdt (at least for the platforms I mentioned)
that got configured was the same.
So whether efibootguard configures the wdt, or UEFI configures the wdt and then
dies, the hardware wdt gets configured... and it will pop! It does! I've tested it!

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

-

[Jan Kiszka]

On 22.08.20 20:05, Awan, Arsalan wrote:
>> Or is this the UEFI boot services watchdog that terminates early during
>> Linux boot? That's unfortunately a completely useless thing when it
>> comes to A/B update monitoring.
>>
> Arsalan:
> I'm not sure if I exactly got you here, but I want to share my experience with this
> method that the underlying hardware wdt (at least for the platforms I mentioned)
> that got configured was the same.
> So whether efibootguard configures the wdt, or UEFI configures the wdt and then
> dies, the hardware wdt gets configured... and it will pop! It does! I've tested it!

UEFI Spec 2.6, EFI_BOOT_SERVICES.SetWatchdogTimer():

"The watchdog timer is only used during boot services. On successful
completion of EFI_BOOT_SERVICES.ExitBootServices() the watchdog timer is
disabled."

ExitBootServices is run early during Linux boot. Thus, a watchdog that
is managed by UEFI is completely useless for our purposes. We've looked
into that already, even started discussions with the community. The spec
will have to be enhanced to make this usable (and to back its
implementation also by no-way-out hw watchdogs...).