Hi Andrew,
> Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support
>
> On Tue, 2024-09-24 at 03:03 +0000, Jamin Lin wrote:
> > Hi Andrew,
> >
> > > Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support
> > >
> > > Hi Jamin,
> > >
> > > On Mon, 2024-09-23 at 17:42 +0800, Jamin Lin wrote:
> > >
> > > > +
> > > > + /* interrupt status */
> > > > + group_value = set->int_status;
> > > > + group_value = deposit32(group_value, pin_idx, 1,
> > > > + SHARED_FIELD_EX32(data,
> > > > + GPIO_CONTROL_INT_STATUS));
> > >
> > > This makes me a bit wary.
> > >
> > > The interrupt status field is W1C, where a set bit on read indicates
> > > an interrupt is pending. If the bit extracted from data is set it
> > > should clear the corresponding bit in group_value. However, if the
> > > extracted bit is clear then the value of the corresponding bit in
> > > group_value
> should be unchanged.
> > >
> > > SHARED_FIELD_EX32() extracts the interrupt status bit from the write
> (data).
> > > group_value is set to the set's interrupt status, which means that
> > > for any pin with an interrupt pending, the corresponding bit is set.
> > > The deposit32() call updates the bit at pin_idx in the group, using
> > > the value extracted from the write (data).
> > >
> > > However, the result is that if the interrupt was pending and the
> > > write was acknowledging it, then the update has no effect.
> > > Alternatively, if the interrupt was pending but the write was
> > > acknowledging it, then the update will mark the interrupt as
> > > pending. Or, if the interrupt was pending but the write was _not_
> > > acknowledging it, then the interrupt will _no longer_ be marked pending.
> > > If
> this is intentional it feels a bit hard to follow.
> > >
> > > > + cleared = ctpop32(group_value & set->int_status);
> > >
> > > Can this rather be expressed as
> > >
> > > ```
> > > cleared = SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS); ```
> > >
> > > > + if (s->pending && cleared) {
> > > > + assert(s->pending >= cleared);
> > > > + s->pending -= cleared;
> > >
> > > We're only ever going to be subtracting 1, as each GPIO has its own
> register.
> > > This feels overly abstract.
> > >
> > > > + }
> > > > + 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.
>
> Happy to take a look in a v2 of the series :)
>
> > > > +
> > > > /****************** Setup functions ******************/
> > >
> > > Bit of a nitpick, but I'm not personally a fan of banner comments like
> > > this.
> > >
> > Did you mean change as following?
> >
> > A.
> >
> > /************ Setup functions *****************/
> >
> > 1. /* Setup functions */
> > 2. /*
> > * Setup functions
> > */
>
> Either is fine, but I prefer 1.
>
Thanks for suggestion.
Will update it in V2
Thanks-Jamin
> Cheers,
>
> Andrew
