drivers: pinctrl: new state based API proposal #37572

gmarull · 2021-08-10T10:56:45Z

Introduction

This RFC provides a new state-based pin controller API, following in some way the Linux design principles.

API proposal

A detailed description of the API, including design principles, Devicetree representations, usage, etc. can be found here https://github.com/zephyrproject-rtos/zephyr/blob/1da6d9a9e290a7b9ea0098bc8a89bcc22d0cc155/doc/guides/pinctrl/index.rst (part of the doc: guides: add pinctrl guide commit).

References material

Pinctrl states used in power management context (Linux): https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-omap.c#L1560
A quick practical overview (Linux, STM32): https://wiki.st.com/stm32mpu/wiki/Pinctrl_overview

For reviewers

This PR only introduces the API, tests and documentation. For actual implementations or board samples, refer to the following PRs:

nRF driver (includes dynamic pinctrl sample): #39429
STM32 driver: #39430

Resolves #22748
Resolves #29369

henrikbrixandersen · 2021-08-10T13:55:56Z

It would be interesting to hear the motivation for a different approach than the already existing proposal in #35847.

gmarull · 2021-08-10T14:10:30Z

It would be interesting to hear the motivation for a different approach than the already existing proposal in #35847.

I think the main differences are:

The API is state based, so a driver provides a state and not a configuration set (same as Linux approach)
Drivers do not need to store pinctrl configurations themselves

nandojve · 2021-08-13T12:24:40Z

Hi @gmarull ,

Cypress require by pin configurations, see below:

zephyr/dts/arm/cypress/psoc6-pinctrl.dtsi

Lines 11 to 16 in 374aab1

    
           pinctrl@40310000 { 
        
           	/* instance, signal, port, pin, hsiom [, flag1, ... ] */ 
        
           	DT_CYPRESS_HSIOM(spi0, mosi,  0, 2, act_8, drive-push-pull); 
        
           	DT_CYPRESS_HSIOM(spi0, miso,  0, 3, act_8, input-enable); 
        
           	DT_CYPRESS_HSIOM(spi0,  clk,  0, 4, act_8, drive-push-pull); 
        
           	DT_CYPRESS_HSIOM(spi0, sel0,  0, 5, act_8, drive-push-pull);

I'm not complete sure if with states we can represent this SoC. However, if we can continue to define by pin + by group, it can be advantageous.

This is the autogenerated devicetree documentation:

zephyr/dts/arm/cypress/pinctrl_cypress_psoc6.h

Lines 73 to 83 in 374aab1

    
            * Flags are optional and should be pass one by one as arguments: 
        
            * 
        
            * DT_CYPRESS_PIN(uart5, rx, 5, 0, act_6, bias-pull-up, input-enable); 
        
            * 
        
            * Will become: 
        
            * 
        
            * p5_0_uart5_rx: p5_0_uart5_rx { 
        
            *    cypress,pins = <&gpio_prt5 0x0 0x12 >; 
        
            *    bias-pull-up; 
        
            *    input-enable; 
        
            * }

Since properties are defined direct on pins, that allows create a common API between gpio and pinctrl .

It can work if the properties defined at states are propagate to all pins that are defined inside that state. This way we can have both definitions and maybe share a common API between gpio and pinctrl.

This will allow less rework on Cypress/Atmel boards because it will be necessary only create the pin groups.

gmarull · 2021-08-13T12:39:47Z

Thanks for sharing @nandojve. How does the SoC handle low power modes? Is it done automatically by the peripheral?

gmarull · 2021-08-13T12:42:02Z

FYI 1cf34dcf9361188e32059cd1831b2ace3e6d9bb6 adds a quick proof of concept on how runtime pinctrl could work together with a usable example.

This patch tries to showcase how runtime pinmux could work in the
context of Zephyr.

This is really a quick and dirty proof of concept, but it serves to
demonstrate the implications of runtime pinctrl, how applications would
use it, etc.

It can be tested on a nRF52840DK board by connecting it to a PC using a
USB cable and by using an extra USB-serial converter with RX pin
connected to P1.02. The USB-serial converter will display "Hello from
second pin configuration set!" and the board virtual serial port will
show "Hello from first pin configuration set!".

nandojve · 2021-08-13T15:40:31Z

Thanks for sharing @nandojve. How does the SoC handle low power modes? Is it done automatically by the peripheral?

In case of Cypress, peripheral is complete disconnected from pins. I mean, for each pin use case the pin must be configured. So, it is necessary case to achieve low power consumption.

I think the main problematic is that there are SoCs that handle on the peripheral itself and there are other SoCs that require a pin state change to achieve the lowest power consumption. Zephyr should handle both situations.

FYI 1cf34dc adds a quick proof of concept on how runtime pinctrl could work together with a usable example.

What you say it is pincfgs at below:

const struct pinctrl_state uart1_cfg0_sleep = {
	.name = "sleep",
	.pincfgs = (const uint32_t[]){
		PINCTRL_NRF(0, 6, INP, IDISC, PN, UART_TX),
		PINCTRL_NRF(0, 8, INP, IDISC, PN, UART_RX),
	},
	.pincfgs_cnt = 2U,
};

is in fact, at current Zephyr implementation like a gpio c structure. See below:

This
DT_CYPRESS_PIN(uart5, rx, 5, 0, act_6, bias-pull-up, input-enable);

auto generate this

p5_0_uart5_rx: p5_0_uart5_rx { 
   cypress,pins = <&gpio_prt5 0x0 0x12 >; 
   bias-pull-up; 
   input-enable; 
}

which auto generate

zephyr/soc/arm/cypress/common/soc_gpio.h

Lines 67 to 71 in dc63385

    
           struct soc_gpio_pin { 
        
           	GPIO_PRT_Type *regs; /** pointer to registers of the GPIO controller */ 
        
           	uint32_t pinum;      /** pin number */ 
        
           	uint32_t flags;      /** pin flags/attributes */ 
        
           };

and currently it is used by drivers

zephyr/drivers/serial/uart_psoc6.c

Lines 53 to 54 in dc63385

    
           uint32_t num_pins; 
        
           struct soc_gpio_pin pins[];

Now, you add a very interesting point related to how to group it. What I would like know is if we can store properties from pincfg-node at pin struct, instead on a group state C structure.

Example, below constructions could be equivalent at c structure:

n: pin_group_1 {
	pins = <
                     PINCTRL_NRF(0, 8, INP, IDISC, PN, UART_RX
                     PINCTRL_NRF(0, 7, INP, ICONN, PU, UART_CTS)
                   >;

	bias-pull-up; 
	input-enable; 
}

p5_0_uart5_rx: p5_0_uart5_rx { 
   cypress,pins = <&gpio_prt5 0x0 0x12 >; 
   bias-pull-up; 
   input-enable; 
} 

n: pin_group_2 {
	pins = <&p5_0_uart5_rx>;
}

In this case, pin_group_1, each pin will receive 2 flag properties bias-pull-up and input-enable at pin C structure. At pin_group_2 will happen the same. If we can automatically pick properties on group and propagate to each pin I think we can have a solution that may solve low power for any SoC and share API between gpio and pinctrl.

If we will do this on a centralized way (at some pinctrl manager) or distributed (on each driver) it is another thing.

mbolivar-nordic

Compared to #37621, I like that thought has been put into runtime changes and power management.

As discussed offline, I think the nRF specific pincfg properties look pretty hard to use and I hope you can split them out into function-specific properties like tx, rx, rts, etc. Since the pinctrl driver already knows about the compatible, that should be possible.

I also agree that states could in principle be moved away from strings but I'd like to know the plan for doing that.

galak

Is the device init/de-init stuff needed? If not its better to keep that orthogonal to this change.

drivers/pinctrl/Kconfig

galak

Need cleanup in the Kconfig.

Initial skeleton for pinctrl drivers. This patch includes common infrastructure and API definitions for pinctrl drivers. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

If a certain state has to be skipped, a macro named Z_PINCTRL_SKIP_<STATE> can be defined evaluating to 1. This can be useful, for example, to automatically ignore the sleep state if no device power management is enabled. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Add support for dynamic pin control, that is, allow to change device pin configuration at runtime. Because no device de-initialization is available yet, this API has limited usage options, e.g. modify pin configuration at early boot stage (before device driver is initialized) Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

When using group based representation on pinctrl nodes, the pin configuration properties end up being at the grand-children level, so the `pincfg-node.yaml` file can't be used. Having a common file that can be used for both cases would require tooling changes, so for now a copy that operated at the grand-children level has been created. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Add a set of tests to check the API behavior. The API tests can only run on a platform that does not have an actual pinctrl driver, e.g. native_posix. The test itself implements a pinctrl mock driver and provides the required "pinctrl_soc.h" header with required types/macros. The implementation is used in the tests to verify the behavior of the API or Devicetree macros. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

A pre-defined Doxygen macro allows for better control of what is included in the final documentation than maintaining a long list of CONFIG_* entries. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Add pinctrl API documentation to the reference guides. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Enable figures enumeration. This option allows to use :numref: in order to reference figures, thus allowing more precise references other than "the figure below" or similar. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Add a user guide that provides general concepts on pin control, details on Zephyr model, implementation guidelines, etc. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Add myself as code owner for pinctrl drivers. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

marc-hb · 2023-02-10T08:10:12Z

doc/conf.py

@@ -108,6 +108,8 @@

 todo_include_todos = False

+numfig = True


This single numfig = True line made both the incremental build and the build from scratch MULTIPLE times slower. On the system I'm currently using it, commenting it out speeds things up like this:

building docs from scratch: 4.5 minutes down from 35 minutes (!)

incremental build with zero change: 15 seconds down from 75 seconds

This is with sphinx 5.3, similar speed-up reproduced with sphinx 5.0.2

I don't understand how no one noticed yet. When I make typos in a doc update I don't want to wait minutes and minutes to check whether I successfully fixed it...

I think there's been other serious regressions but this is by far the biggest one.

That's an interesting finding, I guess we could live without it (just not referencing to figures). Could you report a bug to the Sphinx project? It looks like such build time increase is not justified just because we enumerate figures...

I already spent an inordinate amount of time bisecting this and I now have a fantastic, one-line workaround "permanently" committed in my workspace so I'm not going to report this, sorry :-( I bet the sphinx project could request a reproduction simpler and smaller than the whole zephyr project (I would!) with fewer dependencies and finding that could take a lot more time. It could be worse: maybe it does not reproduce outside Zephyr? I've also never reported any sphinx issue before.

What I'm more tempted to do now that my doc build times have become "reasonable" again is to try to bisect the other, less dramatic performance regressions of the incremental build. Afraid they could be just down to a bigger and bigger repo though...

I already spent an inordinate amount of time bisecting this and I now have a fantastic, one-line workaround "permanently" committed in my workspace so I'm not going to report this, sorry :-( I bet the sphinx project could request a reproduction simpler and smaller than the whole zephyr project (I would!) with fewer dependencies and finding that could take a lot more time. It could be worse: maybe it does not reproduce outside Zephyr? I've also never reported any sphinx issue before.

This is sad, considering it is just about opening an issue here https://github.com/sphinx-doc/sphinx. Just by reporting you'll increase the chances of a Sphinx developer taking a look at it.

What I'm more tempted to do now that my doc build times have become "reasonable" again is to try to bisect the other, less dramatic performance regressions of the incremental build. Afraid they could be just down to a bigger and bigger repo though...

I think that our docs with current structure are always going to result in slow builds. Using breathe is a problem, or having tons of pages with redundant content (e.g. boards or many samples) doesn't help either. I suspect changing this will face opposition or too many questions, though.

This is sad, considering it is just about opening an issue here https://github.com/sphinx-doc/sphinx.

Filing a good bug is usually work, sometimes a lot of work. Also filing is usually just the beginning of a conversation. The first answer is usually: "can you reproduce with the latest [sphinx] version?". Fair enough; we all ask the same.

Also: maybe it's not a bug? Maybe numfig is inherently slow. I found this in sphinx-doc/sphinx#5888 (comment):

... Sphinx assigns section and figure numbers on that action. Therefore, it takes long for large document enabled numbered toctree or numfig.

If you think/hope it's very quick and easy in this particular case to file then why not just do it? :-) It took me a long time to bisect but right now everyone has exactly as much information as I do.

Just by reporting you'll increase the chances of a Sphinx developer taking a look at it.

https://github.com/sphinx-doc/sphinx/issues has currently 1079 open issues so I wouldn't hold my breath. It's only free software.

A new sphinx bug would certainly be much better place to discuss numfig between just ourselves than this merged PR here but sorry again: I don't have (more) time to spend on discussing numfig.

Now I just did "something": I searched for existing numfig bugs and I found none about performance but I mentioned this performance issue in 2 existing bugs that already mentioned numfig cost "in passing". As opposed to filing a brand new bug "in the void", this could draw attention from some people already involved with numfig?

I think that our docs with current structure are always going to result in slow builds.

I understand the time to build from scratch is rarely ever going to get better and I think that's OK. However for "iterative" documentation writing the "zero-change", incremental build time should stay around < 5-6 seconds as it used to be not so long ago. Just like when building anything else.

maybe it's not a bug? Maybe numfig is inherently slow.

While using top I also got a vague impression that fewer sphinx-build processes get started while using numfig. Maybe there's some serialization of some sort? Just a wild guess.

I guess we could live without it (just not referencing to figures)

Maybe it could be disabled in make html-fast and left in make html?

However for "iterative" documentation writing the "zero-change", incremental build time should stay around < 5-6 seconds as it used to be not so long ago. Just like when building anything else.

#55708 adds an new SPHINXOPTS tag that turns off numfig and breathe and brings the incremental build time back down to asomewhat usable 9 seconds.

This reverts commit 4516117. A git bisect showed that the duration of an incremental build doubled after this commit enabled `numfig=True`. Measurements shared and discussed in zephyrproject-rtos#37572, zephyrproject-rtos#55708 and zephyrproject-rtos#56631 confirmed this. Here are yet more measurements below in two different system configurations building docs for very recent Zephyr commit b10817b + all `:numref:` removed by the previous commit. In other words these numbers show the cost of `numfig=True` _without_ even using `:numref:`. * Ubuntu 22, 8 cores sphinx-build --version 4.3.2 numfig=True numfig=False - from scratch 7 min 15 s 6 min 40 s - one-line .rst change 48 s 24s * Current Arch Linux, 72 cores sphinx-build --version 6.2.1 numfig=True numfig=False - from scratch 5 min 0 s 4 min 50 s - one-line .rst change 37 s 18 s Signed-off-by: Marc Herbert <marc.herbert@intel.com>

This reverts commit 4516117. A git bisect showed that the duration of an incremental build doubled after this commit enabled `numfig=True`. Measurements shared and discussed in #37572, #55708 and #56631 confirmed this. Here are yet more measurements below in two different system configurations building docs for very recent Zephyr commit b10817b + all `:numref:` removed by the previous commit. In other words these numbers show the cost of `numfig=True` _without_ even using `:numref:`. * Ubuntu 22, 8 cores sphinx-build --version 4.3.2 numfig=True numfig=False - from scratch 7 min 15 s 6 min 40 s - one-line .rst change 48 s 24s * Current Arch Linux, 72 cores sphinx-build --version 6.2.1 numfig=True numfig=False - from scratch 5 min 0 s 4 min 50 s - one-line .rst change 37 s 18 s Signed-off-by: Marc Herbert <marc.herbert@intel.com>

This reverts commit 4516117. A git bisect showed that the duration of an incremental build doubled after this commit enabled `numfig=True`. Measurements shared and discussed in zephyrproject-rtos#37572, zephyrproject-rtos#55708 and zephyrproject-rtos#56631 confirmed this. Here are yet more measurements below in two different system configurations building docs for very recent Zephyr commit b10817b + all `:numref:` removed by the previous commit. In other words these numbers show the cost of `numfig=True` _without_ even using `:numref:`. * Ubuntu 22, 8 cores sphinx-build --version 4.3.2 numfig=True numfig=False - from scratch 7 min 15 s 6 min 40 s - one-line .rst change 48 s 24s * Current Arch Linux, 72 cores sphinx-build --version 6.2.1 numfig=True numfig=False - from scratch 5 min 0 s 4 min 50 s - one-line .rst change 37 s 18 s Signed-off-by: Marc Herbert <marc.herbert@intel.com>

gmarull added the RFC Request For Comments: want input from the community label Aug 10, 2021

gmarull requested review from mbolivar-nordic and carlescufi August 10, 2021 10:56

github-actions bot added area: API Changes to public APIs area: Boards area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Aug 10, 2021

gmarull requested a review from anangl August 10, 2021 10:57

carlescufi requested review from mnkp, galak and henrikbrixandersen August 10, 2021 10:58

gmarull force-pushed the rfc/pinmux branch from 40d8e9a to bea78a3 Compare August 10, 2021 10:59

MaureenHelm self-requested a review August 10, 2021 16:53

gmarull mentioned this pull request Aug 12, 2021

drivers: spi_nrfx_spim: Fixes related to 32 Mbps speed on nRF5340 #37632

Merged

gmarull force-pushed the rfc/pinmux branch 2 times, most recently from adfa2af to ca4c7d2 Compare August 12, 2021 14:27

nandojve self-requested a review August 12, 2021 14:52

gmarull mentioned this pull request Aug 12, 2021

DT: new pinctrl helpers #37644

Merged

gmarull force-pushed the rfc/pinmux branch 2 times, most recently from 26d05a7 to a6542f7 Compare August 13, 2021 08:48

gmarull force-pushed the rfc/pinmux branch from a6542f7 to 1cf34dc Compare August 13, 2021 12:37

github-actions bot added the area: Samples Samples label Aug 13, 2021

mbolivar-nordic reviewed Aug 14, 2021

View reviewed changes

galak reviewed Aug 16, 2021

View reviewed changes

galak reviewed Oct 21, 2021

View reviewed changes

drivers/pinctrl/Kconfig Outdated Show resolved Hide resolved

galak reviewed Oct 21, 2021

View reviewed changes

drivers/pinctrl/Kconfig Outdated Show resolved Hide resolved

galak requested changes Oct 21, 2021

View reviewed changes

gmarull requested a review from galak October 21, 2021 16:20

gmarull mentioned this pull request Oct 25, 2021

Configuring alternate pin functions from code #39691

Closed

gmarull added 10 commits October 25, 2021 20:59

drivers: pinctrl: initial skeleton

84d040c

Initial skeleton for pinctrl drivers. This patch includes common infrastructure and API definitions for pinctrl drivers. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

doc: doxygen: add Doxygen predefined macro

a479319

A pre-defined Doxygen macro allows for better control of what is included in the final documentation than maintaining a long list of CONFIG_* entries. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

doc: reference: add pinctrl API

b6970e0

Add pinctrl API documentation to the reference guides. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

doc: enable figures enumeration

ff13f41

Enable figures enumeration. This option allows to use :numref: in order to reference figures, thus allowing more precise references other than "the figure below" or similar. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

doc: guides: add pinctrl guide

89ffb51

Add a user guide that provides general concepts on pin control, details on Zephyr model, implementation guidelines, etc. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

CODEOWNERS: add myself to pinctrl

bc30583

Add myself as code owner for pinctrl drivers. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

gmarull force-pushed the rfc/pinmux branch from 9e91d81 to bc30583 Compare October 25, 2021 18:59

galak approved these changes Oct 25, 2021

View reviewed changes

galak merged commit a0bd3b0 into zephyrproject-rtos:main Oct 25, 2021

erwango mentioned this pull request Nov 9, 2021

pinctrl: add STM32 implementation #40194

Merged

gmarull deleted the rfc/pinmux branch January 27, 2022 08:58

mbolivar-nordic mentioned this pull request May 25, 2022

add support to store devicetree phandle+specifier as "foo_dt_spec" structures #23691

Open

marc-hb reviewed Feb 10, 2023

View reviewed changes

This was referenced Feb 10, 2023

Make it possible to disable numfig for certain elements sphinx-doc/sphinx#10316

Open

"make clean html" still searches for "now outdated files" = inefficient sphinx-doc/sphinx#5888

Open

marc-hb mentioned this pull request May 5, 2023

Revert "doc: enable figures enumeration" cause numfig is too slow #57624

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: pinctrl: new state based API proposal #37572

drivers: pinctrl: new state based API proposal #37572

gmarull commented Aug 10, 2021 •

edited

henrikbrixandersen commented Aug 10, 2021

gmarull commented Aug 10, 2021

nandojve commented Aug 13, 2021

gmarull commented Aug 13, 2021

gmarull commented Aug 13, 2021

nandojve commented Aug 13, 2021

mbolivar-nordic left a comment

galak left a comment

galak left a comment

marc-hb Feb 10, 2023 •

edited

gmarull Feb 10, 2023 •

edited

marc-hb Feb 10, 2023

gmarull Feb 10, 2023

marc-hb Feb 10, 2023

marc-hb Feb 12, 2023

marc-hb Mar 11, 2023

drivers: pinctrl: new state based API proposal #37572

drivers: pinctrl: new state based API proposal #37572

Conversation

gmarull commented Aug 10, 2021 • edited

Introduction

API proposal

References material

For reviewers

henrikbrixandersen commented Aug 10, 2021

gmarull commented Aug 10, 2021

nandojve commented Aug 13, 2021

gmarull commented Aug 13, 2021

gmarull commented Aug 13, 2021

nandojve commented Aug 13, 2021

mbolivar-nordic left a comment

Choose a reason for hiding this comment

galak left a comment

Choose a reason for hiding this comment

galak left a comment

Choose a reason for hiding this comment

marc-hb Feb 10, 2023 • edited

Choose a reason for hiding this comment

gmarull Feb 10, 2023 • edited

Choose a reason for hiding this comment

marc-hb Feb 10, 2023

Choose a reason for hiding this comment

gmarull Feb 10, 2023

Choose a reason for hiding this comment

marc-hb Feb 10, 2023

Choose a reason for hiding this comment

marc-hb Feb 12, 2023

Choose a reason for hiding this comment

marc-hb Mar 11, 2023

Choose a reason for hiding this comment

gmarull commented Aug 10, 2021 •

edited

marc-hb Feb 10, 2023 •

edited

gmarull Feb 10, 2023 •

edited