new pinctrl API #37621

galak · 2021-08-11T17:57:30Z

No description provided.

galak · 2021-08-11T22:44:32Z

@mbolivar-nordic need to see how we handle pinctrl-names property as it doesn't follow the pattern other FOO-names.

From a usage point of view, having PINCTRL_DT_SOC_PINS_DECLARE_BY_NAME just have a means of mapping NAME to IDX would be the easiest.

So we could do something like:

#define PINCTRL_DT_SOC_PINS_DECLARE_BY_NAME(node, name) \
PINCTRL_DT_SOC_PINS_DECLARE(node, PINCTRL_DT_NAME_TO_IDX(node, name))

gmarull · 2021-08-12T07:23:46Z

I see a problem with this approach when it comes to runtime support (considering we want to have such feature).

Pin configuration can usually be changed only if a device is turned off. Let's assume for a moment that we had init and deinit support for devices. Then, an application that wants to change pins at runtime (e.g. the CircuitPython case) would do:

call deinit
configure new states (pin configuration for each state supported by the device driver, e.g. default and sleep)
call init

With the approach proposed in this PR, step (2) can't be performed. An application can configure pins on its own, but I assume this is only useful if the application is directly managing the device, that is, without using a Zephyr driver. As an example: a device driver that implements power management and uses pinctrl would overwrite any pinctrl change made by the application as soon as it runs PM hooks. This is difficult to solve if the driver stores its own pinctrl settings internally.

gmarull · 2021-08-12T16:52:09Z

Another note:

In some cases such as nRF or STM32F1 it is necessary to know the device register address to configure pins (some pin settings are stored in each device registers or there are actions to be taken that are device dependent).

uint32_t base:

zephyr/drivers/pinmux/pinmux_stm32.c

Lines 83 to 90 in 00bfac0

    
            * @param base device base register value 
        
            * 
        
            * @return 0 on success, -EINVAL otherwise 
        
            */ 
        
           int stm32_dt_pinctrl_configure(const struct soc_gpio_pinctrl *pinctrl, 
        
           			       size_t list_size, uint32_t base) 
        
           { 
        
           	uint32_t pin, mux;

mbolivar-nordic

Just a couple of comments from an initial look.

mbolivar-nordic · 2021-08-12T18:28:27Z

include/drivers/pinctrl.h

+ */
+
+/**
+ * @typedef pinctrl_soc_pins_t


The naming here feels confused.

This looks like it should be called pinctrl_dt_spec_t because PINCTRL_DT_SPEC_GET expands to an initializer for this type.

But then the next patch adds struct pinctrl_pins_dt_spec, which contains one of these plus a num_pins.

I think that in general foo_dt_spec should be initialized by FOO_DT_SPEC_GET. So probably PINCTRL_DT_SPEC_GET should be renamed to PINCTRL_SOC_PINS_GET.

agreed the names are a bit of a mess and need cleaning up. Trying to have the drivers see SPEC similar to what we have for GPIO, SPI, etc.

mbolivar-nordic · 2021-08-12T18:29:57Z

soc/arm/nxp_kinetis/common/soc_pinctrl.h

+	},
+
+#define PINCTRL_DT_SPEC_GET(node, n) \
+{ UTIL_LISTIFY(DT_PROP_LEN(node, pinctrl_##n), NXP_KINETIS_PIN_ELEM, node, n) }


I keep seeing the length of these being taken with ## pasting.

My rule of thumb is if users have to paste, it's usually a gap in the DT API.

So I think we should add DT_NUM_PINCTRLS(node_id, pc_idx) and DT_INST_NUM_PINCTRLS(inst, pc_idx) to #37644 and use them here.

gmarull · 2021-08-18T16:09:37Z

drivers/serial/uart_mcux.c

@@ -40,6 +42,8 @@ static int uart_mcux_configure(const struct device *dev,
 	uint32_t clock_freq;
 	status_t retval;

+	pinctrl_set_pin_state(config->pincfg.states[0]);


Using states by index is fragile:

Indexes have a meaning now (0 means default state in this case)

This won't work even though DT representation is correct (assume this driver had PM support):

pinctrl-0 = <config for sleep>; pinctrl-1 = <config for default>; pinctrl-names = "sleep", "default";

If the application needs to reconfigure pins, it also needs to know how driver expects them to be sorted.

Sure, I wasn't suggesting not supporting by name, just that the index solution would be the lowest level.

I think having 2 ways of setting states not being fully equivalent (index vs. id) will make runtime pinctrl support fragile. I'd personally not take this path for such little gain: number of states is low (1, 2, 3...) and they can use an integer based identifier, so lookup overhead is low.

We can't require that pinctrl-names is set so we have to allow for cases in which only pinctrl-N is utilized.

-1 on that, I think we need to make usage consistent, otherwise we'll end up with a mixed usage depending on what developer thinks it is more "efficient" without accounting for side-effects such as usability of the runtime case.

-1 on that, I think we need to make usage consistent, otherwise we'll end up with a mixed usage depending on what developer thinks it is more "efficient" without accounting for side-effects such as usability of the runtime case.

This goes back to the design point of "upstream" devicetree bindings. As we start seeing more and more Cortex-A SoCs being supported, we are going to have more devicetree(s) that cross between Linux and Zephyr. As such we can't impose rules like this that don't exist already.

This is really about a design decision, not an imposed rule. I don't see how the states concept works without naming pinctrl-N. It's what you need to do in Linux, for example. If the API doesn't have a clear design and allows for these sort of "shortcuts" I don't think we can expect a consistent/portable behavior. Whether this is acceptable or not is another thing.

from Gerard Marull-Paretas PR

Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com> Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Add pinctrl_sam driver and required infrastructure. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

Update driver to use new pinctrl APIs to setup pins. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Update driver to use new pinctrl APIs to setup pins Signed-off-by: Kumar Gala <kumar.gala@linaro.org

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

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. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

Add support for managing UARTE peripheral pins. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

stub out/comment out so the files builds Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

This patch replaces driver specific pin configuration code with the new pinctrl generic API. Notes: - uart driver has not been ported yet (this is an RFC!) - 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. - 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 <gerard.marull@nordicsemi.no>

tweaks to uarte driver to build with pinctrl API Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

This patch changes old pin based configuration to the state based pinctrl configuration. All pin selection and GPIO configuration are now described in devicetree. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

gmarull · 2021-09-02T07:20:13Z

drivers/pinctrl/pinctrl_nrf.c

+}
+#endif
+
+int z_impl_pinctrl_pin_configure(const pinctrl_soc_pins_t *pin_spec)


In the case of Nordic (see pinctrl->reg) and STM32F1 (see uint32_t base) the pinctrl_soc_pins_t will not be enough to configure a pin, since knowledge about the peripheral is also required. In case of Nordic, doing the configuration at once per peripheral is more efficient (due to the required "switch/case"). In case of STM32F1 I'm not sure if it would be possible, @erwango ?

In the case of Nordic (see pinctrl->reg) and STM32F1 (see uint32_t base) the pinctrl_soc_pins_t will not be enough to configure a pin, since knowledge about the peripheral is also required.

True. Thanks for raising it.

In case of Nordic, doing the configuration at once per peripheral is more efficient (due to the required "switch/case"). In case of STM32F1 I'm not sure if it would be possible, @erwango ?

Configuring all pins at once (using an array) would also save some operations in F1 case

than whatever reg is needed should be placed in pinctrl_soc_pins_t

reg is not per-pin, but per pin set. pinctrl_pin_configure configures a single pin.

Not sure I follow that comment. If the reg varies between 2 pins than its effectively per-pin.

reg does not vary between 2 pins of the same peripheral.

For F1 case (and NRF as I understand) peripheral reg is required for proper pin configuration.
As a consequence, all pins configured for a peripheral have same reg value.

gotcha. So I think we should add void * for user_data. Suggestions on what to call it user_data doesn't seem right.

reg does not vary between 2 pins of the same peripheral.

Understood, how will this work for the runtime case? The user will end having to know what "reg" value to pass?

I have updated #37572 with a PoC (including other changes)

galak requested review from mnkp, gmarull and henrikbrixandersen August 11, 2021 17:57

github-actions bot added area: API Changes to public APIs area: Boards platform: NXP NXP labels Aug 11, 2021

This was referenced Aug 11, 2021

Introduce new pinctrl API (variant b) #35847

Closed

nxp: Convert mcux uart to use pinctrl API #36146

Closed

galak requested a review from carlescufi August 11, 2021 17:59

MaureenHelm self-requested a review August 11, 2021 18:02

nandojve self-requested a review August 11, 2021 18:13

galak requested a review from mbolivar-nordic August 11, 2021 22:39

galak force-pushed the pinctrl-api branch from ebe92ae to 30b50c3 Compare August 11, 2021 22:51

mbolivar-nordic reviewed Aug 12, 2021

View reviewed changes

mbolivar-nordic mentioned this pull request Aug 13, 2021

DT: new pinctrl helpers #37644

Merged

galak force-pushed the pinctrl-api branch 2 times, most recently from 1f484e4 to 19c8d2a Compare August 13, 2021 19:06

galak mentioned this pull request Aug 17, 2021

drivers: pinctrl: new state based API proposal #37572

Merged

galak force-pushed the pinctrl-api branch from 19c8d2a to 3ce7937 Compare August 18, 2021 15:37

github-actions bot added area: Devicetree area: Devicetree Tooling PR modifies or adds a Device Tree tooling labels Aug 18, 2021

galak force-pushed the pinctrl-api branch 2 times, most recently from aeceaab to 510c3d6 Compare August 18, 2021 15:55

gmarull reviewed Aug 18, 2021

View reviewed changes

galak force-pushed the pinctrl-api branch from 510c3d6 to 12c96d6 Compare August 18, 2021 18:48

github-actions bot added the area: Devicetree Binding PR modifies or adds a Device Tree binding label Aug 18, 2021

galak added the dev-review To be discussed in dev-review meeting label Aug 18, 2021

galak and others added 14 commits August 31, 2021 12:20

ADD pinctrl-device.yaml

ad60bff

from Gerard Marull-Paretas PR

api: Add pinctlr driver API

0208ff9

Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com> Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: Add pinctrl_sam driver

fc30824

Add pinctrl_sam driver and required infrastructure. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

drivers: serial: update usart_sam driver to use new pinctrl API

71bbbd8

Update driver to use new pinctrl APIs to setup pins. Signed-off-by: Piotr Mienkowski <piotr.mienkowski@gmail.com>

pinmux: mcux: Add support for ...

c10b2dc

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: serial: Convert uart_mcux.c to new pinctrl API

433c841

Update driver to use new pinctrl APIs to setup pins Signed-off-by: Kumar Gala <kumar.gala@linaro.org

boards: arm: nxp: use driver to set uart pins

e3630b8

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

nrf: add soc_pinctrl.h

f3b2e29

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: pinctrl: nrf: add support for uarte peripheral

43a02a6

Add support for managing UARTE peripheral pins. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

pinctrl: HACK pinctrl_nrf.c so it builds

2e7026d

stub out/comment out so the files builds Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

drivers: serial: NRF UARTE: changes to build with pinctrl API

1bb86a1

tweaks to uarte driver to build with pinctrl API Signed-off-by: Kumar Gala <kumar.gala@linaro.org>

galak force-pushed the pinctrl-api branch from 12c96d6 to 84d9588 Compare August 31, 2021 17:22

gmarull reviewed Sep 2, 2021

View reviewed changes

galak removed the dev-review To be discussed in dev-review meeting label Oct 6, 2021

galak closed this Oct 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new pinctrl API #37621

new pinctrl API #37621

galak commented Aug 11, 2021

galak commented Aug 11, 2021

gmarull commented Aug 12, 2021

gmarull commented Aug 12, 2021

mbolivar-nordic left a comment

mbolivar-nordic Aug 12, 2021

galak Aug 13, 2021

mbolivar-nordic Aug 12, 2021

gmarull Aug 18, 2021

galak Aug 18, 2021

gmarull Aug 18, 2021

galak Aug 18, 2021

gmarull Aug 18, 2021

galak Aug 18, 2021

gmarull Aug 18, 2021

gmarull Sep 2, 2021 •

edited

erwango Sep 2, 2021

galak Sep 2, 2021

gmarull Sep 2, 2021

galak Sep 2, 2021

gmarull Sep 2, 2021

erwango Sep 2, 2021

galak Sep 2, 2021

galak Sep 2, 2021

gmarull Sep 2, 2021

new pinctrl API #37621

new pinctrl API #37621

Conversation

galak commented Aug 11, 2021

galak commented Aug 11, 2021

gmarull commented Aug 12, 2021

gmarull commented Aug 12, 2021

mbolivar-nordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarull Sep 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarull Sep 2, 2021 •

edited