Skip to content

Conversation

@mnkp
Copy link
Member

@mnkp mnkp commented Mar 19, 2020

This PR is based on #20386 and tries to showcase pinctrl implementation that is closer to the way pinctrl subsystem is implemented on Linux.

In this proposal the pinmux database is kept away from the SoC level DTS. Configuration is done fully within the board dts.

Three different examples of how pin configuration data could be encoded in pinctrl node are presented.

Introduce Linux like dts bindings for stm32 devices.

Signed-off-by: Piotr Mienkowski <[email protected]>
@mnkp mnkp added area: Boards area: Devicetree area: Pinmux platform: STM32 ST Micro STM32 DNM This PR should not be merged (Do Not Merge) dev-review To be discussed in dev-review meeting labels Mar 19, 2020
/* Extracted from Linux: include/dt-bindings/pinctrl/stm32-pinfunc.h */
/* define PIN modes */
#define GPIO 0x0
#define AF0 0x1
Copy link
Contributor

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.

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

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.

};
};

&pinctrl {
Copy link
Member

@erwango erwango Mar 20, 2020

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

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

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.

@github-actions github-actions bot added area: API Changes to public APIs has-conflicts Issue/PR has conflicts with another issue/PR area: Device Tree area: Devicetree Binding PR modifies or adds a Device Tree binding and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jun 29, 2020
@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 1, 2020
@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 2, 2020
@galak galak removed area: Boards dev-review To be discussed in dev-review meeting labels Jul 9, 2020
@nashif nashif added the Stale label Sep 10, 2020
@github-actions github-actions bot closed this Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: Boards area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: Pinmux DNM This PR should not be merged (Do Not Merge) has-conflicts Issue/PR has conflicts with another issue/PR platform: STM32 ST Micro STM32 Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants