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

[Peter Maydell]

On Sun, 22 Mar 2020 at 21:19, Guenter Roeck <address@hidden> 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 <address@hidden>
> ---
> 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