Re: [PATCH v6 1/3] hw/misc: Implement STM32L4x5 EXTI

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 8/1/24 19:03, Inès Varhol wrote:
> Although very similar to the STM32F4xx EXTI, STM32L4x5 EXTI generates
> more than 32 event/interrupt requests and thus uses more registers
> than STM32F4xx EXTI which generates 23 event/interrupt requests.
> 
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
> 
> Should the for loop variables be `unsigned` rather than `int` ?

It depends on the iterated range. Here you iterate over
ARRAY_SIZE(Stm32l4x5ExtiState::irq) which is a size_t type,
which is unsigned. Amusingly we use both similarly:

$ git grep 'for (size_t' | wc -l
       56
$ git grep 'for (unsigned' | wc -l
       59

>   docs/system/arm/b-l475e-iot01a.rst |   5 +-
>   hw/misc/Kconfig                    |   3 +
>   hw/misc/meson.build                |   1 +
>   hw/misc/stm32l4x5_exti.c           | 292 +++++++++++++++++++++++++++++
>   hw/misc/trace-events               |   5 +
>   include/hw/misc/stm32l4x5_exti.h   |  51 +++++
>   6 files changed, 354 insertions(+), 3 deletions(-)
>   create mode 100644 hw/misc/stm32l4x5_exti.c
>   create mode 100644 include/hw/misc/stm32l4x5_exti.h
> 
> diff --git a/docs/system/arm/b-l475e-iot01a.rst b/docs/system/arm/b-l475e-iot01a.rst
> index 2b128e6b84..72f256ace7 100644
> --- a/docs/system/arm/b-l475e-iot01a.rst
> +++ b/docs/system/arm/b-l475e-iot01a.rst
> @@ -12,17 +12,16 @@ USART, I2C, SPI, CAN and USB OTG, as well as a variety of sensors.
>   Supported devices
>   """""""""""""""""
>   
> -Currently, B-L475E-IOT01A machine's implementation is minimal,
> -it only supports the following device:
> +Currently B-L475E-IOT01A machine's only supports the following devices:
>   
>   - Cortex-M4F based STM32L4x5 SoC
> +- STM32L4x5 EXTI (Extended interrupts and events controller)
>   
>   Missing devices
>   """""""""""""""
>   
>   The B-L475E-IOT01A does *not* support the following devices:
>   
> -- Extended interrupts and events controller (EXTI)
>   - Reset and clock control (RCC)
>   - Serial ports (UART)
>   - System configuration controller (SYSCFG)
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index cc8a8c1418..3efe3dc2cc 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -87,6 +87,9 @@ config STM32F4XX_SYSCFG
>   config STM32F4XX_EXTI
>       bool
>   
> +config STM32L4X5_EXTI
> +    bool
> +
>   config MIPS_ITU
>       bool
>   
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 36c20d5637..16db6e228d 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -110,6 +110,7 @@ system_ss.add(when: 'CONFIG_XLNX_VERSAL_TRNG', if_true: files(
>   system_ss.add(when: 'CONFIG_STM32F2XX_SYSCFG', if_true: files('stm32f2xx_syscfg.c'))
>   system_ss.add(when: 'CONFIG_STM32F4XX_SYSCFG', if_true: files('stm32f4xx_syscfg.c'))
>   system_ss.add(when: 'CONFIG_STM32F4XX_EXTI', if_true: files('stm32f4xx_exti.c'))
> +system_ss.add(when: 'CONFIG_STM32L4X5_EXTI', if_true: files('stm32l4x5_exti.c'))
>   system_ss.add(when: 'CONFIG_MPS2_FPGAIO', if_true: files('mps2-fpgaio.c'))
>   system_ss.add(when: 'CONFIG_MPS2_SCC', if_true: files('mps2-scc.c'))
>   
> diff --git a/hw/misc/stm32l4x5_exti.c b/hw/misc/stm32l4x5_exti.c
> new file mode 100644
> index 0000000000..aedf1fb370
> --- /dev/null
> +++ b/hw/misc/stm32l4x5_exti.c
> @@ -0,0 +1,292 @@
> +/*
> + * STM32L4x5 EXTI (Extended interrupts and events controller)
> + *
> + * Copyright (c) 2023 Arnaud Minier <arnaud.minier@telecom-paris.fr>
> + * Copyright (c) 2023 Samuel Tardieu <samuel.tardieu@telecom-paris.fr>
> + * Copyright (c) 2023 Inès Varhol <ines.varhol@telecom-paris.fr>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + * This work is based on the stm32f4xx_exti by Alistair Francis.
> + * Original code is licensed under the MIT License:
> + *
> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
> + */
> +
> +/*
> + * The reference used is the STMicroElectronics RM0351 Reference manual
> + * for STM32L4x5 and STM32L4x6 advanced Arm ® -based 32-bit MCUs.
> + * https://www.st.com/en/microcontrollers-microprocessors/stm32l4x5/documentation.html
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "trace.h"
> +#include "hw/irq.h"
> +#include "migration/vmstate.h"
> +#include "hw/misc/stm32l4x5_exti.h"
> +
> +#define EXTI_IMR1   0x00
> +#define EXTI_EMR1   0x04
> +#define EXTI_RTSR1  0x08
> +#define EXTI_FTSR1  0x0C
> +#define EXTI_SWIER1 0x10
> +#define EXTI_PR1    0x14

> +#define EXTI_IMR2   0x20
> +#define EXTI_EMR2   0x24
> +#define EXTI_RTSR2  0x28
> +#define EXTI_FTSR2  0x2C
> +#define EXTI_SWIER2 0x30
> +#define EXTI_PR2    0x34
> +
> +#define EXTI_NUM_GPIO_EVENT_IN_LINES 16

   #define EXTI_MAX_IRQ_PER_BANK 32

> +
> +/* 0b11111111_10000010_00000000_00000000 */
> +#define DIRECT_LINE_MASK1 0xFF820000
> +/* 0b00000000_00000000_00000000_10000111 */
> +#define DIRECT_LINE_MASK2 0x00000087
> +/* 0b11111111_11111111_11111111_00000000 */
> +#define RESERVED_BITS_MASK2 0xFFFFFF00
> +
> +/* 0b00000000_00000000_00000000_01111000 */
> +#define ACTIVABLE_MASK2 (~DIRECT_LINE_MASK2 & ~RESERVED_BITS_MASK2)


You might want to declare:

   #define EXTI_IRQS_BANK0  32
   #define EXTI_IRQS_BANK1  8

   static const unsigned irqs_per_bank[EXTI_NUM_REGISTER] = {
       EXTI_IRQS_BANK0,
       EXTI_IRQS_BANK1,
   };

   static const uint32_t exti_romask[EXTI_NUM_REGISTER] = {
       0xff820000, /* 0b11111111_10000010_00000000_00000000 */
       0x00000087, /* 0b00000000_00000000_00000000_10000111 */
   };

And add descriptive helpers such:

static unsigned regbank_index_by_irq(unsigned irq)
{
     return irq >= EXTI_MAX_IRQ_PER_BANK ? 1 : 0;
}

static unsigned regbank_index_by_addr(hwaddr addr)
{
     return addr >= EXTI_IMR2 ? 1 : 0;
}

static unsigned valid_mask(unsigned bank)
{
     return MAKE_64BIT_MASK(0, irqs_per_bank[bank]);
}

(then see below)

> +static void stm32l4x5_exti_reset_hold(Object *obj)
> +{
> +    Stm32l4x5ExtiState *s = STM32L4X5_EXTI(obj);
> +
> +    s->imr[0] = DIRECT_LINE_MASK1;
> +    s->emr[0] = 0x00000000;
> +    s->rtsr[0] = 0x00000000;
> +    s->ftsr[0] = 0x00000000;
> +    s->swier[0] = 0x00000000;
> +    s->pr[0] = 0x00000000;
> +
> +    s->imr[1] = DIRECT_LINE_MASK2;
> +    s->emr[1] = 0x00000000;
> +    s->rtsr[1] = 0x00000000;
> +    s->ftsr[1] = 0x00000000;
> +    s->swier[1] = 0x00000000;
> +    s->pr[1] = 0x00000000;
> +}

BTW stm32l4x5_exti_reset_hold() can also be unified:

    static void stm32l4x5_exti_reset_hold(Object *obj)
    {
        Stm32l4x5ExtiState *s = STM32L4X5_EXTI(obj);

        for (unsigned bank = 0; bank < EXTI_NUM_REGISTER; bank++) {
            s->imr[bank] = exti_romask[bank];
            s->emr[bank] = 0;
            s->rtsr[bank] = 0;
            s->ftsr[bank] = 0;
            s->swier[bank] = 0;
            s->pr[bank] = 0;
        }
    }

> +static void stm32l4x5_exti_set_irq(void *opaque, int irq, int level)
> +{
> +    Stm32l4x5ExtiState *s = opaque;
> +    const unsigned n = irq >= 32;

Then:

       n = regbank_index_by_irq(irq);

Maybe 'bank' is a better variable name instead of 'n', in particular
in stm32l4x5_exti_read() it makes it clear registers behave identically
for each bank of registers.

> +    const int oirq = irq;
> +
> +    trace_stm32l4x5_exti_set_irq(irq, level);
> +
> +    if (irq >= 32) {
> +        /* Shift the value to enable access in x2 registers. */
> +        irq -= 32;
> +    }

Alternatively:

       irq %= EXTI_MAX_IRQ_PER_BANK;

> +    /* If the interrupt is masked, pr won't be raised */
> +    if (!((1 << irq) & s->imr[n])) {

Alternatively:

        if (!extract32(s->imr[n], irq, 1)) {

> +        return;
> +    }
> +
> +    if (((1 << irq) & s->rtsr[n]) && level) {
> +        /* Rising Edge */
> +        s->pr[n] |= 1 << irq;
> +        qemu_irq_pulse(s->irq[oirq]);
> +    } else if (((1 << irq) & s->ftsr[n]) && !level) {
> +        /* Falling Edge */
> +        s->pr[n] |= 1 << irq;
> +        qemu_irq_pulse(s->irq[oirq]);
> +    }

        else { // Can we get here?

> +}
> +
> +static uint64_t stm32l4x5_exti_read(void *opaque, hwaddr addr,
> +                                    unsigned int size)
> +{
> +    Stm32l4x5ExtiState *s = opaque;
> +    uint32_t r = 0;
> +    const unsigned n = addr >= EXTI_IMR2;

       bank = regbank_index_by_addr(addr);

> +
> +    switch (addr) {
> +    case EXTI_IMR1:
> +    case EXTI_IMR2:
> +        r = s->imr[n];
> +        break;
> +    case EXTI_EMR1:
> +    case EXTI_EMR2:
> +        r = s->emr[n];
> +        break;
> +    case EXTI_RTSR1:
> +    case EXTI_RTSR2:
> +        r = s->rtsr[n];
> +        break;
> +    case EXTI_FTSR1:
> +    case EXTI_FTSR2:
> +        r = s->ftsr[n];
> +        break;
> +    case EXTI_SWIER1:
> +    case EXTI_SWIER2:
> +        r = s->swier[n];
> +        break;
> +    case EXTI_PR1:
> +    case EXTI_PR2:
> +        r = s->pr[n];
> +        break;
> +
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "STM32L4X5_exti_read: Bad offset 0x%x\n", (int)addr);

Here you can avoid the cast by using the HWADDR_PRIx format.

> +        break;
> +    }
> +
> +    trace_stm32l4x5_exti_read(addr, r);
> +
> +    return r;
> +}
> +
> +static void stm32l4x5_exti_write(void *opaque, hwaddr addr,
> +                                 uint64_t val64, unsigned int size)
> +{
> +    Stm32l4x5ExtiState *s = opaque;
> +    const uint32_t value = (uint32_t)val64;

I don't see the benefit of casting to 32-bits, besides we have
".valid.max_access_size = 4" so the caller always pass 32-bits.

> +
> +    trace_stm32l4x5_exti_write(addr, value);
> +
> +    switch (addr) {
> +    case EXTI_IMR1:
> +        s->imr[0] = value;
> +        return;

You can unify banks:

        case EXTI_IMR1:
        case EXTI_IMR2:
            s->imr[bank] = value & valid_mask(bank);
            return;

> +    case EXTI_EMR1:
> +        s->emr[0] = value;
> +        return;

        case EXTI_EMR1:
        case EXTI_EMR2:
            s->emr[bank] = value & valid_mask(bank);
            return;

> +    case EXTI_RTSR1:
> +        s->rtsr[0] = value & ~DIRECT_LINE_MASK1;
> +        return;

        case EXTI_RTSR1:
        case EXTI_RTSR2:
            s->rtsr[bank] = value & ~exti_romask[bank];
            return;

> +    case EXTI_FTSR1:
> +        s->ftsr[0] = value & ~DIRECT_LINE_MASK1;
> +        return;

        case EXTI_FTSR1:
        case EXTI_FTSR2:
            s->ftsr[bank] = value & ~exti_romask[bank];
            return;

> +    case EXTI_SWIER1: {
> +        const uint32_t set1 = value & ~DIRECT_LINE_MASK1;
> +        const uint32_t pend1 = set1 & ~s->swier[0] & s->imr[0] & ~s->pr[0];
> +        s->swier[0] = set1;
> +        s->pr[0] |= pend1;
> +        for (int i = 0; i < 32; i++) {
> +            if (pend1 & (1 << i)) {
> +                qemu_irq_pulse(s->irq[i]);
> +            }
> +        }
> +        return;

      case EXTI_SWIER1:
      case EXTI_SWIER2:
           set = value & ~exti_romask[bank];
           pend = set & ~s->swier[bank] & s->imr[bank] & ~s->pr[bank];
           s->swier[bank] = set;
           s->pr[bank] |= pend;
           for (unsigned i = 0; i < irqs_per_bank[bank]; i++) {
               if (pend & (1 << i)) { // or extract32()
                   qemu_irq_pulse(s->irq[i]);
               }
           }
           return;

> +    }
> +    case EXTI_PR1: {
> +        const uint32_t cleared1 = s->pr[0] & value & ~DIRECT_LINE_MASK1;
> +        /* This bit is cleared by writing a 1 to it */
> +        s->pr[0] &= ~cleared1;
> +        /* Software triggered interrupts are cleared as well */
> +        s->swier[0] &= ~cleared1;
> +        return;

Ditto.

> +    }
> +    case EXTI_IMR2:
> +        s->imr[1] = value & ~RESERVED_BITS_MASK2;
> +        return;
> +    case EXTI_EMR2:
> +        s->emr[1] = value & ~RESERVED_BITS_MASK2;
> +        return;
> +    case EXTI_RTSR2:
> +        s->rtsr[1] = value & ACTIVABLE_MASK2;
> +        return;
> +    case EXTI_FTSR2:
> +        s->ftsr[1] = value & ACTIVABLE_MASK2;
> +        return;
> +    case EXTI_SWIER2: {
> +        const uint32_t set2 = value & ACTIVABLE_MASK2;
> +        const uint32_t pend2 = set2 & ~s->swier[1] & s->imr[1] & ~s->pr[1];
> +        s->swier[1] = set2;
> +        s->pr[1] |= pend2;
> +        for (int i = 0; i < 8; i++) {
> +            if (pend2 & (1 << i)) {
> +                qemu_irq_pulse(s->irq[32 + i]);
> +            }
> +        }
> +        return;
> +    }
> +    case EXTI_PR2: {
> +        const uint32_t cleared = s->pr[1] & value & ACTIVABLE_MASK2;
> +        /* This bit is cleared by writing a 1 to it */
> +        s->pr[1] &= ~cleared;
> +        /* Software triggered interrupts are cleared as well */
> +        s->swier[1] &= ~cleared;
> +        return;
> +    }
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "STM32L4X5_exti_write: Bad offset 0x%x\n", (int)addr);
> +    }
> +}
> +
> +static const MemoryRegionOps stm32l4x5_exti_ops = {
> +    .read = stm32l4x5_exti_read,
> +    .write = stm32l4x5_exti_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl.min_access_size = 4,
> +    .impl.max_access_size = 4,
> +    .impl.unaligned = false,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +    .valid.unaligned = false,
> +};

Code totally untested =)

Regards,

Phil.