Skip to content

dts: stm32: Introduce linux like pinctrl DT bindings #23592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

dts: stm32: Introduce linux like pinctrl DT bindings #23592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 62 additions & 1 deletion boards/arm/disco_l475_iot1/disco_l475_iot1.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
/dts-v1/;
#include <st/l4/stm32l475Xg.dtsi>
#include "arduino_r3_connector.dtsi"

#include <dt-bindings/pinctrl/stm32l4-pins.h>

/ {
model = "STMicroelectronics B-L475E-IOT01Ax board";
Expand Down Expand Up @@ -49,13 +49,74 @@
};
};

&pinctrl {
Copy link
Member

@erwango erwango Mar 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial idea on defining pin configurations on SoC side, was to factorize configurations (minimize code reviews) and provide validated helpers to board users. This does not prevent to add or amend configurations on board side.

Can you elaborate on the reasons that made you prefer defining configurations on board side ?

A consequence of this choice of defining configuration on SoC side, is what I think is the main difference vs Linux version:
Linux defines pins combinations on SoC side (eg: uart4_pins_a defines rx/tx), while I chose to define single pin configurations. Aim was to avoid defining the whole combinatorial. This is similar to what you've done below.

Copy link
Member Author

@mnkp mnkp Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on the reasons that made you prefer defining configurations on board side ?

Actually, I was just trying to show the differences between the Linux implementation and the one in #20386 to make it easier to analyse and discuss them.

In my opinion the most important one is the following:

  • Linux DTS only defines pinmux nodes that are used. Approach proposed in dts: stm32: Introduce linux like pinctrl bindings #20386 would require us to embed a full database of possible pin configurations in DTS. E.g. if pin PA3 can be connected to GPIOA, UART2 RX, UART4 TX, SPI1 MISO or I2C4 SCL we would need to define all these nodes. And it had to be done for all the pins.

Analyzing / processing of DTS data would become difficult for humans and could negatively impact compilation time. Also, to build all the DTS nodes we would need to use scripts, doing this by hand is not really feasible.

Following the Linux approach seems more efficient.

Using the following template we would give user full flexibility to configure the pin without the need to provide any kind of database of valid configurations.

usart2_pins_default: pinmux_usart2_pins {
	pinmux = <STM32_PINMUX('A', 2, AF7)>, <STM32_PINMUX('A', 3, AF7)>;
	bias-disable;
};

the obvious disadvantage of this style is the lack of validation. It would be easy to make a mistake when typing in the data. The other template doesn't have this problem

usart2_rx_pins_default: pinmux_usart2_rx_pins {
	pinmux = <PIN_PA3_UART2_RX>;
	bias-disable;
};

In this case all the valid pin configurations would be encoded as a macro provided by a single header file. Instead of defining multiple DTS nodes we would have

#define PIN_PA3_GPIO                STM32_PINMUX('A', 3, AF0)
#define PIN_PA3_UART4_TX            STM32_PINMUX('A', 3, AF1)
#define PIN_PA3_SPI1_MISO           STM32_PINMUX('A', 3, AF2)
#define PIN_PA3_I2C4_SCL            STM32_PINMUX('A', 3, AF3)
#define PIN_PA3_UART2_RX            STM32_PINMUX('A', 3, AF7)

Such textual database would be easily searchable by pin number or the peripheral name.

There is another problem with embedding pinmux nodes in SoC files. The DTS properties used to configure a pin such as drive-open-drain, bias-pull-up, slew-rate are all board dependent. They cannot be defined at the soc level. While they are not always needed any design that does use them would need to provide pinmux node definitions at the board level anyway.

Copy link
Member

@erwango erwango Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on the reasons that made you prefer defining configurations on board side ?

Actually, I was just trying to show the differences between the Linux implementation and the one in #20386 to make it easier to analyse and discuss them.

In my opinion the most important one is the following:

  • Linux DTS only defines pinmux nodes that are used. Approach proposed in dts: stm32: Introduce linux like pinctrl bindings #20386 would require us to embed a full database of possible pin configurations in DTS. E.g. if pin PA3 can be connected to GPIOA, UART2 RX, UART4 TX, SPI1 MISO or I2C4 SCL we would need to define all these nodes. And it had to be done for all the pins.

Analyzing / processing of DTS data would become difficult for humans and could negatively impact compilation time. Also, to build all the DTS nodes we would need to use scripts, doing this by hand is not really feasible.

Following the Linux approach seems more efficient.

I acknowledge this difference between Linux and my proposal, which was made on purpose. There is difference between Linux and Zephyr (at least STM32-wise), there are only a handful of STM32 boards in Linux, while there are around 70 in Zephyr, also these boards often share similar pin configuration. The needs in term of pin configuration exposure are then different.
Redefining full pin configuration for each board would imply lot of code redundancy with the consequences: copy/paste errors, maintainability issues...

This balances the issues you raise about analyzing/generating these files.
This being said, I recognize the negative impact on processing that would occur if we intend to propose the whole pin configuration combinatorial in soc files. but we can also limit the available combinations to the one that are in use in available boards, which reduces the additional load to an acceptable level I guess.

Using the following template we would give user full flexibility to configure the pin without the need to provide any kind of database of valid configurations.

usart2_pins_default: pinmux_usart2_pins {
	pinmux = <STM32_PINMUX('A', 2, AF7)>, <STM32_PINMUX('A', 3, AF7)>;
	bias-disable;
};

In both cases, user would have the possibility to overwrite the provided combinations in board files. So I don't think one solution would be less flexible than another.

the obvious disadvantage of this style is the lack of validation. It would be easy to make a mistake when typing in the data. The other template doesn't have this problem

usart2_rx_pins_default: pinmux_usart2_rx_pins {
	pinmux = <PIN_PA3_UART2_RX>;
	bias-disable;
};

In this case all the valid pin configurations would be encoded as a macro provided by a single header file. Instead of defining multiple DTS nodes we would have

#define PIN_PA3_GPIO                STM32_PINMUX('A', 3, AF0)
#define PIN_PA3_UART4_TX            STM32_PINMUX('A', 3, AF1)
#define PIN_PA3_SPI1_MISO           STM32_PINMUX('A', 3, AF2)
#define PIN_PA3_I2C4_SCL            STM32_PINMUX('A', 3, AF3)
#define PIN_PA3_UART2_RX            STM32_PINMUX('A', 3, AF7)

Such textual database would be easily searchable by pin number or the peripheral name.

This is indeed interesting. Though we'll need to add the series identifier to avoid cross-series confusions:

#define PIN_F4_PA3_GPIO                STM32_PINMUX('A', 3, AF0)
#define PIN_F4_PA3_UART4_TX            STM32_PINMUX('A', 3, AF1)

There is another problem with embedding pinmux nodes in SoC files. The DTS properties used to configure a pin such as drive-open-drain, bias-pull-up, slew-rate are all board dependent. They cannot be defined at the soc level. While they are not always needed any design that does use them would need to provide pinmux node definitions at the board level anyway.

I'm not so sure. While he have a lot of boards, they all nearly share the same pin configurations today. And this has been proven efficient since in general configuration issues are common to all the boards. And fixes applied successfully to every board (IIRC I don't have examples of issues that occurred on some boards when fixing common configurations).

And again, if need arise, it would still be possible to overwrite a configuration in board file.

usart1_pins_default: pinmux_usart1_pins {
pinmux = <STM32_PINMUX('A', 9, AF7)>, <STM32_PINMUX('A', 10, AF7)>;
bias-disable;
};

usart2_pins_default: pinmux_usart2_pins {
pinmux = <STM32_PINMUX('A', 2, AF7)>, <STM32_PINMUX('A', 3, AF7)>;
bias-disable;
};
};

&pinctrl {
usart1_pins_default: pinmux_usart1_pins {
rx {
pinmux = <STM32_PINMUX('A', 10, AF7)>;
bias-disable;
};
tx {
pinmux = <STM32_PINMUX('A', 9, AF7)>;
bias-pull-up;
};
};

usart2_pins_default: pinmux_usart2_pins {
rx {
pinmux = <STM32_PINMUX('A', 3, AF7)>;
bias-disable;
};
tx {
pinmux = <STM32_PINMUX('A', 2, AF7)>;
bias-pull-up;
};
};
};

&pinctrl {
usart1_rx_pins_default: pinmux_usart1_rx_pins {
pinmux = <STM32_PINMUX('A', 10, AF7)>;
bias-disable;
};

usart1_tx_pins_default: pinmux_usart1_tx_pins {
pinmux = <STM32_PINMUX('A', 9, AF7)>;
bias-pull-up;
};

usart2_rx_pins_default: pinmux_usart2_rx_pins {
pinmux = <PIN_PA3_UART2_RX>;
bias-disable;
};

usart2_tx_pins_default: pinmux_usart2_tx_pins {
pinmux = <PIN_PA2_UART2_TX STM32_PUSHPULL_PULLUP>;
};
};

&usart1 {
current-speed = <115200>;
pinctrl-0 = <&usart1_pins_default>;
pinctrl-names = "default";
status = "okay";
};

&usart2 {
current-speed = <115200>;
pinctrl-0 = <&usart2_rx_pins_default &usart2_tx_pins_default>;
pinctrl-names = "default";
status = "okay";
};

Expand Down
50 changes: 47 additions & 3 deletions dts/bindings/pinctrl/st,stm32-pinmux.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,50 @@ properties:
reg:
required: true

pinmux-cells:
- pin
- function
child-binding:
Copy link
Contributor

@b0661 b0661 Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my view the usage of child-binding here is not optimal. But as you are following #20386 it is clear to do so. You may have a look at #22255 which is the solution we are using for the OOT pinctrl driver.

title: STM32 PIN configurations

description: |
This binding gives a base representation of the STM32 pins configration

properties:
pinmux:
required: false
type: int

bias-disable:
required: false
type: boolean

bias-pull-down:
required: false
type: boolean

bias-pull-up:
required: false
type: boolean

drive-push-pull:
required: false
type: boolean

drive-open-drain:
required: false
type: boolean

output-low:
required: false
type: boolean

output-high:
required: false
type: boolean

slew-rate:
type: int
default: 2
enum:
- 0
- 1
- 2
- 3
25 changes: 25 additions & 0 deletions include/dt-bindings/pinctrl/stm32-pinctrl-common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,31 @@
#ifndef ZEPHYR_STM32_PINCTRL_COMMON_H_
#define ZEPHYR_STM32_PINCTRL_COMMON_H_

/* Extracted from Linux: include/dt-bindings/pinctrl/stm32-pinfunc.h */
/* define PIN modes */
#define GPIO 0x0
#define AF0 0x1
Copy link
Contributor

@b0661 b0661 Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would start the numbering with AF0 0x0 to use the alternate functions directly for controlling the hardware.

In case you use something like this in the pinctrl/pinmux driver to get the LL_GPIO_ALT_FUNC numbers:

static inline u32_t pinctrl_stm32_ll_gpio_alt_func(u16_t func)
{
	if ((func >= AF0) &&
	    (func <= AF15)) {
		return func - AF0;
	}
	__ASSERT(0, "function 0x%x unknown - no LL_GPIO_ALT_FUNC", (int)func);
	return func;
}

the compiler can eliminate at least the >= AF0 check.

#define AF1 0x2
#define AF2 0x3
#define AF3 0x4
#define AF4 0x5
#define AF5 0x6
#define AF6 0x7
#define AF7 0x8
#define AF8 0x9
#define AF9 0xa
#define AF10 0xb
#define AF11 0xc
#define AF12 0xd
#define AF13 0xe
#define AF14 0xf
#define AF15 0x10
#define ANALOG 0x11

/* define Pins number*/
#define PIN_NO(port, line) (((port) - 'A') * 0x10 + (line))

#define STM32_PINMUX(port, line, mode) (((PIN_NO(port, line)) << 8) | (mode))

/**
* @brief numerical IDs for IO ports
Expand Down
16 changes: 16 additions & 0 deletions include/dt-bindings/pinctrl/stm32l4-pins.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright (c) 2017 Linaro Limited
*
* SPDX-License-Identifier: Apache-2.0
*/

#ifndef INCLUDE_DT_BINDINGS_PINCTRL_STM32L4_PINCTRL_H_
#define INCLUDE_DT_BINDINGS_PINCTRL_STM32L4_PINCTRL_H_

#include "stm32-pinctrl.h"

#define PIN_PA3_UART2_RX STM32_PINMUX('A', 3, AF7)
#define PIN_PA2_UART2_TX STM32_PINMUX('A', 2, AF7)


#endif /* INCLUDE_DT_BINDINGS_PINCTRL_STM32L4_PINCTRL_H_ */