Re: [PATCH v7 1/2] hw/watchdog: Implement SBSA watchdog device

[Peter Maydell]

On Fri, 23 Oct 2020 at 17:01, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Generic watchdog device model implementation as per ARM SBSA v6.0
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>

Thanks for the respin; only a couple of minor issues left.

General note: you implement WS0 with an external qemu_irq line.
You correctly call qemu_set_irq(s->irq, 1) when you set the WS0
bit in sbsa_gwdt_timer_sysinterrupt(), but you also need to lower
the line by calling qemu_set_irq(s->irq, 0) whenever WS0 is cleared.
(The exception to this is that you mustn't call qemu_set_irq()
in the sysbus reset method, because it can confuse the device
on the other end of the irq line which is also going to be
resetting.)

Otherwise I just have some minor nits about bit operation style.

> +static void sbsa_gwdt_write(void *opaque, hwaddr offset, uint64_t data,
> +                             unsigned size) {
> +    SBSA_GWDTState *s = SBSA_GWDT(opaque);
> +    bool enable;
> +
> +    switch (offset) {
> +    case SBSA_GWDT_WCS:
> +        enable = data & SBSA_GWDT_WCS_EN;
> +        if (enable) {
> +            s->wcs |= SBSA_GWDT_WCS_EN;
> +        } else {
> +            s->wcs &= ~SBSA_GWDT_WCS_EN;
> +        }
> +        s->wcs &= ~SBSA_GWDT_WCS_WS0;
> +        s->wcs &= ~SBSA_GWDT_WCS_WS1;

These 8 lines are a long-winded way to write:

  /* All writes clear WS0 and WS1 */
  s->wcs = data & SBSA_GWDT_WCS_EN;

> +        sbsa_gwdt_update_timer(s, EXPLICIT_REFRESH);
> +        break;
> +
> +    case SBSA_GWDT_WOR:
> +        /*
> +         * store the lower 32 bits of WOR for now.
> +         * explicit refresh to be triggered on upper 16 bits
> +         * being written to WORU register (that follows this write)
> +         */

There's nothing in the spec that I can see that obliges the guest
to write the high half last. I think you should call the
timer update function and clear WS0/WS1 for a write here as well
as one to WORU.

> +        s->worl = data;
> +        break;
> +
> +    case SBSA_GWDT_WORU:
> +        s->woru = data & SBSA_GWDT_WOR_MASK;
> +        s->wcs &= ~SBSA_GWDT_WCS_WS0;
> +        s->wcs &= ~SBSA_GWDT_WCS_WS1;

You might as well combine these into one line:
           s->wcs &= ~(SBSA_GWDT_WCS_WS0 | SBSA_GWDT_WCS_WS1);
(same applies for the same code in the SBSA_GWDT_WRR write handling.)

> +        sbsa_gwdt_update_timer(s, EXPLICIT_REFRESH);
> +        break;
> +
> +    case SBSA_GWDT_WCV:
> +        s->wcvl = data;
> +        break;
> +
> +    case SBSA_GWDT_WCVU:
> +        s->wcvu = data;
> +        break;
> +
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "bad address in control frame write :"
> +                " 0x%x\n", (int)offset);
> +    }
> +    return;
> +}
> +
> +static void wdt_sbsa_gwdt_reset(DeviceState *dev)
> +{
> +    SBSA_GWDTState *s = SBSA_GWDT(dev);
> +
> +    timer_del(s->timer);
> +
> +    s->wcs &= ~SBSA_GWDT_WCS_EN;
> +    s->wcs &= ~SBSA_GWDT_WCS_WS0;
> +    s->wcs &= ~SBSA_GWDT_WCS_WS1;

Just saying s->wcs = 0; is clearer than clearing all
the bits one by one.

> +    s->wcvl = 0;
> +    s->wcvu = 0;
> +    s->worl = 0;
> +    s->woru = 0;
> +    s->id = SBSA_GWDT_ID;
> +}

thanks
-- PMM