Re: [RFC] STM32 pinmux - simplify pinmux logic


Erwan Gouriou
 

Hi Christer,


I agree with the observation that gpio/pinmux configuration is
overcomplicated.
Your proposal is interesting and look rather easy to use.
Maybe you can also have a look to Adam's porting of STM32F3x family, in
which
he's trying to by pass pin configuration this in another way:
https://gerrit.zephyrproject.org/r/#/c/9701
I'd be glad to get your feedback

Btw, I think that you could submit your RFC as a patch in Gerrit, so we can
better appreciate the differences and comment easily.

Best
Erwan

On 15 January 2017 at 20:02, Christer Weinigel <christer(a)weinigel.se> wrote:

Hi all,

BTW, for RFC patches like this that break the build, should
I submit them to Gerrit instead or do you prefer to see them
on the list?

I've just managed to port Zephyr to my STM32F405 based board.
One thing that I stumbled on and took me a few hours to figure
out is that I'm using UART2 on PD5/PD6 as my serial port and
for some reason I did not get any UART output. It turns out
that in addition to adding a few lines to pinmux_stm32f4.h
defining the UART2 alternate functions on PD5/PD6 I also had
to add mappings for PD5/PD6 to soc_pinmux.c. Without the
second change everything compiles nicely but the alternate
functions won't be activated anyway.

After thinking about this a bit I feel that the indirection
through stm32_get_pin_config in soc_pinmux.c is a bit, well,
overcomplicated. It also limits what I can do from my board
file, what if I acutally want to have a UART where RX is
pulled down (I want to see continuous breaks if nothing is
connected to the port)? Right now I can't do that without
modifying the Zephyr core.

Can't we just simplify this so that we encode the conf part
directly in the table? This allows us to remove all the
code in soc_pinmux.c and this way it's kind of obvious that
if you add a line in pinmux_stm32f4.h you have to specify
both the conf part and the altf part.

This is only a RFC, the patch as is breaks the stm32f1 and
stm32l4 ports. I can fix those up if you think this way of
doing things is a good idea.

/Christer

---
arch/arm/soc/st_stm32/stm32f4/Makefile | 1 -
arch/arm/soc/st_stm32/stm32f4/soc_pinmux.c | 112
-----------------------------
drivers/pinmux/stm32/pinmux_stm32.c | 9 +--
drivers/pinmux/stm32/pinmux_stm32f4.h | 24 ++++---
4 files changed, 20 insertions(+), 126 deletions(-)
delete mode 100644 arch/arm/soc/st_stm32/stm32f4/soc_pinmux.c

diff --git a/arch/arm/soc/st_stm32/stm32f4/Makefile
b/arch/arm/soc/st_stm32/stm32f4/Makefile
index 6ae6e19..1ef7b4a 100644
--- a/arch/arm/soc/st_stm32/stm32f4/Makefile
+++ b/arch/arm/soc/st_stm32/stm32f4/Makefile
@@ -1,7 +1,6 @@
obj-y += soc.o

obj-$(CONFIG_GPIO) += soc_gpio.o
-obj-$(CONFIG_PINMUX) += soc_pinmux.o

zephyr: $(KERNEL_HEX_NAME)
all: $(KERNEL_HEX_NAME)
diff --git a/arch/arm/soc/st_stm32/stm32f4/soc_pinmux.c
b/arch/arm/soc/st_stm32/stm32f4/soc_pinmux.c
deleted file mode 100644
index ef79ef4..0000000
--- a/arch/arm/soc/st_stm32/stm32f4/soc_pinmux.c
+++ /dev/null
@@ -1,112 +0,0 @@
-/*
- * Copyright (c) 2016 Linaro Limited.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#include <errno.h>
-
-#include "soc.h"
-#include <device.h>
-#include <misc/util.h>
-#include <pinmux/stm32/pinmux_stm32.h>
-#include <drivers/clock_control/stm32_clock_control.h>
-
-static const stm32_pin_func_t pin_pa9_funcs[] = {
- [STM32F4_PINMUX_FUNC_PA9_USART1_TX - 1] =
- STM32F4X_PIN_CONFIG_AF_PUSH_UP,
-};
-
-static const stm32_pin_func_t pin_pa10_funcs[] = {
- [STM32F4_PINMUX_FUNC_PA10_USART1_RX - 1] =
- STM32F4X_PIN_CONFIG_AF_PUSH_UP,
-};
-
-static const stm32_pin_func_t pin_pb6_funcs[] = {
- [STM32F4_PINMUX_FUNC_PB6_USART1_TX - 1] =
- STM32F4X_PIN_CONFIG_AF_PUSH_UP,
-};
-
-static const stm32_pin_func_t pin_pb7_funcs[] = {
- [STM32F4_PINMUX_FUNC_PB7_USART1_RX - 1] =
- STM32F4X_PIN_CONFIG_AF_PUSH_UP,
-};
-
-static const stm32_pin_func_t pin_pa2_funcs[] = {
- [STM32F4_PINMUX_FUNC_PA2_USART2_TX - 1] =
- STM32F4X_PIN_CONFIG_AF_PUSH_UP,
-};
-
-static const stm32_pin_func_t pin_pa3_funcs[] = {
- [STM32F4_PINMUX_FUNC_PA3_USART2_RX - 1] =
- STM32F4X_PIN_CONFIG_AF_PUSH_UP,
-};
-
-static const stm32_pin_func_t pin_pa0_funcs[] = {
- [STM32F4_PINMUX_FUNC_PA0_PWM2_CH1 - 1] =
- STM32F4X_PIN_CONFIG_AF_PUSH_UP,
-};
-
-static const stm32_pin_func_t pin_pd5_funcs[] = {
- [STM32F4_PINMUX_FUNC_PD5_USART2_TX - 1] =
- STM32F4X_PIN_CONFIG_AF_PUSH_UP,
-};
-
-static const stm32_pin_func_t pin_pd6_funcs[] = {
- [STM32F4_PINMUX_FUNC_PD6_USART2_RX - 1] =
- STM32F4X_PIN_CONFIG_AF_PUSH_UP,
-};
-
-/**
- * @brief pin configuration
- */
-static const struct stm32_pinmux_conf pins[] = {
- STM32_PIN_CONF(STM32_PIN_PA9, pin_pa9_funcs),
- STM32_PIN_CONF(STM32_PIN_PA10, pin_pa10_funcs),
- STM32_PIN_CONF(STM32_PIN_PB6, pin_pb6_funcs),
- STM32_PIN_CONF(STM32_PIN_PB7, pin_pb7_funcs),
- STM32_PIN_CONF(STM32_PIN_PA2, pin_pa2_funcs),
- STM32_PIN_CONF(STM32_PIN_PA3, pin_pa3_funcs),
- STM32_PIN_CONF(STM32_PIN_PA0, pin_pa0_funcs),
- STM32_PIN_CONF(STM32_PIN_PD5, pin_pa2_funcs),
- STM32_PIN_CONF(STM32_PIN_PD6, pin_pa3_funcs),
-};
-
-int stm32_get_pin_config(int pin, int func)
-{
- /* GPIO function is always available, to save space it is not
- * listed in alternate functions array
- */
- if (func == STM32_PINMUX_FUNC_GPIO) {
- return STM32F4X_PIN_CONFIG_BIAS_HIGH_IMPEDANCE;
- }
-
- /* analog function is another 'known' setting */
- if (func == STM32_PINMUX_FUNC_ANALOG) {
- return STM32F4X_PIN_CONFIG_ANALOG;
- }
-
- func -= 1;
-
- for (int i = 0; i < ARRAY_SIZE(pins); i++) {
- if (pins[i].pin == pin) {
- if (func > pins[i].nfuncs) {
- return -EINVAL;
- }
-
- return pins[i].funcs[func];
- }
- }
-
- return -EINVAL;
-}
diff --git a/drivers/pinmux/stm32/pinmux_stm32.c
b/drivers/pinmux/stm32/pinmux_stm32.c
index dfc13b4..9a9d94a 100644
--- a/drivers/pinmux/stm32/pinmux_stm32.c
+++ b/drivers/pinmux/stm32/pinmux_stm32.c
@@ -102,17 +102,18 @@ static int stm32_pin_configure(int pin, int func,
int altf)
int _pinmux_stm32_set(uint32_t pin, uint32_t func,
struct device *clk)
{
- int config;
+ int conf;
+ int altf;

/* make sure to enable port clock first */
if (enable_port(STM32_PORT(pin), clk)) {
return -EIO;
}

- /* determine config for alternate function */
- config = stm32_get_pin_config(pin, func);
+ conf = STM32F4_PINMUX_CONF(func);
+ altf = STM32F4_PINMUX_ALTF(func);

- return stm32_pin_configure(pin, config, func);
+ return stm32_pin_configure(pin, conf, altf);
}

/**
diff --git a/drivers/pinmux/stm32/pinmux_stm32f4.h
b/drivers/pinmux/stm32/pinmux_stm32f4.h
index 8ec7f25..79c97d6 100644
--- a/drivers/pinmux/stm32/pinmux_stm32f4.h
+++ b/drivers/pinmux/stm32/pinmux_stm32f4.h
@@ -21,18 +21,24 @@
* @file Header for STM32F4 pin multiplexing helper
*/

-#define STM32F4_PINMUX_FUNC_PA9_USART1_TX STM32_PINMUX_FUNC_ALT_7
-#define STM32F4_PINMUX_FUNC_PA10_USART1_RX STM32_PINMUX_FUNC_ALT_7
+#include "soc.h"

-#define STM32F4_PINMUX_FUNC_PB6_USART1_TX STM32_PINMUX_FUNC_ALT_7
-#define STM32F4_PINMUX_FUNC_PB7_USART1_RX STM32_PINMUX_FUNC_ALT_7
+#define STM32F4_PINMUX_ENCODE(conf, altf) (((conf)<<8) | (altf))
+#define STM32F4_PINMUX_CONF(func) ((func) >> 8)
+#define STM32F4_PINMUX_ALTF(func) ((func) & 0xff)

-#define STM32F4_PINMUX_FUNC_PA2_USART2_TX STM32_PINMUX_FUNC_ALT_7
-#define STM32F4_PINMUX_FUNC_PA3_USART2_RX STM32_PINMUX_FUNC_ALT_7
+#define STM32F4_PINMUX_FUNC_PA9_USART1_TX STM32F4_PINMUX_ENCODE(
STM32F4X_PIN_CONFIG_AF_PUSH_UP, STM32_PINMUX_FUNC_ALT_7)
+#define STM32F4_PINMUX_FUNC_PA10_USART1_RX STM32F4_PINMUX_ENCODE(
STM32F4X_PIN_CONFIG_AF_PUSH_UP, STM32_PINMUX_FUNC_ALT_7

-#define STM32F4_PINMUX_FUNC_PD5_USART2_TX STM32_PINMUX_FUNC_ALT_7
-#define STM32F4_PINMUX_FUNC_PD6_USART2_RX STM32_PINMUX_FUNC_ALT_7
+#define STM32F4_PINMUX_FUNC_PB6_USART1_TX STM32F4_PINMUX_ENCODE(
STM32F4X_PIN_CONFIG_AF_PUSH_UP, STM32_PINMUX_FUNC_ALT_7)
+#define STM32F4_PINMUX_FUNC_PB7_USART1_RX STM32F4_PINMUX_ENCODE(
STM32F4X_PIN_CONFIG_AF_PUSH_UP, STM32_PINMUX_FUNC_ALT_7)

-#define STM32F4_PINMUX_FUNC_PA0_PWM2_CH1 STM32_PINMUX_FUNC_ALT_1
+#define STM32F4_PINMUX_FUNC_PA2_USART2_TX STM32F4_PINMUX_ENCODE(
STM32F4X_PIN_CONFIG_AF_PUSH_UP, STM32_PINMUX_FUNC_ALT_7)
+#define STM32F4_PINMUX_FUNC_PA3_USART2_RX STM32F4_PINMUX_ENCODE(
STM32F4X_PIN_CONFIG_AF_PUSH_UP, STM32_PINMUX_FUNC_ALT_7)
+
+#define STM32F4_PINMUX_FUNC_PD5_USART2_TX STM32F4_PINMUX_ENCODE(
STM32F4X_PIN_CONFIG_AF_PUSH_UP, STM32_PINMUX_FUNC_ALT_7)
+#define STM32F4_PINMUX_FUNC_PD6_USART2_RX STM32F4_PINMUX_ENCODE(
STM32F4X_PIN_CONFIG_AF_PUSH_UP, STM32_PINMUX_FUNC_ALT_7)
+
+#define STM32F4_PINMUX_FUNC_PA0_PWM2_CH1 STM32F4_PINMUX_ENCODE(
STM32F4X_PIN_CONFIG_AF_PUSH_UP, STM32_PINMUX_FUNC_ALT_1)

#endif /* _STM32F4_PINMUX_H_ */
--
1.9.1

Join devel@lists.zephyrproject.org to automatically receive all group messages.