Re: [PATCH v2 01/11] hw/watchdog/wdt_aspeed: Rename MMIO region size as 'iosize'

[Peter Delevoryas]

On Fri, Dec 30, 2022 at 12:34:54PM +0100, Philippe Mathieu-Daudé wrote:
> Avoid confusing two different things:
> - the WDT I/O region size ('iosize')
> - at which offset the SoC map the WDT ('offset')
> While it is often the same, we can map smaller region sizes
> at larger offsets.
> 
> Here we are interested in the I/O region size, so rename as
> 'iosize'.
> 
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/aspeed_ast10x0.c          | 2 +-
>  hw/arm/aspeed_ast2600.c          | 2 +-
>  hw/arm/aspeed_soc.c              | 2 +-
>  hw/watchdog/wdt_aspeed.c         | 8 ++++----
>  include/hw/watchdog/wdt_aspeed.h | 2 +-
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 4d0b9b115f..122b3fd3f3 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -325,7 +325,7 @@ static void aspeed_soc_ast1030_realize(DeviceState 
> *dev_soc, Error **errp)
>              return;
>          }
>          aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>      }
>  
>      /* GPIO */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index cd75465c2b..a79e05ddbd 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -472,7 +472,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
> Error **errp)
>              return;
>          }
>          aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>      }
>  
>      /* RAM */
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index b05b9dd416..2c0924d311 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -393,7 +393,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
> **errp)
>              return;
>          }
>          aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->wdt[i]), 0,
> -                        sc->memmap[ASPEED_DEV_WDT] + i * awc->offset);
> +                        sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize);
>      }
>  
>      /* RAM  */
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index d753693a2e..958725a1b5 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -309,7 +309,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass 
> *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>  
>      dc->desc = "ASPEED 2400 Watchdog Controller";
> -    awc->offset = 0x20;
> +    awc->iosize = 0x20;
>      awc->ext_pulse_width_mask = 0xff;
>      awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>      awc->wdt_reload = aspeed_wdt_reload;
> @@ -346,7 +346,7 @@ static void aspeed_2500_wdt_class_init(ObjectClass 
> *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>  
>      dc->desc = "ASPEED 2500 Watchdog Controller";
> -    awc->offset = 0x20;
> +    awc->iosize = 0x20;
>      awc->ext_pulse_width_mask = 0xfffff;
>      awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
> @@ -369,7 +369,7 @@ static void aspeed_2600_wdt_class_init(ObjectClass 
> *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>  
>      dc->desc = "ASPEED 2600 Watchdog Controller";
> -    awc->offset = 0x40;
> +    awc->iosize = 0x40;
>      awc->ext_pulse_width_mask = 0xfffff; /* TODO */
>      awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
> @@ -392,7 +392,7 @@ static void aspeed_1030_wdt_class_init(ObjectClass 
> *klass, void *data)
>      AspeedWDTClass *awc = ASPEED_WDT_CLASS(klass);
>  
>      dc->desc = "ASPEED 1030 Watchdog Controller";
> -    awc->offset = 0x80;
> +    awc->iosize = 0x80;

Just noticed, the offset/iosize for the AST1030 seems to be incorrect. It
should be 0x40, not 0x80.

It should have the same value as the AST2600, since they should be the exact
same peripheral.

This is not your fault, just a bug report from me.

This seems to be an error in the original patch from Steven Lee, but also a bug 
in the AspeedSDK
Zephyr kernel driver?

e259e01ecb ("aspeed/wdt: Add AST1030 support")
https://github.com/AspeedTech-BMC/zephyr/blob/1b9764a854abbea8b38445f1d5de9f4441e29c3b/drivers/watchdog/wdt_aspeed.c#L21

The only other explanation is that the datasheet I have is old and wrong. I'm
referencing the AST1030 A0 v0.5 datasheet from February 2021.

Sorry for not noticing this earlier!

So to be clear:

Chip: Watchdog[N] Iosize
AST2400: 0x20
AST2500: 0x20
AST2600: 0x40
AST1030: 0x40

Chip: Watchdog[N] WDT_REGS_MAX
AST2400: 0x18
AST2500: 0x1C (Added "Reset Mask" register)
AST2600: 0x30 (Added even more reset control and mask registers)
AST1030: 0x30

Thanks,
Peter

>      awc->ext_pulse_width_mask = 0xfffff; /* TODO */
>      awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
> diff --git a/include/hw/watchdog/wdt_aspeed.h 
> b/include/hw/watchdog/wdt_aspeed.h
> index dfa5dfa424..db91ee6b51 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -40,7 +40,7 @@ struct AspeedWDTState {
>  struct AspeedWDTClass {
>      SysBusDeviceClass parent_class;
>  
> -    uint32_t offset;
> +    uint32_t iosize;
>      uint32_t ext_pulse_width_mask;
>      uint32_t reset_ctrl_reg;
>      void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
> -- 
> 2.38.1
> 
>