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
