On Thu, 2024-09-26 at 15:45 +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
I think you should s/group_value/reg_value/ here as that's what's used
in the removed code below.
Though maybe Cédric can fix it up for you when he applies it if another
revision isn't otherwise necessary.
> 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
>
Maybe this is a bit forward, but:
Suggested-by: Andrew Jeffery <and...@codeconstruct.com.au>
Reviewed-by: Andrew Jeffery <and...@codeconstruct.com.au>
> Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> ---
> hw/gpio/aspeed_gpio.c | 27 +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index 8725606aec..16c18ea2f7 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -641,7 +641,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];
> @@ -703,16 +703,23 @@ static void aspeed_gpio_write_index_mode(void *opaque,
> hwaddr offset,
> FIELD_EX32(data, GPIO_INDEX_REG, INT_SENS_2));
> 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;
> + /* interrupt status */
> + 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);
> +
> + 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.
> + */
> + 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;
