Hi Andres,
> Subject: Re: [PATCH v2 4/6] hw/gpio/aspeed: Fix clear incorrect interrupt
> status
> for GPIO index mode
>
> Hi Jamin,
>
> On Wed, 2024-09-25 at 11:34 +0800, Jamin Lin wrote:
> > 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).
> >
> > The result is that if multiple interrupt status bits were pending and
> > the write was acknowledging specific one bit, then the all interrupt
> > status bits will be cleared.
> > However, it is index mode and should only clear the corresponding bit.
> >
> > 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:
> >
> > reg_value == 0b11
> > pending == 2
> > s->pending = 0
>
> Note that I had a typo in my reply that prompted this patch, this was meant to
> be:
>
> s->pending == 0
>
Will update
> > set->int_status == 0b00
> >
> > It should only clear bit 0 in index mode and the correct result should
> > be as following.
> >
> > set->int_status == 0b11
> > s->pending == 2
> >
> > pending == 1
> > s->pending = 1
>
> Same typo here now :)
>
Will update
> s->pending == 1
>
> > set->int_status == 0b10
> >
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > ---
> > hw/gpio/aspeed_gpio.c | 19 ++++++++++---------
> > 1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c index
> > 6e6ab48b56..58ae63e3c1 100644
> > --- a/hw/gpio/aspeed_gpio.c
> > +++ b/hw/gpio/aspeed_gpio.c
> > @@ -642,7 +642,7 @@ static void aspeed_gpio_write_index_mode(void
> *opaque, hwaddr offset,
> > uint32_t pin_idx = reg_idx_number % ASPEED_GPIOS_PER_SET;
> > uint32_t group_idx = pin_idx / GPIOS_PER_GROUP;
> > uint32_t reg_value = 0;
> > - uint32_t cleared;
> > + uint32_t pending = 0;
> >
> > set = &s->sets[set_idx];
> > props = &agc->props[set_idx];
> > @@ -705,15 +705,16 @@ static void aspeed_gpio_write_index_mode(void
> *opaque, hwaddr offset,
> > set->int_sens_2 = update_value_control_source(set,
> set->int_sens_2,
> >
> reg_value);
> > /* set interrupt status */
> > - reg_value = set->int_status;
> > - reg_value = deposit32(reg_value, pin_idx, 1,
> > - FIELD_EX32(data, GPIO_INDEX_REG,
> INT_STATUS));
> > - cleared = ctpop32(reg_value & set->int_status);
> > - if (s->pending && cleared) {
> > - assert(s->pending >= cleared);
> > - s->pending -= cleared;
> > + if (FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS)) {
> > + pending = extract32(set->int_status, pin_idx, 1);
> > + if (pending) {
> > + if (s->pending) {
> > + assert(s->pending >= pending);
> > + s->pending -= pending;
> > + }
> > + set->int_status = deposit32(set->int_status, pin_idx, 1,
> 0);
> > + }
>
> So I think we can get away without the nested conditionals?
>
> if (FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS)) {
> /* pending is either 1 or 0 for a 1-bit field */
> pending = extract32(set->int_status, pin_idx, 1);
>
> /*
> * The assert is effectively a compressed form of
> *
> * assert((s->pending == 0 && pending == 0) ||
> * (s->pending >= 1));
> *
> * This seems reasonable.
> *
> * Another way to write it is:
> *
> * assert(!pending || s->pending));
> */
> assert(s->pending >= pending);
>
> /* No change to s->pending if pending is 0 */
> s->pending -= pending;
>
> /*
> * The write acknowledged the interrupt regardless of whether it
> * was pending or not. The post-condition is that it mustn't be
> * pending. Unconditionally clear the status bit.
> */
> s->int_status = deposit32(set->int_status, pin_idx, 1, 0);
> }
>
> Thoughts?
>
Got it.
I am happy to update them in V3.
Thanks for suggestion and review.
Jamin
> Up to you whether you keep the comments if the idea makes sense and you
> choose to adopt it.
>
> Andrew
