| Message ID | 20221230113504.37032-2-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/aspeed_ast10x0: Map more peripherals & few more fixes | expand |
> > 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); How does the specification say here (I didn't find it)? I read this is for a case where multiple WDTs are implemented. And offset means the distance io registers of WDT[1] are located from WDT[0]. Or does the spec explicitly says the io registers of WDT[1] is located right after WDT[0] without any gaps in this case, iosize is better)? > } > > /* 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; > 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 >
On 12/31/22 23:43, Dong, Eddie 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); > > > How does the specification say here (I didn't find it)? > I read this is for a case where multiple WDTs are implemented. > And offset means the distance io registers of WDT[1] are located from WDT[0]. The specs say 'offset' > Or does the spec explicitly says the io registers of WDT[1] is located right after > WDT[0] without any gaps in this case, iosize is better)? The IO regions are contiguous but size/width is larger than the register set. That said, the model takes some shortcuts and Phil's proposal is a sign that we need to clarify and distinguish the number of regs, the region width and the offset where it is mapped in the overall MMIO region of the SoC. > >> } >> >> /* 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); May be, as a clarification, introduce : hwaddr wdt_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; >> 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 >> >
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 > >
Philippe, On 1/2/23 13:30, Cédric Le Goater wrote: > On 12/31/22 23:43, Dong, Eddie 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); >> >> >> How does the specification say here (I didn't find it)? >> I read this is for a case where multiple WDTs are implemented. >> And offset means the distance io registers of WDT[1] are located from WDT[0]. > > The specs say 'offset' > >> Or does the spec explicitly says the io registers of WDT[1] is located right after >> WDT[0] without any gaps in this case, iosize is better)? > > The IO regions are contiguous but size/width is larger than the register set. > > That said, the model takes some shortcuts and Phil's proposal is a sign that > we need to clarify and distinguish the number of regs, the region width and > the offset where it is mapped in the overall MMIO region of the SoC. > > >> >>> } >>> >>> /* 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); > > May be, as a clarification, introduce : > > hwaddr wdt_offset = sc->memmap[ASPEED_DEV_WDT] + i * awc->iosize That would be nice to add since you are about to respin. Thanks, C. > > > >>> } >>> >>> /* 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; >>> 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 >>> >> >
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; 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, 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(-)