[PATCH v2 0/2] hw/arm: Fix STM32L4x5 EXTI to CPU irq fan-in connections

[PATCH v2 0/2] hw/arm: Fix STM32L4x5 EXTI to CPU irq fan-in connections

[Inès Varhol]

The original code was connecting several outbounds qemu_irqs to the
same qemu_irq without using a TYPE_OR_IRQ.

This patch fixes the issue by using OR gates when necessary (1st commit).

I attempted to check that the problem is fixed by using a QTest (2nd commit)
but actually the test is passing even before the fix :
when any fan-in input line is raised, the output is raised too.

Changes from v1 :
- using SoC State fields for EXTI OR gates
- correcting length of array `exti_or_gates_num_lines_in`
- using a for loop in the test for more clarity
- correcting typo in test comment

Fixes: 52671f69f7a4 ("[PATCH v8 0/3] Add device STM32L4x5 EXTI")
Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>

Inès Varhol (2):
  hw/arm: Use TYPE_OR_IRQ when connecting STM32L4x5 EXTI fan-in IRQs
  tests/qtest: Check that EXTI fan-in irqs are correctly connected

 include/hw/arm/stm32l4x5_soc.h    |  4 ++
 hw/arm/stm32l4x5_soc.c            | 80 +++++++++++++++++++++++++++----
 tests/qtest/stm32l4x5_exti-test.c | 37 ++++++++++++++
 3 files changed, 111 insertions(+), 10 deletions(-)

-- 
2.43.2

[PATCH v2 1/2] hw/arm: Use TYPE_OR_IRQ when connecting STM32L4x5 EXTI fan-in IRQs

[Inès Varhol]

Fixes: 52671f69f7a4 ("[PATCH v8 0/3] Add device STM32L4x5 EXTI")
Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
---
 include/hw/arm/stm32l4x5_soc.h |  4 ++
 hw/arm/stm32l4x5_soc.c         | 80 +++++++++++++++++++++++++++++-----
 2 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/include/hw/arm/stm32l4x5_soc.h b/include/hw/arm/stm32l4x5_soc.h
index baf70410b5..4f314b7a93 100644
--- a/include/hw/arm/stm32l4x5_soc.h
+++ b/include/hw/arm/stm32l4x5_soc.h
@@ -26,6 +26,7 @@
 
 #include "exec/memory.h"
 #include "hw/arm/armv7m.h"
+#include "hw/or-irq.h"
 #include "hw/misc/stm32l4x5_syscfg.h"
 #include "hw/misc/stm32l4x5_exti.h"
 #include "qom/object.h"
@@ -36,12 +37,15 @@
 #define TYPE_STM32L4X5XG_SOC "stm32l4x5xg-soc"
 OBJECT_DECLARE_TYPE(Stm32l4x5SocState, Stm32l4x5SocClass, STM32L4X5_SOC)
 
+#define NUM_EXTI_OR_GATES 4
+
 struct Stm32l4x5SocState {
     SysBusDevice parent_obj;
 
     ARMv7MState armv7m;
 
     Stm32l4x5ExtiState exti;
+    OrIRQState exti_or_gates[NUM_EXTI_OR_GATES];
     Stm32l4x5SyscfgState syscfg;
 
     MemoryRegion sram1;
diff --git a/hw/arm/stm32l4x5_soc.c b/hw/arm/stm32l4x5_soc.c
index f470ff74ec..d1786e0da1 100644
--- a/hw/arm/stm32l4x5_soc.c
+++ b/hw/arm/stm32l4x5_soc.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
+#include "hw/or-irq.h"
 #include "hw/arm/stm32l4x5_soc.h"
 #include "hw/qdev-clock.h"
 #include "hw/misc/unimp.h"
@@ -42,21 +43,24 @@
 #define NUM_EXTI_IRQ 40
 /* Match exti line connections with their CPU IRQ number */
 /* See Vector Table (Reference Manual p.396) */
+/*
+ * Some IRQs are connected to the same CPU IRQ (denoted by -1)
+ * and require an intermediary OR gate to function correctly.
+ */
 static const int exti_irq[NUM_EXTI_IRQ] = {
     6,                      /* GPIO[0]                 */
     7,                      /* GPIO[1]                 */
     8,                      /* GPIO[2]                 */
     9,                      /* GPIO[3]                 */
     10,                     /* GPIO[4]                 */
-    23, 23, 23, 23, 23,     /* GPIO[5..9]              */
-    40, 40, 40, 40, 40, 40, /* GPIO[10..15]            */
-    1,                      /* PVD                     */
+    -1, -1, -1, -1, -1,     /* GPIO[5..9] OR gate 23   */
+    -1, -1, -1, -1, -1, -1, /* GPIO[10..15] OR gate 40 */
+    -1,                     /* PVD OR gate 1           */
     67,                     /* OTG_FS_WKUP, Direct     */
     41,                     /* RTC_ALARM               */
     2,                      /* RTC_TAMP_STAMP2/CSS_LSE */
     3,                      /* RTC wakeup timer        */
-    63,                     /* COMP1                   */
-    63,                     /* COMP2                   */
+    -1, -1,                 /* COMP[1..2] OR gate 63   */
     31,                     /* I2C1 wakeup, Direct     */
     33,                     /* I2C2 wakeup, Direct     */
     72,                     /* I2C3 wakeup, Direct     */
@@ -69,18 +73,39 @@ static const int exti_irq[NUM_EXTI_IRQ] = {
     65,                     /* LPTIM1, Direct          */
     66,                     /* LPTIM2, Direct          */
     76,                     /* SWPMI1 wakeup, Direct   */
-    1,                      /* PVM1 wakeup             */
-    1,                      /* PVM2 wakeup             */
-    1,                      /* PVM3 wakeup             */
-    1,                      /* PVM4 wakeup             */
+    -1, -1, -1, -1,         /* PVM[1..4] OR gate 1     */
     78                      /* LCD wakeup, Direct      */
 };
 
+static const int exti_or_gates_out[NUM_EXTI_OR_GATES] = {
+    23, 40, 63, 1,
+};
+
+static const int exti_or_gates_num_lines_in[NUM_EXTI_OR_GATES] = {
+    5, 6, 2, 5,
+};
+
+/* 3 OR gates with consecutive inputs */
+#define NUM_EXTI_SIMPLE_OR_GATES 3
+static const int exti_or_gates_first_line_in[NUM_EXTI_SIMPLE_OR_GATES] = {
+    5, 10, 21,
+};
+
+/* 1 OR gate with non-consecutive inputs */
+#define EXTI_OR_GATE1_NUM_LINES_IN 5
+static const int exti_or_gate1_lines_in[EXTI_OR_GATE1_NUM_LINES_IN] = {
+    16, 35, 36, 37, 38,
+};
+
 static void stm32l4x5_soc_initfn(Object *obj)
 {
     Stm32l4x5SocState *s = STM32L4X5_SOC(obj);
 
     object_initialize_child(obj, "exti", &s->exti, TYPE_STM32L4X5_EXTI);
+    for (unsigned i = 0; i < NUM_EXTI_OR_GATES; i++) {
+        object_initialize_child(obj, "exti_or_gates[*]", &s->exti_or_gates[i],
+                                TYPE_OR_IRQ);
+    }
     object_initialize_child(obj, "syscfg", &s->syscfg, TYPE_STM32L4X5_SYSCFG);
 
     s->sysclk = qdev_init_clock_in(DEVICE(s), "sysclk", NULL, NULL, 0);
@@ -175,8 +200,43 @@ static void stm32l4x5_soc_realize(DeviceState *dev_soc, Error **errp)
         return;
     }
     sysbus_mmio_map(busdev, 0, EXTI_ADDR);
+
+    /* IRQs with fan-in that require an OR gate */
+    for (unsigned i = 0; i < NUM_EXTI_OR_GATES; i++) {
+        if (!object_property_set_int(OBJECT(&s->exti_or_gates[i]), "num-lines",
+                                     exti_or_gates_num_lines_in[i], errp)) {
+            return;
+        }
+        if (!qdev_realize(DEVICE(&s->exti_or_gates[i]), NULL, errp)) {
+            return;
+        }
+
+        qdev_connect_gpio_out(DEVICE(&s->exti_or_gates[i]), 0,
+            qdev_get_gpio_in(armv7m, exti_or_gates_out[i]));
+
+        if (i < NUM_EXTI_SIMPLE_OR_GATES) {
+            /* consecutive inputs for OR gates 23, 40, 63 */
+            for (unsigned j = 0; j < exti_or_gates_num_lines_in[i]; j++) {
+                sysbus_connect_irq(SYS_BUS_DEVICE(&s->exti),
+                    exti_or_gates_first_line_in[i] + j,
+                    qdev_get_gpio_in(DEVICE(&s->exti_or_gates[i]), j));
+            }
+        } else {
+            /* non-consecutive inputs for OR gate 1 */
+            for (unsigned j = 0; j < EXTI_OR_GATE1_NUM_LINES_IN; j++) {
+                sysbus_connect_irq(SYS_BUS_DEVICE(&s->exti),
+                    exti_or_gate1_lines_in[j],
+                    qdev_get_gpio_in(DEVICE(&s->exti_or_gates[i]), j));
+            }
+        }
+    }
+
+    /* IRQs that don't require fan-in */
     for (unsigned i = 0; i < NUM_EXTI_IRQ; i++) {
-        sysbus_connect_irq(busdev, i, qdev_get_gpio_in(armv7m, exti_irq[i]));
+        if (exti_irq[i] != -1) {
+            sysbus_connect_irq(busdev, i,
+                               qdev_get_gpio_in(armv7m, exti_irq[i]));
+        }
     }
 
     for (unsigned i = 0; i < 16; i++) {
-- 
2.43.2

Re: [PATCH v2 1/2] hw/arm: Use TYPE_OR_IRQ when connecting STM32L4x5 EXTI fan-in IRQs

[Philippe Mathieu-Daudé]

On 20/2/24 19:34, Inès Varhol wrote:
> Fixes: 52671f69f7a4 ("[PATCH v8 0/3] Add device STM32L4x5 EXTI")
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>   include/hw/arm/stm32l4x5_soc.h |  4 ++
>   hw/arm/stm32l4x5_soc.c         | 80 +++++++++++++++++++++++++++++-----
>   2 files changed, 74 insertions(+), 10 deletions(-)

Thanks for cleaning that!
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 1/2] hw/arm: Use TYPE_OR_IRQ when connecting STM32L4x5 EXTI fan-in IRQs

[Alistair Francis]

On Wed, Feb 21, 2024 at 4:42 AM Inès Varhol
<ines.varhol@telecom-paris.fr> wrote:
>
> Fixes: 52671f69f7a4 ("[PATCH v8 0/3] Add device STM32L4x5 EXTI")
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/arm/stm32l4x5_soc.h |  4 ++
>  hw/arm/stm32l4x5_soc.c         | 80 +++++++++++++++++++++++++++++-----
>  2 files changed, 74 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/arm/stm32l4x5_soc.h b/include/hw/arm/stm32l4x5_soc.h
> index baf70410b5..4f314b7a93 100644
> --- a/include/hw/arm/stm32l4x5_soc.h
> +++ b/include/hw/arm/stm32l4x5_soc.h
> @@ -26,6 +26,7 @@
>
>  #include "exec/memory.h"
>  #include "hw/arm/armv7m.h"
> +#include "hw/or-irq.h"
>  #include "hw/misc/stm32l4x5_syscfg.h"
>  #include "hw/misc/stm32l4x5_exti.h"
>  #include "qom/object.h"
> @@ -36,12 +37,15 @@
>  #define TYPE_STM32L4X5XG_SOC "stm32l4x5xg-soc"
>  OBJECT_DECLARE_TYPE(Stm32l4x5SocState, Stm32l4x5SocClass, STM32L4X5_SOC)
>
> +#define NUM_EXTI_OR_GATES 4
> +
>  struct Stm32l4x5SocState {
>      SysBusDevice parent_obj;
>
>      ARMv7MState armv7m;
>
>      Stm32l4x5ExtiState exti;
> +    OrIRQState exti_or_gates[NUM_EXTI_OR_GATES];
>      Stm32l4x5SyscfgState syscfg;
>
>      MemoryRegion sram1;
> diff --git a/hw/arm/stm32l4x5_soc.c b/hw/arm/stm32l4x5_soc.c
> index f470ff74ec..d1786e0da1 100644
> --- a/hw/arm/stm32l4x5_soc.c
> +++ b/hw/arm/stm32l4x5_soc.c
> @@ -26,6 +26,7 @@
>  #include "qapi/error.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/or-irq.h"
>  #include "hw/arm/stm32l4x5_soc.h"
>  #include "hw/qdev-clock.h"
>  #include "hw/misc/unimp.h"
> @@ -42,21 +43,24 @@
>  #define NUM_EXTI_IRQ 40
>  /* Match exti line connections with their CPU IRQ number */
>  /* See Vector Table (Reference Manual p.396) */
> +/*
> + * Some IRQs are connected to the same CPU IRQ (denoted by -1)
> + * and require an intermediary OR gate to function correctly.
> + */
>  static const int exti_irq[NUM_EXTI_IRQ] = {
>      6,                      /* GPIO[0]                 */
>      7,                      /* GPIO[1]                 */
>      8,                      /* GPIO[2]                 */
>      9,                      /* GPIO[3]                 */
>      10,                     /* GPIO[4]                 */
> -    23, 23, 23, 23, 23,     /* GPIO[5..9]              */
> -    40, 40, 40, 40, 40, 40, /* GPIO[10..15]            */
> -    1,                      /* PVD                     */
> +    -1, -1, -1, -1, -1,     /* GPIO[5..9] OR gate 23   */
> +    -1, -1, -1, -1, -1, -1, /* GPIO[10..15] OR gate 40 */
> +    -1,                     /* PVD OR gate 1           */
>      67,                     /* OTG_FS_WKUP, Direct     */
>      41,                     /* RTC_ALARM               */
>      2,                      /* RTC_TAMP_STAMP2/CSS_LSE */
>      3,                      /* RTC wakeup timer        */
> -    63,                     /* COMP1                   */
> -    63,                     /* COMP2                   */
> +    -1, -1,                 /* COMP[1..2] OR gate 63   */
>      31,                     /* I2C1 wakeup, Direct     */
>      33,                     /* I2C2 wakeup, Direct     */
>      72,                     /* I2C3 wakeup, Direct     */
> @@ -69,18 +73,39 @@ static const int exti_irq[NUM_EXTI_IRQ] = {
>      65,                     /* LPTIM1, Direct          */
>      66,                     /* LPTIM2, Direct          */
>      76,                     /* SWPMI1 wakeup, Direct   */
> -    1,                      /* PVM1 wakeup             */
> -    1,                      /* PVM2 wakeup             */
> -    1,                      /* PVM3 wakeup             */
> -    1,                      /* PVM4 wakeup             */
> +    -1, -1, -1, -1,         /* PVM[1..4] OR gate 1     */
>      78                      /* LCD wakeup, Direct      */
>  };
>
> +static const int exti_or_gates_out[NUM_EXTI_OR_GATES] = {
> +    23, 40, 63, 1,
> +};
> +
> +static const int exti_or_gates_num_lines_in[NUM_EXTI_OR_GATES] = {
> +    5, 6, 2, 5,
> +};
> +
> +/* 3 OR gates with consecutive inputs */
> +#define NUM_EXTI_SIMPLE_OR_GATES 3
> +static const int exti_or_gates_first_line_in[NUM_EXTI_SIMPLE_OR_GATES] = {
> +    5, 10, 21,
> +};
> +
> +/* 1 OR gate with non-consecutive inputs */
> +#define EXTI_OR_GATE1_NUM_LINES_IN 5
> +static const int exti_or_gate1_lines_in[EXTI_OR_GATE1_NUM_LINES_IN] = {
> +    16, 35, 36, 37, 38,
> +};
> +
>  static void stm32l4x5_soc_initfn(Object *obj)
>  {
>      Stm32l4x5SocState *s = STM32L4X5_SOC(obj);
>
>      object_initialize_child(obj, "exti", &s->exti, TYPE_STM32L4X5_EXTI);
> +    for (unsigned i = 0; i < NUM_EXTI_OR_GATES; i++) {
> +        object_initialize_child(obj, "exti_or_gates[*]", &s->exti_or_gates[i],
> +                                TYPE_OR_IRQ);
> +    }
>      object_initialize_child(obj, "syscfg", &s->syscfg, TYPE_STM32L4X5_SYSCFG);
>
>      s->sysclk = qdev_init_clock_in(DEVICE(s), "sysclk", NULL, NULL, 0);
> @@ -175,8 +200,43 @@ static void stm32l4x5_soc_realize(DeviceState *dev_soc, Error **errp)
>          return;
>      }
>      sysbus_mmio_map(busdev, 0, EXTI_ADDR);
> +
> +    /* IRQs with fan-in that require an OR gate */
> +    for (unsigned i = 0; i < NUM_EXTI_OR_GATES; i++) {
> +        if (!object_property_set_int(OBJECT(&s->exti_or_gates[i]), "num-lines",
> +                                     exti_or_gates_num_lines_in[i], errp)) {
> +            return;
> +        }
> +        if (!qdev_realize(DEVICE(&s->exti_or_gates[i]), NULL, errp)) {
> +            return;
> +        }
> +
> +        qdev_connect_gpio_out(DEVICE(&s->exti_or_gates[i]), 0,
> +            qdev_get_gpio_in(armv7m, exti_or_gates_out[i]));
> +
> +        if (i < NUM_EXTI_SIMPLE_OR_GATES) {
> +            /* consecutive inputs for OR gates 23, 40, 63 */
> +            for (unsigned j = 0; j < exti_or_gates_num_lines_in[i]; j++) {
> +                sysbus_connect_irq(SYS_BUS_DEVICE(&s->exti),
> +                    exti_or_gates_first_line_in[i] + j,
> +                    qdev_get_gpio_in(DEVICE(&s->exti_or_gates[i]), j));
> +            }
> +        } else {
> +            /* non-consecutive inputs for OR gate 1 */
> +            for (unsigned j = 0; j < EXTI_OR_GATE1_NUM_LINES_IN; j++) {
> +                sysbus_connect_irq(SYS_BUS_DEVICE(&s->exti),
> +                    exti_or_gate1_lines_in[j],
> +                    qdev_get_gpio_in(DEVICE(&s->exti_or_gates[i]), j));
> +            }
> +        }
> +    }
> +
> +    /* IRQs that don't require fan-in */
>      for (unsigned i = 0; i < NUM_EXTI_IRQ; i++) {
> -        sysbus_connect_irq(busdev, i, qdev_get_gpio_in(armv7m, exti_irq[i]));
> +        if (exti_irq[i] != -1) {
> +            sysbus_connect_irq(busdev, i,
> +                               qdev_get_gpio_in(armv7m, exti_irq[i]));
> +        }
>      }
>
>      for (unsigned i = 0; i < 16; i++) {
> --
> 2.43.2
>
>

[PATCH v2 2/2] tests/qtest: Check that EXTI fan-in irqs are correctly connected

[Inès Varhol]

This commit adds a QTest that verifies each input line of a specific
EXTI OR gate can influence the output line.

Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

Hello,

I expected this test to fail after switching the two patch commits,
but it didn't.
I'm mentionning it in case it reveals a problem with the test I didn't notice.


 tests/qtest/stm32l4x5_exti-test.c | 37 +++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tests/qtest/stm32l4x5_exti-test.c b/tests/qtest/stm32l4x5_exti-test.c
index c390077713..81830be8ae 100644
--- a/tests/qtest/stm32l4x5_exti-test.c
+++ b/tests/qtest/stm32l4x5_exti-test.c
@@ -31,6 +31,7 @@
 
 #define EXTI0_IRQ 6
 #define EXTI1_IRQ 7
+#define EXTI5_9_IRQ 23
 #define EXTI35_IRQ 1
 
 static void enable_nvic_irq(unsigned int n)
@@ -499,6 +500,40 @@ static void test_interrupt(void)
     g_assert_false(check_nvic_pending(EXTI1_IRQ));
 }
 
+static void test_orred_interrupts(void)
+{
+    /*
+     * For lines EXTI5..9 (fanned-in to NVIC irq 23),
+     * test that raising the line pends interrupt
+     * 23 in NVIC.
+     */
+    enable_nvic_irq(EXTI5_9_IRQ);
+    /* Check that there are no interrupts already pending in PR */
+    g_assert_cmpuint(exti_readl(EXTI_PR1), ==, 0x00000000);
+    /* Check that this specific interrupt isn't pending in NVIC */
+    g_assert_false(check_nvic_pending(EXTI5_9_IRQ));
+
+    /* Enable interrupt lines EXTI[5..9] */
+    exti_writel(EXTI_IMR1, (0x1F << 5));
+
+    /* Configure interrupt on rising edge */
+    exti_writel(EXTI_RTSR1, (0x1F << 5));
+
+    /* Raise GPIO line i, check that the interrupt is pending */
+    for (unsigned i = 5; i < 10; i++) {
+        exti_set_irq(i, 1);
+        g_assert_cmpuint(exti_readl(EXTI_PR1), ==, 1 << i);
+        g_assert_true(check_nvic_pending(EXTI5_9_IRQ));
+
+        exti_writel(EXTI_PR1, 1 << i);
+        g_assert_cmpuint(exti_readl(EXTI_PR1), ==, 0x00000000);
+        g_assert_true(check_nvic_pending(EXTI5_9_IRQ));
+
+        unpend_nvic_irq(EXTI5_9_IRQ);
+        g_assert_false(check_nvic_pending(EXTI5_9_IRQ));
+    }
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -515,6 +550,8 @@ int main(int argc, char **argv)
     qtest_add_func("stm32l4x5/exti/masked_interrupt", test_masked_interrupt);
     qtest_add_func("stm32l4x5/exti/interrupt", test_interrupt);
     qtest_add_func("stm32l4x5/exti/test_edge_selector", test_edge_selector);
+    qtest_add_func("stm32l4x5/exti/test_orred_interrupts",
+                   test_orred_interrupts);
 
     qtest_start("-machine b-l475e-iot01a");
     ret = g_test_run();
-- 
2.43.2

Re: [PATCH v2 2/2] tests/qtest: Check that EXTI fan-in irqs are correctly connected

[Peter Maydell]

On Tue, 20 Feb 2024 at 18:41, Inès Varhol <ines.varhol@telecom-paris.fr> wrote:
>
> This commit adds a QTest that verifies each input line of a specific
> EXTI OR gate can influence the output line.
>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>
> Hello,
>
> I expected this test to fail after switching the two patch commits,
> but it didn't.
> I'm mentionning it in case it reveals a problem with the test I didn't notice.

The specific thing that goes wrong if you don't have the OR
gate handling is that the NVIC input will see every up
and down transition from each input separately. (This happens
because a GPIO/irq 'input' in QEMU is basically a function,
and wiring up an 'output' to an 'input' is setting "this
is the function pointer you should call when the output
changes". Nothing syntactically stops you passing the
same function pointer to multiple outputs.)

So if you have for instance
 raise A; raise B; drop B; drop A
where A and B are ORed together into an NVIC input,
the NVIC input is supposed to see the line go high
at "raise A" and only drop at the last "drop B". Without
the OR gate, it will see it go high at "raise A", and then
drop at "drop B". (Well, it sees "level is 1", "level is 1",
"level is 0", "level is 0", but inputs expect to sometimes see
calls for "level happens to be the same thing it was
previously", so it doesn't cause the NVIC to change state.)

Your test case doesn't as far as I can see check this situation,
because it's (a) testing every input in order, not checking
what happens when multiple inputs are raised and lowered
in overlapping ways and (b) using rising-edge interrupts.
So that's why it doesn't fail even without the bug fix in
the previous patch.

>  #define EXTI0_IRQ 6
>  #define EXTI1_IRQ 7
> +#define EXTI5_9_IRQ 23
>  #define EXTI35_IRQ 1
>
>  static void enable_nvic_irq(unsigned int n)
> @@ -499,6 +500,40 @@ static void test_interrupt(void)
>      g_assert_false(check_nvic_pending(EXTI1_IRQ));
>  }
>
> +static void test_orred_interrupts(void)
> +{
> +    /*
> +     * For lines EXTI5..9 (fanned-in to NVIC irq 23),
> +     * test that raising the line pends interrupt
> +     * 23 in NVIC.
> +     */
> +    enable_nvic_irq(EXTI5_9_IRQ);
> +    /* Check that there are no interrupts already pending in PR */
> +    g_assert_cmpuint(exti_readl(EXTI_PR1), ==, 0x00000000);
> +    /* Check that this specific interrupt isn't pending in NVIC */
> +    g_assert_false(check_nvic_pending(EXTI5_9_IRQ));
> +
> +    /* Enable interrupt lines EXTI[5..9] */
> +    exti_writel(EXTI_IMR1, (0x1F << 5));
> +
> +    /* Configure interrupt on rising edge */
> +    exti_writel(EXTI_RTSR1, (0x1F << 5));
> +
> +    /* Raise GPIO line i, check that the interrupt is pending */
> +    for (unsigned i = 5; i < 10; i++) {
> +        exti_set_irq(i, 1);
> +        g_assert_cmpuint(exti_readl(EXTI_PR1), ==, 1 << i);
> +        g_assert_true(check_nvic_pending(EXTI5_9_IRQ));
> +
> +        exti_writel(EXTI_PR1, 1 << i);
> +        g_assert_cmpuint(exti_readl(EXTI_PR1), ==, 0x00000000);
> +        g_assert_true(check_nvic_pending(EXTI5_9_IRQ));
> +
> +        unpend_nvic_irq(EXTI5_9_IRQ);
> +        g_assert_false(check_nvic_pending(EXTI5_9_IRQ));
> +    }
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int ret;
> @@ -515,6 +550,8 @@ int main(int argc, char **argv)
>      qtest_add_func("stm32l4x5/exti/masked_interrupt", test_masked_interrupt);
>      qtest_add_func("stm32l4x5/exti/interrupt", test_interrupt);
>      qtest_add_func("stm32l4x5/exti/test_edge_selector", test_edge_selector);
> +    qtest_add_func("stm32l4x5/exti/test_orred_interrupts",
> +                   test_orred_interrupts);
>
>      qtest_start("-machine b-l475e-iot01a");
>      ret = g_test_run();
> --
> 2.43.2

thanks
-- PMM

Re: [PATCH v2 2/2] tests/qtest: Check that EXTI fan-in irqs are correctly connected

[Peter Maydell]

On Thu, 22 Feb 2024 at 15:09, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 20 Feb 2024 at 18:41, Inès Varhol <ines.varhol@telecom-paris.fr> wrote:
> >
> > This commit adds a QTest that verifies each input line of a specific
> > EXTI OR gate can influence the output line.
> >
> > Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >
> > Hello,
> >
> > I expected this test to fail after switching the two patch commits,
> > but it didn't.
> > I'm mentionning it in case it reveals a problem with the test I didn't notice.
>
> The specific thing that goes wrong if you don't have the OR
> gate handling is that the NVIC input will see every up
> and down transition from each input separately. (This happens
> because a GPIO/irq 'input' in QEMU is basically a function,
> and wiring up an 'output' to an 'input' is setting "this
> is the function pointer you should call when the output
> changes". Nothing syntactically stops you passing the
> same function pointer to multiple outputs.)
>
> So if you have for instance
>  raise A; raise B; drop B; drop A
> where A and B are ORed together into an NVIC input,
> the NVIC input is supposed to see the line go high
> at "raise A" and only drop at the last "drop B". Without

...at the last "drop *A*", I mean.

> the OR gate, it will see it go high at "raise A", and then
> drop at "drop B". (Well, it sees "level is 1", "level is 1",
> "level is 0", "level is 0", but inputs expect to sometimes see
> calls for "level happens to be the same thing it was
> previously", so it doesn't cause the NVIC to change state.)

-- PMM

Re: [PATCH v2 0/2] hw/arm: Fix STM32L4x5 EXTI to CPU irq fan-in connections

[Peter Maydell]

On Tue, 20 Feb 2024 at 18:41, Inès Varhol <ines.varhol@telecom-paris.fr> wrote:
>
> The original code was connecting several outbounds qemu_irqs to the
> same qemu_irq without using a TYPE_OR_IRQ.
>
> This patch fixes the issue by using OR gates when necessary (1st commit).
>
> I attempted to check that the problem is fixed by using a QTest (2nd commit)
> but actually the test is passing even before the fix :
> when any fan-in input line is raised, the output is raised too.
>
> Changes from v1 :
> - using SoC State fields for EXTI OR gates
> - correcting length of array `exti_or_gates_num_lines_in`
> - using a for loop in the test for more clarity
> - correcting typo in test comment
>
> Fixes: 52671f69f7a4 ("[PATCH v8 0/3] Add device STM32L4x5 EXTI")
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
>
> Inès Varhol (2):
>   hw/arm: Use TYPE_OR_IRQ when connecting STM32L4x5 EXTI fan-in IRQs
>   tests/qtest: Check that EXTI fan-in irqs are correctly connected



Applied to target-arm.next, thanks.

-- PMM