Message ID | 20241216075353.1308043-2-jamin_lin@aspeedtech.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Support timer for AST2700 | expand |
Hi Jamin, On 16/12/24 08:53, Jamin Lin via wrote: > It set "aspeed_timer_ops" struct which containing read and write callbacks > to be used when I/O is performed on the TIMER region. > > Besides, in the previous design of ASPEED SOCs, the timer registers address > space are contiguous. > > ex: TMC00-TMC0C are used for TIMER0. > ex: TMC10-TMC1C are used for TIMER1. > ex: TMC80-TMC8C are used for TIMER7. > > The TMC30 is a control register and TMC34 is an interrupt status register for > TIMER0-TIMER7. > > However, the register set have a significant change in AST2700. The TMC00-TMC3C > are used for TIMER0 and TMC40-TMC7C are used for TIMER1. In additional, > TMC20-TMC3C and TMC60-TMC7C are reserved registers for TIMER0 and TIMER1, > respectively. > > Besides, each TIMER has their own control and interrupt status register. > In other words, users are able to set control and interrupt status for TIMER0 > in one register. Both aspeed_timer_read and aspeed_timer_write callback > functions are not compatible AST2700. > > Introduce a new "const MemoryRegionOps *" attribute in AspeedTIMERClass and use > it in aspeed_timer_realize function. > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > --- > hw/timer/aspeed_timer.c | 7 ++++++- > include/hw/timer/aspeed_timer.h | 1 + > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > index 149f7cc5a6..970bf1d79d 100644 > --- a/hw/timer/aspeed_timer.c > +++ b/hw/timer/aspeed_timer.c > @@ -606,6 +606,7 @@ static void aspeed_timer_realize(DeviceState *dev, Error **errp) > int i; > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > AspeedTimerCtrlState *s = ASPEED_TIMER(dev); > + AspeedTimerClass *atc = ASPEED_TIMER_GET_CLASS(s); > > assert(s->scu); > > @@ -613,7 +614,7 @@ static void aspeed_timer_realize(DeviceState *dev, Error **errp) > aspeed_init_one_timer(s, i); > sysbus_init_irq(sbd, &s->timers[i].irq); > } > - memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, s, > + memory_region_init_io(&s->iomem, OBJECT(s), atc->reg_ops, s, > TYPE_ASPEED_TIMER, 0x1000); > sysbus_init_mmio(sbd, &s->iomem); > } > @@ -708,6 +709,7 @@ static void aspeed_2400_timer_class_init(ObjectClass *klass, void *data) > dc->desc = "ASPEED 2400 Timer"; > awc->read = aspeed_2400_timer_read; > awc->write = aspeed_2400_timer_write; > + awc->reg_ops = &aspeed_timer_ops; Simpler (and safer) to initialize a common field once, in the parent class, timer_class_init(). Otherwise, Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > } > > static const TypeInfo aspeed_2400_timer_info = { > @@ -724,6 +726,7 @@ static void aspeed_2500_timer_class_init(ObjectClass *klass, void *data) > dc->desc = "ASPEED 2500 Timer"; > awc->read = aspeed_2500_timer_read; > awc->write = aspeed_2500_timer_write; > + awc->reg_ops = &aspeed_timer_ops; > }
Hi Philippe, > Subject: Re: [PATCH v1 1/3] hw/timer/aspeed: Support different memory region > ops > > Hi Jamin, > > On 16/12/24 08:53, Jamin Lin via wrote: > > It set "aspeed_timer_ops" struct which containing read and write > > callbacks to be used when I/O is performed on the TIMER region. > > > > Besides, in the previous design of ASPEED SOCs, the timer registers > > address space are contiguous. > > > > ex: TMC00-TMC0C are used for TIMER0. > > ex: TMC10-TMC1C are used for TIMER1. > > ex: TMC80-TMC8C are used for TIMER7. > > > > The TMC30 is a control register and TMC34 is an interrupt status > > register for TIMER0-TIMER7. > > > > However, the register set have a significant change in AST2700. The > > TMC00-TMC3C are used for TIMER0 and TMC40-TMC7C are used for > TIMER1. > > In additional, TMC20-TMC3C and TMC60-TMC7C are reserved registers for > > TIMER0 and TIMER1, respectively. > > > > Besides, each TIMER has their own control and interrupt status register. > > In other words, users are able to set control and interrupt status for > > TIMER0 in one register. Both aspeed_timer_read and aspeed_timer_write > > callback functions are not compatible AST2700. > > > > Introduce a new "const MemoryRegionOps *" attribute in > > AspeedTIMERClass and use it in aspeed_timer_realize function. > > > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > > --- > > hw/timer/aspeed_timer.c | 7 ++++++- > > include/hw/timer/aspeed_timer.h | 1 + > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index > > 149f7cc5a6..970bf1d79d 100644 > > --- a/hw/timer/aspeed_timer.c > > +++ b/hw/timer/aspeed_timer.c > > @@ -606,6 +606,7 @@ static void aspeed_timer_realize(DeviceState *dev, > Error **errp) > > int i; > > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > > AspeedTimerCtrlState *s = ASPEED_TIMER(dev); > > + AspeedTimerClass *atc = ASPEED_TIMER_GET_CLASS(s); > > > > assert(s->scu); > > > > @@ -613,7 +614,7 @@ static void aspeed_timer_realize(DeviceState *dev, > Error **errp) > > aspeed_init_one_timer(s, i); > > sysbus_init_irq(sbd, &s->timers[i].irq); > > } > > - memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, > s, > > + memory_region_init_io(&s->iomem, OBJECT(s), atc->reg_ops, s, > > TYPE_ASPEED_TIMER, 0x1000); > > sysbus_init_mmio(sbd, &s->iomem); > > } > > @@ -708,6 +709,7 @@ static void > aspeed_2400_timer_class_init(ObjectClass *klass, void *data) > > dc->desc = "ASPEED 2400 Timer"; > > awc->read = aspeed_2400_timer_read; > > awc->write = aspeed_2400_timer_write; > > + awc->reg_ops = &aspeed_timer_ops; > > Simpler (and safer) to initialize a common field once, in the parent class, > timer_class_init(). Otherwise, > Thanks for review and suggestion. Will fix it. Jamin > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > } > > > > static const TypeInfo aspeed_2400_timer_info = { @@ -724,6 +726,7 @@ > > static void aspeed_2500_timer_class_init(ObjectClass *klass, void *data) > > dc->desc = "ASPEED 2500 Timer"; > > awc->read = aspeed_2500_timer_read; > > awc->write = aspeed_2500_timer_write; > > + awc->reg_ops = &aspeed_timer_ops; > > }
On Mon, 2024-12-16 at 15:53 +0800, Jamin Lin wrote: > It set "aspeed_timer_ops" struct which containing read and write > callbacks > to be used when I/O is performed on the TIMER region. > > Besides, in the previous design of ASPEED SOCs, the timer registers > address > space are contiguous. > > ex: TMC00-TMC0C are used for TIMER0. > ex: TMC10-TMC1C are used for TIMER1. > ex: TMC80-TMC8C are used for TIMER7. > > The TMC30 is a control register and TMC34 is an interrupt status > register for > TIMER0-TIMER7. > > However, the register set have a significant change in AST2700. The > TMC00-TMC3C > are used for TIMER0 and TMC40-TMC7C are used for TIMER1. In > additional, > TMC20-TMC3C and TMC60-TMC7C are reserved registers for TIMER0 and > TIMER1, > respectively. > > Besides, each TIMER has their own control and interrupt status > register. > In other words, users are able to set control and interrupt status > for TIMER0 > in one register. Both aspeed_timer_read and aspeed_timer_write > callback > functions are not compatible AST2700. > > Introduce a new "const MemoryRegionOps *" attribute in > AspeedTIMERClass and use > it in aspeed_timer_realize function. > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > --- > hw/timer/aspeed_timer.c | 7 ++++++- > include/hw/timer/aspeed_timer.h | 1 + > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > index 149f7cc5a6..970bf1d79d 100644 > --- a/hw/timer/aspeed_timer.c > +++ b/hw/timer/aspeed_timer.c > @@ -606,6 +606,7 @@ static void aspeed_timer_realize(DeviceState > *dev, Error **errp) > int i; > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > AspeedTimerCtrlState *s = ASPEED_TIMER(dev); > + AspeedTimerClass *atc = ASPEED_TIMER_GET_CLASS(s); > > assert(s->scu); > > @@ -613,7 +614,7 @@ static void aspeed_timer_realize(DeviceState > *dev, Error **errp) > aspeed_init_one_timer(s, i); > sysbus_init_irq(sbd, &s->timers[i].irq); > } > - memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, > s, > + memory_region_init_io(&s->iomem, OBJECT(s), atc->reg_ops, s, > TYPE_ASPEED_TIMER, 0x1000); > sysbus_init_mmio(sbd, &s->iomem); > } > @@ -708,6 +709,7 @@ static void > aspeed_2400_timer_class_init(ObjectClass *klass, void *data) > dc->desc = "ASPEED 2400 Timer"; > awc->read = aspeed_2400_timer_read; > awc->write = aspeed_2400_timer_write; > + awc->reg_ops = &aspeed_timer_ops; > } > > static const TypeInfo aspeed_2400_timer_info = { > @@ -724,6 +726,7 @@ static void > aspeed_2500_timer_class_init(ObjectClass *klass, void *data) > dc->desc = "ASPEED 2500 Timer"; > awc->read = aspeed_2500_timer_read; > awc->write = aspeed_2500_timer_write; > + awc->reg_ops = &aspeed_timer_ops; > } > > static const TypeInfo aspeed_2500_timer_info = { > @@ -740,6 +743,7 @@ static void > aspeed_2600_timer_class_init(ObjectClass *klass, void *data) > dc->desc = "ASPEED 2600 Timer"; > awc->read = aspeed_2600_timer_read; > awc->write = aspeed_2600_timer_write; > + awc->reg_ops = &aspeed_timer_ops; > } > > static const TypeInfo aspeed_2600_timer_info = { > @@ -756,6 +760,7 @@ static void > aspeed_1030_timer_class_init(ObjectClass *klass, void *data) > dc->desc = "ASPEED 1030 Timer"; > awc->read = aspeed_2600_timer_read; > awc->write = aspeed_2600_timer_write; > + awc->reg_ops = &aspeed_timer_ops; > } > > static const TypeInfo aspeed_1030_timer_info = { > diff --git a/include/hw/timer/aspeed_timer.h > b/include/hw/timer/aspeed_timer.h > index 07dc6b6f2c..8d0b15f055 100644 > --- a/include/hw/timer/aspeed_timer.h > +++ b/include/hw/timer/aspeed_timer.h > @@ -73,6 +73,7 @@ struct AspeedTimerClass { > > uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset); > void (*write)(AspeedTimerCtrlState *s, hwaddr offset, uint64_t > value); > + const MemoryRegionOps *reg_ops; So given the layout changes for the AST2700, perhaps we can improve the way we've organised the call delegation? Currently the callbacks in `aspeed_timer_ops` are generic (aspeed_timer_read(), aspeed_timer_write()), and then we specialise some bits in the default label of the switch statement by delegating to the SoC-specific callbacks. Perhaps we should instead call through the SoC-specific callbacks first, and then have those call the generic op implementation for accesses to registers have common behaviours across the AST2[456]00 SoCs. With that perspective, the change in layout for the AST2700 is effectively a specialisation for all the registers. Later, if there's some tinkering with the timer registers for a hypothetical AST2800, we can follow the same strategy by extracting out the common behaviours for the AST2700 and AST2800, and invoke them through the default label. As a quick PoC to demonstrate my line of thinking (not compiled, not tested, only converts AST2400): diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index 4868651ad489..c7486af4ced2 100644 --- a/hw/timer/aspeed_timer.c +++ b/hw/timer/aspeed_timer.c @@ -239,9 +239,8 @@ static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg) return value; } -static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size) +static uint64_t aspeed_timer_read_generic(AspeedTimerCtrlState *s, hwaddr offset) { - AspeedTimerCtrlState *s = opaque; const int reg = (offset & 0xf) / 4; uint64_t value; @@ -256,13 +255,20 @@ static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size) value = aspeed_timer_get_value(&s->timers[(offset >> 4) - 1], reg); break; default: - value = ASPEED_TIMER_GET_CLASS(s)->read(s, offset); + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n", + __func__, offset); + value = 0; break; } - trace_aspeed_timer_read(offset, size, value); return value; } +static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned size) +{ + AspeedTimerCtrlState *s = opaque; + return ASPEED_TIMER_GET_CLASS(s)->read(s, offset, size); +} + static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg, uint32_t value) { @@ -431,12 +437,11 @@ static void aspeed_timer_set_ctrl2(AspeedTimerCtrlState *s, uint32_t value) trace_aspeed_timer_set_ctrl2(value); } -static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value, - unsigned size) +static void aspeed_timer_write_generic(AspeedTimerCtrlState *s, hwaddr offset, + uint64_t value) { const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF); const int reg = (offset & 0xf) / 4; - AspeedTimerCtrlState *s = opaque; switch (offset) { /* Control Registers */ @@ -451,11 +456,20 @@ static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value, aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS) - 1, reg, tv); break; default: - ASPEED_TIMER_GET_CLASS(s)->write(s, offset, value); + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n", + __func__, offset); + value = 0; break; } } +static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value, + unsigned size) +{ + AspeedTimerCtrlState *s = opaque; + ASPEED_TIMER_GET_CLASS(s)->write(s, offset, value); +} + static const MemoryRegionOps aspeed_timer_ops = { .read = aspeed_timer_read, .write = aspeed_timer_write, @@ -465,7 +479,7 @@ static const MemoryRegionOps aspeed_timer_ops = { .valid.unaligned = false, }; -static uint64_t aspeed_2400_timer_read(AspeedTimerCtrlState *s, hwaddr offset) +static uint64_t aspeed_2400_timer_read(AspeedTimerCtrlState *s, hwaddr offset, unsigned size) { uint64_t value; @@ -475,17 +489,18 @@ static uint64_t aspeed_2400_timer_read(AspeedTimerCtrlState *s, hwaddr offset) break; case 0x38: case 0x3C: - default: qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n", __func__, offset); value = 0; break; + default: + return aspeed_timer_read_generic(s, offset); } + trace_aspeed_timer_read(offset, size, value); return value; } -static void aspeed_2400_timer_write(AspeedTimerCtrlState *s, hwaddr offset, - uint64_t value) +static void aspeed_2400_timer_write(AspeedTimerCtrlState *s, hwaddr offset, uint64_t value) { const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF); @@ -495,10 +510,12 @@ static void aspeed_2400_timer_write(AspeedTimerCtrlState *s, hwaddr offset, break; case 0x38: case 0x3C: - default: qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n", __func__, offset); break; + default: + aspeed_timer_write_generic(s, offset, value); + break; } } diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h index 07dc6b6f2cbd..540b23656815 100644 --- a/include/hw/timer/aspeed_timer.h +++ b/include/hw/timer/aspeed_timer.h @@ -71,7 +71,7 @@ struct AspeedTimerCtrlState { struct AspeedTimerClass { SysBusDeviceClass parent_class; - uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset); + uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset, unsigned size); void (*write)(AspeedTimerCtrlState *s, hwaddr offset, uint64_t value); };
Hi Andrew, > From: Andrew Jeffery <andrew@codeconstruct.com.au> > Sent: Thursday, January 9, 2025 9:59 AM > To: Jamin Lin <jamin_lin@aspeedtech.com>; Cédric Le Goater <clg@kaod.org>; > Peter Maydell <peter.maydell@linaro.org>; Steven Lee > <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Joel Stanley > <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open > list:All patches CC here <qemu-devel@nongnu.org> > Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang > <yunlin.tang@aspeedtech.com> > Subject: Re: [PATCH v1 1/3] hw/timer/aspeed: Support different memory region > ops > > On Mon, 2024-12-16 at 15:53 +0800, Jamin Lin wrote: > > It set "aspeed_timer_ops" struct which containing read and write > > callbacks to be used when I/O is performed on the TIMER region. > > > > Besides, in the previous design of ASPEED SOCs, the timer registers > > address space are contiguous. > > > > ex: TMC00-TMC0C are used for TIMER0. > > ex: TMC10-TMC1C are used for TIMER1. > > ex: TMC80-TMC8C are used for TIMER7. > > > > The TMC30 is a control register and TMC34 is an interrupt status > > register for TIMER0-TIMER7. > > > > However, the register set have a significant change in AST2700. The > > TMC00-TMC3C are used for TIMER0 and TMC40-TMC7C are used for > TIMER1. > > In additional, TMC20-TMC3C and TMC60-TMC7C are reserved registers for > > TIMER0 and TIMER1, respectively. > > > > Besides, each TIMER has their own control and interrupt status > > register. > > In other words, users are able to set control and interrupt status for > > TIMER0 in one register. Both aspeed_timer_read and aspeed_timer_write > > callback functions are not compatible AST2700. > > > > Introduce a new "const MemoryRegionOps *" attribute in > > AspeedTIMERClass and use it in aspeed_timer_realize function. > > > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > > --- > > hw/timer/aspeed_timer.c | 7 ++++++- > > include/hw/timer/aspeed_timer.h | 1 + > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index > > 149f7cc5a6..970bf1d79d 100644 > > --- a/hw/timer/aspeed_timer.c > > +++ b/hw/timer/aspeed_timer.c > > @@ -606,6 +606,7 @@ static void aspeed_timer_realize(DeviceState *dev, > > Error **errp) > > int i; > > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > > AspeedTimerCtrlState *s = ASPEED_TIMER(dev); > > + AspeedTimerClass *atc = ASPEED_TIMER_GET_CLASS(s); > > > > assert(s->scu); > > > > @@ -613,7 +614,7 @@ static void aspeed_timer_realize(DeviceState *dev, > > Error **errp) > > aspeed_init_one_timer(s, i); > > sysbus_init_irq(sbd, &s->timers[i].irq); > > } > > - memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, > s, > > + memory_region_init_io(&s->iomem, OBJECT(s), atc->reg_ops, s, > > TYPE_ASPEED_TIMER, 0x1000); > > > > sysbus_init_mmio(sbd, &s->iomem); > > } > > @@ -708,6 +709,7 @@ static void > > aspeed_2400_timer_class_init(ObjectClass *klass, void *data) > > dc->desc = "ASPEED 2400 Timer"; > > awc->read = aspeed_2400_timer_read; > > awc->write = aspeed_2400_timer_write; > > + awc->reg_ops = &aspeed_timer_ops; > > } > > > > static const TypeInfo aspeed_2400_timer_info = { @@ -724,6 +726,7 @@ > > static void aspeed_2500_timer_class_init(ObjectClass *klass, void > > *data) > > dc->desc = "ASPEED 2500 Timer"; > > awc->read = aspeed_2500_timer_read; > > awc->write = aspeed_2500_timer_write; > > + awc->reg_ops = &aspeed_timer_ops; > > } > > > > static const TypeInfo aspeed_2500_timer_info = { @@ -740,6 +743,7 @@ > > static void aspeed_2600_timer_class_init(ObjectClass *klass, void > > *data) > > dc->desc = "ASPEED 2600 Timer"; > > awc->read = aspeed_2600_timer_read; > > awc->write = aspeed_2600_timer_write; > > + awc->reg_ops = &aspeed_timer_ops; > > } > > > > static const TypeInfo aspeed_2600_timer_info = { @@ -756,6 +760,7 @@ > > static void aspeed_1030_timer_class_init(ObjectClass *klass, void > > *data) > > dc->desc = "ASPEED 1030 Timer"; > > awc->read = aspeed_2600_timer_read; > > awc->write = aspeed_2600_timer_write; > > + awc->reg_ops = &aspeed_timer_ops; > > } > > > > static const TypeInfo aspeed_1030_timer_info = { diff --git > > a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h > > index 07dc6b6f2c..8d0b15f055 100644 > > --- a/include/hw/timer/aspeed_timer.h > > +++ b/include/hw/timer/aspeed_timer.h > > @@ -73,6 +73,7 @@ struct AspeedTimerClass { > > > > uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset); > > void (*write)(AspeedTimerCtrlState *s, hwaddr offset, uint64_t > > value); > > + const MemoryRegionOps *reg_ops; > > So given the layout changes for the AST2700, perhaps we can improve the way > we've organised the call delegation? > > Currently the callbacks in `aspeed_timer_ops` are generic > (aspeed_timer_read(), aspeed_timer_write()), and then we specialise some > bits in the default label of the switch statement by delegating to the > SoC-specific callbacks. > > Perhaps we should instead call through the SoC-specific callbacks first, and > then have those call the generic op implementation for accesses to registers > have common behaviours across the AST2[456]00 SoCs. > > With that perspective, the change in layout for the AST2700 is effectively a > specialisation for all the registers. Later, if there's some tinkering with the > timer registers for a hypothetical AST2800, we can follow the same strategy by > extracting out the common behaviours for the AST2700 and AST2800, and > invoke them through the default label. > > As a quick PoC to demonstrate my line of thinking (not compiled, not tested, > only converts AST2400): > Thank you for your review and suggestion. Currently, I am working on QEMU to support the "AST2700 A1" boot(I should refactor INTC model). Once I have completed that task, I will revise the timer model with your suggestion. I will update you later. Thanks again for your input. Jamin > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index > 4868651ad489..c7486af4ced2 100644 > --- a/hw/timer/aspeed_timer.c > +++ b/hw/timer/aspeed_timer.c > @@ -239,9 +239,8 @@ static uint64_t aspeed_timer_get_value(AspeedTimer > *t, int reg) > return value; > } > > -static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned > size) > +static uint64_t aspeed_timer_read_generic(AspeedTimerCtrlState *s, > +hwaddr offset) > { > - AspeedTimerCtrlState *s = opaque; > const int reg = (offset & 0xf) / 4; > uint64_t value; > > @@ -256,13 +255,20 @@ static uint64_t aspeed_timer_read(void *opaque, > hwaddr offset, unsigned size) > value = aspeed_timer_get_value(&s->timers[(offset >> 4) - 1], reg); > break; > default: > - value = ASPEED_TIMER_GET_CLASS(s)->read(s, offset); > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" > HWADDR_PRIx "\n", > + __func__, offset); > + value = 0; > break; > } > - trace_aspeed_timer_read(offset, size, value); > return value; > } > > +static uint64_t aspeed_timer_read(void *opaque, hwaddr offset, unsigned > +size) { > + AspeedTimerCtrlState *s = opaque; > + return ASPEED_TIMER_GET_CLASS(s)->read(s, offset, size); } > + > static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int > reg, > uint32_t value) { @@ -431,12 > +437,11 @@ static void aspeed_timer_set_ctrl2(AspeedTimerCtrlState *s, > uint32_t value) > trace_aspeed_timer_set_ctrl2(value); > } > > -static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value, > - unsigned size) > +static void aspeed_timer_write_generic(AspeedTimerCtrlState *s, hwaddr > offset, > + uint64_t value) > { > const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF); > const int reg = (offset & 0xf) / 4; > - AspeedTimerCtrlState *s = opaque; > > switch (offset) { > /* Control Registers */ > @@ -451,11 +456,20 @@ static void aspeed_timer_write(void *opaque, > hwaddr offset, uint64_t value, > aspeed_timer_set_value(s, (offset >> TIMER_NR_REGS) - 1, reg, tv); > break; > default: > - ASPEED_TIMER_GET_CLASS(s)->write(s, offset, value); > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" > HWADDR_PRIx "\n", > + __func__, offset); > + value = 0; > break; > } > } > > +static void aspeed_timer_write(void *opaque, hwaddr offset, uint64_t value, > + unsigned size) { > + AspeedTimerCtrlState *s = opaque; > + ASPEED_TIMER_GET_CLASS(s)->write(s, offset, value); } > + > static const MemoryRegionOps aspeed_timer_ops = { > .read = aspeed_timer_read, > .write = aspeed_timer_write, > @@ -465,7 +479,7 @@ static const MemoryRegionOps aspeed_timer_ops = { > .valid.unaligned = false, > }; > > -static uint64_t aspeed_2400_timer_read(AspeedTimerCtrlState *s, hwaddr > offset) > +static uint64_t aspeed_2400_timer_read(AspeedTimerCtrlState *s, hwaddr > +offset, unsigned size) > { > uint64_t value; > > @@ -475,17 +489,18 @@ static uint64_t > aspeed_2400_timer_read(AspeedTimerCtrlState *s, hwaddr offset) > break; > case 0x38: > case 0x3C: > - default: > qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" > HWADDR_PRIx "\n", > __func__, offset); > value = 0; > break; > + default: > + return aspeed_timer_read_generic(s, offset); > } > + trace_aspeed_timer_read(offset, size, value); > return value; > } > > -static void aspeed_2400_timer_write(AspeedTimerCtrlState *s, hwaddr > offset, > - uint64_t value) > +static void aspeed_2400_timer_write(AspeedTimerCtrlState *s, hwaddr > +offset, uint64_t value) > { > const uint32_t tv = (uint32_t)(value & 0xFFFFFFFF); > > @@ -495,10 +510,12 @@ static void > aspeed_2400_timer_write(AspeedTimerCtrlState *s, hwaddr offset, > break; > case 0x38: > case 0x3C: > - default: > qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" > HWADDR_PRIx "\n", > __func__, offset); > break; > + default: > + aspeed_timer_write_generic(s, offset, value); > + break; > } > } > > diff --git a/include/hw/timer/aspeed_timer.h > b/include/hw/timer/aspeed_timer.h index 07dc6b6f2cbd..540b23656815 > 100644 > --- a/include/hw/timer/aspeed_timer.h > +++ b/include/hw/timer/aspeed_timer.h > @@ -71,7 +71,7 @@ struct AspeedTimerCtrlState { struct AspeedTimerClass > { > SysBusDeviceClass parent_class; > > - uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset); > + uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset, unsigned > + size); > void (*write)(AspeedTimerCtrlState *s, hwaddr offset, uint64_t > value); }; >
On 1/9/25 03:26, Jamin Lin wrote: > Hi Andrew, > >> From: Andrew Jeffery <andrew@codeconstruct.com.au> >> Sent: Thursday, January 9, 2025 9:59 AM >> To: Jamin Lin <jamin_lin@aspeedtech.com>; Cédric Le Goater <clg@kaod.org>; >> Peter Maydell <peter.maydell@linaro.org>; Steven Lee >> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Joel Stanley >> <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open >> list:All patches CC here <qemu-devel@nongnu.org> >> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang >> <yunlin.tang@aspeedtech.com> >> Subject: Re: [PATCH v1 1/3] hw/timer/aspeed: Support different memory region >> ops >> >> On Mon, 2024-12-16 at 15:53 +0800, Jamin Lin wrote: >>> It set "aspeed_timer_ops" struct which containing read and write >>> callbacks to be used when I/O is performed on the TIMER region. >>> >>> Besides, in the previous design of ASPEED SOCs, the timer registers >>> address space are contiguous. >>> >>> ex: TMC00-TMC0C are used for TIMER0. >>> ex: TMC10-TMC1C are used for TIMER1. >>> ex: TMC80-TMC8C are used for TIMER7. >>> >>> The TMC30 is a control register and TMC34 is an interrupt status >>> register for TIMER0-TIMER7. >>> >>> However, the register set have a significant change in AST2700. The >>> TMC00-TMC3C are used for TIMER0 and TMC40-TMC7C are used for >> TIMER1. >>> In additional, TMC20-TMC3C and TMC60-TMC7C are reserved registers for >>> TIMER0 and TIMER1, respectively. >>> >>> Besides, each TIMER has their own control and interrupt status >>> register. >>> In other words, users are able to set control and interrupt status for >>> TIMER0 in one register. Both aspeed_timer_read and aspeed_timer_write >>> callback functions are not compatible AST2700. >>> >>> Introduce a new "const MemoryRegionOps *" attribute in >>> AspeedTIMERClass and use it in aspeed_timer_realize function. >>> >>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> >>> --- >>> hw/timer/aspeed_timer.c | 7 ++++++- >>> include/hw/timer/aspeed_timer.h | 1 + >>> 2 files changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index >>> 149f7cc5a6..970bf1d79d 100644 >>> --- a/hw/timer/aspeed_timer.c >>> +++ b/hw/timer/aspeed_timer.c >>> @@ -606,6 +606,7 @@ static void aspeed_timer_realize(DeviceState *dev, >>> Error **errp) >>> int i; >>> SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>> AspeedTimerCtrlState *s = ASPEED_TIMER(dev); >>> + AspeedTimerClass *atc = ASPEED_TIMER_GET_CLASS(s); >>> >>> assert(s->scu); >>> >>> @@ -613,7 +614,7 @@ static void aspeed_timer_realize(DeviceState *dev, >>> Error **errp) >>> aspeed_init_one_timer(s, i); >>> sysbus_init_irq(sbd, &s->timers[i].irq); >>> } >>> - memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, >> s, >>> + memory_region_init_io(&s->iomem, OBJECT(s), atc->reg_ops, s, >>> TYPE_ASPEED_TIMER, 0x1000); >> >> >>> sysbus_init_mmio(sbd, &s->iomem); >>> } >>> @@ -708,6 +709,7 @@ static void >>> aspeed_2400_timer_class_init(ObjectClass *klass, void *data) >>> dc->desc = "ASPEED 2400 Timer"; >>> awc->read = aspeed_2400_timer_read; >>> awc->write = aspeed_2400_timer_write; >>> + awc->reg_ops = &aspeed_timer_ops; >>> } >>> >>> static const TypeInfo aspeed_2400_timer_info = { @@ -724,6 +726,7 @@ >>> static void aspeed_2500_timer_class_init(ObjectClass *klass, void >>> *data) >>> dc->desc = "ASPEED 2500 Timer"; >>> awc->read = aspeed_2500_timer_read; >>> awc->write = aspeed_2500_timer_write; >>> + awc->reg_ops = &aspeed_timer_ops; >>> } >>> >>> static const TypeInfo aspeed_2500_timer_info = { @@ -740,6 +743,7 @@ >>> static void aspeed_2600_timer_class_init(ObjectClass *klass, void >>> *data) >>> dc->desc = "ASPEED 2600 Timer"; >>> awc->read = aspeed_2600_timer_read; >>> awc->write = aspeed_2600_timer_write; >>> + awc->reg_ops = &aspeed_timer_ops; >>> } >>> >>> static const TypeInfo aspeed_2600_timer_info = { @@ -756,6 +760,7 @@ >>> static void aspeed_1030_timer_class_init(ObjectClass *klass, void >>> *data) >>> dc->desc = "ASPEED 1030 Timer"; >>> awc->read = aspeed_2600_timer_read; >>> awc->write = aspeed_2600_timer_write; >>> + awc->reg_ops = &aspeed_timer_ops; >>> } >>> >>> static const TypeInfo aspeed_1030_timer_info = { diff --git >>> a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h >>> index 07dc6b6f2c..8d0b15f055 100644 >>> --- a/include/hw/timer/aspeed_timer.h >>> +++ b/include/hw/timer/aspeed_timer.h >>> @@ -73,6 +73,7 @@ struct AspeedTimerClass { >>> >>> uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset); >>> void (*write)(AspeedTimerCtrlState *s, hwaddr offset, uint64_t >>> value); >>> + const MemoryRegionOps *reg_ops; >> >> So given the layout changes for the AST2700, perhaps we can improve the way >> we've organised the call delegation? >> >> Currently the callbacks in `aspeed_timer_ops` are generic >> (aspeed_timer_read(), aspeed_timer_write()), and then we specialise some >> bits in the default label of the switch statement by delegating to the >> SoC-specific callbacks. >> >> Perhaps we should instead call through the SoC-specific callbacks first, and >> then have those call the generic op implementation for accesses to registers >> have common behaviours across the AST2[456]00 SoCs. >> >> With that perspective, the change in layout for the AST2700 is effectively a >> specialisation for all the registers. Later, if there's some tinkering with the >> timer registers for a hypothetical AST2800, we can follow the same strategy by >> extracting out the common behaviours for the AST2700 and AST2800, and >> invoke them through the default label. >> >> As a quick PoC to demonstrate my line of thinking (not compiled, not tested, >> only converts AST2400): >> > Thank you for your review and suggestion. > Currently, I am working on QEMU to support the "AST2700 A1" boot(I should refactor INTC model). Is that the reason why the QEMU ast2700-evb machine doesn't boot with the v09.04 SDK images ? > Once I have completed that task, I will revise the timer model with your suggestion. Please replace suffix '_generic' by '_common' when you do. Thanks, C.
Hi Cedric, > -----Original Message----- > From: Cédric Le Goater <clg@kaod.org> > Sent: Thursday, January 9, 2025 3:02 PM > To: Jamin Lin <jamin_lin@aspeedtech.com>; Andrew Jeffery > <andrew@codeconstruct.com.au>; Peter Maydell <peter.maydell@linaro.org>; > Steven Lee <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; > Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs > <qemu-arm@nongnu.org>; open list:All patches CC here > <qemu-devel@nongnu.org> > Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang > <yunlin.tang@aspeedtech.com> > Subject: Re: [PATCH v1 1/3] hw/timer/aspeed: Support different memory region > ops > > On 1/9/25 03:26, Jamin Lin wrote: > > Hi Andrew, > > > >> From: Andrew Jeffery <andrew@codeconstruct.com.au> > >> Sent: Thursday, January 9, 2025 9:59 AM > >> To: Jamin Lin <jamin_lin@aspeedtech.com>; Cédric Le Goater > >> <clg@kaod.org>; Peter Maydell <peter.maydell@linaro.org>; Steven Lee > >> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Joel > >> Stanley <joel@jms.id.au>; open list:ASPEED BMCs > >> <qemu-arm@nongnu.org>; open list:All patches CC here > >> <qemu-devel@nongnu.org> > >> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang > >> <yunlin.tang@aspeedtech.com> > >> Subject: Re: [PATCH v1 1/3] hw/timer/aspeed: Support different memory > >> region ops > >> > >> On Mon, 2024-12-16 at 15:53 +0800, Jamin Lin wrote: > >>> It set "aspeed_timer_ops" struct which containing read and write > >>> callbacks to be used when I/O is performed on the TIMER region. > >>> > >>> Besides, in the previous design of ASPEED SOCs, the timer registers > >>> address space are contiguous. > >>> > >>> ex: TMC00-TMC0C are used for TIMER0. > >>> ex: TMC10-TMC1C are used for TIMER1. > >>> ex: TMC80-TMC8C are used for TIMER7. > >>> > >>> The TMC30 is a control register and TMC34 is an interrupt status > >>> register for TIMER0-TIMER7. > >>> > >>> However, the register set have a significant change in AST2700. The > >>> TMC00-TMC3C are used for TIMER0 and TMC40-TMC7C are used for > >> TIMER1. > >>> In additional, TMC20-TMC3C and TMC60-TMC7C are reserved registers > >>> for > >>> TIMER0 and TIMER1, respectively. > >>> > >>> Besides, each TIMER has their own control and interrupt status > >>> register. > >>> In other words, users are able to set control and interrupt status > >>> for > >>> TIMER0 in one register. Both aspeed_timer_read and > >>> aspeed_timer_write callback functions are not compatible AST2700. > >>> > >>> Introduce a new "const MemoryRegionOps *" attribute in > >>> AspeedTIMERClass and use it in aspeed_timer_realize function. > >>> > >>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > >>> --- > >>> hw/timer/aspeed_timer.c | 7 ++++++- > >>> include/hw/timer/aspeed_timer.h | 1 + > >>> 2 files changed, 7 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index > >>> 149f7cc5a6..970bf1d79d 100644 > >>> --- a/hw/timer/aspeed_timer.c > >>> +++ b/hw/timer/aspeed_timer.c > >>> @@ -606,6 +606,7 @@ static void aspeed_timer_realize(DeviceState > >>> *dev, Error **errp) > >>> int i; > >>> SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > >>> AspeedTimerCtrlState *s = ASPEED_TIMER(dev); > >>> + AspeedTimerClass *atc = ASPEED_TIMER_GET_CLASS(s); > >>> > >>> assert(s->scu); > >>> > >>> @@ -613,7 +614,7 @@ static void aspeed_timer_realize(DeviceState > >>> *dev, Error **errp) > >>> aspeed_init_one_timer(s, i); > >>> sysbus_init_irq(sbd, &s->timers[i].irq); > >>> } > >>> - memory_region_init_io(&s->iomem, OBJECT(s), > &aspeed_timer_ops, > >> s, > >>> + memory_region_init_io(&s->iomem, OBJECT(s), atc->reg_ops, s, > >>> TYPE_ASPEED_TIMER, 0x1000); > >> > >> > >>> sysbus_init_mmio(sbd, &s->iomem); > >>> } > >>> @@ -708,6 +709,7 @@ static void > >>> aspeed_2400_timer_class_init(ObjectClass *klass, void *data) > >>> dc->desc = "ASPEED 2400 Timer"; > >>> awc->read = aspeed_2400_timer_read; > >>> awc->write = aspeed_2400_timer_write; > >>> + awc->reg_ops = &aspeed_timer_ops; > >>> } > >>> > >>> static const TypeInfo aspeed_2400_timer_info = { @@ -724,6 +726,7 > >>> @@ static void aspeed_2500_timer_class_init(ObjectClass *klass, void > >>> *data) > >>> dc->desc = "ASPEED 2500 Timer"; > >>> awc->read = aspeed_2500_timer_read; > >>> awc->write = aspeed_2500_timer_write; > >>> + awc->reg_ops = &aspeed_timer_ops; > >>> } > >>> > >>> static const TypeInfo aspeed_2500_timer_info = { @@ -740,6 +743,7 > >>> @@ static void aspeed_2600_timer_class_init(ObjectClass *klass, void > >>> *data) > >>> dc->desc = "ASPEED 2600 Timer"; > >>> awc->read = aspeed_2600_timer_read; > >>> awc->write = aspeed_2600_timer_write; > >>> + awc->reg_ops = &aspeed_timer_ops; > >>> } > >>> > >>> static const TypeInfo aspeed_2600_timer_info = { @@ -756,6 +760,7 > >>> @@ static void aspeed_1030_timer_class_init(ObjectClass *klass, void > >>> *data) > >>> dc->desc = "ASPEED 1030 Timer"; > >>> awc->read = aspeed_2600_timer_read; > >>> awc->write = aspeed_2600_timer_write; > >>> + awc->reg_ops = &aspeed_timer_ops; > >>> } > >>> > >>> static const TypeInfo aspeed_1030_timer_info = { diff --git > >>> a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h > >>> index 07dc6b6f2c..8d0b15f055 100644 > >>> --- a/include/hw/timer/aspeed_timer.h > >>> +++ b/include/hw/timer/aspeed_timer.h > >>> @@ -73,6 +73,7 @@ struct AspeedTimerClass { > >>> > >>> uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset); > >>> void (*write)(AspeedTimerCtrlState *s, hwaddr offset, uint64_t > >>> value); > >>> + const MemoryRegionOps *reg_ops; > >> > >> So given the layout changes for the AST2700, perhaps we can improve > >> the way we've organised the call delegation? > >> > >> Currently the callbacks in `aspeed_timer_ops` are generic > >> (aspeed_timer_read(), aspeed_timer_write()), and then we specialise > >> some bits in the default label of the switch statement by delegating > >> to the SoC-specific callbacks. > >> > >> Perhaps we should instead call through the SoC-specific callbacks > >> first, and then have those call the generic op implementation for > >> accesses to registers have common behaviours across the AST2[456]00 SoCs. > >> > >> With that perspective, the change in layout for the AST2700 is > >> effectively a specialisation for all the registers. Later, if there's > >> some tinkering with the timer registers for a hypothetical AST2800, > >> we can follow the same strategy by extracting out the common > >> behaviours for the AST2700 and AST2800, and invoke them through the > default label. > >> > >> As a quick PoC to demonstrate my line of thinking (not compiled, not > >> tested, only converts AST2400): > >> > > Thank you for your review and suggestion. > > Currently, I am working on QEMU to support the "AST2700 A1" boot(I should > refactor INTC model). > > Is that the reason why the QEMU ast2700-evb machine doesn't boot with the > v09.04 SDK images ? > Yes, "ast2700-default-obmc.tar.gz" is used for AST2700 A1. The design between the AST2700 A0 and A1 is different, especially the INTC controllers. I am refactoring the INTC model to enable the AST2700 A1 to boot into the OS. If you want to test SDK v09.04, please use "ast2700-a0-default-obmc.tar.gz". I estimate to send the v1 patch to support A1 in the end of this month before the Chinese New Year. > > Once I have completed that task, I will revise the timer model with your > suggestion. > > Please replace suffix '_generic' by '_common' when you do. Got it. Thanks for suggestion. Jamin > > > > Thanks, > > C. >
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index 149f7cc5a6..970bf1d79d 100644 --- a/hw/timer/aspeed_timer.c +++ b/hw/timer/aspeed_timer.c @@ -606,6 +606,7 @@ static void aspeed_timer_realize(DeviceState *dev, Error **errp) int i; SysBusDevice *sbd = SYS_BUS_DEVICE(dev); AspeedTimerCtrlState *s = ASPEED_TIMER(dev); + AspeedTimerClass *atc = ASPEED_TIMER_GET_CLASS(s); assert(s->scu); @@ -613,7 +614,7 @@ static void aspeed_timer_realize(DeviceState *dev, Error **errp) aspeed_init_one_timer(s, i); sysbus_init_irq(sbd, &s->timers[i].irq); } - memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_timer_ops, s, + memory_region_init_io(&s->iomem, OBJECT(s), atc->reg_ops, s, TYPE_ASPEED_TIMER, 0x1000); sysbus_init_mmio(sbd, &s->iomem); } @@ -708,6 +709,7 @@ static void aspeed_2400_timer_class_init(ObjectClass *klass, void *data) dc->desc = "ASPEED 2400 Timer"; awc->read = aspeed_2400_timer_read; awc->write = aspeed_2400_timer_write; + awc->reg_ops = &aspeed_timer_ops; } static const TypeInfo aspeed_2400_timer_info = { @@ -724,6 +726,7 @@ static void aspeed_2500_timer_class_init(ObjectClass *klass, void *data) dc->desc = "ASPEED 2500 Timer"; awc->read = aspeed_2500_timer_read; awc->write = aspeed_2500_timer_write; + awc->reg_ops = &aspeed_timer_ops; } static const TypeInfo aspeed_2500_timer_info = { @@ -740,6 +743,7 @@ static void aspeed_2600_timer_class_init(ObjectClass *klass, void *data) dc->desc = "ASPEED 2600 Timer"; awc->read = aspeed_2600_timer_read; awc->write = aspeed_2600_timer_write; + awc->reg_ops = &aspeed_timer_ops; } static const TypeInfo aspeed_2600_timer_info = { @@ -756,6 +760,7 @@ static void aspeed_1030_timer_class_init(ObjectClass *klass, void *data) dc->desc = "ASPEED 1030 Timer"; awc->read = aspeed_2600_timer_read; awc->write = aspeed_2600_timer_write; + awc->reg_ops = &aspeed_timer_ops; } static const TypeInfo aspeed_1030_timer_info = { diff --git a/include/hw/timer/aspeed_timer.h b/include/hw/timer/aspeed_timer.h index 07dc6b6f2c..8d0b15f055 100644 --- a/include/hw/timer/aspeed_timer.h +++ b/include/hw/timer/aspeed_timer.h @@ -73,6 +73,7 @@ struct AspeedTimerClass { uint64_t (*read)(AspeedTimerCtrlState *s, hwaddr offset); void (*write)(AspeedTimerCtrlState *s, hwaddr offset, uint64_t value); + const MemoryRegionOps *reg_ops; }; #endif /* ASPEED_TIMER_H */
It set "aspeed_timer_ops" struct which containing read and write callbacks to be used when I/O is performed on the TIMER region. Besides, in the previous design of ASPEED SOCs, the timer registers address space are contiguous. ex: TMC00-TMC0C are used for TIMER0. ex: TMC10-TMC1C are used for TIMER1. ex: TMC80-TMC8C are used for TIMER7. The TMC30 is a control register and TMC34 is an interrupt status register for TIMER0-TIMER7. However, the register set have a significant change in AST2700. The TMC00-TMC3C are used for TIMER0 and TMC40-TMC7C are used for TIMER1. In additional, TMC20-TMC3C and TMC60-TMC7C are reserved registers for TIMER0 and TIMER1, respectively. Besides, each TIMER has their own control and interrupt status register. In other words, users are able to set control and interrupt status for TIMER0 in one register. Both aspeed_timer_read and aspeed_timer_write callback functions are not compatible AST2700. Introduce a new "const MemoryRegionOps *" attribute in AspeedTIMERClass and use it in aspeed_timer_realize function. Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> --- hw/timer/aspeed_timer.c | 7 ++++++- include/hw/timer/aspeed_timer.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-)