On 2/13/25 04:35, Jamin Lin wrote:
> The behavior of the INTC set IRQ is almost identical between INTC and INTCIO.
> To reduce duplicated code, introduce the "aspeed_intc_set_irq_handler" function
> to handle both INTC and INTCIO IRQ behavior.
It's good practice to add "No functional change".
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
> hw/intc/aspeed_intc.c | 62 ++++++++++++++++++++++++-------------------
> 1 file changed, 34 insertions(+), 28 deletions(-)
>
> diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
> index 59c1069294..fd4f75805a 100644
> --- a/hw/intc/aspeed_intc.c
> +++ b/hw/intc/aspeed_intc.c
> @@ -92,11 +92,40 @@ static void aspeed_intc_update(AspeedINTCState *s, int inpin_idx,
> qemu_set_irq(s->output_pins[outpin_idx], level);
> }
>
> +static void aspeed_intc_set_irq_handler(AspeedINTCState *s,
> + const AspeedINTCIRQ *intc_irq,
> + uint32_t select)
> +{
> + const char *name = object_get_typename(OBJECT(s));
> +
> + 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.
> + * b. status register value is not 0 means previous
> + * source interrupt does not be executed, yet.
> + *
> + * save source interrupt to pending variable.
> + */
> + 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[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);
> + }
> +}
> +
> /*
> - * The address of GICINT128 to GICINT136 are from 0x1000 to 0x1804.
> - * Utilize "address & 0x0f00" to get the irq and irq output pin index
> - * The value of irq should be 0 to num_inpins.
> - * The irq 0 indicates GICINT128, irq 1 indicates GICINT129 and so on.
> + * GICINT128 to GICINT136 map 1:1 to input and output IRQs 0 to 8.
> + * The value of input IRQ should be between 0 and the number of inputs.
> */
> static void aspeed_intc_set_irq(void *opaque, int irq, int level)
> {
> @@ -135,30 +164,7 @@ static void aspeed_intc_set_irq(void *opaque, int irq, int level)
> }
>
> trace_aspeed_intc_select(name, select);
> -
> - 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.
> - * b. status register value is not 0 means previous
> - * source interrupt does not be executed, yet.
> - *
> - * save source interrupt to pending variable.
> - */
> - 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[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);
> - }
> + aspeed_intc_set_irq_handler(s, intc_irq, select);
> }
>
> static void aspeed_intc_enable_handler(AspeedINTCState *s, hwaddr offset,