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

[Andrew Jeffery]

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.

Cheers,

Andrew