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

new pinctrl API #37621

Closed
wants to merge 14 commits into from
Closed

new pinctrl API #37621

wants to merge 14 commits into from

Conversation

@galak
Copy link
Contributor

@galak galak commented Aug 11, 2021

No description provided.

@galak
Copy link
Contributor Author

@galak galak commented Aug 11, 2021

@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))

Loading

@gmarull
Copy link
Collaborator

@gmarull gmarull commented Aug 12, 2021

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:

  1. call deinit
  2. configure new states (pin configuration for each state supported by the device driver, e.g. default and sleep)
  3. 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.

Loading

@gmarull
Copy link
Collaborator

@gmarull gmarull commented Aug 12, 2021

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:

* @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;

Loading

Copy link
Member

@mbolivar-nordic mbolivar-nordic left a comment

Just a couple of comments from an initial look.

Loading

*/

/**
* @typedef pinctrl_soc_pins_t
Copy link
Member

@mbolivar-nordic mbolivar-nordic Aug 12, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@galak galak Aug 13, 2021

Choose a reason for hiding this comment

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

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.

Loading

},

#define PINCTRL_DT_SPEC_GET(node, n) \
{ UTIL_LISTIFY(DT_PROP_LEN(node, pinctrl_##n), NXP_KINETIS_PIN_ELEM, node, n) }
Copy link
Member

@mbolivar-nordic mbolivar-nordic Aug 12, 2021

Choose a reason for hiding this comment

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

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.

Loading

@@ -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]);
Copy link
Collaborator

@gmarull gmarull Aug 18, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@galak galak Aug 18, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Collaborator

@gmarull gmarull Aug 18, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@galak galak Aug 18, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Collaborator

@gmarull gmarull Aug 18, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor Author

@galak galak Aug 18, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Collaborator

@gmarull gmarull Aug 18, 2021

Choose a reason for hiding this comment

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

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.

Loading

galak and others added 14 commits Aug 31, 2021
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>
}
#endif

int z_impl_pinctrl_pin_configure(const pinctrl_soc_pins_t *pin_spec)
Copy link
Collaborator

@gmarull gmarull Sep 2, 2021

Choose a reason for hiding this comment

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

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 ?

Loading

Copy link
Member

@erwango erwango Sep 2, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor Author

@galak galak Sep 2, 2021

Choose a reason for hiding this comment

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

than whatever reg is needed should be placed in pinctrl_soc_pins_t

Loading

Copy link
Collaborator

@gmarull gmarull Sep 2, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor Author

@galak galak Sep 2, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Collaborator

@gmarull gmarull Sep 2, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member

@erwango erwango Sep 2, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@galak galak Sep 2, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor Author

@galak galak Sep 2, 2021

Choose a reason for hiding this comment

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

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?

Loading

Copy link
Collaborator

@gmarull gmarull Sep 2, 2021

Choose a reason for hiding this comment

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

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

Loading

@galak galak removed the dev-review label Oct 6, 2021
@galak galak closed this Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants