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.
> Signed-off-by: Arsalan H. Awan <arsalan_a...@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 <arsalan_a...@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"
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.
> + 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.
> +{
> +#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.
> + val = readl (AMD_WDT_CONTROL(watchdog.base));
> + val |= AMD_WDT_START_STOP_BIT;
I think the kernel does more, please cross-check.
> + 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?).
> + 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.
> +
> + 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.
> + 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.
> + 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.
> +*
> +*
> +***************************************************************************/
> +
> +#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
--
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 efibootguard-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/efibootguard-dev/df1a9c0a-79f1-ef98-15b4-23d3b6c2cf4c%40siemens.com.
