diff mbox series

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

Message ID 20221230113504.37032-3-philmd@linaro.org (mailing list archive)
State New, archived
Headers show
Series hw/arm/aspeed_ast10x0: Map more peripherals & few more fixes | expand

Commit Message

Philippe Mathieu-Daudé Dec. 30, 2022, 11:34 a.m. UTC
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.

[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(-)

Comments

Philippe Mathieu-Daudé Dec. 30, 2022, 12:31 p.m. UTC | #1
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.

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);
>   }
>
Peter Delevoryas Dec. 30, 2022, 6:31 p.m. UTC | #2
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);
> >   }
>
Dong, Eddie Dec. 31, 2022, 10:52 p.m. UTC | #3
> 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),
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
>
Philippe Mathieu-Daudé Jan. 2, 2023, 10:05 a.m. UTC | #4
On 31/12/22 23:52, Dong, Eddie wrote:

>>    #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),
> while iosize is for all devices, and its initial value comes from the per device type REGS_MAX.

You are correct. I don't have access to the specifications neither,
but I suspect you are right, the current watchdog is modelling an
older version that what the AST1030 has.

I started these WDT patches to understand the unimplemented accesses
done by Zephyr, but eventually they resulted irrelevant to the fixes
(see following patches) so I'll simply drop them.

Thanks,

Phil.
Cédric Le Goater Jan. 2, 2023, 1:31 p.m. UTC | #5
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
AST2600   0x30/4      0x40
AST1030   0x4C/4      0x80


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
>>
>
Peter Delevoryas Jan. 3, 2023, 3:31 p.m. UTC | #6
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
> > > 
> > 
>
Cédric Le Goater Jan. 3, 2023, 3:48 p.m. UTC | #7
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.
Peter Delevoryas Jan. 3, 2023, 3:52 p.m. UTC | #8
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.
>
Cédric Le Goater Jan. 18, 2023, 8:48 a.m. UTC | #9
Philippe,

On 1/2/23 14:31, 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
> AST2600   0x30/4      0x40
> AST1030   0x4C/4      0x80
> 

That might be a big change for the next respin. If you don't have time, we can
make adjustments later. May be add a TODO with the above values ?

Thanks,

C.

  


> 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
>>>
>>
>
diff mbox series

Patch

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);
 }