Re: [PATCH v2 2/3] hw/misc: In STM32L4x5 EXTI, handle direct and configurable interrupts

[Peter Maydell <peter.maydell@linaro.org>]

On Wed, 22 May 2024 at 21:40, Inès Varhol <ines.varhol@telecom-paris.fr> wrote:
>
> The previous implementation for EXTI interrupts only handled
> "configurable" interrupts, like those originating from STM32L4x5 SYSCFG
> (the only device currently connected to the EXTI up until now).
>
> In order to connect STM32L4x5 USART to the EXTI, this commit adds
> handling for direct interrupts (interrupts without configurable edge).
>
> The implementation of configurable interrupts (interrupts supporting
> edge selection) was incorrectly expecting alternating input levels :
> this commits adds a new status field `irq_levels` to actually detect
> edges.

This patch is now doing two different things:
 * correcting the handling of detecting of rising/falling edges
 * adding the direct-interrupt support

These should be two separate patches, I think.

> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>  include/hw/misc/stm32l4x5_exti.h |  2 ++
>  hw/misc/stm32l4x5_exti.c         | 25 ++++++++++++++++++++-----
>  2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/misc/stm32l4x5_exti.h b/include/hw/misc/stm32l4x5_exti.h
> index 82f75a2417..62f79362f2 100644
> --- a/include/hw/misc/stm32l4x5_exti.h
> +++ b/include/hw/misc/stm32l4x5_exti.h
> @@ -45,6 +45,8 @@ struct Stm32l4x5ExtiState {
>      uint32_t swier[EXTI_NUM_REGISTER];
>      uint32_t pr[EXTI_NUM_REGISTER];
>
> +    /* used for edge detection */
> +    uint32_t irq_levels[EXTI_NUM_REGISTER];
>      qemu_irq irq[EXTI_NUM_LINES];
>  };
>
> diff --git a/hw/misc/stm32l4x5_exti.c b/hw/misc/stm32l4x5_exti.c
> index eebefc6cd3..bdc3dc10d6 100644
> --- a/hw/misc/stm32l4x5_exti.c
> +++ b/hw/misc/stm32l4x5_exti.c
> @@ -87,6 +87,7 @@ static void stm32l4x5_exti_reset_hold(Object *obj, ResetType type)
>          s->ftsr[bank] = 0x00000000;
>          s->swier[bank] = 0x00000000;
>          s->pr[bank] = 0x00000000;
> +        s->irq_levels[bank] = 0x00000000;
>      }
>  }
>
> @@ -106,17 +107,30 @@ static void stm32l4x5_exti_set_irq(void *opaque, int irq, int level)
>          return;
>      }
>
> +    /* In case of a direct line interrupt */
> +    if (extract32(exti_romask[bank], irq, 1)) {
> +        qemu_set_irq(s->irq[oirq], level);
> +        return;
> +    }
> +
> +    /* In case of a configurable interrupt */
>      if (((1 << irq) & s->rtsr[bank]) && level) {
>          /* Rising Edge */
> -        s->pr[bank] |= 1 << irq;
> -        qemu_irq_pulse(s->irq[oirq]);
> +        if (!extract32(s->irq_levels[bank], irq, 1)) {
> +            s->pr[bank] |= 1 << irq;
> +            qemu_irq_pulse(s->irq[oirq]);
> +        }
> +        s->irq_levels[bank] |= 1 << irq;
>      } else if (((1 << irq) & s->ftsr[bank]) && !level) {
>          /* Falling Edge */
> -        s->pr[bank] |= 1 << irq;
> -        qemu_irq_pulse(s->irq[oirq]);
> +        if (extract32(s->irq_levels[bank], irq, 1)) {
> +            s->pr[bank] |= 1 << irq;
> +            qemu_irq_pulse(s->irq[oirq]);
> +        }
> +        s->irq_levels[bank] &= ~(1 << irq);
>      }

The irq_levels[] array should be updated based on 'level'
separately from determining whether it's a rising or falling
edge. With this code, if for instance the line is
configured for rising-edge detection then we never clear
the irq_levels[] bit when the level falls again, because
the only place we're clearing irq_levels bits is inside the
falling-edge-detected codepath.

We also don't get the case of "guest set the bit in both the
RTSR and FTSR right, which the datasheet specifically calls out
as permitted.

I think something like this will do the right thing:

    if (level == extract32(s->irq_levels[bank], irq, 1)) {
        /* No change in IRQ line state: do nothing */
        return;
    }
    s->irq_levels[bank] = deposit32(s->irq_levels[bank], irq, level);

    if ((level && extract32(s->rtsr[bank], irq, 1) ||
        (!level && extract32(s->ftsr[bank], irq, 1)) {
        /*
         * Trigger on this rising or falling edge. Note that the guest
         * can configure the same interrupt line to trigger on both
         * rising and falling edges if it wishes, or to not trigger
         * at all.
         */
        s->pr[bank] |= 1 << irq;
        qemu_irq_pulse(s->irq[oirq]);
    }

(I would then consider the "In the following situations" comment
to be a bit redundant and deleteable, but that's a matter of taste.)

>      /*
> -     * In the following situations :
> +     * In the following situations (for configurable interrupts) :
>       * - falling edge but rising trigger selected
>       * - rising edge but falling trigger selected
>       * - no trigger selected
> @@ -262,6 +276,7 @@ static const VMStateDescription vmstate_stm32l4x5_exti = {
>          VMSTATE_UINT32_ARRAY(ftsr, Stm32l4x5ExtiState, EXTI_NUM_REGISTER),
>          VMSTATE_UINT32_ARRAY(swier, Stm32l4x5ExtiState, EXTI_NUM_REGISTER),
>          VMSTATE_UINT32_ARRAY(pr, Stm32l4x5ExtiState, EXTI_NUM_REGISTER),
> +        VMSTATE_UINT32_ARRAY(irq_levels, Stm32l4x5ExtiState, EXTI_NUM_REGISTER),
>          VMSTATE_END_OF_LIST()
>      }

If you add a new field to vmstate you must always bump the version numbers.

thanks
-- PMM