| Message ID | 20250213033531.3367697-12-jamin_lin@aspeedtech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support AST2700 A1 | expand |
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,
Hi Cedric, > > It's good practice to add "No functional change". > > Reviewed-by: Cédric Le Goater <clg@redhat.com> > > Thanks, > > C. > Thanks for your review and suggestion. I will add "No functional change" in commit log. Jamin > > > 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,
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,
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. Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> --- hw/intc/aspeed_intc.c | 62 ++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 28 deletions(-)