Hi Cedric,
> Subject: Re: [PATCH v4 12/23] hw/intc/aspeed: Introduce AspeedINTCIRQ
> structure to save the irq index and register address
>
> On 3/3/25 10:54, Jamin Lin wrote:
> > The INTC controller supports GICINT128 to GICINT136, mapping 1:1 to
> > input and output IRQs 0 to 8. Previously, the formula "address &
> > 0x0f00" was used to derive the IRQ index numbers.
> >
> > However, the INTC controller also supports GICINT192_201, mapping 1
> > input IRQ pin to 10 output IRQ pins. The pin numbers for input and output
> > are
> different.
> > It is difficult to use a formula to determine the index number of INTC
> > model supported input and output IRQs.
> >
> > To simplify and improve readability, introduces the AspeedINTCIRQ
> > structure to save the input/output IRQ index and its enable/status register
> address.
> >
> > Introduce the "aspeed_2700_intc_irqs" table to store IRQ information for
> INTC.
> > Introduce the "aspeed_intc_get_irq" function to retrieve the
> > input/output IRQ pin index from the provided status/enable register address.
> >
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > ---
> > include/hw/intc/aspeed_intc.h | 10 +++
> > hw/intc/aspeed_intc.c | 120
> ++++++++++++++++++++--------------
> > 2 files changed, 82 insertions(+), 48 deletions(-)
> >
> > diff --git a/include/hw/intc/aspeed_intc.h
> > b/include/hw/intc/aspeed_intc.h index f9a0273f78..112a01f402 100644
> > --- a/include/hw/intc/aspeed_intc.h
> > +++ b/include/hw/intc/aspeed_intc.h
> > @@ -20,6 +20,14 @@ OBJECT_DECLARE_TYPE(AspeedINTCState,
> AspeedINTCClass, ASPEED_INTC)
> > #define ASPEED_INTC_MAX_INPINS 9
> > #define ASPEED_INTC_MAX_OUTPINS 9
> >
> > +typedef struct AspeedINTCIRQ {
> > + int inpin_idx;
> > + int outpin_idx;
> > + int num_outpins;
> > + uint32_t enable_addr;
> > + uint32_t status_addr;
>
> The _addr suffix is confusing. Doesn't it refer to a register index ?
>
I will change to enable_reg and status_reg.
> > +} AspeedINTCIRQ;
> > +
> > struct AspeedINTCState {
> > /*< private >*/
> > SysBusDevice parent_obj;
> > @@ -47,6 +55,8 @@ struct AspeedINTCClass {
> > uint64_t reg_size;
> > uint64_t reg_offset;
> > const MemoryRegionOps *reg_ops;
> > + const AspeedINTCIRQ *irq_table;
> > + int irq_table_count;
> > };
> >
> > #endif /* ASPEED_INTC_H */
> > diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
> > 9bc3e089d8..5730a7604d 100644
> > --- a/hw/intc/aspeed_intc.c
> > +++ b/hw/intc/aspeed_intc.c
> > @@ -40,7 +40,23 @@ REG32(GICINT135_STATUS, 0x704)
> > REG32(GICINT136_EN, 0x800)
> > REG32(GICINT136_STATUS, 0x804)
> >
> > -#define GICINT_STATUS_BASE R_GICINT128_STATUS
> > +static const AspeedINTCIRQ *aspeed_intc_get_irq(AspeedINTCClass *aic,
> > + uint32_t addr) {
> > + int i;
> > +
> > + for (i = 0; i < aic->irq_table_count; i++) {
> > + if (aic->irq_table[i].enable_addr == addr ||
> > + aic->irq_table[i].status_addr == addr) {
> > + return &aic->irq_table[i];
> > + }
> > + }
> > +
> > + /*
> > + * Invalid addr.
> > + */
> > + g_assert_not_reached();
> > +}
> >
> > /*
> > * Update the state of an interrupt controller pin by setting @@
> > -81,15 +97,10 @@ static void aspeed_intc_set_irq(void *opaque, int irq, int
> level)
> > AspeedINTCState *s = (AspeedINTCState *)opaque;
> > AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > const char *name = object_get_typename(OBJECT(s));
> > - uint32_t status_addr = GICINT_STATUS_BASE + ((0x100 * irq) >> 2);
> > + const AspeedINTCIRQ *intc_irq;
> > uint32_t select = 0;
> > uint32_t enable;
> > int i;
> > - int inpin_idx;
> > - int outpin_idx;
> > -
> > - inpin_idx = irq;
> > - outpin_idx = irq;
> >
> > if (irq >= aic->num_inpins) {
> > qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid input pin
> index:
> > %d\n", @@ -97,15 +108,16 @@ static void aspeed_intc_set_irq(void
> *opaque, int irq, int level)
> > return;
> > }
> >
> > - trace_aspeed_intc_set_irq(name, inpin_idx, level);
> > - enable = s->enable[inpin_idx];
> > + intc_irq = &aic->irq_table[irq];
> > + trace_aspeed_intc_set_irq(name, intc_irq->inpin_idx, level);
> > + enable = s->enable[intc_irq->inpin_idx];
> >
> > if (!level) {
> > return;
> > }
> >
> > for (i = 0; i < aic->num_lines; i++) {
> > - if (s->orgates[inpin_idx].levels[i]) {
> > + if (s->orgates[intc_irq->inpin_idx].levels[i]) {
> > if (enable & BIT(i)) {
> > select |= BIT(i);
> > }
> > @@ -118,7 +130,7 @@ static void aspeed_intc_set_irq(void *opaque, int
> > irq, int level)
> >
> > trace_aspeed_intc_select(name, select);
> >
> > - if (s->mask[inpin_idx] || s->regs[status_addr]) {
> > + if (s->mask[intc_irq->inpin_idx] ||
> > + s->regs[intc_irq->status_addr]) {
> > /*
> > * a. mask is not 0 means in ISR mode
> > * sources interrupt routine are executing.
> > @@ -127,17 +139,19 @@ static void aspeed_intc_set_irq(void *opaque, int
> irq, int level)
> > *
> > * save source interrupt to pending variable.
> > */
> > - s->pending[inpin_idx] |= select;
> > - trace_aspeed_intc_pending_irq(name, inpin_idx,
> s->pending[inpin_idx]);
> > + s->pending[intc_irq->inpin_idx] |= select;
> > + trace_aspeed_intc_pending_irq(name, intc_irq->inpin_idx,
> > +
> > + s->pending[intc_irq->inpin_idx]);
> > } else {
> > /*
> > * notify firmware which source interrupt are coming
> > * by setting status register
> > */
> > - s->regs[status_addr] = select;
> > - trace_aspeed_intc_trigger_irq(name, inpin_idx, outpin_idx,
> > - s->regs[status_addr]);
> > - aspeed_intc_update(s, inpin_idx, outpin_idx, 1);
> > + s->regs[intc_irq->status_addr] = select;
> > + trace_aspeed_intc_trigger_irq(name, intc_irq->inpin_idx,
> > + intc_irq->outpin_idx,
> > +
> s->regs[intc_irq->status_addr]);
> > + aspeed_intc_update(s, intc_irq->inpin_idx,
> > + intc_irq->outpin_idx, 1);
> > }
> > }
> >
> > @@ -146,19 +160,17 @@ static void
> aspeed_intc_enable_handler(AspeedINTCState *s, hwaddr offset,
> > {
> > AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > const char *name = object_get_typename(OBJECT(s));
> > + const AspeedINTCIRQ *intc_irq;
> > uint32_t addr = offset >> 2;
> > uint32_t old_enable;
> > uint32_t change;
> > - uint32_t irq;
> > - int inpin_idx;
> >
> > - irq = (offset & 0x0f00) >> 8;
> > - inpin_idx = irq;
> > + intc_irq = aspeed_intc_get_irq(aic, addr);
>
> I would keep the 'inpin_idx' variable and assign it to 'intc_irq->inpin_idx'.
> This would reduce the number of change.
>
Will do
>
> > - if (inpin_idx >= aic->num_inpins) {
> > + if (intc_irq->inpin_idx >= aic->num_inpins) {
>
> Since both values are defined at compile time, shouldn't that be an assert ?
Will do
>
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "%s: Invalid input pin index: %d\n",
> > - __func__, inpin_idx);
> > + __func__, intc_irq->inpin_idx);
> > return;
> > }
> >
> > @@ -169,17 +181,17 @@ static void
> aspeed_intc_enable_handler(AspeedINTCState *s, hwaddr offset,
> > */
> >
> > /* disable all source interrupt */
> > - if (!data && !s->enable[inpin_idx]) {
> > + if (!data && !s->enable[intc_irq->inpin_idx]) {
> > s->regs[addr] = data;
> > return;
> > }
> >
> > - old_enable = s->enable[inpin_idx];
> > - s->enable[inpin_idx] |= data;
> > + old_enable = s->enable[intc_irq->inpin_idx];
> > + s->enable[intc_irq->inpin_idx] |= data;
> >
> > /* enable new source interrupt */
> > - if (old_enable != s->enable[inpin_idx]) {
> > - trace_aspeed_intc_enable(name, s->enable[inpin_idx]);
> > + if (old_enable != s->enable[intc_irq->inpin_idx]) {
> > + trace_aspeed_intc_enable(name,
> > + s->enable[intc_irq->inpin_idx]);
> > s->regs[addr] = data;
> > return;
> > }
> > @@ -187,11 +199,11 @@ static void
> aspeed_intc_enable_handler(AspeedINTCState *s, hwaddr offset,
> > /* mask and unmask source interrupt */
> > change = s->regs[addr] ^ data;
> > if (change & data) {
> > - s->mask[inpin_idx] &= ~change;
> > - trace_aspeed_intc_unmask(name, change, s->mask[inpin_idx]);
> > + s->mask[intc_irq->inpin_idx] &= ~change;
> > + trace_aspeed_intc_unmask(name, change,
> > + s->mask[intc_irq->inpin_idx]);
> > } else {
> > - s->mask[inpin_idx] |= change;
> > - trace_aspeed_intc_mask(name, change, s->mask[inpin_idx]);
> > + s->mask[intc_irq->inpin_idx] |= change;
> > + trace_aspeed_intc_mask(name, change,
> > + s->mask[intc_irq->inpin_idx]);
> > }
> >
> > s->regs[addr] = data;
> > @@ -202,24 +214,20 @@ static void
> aspeed_intc_status_handler(AspeedINTCState *s, hwaddr offset,
> > {
> > AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > const char *name = object_get_typename(OBJECT(s));
> > + const AspeedINTCIRQ *intc_irq;
> > uint32_t addr = offset >> 2;
> > - uint32_t irq;
> > - int inpin_idx;
> > - int outpin_idx;
>
>
> I would keep both these variables to reduce the number of changes.
Will do
Thanks for your review and suggestions.
Jamin
>
>
> Thanks,
>
> C.
>
>
> > if (!data) {
> > qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid data 0\n",
> __func__);
> > return;
> > }
> >
> > - irq = (offset & 0x0f00) >> 8;
> > - inpin_idx = irq;
> > - outpin_idx = irq;
> > + intc_irq = aspeed_intc_get_irq(aic, addr);
> >
> > - if (inpin_idx >= aic->num_inpins) {
> > + if (intc_irq->inpin_idx >= aic->num_inpins) {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "%s: Invalid input pin index: %d\n",
> > - __func__, inpin_idx);
> > + __func__, intc_irq->inpin_idx);
> > return;
> > }
> >
> > @@ -238,22 +246,24 @@ static void
> > aspeed_intc_status_handler(AspeedINTCState *s, hwaddr offset,
> >
> > /* All source ISR execution are done */
> > if (!s->regs[addr]) {
> > - trace_aspeed_intc_all_isr_done(name, inpin_idx);
> > - if (s->pending[inpin_idx]) {
> > + trace_aspeed_intc_all_isr_done(name, intc_irq->inpin_idx);
> > + if (s->pending[intc_irq->inpin_idx]) {
> > /*
> > * handle pending source interrupt
> > * notify firmware which source interrupt are pending
> > * by setting status register
> > */
> > - s->regs[addr] = s->pending[inpin_idx];
> > - s->pending[inpin_idx] = 0;
> > - trace_aspeed_intc_trigger_irq(name, inpin_idx, outpin_idx,
> > + s->regs[addr] = s->pending[intc_irq->inpin_idx];
> > + s->pending[intc_irq->inpin_idx] = 0;
> > + trace_aspeed_intc_trigger_irq(name, intc_irq->inpin_idx,
> > + intc_irq->outpin_idx,
> > s->regs[addr]);
> > - aspeed_intc_update(s, inpin_idx, outpin_idx, 1);
> > + aspeed_intc_update(s, intc_irq->inpin_idx,
> > + intc_irq->outpin_idx, 1);
> > } else {
> > /* clear irq */
> > - trace_aspeed_intc_clear_irq(name, inpin_idx, outpin_idx, 0);
> > - aspeed_intc_update(s, inpin_idx, outpin_idx, 0);
> > + trace_aspeed_intc_clear_irq(name, intc_irq->inpin_idx,
> > + intc_irq->outpin_idx, 0);
> > + aspeed_intc_update(s, intc_irq->inpin_idx,
> > + intc_irq->outpin_idx, 0);
> > }
> > }
> > }
> > @@ -416,6 +426,18 @@ static const TypeInfo aspeed_intc_info = {
> > .abstract = true,
> > };
> >
> > +static AspeedINTCIRQ aspeed_2700_intc_irqs[ASPEED_INTC_MAX_INPINS] =
> {
> > + {0, 0, 1, R_GICINT128_EN, R_GICINT128_STATUS},
> > + {1, 1, 1, R_GICINT129_EN, R_GICINT129_STATUS},
> > + {2, 2, 1, R_GICINT130_EN, R_GICINT130_STATUS},
> > + {3, 3, 1, R_GICINT131_EN, R_GICINT131_STATUS},
> > + {4, 4, 1, R_GICINT132_EN, R_GICINT132_STATUS},
> > + {5, 5, 1, R_GICINT133_EN, R_GICINT133_STATUS},
> > + {6, 6, 1, R_GICINT134_EN, R_GICINT134_STATUS},
> > + {7, 7, 1, R_GICINT135_EN, R_GICINT135_STATUS},
> > + {8, 8, 1, R_GICINT136_EN, R_GICINT136_STATUS}, };
> > +
> > static void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(klass); @@ -428,6 +450,8 @@
> > static void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
> > aic->mem_size = 0x4000;
> > aic->reg_size = 0x808;
> > aic->reg_offset = 0x1000;
> > + aic->irq_table = aspeed_2700_intc_irqs;
> > + aic->irq_table_count = ARRAY_SIZE(aspeed_2700_intc_irqs);
> > }
> >
> > static const TypeInfo aspeed_2700_intc_info = {
