Hi Cedric,
>
> It's good practice to add "No functional change".
>
Thanks for review and suggestion
Will add "No functional change" in commit log.
Jamin
>
> Looks good to me.
>
>
> Reviewed-by: Cédric Le Goater <c...@redhat.com>
>
> Thanks,
>
> C.
>
>
>
> >
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > ---
> > hw/intc/aspeed_intc.c | 190 ++++++++++++++++++++++++------------------
> > 1 file changed, 108 insertions(+), 82 deletions(-)
> >
> > diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
> > 316885a27a..8f9fa97acc 100644
> > --- a/hw/intc/aspeed_intc.c
> > +++ b/hw/intc/aspeed_intc.c
> > @@ -114,6 +114,112 @@ static void aspeed_intc_set_irq(void *opaque, int
> irq, int level)
> > }
> > }
> >
> > +static void aspeed_intc_enable_handler(AspeedINTCState *s, hwaddr offset,
> > + uint64_t data) {
> > + AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > + uint32_t addr = offset >> 2;
> > + uint32_t old_enable;
> > + uint32_t change;
> > + uint32_t irq;
> > +
> > + irq = (offset & 0x0f00) >> 8;
> > +
> > + if (irq >= aic->num_ints) {
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid interrupt
> number: %d\n",
> > + __func__, irq);
> > + return;
> > + }
> > +
> > + /*
> > + * The enable registers are used to enable source interrupts.
> > + * They also handle masking and unmasking of source interrupts
> > + * during the execution of the source ISR.
> > + */
> > +
> > + /* disable all source interrupt */
> > + if (!data && !s->enable[irq]) {
> > + s->regs[addr] = data;
> > + return;
> > + }
> > +
> > + old_enable = s->enable[irq];
> > + s->enable[irq] |= data;
> > +
> > + /* enable new source interrupt */
> > + if (old_enable != s->enable[irq]) {
> > + trace_aspeed_intc_enable(s->enable[irq]);
> > + s->regs[addr] = data;
> > + return;
> > + }
> > +
> > + /* mask and unmask source interrupt */
> > + change = s->regs[addr] ^ data;
> > + if (change & data) {
> > + s->mask[irq] &= ~change;
> > + trace_aspeed_intc_unmask(change, s->mask[irq]);
> > + } else {
> > + s->mask[irq] |= change;
> > + trace_aspeed_intc_mask(change, s->mask[irq]);
> > + }
> > +
> > + s->regs[addr] = data;
> > +}
> > +
> > +static void aspeed_intc_status_handler(AspeedINTCState *s, hwaddr offset,
> > + uint64_t data) {
> > + AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > + uint32_t addr = offset >> 2;
> > + uint32_t irq;
> > +
> > + if (!data) {
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid data 0\n",
> __func__);
> > + return;
> > + }
> > +
> > + irq = (offset & 0x0f00) >> 8;
> > +
> > + if (irq >= aic->num_ints) {
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid interrupt
> number: %d\n",
> > + __func__, irq);
> > + return;
> > + }
> > +
> > + /* clear status */
> > + s->regs[addr] &= ~data;
> > +
> > + /*
> > + * These status registers are used for notify sources ISR are executed.
> > + * If one source ISR is executed, it will clear one bit.
> > + * If it clear all bits, it means to initialize this register status
> > + * rather than sources ISR are executed.
> > + */
> > + if (data == 0xffffffff) {
> > + return;
> > + }
> > +
> > + /* All source ISR execution are done */
> > + if (!s->regs[addr]) {
> > + trace_aspeed_intc_all_isr_done(irq);
> > + if (s->pending[irq]) {
> > + /*
> > + * handle pending source interrupt
> > + * notify firmware which source interrupt are pending
> > + * by setting status register
> > + */
> > + s->regs[addr] = s->pending[irq];
> > + s->pending[irq] = 0;
> > + trace_aspeed_intc_trigger_irq(irq, s->regs[addr]);
> > + aspeed_intc_update(s, irq, 1);
> > + } else {
> > + /* clear irq */
> > + trace_aspeed_intc_clear_irq(irq, 0);
> > + aspeed_intc_update(s, irq, 0);
> > + }
> > + }
> > +}
> > +
> > static uint64_t aspeed_intc_read(void *opaque, hwaddr offset, unsigned
> int size)
> > {
> > AspeedINTCState *s = ASPEED_INTC(opaque); @@ -140,9 +246,6
> @@
> > static void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> > AspeedINTCState *s = ASPEED_INTC(opaque);
> > AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > uint32_t addr = offset >> 2;
> > - uint32_t old_enable;
> > - uint32_t change;
> > - uint32_t irq;
> >
> > if (offset >= aic->reg_size) {
> > qemu_log_mask(LOG_GUEST_ERROR, @@ -163,45 +266,7 @@
> static
> > void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> > case R_GICINT134_EN:
> > case R_GICINT135_EN:
> > case R_GICINT136_EN:
> > - irq = (offset & 0x0f00) >> 8;
> > -
> > - if (irq >= aic->num_ints) {
> > - qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid interrupt
> number: %d\n",
> > - __func__, irq);
> > - return;
> > - }
> > -
> > - /*
> > - * These registers are used for enable sources interrupt and
> > - * mask and unmask source interrupt while executing source ISR.
> > - */
> > -
> > - /* disable all source interrupt */
> > - if (!data && !s->enable[irq]) {
> > - s->regs[addr] = data;
> > - return;
> > - }
> > -
> > - old_enable = s->enable[irq];
> > - s->enable[irq] |= data;
> > -
> > - /* enable new source interrupt */
> > - if (old_enable != s->enable[irq]) {
> > - trace_aspeed_intc_enable(s->enable[irq]);
> > - s->regs[addr] = data;
> > - return;
> > - }
> > -
> > - /* mask and unmask source interrupt */
> > - change = s->regs[addr] ^ data;
> > - if (change & data) {
> > - s->mask[irq] &= ~change;
> > - trace_aspeed_intc_unmask(change, s->mask[irq]);
> > - } else {
> > - s->mask[irq] |= change;
> > - trace_aspeed_intc_mask(change, s->mask[irq]);
> > - }
> > - s->regs[addr] = data;
> > + aspeed_intc_enable_handler(s, offset, data);
> > break;
> > case R_GICINT128_STATUS:
> > case R_GICINT129_STATUS:
> > @@ -212,46 +277,7 @@ static void aspeed_intc_write(void *opaque,
> hwaddr offset, uint64_t data,
> > case R_GICINT134_STATUS:
> > case R_GICINT135_STATUS:
> > case R_GICINT136_STATUS:
> > - irq = (offset & 0x0f00) >> 8;
> > -
> > - if (irq >= aic->num_ints) {
> > - qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid interrupt
> number: %d\n",
> > - __func__, irq);
> > - return;
> > - }
> > -
> > - /* clear status */
> > - s->regs[addr] &= ~data;
> > -
> > - /*
> > - * These status registers are used for notify sources ISR are
> executed.
> > - * If one source ISR is executed, it will clear one bit.
> > - * If it clear all bits, it means to initialize this register
> > status
> > - * rather than sources ISR are executed.
> > - */
> > - if (data == 0xffffffff) {
> > - return;
> > - }
> > -
> > - /* All source ISR execution are done */
> > - if (!s->regs[addr]) {
> > - trace_aspeed_intc_all_isr_done(irq);
> > - if (s->pending[irq]) {
> > - /*
> > - * handle pending source interrupt
> > - * notify firmware which source interrupt are pending
> > - * by setting status register
> > - */
> > - s->regs[addr] = s->pending[irq];
> > - s->pending[irq] = 0;
> > - trace_aspeed_intc_trigger_irq(irq, s->regs[addr]);
> > - aspeed_intc_update(s, irq, 1);
> > - } else {
> > - /* clear irq */
> > - trace_aspeed_intc_clear_irq(irq, 0);
> > - aspeed_intc_update(s, irq, 0);
> > - }
> > - }
> > + aspeed_intc_status_handler(s, offset, data);
> > break;
> > default:
> > s->regs[addr] = data;
