| Message ID | 20221229152325.32041-2-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/aspeed_ast10x0: Map more peripherals & few more fixes | expand |
On Thu, Dec 29, 2022 at 04:23:17PM +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. Rename as 'iosize' > and map the whole range, not only the first implemented registers. > Unimplemented registers accesses are already logged as guest-errors. > > Otherwise when booting the demo in [*] we get: > > aspeed.io: unimplemented device write (size 4, offset 0x1851a8, value 0x030f1ff1) > aspeed.io: unimplemented device write (size 4, offset 0x1851ac, value 0x03fffff1) > aspeed.io: unimplemented device write (size 4, offset 0x185128, value 0x030f1ff1) > aspeed.io: unimplemented device write (size 4, offset 0x18512c, value 0x03fffff1) > > [*] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07 > > 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 | 12 +++++++----- > include/hw/watchdog/wdt_aspeed.h | 2 +- > 5 files changed, 11 insertions(+), 9 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..c1311ce74c 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); > > @@ -270,8 +271,9 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp) > */ > s->pclk_freq = PCLK_HZ; > > + assert(awc->iosize >= ASPEED_WDT_REGS_MAX * 4); > 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); Does this fix the unimplemented thing you referred to in the commit message? I'm not sure which part of this diff actually removes the unimplemented traces. > sysbus_init_mmio(sbd, &s->iomem); > } > > @@ -309,7 +311,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 +348,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 +371,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 +394,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; > 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; Oh yeah, iosize is a way better name for this. +1. But to be honest, I don't understand how this is changing the unimplemented traces? Reviewed-by: Peter Delevoryas <peter@pjd.dev> > uint32_t ext_pulse_width_mask; > uint32_t reset_ctrl_reg; > void (*reset_pulse)(AspeedWDTState *s, uint32_t property); > -- > 2.38.1 >
On 29/12/22 21:42, Peter Delevoryas wrote: > On Thu, Dec 29, 2022 at 04:23:17PM +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. Rename as 'iosize' >> and map the whole range, not only the first implemented registers. >> Unimplemented registers accesses are already logged as guest-errors. >> >> Otherwise when booting the demo in [*] we get: >> >> aspeed.io: unimplemented device write (size 4, offset 0x1851a8, value 0x030f1ff1) >> aspeed.io: unimplemented device write (size 4, offset 0x1851ac, value 0x03fffff1) >> aspeed.io: unimplemented device write (size 4, offset 0x185128, value 0x030f1ff1) >> aspeed.io: unimplemented device write (size 4, offset 0x18512c, value 0x03fffff1) >> >> [*] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07 >> >> 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 | 12 +++++++----- >> include/hw/watchdog/wdt_aspeed.h | 2 +- >> 5 files changed, 11 insertions(+), 9 deletions(-) >> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c >> index d753693a2e..c1311ce74c 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); >> >> @@ -270,8 +271,9 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp) >> */ >> s->pclk_freq = PCLK_HZ; >> >> + assert(awc->iosize >= ASPEED_WDT_REGS_MAX * 4); >> 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); > > Does this fix the unimplemented thing you referred to in the commit message? Yes, I'll reword the description to be clearer. > I'm not sure which part of this diff actually removes the unimplemented traces. Having: #define ASPEED_WDT_REGS_MAX (0x20 / 4) Before this patch, we map regions of 0x20 ... >> @@ -392,7 +394,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; ... every span of 0x80, so there is a gap of 0x60, apparently accessed by the Zephyr WDT driver (address 0x28 - register #10 - is accessed in the traces). From the driver source we can see [1] added in [2]: #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 So the traces likely correspond to this code: 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; } After this patch, we map (in this case) a MMIO region of 0x80. Accesses to address 0x28 hits this device model but is handled as 'WDT register not covered'. Better would be to extend the Aspeed WDT model in QEMU, or at least report the valid-but-unimplemented registers as UNIMP instead of GUEST_ERRORS. [1] https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31 [2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b >> 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; > > Oh yeah, iosize is a way better name for this. +1. But to be honest, I don't > understand how this is changing the unimplemented traces? > > Reviewed-by: Peter Delevoryas <peter@pjd.dev> Thanks! Phil.
On 12/29/22 16:23, 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. Rename as 'iosize' > and map the whole range, not only the first implemented registers. > Unimplemented registers accesses are already logged as guest-errors. > > Otherwise when booting the demo in [*] we get: > > aspeed.io: unimplemented device write (size 4, offset 0x1851a8, value 0x030f1ff1) > aspeed.io: unimplemented device write (size 4, offset 0x1851ac, value 0x03fffff1) > aspeed.io: unimplemented device write (size 4, offset 0x185128, value 0x030f1ff1) > aspeed.io: unimplemented device write (size 4, offset 0x18512c, value 0x03fffff1) These are unimplemented registers related to software mode reset, which is a new feature of the AST2600 SoC. The AST10x0 SoCs add a few more regs, hence the larger MMIO width for the WDT registers. Until now, they have been harmless. > > [*] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07 > > 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 | 12 +++++++----- > include/hw/watchdog/wdt_aspeed.h | 2 +- > 5 files changed, 11 insertions(+), 9 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..c1311ce74c 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); > > @@ -270,8 +271,9 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp) > */ > s->pclk_freq = PCLK_HZ; > > + assert(awc->iosize >= ASPEED_WDT_REGS_MAX * 4); > 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); > } > > @@ -309,7 +311,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 +348,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 +371,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 +394,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; > 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);
On 12/30/22 08:32, Philippe Mathieu-Daudé wrote: > On 29/12/22 21:42, Peter Delevoryas wrote: >> On Thu, Dec 29, 2022 at 04:23:17PM +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. Rename as 'iosize' >>> and map the whole range, not only the first implemented registers. >>> Unimplemented registers accesses are already logged as guest-errors. >>> >>> Otherwise when booting the demo in [*] we get: >>> >>> aspeed.io: unimplemented device write (size 4, offset 0x1851a8, value 0x030f1ff1) >>> aspeed.io: unimplemented device write (size 4, offset 0x1851ac, value 0x03fffff1) >>> aspeed.io: unimplemented device write (size 4, offset 0x185128, value 0x030f1ff1) >>> aspeed.io: unimplemented device write (size 4, offset 0x18512c, value 0x03fffff1) >>> >>> [*] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07 >>> >>> 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 | 12 +++++++----- >>> include/hw/watchdog/wdt_aspeed.h | 2 +- >>> 5 files changed, 11 insertions(+), 9 deletions(-) > > >>> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c >>> index d753693a2e..c1311ce74c 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); >>> @@ -270,8 +271,9 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp) >>> */ >>> s->pclk_freq = PCLK_HZ; >>> + assert(awc->iosize >= ASPEED_WDT_REGS_MAX * 4); >>> 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); >> >> Does this fix the unimplemented thing you referred to in the commit message? > > Yes, I'll reword the description to be clearer. > >> I'm not sure which part of this diff actually removes the unimplemented traces. > > Having: > > #define ASPEED_WDT_REGS_MAX (0x20 / 4) > > Before this patch, we map regions of 0x20 ... > >>> @@ -392,7 +394,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; > > ... every span of 0x80, so there is a gap of 0x60, apparently accessed > by the Zephyr WDT driver (address 0x28 - register #10 - is accessed in > the traces). > > From the driver source we can see [1] added in [2]: > > #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 > > So the traces likely correspond to this code: > > 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; > } Yes. These registers have been on the TODO list for 3/4 years :/ > After this patch, we map (in this case) a MMIO region of 0x80. > Accesses to address 0x28 hits this device model but is handled > as 'WDT register not covered'. If we are to change things, adding the definitions of the software mode reset regs in the WDT model would be cleaner. All accesses to these regs could generate an UNIMP log saying that "support for software mode reset" is not implemented. Thanks, C. > > Better would be to extend the Aspeed WDT model in QEMU, or at > least report the valid-but-unimplemented registers as UNIMP instead > of GUEST_ERRORS. > > [1] https://github.com/AspeedTech-BMC/zephyr/blob/v00.01.08/drivers/watchdog/wdt_aspeed.c#L31 > [2] https://github.com/AspeedTech-BMC/zephyr/commit/2e99f10ac27b > >>> 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; >> >> Oh yeah, iosize is a way better name for this. +1. But to be honest, I don't >> understand how this is changing the unimplemented traces? >> >> Reviewed-by: Peter Delevoryas <peter@pjd.dev> > > Thanks! > > Phil. >
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..c1311ce74c 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); @@ -270,8 +271,9 @@ static void aspeed_wdt_realize(DeviceState *dev, Error **errp) */ s->pclk_freq = PCLK_HZ; + assert(awc->iosize >= ASPEED_WDT_REGS_MAX * 4); 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); } @@ -309,7 +311,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 +348,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 +371,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 +394,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; 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);
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. Rename as 'iosize' and map the whole range, not only the first implemented registers. Unimplemented registers accesses are already logged as guest-errors. Otherwise when booting the demo in [*] we get: aspeed.io: unimplemented device write (size 4, offset 0x1851a8, value 0x030f1ff1) aspeed.io: unimplemented device write (size 4, offset 0x1851ac, value 0x03fffff1) aspeed.io: unimplemented device write (size 4, offset 0x185128, value 0x030f1ff1) aspeed.io: unimplemented device write (size 4, offset 0x18512c, value 0x03fffff1) [*] https://github.com/AspeedTech-BMC/zephyr/releases/tag/v00.01.07 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 | 12 +++++++----- include/hw/watchdog/wdt_aspeed.h | 2 +- 5 files changed, 11 insertions(+), 9 deletions(-)