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

[Jamin Lin]

Hi Andrew,

> Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support
> 
> 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.

Got it. Will update it.
Thanks for suggestion and review.

> 
> >
> > 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