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_lin@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;
+} 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);
- 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;
}
@@ -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;
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 = {
--
2.34.1On 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_lin@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 ?
> +} 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.
> - 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 ?
> 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.
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 = {
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_lin@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 = {
© 2016 - 2026 Red Hat, Inc.