Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support

[Andrew Jeffery <andrew@codeconstruct.com.au>]

On Tue, 2024-09-24 at 06:48 +0000, Jamin Lin wrote:
> Hi Andrew,
> 
> > Subject: RE: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support
> > 
> > Hi Andrew,
> > 
> > > Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support
> > > 
> > > Hi Jamin,
> > > 
> > > 
> > > > +    }
> > > > +    set->int_status &= ~group_value;
> > > 
> > > This feels like it misbehaves in the face of multiple pending interrupts.
> > > 
> > > For example, say we have an interrupt pending for GPIOA0, where the
> > > following statements are true:
> > > 
> > >    set->int_status == 0b01
> > >    s->pending == 1
> > > 
> > > Before it is acknowledged, an interrupt becomes pending for GPIOA1:
> > > 
> > >    set->int_status == 0b11
> > >    s->pending == 2
> > > 
> > > A write is issued to acknowledge the interrupt for GPIOA0. This causes
> > > the following sequence:
> > > 
> > >    group_value == 0b11
> > >    cleared == 2
> > >    s->pending = 0
> > >    set->int_status == 0b00
> > > 
> > > It seems the pending interrupt for GPIOA1 is lost?
> > > 
> > Thanks for review and input.
> > I should check "int_status" bit of write data in write callback function. If 1 clear
> > status flag(group value), else should not change group value.
> > I am checking and testing this issue and will update to you or directly resend
> > the new patch series.
> 
> I appreciate your review and finding this issue.
> My changes as following.
> If you agree, I will add them in v2 patch.
> Thanks-Jamin
> 
> static void aspeed_gpio_2700_write_control_reg(AspeedGPIOState *s,
>                                 uint32_t pin, uint32_t type, uint64_t data)
> {
> ---
>     /* interrupt status */
>     if (SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS)) {
>         cleared = extract32(set->int_status, pin_idx, 1);
>         if (cleared) {
>             if (s->pending) {
>                 assert(s->pending >= cleared);
>                 s->pending -= cleared;
>             }
>             set->int_status = deposit32(set->int_status, pin_idx, 1, 0);
>         }
>     }
> ----
> }

The logic is easier to follow. Not sure about calling the value
extracted from set->int_status 'cleared' though, seems confusing on
first pass. It would feel more appropriate if it were called 'pending'.
I think 'cleared' is derived from `SHARED_FIELD_EX32(data,
GPIO_CONTROL_INT_STATUS)`. Anyway, that's just some quibbling over
names.

> 
> By the way, I found the same issue in "aspeed_gpio_write_index_mode" and my changes as following.
> If you agree this change, I will create a new patch in v2 patch series.
> 
> static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
>                                                 uint64_t data, uint32_t size)
> {
> ---
>     case gpio_reg_idx_interrupt:
>         if (FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS)) {
>             cleared = extract32(set->int_status, pin_idx, 1);
>             if (cleared) {
>                 if (s->pending) {
>                     assert(s->pending >= cleared);
>                     s->pending -= cleared;
>                 }
>                 set->int_status = deposit32(set->int_status, pin_idx, 1, 0);
>             }
>         }
>         break;
> ---
> }

I'll take a look in v2.

Cheers,

Andrew