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
