drivers: pinctrl device interface (Linux-like) and STM32F0 driver #5799
Conversation
b0661
requested review from
dbkinder,
erwango,
mbolivar,
superna9999,
tbursztyka and
ydamigos
as code owners
Codecov Report
@@ Coverage Diff @@
## master #5799 +/- ##
===========================================
+ Coverage 52.17% 64.16% +11.99%
===========================================
Files 215 432 +217
Lines 26013 41308 +15295
Branches 5594 6938 +1344
===========================================
+ Hits 13572 26506 +12934
- Misses 10183 11640 +1457
- Partials 2258 3162 +904
Continue to review full report at Codecov.
|
b0661
force-pushed
the
pinctrl_test
branch
2 times, most recently
from
76611e9
to
efa7ae1
Compare
| ============================== | ||
|
|
||
| There are 2 samples that allow you to test that the button and LED on | ||
| the board are working properly with Zephyr: |
There was a problem hiding this comment.
I'd delete the code block below and just say here:
... with Zephyr: :ref:`blinky-sample` and :ref:`button-sample`
| +--------+--------+--------------------+ | ||
| | Signal | GPIO | Connection on CN10 | | ||
| +========+========+====================+ | ||
| | IN | PB4 | 27 <- 29 | |
There was a problem hiding this comment.
I'm not sure what the arrow indicates here for a jumper? Can you explain?
There was a problem hiding this comment.
Arrow shows signal from output to input.
| +========+========+====================+ | ||
| | SCK | PB3 | 31 (open) | | ||
| +--------+--------+--------------------+ | ||
| | MISO | PB4 | 27 <- 29 | |
There was a problem hiding this comment.
Same question, what does the arrow mean for a jumper?
There was a problem hiding this comment.
Arrow shows signal from output to input
There was a problem hiding this comment.
Hit submit way to early on that previous review. Looks like we're bringing in a whole set of Chaos subsystem documentation into the Zephyr project's documentation set here, and it's an external component, yes? Would seem better to refer to their documentation rather than duplicating it here...
| @@ -0,0 +1,316 @@ | |||
| .. | |||
| Copyright (c) 2017 Bobby Noelte | |||
There was a problem hiding this comment.
We haven't been putting copyright or SPDX tags on documentation...
There was a problem hiding this comment.
When writing the documentation for pinctrl I had a major problem to figure out whether I can re-use the pinctrl docs from linux. In the end, after your advise in an email, I just reference the documentation and otherwise somehow re-invented the wheel. I would not mind somebody else re-using my documentation. So I pledge to have the copyright notice also in the documentation. Al least not to remove it in my documentation.
| Pin Multiplexing | ||
| **************** | ||
|
|
||
| Pin Initialisation |
There was a problem hiding this comment.
American spelling please (Initialization)
|
|
||
| Pin Multiplexing | ||
| **************** | ||
|
|
There was a problem hiding this comment.
... more coming I presume?
doc/subsystems/chaos_pp/chaos_pp.rst
Outdated
|
|
||
| An active argument is an argument that expands each time that it is scanned by the preprocessor. | ||
|
|
||
| * `Active Arguments <file:../../../../../ext/lib/preprocessor/chaos/built-docs/active-arguments.html>`_ |
There was a problem hiding this comment.
Hmm... we need to figure out a better way to link to this documentation...
There was a problem hiding this comment.
Would it be possible to copy the html files to the sphinx html output directory during make htmldocs? Sorry there is no offical web site that can be referenced.
There was a problem hiding this comment.
I'm not feeling good about using the Zephyr project as the home for documenting an (unrelated) external component (Chaos preprocessor library). I'll let @nashif advise how to proceed here.
doc/subsystems/chaos_pp/chaos_pp.rst
Outdated
|
|
||
| .. _chaos_pp: | ||
|
|
||
| Chaos Preprocessor Library |
There was a problem hiding this comment.
Oh my. Not sure we want to document a component in /ext like this. Isn't there an external repo/site we can refer to rather than, what looks like, duplicating the library documentation here?
There was a problem hiding this comment.
Sorry, there is no external documentation site.
|
Hi @b0661, this is quite a work! It'll require some time to review in deep. |
| #define PINCTRL_STM32_GPIO_COUNT 8 | ||
| #elif defined(GPIOG) | ||
| #define PINCTRL_STM32_GPIO_COUNT 7 | ||
| #elif defined(GPIOF) |
There was a problem hiding this comment.
In some series (stm32f401 for instance), GPIOH w/o GPIOF and GPIOG.
So PINCTRL_STM32_GPIO_COUNT will be 8 while it should actually be 6.
So either it should be computed in a different way, either you should consider not to use it with care.
Besides, for footprint optimization purpose, we should let the possibility to only activate a subset
of GPIOs, do you take that option into account? (I didn't parsed everything yet, so this is just a question)
There was a problem hiding this comment.
Only activated GPIOs will be instantiated. Look for CHAOS_PP_WHEN(_active)
There was a problem hiding this comment.
CHAOS_PP_WHEN(_active) is not used anymore.
we should let the possibility to only activate a subset
The correct number of activated GPIOs is now provided by SOC_GPIO_CONTROLLER_COUNT. For all activated GPIOs a SOC_GPIO_CONTROLLER_0 ... define is created based on the gpio-controller directive in the device tree.
arch/arm/soc/st_stm32/stm32f0/soc.h
Outdated
| @@ -43,6 +43,11 @@ | |||
| #include <stm32f0xx_ll_system.h> | |||
| #endif /* CONFIG_CLOCK_CONTROL_STM32_CUBE */ | |||
|
|
|||
| #ifdef CONFIG_PINCTRL_STM32 | |||
| #include <stm32f0xx_ll_gpio.h> | |||
| #define CONFIG_PINCTRL_STM32_PREFIX ST_STM32_PINCTRL_48000000 | |||
There was a problem hiding this comment.
Is this really needed?
Can't we get the prefix from device tree? (derivate from pinctrl node compatible for instance)
There was a problem hiding this comment.
Good idea. I will investigate in using the -controller (e.g. gpio-controller, pin-controller>, ...) directive and create something like #define PIN_CONTROLLER_0 ST_STM32_PINCTRL_48000000.
There was a problem hiding this comment.
@erwango Partially Out of subject, but couln't we use the aliases to generate the fixups ?
There was a problem hiding this comment.
My current solution generates defines for the SOC node based on device-controller directives. So a device driver gets defines of the form #define DEVICECONTROLLER_0 ST_STM32DEVICE_48000000 and additional there is a define for the total amount of such devices. You can generate also fixups from this, but finally the drivers must be adapted.
| # Pinctrl driver | ||
| CONFIG_PINCTRL=y | ||
| CONFIG_PINCTRL_PINMUX=y | ||
| CONFIG_PINCTRL_STM32=y |
There was a problem hiding this comment.
Move to arch/arm/soc/st/stm32//
|
|
||
| # GPIO Controller | ||
| CONFIG_GPIO=y | ||
| CONFIG_GPIO_PINCTRL=y | ||
| CONFIG_GPIO_STM32=y |
There was a problem hiding this comment.
Move to arch/arm/soc/st/stm32//
There was a problem hiding this comment.
@erwango Not quite shure what to do here?
arch/arm/soc/st_stm32/stm32f0/soc.h
Outdated
| #include <stm32f0xx_ll_exti.h> | ||
| #include <stm32f0xx_ll_gpio.h> | ||
| #include <stm32f0xx_ll_system.h> | ||
| #define CONFIG_GPIO_STM32_GPIOA_PREFIX ST_STM32_GPIO_PINCTRL_48000000 |
There was a problem hiding this comment.
Same as for pinctrl, can it be derived from dts?
There was a problem hiding this comment.
Yes it can. Will change.
drivers/gpio/gpio_pinctrl_stm32.c
Outdated
| #elif defined(GPIOG) | ||
| #define GPIO_PINCTRL_STM32_PORTS_COUNT 7 | ||
| #elif defined(GPIOF) | ||
| #define GPIO_PINCTRL_STM32_PORTS_COUNT 6 |
There was a problem hiding this comment.
As mentioned for PINCTRL, in some SoCs, GPIOH can be present while GPIOF isn't.
There was a problem hiding this comment.
No Problem as long as you do not activate the node in the device tree, the device will not be created.
There was a problem hiding this comment.
ok, I'll have a try and see how this all works.
| # SPI1 - master | ||
| # SPI2 - slave | ||
| CONFIG_SPI=y | ||
| CONFIG_SPI_1=y |
There was a problem hiding this comment.
Ok. will move CONFIG_SPI. CONFIG_SPI_1 will come from DT.
| # SPI2 - slave | ||
| CONFIG_SPI=y | ||
| CONFIG_SPI_1=y | ||
| CONFIG_SPI_1_IRQ_PRI=10 |
There was a problem hiding this comment.
Is that needed? Should come from dts.
| CONFIG_SPI_1_IRQ_PRI=10 | ||
| CONFIG_SPI_2=y | ||
| CONFIG_SPI_2_IRQ_PRI=11 | ||
| CONFIG_SPI_STM32=y |
| CONFIG_SPI_2=y | ||
| CONFIG_SPI_2_IRQ_PRI=11 | ||
| CONFIG_SPI_STM32=y | ||
| CONFIG_SPI_STM32_INTERRUPT=y |
| CONFIG_SPI_2_IRQ_PRI=11 | ||
| CONFIG_SPI_STM32=y | ||
| CONFIG_SPI_STM32_INTERRUPT=y | ||
| CONFIG_SPI_STM32_HAS_FIFO=y |
There was a problem hiding this comment.
Not needed, derived from spi/Kconfig
erwango
added
area: GPIO
area: Devicetree
area: Pinmux
RFC
|
Very impressive, I will have a look, but there is a lot to review ! |
drivers/gpio/gpio_pinctrl_stm32.c
Outdated
| } | ||
| /* Assure pin is on input before doing configuration | ||
| * to prevent damage */ | ||
| err = pinctrl_mux_set(data->pinctrl, pinctrl_pin, |
There was a problem hiding this comment.
I'm not sure about this... yet I don't know how it should be handled !
But if, let's says the bootloader has already configured the pin and the config is vital to the system, doing this will maybe cause unwanted side effects !
Changing the config won't harm if already in output, meaning it already has been configured somewhere.
There was a problem hiding this comment.
You are right, will rethink.
There was a problem hiding this comment.
Changing the config won't harm if already in output, meaning it already has been configured somewhere.
Reverted to not switch to input before configuration.
drivers/pinctrl/Kconfig
Outdated
| @@ -54,5 +54,8 @@ config PINCTRL_MUX_REQUEST | |||
| help | |||
| Supports pin allocation management by pinctrl_mux_request() and | |||
| pinctrl_mux_free(). | |||
|
|
|||
| source "drivers/pinctrl/Kconfig.stm32" | |||
|
|
|||
drivers/pinctrl/Kconfig.stm32
Outdated
| config PINCTRL_STM32 | ||
| bool "Pinctrl driver for STM32 MCUs" | ||
| depends on PINCTRL && SOC_FAMILY_STM32 | ||
| select HAS_DTS_PINCTRL |
There was a problem hiding this comment.
select is not aligned
drivers/pinctrl/pinctrl_stm32.c
Outdated
| #include <chaos/preprocessor/control/when.h> | ||
| #include <chaos/preprocessor/repetition/repeat_from_to.h> | ||
|
|
||
|
|
There was a problem hiding this comment.
Sorry, currently there are a lot of these things. Wanted to get that whole thing out to get feedback. Will look into it. Thank you for reviewing.
drivers/pinctrl/pinctrl_stm32.c
Outdated
| pinctrl_stm32_device_init | ||
|
|
||
| #include "pinctrl_lib.h" | ||
|
|
drivers/pinctrl/pinctrl_stm32.c
Outdated
| #define CONFIG_PINCTRL_LIB_CONTROLLER_0_DATA &pinctrl_stm32_data | ||
| #define CONFIG_PINCTRL_LIB_CONTROLLER_0_PIN_COUNT PINCTRL_STM32_PIN_COUNT | ||
| #define CONFIG_PINCTRL_LIB_CONTROLLER_0_DEVICE_INIT \ | ||
| pinctrl_stm32_device_init |
There was a problem hiding this comment.
while it seems a good idea, using defines to pass functions looks ... ugly
There was a problem hiding this comment.
@superna9999 Do you have a good option?
Extend pinctrl directive handling to fill EDTS database. Signed-off-by: Bobby Noelte <[email protected]>
modules/pincontroller.py: Support and helper functions for the pin controller template. Extracts all pin control data of a pin controller from the EDTS. templates/drivers/pinctrl_tmpl.c: The template abstracts away the complex generation of the generic parts of a pinctrl driver such as: - compile time initialisation of configuration data from device tree - access functions to config data - compile time initialisation of whole driver structs The driver implementer does only have to implement the necessary hardware access routines. The template is to be used by inline code generation. A driver implementer must only define a set of python global variables to configure the code generation. Signed-off-by: Bobby Noelte <[email protected]>
Document the pinctrl template that can be included by inline code generation. Signed-off-by: Bobby Noelte <[email protected]>
A kernel configuration and build environment for pinctrl drivers. Add device tree bindings for the STM32 pin controller. Add STM32 pin controller driver. The driver uses the pinctrl library to implement the generic driver functionality. Signed-off-by: Bobby Noelte <[email protected]>
Add device tree bindings for the STM32 GPIO drivers that use the PINCTRL driver as backend (GPIO_PINCTRL). Add STM32 GPIO driver that uses the PINCTRL driver as backend. Signed-off-by: Bobby Noelte <[email protected]>
Enable STM32 driver configuration by device tree. Set st,gpio-bank for gpio nodes. Signed-off-by: Bobby Noelte <[email protected]>
Switch STM32F0x based boards configuration to use the pinctrl and gpio-pinctrl devices. Provide device tree definitions to allow the default pin configuration to be initialised by the PINCTRL driver. Provide correct configuration for samples. Remove unneeded dts.fixup Signed-off-by: Bobby Noelte <[email protected]>
Unit test the pinctrl template API. Signed-off-by: Bobby Noelte <[email protected]>
The NUCLEO FO91RC and F030R8 boards use the GPIO driver with PINCTRL backend by default. The STM32F0 SoC on the board does not support level interrupts. The test is adapted to exclude the level interrupt tests and to correctly set up the GPIO pins for the board and the choosen GPIO driver with PINCTRL backend. Signed-off-by: Bobby Noelte <[email protected]>
Use correct pin definition depending on CONFIG_PINCTRL. Signed-off-by: Bobby Noelte <[email protected]>
Use correct pin definition depending on CONFIG_PINCTRL. Signed-off-by: Bobby Noelte <[email protected]>
Document the usage of the STM32 pinctrl driver and the related STM32 GPIO driver (with pinctrl backend). Signed-off-by: Bobby Noelte <[email protected]>
Extend the board documentation with the PINCTRL peripherial and a more eleborate flashing example. Add documentation how to test the board. Signed-off-by: Bobby Noelte <[email protected]>
Describe CAN and I2C1 pin mapping. I2C1 pin mapping is changed to allow for CAN pins. Signed-off-by: Bobby Noelte <[email protected]>
Add generic bindings for clock consumers and clock providers. Add a common binding for fixed clocks. To be included by SoC specific bindings. Signed-off-by: Bobby Noelte <[email protected]>
Support more clocks related directives (besides clocks) and fill EDTS database - clock-names - clock-output-names - clock-indices - clock-ranges - clock-frequency - clock-accuracy - oscillator - assigned-clocks - assigned-clock-parents - assigned-clock-rates Signed-off-by: Bobby Noelte <[email protected]>
All flash controllers have a mandatory compatible property. Add it to the generic binding that is included by all. Signed-off-by: Bobby Noelte <[email protected]>
Add common bindings for flash devices to be included by SoC specific bindings. Signed-off-by: Bobby Noelte <[email protected]>
Extend flash handling to fill the EDTS database. Signed-off-by: Bobby Noelte <[email protected]>
Extend <device>-controller directive handling to fill EDTS database. Signed-off-by: Bobby Noelte <[email protected]>
Fix false passed on localized error message in make invocation. Fixes zephyrproject-rtos#8348 Signed-off-by: Bobby Noelte <[email protected]>
The PR provides a new device interface for pin controllers. As a proof of concept the interface is implemented and tested for the STM32FOx SoC.
The Zephyr pin controller follows the concepts of the Linux PINCTRL (PIN CONTROL) subsystem.
The Zephyr pin controller does not provide the full scope of the Linux PINCTRL subsystem. Also the PINCTRL interface is specific to Zephyr and not compatible (but similar) to the Linux PINCTRL interface.
As a major goal the device tree bindings are kept compatible to the Linux pinctrl bindings as much as possible.
Review
This is the master pull request.
To ease commenting and review several review/ RFC pull requests are available that focus only on specific aspects. Please use these PRs instead of this one. I will do my best to keep them in sync.
pinctrl driver interface
pinctrl api documentation
pinctrl usage documentation
generic binding for pinctrl devices
extend device bindings by pinctrl
extract new pinctrl and related directives
Some patches are extracted for independent inclusion into master:
Features
The PINCTRL device driver provides applications and other drivers access:
and functions,
The driver also provides initial setup of pin configuration and pin multiplexing controlled by the device tree pinctrl state information (the "default" state). This obsoletes the SoC specific pinmux.c.
PINCTRL device driver API and usage documentation is added.
Relates to device tree: generate pinmux #4830
The PINCTRL device interface provides also the PINMUX interface as a generic shim. For more sophisticated configuration the PINCTRL interface can be used directly.
Fixes Streamline GPIO, Pinmux driver API #2288, api: pinmux/gpio: It isn't possible to set pins as input and output simultaneously #6084
The PINCTRL device interface provides a superset to the GPIO configuraton properties with well understood configuration properties. It can be used instead of the GPIO configuration interface.
Relates to rework GPIO flags #4506.
The new GPIO-PINCTRL driver (GPIO that uses the PINCTRL driver as backend) provides GPIO pin number to PINCTRL pin number mapping controlled by device tree info (gpio-ranges).
Documentation for PINCTRL API and for extensions to the GPIO API, and PINCTRL and GPIO usage documentation is added.
Relates to device tree: gpio #4829
Design
PINCTRL driver interface
The PINCTRL driver interface API is added in include/pinctrl.h. Defines that may be used in the device tree and in the interface are in include/pinctrl_common.h. That way they can be included from the interface and from the device tree bindings (e.g include/dt-bindings/pinctrl_stm32.h). Within include/pinctrl.h a generic shim that maps the PINMUX interface to the PINCTRL interface is added.
GPIO driver interface extensions
For GPIO-PINCTRL drivers (GPIO drivers that use the PINCTRL driver as backend) generic PIN number defines are introduced. These PIN number defines allow to describe a set of pins by just or-ing the defines. Such operations on a set of pins of a port can easily be described and implemented.
Due to the new gpio-ranges device tree directive a mapping from GPIO pin numbers to PINCTRL pin numbers is possible.
Extraction of PINCTRL info from device tree
Extraction of device tree info is - as usual - done by the extract_dts_includes.py script. To allow modularization of the monolitic script all global data and functions are factorized to a python module that can be imported by other modules. The extraction of pinctrl directives and gpio-ranges directives is added as separate python modules.
For a pin controller node extract_dts_includes.py extracts five data sets:
For all active client nodes that reference a pin-controller by pinctrl-x(&pinctrl, ...) FUNCTION defines are generated. The function equals the client node.
For all pinctrl names of pinctrl-x directives of active client nodes STATE_NAME defines are generated. This is mostly to allow deduplication of commonly used state names such as e.g. "default".
For all pinctrl-x directives of active client nodes PINCTRL_STATE defines are generated. The pinctrl state references the FUNCTION (aka. node) it is given in. The pinctrl state also references the STATE_NAME given in the pinctrl-names directive associated to the pinctrl-x directive. All the pins that a pinctrl state controls are regarded to be a pin group. The name of the pin group is the same as the name of the pinctrl state.
For all subnodes of pin configurations referenced by active client nodes, pinctrl-x(&xxx, pinconf_a, ...), PINCTRL defines are generated. The pinctrl references the FUNCTION (aka. node) it is referenced from. The pinctrl also references the PINCTRL_STATE it is referenced from.
For all gpio-ranges directives of active GPIO nodes GPIO_RANGE defines are generated.
For the clients of pin controllers extract_dts_includes.py extracts two data sets:
For all pin controllers that are referenced in a pinctrl-x directive PINCTRL_CONTROLLER defines are generated.
For all subnodes of pin configurations referenced by active client nodes, pinctrl-x(&xxx, pinconf_a, ...), PINCTRL defines are generated. The pinctrl references the PINCTRL_CONTROLLER given in the pinctrl-x directive. The pinctrl also references the PINCTRL_STATE the pin controller associates to this pinctrl.
Generic PINCTRL driver template
To simplify driver implementation a template for the hardware independent pinctrl driver functions and all driver data structures and content is added.
The pin controller template provides the equivalent to a C++ template for pin controller driver implementation. The template parameters are given by simple python global variables. The template is expanded by the inline code generator with the help of Python. The template generates the appropriate data structures and fills them using the information provided in
generated_dts_board.h.The usage of the PINCTRL template itself is optional.
STM32F0 Driver Implementation
New STM32 PINCTRL driver
Driver implementation in pinctrl_stm32.[ch].
Extended STM32 GPIO driver using the pinctrl driver as a backend
The driver implementation for gpio_pinctrl_stm32.[ch] starts with a copy of the gpio_stm32 driver to enable easy tracking of changes. The driver is currently only used for STM32F0x SoCs. At the end it shall become the only driver for STM32Fx SoCs.
Extended STM32 SPI driver using the new pinctrl device tree directives.
The driver implementation for spi_stm32.[ch] starts with a copy of the spi_ll_stm32 driver to enable easy tracking of changes. The driver is currently only used for STM32F0x SoCs. At the end it shall become the only driver for STM32Fx SoCs.
Testing
Samples were executed sucessfully on the NUCLEO F091RC board:
Driver tests were also executed on the NUCLEO F091RC board:
Unit tests were created and executed for:
Transition Strategy
Pinctrl Driver
The pinctrl interface and driver is independent from other drivers.
STM32F0x is switched to the pinctrl driver. Others may switch at any time.
Other Drivers
Strategy is to have two driver variants during transition time. The new/enhanced driver is started as a pure copy of the current one to enable easy tracking of changes. After all driver users have switched to the new driver variant it should replace the current one and only one driver should be there to maintain.
Patches
PINCTRL driver interface and related GPIO driver interface
Extraction of PINCTRL and related info from device tree
(with some preparatory work)
Template specifically for pinctrl driver implementations
STM32F0 pinctrl driver implementation as proof of concept
STM32F0 GPIO driver with PINCTRL driver backend as proof of concept
STM32F0 SPI driver with device tree support as proof of concept
STM32FO device tree
STM32F0x based boards configuration set to pinctrl as proof of concept
Testing - Unit Tests
Testing - Drivers
Testing - Samples
General tools for documentation
Documentation
Todo
Change History
2018-06-23
2018-06-15
2018-06-15
2018-05-21
2018-05-18
2018-05-10
2018-04-03
2018-03-27
2018-03-22
2018-02-25
2018-02-21
2018-02-20
2018-01-28
2018-01-24
Signed-off-by: Bobby Noelte [email protected]