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_...@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);
};
