Re: [PATCH qemu 1/1] Implement STM32L4x5 EXTI

[Philippe Mathieu-Daudé]

Hi Arnaud,

On 11/11/23 15:33, ~aminier wrote:
> From: Arnaud Minier <arnaud.minier@telecom-paris.fr>
>
> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>   hw/arm/Kconfig                    |   1 +
>   hw/arm/stm32l4x5_soc.c            |  65 +++++-
>   hw/misc/Kconfig                   |   3 +
>   hw/misc/meson.build               |   1 +
>   hw/misc/stm32l4x5_exti.c          | 329 ++++++++++++++++++++++++++++++
>   hw/misc/trace-events              |   5 +
>   include/hw/arm/stm32l4x5_soc.h    |   3 +
>   include/hw/misc/stm32l4x5_exti.h  |  64 ++++++
>   tests/qtest/meson.build           |   5 +
>   tests/qtest/stm32l4x5_exti-test.c | 102 +++++++++
>   10 files changed, 576 insertions(+), 2 deletions(-)
>   create mode 100644 hw/misc/stm32l4x5_exti.c
>   create mode 100644 include/hw/misc/stm32l4x5_exti.h
>   create mode 100644 tests/qtest/stm32l4x5_exti-test.c


> diff --git a/hw/arm/stm32l4x5_soc.c b/hw/arm/stm32l4x5_soc.c
> index 198d3f6d3e..6f2a1b34b3 100644
> --- a/hw/arm/stm32l4x5_soc.c
> +++ b/hw/arm/stm32l4x5_soc.c
> @@ -43,10 +43,51 @@
>   #define SRAM2_BASE_ADDRESS 0x10000000
>   #define SRAM2_SIZE (32 * KiB)
>   
> +static const hwaddr exti_addr = 0x40010400;

Why not a #define?

> +#define NUM_EXTI_IRQ 40


> diff --git a/hw/misc/stm32l4x5_exti.c b/hw/misc/stm32l4x5_exti.c


> +static void stm32l4x5_exti_set_irq(void *opaque, int irq, int level)
> +{
> +    Stm32l4x5ExtiState *s = opaque;
> +
> +    trace_stm32l4x5_exti_set_irq(irq, level);
> +
> +    if (irq >= NUM_INTERRUPT_OUT_LINES) {
> +        return;

This can not happen. If you are unsure, this would be a programming
error, thus aborting is better, but nothing needed IMO.

> +    }
> +
> +    if (irq < 32) {
> +        if (((1 << irq) & s->exti_rtsr1) && level) {
> +            /* Rising Edge */
> +            s->exti_pr1 |= 1 << irq;
> +        }
> +
> +        if (((1 << irq) & s->exti_ftsr1) && !level) {
> +            /* Falling Edge */
> +            s->exti_pr1 |= 1 << irq;
> +        }
> +
> +        if (!((1 << irq) & s->exti_imr1)) {
> +            /* Interrupt is masked */
> +            return;
> +        }
> +    } else {
> +        /* Shift the value to enable access in x2 registers*/
> +        int irq_x2 = irq - 32;
> +        if (((1 << irq_x2) & s->exti_rtsr2) && level) {
> +            /* Rising Edge */
> +            s->exti_pr2 |= 1 << irq_x2;
> +        }
> +
> +        if (((1 << irq_x2) & s->exti_ftsr2) && !level) {
> +            /* Falling Edge */
> +            s->exti_pr2 |= 1 << irq_x2;
> +        }
> +
> +        if (!((1 << irq_x2) & s->exti_imr2)) {
> +            /* Interrupt is masked */
> +            return;
> +        }
> +    }
> +    qemu_irq_pulse(s->irq[irq]);
> +}

Could be simpler avoiding duplication, as:

---
static void stm32l4x5_exti_set_irq(void *opaque, int irq, int level)
{
    Stm32l4x5ExtiState *s = opaque;
    int oirq = irq;
    uint32_t *rtsr;
    uint32_t *ftsr;
    uint32_t *pr;
    uint32_t *imr;

    trace_stm32l4x5_exti_set_irq(irq, level);

    if (irq < 32) {
        rtsr = &s->exti_rtsr1;
        ftsr = &s->exti_ftsr1;
        pr = &s->exti_pr1;
        imr = &s->exti_imr1;
    } else {
        rtsr = &s->exti_rtsr2;
        ftsr = &s->exti_ftsr2;
        pr = &s->exti_pr2;
        imr = &s->exti_imr2;
        /* Shift the value to enable access in x2 registers. */
        irq -= 32;
    }

    if (((1 << irq) & *rtsr) && level) {
        /* Rising Edge */
        *pr |= 1 << irq;
    }

    if (((1 << irq) & *ftsr) && !level) {
        /* Falling Edge */
        *pr |= 1 << irq;
    }

    if (!((1 << irq) & *imr)) {
        /* Interrupt is masked */
        return;
    }

    qemu_irq_pulse(s->irq[oirq]);
}
---

But changing Stm32l4x5ExtiState as:

---
struct Stm32l4x5ExtiState {
    SysBusDevice parent_obj;

    MemoryRegion mmio;

    uint32_t imr[2];
    uint32_t emr[2];
    uint32_t rtsr[2];
    uint32_t ftsr[2];
    uint32_t swier[2];
    uint32_t pr[2];

    qemu_irq irq[NUM_INTERRUPT_OUT_LINES];
};
---

We get even simpler:

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

    trace_stm32l4x5_exti_set_irq(irq, level);

    if (irq >= 32) {
        /* Shift the value to enable access in x2 registers. */
        irq -= 32;
    }

    if (((1 << irq) & s->rtsr[n]) && level) {
        /* Rising Edge */
        s->pr[n] |= 1 << irq;
    }

    if (((1 << irq) & s->ftsr[n]) && !level) {
        /* Falling Edge */
        s->pr[n] |= 1 << irq;
    }

    if (!((1 << irq) & s->imr[n])) {
        /* Interrupt is masked */
        return;
    }

    qemu_irq_pulse(s->irq[oirq]);
}
---

(code untested).

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

       unsigned n = addr >= EXTI_IMR2;
> +
> +    switch (addr) {
> +    case EXTI_IMR1:
> +        r = s->exti_imr1;
> +        break;

This becomes:

       case EXTI_IMR1:
       case EXTI_IMR2:
           r = s->exti_imr[n];
           break;
       ...

> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "STM32L4X5_exti_read: Bad offset %x\n", (int)addr);

Please use '0x' prefix for hexadecimal.

> +        break;
> +    }
> +
> +    trace_stm32l4x5_exti_read(addr, r);
> +
> +    return r;
> +}


> +static const MemoryRegionOps stm32l4x5_exti_ops = {
> +    .read = stm32l4x5_exti_read,
> +    .write = stm32l4x5_exti_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,

Your implementation is 32-bit wide (all your registers are),
so:

       .impl.min_access_size = 4,
       .impl.max_access_size = 4,

What are the allowed accesses? any 8/16/32/64 bits?
(This is what happens when .valid fields aren't set).

> +};


> +static void stm32l4x5_exti_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = stm32l4x5_exti_reset;

Better set a ResettableClass handler. DeviceClass::reset will soon
be deprecated.

> +    dc->vmsd = &vmstate_stm32l4x5_exti;
> +}


> +static void stm32l4x5_exti_register_types(void)
> +{
> +    type_register_static(&stm32l4x5_exti_info);
> +}

Preferably use the DEFINE_TYPES() macro.

> +type_init(stm32l4x5_exti_register_types)


> diff --git a/include/hw/misc/stm32l4x5_exti.h b/include/hw/misc/stm32l4x5_exti.h
> new file mode 100644
> index 0000000000..4305e7fcbb
> --- /dev/null
> +++ b/include/hw/misc/stm32l4x5_exti.h
> @@ -0,0 +1,64 @@
> +/*
> + * STM32L4x5 SoC family EXTI


"STM32L4x5 EXTI (Extended interrupts and events controller)"


> +#define NUM_GPIO_EVENT_IN_LINES 16

Since not used externally, NUM_GPIO_EVENT_IN_LINES could be
restricted to the source.

> +#define NUM_INTERRUPT_OUT_LINES 40
> +
> +struct Stm32l4x5ExtiState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion mmio;
> +
> +    uint32_t exti_imr1;
> +    uint32_t exti_emr1;
> +    uint32_t exti_rtsr1;
> +    uint32_t exti_ftsr1;
> +    uint32_t exti_swier1;
> +    uint32_t exti_pr1;
> +    uint32_t exti_imr2;
> +    uint32_t exti_emr2;
> +    uint32_t exti_rtsr2;
> +    uint32_t exti_ftsr2;
> +    uint32_t exti_swier2;
> +    uint32_t exti_pr2;

See previous suggestion for Stm32l4x5ExtiState. Besides, no
need to use the cumbersome 'exti_' prefix.

> +    qemu_irq irq[NUM_INTERRUPT_OUT_LINES];
> +};
> +
> +#endif

Good patch quality, looking forward for v2!

Regards,

Phil.