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

[Peter Delevoryas]

On Tue, Jan 03, 2023 at 04:48:14PM +0100, Cédric Le Goater wrote:
> 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.

Oh really? Hmmm ok, perhaps my datasheet is outdated then, I'm referencing
AST1030 A0 v0.5 from Feb 2021, which might be outdated.

Thanks, sorry for the confusion. Erg.
Peter

> 
> C.
>