diff mbox series

[v2,2/8] hw/watchdog: Implement full i.MX watchdog support

Message ID 20200322211919.11335-3-linux@roeck-us.net
State New
Headers show
Series hw/arm: Implement i.MX watchdog support | expand

Commit Message

Guenter Roeck March 22, 2020, 9:19 p.m. UTC
Implement full support for the watchdog in i.MX systems.
Pretimeout support is optional because the watchdog hardware on i.MX31
does not support pretimeouts.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Fixup of CONFIG_WDT_IMX -> CONFIG_WDT_IMX2 moved to patch 1/8

 hw/watchdog/wdt_imx2.c         | 196 +++++++++++++++++++++++++++++++--
 include/hw/watchdog/wdt_imx2.h |  49 ++++++++-
 2 files changed, 231 insertions(+), 14 deletions(-)

Comments

Peter Maydell April 16, 2020, 3:20 p.m. UTC | #1
On Sun, 22 Mar 2020 at 21:19, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Implement full support for the watchdog in i.MX systems.
> Pretimeout support is optional because the watchdog hardware on i.MX31
> does not support pretimeouts.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Fixup of CONFIG_WDT_IMX -> CONFIG_WDT_IMX2 moved to patch 1/8

Sorry for not getting to this earlier, I've been focusing on
work for the 5.0 release. Some comments below, but overall
this looks pretty good.

>
>  hw/watchdog/wdt_imx2.c         | 196 +++++++++++++++++++++++++++++++--
>  include/hw/watchdog/wdt_imx2.h |  49 ++++++++-
>  2 files changed, 231 insertions(+), 14 deletions(-)
>
> diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c
> index ad1ef02e9e..f5339f3590 100644
> --- a/hw/watchdog/wdt_imx2.c
> +++ b/hw/watchdog/wdt_imx2.c
> @@ -13,24 +13,157 @@
>  #include "qemu/bitops.h"
>  #include "qemu/module.h"
>  #include "sysemu/watchdog.h"
> +#include "migration/vmstate.h"
> +#include "hw/qdev-properties.h"
>
>  #include "hw/watchdog/wdt_imx2.h"
>
> -#define IMX2_WDT_WCR_WDA    BIT(5)      /* -> External Reset WDOG_B */
> -#define IMX2_WDT_WCR_SRS    BIT(4)      /* -> Software Reset Signal */
> +static void imx2_wdt_interrupt(void *opaque)
> +{
> +    IMX2WdtState *s = IMX2_WDT(opaque);
> +
> +    s->wicr |= IMX2_WDT_WICR_WTIS;
> +    qemu_set_irq(s->irq, 1);
> +}
>
> -static uint64_t imx2_wdt_read(void *opaque, hwaddr addr,
> -                              unsigned int size)
> +static void imx2_wdt_expired(void *opaque)
>  {
> +    IMX2WdtState *s = IMX2_WDT(opaque);
> +
> +    s->wrsr = IMX2_WDT_WRSR_TOUT;
> +
> +    /* Perform watchdog action if watchdog is enabled */
> +    if (s->wcr & IMX2_WDT_WCR_WDE) {
> +        watchdog_perform_action();
> +    }
> +}
> +
> +static void imx2_wdt_reset(DeviceState *dev)
> +{
> +    IMX2WdtState *s = IMX2_WDT(dev);
> +
> +    s->wcr = IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS;
> +    s->wsr = 0;
> +    s->wrsr &= ~(IMX2_WDT_WRSR_TOUT | IMX2_WDT_WRSR_SFTW);
> +    s->wicr = 4;
> +    s->wmcr = IMX2_WDT_WMCR_PDE;

Your reset function probably also needs to ptimer_stop()
the timers or otherwise put them into whatever is the
correct state for the device-as-reset.

> +}

> +
>  static void imx2_wdt_write(void *opaque, hwaddr addr,
>                             uint64_t value, unsigned int size)
>  {
> -    if (addr == IMX2_WDT_WCR &&
> -        (~value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS))) {
> -        watchdog_perform_action();
> +    IMX2WdtState *s = IMX2_WDT(opaque);
> +
> +    switch (addr) {
> +    case IMX2_WDT_WCR:
> +        s->wcr = value;
> +        if (!(value & IMX2_WDT_WCR_SRS)) {
> +            s->wrsr = IMX2_WDT_WRSR_SFTW;
> +        }
> +        if (!(value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS)) ||
> +            (!(value & IMX2_WDT_WCR_WT) && (value & IMX2_WDT_WCR_WDE))) {
> +            watchdog_perform_action();
> +        }
> +        s->wcr |= IMX2_WDT_WCR_SRS;
> +        imx_wdt2_update_timer(s, true);
> +        break;
> +    case IMX2_WDT_WSR:
> +        if (s->wsr == IMX2_WDT_SEQ1 && value == IMX2_WDT_SEQ2) {
> +            imx_wdt2_update_timer(s, false);
> +        }
> +        s->wsr = value;
> +        break;
> +    case IMX2_WDT_WRSR:
> +        break;
> +    case IMX2_WDT_WICR:
> +        if (!s->pretimeout_support) {
> +            return;
> +        }
> +        /* The pretimeout value is write-once */

My imx6 manual says that the WICR WIE bit is also write-once,
so I think that changes to it should also be guarded under
!pretimeout_locked, like the WICT bits.

(In fact quite a lot of registers seem to have write-once bits.)

> +        if (s->pretimeout_locked) {
> +            value &= ~IMX2_WDT_WICR_WICT;
> +            s->wicr &= (IMX2_WDT_WICR_WTIS | IMX2_WDT_WICR_WICT);
> +        } else {
> +            s->wicr &= IMX2_WDT_WICR_WTIS;
> +        }
> +        s->wicr |= value & (IMX2_WDT_WICR_WIE | IMX2_WDT_WICR_WICT);
> +        if (value & IMX2_WDT_WICR_WTIS) {
> +            s->wicr &= ~IMX2_WDT_WICR_WTIS;
> +            qemu_set_irq(s->irq, 0);
> +        }
> +        imx_wdt2_update_itimer(s, true);
> +        s->pretimeout_locked = true;
> +        break;
> +    case IMX2_WDT_WMCR:
> +        s->wmcr = value & IMX2_WDT_WMCR_PDE;
> +        break;
>      }
>  }

>  static void imx2_wdt_realize(DeviceState *dev, Error **errp)
>  {
>      IMX2WdtState *s = IMX2_WDT(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>
>      memory_region_init_io(&s->mmio, OBJECT(dev),
>                            &imx2_wdt_ops, s,
> -                          TYPE_IMX2_WDT".mmio",
> -                          IMX2_WDT_REG_NUM * sizeof(uint16_t));
> -    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
> +                          TYPE_IMX2_WDT,
> +                          IMX2_WDT_MMIO_SIZE);
> +    sysbus_init_mmio(sbd, &s->mmio);
> +    sysbus_init_irq(sbd, &s->irq);
> +
> +    s->timer = ptimer_init(imx2_wdt_expired, s, PTIMER_POLICY_DEFAULT);

PTIMER_POLICY_DEFAULT is almost never the right thing: it
basically means "same as the old legacy behaviour of our
first somewhat-broken implementation of ptimer". The comments
in include/hw/ptimer.h suggests some behaviours real timers
tend to have which can be enabled with suitable bits.
If the default really is the right thing for this timer then
a comment is helpful to indicate that we've looked at this
and made an active decision to use the 'default' rather than
the timer implementation just having been grandfathered in.


> +    ptimer_transaction_begin(s->timer);
> +    ptimer_set_freq(s->timer, 2);
> +    ptimer_set_limit(s->timer, 0xff, 1);
> +    ptimer_transaction_commit(s->timer);
> +    if (s->pretimeout_support) {
> +        s->itimer = ptimer_init(imx2_wdt_interrupt, s, PTIMER_POLICY_DEFAULT);
> +        ptimer_transaction_begin(s->itimer);
> +        ptimer_set_freq(s->itimer, 2);
> +        ptimer_set_limit(s->itimer, 0xff, 1);
> +        ptimer_transaction_commit(s->itimer);
> +    }
>  }

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c
index ad1ef02e9e..f5339f3590 100644
--- a/hw/watchdog/wdt_imx2.c
+++ b/hw/watchdog/wdt_imx2.c
@@ -13,24 +13,157 @@ 
 #include "qemu/bitops.h"
 #include "qemu/module.h"
 #include "sysemu/watchdog.h"
+#include "migration/vmstate.h"
+#include "hw/qdev-properties.h"
 
 #include "hw/watchdog/wdt_imx2.h"
 
-#define IMX2_WDT_WCR_WDA    BIT(5)      /* -> External Reset WDOG_B */
-#define IMX2_WDT_WCR_SRS    BIT(4)      /* -> Software Reset Signal */
+static void imx2_wdt_interrupt(void *opaque)
+{
+    IMX2WdtState *s = IMX2_WDT(opaque);
+
+    s->wicr |= IMX2_WDT_WICR_WTIS;
+    qemu_set_irq(s->irq, 1);
+}
 
-static uint64_t imx2_wdt_read(void *opaque, hwaddr addr,
-                              unsigned int size)
+static void imx2_wdt_expired(void *opaque)
 {
+    IMX2WdtState *s = IMX2_WDT(opaque);
+
+    s->wrsr = IMX2_WDT_WRSR_TOUT;
+
+    /* Perform watchdog action if watchdog is enabled */
+    if (s->wcr & IMX2_WDT_WCR_WDE) {
+        watchdog_perform_action();
+    }
+}
+
+static void imx2_wdt_reset(DeviceState *dev)
+{
+    IMX2WdtState *s = IMX2_WDT(dev);
+
+    s->wcr = IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS;
+    s->wsr = 0;
+    s->wrsr &= ~(IMX2_WDT_WRSR_TOUT | IMX2_WDT_WRSR_SFTW);
+    s->wicr = 4;
+    s->wmcr = IMX2_WDT_WMCR_PDE;
+}
+
+static uint64_t imx2_wdt_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    IMX2WdtState *s = IMX2_WDT(opaque);
+
+    switch (addr) {
+    case IMX2_WDT_WCR:
+        return s->wcr;
+    case IMX2_WDT_WSR:
+        return s->wsr;
+    case IMX2_WDT_WRSR:
+        return s->wrsr;
+    case IMX2_WDT_WICR:
+        return s->wicr;
+    case IMX2_WDT_WMCR:
+        return s->wmcr;
+    }
     return 0;
 }
 
+static void imx_wdt2_update_itimer(IMX2WdtState *s, bool start)
+{
+    bool running = (s->wcr & IMX2_WDT_WCR_WDE) && (s->wcr & IMX2_WDT_WCR_WT);
+    bool enabled = s->wicr & IMX2_WDT_WICR_WIE;
+
+    ptimer_transaction_begin(s->itimer);
+    if (start || !enabled) {
+        ptimer_stop(s->itimer);
+    }
+    if (running && enabled) {
+        int count = ptimer_get_count(s->timer);
+        int pretimeout = s->wicr & IMX2_WDT_WICR_WICT;
+
+        /*
+         * Only (re-)start pretimeout timer if its counter value is larger
+         * than 0. Otherwise it will fire right away and we'll get an
+         * interrupt loop.
+         */
+        if (count > pretimeout) {
+            ptimer_set_count(s->itimer, count - pretimeout);
+            if (start) {
+                ptimer_run(s->itimer, 1);
+            }
+        }
+    }
+    ptimer_transaction_commit(s->itimer);
+}
+
+static void imx_wdt2_update_timer(IMX2WdtState *s, bool start)
+{
+    ptimer_transaction_begin(s->timer);
+    if (start) {
+        ptimer_stop(s->timer);
+    }
+    if ((s->wcr & IMX2_WDT_WCR_WDE) && (s->wcr & IMX2_WDT_WCR_WT)) {
+        int count = (s->wcr & IMX2_WDT_WCR_WT) >> 8;
+
+        ptimer_set_count(s->timer, count);
+        if (start) {
+            ptimer_run(s->timer, 1);
+        }
+    }
+    ptimer_transaction_commit(s->timer);
+    if (s->pretimeout_support) {
+        imx_wdt2_update_itimer(s, start);
+    }
+}
+
 static void imx2_wdt_write(void *opaque, hwaddr addr,
                            uint64_t value, unsigned int size)
 {
-    if (addr == IMX2_WDT_WCR &&
-        (~value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS))) {
-        watchdog_perform_action();
+    IMX2WdtState *s = IMX2_WDT(opaque);
+
+    switch (addr) {
+    case IMX2_WDT_WCR:
+        s->wcr = value;
+        if (!(value & IMX2_WDT_WCR_SRS)) {
+            s->wrsr = IMX2_WDT_WRSR_SFTW;
+        }
+        if (!(value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS)) ||
+            (!(value & IMX2_WDT_WCR_WT) && (value & IMX2_WDT_WCR_WDE))) {
+            watchdog_perform_action();
+        }
+        s->wcr |= IMX2_WDT_WCR_SRS;
+        imx_wdt2_update_timer(s, true);
+        break;
+    case IMX2_WDT_WSR:
+        if (s->wsr == IMX2_WDT_SEQ1 && value == IMX2_WDT_SEQ2) {
+            imx_wdt2_update_timer(s, false);
+        }
+        s->wsr = value;
+        break;
+    case IMX2_WDT_WRSR:
+        break;
+    case IMX2_WDT_WICR:
+        if (!s->pretimeout_support) {
+            return;
+        }
+        /* The pretimeout value is write-once */
+        if (s->pretimeout_locked) {
+            value &= ~IMX2_WDT_WICR_WICT;
+            s->wicr &= (IMX2_WDT_WICR_WTIS | IMX2_WDT_WICR_WICT);
+        } else {
+            s->wicr &= IMX2_WDT_WICR_WTIS;
+        }
+        s->wicr |= value & (IMX2_WDT_WICR_WIE | IMX2_WDT_WICR_WICT);
+        if (value & IMX2_WDT_WICR_WTIS) {
+            s->wicr &= ~IMX2_WDT_WICR_WTIS;
+            qemu_set_irq(s->irq, 0);
+        }
+        imx_wdt2_update_itimer(s, true);
+        s->pretimeout_locked = true;
+        break;
+    case IMX2_WDT_WMCR:
+        s->wmcr = value & IMX2_WDT_WMCR_PDE;
+        break;
     }
 }
 
@@ -45,28 +178,67 @@  static const MemoryRegionOps imx2_wdt_ops = {
          * real device but in practice there is no reason for a guest
          * to access this device unaligned.
          */
-        .min_access_size = 4,
-        .max_access_size = 4,
+        .min_access_size = 2,
+        .max_access_size = 2,
         .unaligned = false,
     },
 };
 
+static const VMStateDescription vmstate_imx2_wdt = {
+    .name = "imx2.wdt",
+    .fields = (VMStateField[]) {
+        VMSTATE_PTIMER(timer, IMX2WdtState),
+        VMSTATE_PTIMER(itimer, IMX2WdtState),
+        VMSTATE_BOOL(pretimeout_locked, IMX2WdtState),
+        VMSTATE_UINT16(wcr, IMX2WdtState),
+        VMSTATE_UINT16(wsr, IMX2WdtState),
+        VMSTATE_UINT16(wrsr, IMX2WdtState),
+        VMSTATE_UINT16(wmcr, IMX2WdtState),
+        VMSTATE_UINT16(wicr, IMX2WdtState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void imx2_wdt_realize(DeviceState *dev, Error **errp)
 {
     IMX2WdtState *s = IMX2_WDT(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
     memory_region_init_io(&s->mmio, OBJECT(dev),
                           &imx2_wdt_ops, s,
-                          TYPE_IMX2_WDT".mmio",
-                          IMX2_WDT_REG_NUM * sizeof(uint16_t));
-    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
+                          TYPE_IMX2_WDT,
+                          IMX2_WDT_MMIO_SIZE);
+    sysbus_init_mmio(sbd, &s->mmio);
+    sysbus_init_irq(sbd, &s->irq);
+
+    s->timer = ptimer_init(imx2_wdt_expired, s, PTIMER_POLICY_DEFAULT);
+    ptimer_transaction_begin(s->timer);
+    ptimer_set_freq(s->timer, 2);
+    ptimer_set_limit(s->timer, 0xff, 1);
+    ptimer_transaction_commit(s->timer);
+    if (s->pretimeout_support) {
+        s->itimer = ptimer_init(imx2_wdt_interrupt, s, PTIMER_POLICY_DEFAULT);
+        ptimer_transaction_begin(s->itimer);
+        ptimer_set_freq(s->itimer, 2);
+        ptimer_set_limit(s->itimer, 0xff, 1);
+        ptimer_transaction_commit(s->itimer);
+    }
 }
 
+static Property imx2_wdt_properties[] = {
+    DEFINE_PROP_BOOL("pretimeout-support", IMX2WdtState, pretimeout_support,
+                     false),
+};
+
 static void imx2_wdt_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    device_class_set_props(dc, imx2_wdt_properties);
     dc->realize = imx2_wdt_realize;
+    dc->reset = imx2_wdt_reset;
+    dc->vmsd = &vmstate_imx2_wdt;
+    dc->desc = "i.MX watchdog timer";
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
diff --git a/include/hw/watchdog/wdt_imx2.h b/include/hw/watchdog/wdt_imx2.h
index b91b002528..bed7aa6835 100644
--- a/include/hw/watchdog/wdt_imx2.h
+++ b/include/hw/watchdog/wdt_imx2.h
@@ -12,22 +12,67 @@ 
 #ifndef IMX2_WDT_H
 #define IMX2_WDT_H
 
+#include "qemu/bitops.h"
 #include "hw/sysbus.h"
+#include "hw/irq.h"
+#include "hw/ptimer.h"
 
 #define TYPE_IMX2_WDT "imx2.wdt"
 #define IMX2_WDT(obj) OBJECT_CHECK(IMX2WdtState, (obj), TYPE_IMX2_WDT)
 
 enum IMX2WdtRegisters {
-    IMX2_WDT_WCR     = 0x0000,
-    IMX2_WDT_REG_NUM = 0x0008 / sizeof(uint16_t) + 1,
+    IMX2_WDT_WCR  = 0x0000, /* Control Register */
+    IMX2_WDT_WSR  = 0x0002, /* Service Register */
+    IMX2_WDT_WRSR = 0x0004, /* Reset Status Register */
+    IMX2_WDT_WICR = 0x0006, /* Interrupt Control Register */
+    IMX2_WDT_WMCR = 0x0008, /* Misc Register */
 };
 
+#define IMX2_WDT_MMIO_SIZE 0x000a
+
+/* Control Register definitions */
+#define IMX2_WDT_WCR_WT         (0xFF << 8) /* Watchdog Timeout Field */
+#define IMX2_WDT_WCR_WDA        BIT(5)      /* WDOG Assertion */
+#define IMX2_WDT_WCR_SRS        BIT(4)      /* Software Reset Signal */
+#define IMX2_WDT_WCR_WDT        BIT(3)      /* WDOG Timeout Assertion */
+#define IMX2_WDT_WCR_WDE        BIT(2)      /* Watchdog Enable */
+#define IMX2_WDT_WCR_WDZST      BIT(0)      /* Watchdog Timer Suspend */
+
+/* Service Register definitions */
+#define IMX2_WDT_SEQ1           0x5555      /* service sequence 1 */
+#define IMX2_WDT_SEQ2           0xAAAA      /* service sequence 2 */
+
+/* Reset Status Register definitions */
+#define IMX2_WDT_WRSR_TOUT      BIT(1)      /* Reset due to Timeout */
+#define IMX2_WDT_WRSR_SFTW      BIT(0)      /* Reset due to Timeout */
+
+/* Interrupt Control Register definitions */
+#define IMX2_WDT_WICR_WIE       BIT(15)     /* Interrupt Enable */
+#define IMX2_WDT_WICR_WTIS      BIT(14)     /* Interrupt Status */
+#define IMX2_WDT_WICR_WICT      0xff        /* Interrupt Timeout */
+
+/* Misc Control Register definitions */
+#define IMX2_WDT_WMCR_PDE       BIT(0)      /* Power-Down Enable */
 
 typedef struct IMX2WdtState {
     /* <private> */
     SysBusDevice parent_obj;
 
+    /*< public >*/
     MemoryRegion mmio;
+    qemu_irq irq;
+
+    struct ptimer_state *timer;
+    struct ptimer_state *itimer;
+
+    bool pretimeout_support;
+    bool pretimeout_locked;
+
+    uint16_t wcr;
+    uint16_t wsr;
+    uint16_t wrsr;
+    uint16_t wicr;
+    uint16_t wmcr;
 } IMX2WdtState;
 
 #endif /* IMX2_WDT_H */