> 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. > > Signed-off-by: Arsalan H. Awan <[email protected]> > > --- > > 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 <[email protected]> > > + * > > + * 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" > > Please move the used defines here. > 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. > > + 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) > > 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. > Arsalan: I did what AMD did for their kernel implementation of this watchdog driver. > > +{ > > +#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 */ > > That comment is not correct. > 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. > > + 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; > > 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 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. > > + > > + 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 (); > > Is that ping actually needed? The kernel doesn't issue it. > 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. > > + 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. > > Undesirable additional license. Just derive from the kernel, and we stay > GPL-only. > Arsalan: OK! > > +* > > +* > > +***************************************************************************/ > > + > > +#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_ */ > > > > Thanks, > Jan > > -- > Siemens AG, Corporate Technology, CT RDA IOT SES-DE > Corporate Competence Center Embedded Linux - Arsalan Mentor, a Siemens business. Mentor Embedded Linux. -- You received this message because you are subscribed to the Google Groups "EFI Boot Guard" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/efibootguard-dev/1598081822469.48919%40mentor.com.
