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
