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
   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
   set->int_status == 0b10

Signed-off-by: Jamin Lin <jamin_lin@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);
+            }
         }
-        set->int_status &= ~reg_value;
         break;
     case gpio_reg_idx_debounce:
         reg_value = set->debounce_1;
-- 
2.34.1

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

>    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 :)

    s->pending == 1

>    set->int_status == 0b10
> 
> Signed-off-by: Jamin Lin <jamin_lin@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?

Up to you whether you keep the comments if the idea makes sense and you
choose to adopt it.

Andrew

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_lin@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