Skip to content

Conversation

@gmarull
Copy link
Member

@gmarull gmarull commented Oct 14, 2021

This PR adds an initial implementation of a pinctrl driver for nRF based on the pinctrl API proposal in #37572. The driver only supports the UART/E peripherals for now, but could be extended to cover the rest. A sample on how dynamic pinctrl can be used is also provided.

Note: it will be enabled and boards ported once all peripherals are ready.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

Looking really awesome. A few random comments. This is great work.

@gmarull gmarull force-pushed the pinctrl-nrf branch 3 times, most recently from 77dc566 to e124ffc Compare November 12, 2021 14:39
@gmarull gmarull force-pushed the pinctrl-nrf branch 2 times, most recently from d101181 to 8e5fc9e Compare November 15, 2021 10:27
@gmarull gmarull force-pushed the pinctrl-nrf branch 3 times, most recently from cbccaed to 67f3ba1 Compare November 15, 2021 11:54
Comment on lines 9 to 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we ever settle on the above style versus this one?

			psels = <NRF_PSEL(UART_TX, 0, 29)
				 NRF_PSEL(UART_RX, 0, 11)>;

I seem to recall @carlescufi had an opinion on this, but I can't remember what it was.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think nothing was agreed, but Linux seems to use

			psels = <NRF_PSEL(UART_TX, 0, 29)>,
				<NRF_PSEL(UART_RX, 0, 11)>;

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear since I am going on vacation, I don't care personally. I just wanted to bring it up.

@gmarull gmarull force-pushed the pinctrl-nrf branch 2 times, most recently from 2949a29 to 4d3bad0 Compare November 18, 2021 16:52
@joerchan
Copy link
Contributor

@gmarull Can this be used so that the application core can get the GPIO pins assigned to the network on nrf53?
The application core needs to know these pins because it has to grant the network core access to these pins. At the moment the UART pins are now hardcoded instead of from the device tree.
https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/arm/nrf5340dk_nrf5340/nrf5340_cpunet_reset.c#L17

This becomes more complex when TFM is enabled since the MCU selection has to be done in the secure processing environment.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

I would like to see 8e64dd0#r752354973 resolved at some point, but I don't consider it a blocker and it could be fixed in a follow-up PR.

I'm going on vacation so I'm going to approve this as from my perspective it looks great.

@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Nov 20, 2021

@gmarull Can this be used so that the application core can get the GPIO pins assigned to the network on nrf53?

This idea came from me, just for context (so if it's a bad one, please don't blame @joerchan 😉 ). I was thinking maybe dynamic pin control could be used for this purpose from the board files. That would potentially let us put the pinctrl nodes into BOARD.dts and let the user override them.

@joerchan
Copy link
Contributor

The problem is really about getting the PIN assignment from the network core. For unique use-cases like MPSL FEM this is not so difficult, as the same DTS node can be included in both cores without issue. (for more context see here: #40513 (comment)).
The problem with UART0 is that there is a node for it in both cores.
Just a quick look it looks like it might be easier to group the pin assignments here, but put them under a different node, like net-uart0 for example.

Sorry if I'm causing noise in this PR. We can start a discussion about this in another place.

gmarull and others added 9 commits November 26, 2021 11:44
Add initial support for nRF pin controller driver. The implementation in
this patch does not yet support any peripheral. Only states
representation and basic driver functionality is introduced.

Note:
The nrf_pin_configure function has been marked as __unused since it may
not be used in certain scenarios until all peripherals are supported by
the pinctrl driver. For example, if only UART/E is supported but the
board does not enable UART, the function will never get called. However,
that board will likely have other peripherals that will gain support in
the future.

Thanks to Marti Bolivar for bindings documentation.

Co-authored-by: Marti Bolivar <[email protected]>
Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add support for configuring UART/UARTE peripheral pins.

Co-authored-by: Andrzej Głąbek <[email protected]>
Signed-off-by: Gerard Marull-Paretas <[email protected]>
This patch adds support for the new pinctrl API to the UARTE driver. The
old pin property based solution is still kept so that users have time to
transition to the new model.

Notes:

- Some build assertions cannot be performed since the driver does not
  have direct access to pin settings anymore. As a result user will not
  be notified if HWFC is enabled but RTS/CTS pins are not configured.
- Hardware flow control can be enabled regardless of pin configuration,
  it is now up to the user to configure RTS/CTS pins in DT.
- Some RX enable checks that were performed using pin information has
  been replaced with a DT property that informs if RX is enabled or not.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
This patch adds support for the new pinctrl API to the UART driver. The
old pin property based solution is still kept so that users have time to
transition to the new model.

Notes:

- A new property to disable RX has been introduced: disable-rx. It is no
  longer possible to do it automatically depending on pin information,
  since it's not available when using pinctrl.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
This script can be used to automatically migrate the Devicetree files of
nRF-based boards using the old <signal>-pin properties to select
peripheral pins. The script will parse a board Devicetree file and will
first adjust that file by removing old pin-related properties replacing
them with pinctrl states.  A board-pinctrl.dtsi file will be generated
containing the configuration for all pinctrl states. Note that script
will also work on files that have been partially ported.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add a unit test that allows to validate if pin control configuration
stored in Devicetree is parsed correctly.

nRF boards used to run this test should add the pinctrl:nrf tag.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Make nrfx GPIO driver part of the PRE_KERNEL_1 initialization stage. As
a result, the GPIO driver can now be initialized before UART if
required, a device that is also initialized during PRE_KERNEL_1.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add a sample that shows how dynamic pin control can be useful to remap a
peripheral to a different set of pins.

NOTE: default pin configuration is provided for the nRF52840-DK board.
Such configuration will be removed once all nRF-based boards are ported
to pinctrl.

Thanks to Francesco for documentation suggestions.

Co-authored-by: Francesco D. Servidio <[email protected]>
Signed-off-by: Gerard Marull-Paretas <[email protected]>
Since the addition of pinctrl the common folder is added to the Zephyr
include path. This can be re-used to place the soc_nrf_common.h header
in the common folder and include it directly, without the need of
relative paths.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
@github-actions github-actions bot added area: API Changes to public APIs area: Devicetree Binding PR modifies or adds a Device Tree binding area: Documentation area: Samples Samples labels Nov 26, 2021
Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

Thank you for all the effort put into this PR.

@carlescufi carlescufi merged commit a8c9347 into zephyrproject-rtos:main Nov 26, 2021
@gmarull gmarull deleted the pinctrl-nrf branch April 24, 2023 09:24
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: Documentation area: Samples Samples DNM This PR should not be merged (Do Not Merge)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants