[PATCH 4/4] STM32F4 GPIO: Add GPIO implementation for STM32F4

[Cedric Berger]

Hello,

Have you tried a tiny bit to optimize this code because at a quick 
glance, it sounds like for critical routines, you choose some slow and 
complicated ways to do simple things, see a few some example below:

> +static uint16_t GPIO_PIN_x[] = {
> +    GPIO_PIN_0,
> +    GPIO_PIN_1,
> +    GPIO_PIN_2,
> +    GPIO_PIN_3,
> +    GPIO_PIN_4,
> +    GPIO_PIN_5,
> +    GPIO_PIN_6,
> +    GPIO_PIN_7,
> +    GPIO_PIN_8,
> +    GPIO_PIN_9,
> +    GPIO_PIN_10,
> +    GPIO_PIN_11,
> +    GPIO_PIN_12,
> +    GPIO_PIN_13,
> +    GPIO_PIN_14,
> +    GPIO_PIN_15
> +};

[...]

> +
> +/**
> +  * @brief Converts pin number from 0-15 to HAL pin mask.
> +  * @param pin is the pin number from 0-15
> +  */
> +#define STM32F4_GET_HAL_GPIO_PIN(pin) (GPIO_PIN_x[( pin )])

So, one macro and one static array indirection to basically write "1<<pin"?

> +/**
> +  * @note Warning: only one pin can be passed as argument
> +  * @note If using interrupt mode, use rtems_gpio_configure_interrupt().
> +  * @note If using alternate mode, use rtems_gpio_configure().
> +  */
> +rtems_status_code stm32f4_gpio_set_pin_mode(
> +    rtems_gpio *base,
> +    rtems_gpio_pin_mode mode
> +)
> +{
[...]
> +    LL_GPIO_SetPinMode(gpio->port, pin_mask, stm32f4_mode);
> +    if (stm32f4_mode == LL_GPIO_MODE_OUTPUT) {
> +        LL_GPIO_SetPinOutputType(gpio->port, pin_mask, stm32f4_output_type);
> +    }
> +
> +    return RTEMS_SUCCESSFUL;
> +}

Here I see that you're using the new, low level, API. That's very good.

RTEMS should really only use the Low Level API everywhere possible.

> +rtems_status_code stm32f4_gpio_write(
> +    rtems_gpio *base,
> +    rtems_gpio_pin_state value
> +)
> +{
> +    stm32f4_gpio *gpio = get_gpio_from_base(base);
> +    uint32_t pin_mask = STM32F4_GET_HAL_GPIO_PIN(gpio->pin);
> +
> +    HAL_GPIO_WritePin(gpio->port, pin_mask, value);
> +    return RTEMS_SUCCESSFUL;
> +}

In particular here, this function should be fast, and you do 2 function 
calls plus one unneeded array indirection.

Why not simply something like (totally untested):

rtems_status_code stm32f4_gpio_write(
     rtems_gpio *base,
     rtems_gpio_pin_state value
)
{
     stm32f4_gpio *gpio = RTEMS_CONTAINER_OF(base, stm32f4_gpio, base);
     if (value)
         gpio->BSRR = 1 << gpio->pin;
         // or LL_GPIO_SetOutputPin(gpio, 1 << gpio->pin)
     else
         gpio->BSRR = 1 << (gpio->pin + 16);
         // or LL_GPIO_ResetOutputPin(gpio, 1 << gpio->pin)
}

As a general rule, I believe that RTEMS should stop using the HAL for 
all simple devices.

The HAL is fine for complicated IPs like Ethernet, USB, memory controller.

For simple IPs (serial, CAN, gpio, DMA, everything else) the HAL is just 
a huge fat pile of slow and unnecessary code.

ST was forced to develop the LL API after everyone complained about the HAL.

Thanks,

Cedric