Skip to content
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.

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

drivers: pinctrl: new state based API proposal #37572

Merged
merged 10 commits into from Oct 25, 2021

Conversation

@gmarull
Copy link
Collaborator

@gmarull gmarull commented Aug 10, 2021

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

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
Copy link
Member

@henrikbrixandersen henrikbrixandersen commented Aug 10, 2021

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

Loading

@gmarull
Copy link
Collaborator Author

@gmarull gmarull commented Aug 10, 2021

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

Loading

@MaureenHelm MaureenHelm self-requested a review Aug 10, 2021
@gmarull gmarull force-pushed the rfc/pinmux branch 2 times, most recently from adfa2af to ca4c7d2 Aug 12, 2021
@nandojve nandojve self-requested a review Aug 12, 2021
@gmarull gmarull force-pushed the rfc/pinmux branch 2 times, most recently from 26d05a7 to a6542f7 Aug 13, 2021
@nandojve
Copy link
Member

@nandojve nandojve commented Aug 13, 2021

Hi @gmarull ,

Cypress require by pin configurations, see below:

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:

* 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.

Loading

@gmarull
Copy link
Collaborator Author

@gmarull gmarull commented Aug 13, 2021

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

Loading

@gmarull
Copy link
Collaborator Author

@gmarull gmarull commented Aug 13, 2021

FYI 1cf34dc 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!".

Loading

@nandojve
Copy link
Member

@nandojve nandojve commented Aug 13, 2021

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

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

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.

Loading

Copy link
Member

@mbolivar-nordic mbolivar-nordic left a comment

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.

Loading

Copy link
Contributor

@galak galak left a comment

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

Loading

Copy link
Member

@martinjaeger martinjaeger left a comment

I agree with the approach to define default pin configuration (different from reset value stated in reference manuals) only in the board files so that they are obvious to the user.

The code is really clean and the API is very well explained in the documentation. The small typos I found as well have been fixed in the meantime already.

Loading

Copy link
Member

@carlescufi carlescufi left a comment

Great job @gmarull. I've given some thought to the API naming again, but I can't come up with anything better than what we have now.

Loading

include/drivers/pinctrl.h Show resolved Hide resolved
Loading
include/drivers/pinctrl.h Show resolved Hide resolved
Loading
doc/guides/pinctrl/index.rst Outdated Show resolved Hide resolved
Loading
doc/guides/pinctrl/index.rst Outdated Show resolved Hide resolved
Loading
doc/guides/pinctrl/index.rst Outdated Show resolved Hide resolved
Loading
doc/guides/pinctrl/index.rst Outdated Show resolved Hide resolved
Loading
doc/guides/pinctrl/index.rst Outdated Show resolved Hide resolved
Loading
drivers/pinctrl/Kconfig Outdated Show resolved Hide resolved
Loading
drivers/pinctrl/common.c Show resolved Hide resolved
Loading
dts/bindings/pinctrl/pincfg-node-group.yaml Show resolved Hide resolved
Loading
@gmarull gmarull force-pushed the rfc/pinmux branch 2 times, most recently from 183021b to 9e91d81 Oct 21, 2021
@gmarull gmarull requested review from erwango and carlescufi Oct 21, 2021
drivers/pinctrl/Kconfig Show resolved Hide resolved
Loading
drivers/pinctrl/Kconfig Outdated Show resolved Hide resolved
Loading
drivers/pinctrl/Kconfig Outdated Show resolved Hide resolved
Loading
Copy link
Contributor

@galak galak left a comment

Need cleanup in the Kconfig.

Loading

gmarull added 10 commits Oct 25, 2021
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>
galak
galak approved these changes Oct 25, 2021
@galak galak merged commit a0bd3b0 into zephyrproject-rtos:main Oct 25, 2021
18 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment