Re: [PATCH v4 1/3] hw/gpio: Implement STM32L4x5 GPIO

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

Hi Inès,

On 7/2/24 14:23, Inès Varhol wrote:
> Features supported :
> - the 8 STM32L4x5 GPIOs are initialized with their reset values
>      (except IDR, see below)
> - input mode : setting a pin in input mode "externally" (using input
>      irqs) results in an out irq (transmitted to SYSCFG)
> - output mode : setting a bit in ODR sets the corresponding out irq
>      (if this line is configured in output mode)
> - pull-up, pull-down
> - push-pull, open-drain
> 
> Difference with the real GPIOs :
> - Alternate Function and Analog mode aren't implemented :
>      pins in AF/Analog behave like pins in input mode
> - floating pins stay at their last value
> - register IDR reset values differ from the real one :
>      values are coherent with the other registers reset values
>      and the fact that AF/Analog modes aren't implemented
> - setting I/O output speed isn't supported
> - locking port bits isn't supported
> - ADC function isn't supported
> - GPIOH has 16 pins instead of 2 pins
> - writing to registers LCKR, AFRL, AFRH and ASCR is ineffective
> 
> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>   MAINTAINERS                        |   1 +
>   docs/system/arm/b-l475e-iot01a.rst |   2 +-
>   include/hw/gpio/stm32l4x5_gpio.h   |  70 +++++
>   hw/gpio/stm32l4x5_gpio.c           | 456 +++++++++++++++++++++++++++++
>   hw/gpio/Kconfig                    |   3 +
>   hw/gpio/meson.build                |   1 +
>   hw/gpio/trace-events               |   6 +
>   7 files changed, 538 insertions(+), 1 deletion(-)
>   create mode 100644 include/hw/gpio/stm32l4x5_gpio.h
>   create mode 100644 hw/gpio/stm32l4x5_gpio.c


> +static void update_gpio_idr(Stm32l4x5GpioState *s)
> +{
> +    uint32_t new_idr_mask = 0;
> +    uint32_t new_idr = s->odr;
> +
> +    for (int i = 0; i < GPIO_NUM_PINS; i++) {
> +        /* output mode */
> +        if (extract32(s->moder, 2 * i, 2) == 1) {
> +            if (extract32(s->otyper, i, 1) == 0) {
> +                /* push-pull */
> +                new_idr_mask |= (1 << i);
> +            } else if (!(s->odr & (1 << i))) {
> +                /* open-drain ODR 0 */
> +                new_idr_mask |= (1 << i);
> +            } else if ((s->disconnected_pins & (1 << i)) &&
> +                       (extract32(s->pupdr, 2 * i, 2) == 1)) {
> +                /* open-drain pull-up ODR 1 with disconnected pin */
> +                new_idr_mask |= (1 << i);
> +            } else if ((s->disconnected_pins & (1 << i)) &&
> +                       (extract32(s->pupdr, 2 * i, 2) == 2)) {
> +                /* open-drain pull-down ODR 1 with disconnected pin */
> +                new_idr_mask |= (1 << i);
> +                new_idr &= ~(1 << i);
> +            } else if (!(s->pins_connected_high & (1 << i))) {
> +                /* open-drain ODR 1 with pin connected low */
> +                new_idr_mask |= (1 << i);
> +                new_idr &= ~(1 << i);
> +            }
> +            /*
> +             * The only case left is for open-drain ODR 1
> +             * with disconnected pin without pull-up or pull-down :
> +             * the value is floating.
> +             */
> +        /* input or analog mode with connected pin */
> +        } else if (!(s->disconnected_pins & (1 << i))) {
> +            if (s->pins_connected_high & (1 << i)) {
> +                /* pin high */
> +                new_idr_mask |= (1 << i);
> +                new_idr |= (1 << i);
> +            } else {
> +                /* pin low */
> +                new_idr_mask |= (1 << i);
> +                new_idr &= ~(1 << i);
> +            }
> +        /* input or analog mode with disconnected pin */
> +        } else {
> +            if (extract32(s->pupdr, 2 * i, 2) == 1) {
> +                /* pull-up */
> +                new_idr_mask |= (1 << i);
> +                new_idr |= (1 << i);
> +            } else if (extract32(s->pupdr, 2 * i, 2) == 2) {
> +                /* pull-down */

You can extract for clarity:

   static bool is_pull_up(Stm32l4x5GpioState *s, unsigned pin)
   {
       return extract32(s->pupdr, 2 * pin, 2) == 1;
   }

   static bool is_pull_down(Stm32l4x5GpioState *s, unsigned pin)
   {
       return extract32(s->pupdr, 2 * pin, 2) == 2;
   }

> +                new_idr_mask |= (1 << i);
> +                new_idr &= ~(1 << i);
> +            }
> +            /*
> +             * The only case left is for a disconnected pin
> +             * without pull-up or pull-down :
> +             * the value is floating.
> +             */
> +        }
> +    }
> +
> +    uint32_t old_idr = s->idr;

Please declare new variable at the beginning of the function.
(https://www.qemu.org/docs/master/devel/style.html#declarations)

> +    s->idr = (old_idr & ~new_idr_mask) | (new_idr & new_idr_mask);
> +    trace_stm32l4x5_gpio_update_idr(s->name, old_idr, s->idr);
> +
> +    for (int i = 0; i < GPIO_NUM_PINS; i++) {
> +        if (new_idr_mask & (1 << i)) {

Maybe evaluate (new_idr & (1 << i)) and (old_idr & (1 << i)) once
in local variables?

> +            if ((new_idr & (1 << i)) > (old_idr & (1 << i))) {
> +                qemu_irq_raise(s->pin[i]);
> +            } else if ((new_idr & (1 << i)) < (old_idr & (1 << i))) {
> +                qemu_irq_lower(s->pin[i]);
> +            }
> +        }
> +    }
> +}
> +
> +/*
> + * Return

"mask of"

> pins both configured in output mode
> + * and externally driven (except pins in open-drain
> + * mode externally set to 0).
> + */
> +static uint32_t get_gpio_pins_to_disconnect(Stm32l4x5GpioState *s)

Maybe get_gpio_pinmask_to_disconnect() is clearer.

> +{
> +    uint32_t pins_to_disconnect = 0;

> +    for (int i = 0; i < GPIO_NUM_PINS; i++) {
> +        /* for each connected pin in output mode */
> +        if ((~s->disconnected_pins & (1 << i)) &&
> +            (extract32(s->moder, 2 * i, 2) == 1)) {
> +            /* if either push-pull or high level */
> +            if ((extract32(s->otyper, i, 1) == 0) ||
> +                (extract16(s->pins_connected_high, i, 1)) == 1) { > +                pins_to_disconnect |= (1 << i);
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "Line %d can't be driven externally\n",
> +                              i);
> +            }
> +        }
> +    }
> +    return pins_to_disconnect;
> +}


> +static void stm32l4x5_gpio_init(Object *obj)
> +{
> +    Stm32l4x5GpioState *s = STM32L4X5_GPIO(obj);
> +
> +    memory_region_init_io(&s->mmio, obj, &stm32l4x5_gpio_ops, s,
> +                          TYPE_STM32L4X5_GPIO, 0x400);
> +
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->pin, GPIO_NUM_PINS);
> +    qdev_init_gpio_in(DEVICE(obj), stm32l4x5_gpio_set, GPIO_NUM_PINS);
> +
> +    s->clk = qdev_init_clock_in(DEVICE(s), "clk", NULL, s, 0);
> +
> +    object_property_add(obj, "disconnected-pins", "uint16",
> +        disconnected_pins_get, disconnected_pins_set, NULL,
> +        &s->disconnected_pins);

(alignment is off)

> +    object_property_add(obj, "clock-freq-hz", "uint32",
> +        clock_freq_get, NULL, NULL, NULL);

(Ditto)

> +}
> +
> +static void stm32l4x5_gpio_realize(DeviceState *dev, Error **errp)
> +{
> +    Stm32l4x5GpioState *s = STM32L4X5_GPIO(dev);

> +    if (!clock_has_source(s->clk)) {
> +        error_setg(errp, "GPIO: clk input must be connected");
> +        return;
> +    }
> +}

Mostly style comments, otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>