On Sun, Jul 21, 2024 at 9:19 AM Jan Kiszka <jan.kis...@siemens.com> wrote:
>
> On 17.07.24 17:15, marc.ferl...@gmail.com wrote:
> > From: Marc Ferland <marc.ferl...@sonatest.com>
> >
> > This driver adds support for the WDT included in Advantechs's EC
> > chip (EIOIS200). Unfortunatly, no documentation is available at this
> > time and so the driver was inspired from the code available at:
> >
> > https://github.com/ADVANTECH-Corp/eio-is200-linux-kernel-driver
>
> Thanks for the patch. I saw you trying to contribute to that downstream
> driver as well. Is anyone working on properly upstreaming all those
> bits? You already found issues, and I bet the respective subsystem
> maintainer will found some more.
>
I don't think so. Advantech tried to do it but ultimately decided it was too
much effort I guess...
See: https://lore.kernel.org/lkml/?q=eiois200
And yes, there are many issues with the original driver!
> >
> > This driver was tested on the SOM-6872.
> >
> > Signed-off-by: Marc Ferland <marc.ferl...@sonatest.com>
> > ---
> > Makefile.am | 1 +
> > drivers/watchdog/eiois200_wdt.c | 442 ++++++++++++++++++++++++++++++++
> > 2 files changed, 443 insertions(+)
> > create mode 100644 drivers/watchdog/eiois200_wdt.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 2ea47ae..34a7ee7 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -175,6 +175,7 @@ efi_sources_watchdogs = \
> > drivers/watchdog/ipmi_wdt.c \
> > drivers/watchdog/itco.c \
> > drivers/watchdog/hpwdt.c \
> > + drivers/watchdog/eiois200_wdt.c \
> > drivers/utils/simatic.c \
> > drivers/utils/smbios.c
> > else
> > diff --git a/drivers/watchdog/eiois200_wdt.c
> > b/drivers/watchdog/eiois200_wdt.c
> > new file mode 100644
> > index 0000000..1459c2b
> > --- /dev/null
> > +++ b/drivers/watchdog/eiois200_wdt.c
> > @@ -0,0 +1,442 @@
> > +/*
> > + * EFI Boot Guard
> > + *
> > + * Copyright (c) Sonatest AP Inc, 2024
> > + *
> > + * Authors:
> > + * Marc Ferland <marc.ferl...@sonatest.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 <sys/io.h>
> > +#include <mmio.h>
> > +#include "utils.h"
> > +
> > +/* #define EIO200IS_WDT_DEBUG */
> > +
> > +#ifdef EIO200IS_WDT_DEBUG
> > +#define DBG(fmt, ...) Print(fmt, ##__VA_ARGS__)
> > +#else
> > +#define DBG(fmt, ...) {}
> > +#endif
> > +
> > +#define EIOIS200_MODE_ENTER 0x87
> > +#define EIOIS200_MODE_EXIT 0xaa
> > +#define EIOIS200_CHIPID1 0x20
> > +#define EIOIS200_CHIPID2 0x21
> > +#define EIOIS200_200_CHIPID 0x9610
> > +#define EIOIS200_211_CHIPID 0x9620
> > +#define EIOIS200_SIOCTRL 0x23
> > +#define EIOIS200_SIOCTRL_SIOEN (1 << 0)
> > +#define EIOIS200_SIOCTRL_SWRST (1 << 1)
> > +#define EIOIS200_IRQCTRL 0x70
> > +
> > +#define EIOIS200_PMC_STATUS_IBF (1 << 1)
> > +#define EIOIS200_PMC_STATUS_OBF (1 << 0)
> > +#define EIOIS200_LDAR 0x30
> > +#define EIOIS200_LDAR_LDACT (1 << 0)
> > +#define EIOIS200_IOBA0H 0x60
> > +#define EIOIS200_IOBA0L 0x61
> > +#define EIOIS200_IOBA1H 0x62
> > +#define EIOIS200_IOBA1L 0x63
> > +#define EIOIS200_FLAG_PMC_READ (1 << 0)
> > +
> > +#define PMC_WDT_CMD_WRITE 0x2a
> > +#define PMC_WDT_CMD_READ 0x2b
> > +#define PMC_WDT_CTRL_START 0x01
> > +#define PMC_WDT_MIN_TIMEOUT_MS 1000
> > +#define PMC_WDT_MAX_TIMEOUT_MS 32767000
> > +
> > +#define WDT_STA_AVAILABLE (1 << 0)
> > +#define WDT_STA_RESET (1 << 7)
> > +
> > +#define WDT_REG_STATUS 0x00
> > +#define WDT_REG_CONTROL 0x02
> > +#define WDT_REG_RESET_EVT_TIME 0x14
> > +
> > +/* Logical device selection */
> > +#define EIOIS200_LDN 0x07 /* index */
> > +#define EIOIS200_LDN_PMC0 0x0c /* value */
> > +#define EIOIS200_LDN_PMC1 0x0d /* value */
> > +
> > +enum eiois200_port_id {
> > + EIOIS200_PNP,
> > + EIOIS200_PNP_COUNT
> > +};
> > +
> > +struct eiois200_dev_port {
> > + UINT16 index_port;
> > + UINT16 data_port;
> > +};
> > +
> > +struct pmc_port {
> > + UINT16 cmd;
> > + UINT16 data;
> > +};
> > +
> > +/* We currently only support the EIOIS200 as implemented on the
> > + * SOM-6872 COM-Express from Advantech. Other boards might have
> > + * different port adresses. */
> > +static const struct eiois200_dev_port pnp_port[EIOIS200_PNP_COUNT] = {
> > + [EIOIS200_PNP] = {
> > + .index_port = 0x0299,
> > + .data_port = 0x029a,
> > + },
> > +};
> > +
> > +static BOOLEAN probed_before = FALSE;
> > +static EFI_EVENT cmdtimer;
> > +
> > +static void eio200_enter(const struct eiois200_dev_port *p)
> > +{
> > + UINT16 iport = p->index_port;
> > +
> > + /* unlock EC io port */
> > + outb(EIOIS200_MODE_ENTER, iport);
> > + outb(EIOIS200_MODE_ENTER, iport);
> > +}
> > +
> > +static void eio200_exit(const struct eiois200_dev_port *p)
> > +{
> > + UINT16 iport = p->index_port;
> > +
> > + /* lock EC io port */
> > + outb(EIOIS200_MODE_EXIT, iport);
> > +}
>
> Is that back and forth with enter/exit required for all the accesses, or
> would it be enough to perform the enter once and then exit before
> returning from init()?
>
I mimicked the original driver from Advantech here but I'll give it a try.
Thanks for the suggestion.
> > +
> > +static UINT8 eio200_read(const struct eiois200_dev_port *p, UINT8 index)
> > +{
> > + UINT16 iport = p->index_port;
> > + UINT16 dport = p->data_port;
> > +
> > + outb(index, iport);
> > + return inb(dport);
> > +}
> > +
> > +static void eio200_write(const struct eiois200_dev_port *p,
> > + UINT8 index, UINT8 val)
> > +{
> > + UINT16 iport = p->index_port;
> > + UINT16 dport = p->data_port;
> > +
> > + outb(index, iport);
> > + outb(val, dport);
> > +}
> > +
> > +static const struct eiois200_dev_port * eio200_find(void)
> > +{
> > + const struct eiois200_dev_port *p;
> > + UINT16 chipid;
> > + int i;
> > +
> > + for (i = 0; i < EIOIS200_PNP_COUNT; ++i) {
> > + p = &pnp_port[i];
> > +
> > + eio200_enter(p);
> > +
> > + chipid = eio200_read(p, EIOIS200_CHIPID1) << 8;
> > + chipid |= eio200_read(p, EIOIS200_CHIPID2);
>
> I don't think it is a good idea to peek on these ports on all the
> hardware we run on. You should combine this with some safer platform
> detection.
>
This part was also inspired from the original driver. I was thinking about
using the DMI entries just like you did for the SIMATIC machine, but the entries
are all empty. For example:
Handle 0x0001, DMI type 1, 27 bytes
System Information
Manufacturer: Default string
Product Name: Default string
Version: Default string
Serial Number: Default string
UUID: 03000200-0400-0500-0006-000700080009
Wake-up Type: Power Switch
SKU Number: Default string
Family: Default string
I'll try to contact the manufacturer, maybe they can update their DMI
tables and make
life a bit easier.
> > +
> > + DBG(L"ChipID: 0x%02x\n", chipid);
> > +
> > + eio200_exit(p);
> > +
> > + if (chipid != EIOIS200_200_CHIPID &&
> > + chipid != EIOIS200_211_CHIPID)
> > + continue;
> > +
> > + return p;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static void eio200_enable(const struct eiois200_dev_port *p)
> > +{
> > + UINT8 reg;
> > +
> > + eio200_enter(p);
> > +
> > + /* set the enable flag */
> > + reg = eio200_read(p, EIOIS200_SIOCTRL);
> > + reg |= EIOIS200_SIOCTRL_SIOEN;
> > + eio200_write(p, EIOIS200_SIOCTRL, reg);
> > +
> > + eio200_exit(p);
> > +}
> > +
> > +static void eio200_read_pmc_ports(const struct eiois200_dev_port *p,
> > + struct pmc_port *pmc_port)
> > +{
> > + eio200_enter(p);
> > +
> > + /* switch to logical device pmc1 */
> > + eio200_write(p, EIOIS200_LDN, EIOIS200_LDN_PMC1);
> > +
> > + /* activate device */
> > + eio200_write(p, EIOIS200_LDAR, EIOIS200_LDAR_LDACT);
> > +
> > + /* read pmc cmd and data port */
> > + pmc_port->data = eio200_read(p, EIOIS200_IOBA0H) << 8;
> > + pmc_port->data |= eio200_read(p, EIOIS200_IOBA0L);
> > + pmc_port->cmd = eio200_read(p, EIOIS200_IOBA1H) << 8;
> > + pmc_port->cmd |= eio200_read(p, EIOIS200_IOBA1L);
> > +
> > + /* disable irq */
> > + eio200_write(p, EIOIS200_IRQCTRL, 0);
> > +
> > + eio200_exit(p);
> > +}
> > +
> > +static EFI_STATUS pmc_wait_iobf(const struct pmc_port *p, UINTN iobf)
> > +{
> > + EFI_STATUS timer_status;
> > +
> > + BS->SetTimer(cmdtimer, TimerRelative, 50000); /* 5000 usec */
> > +
>
> Why do we need a timer in addition to the stall? It's not that things
> are happening asynchronously here. Just count the loops you are taking.
>
Agreed.
> > + do {
> > + UINT8 status = inb(p->cmd);
> > +
> > + if (iobf == EIOIS200_PMC_STATUS_IBF) {
> > + if (!(status & EIOIS200_PMC_STATUS_IBF))
> > + return EFI_SUCCESS;
> > + } else {
> > + if (status & EIOIS200_PMC_STATUS_OBF)
> > + return EFI_SUCCESS;
> > + }
> > +
> > + BS->Stall(200); /* 200 usec */
> > +
> > + timer_status = BS->CheckEvent(cmdtimer);
> > + } while (timer_status == EFI_NOT_READY);
> > +
> > + return EFI_DEVICE_ERROR;
> > +}
> > +
> > +static EFI_STATUS pmc_outb(const struct pmc_port *p, UINT8 value, UINT16
> > port)
> > +{
> > + EFI_STATUS err;
> > +
> > + err = pmc_wait_iobf(p, EIOIS200_PMC_STATUS_IBF);
> > + if (EFI_ERROR(err))
> > + return err;
> > +
> > + outb(value, port);
> > +
> > + return EFI_SUCCESS;
> > +}
> > +
> > +static EFI_STATUS pmc_inb(const struct pmc_port *p, UINT16 port, UINT8
> > *value)
> > +{
> > + EFI_STATUS err;
> > +
> > + err = pmc_wait_iobf(p, EIOIS200_PMC_STATUS_OBF);
> > + if (EFI_ERROR(err))
> > + return err;
> > +
> > + *value = inb(port);
> > +
> > + return EFI_SUCCESS;
> > +}
> > +
> > +static EFI_STATUS pmc_write_data(const struct pmc_port *p, UINT8 value)
> > +{
> > + return pmc_outb(p, value, p->data);
> > +}
> > +
> > +static EFI_STATUS pmc_write_cmd(const struct pmc_port *p, UINT8 cmd)
> > +{
> > + return pmc_outb(p, cmd, p->cmd);
> > +}
> > +
> > +static EFI_STATUS pmc_read_data(const struct pmc_port *p, UINT8 *value)
> > +{
> > + return pmc_inb(p, p->data, value);
> > +}
> > +
> > +static EFI_STATUS pmc_clear(const struct pmc_port *p)
> > +{
> > + UINT8 status, dummy;
> > +
> > + status = inb(p->cmd);
> > +
> > + if (!(status & EIOIS200_PMC_STATUS_IBF))
> > + return EFI_SUCCESS;
> > +
> > + /* read previous garbage */
> > + dummy = inb(p->data);
> > + (void) dummy;
> > +
> > + BS->Stall(100);
> > +
> > + return EFI_SUCCESS;
> > +}
> > +
> > +static EFI_STATUS pmc_cmd_exec(const struct pmc_port *p, UINT8 cmd, UINT8
> > ctl,
> > + UINT8 devid, UINT8 *payload, UINT8 size)
> > +{
> > + EFI_STATUS err;
> > + BOOLEAN is_read;
> > + UINT8 i;
> > +
> > + err = pmc_clear(p);
> > + if (EFI_ERROR(err))
> > + goto fail;
> > +
> > + err = pmc_write_cmd(p, cmd);
> > + if (EFI_ERROR(err))
> > + goto fail;
> > +
> > + err = pmc_write_data(p, ctl);
> > + if (EFI_ERROR(err))
> > + goto fail;
> > +
> > + err = pmc_write_data(p, devid);
> > + if (EFI_ERROR(err))
> > + goto fail;
> > +
> > + err = pmc_write_data(p, size);
> > + if (EFI_ERROR(err))
> > + goto fail;
> > +
> > + is_read = cmd & EIOIS200_FLAG_PMC_READ;
> > +
> > + for (i = 0; i < size; ++i) {
> > + if (is_read)
> > + err = pmc_read_data(p, &payload[i]);
> > + else
> > + err = pmc_write_data(p, payload[i]);
> > + if (EFI_ERROR(err))
> > + goto fail;
> > + }
> > +
> > + return EFI_SUCCESS;
> > +
> > +fail:
> > + ERROR(L"pmc err: cmd=0x%x ctl=0x%x devid=0x%x size=0x%x\n",
> > + cmd, ctl, devid, size);
> > + return err;
> > +}
> > +
> > +static EFI_STATUS pmc_wdt_read(const struct pmc_port *p, UINT8 ctl,
> > + UINT8 *payload, UINT8 size)
> > +{
> > + return pmc_cmd_exec(p, PMC_WDT_CMD_READ, ctl, 0, payload, size);
> > +}
> > +
> > +static EFI_STATUS pmc_wdt_write(const struct pmc_port *p, UINT8 ctl,
> > + UINT8 *payload, UINT8 size)
> > +{
> > + return pmc_cmd_exec(p, PMC_WDT_CMD_WRITE, ctl, 0, payload, size);
> > +}
> > +
> > +static EFI_STATUS pmc_wdt_set_reset_timeout(const struct pmc_port *p,
> > + UINT32 msec)
> > +{
> > + UINT8 payload[4];
> > +
> > + if (msec < PMC_WDT_MIN_TIMEOUT_MS)
> > + msec = PMC_WDT_MIN_TIMEOUT_MS;
> > + else if (msec > PMC_WDT_MAX_TIMEOUT_MS)
> > + msec = PMC_WDT_MAX_TIMEOUT_MS;
> > +
> > + payload[0] = msec & 0xff;
> > + payload[1] = (msec >> 8) & 0xff;
> > + payload[2] = (msec >> 16) & 0xff;
> > + payload[3] = (msec >> 24) & 0xff;
> > +
> > + return pmc_wdt_write(p, WDT_REG_RESET_EVT_TIME,
> > + payload, sizeof(payload));
> > +}
> > +
> > +static EFI_STATUS pmc_wdt_start(const struct pmc_port *p)
> > +{
> > + UINT8 payload = PMC_WDT_CTRL_START;
> > +
> > + return pmc_wdt_write(p, WDT_REG_CONTROL, &payload, 1);
> > +}
> > +
> > +static EFI_STATUS pmc_wdt_status(const struct pmc_port *p, UINT8 *status)
> > +{
> > + EFI_STATUS err;
> > +
> > + err = pmc_wdt_read(p, WDT_REG_STATUS, status, 1);
> > + if (EFI_ERROR(err))
> > + return err;
> > +
> > + return EFI_SUCCESS;
> > +}
> > +
> > +static EFI_STATUS init(EFI_PCI_IO __attribute__((unused)) * pci_io,
> > + UINT16 __attribute__((unused)) pci_vendor_id,
> > + UINT16 __attribute__((unused)) pci_device_id,
> > + UINTN timeout)
> > +{
> > + const struct eiois200_dev_port *eport;
> > + struct pmc_port pmc;
> > + UINT8 status;
> > + EFI_STATUS err;
> > +
> > + /* We do not use PCI, and machines may have many PCI devices */
>
> See above: We do need some extra detection logic to avoid reading from
> or even writing to random (from the POV of other hardware) I/O ports.
>
I'll see what I can do about this.
Thanks again for your suggestions!
Marc
--
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/CAMRMzCA-CQX82b_O1-EAXyq2cHa1z8uyFBMknYHbUC-bBKS%2B3A%40mail.gmail.com.
