Date
1 - 4 of 4
[RFC] STM32 pinmux - simplify pinmux logic
|
Christer Weinigel
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
|
|
|
|
Erwan Gouriou
Hi Christer,
toggle quoted messageShow quoted text
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,
|
|
|
|
Christer Weinigel
Hi Erwan,
On 2017-01-17 13:38, Erwan Gouriou wrote: I agree with the observation that gpio/pinmux configuration isIt looks quite similar, if I understood this correctly, he's also added some macros to combine the pull up/pull down/function with the alternate function in one word: https://gerrit.zephyrproject.org/r/#/c/9325/19/drivers/pinmux/stm32/pinmux_stm32.h Although I think it's nicer to do what I did and put the pin information in drivers/pinmux/stm32/pinmux_stm32f4.h since it puts everything in one file. Other than that it looks like a nice patchset to add support for the STM32F3xx family. After thinking a bit more about this, I'm actually not too fond of magic macros like my STM32F4_PINMUX_ENCODE. Sometimes abstractions can be nice, but in this case I actually thinks that it obscures more than it helps. I would actually prefer to change the defines to have the a shift built in, that is change the stm32f4x_pin_config_mode enum to defines: #define STM32F4X_PIN_CONFIG_AF_PUSH_MASK (15 << 8) #define STM32F4X_PIN_CONFIG_AF_PUSH_PULL (0 << 8) #define STM32F4X_PIN_CONFIG_AF_PUSH_UP (1 << 8) #define STM32F4X_PIN_CONFIG_AF_PUSH_DOWN (2 << 8) ... STM32F4X_PIN_CONFIG_ANALOG (15 << 8) so that the defines in pinmux_stm32.h can be a bit more explicit not have to use the STM32F4_PINMUX_ENCODE macro: #define STM32F4_PINMUX_FUNC_PA9_USART1_TX (STM32F4X_PIN_CONFIG_AF_PUSH_UP | STM32_PINMUX_FUNC_ALT_7) this also allows more flexibility, we're no longer limited to just two axes of parameters. For example AF_PUSH_UP actually combines three different settings. We can split that into MODE_AF, PULL_UP and OTYPE_PUSH_PULL: #define STM32F4X_PIN_MODE_MASK (3 << 8) #define STM32F4X_PIN_MODE_INPUT (0 << 8) #define STM32F4X_PIN_MODE_OUTPUT (1 << 8) #define STM32F4X_PIN_MODE_AF (2 << 8) #define STM32F4X_PIN_MODE_ANALOG (3 << 8) #define STM32F4X_PIN_PULL_MASK (3 << 10) #define STM32F4X_PIN_PULL_NONE (0 << 10) #define STM32F4X_PIN_PULL_UP (1 << 10) #define STM32F4X_PIN_PULL_DOWN (2 << 10) #define STM32F4X_PIN_OTYPE_MASK (1 << 12) #define STM32F4X_PIN_OTYPE_PUSH_PULL (0 << 12) #define STM32F4X_PIN_OTYPE_OPEN_DRAIN (1 << 12) and for completeness we could also expose the speed settings: #define STM32F4X_PIN_SPEED_MASK (3 << 13) #define STM32F4X_PIN_SPEED_LOW (0 << 13) #define STM32F4X_PIN_SPEED_MEDIUM (1 << 13) #define STM32F4X_PIN_SPEED_FAST (2 << 13) #define STM32F4X_PIN_SPEED_HIGH (3 << 13) The above pin definition would look like this: #define STM32F4_PINMUX_FUNC_PA9_USART1_TX (STM32F4X_PIN_FUNC_AF | STM32F4X_PIN_PULL_UP | STM32_PINMUX_FUNC_ALT_7) It's a bit more verbose, but it's obvious what it does, especially if you look at the corresponding definitions in the data sheet since this is how the hardware registers look. If we do this we can rip out stm32_get_pin_config completely and also simplify the __func_to_foo functions in soc_gpio.c It might also be a good idea to unify these defines between STM32 families. If I recall correctly many STM32F1xx, STM32F2xx and STMF4xx packages are mostly pin compatible. The same PCB could have a STM32Fxx or STM32F4xx MCU mounted on it (with a zero ohm resistor or capacitor to account for any differences between them) and it would be nice if they could use the same board file. Some functions might not be supported on all families, for example, I don't think the STM32F1xx can have a pull-up/pull-down at the same tie as a pin is an output or alternate function, but that shouldn't be problem. Btw, I think that you could submit your RFC as a patch in Gerrit, so we canI'll see if I can clean up my ideas and push a few patches for review in gerrit. /Christer
|
|
|
|
Erwan Gouriou
Hi Christer,
The above pin definition would look like this: I agree this might be clearer for users. Since pinmux is something users might have to play with, the easier to read, the better. I can only agree here. It will help a lot on maintenance and porting of new families. There could be unexpected differences between quite similar boards, but these could be handled easily.
/Christer
|
|
|