Re: [PATCH 1/9] hw/watchdog/wdt_aspeed: Map the whole MMIO range

[Philippe Mathieu-Daudé]

On 29/12/22 21:42, Peter Delevoryas wrote:
> On Thu, Dec 29, 2022 at 04:23:17PM +0100, Philippe Mathieu-Daudé wrote:
> > Avoid confusing two different things:
> > - the WDT I/O region size ('iosize')
> > - at which offset the SoC map the WDT ('offset')
> > While it is often the same, we can map smaller region sizes at
> > larger offsets.
> >
> > Here we are interested in the I/O region size. Rename as 'iosize'
> > and map the whole range, not only the first implemented registers.
> > Unimplemented registers accesses are already logged as guest-errors.
> >
> > Otherwise when booting the demo in [*] we get:
> >
> >    aspeed.io: unimplemented device write (size 4, offset 0x1851a8, value 
> > 0x030f1ff1)
> >    aspeed.io: unimplemented device write (size 4, offset 0x1851ac, value 
> > 0x03fffff1)
> >    aspeed.io: unimplemented device write (size 4, offset 0x185128, value 
> > 0x030f1ff1)
> >    aspeed.io: unimplemented device write (size 4, offset 0x18512c, value 
> > 0x03fffff1)
> >
> > [*] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   hw/arm/aspeed_ast10x0.c          |  2 +-
> >   hw/arm/aspeed_ast2600.c          |  2 +-
> >   hw/arm/aspeed_soc.c              |  2 +-
> >   hw/watchdog/wdt_aspeed.c         | 12 +++++++-----
> >   include/hw/watchdog/wdt_aspeed.h |  2 +-
> >   5 files changed, 11 insertions(+), 9 deletions(-)


> > diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> > index d753693a2e..c1311ce74c 100644
> > --- a/hw/watchdog/wdt_aspeed.c
> > +++ b/hw/watchdog/wdt_aspeed.c
> > @@ -260,6 +260,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error 
> > **errp)
> >   {
> >       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >       AspeedWDTState *s = ASPEED_WDT(dev);
> > +    AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(dev);
> >   
> >       assert(s->scu);
> >   
> > @@ -270,8 +271,9 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
> >        */
> >       s->pclk_freq = PCLK_HZ;
> >   
> > +    assert(awc->iosize >= ASPEED_WDT_REGS_MAX * 4);
> >       memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_wdt_ops, s,
> > -                          TYPE_ASPEED_WDT, ASPEED_WDT_REGS_MAX * 4);
> > +                          TYPE_ASPEED_WDT, awc->iosize);
>
> Does this fix the unimplemented thing you referred to in the commit message?

Yes, I'll reword the description to be clearer.

> I'm not sure which part of this diff actually removes the unimplemented traces.

Having:

  #define ASPEED_WDT_REGS_MAX        (0x20 / 4)

Before this patch, we map regions of 0x20 ...

> > @@ -392,7 +394,7 @@ static void aspeed_1030_wdt_class_init(ObjectClass *klass, 
> > void *data)
> >       AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
> >   
> >       dc->desc = "ASPEED 1030 Watchdog Controller";
> > -    awc->offset = 0x80;
> > +    awc->iosize = 0x80;

... every span of 0x80, so there is a gap of 0x60, apparently accessed
by the Zephyr WDT driver (address 0x28 - register #10 - is accessed in
the traces).

From the driver source we can see [1] added in [2]:

    #define WDT_RELOAD_VAL_REG          0x0004
    #define WDT_RESTART_REG             0x0008
    #define WDT_CTRL_REG                0x000C
    #define WDT_TIMEOUT_STATUS_REG      0x0010
    #define WDT_TIMEOUT_STATUS_CLR_REG  0x0014
    #define WDT_RESET_MASK1_REG         0x001C
    #define WDT_RESET_MASK2_REG         0x0020
    #define WDT_SW_RESET_MASK1_REG      0x0028   <------
    #define WDT_SW_RESET_MASK2_REG      0x002C
    #define WDT_SW_RESET_CTRL_REG       0x0024

So the traces likely correspond to this code:

  static int aspeed_wdt_init(const struct device *dev)
  {
    const struct aspeed_wdt_config *config = dev->config;
    struct aspeed_wdt_data *const data = dev->data;
    uint32_t reg_val;

    /* disable WDT by default */
    reg_val = sys_read32(config->ctrl_base + WDT_CTRL_REG);
    reg_val &= ~WDT_CTRL_ENABLE;
    sys_write32(reg_val, config->ctrl_base + WDT_CTRL_REG);

    sys_write32(data->rst_mask1,
                config->ctrl_base + WDT_SW_RESET_MASK1_REG);
    sys_write32(data->rst_mask2,
                config->ctrl_base + WDT_SW_RESET_MASK2_REG);

    return 0;
  }

After this patch, we map (in this case) a MMIO region of 0x80.
Accesses to address 0x28 hits this device model but is handled
as 'WDT register not covered'.

Better would be to extend the Aspeed WDT model in QEMU, or at
least report the valid-but-unimplemented registers as UNIMP instead
of GUEST_ERRORS.

[1] 
https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
[2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b

> > diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
> > index dfa5dfa424..db91ee6b51 100644
> > --- a/include/hw/watchdog/wdt_aspeed.h
> > +++ b/include/hw/watchdog/wdt_aspeed.h
> > @@ -40,7 +40,7 @@ struct AspeedWDTState {
> >   struct AspeedWDTClass {
> >       SysBusDeviceClass parent_class;
> >   
> > -    uint32_t offset;
> > +    uint32_t iosize;
>
> Oh yeah, iosize is a way better name for this. +1. But to be honest, I don't
> understand how this is changing the unimplemented traces?
>
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>

Thanks!

Phil.