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

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

On Sun, 12 May 2024 at 11:20, 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),
> as well as a comment that will be useful to connect other devices to the
> EXTI.
>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>  hw/misc/stm32l4x5_exti.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/hw/misc/stm32l4x5_exti.c b/hw/misc/stm32l4x5_exti.c
> index eebefc6cd3..1817bbdad2 100644
> --- a/hw/misc/stm32l4x5_exti.c
> +++ b/hw/misc/stm32l4x5_exti.c
> @@ -106,6 +106,27 @@ 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)) {
> +        if (level) {
> +            qemu_irq_raise(s->irq[oirq]);
> +        } else {
> +            qemu_irq_lower(s->irq[oirq]);
> +        }

You can say this more concisely as
           qemu_set_irq(s->irq[oirq], level);

> +        return;
> +    }
> +
> +    /*
> +     * In case of a configurable interrupt
> +     *
> +     * Note that while the real EXTI uses edge detection to tell
> +     * apart a line rising (the level changes from 0 to 1) and a line
> +     * staying high (the level was 1 and is set to 1), the current
> +     * implementation relies on the fact that this handler will only
> +     * be called when there's a level change. That means that the
> +     * devices creating a configurable interrupt (like STM32L4x5 GPIO)
> +     * have to set their IRQs only on a change.
> +     */

You cannot rely on this in QEMU's qemu_irq API. The set
function may be called multiple times with the same input
level value. If you need to detect rising edges then this
device needs to have a state field that records the current
value so it can compare the 'level' argument here against
what it was previously.

>      if (((1 << irq) & s->rtsr[bank]) && level) {
>          /* Rising Edge */
>          s->pr[bank] |= 1 << irq;
> @@ -116,7 +137,7 @@ static void stm32l4x5_exti_set_irq(void *opaque, int irq, int level)
>          qemu_irq_pulse(s->irq[oirq]);
>      }
>      /*
> -     * 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

thanks
-- PMM