Re: [PATCH v2 02/11] hw/watchdog/wdt_aspeed: Extend MMIO range to cover more registers

[Peter Delevoryas]

On Fri, Dec 30, 2022 at 01:31:35PM +0100, Philippe Mathieu-Daudé wrote:
> On 30/12/22 12:34, Philippe Mathieu-Daudé wrote:
> > When booting the Zephyr demo in [1] we get:
> > 
> >    aspeed.io: unimplemented device write (size 4, offset 0x185128, value 
> > 0x030f1ff1) <--
> >    aspeed.io: unimplemented device write (size 4, offset 0x18512c, value 
> > 0x03fffff1)
> > 
> > This corresponds to this Zephyr code [2]:
> > 
> >    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;
> >    }
> > 
> > The register definitions are [3]:
> > 
> >    #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
> > 
> > Currently QEMU only cover a MMIO region of size 0x20:
> > 
> >    #define ASPEED_WDT_REGS_MAX        (0x20 / 4)
> > 
> > Change to map the whole 'iosize' which might be bigger, covering
> > the other registers. The MemoryRegionOps read/write handlers will
> > report the accesses as out-of-bounds guest-errors, but the next
> > commit will report them as unimplemented.

Ahhhh I see, this makes perfect sense now, thanks for the detail!

> 
> I'll amend here for clarity:
> 
> ---
> 
> Memory layout before this change:
> 
>   (qemu) info mtree -f
>     ...
>     000000007e785000-000000007e78501f (prio 0, i/o): aspeed.wdt
>     000000007e785020-000000007e78507f (prio -1000, i/o): aspeed.io
> @0000000000185020
>     000000007e785080-000000007e78509f (prio 0, i/o): aspeed.wdt
>     000000007e7850a0-000000007e7850ff (prio -1000, i/o): aspeed.io
> @00000000001850a0
>     000000007e785100-000000007e78511f (prio 0, i/o): aspeed.wdt
>     000000007e785120-000000007e78517f (prio -1000, i/o): aspeed.io
> @0000000000185120
>     000000007e785180-000000007e78519f (prio 0, i/o): aspeed.wdt
>     000000007e7851a0-000000007e788fff (prio -1000, i/o): aspeed.io
> @00000000001851a0
> 
> After:
> 
>   (qemu) info mtree -f
>     ...
>     000000007e785000-000000007e78507f (prio 0, i/o): aspeed.wdt
>     000000007e785080-000000007e7850ff (prio 0, i/o): aspeed.wdt
>     000000007e785100-000000007e78517f (prio 0, i/o): aspeed.wdt
>     000000007e785180-000000007e7851ff (prio 0, i/o): aspeed.wdt
>     000000007e785200-000000007e788fff (prio -1000, i/o): aspeed.io
> @0000000000185200
> ---
> 
> > [1] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07
> > [2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b
> > [3] 
> > https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31
> > 
> > Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   hw/watchdog/wdt_aspeed.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> > index 958725a1b5..eefca31ae4 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);
> > @@ -271,7 +272,7 @@ static void aspeed_wdt_realize(DeviceState *dev, Error 
> > **errp)
> >       s->pclk_freq = PCLK_HZ;
> >       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);
> >       sysbus_init_mmio(sbd, &s->iomem);
> >   }
>