On Wed, 22 May 2024 at 21:40, Inès Varhol <ines.var...@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.var...@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
