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

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.

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

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