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

[Peter Delevoryas]

On Mon, Jan 02, 2023 at 02:31:31PM +0100, Cédric Le Goater wrote:
> On 12/31/22 23:52, Dong, Eddie 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
> > 
> > The root cause is that ASPEED_WDT_REGS_MAX is too small, right?
> > Probably the Qemu is emulating an old version of the hardware.
> > 
> > Given the meaning of ASPEED_WDT_REGS_MAX, it should be larger than iosize, 
> > not?
> > Probably ASPEED_WDT_REGS_MAX should be per device type (aspeed_2400/2500),
> 
> yes. We would need a new class attribute for it. Please use these values, they
> should be correct.
> 
>            #regs    iosize
> 
> AST2400   0x18/4      0x20
> AST2500   0x20/4      0x20

I think only one additional register was added in the AST2500, bringing it to 
0x1C.

> AST2600   0x30/4      0x40
> AST1030   0x4C/4      0x80

I know the Zephyr driver for the AST1030 directly from Aspeed is claiming that
the iosize is 0x80, but the datasheet I have says it's only 0x40. And, that the
#regs would still just be 0x30/4. Afaik the AST2600 and AST1030 should have the
exact same peripheral.

Peter

> 
> 
> AFAICT, the WDT logic was changed in a compatible way with the previous 
> generation.
> 
> Thanks
> 
> C.
> 
> > while iosize is for all devices, and its initial value comes from the per 
> > device type REGS_MAX.
> > 
> > > 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.
> > > 
> > > [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);
> > >   }
> > > 
> > > --
> > > 2.38.1
> > > 
> > 
>