Re: [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<
+/**
+  * @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




___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

[Duc Doan]

---
 bsps/arm/stm32f4/gpio/gpio.c  | 595 ++
 bsps/arm/stm32f4/include/bsp.h|   4 -
 bsps/arm/stm32f4/include/bsp/stm32f4_gpio.h   |  37 ++
 bsps/arm/stm32f4/include/bsp/stm32f4_hal.h|  17 +
 bsps/arm/stm32f4/start/bspstart.c |  11 +-
 spec/build/bsps/arm/stm32f4/grp.yml   |   4 +-
 .../build/bsps/arm/stm32f4/optnumgpioctrl.yml |  16 +
 7 files changed, 674 insertions(+), 10 deletions(-)
 create mode 100644 bsps/arm/stm32f4/gpio/gpio.c
 create mode 100644 bsps/arm/stm32f4/include/bsp/stm32f4_gpio.h
 create mode 100644 bsps/arm/stm32f4/include/bsp/stm32f4_hal.h
 create mode 100644 spec/build/bsps/arm/stm32f4/optnumgpioctrl.yml

diff --git a/bsps/arm/stm32f4/gpio/gpio.c b/bsps/arm/stm32f4/gpio/gpio.c
new file mode 100644
index 00..e971f91140
--- /dev/null
+++ b/bsps/arm/stm32f4/gpio/gpio.c
@@ -0,0 +1,595 @@
+/**
+  * @file
+  *
+  * @ingroup rtems_bsp/arm/stm32f4
+  *
+  * @brief RTEMS GPIO new API implementation for STM32F4.
+  *
+  * @note RTEMS_GPIO_PINMODE_BSP_SPECIFIC is Alternate mode for STM32F4 BSP
+  */
+
+/*
+ *  Copyright (c) 2022 Duc Doan 
+ *
+ *  The license and distribution terms for this file may be
+ *  found in the file LICENSE in this distribution or at
+ *  http://www.rtems.org/license/LICENSE.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+/*** Helpers */
+static stm32f4_gpio *get_gpio_from_base(
+rtems_gpio *base
+);
+
+/*** GPIO API ***/
+static rtems_status_code stm32f4_gpio_get(
+uint32_t interm_pin,
+rtems_gpio **out
+);
+
+static rtems_status_code stm32f4_gpio_destroy(
+rtems_gpio *base
+);
+
+static rtems_status_code stm32f4_gpio_init(
+rtems_gpio *base
+);
+
+static rtems_status_code stm32f4_gpio_deinit(
+rtems_gpio *base
+);
+
+static rtems_status_code stm32f4_gpio_set_pin_mode(
+rtems_gpio *base,
+rtems_gpio_pin_mode mode
+);
+
+static rtems_status_code stm32f4_gpio_set_pull(
+rtems_gpio *base,
+rtems_gpio_pull pull
+);
+
+static rtems_status_code stm32f4_gpio_configure_interrupt(
+rtems_gpio *base, 
+rtems_gpio_isr isr,
+void *arg,
+rtems_gpio_interrupt_trig trig,
+rtems_gpio_pull pull
+);
+
+static rtems_status_code stm32f4_gpio_remove_interrupt(
+rtems_gpio *base
+);
+
+static rtems_status_code stm32f4_gpio_enable_interrupt(
+rtems_gpio *base
+);
+
+static rtems_status_code stm32f4_gpio_disable_interrupt(
+rtems_gpio *base
+);
+
+static rtems_status_code stm32f4_gpio_read(
+rtems_gpio *base,
+rtems_gpio_pin_state *value
+);
+
+static rtems_status_code stm32f4_gpio_write(
+rtems_gpio *base,
+rtems_gpio_pin_state value
+);
+
+static rtems_status_code stm32f4_gpio_toggle(
+rtems_gpio *base
+);
+
+/*/
+
+/**
+  * @brief STM32F4 GPIO handlers
+  */
+static const rtems_gpio_handlers stm32f4_gpio_handlers = {
+.init = stm32f4_gpio_init,
+.deinit = stm32f4_gpio_deinit,
+.set_pin_mode = stm32f4_gpio_set_pin_mode,
+.set_pull = stm32f4_gpio_set_pull,
+.configure_interrupt = stm32f4_gpio_configure_interrupt,
+.remove_interrupt = stm32f4_gpio_remove_interrupt,
+.enable_interrupt = stm32f4_gpio_enable_interrupt,
+.disable_interrupt = stm32f4_gpio_disable_interrupt,
+.read = stm32f4_gpio_read,
+.write = stm32f4_gpio_write,
+.toggle = stm32f4_gpio_toggle
+};
+
+static GPIO_TypeDef *GPIOx[] = {
+GPIOA, GPIOB, GPIOC, GPIOD, GPIOE,
+GPIOF, GPIOG, GPIOH, GPIOI,
+#ifdef STM32F429X
+GPIOJ, GPIOK
+#endif /* STM32F429X */
+};
+
+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
+};
+
+static uint32_t LL_EXTI_LINE_x[] = {
+LL_EXTI_LINE_0,
+LL_EXTI_LINE_1,
+LL_EXTI_LINE_2,
+LL_EXTI_LINE_3,
+LL_EXTI_LINE_4,
+LL_EXTI_LINE_5,
+LL_EXTI_LINE_6,
+LL_EXTI_LINE_7,
+LL_EXTI_LINE_8,
+LL_EXTI_LINE_9,
+LL_EXTI_LINE_10,
+LL_EXTI_LINE_11,
+LL_EXTI_LINE_12,
+LL_EXTI_LINE_13,
+LL_EXTI_LINE_14,
+LL_EXTI_LINE_15
+};
+
+static unsigned int EXTIx_IRQn[] = {
+EXTI0_IRQn,
+EXTI1_IRQn,
+EXTI2_IRQn,
+EXTI3_IRQn,
+EXTI4_IRQn,
+EXTI9_5_IRQn,
+EXTI9_5_IRQn,
+EXTI9_5_IRQn,
+EXTI9_5_IRQn,
+EXTI9_5_IRQn,
+EXTI15_10_IRQn,
+EXTI15_10_IRQn,
+EXTI15_10_IRQn,
+EXTI15_10_IRQn,
+EXTI15_10_IRQn,
+EXTI15_10_IRQn
+};
+
+/**
+  * @brief Converts intermediate pin number to port pointer.
+  *
+  * Intermediate pin number is a way of numerically labeling
+  * pins. Pins are labeled incrementally across all ports.
+  * Pins 0-15 from port A are 0-15. Pins 0-15 from port B are
+  * 16-31. And so on.
+  *
+  * @param interm_pin is the