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

[Cédric Le Goater]

On 1/3/23 16:31, Peter Delevoryas wrote:
> 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.

yes.

> > 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.

Hmm, I see 5 extra registers in the AST1030 SoC compared to the AST2600 SoC. All
related to write protection.

C.