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):