Hi Cedric,
> Subject: Re: [PATCH v3 13/28] hw/intc/aspeed: Add Support for AST2700
> INTCIO Controller
>
> On 2/13/25 04:35, Jamin Lin wrote:
> > Introduce a new ast2700 INTCIO class to support AST2700 INTCIO.
> > Added new register definitions for INTCIO, including enable and status
> > registers for IRQs GICINT192 through GICINT197.
> > Created a dedicated IRQ array for INTCIO, supporting six input pins
> > and six output pins, aligning with the newly defined registers.
> > Implemented "aspeed_intcio_read" and "aspeed_intcio_write" to handle
> > INTCIO-specific register access.
> >
> > To GICINT196
> |
> >
> > ETH1 |-----------|
> |--------------------------|
> > -------->|0 | | INTCIO
> |
> > ETH2 | 4|
> orgates[0]------>|inpin[0]-------->outpin[0]|
> > -------->|1 5|
> orgates[1]------>|inpin[1]-------->outpin[1]|
> > ETH3 | 6|
> orgates[2]------>|inpin[2]-------->outpin[2]|
> > -------->|2 19|
> orgates[3]------>|inpin[3]-------->outpin[3]|
> > UART0 |
> 20|-->orgates[4]------>|inpin[4]-------->outpin[4]|
> > -------->|7 21|
> orgates[5]------>|inpin[5]-------->outpin[5]|
> > UART1 | 22|
> |--------------------------|
> > -------->|8 23|
> > UART2 | 24|
> > -------->|9 25|
> > UART3 | 26|
> > ---------|10 27|
> > UART5 | 28|
> > -------->|11 29|
> > UART6 | |
> > -------->|12 30|
> > UART7 | 31|
> > -------->|13 |
> > UART8 | OR[0:31] |
> > -------->|14 |
> > UART9 | |
> > -------->|15 |
> > UART10 | |
> > -------->|16 |
> > UART11 | |
> > -------->|17 |
> > UART12 | |
> > -------->|18 |
> > |-----------|
> >
> > Signed-off-by: Jamin Lin <[email protected]>
> > ---
> > hw/intc/aspeed_intc.c | 121
> ++++++++++++++++++++++++++++++++++
> > include/hw/intc/aspeed_intc.h | 1 +
> > 2 files changed, 122 insertions(+)
> >
> > diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
> > 1a9e2bf8ce..a3bf935789 100644
> > --- a/hw/intc/aspeed_intc.c
> > +++ b/hw/intc/aspeed_intc.c
> > @@ -36,6 +36,20 @@ REG32(GICINT136_STATUS, 0x1804)
> > REG32(GICINT192_201_EN, 0x1B00)
> > REG32(GICINT192_201_STATUS, 0x1B04)
> >
> > +/* INTCIO Registers */
> > +REG32(GICINT192_EN, 0x100)
> > +REG32(GICINT192_STATUS, 0x104)
> > +REG32(GICINT193_EN, 0x110)
> > +REG32(GICINT193_STATUS, 0x114)
> > +REG32(GICINT194_EN, 0x120)
> > +REG32(GICINT194_STATUS, 0x124)
> > +REG32(GICINT195_EN, 0x130)
> > +REG32(GICINT195_STATUS, 0x134)
> > +REG32(GICINT196_EN, 0x140)
> > +REG32(GICINT196_STATUS, 0x144)
> > +REG32(GICINT197_EN, 0x150)
> > +REG32(GICINT197_STATUS, 0x154)
> > +
> > static AspeedINTCIRQ aspeed_2700_intc_irqs[ASPEED_INTC_MAX_INPINS]
> = {
> > {0, 0, 10, R_GICINT192_201_EN, R_GICINT192_201_STATUS},
> > {1, 10, 1, R_GICINT128_EN, R_GICINT128_STATUS}, @@ -49,6 +63,15
> > @@ static AspeedINTCIRQ
> aspeed_2700_intc_irqs[ASPEED_INTC_MAX_INPINS] = {
> > {9, 18, 1, R_GICINT136_EN, R_GICINT136_STATUS},
> > };
> >
> > +static AspeedINTCIRQ aspeed_2700_intcio_irqs[ASPEED_INTC_MAX_INPINS]
> = {
> > + {0, 0, 1, R_GICINT192_EN, R_GICINT192_STATUS},
> > + {1, 1, 1, R_GICINT193_EN, R_GICINT193_STATUS},
> > + {2, 2, 1, R_GICINT194_EN, R_GICINT194_STATUS},
> > + {3, 3, 1, R_GICINT195_EN, R_GICINT195_STATUS},
> > + {4, 4, 1, R_GICINT196_EN, R_GICINT196_STATUS},
> > + {5, 5, 1, R_GICINT197_EN, R_GICINT197_STATUS}, };
> > +
>
> I would move the array definition just before routine
> aspeed_2700_intcio_class_init().
>
Will update it
> > static const AspeedINTCIRQ *aspeed_intc_get_irq(AspeedINTCClass *aic,
> > uint32_t addr)
> > {
> > @@ -463,6 +486,71 @@ static void aspeed_intc_write(void *opaque,
> hwaddr offset, uint64_t data,
> > return;
> > }
> >
> > +static uint64_t aspeed_intcio_read(void *opaque, hwaddr offset,
> > + unsigned int size) {
> > + AspeedINTCState *s = ASPEED_INTC(opaque);
> > + AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > + const char *name = object_get_typename(OBJECT(s));
> > + uint32_t addr = offset >> 2;
>
> s/addr/reg/ ? I find that "reg" makes more sense.
>
Will update to reg
> > + uint32_t value = 0;
> > +
> > + if (offset >= aic->reg_size) {
>
> as said in patch 1, this is dead code bc the MMIO aperture has the same size.
> You could remove the check.
>
Will remove it
>
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Out-of-bounds read at offset 0x%"
> HWADDR_PRIx "\n",
> > + __func__, offset);
> > + return 0;
> > + }
> > +
> > + value = s->regs[addr];
> > + trace_aspeed_intc_read(name, offset, size, value);
> > +
> > + return value;
> > +}
> > +
> > +static void aspeed_intcio_write(void *opaque, hwaddr offset, uint64_t data,
> > + unsigned size) {
> > + AspeedINTCState *s = ASPEED_INTC(opaque);
> > + AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > + const char *name = object_get_typename(OBJECT(s));
> > + uint32_t addr = offset >> 2;
> > +
> > + if (offset >= aic->reg_size) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Out-of-bounds write at offset 0x%"
> HWADDR_PRIx "\n",
> > + __func__, offset);
> > + return;
> > + }
> > +
> > + trace_aspeed_intc_write(name, offset, size, data);
> > +
> > + switch (addr) {
> > + case R_GICINT192_EN:
> > + case R_GICINT193_EN:
> > + case R_GICINT194_EN:
> > + case R_GICINT195_EN:
> > + case R_GICINT196_EN:
> > + case R_GICINT197_EN:
> > + aspeed_intc_enable_handler(s, offset, data);
> > + break;
> > + case R_GICINT192_STATUS:
> > + case R_GICINT193_STATUS:
> > + case R_GICINT194_STATUS:
> > + case R_GICINT195_STATUS:
> > + case R_GICINT196_STATUS:
> > + case R_GICINT197_STATUS:
> > + aspeed_intc_status_handler(s, offset, data);
> > + break;
> > + default:
> > + s->regs[addr] = data;
> > + break;
> > + }
> > +
> > + return;
> > +}
> > +
> > +
> > static const MemoryRegionOps aspeed_intc_ops = {
> > .read = aspeed_intc_read,
> > .write = aspeed_intc_write,
> > @@ -473,6 +561,16 @@ static const MemoryRegionOps aspeed_intc_ops = {
> > }
> > };
> >
> > +static const MemoryRegionOps aspeed_intcio_ops = {
> > + .read = aspeed_intcio_read,
> > + .write = aspeed_intcio_write,
> > + .endianness = DEVICE_LITTLE_ENDIAN,
> > + .valid = {
> > + .min_access_size = 4,
> > + .max_access_size = 4,
> > + }
> > +};
> > +
> > static void aspeed_intc_instance_init(Object *obj)
> > {
> > AspeedINTCState *s = ASPEED_INTC(obj); @@ -572,10 +670,33 @@
> > static const TypeInfo aspeed_2700_intc_info = {
> > .class_init = aspeed_2700_intc_class_init,
> > };
> >
> > +static void aspeed_2700_intcio_class_init(ObjectClass *klass, void
> > +*data) {
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > + AspeedINTCClass *aic = ASPEED_INTC_CLASS(klass);
> > +
> > + dc->desc = "ASPEED 2700 INTC IO Controller";
> > + aic->num_lines = 32;
> > + aic->num_inpins = 6;
> > + aic->num_outpins = 6;
> > + aic->mem_size = 0x400;
> > + aic->reg_size = 0x3d8;
> > + aic->reg_ops = &aspeed_intcio_ops;
> > + aic->irq_table = aspeed_2700_intcio_irqs;
> > + aic->irq_table_count = ARRAY_SIZE(aspeed_2700_intcio_irqs);
> > +}
> > +
> > +static const TypeInfo aspeed_2700_intcio_info = {
> > + .name = TYPE_ASPEED_2700_INTCIO,
> > + .parent = TYPE_ASPEED_INTC,
> > + .class_init = aspeed_2700_intcio_class_init, };
> > +
> > static void aspeed_intc_register_types(void)
> > {
> > type_register_static(&aspeed_intc_info);
> > type_register_static(&aspeed_2700_intc_info);
> > + type_register_static(&aspeed_2700_intcio_info);
> > }
> >
> > type_init(aspeed_intc_register_types);
> > diff --git a/include/hw/intc/aspeed_intc.h
> > b/include/hw/intc/aspeed_intc.h index 57146db2ce..e8ead15491 100644
> > --- a/include/hw/intc/aspeed_intc.h
> > +++ b/include/hw/intc/aspeed_intc.h
> > @@ -14,6 +14,7 @@
> >
> > #define TYPE_ASPEED_INTC "aspeed.intc"
> > #define TYPE_ASPEED_2700_INTC TYPE_ASPEED_INTC "-ast2700"
> > +#define TYPE_ASPEED_2700_INTCIO TYPE_ASPEED_INTC "io" "-ast2700"
>
> You could use "io-ast2700". Minor.
>
Will update to "io-ast2700"
Thanks for your review and suggestion.
Jamin
> Thanks,
>
> C.
>
>
> > OBJECT_DECLARE_TYPE(AspeedINTCState, AspeedINTCClass,
> ASPEED_INTC)
> >
> > #define ASPEED_INTC_NR_REGS (0x2000 >> 2)
