The previous implementation set the "aspeed_intc_ops" struct, containing read
and write callbacks, to be used when I/O is performed on the INTC region.
Both "aspeed_intc_read" and "aspeed_intc_write" callback functions were used
for INTC0 (CPU DIE).

To support the INTC1 (I/O DIE) model, introduces a new "reg_ops" class
attribute. This allows setting different memory region operations to support
different INTC models.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/intc/aspeed_intc.c         | 5 ++++-
 include/hw/intc/aspeed_intc.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
index df4da759e1..628f69ea88 100644
--- a/hw/intc/aspeed_intc.c
+++ b/hw/intc/aspeed_intc.c
@@ -302,7 +302,7 @@ static void aspeed_intc_realize(DeviceState *dev, Error **errp)
     AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
     int i;
 
-    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_intc_ops, s,
+    memory_region_init_io(&s->iomem, OBJECT(s), aic->reg_ops, s,
                           TYPE_ASPEED_INTC ".regs", ASPEED_INTC_NR_REGS << 2);
 
     sysbus_init_mmio(sbd, &s->iomem);
@@ -319,11 +319,14 @@ static void aspeed_intc_realize(DeviceState *dev, Error **errp)
 static void aspeed_intc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedINTCClass *aic = ASPEED_INTC_CLASS(klass);
 
     dc->desc = "ASPEED INTC Controller";
     dc->realize = aspeed_intc_realize;
     device_class_set_legacy_reset(dc, aspeed_intc_reset);
     dc->vmsd = NULL;
+
+    aic->reg_ops = &aspeed_intc_ops;
 }
 
 static const TypeInfo aspeed_intc_info = {
diff --git a/include/hw/intc/aspeed_intc.h b/include/hw/intc/aspeed_intc.h
index 10718ed4a1..9a73661403 100644
--- a/include/hw/intc/aspeed_intc.h
+++ b/include/hw/intc/aspeed_intc.h
@@ -39,6 +39,7 @@ struct AspeedINTCClass {
 
     uint32_t num_lines;
     uint32_t num_ints;
+    const MemoryRegionOps *reg_ops;
 };
 
 #endif /* ASPEED_INTC_H */
-- 
2.34.1

On Tue, 2025-01-21 at 15:04 +0800, Jamin Lin wrote:
> The previous implementation set the "aspeed_intc_ops" struct,
> containing read
> and write callbacks, to be used when I/O is performed on the INTC
> region.
> Both "aspeed_intc_read" and "aspeed_intc_write" callback functions
> were used
> for INTC0 (CPU DIE).
> 
> To support the INTC1 (I/O DIE) model, introduces a new "reg_ops"
> class
> attribute. This allows setting different memory region operations to
> support
> different INTC models.

Is there a reason not to make it a different type altogether? I'm not
sure I like the idea of playing tricks with the memory ops. The timer
model does something similar, but I was toying with the idea of
removing that part of its implementation...

Andrew

Hi Andrew, 

> From: Andrew Jeffery <andrew@codeconstruct.com.au>
> Sent: Thursday, January 30, 2025 11:32 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 02/18] hw/intc/aspeed: Support different memory
> region ops
> 
> On Tue, 2025-01-21 at 15:04 +0800, Jamin Lin wrote:
> > The previous implementation set the "aspeed_intc_ops" struct,
> > containing read and write callbacks, to be used when I/O is performed
> > on the INTC region.
> > Both "aspeed_intc_read" and "aspeed_intc_write" callback functions
> > were used for INTC0 (CPU DIE).
> >
> > To support the INTC1 (I/O DIE) model, introduces a new "reg_ops"
> > class
> > attribute. This allows setting different memory region operations to
> > support different INTC models.
> 
> Is there a reason not to make it a different type altogether? I'm not sure I like
> the idea of playing tricks with the memory ops. The timer model does
> something similar, but I was toying with the idea of removing that part of its
> implementation...
> 
Please see my comments in Patch 1.

The reason for the changes is that INTC0 and INTC1 have different address space and register definitions.
So, I created a separate callback functions for INTC0 and INTC1 register read/write operations.
INTC0:
INTC0_10
INTC0_14

INTC1:
INTC1_10
INTC1_14

Thanks-Jamin

> Andrew