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

[Peter Maydell]

On Mon, 12 Feb 2024 at 16:34, 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>
> ---
>  hw/arm/stm32l4x5_soc.c | 69 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 59 insertions(+), 10 deletions(-)
>
> diff --git a/hw/arm/stm32l4x5_soc.c b/hw/arm/stm32l4x5_soc.c
> index f470ff74ec..df5bb02315 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"
> @@ -48,15 +49,14 @@ static const int exti_irq[NUM_EXTI_IRQ] = {
>      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,13 +69,29 @@ 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      */
>  };
>
> +#define NUM_EXTI_OR_GATES 4
> +static const int exti_or_gates_out[NUM_EXTI_OR_GATES] = {
> +    23, 40, 63, 1,
> +};
> +
> +#define NUM_EXTI_SIMPLE_FANIN_IRQ 3
> +static const int exti_or_gates_num_lines_in[NUM_EXTI_SIMPLE_FANIN_IRQ] = {
> +    5, 6, 2,
> +};

This array only has three elements, but you always set the
num-lines property on the OR gate using this array for all
NUM_EXTI_OR_GATES gates. I think the array should be size
NUM_EXTI_OR_GATES and have an extra element '5' at the end.

> +
> +static const int exti_or_gates_first_line_in[NUM_EXTI_SIMPLE_FANIN_IRQ] = {
> +    5, 10, 21,
> +};



> +
> +#define NUM_EXTI_OR_GATE1_NUM_LINES_IN 5
> +static const int exti_or_gate1_lines_in[NUM_EXTI_OR_GATE1_NUM_LINES_IN] = {
> +    16, 35, 36, 37, 38,
> +};



> +
>  static void stm32l4x5_soc_initfn(Object *obj)
>  {
>      Stm32l4x5SocState *s = STM32L4X5_SOC(obj);
> @@ -175,8 +191,41 @@ 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++) {
> +        Object *orgate = object_new(TYPE_OR_IRQ);
> +        object_property_set_int(orgate, "num-lines",
> +            exti_or_gates_num_lines_in[i], &error_fatal);
> +        /* Should unref be used? */
> +        qdev_realize(DEVICE(orgate), NULL, &error_fatal);

There are two patterns for create-and-realize:
 (1) create with qdev_new() etc, realize with qdev_realize_and_unref()
 (2) create with object_initialize_child(), realize with qdev_realize()
(the doc comments in qdev-core.h try to explain these two patterns.)

This is an SoC container object, so it's better to use the
pattern where the sub-devices of it (like these OR gates) are
given fields in the SoC's state struct, initialized with
object_initialize_child() in the SoC init function, and then
realized here with qdev_realize().

There's an example of doing it this way in hw/arm/armsse.c.

> +
> +        qdev_connect_gpio_out(DEVICE(orgate), 0,
> +            qdev_get_gpio_in(armv7m, exti_or_gates_out[i]));
> +
> +        /* consecutive inputs for OR gates 23, 40, 63 */
> +        if (i < NUM_EXTI_SIMPLE_FANIN_IRQ) {
> +            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(orgate), j));
> +            }
> +        /* non-consecutive inputs for OR gate 1 */

This comment would be better inside the block of the 'else', I think.

> +        } else {
> +            for (unsigned j = 0; j < NUM_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(orgate), 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++) {
> --

thanks
-- PMM