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