Skip to content

Conversation

@galak
Copy link
Contributor

@galak galak commented Nov 6, 2019

Some idea's on how to represent the pinmux info in DTS on Kinetis to allow us to auto-generate the info from DTS.

Need to see how we'd convey a pin is used for GPIO and how we'd possibly pass through GPIO config to pinmux (if at all).

@galak galak requested a review from MaureenHelm as a code owner November 6, 2019 22:37
@galak galak added the DNM This PR should not be merged (Do Not Merge) label Nov 6, 2019
@galak galak requested review from erwango, mnkp and ulfalizer November 6, 2019 22:38
@galak galak changed the title NXP Kinetis DTS Pinmux [WIP] NXP Kinetis DTS Pinmux rework Nov 6, 2019
@galak galak requested a review from b0661 November 6, 2019 22:40
Copy link
Contributor

@ulfalizer ulfalizer Nov 6, 2019

Choose a reason for hiding this comment

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

Think a pinmux property like mentioned in Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt in Linux could be used here?

The purpose seems to be to avoid having a ton of nodes.

Copy link
Member

Choose a reason for hiding this comment

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

Having a look into the datasheet I also think it would be most appropriate to use pinmux property for this family.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to use the properties documented in Generic pin multiplexing node content in pinctrl-bindings.txt, for consistency.

@galak galak added the dev-review To be discussed in dev-review meeting label Nov 7, 2019
galak added 2 commits November 6, 2019 21:23
Add child-binding to represent the pin config paramaters for given
functions on the SoC.

Signed-off-by: Kumar Gala <[email protected]>
Convert UART0 on FRDM-K64f (plus a few other UARTs) to new pinmux
definition.

Signed-off-by: Kumar Gala <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Having a look into the datasheet I also think it would be most appropriate to use pinmux property for this family.

&uart0 {
status = "okay";
current-speed = <115200>;
pinctrl-0 = <&uart0_rx_ptb16 &uart0_tx_ptb17>;
Copy link
Member

Choose a reason for hiding this comment

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

To be able to fully assess the proposed implementation we would need to generate defines based on pinctrl-0 property. That is, all the data hidden within uart0_rx_ptb16 node should be made available via the UART defines DT_NXP_KINETIS_UART_4006A000_*.

@mnkp
Copy link
Member

mnkp commented Jan 6, 2020

The method of storing pinctrl configuration data chosen by Linux DTS is quite different to the way other subsystems, e.g. gpio are configured. pinctrl data are stored as subnodes of pinctrl, while data of other subsystems, e.g. gpio are stored in the parent node. To illustrate the difference:

pinctrl:

	pinctrl-0 = <&uart0_pins_a>;

gpio:

       cs-gpios = <&gpio0 31 GPIO_ACTIVE_LOW>;

In the latter case we can generate data directly from the property.

The examples come from Linux DTS and not this PR since I would like describe the concept as described and implemented by Linux.

The uart0_pins_a node contains all the data required to configure uart0 pins. It may look like this:

uart0_pins_a: uart0@0 {
        allwinner,pins = "PB22", "PB23";
        allwinner,function = "uart0";
        allwinner,drive = <SUN4I_PINCTRL_10_MA>;
        allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
};

Such approach has some limitations. It seems appropriate if there exists only one allowed pin assignment, e.g. UART0 RX signal can be connected to one pin only, e.g. PB22. It's not that practical when the signal can be connected to several different pins, e.g. one of PB22, PA1, PC18. Which is a typical scenario. In this case the soc dtsi file would provide one default and arbitrary configuration of uart0_pins_a node which then had to be corrected by some overlay file to adjust pin configuration to the actual board.

To work around this issue we could do what this PR proposes, which is define pinctrl-0 properties as

	pinctrl-0 = <&uart0_rx_ptb16 &uart0_tx_ptb17>;

one pin per sub-node and then within pinctrl node provide a sub-node for every pin / mux (pin function) combination. On K64 platform we would have nodes uart0_rx_pta1, uart0_rx_pta15, uart0_rx_ptb16, `uart0_rx_ptd6 to describe all 4 possible ways to connect UART0 RX signal to a pin. Such approach should work but is non-standard, i.e. this is not how Linux DTS recommends to do it. Also, I'm not sure if it is going to be practical since on SoCs which have flexible pin muxing, i.e. allow connecting peripheral signal to many pins the amount of pinctrl DTS sub nodes that would need to be defined could be huge. And last but not least, it will not be easy for a user to determine which nodes were defined and could be used in a board overlay file.

If we want to go a non-standard way it may be easiest to do pinctrl configuration the same way we do gpios. E.g. we could have rx-pins property in board DTS file which could look as follows

	rx-pins = <&_PINCTRL_ _PINCTRL_PIN_ _PINCTRL_MUX_ _PINCTRL_FLAGS_>;

e.g.

	rx-pins = <&pinctrlb 16 3 PINCTRL_PULL_UP>;

the pin / mux definition could be provided via a macro from an easy to search include file e.g.

	rx-pins = <&pinctrlb K64_PINS_UART0_RX_PTB16 PINCTRL_PULL_UP>;

We already have a few families that adopted similar approach to define pin configuration in the current Zephyr tree, namely: Nordic, TI, ESP32.

There are also families like Silicon Labs Gecko where it is not possible to provide pin muxing configuration as proposed by Linux DTS since they don't have pinctrl node. Pinmuxing is performed directly by a peripheral itself.

@b0661
Copy link
Contributor

b0661 commented Jan 6, 2020

To work around this issue we could do what this PR proposes, which is define pinctrl-0 properties as

pinctrl-0 = <&uart0_rx_ptb16 &uart0_tx_ptb17>;

one pin per sub-node and then within pinctrl node provide a sub-node for every pin / mux (pin function) combination.
Such approach should work but is non-standard, i.e. this is not how Linux DTS recommends to do it.

Can you elaborate? I though this is one standard compliant way to define pin control.

Also, I'm not sure if it is going to be practical since on SoCs which have flexible pin muxing, i.e. allow connecting peripheral signal to many pins the amount of pinctrl DTS sub nodes that would need to be defined could be huge.

"huge" is only a problem if you expect all possible pin/mux/drive/bias/... combinations to be predefined by a pinctrl state node. A user can create an own pinctrl state node and overload the "standard" DTS.

pinctrl-0 = <&my_uart0_pins>;

In my view the policy should be:

  1. to provide pinctrl state nodes for common usage scenarios related to the boards supported by Zephyr AND
  2. explain and document how to do an own pinctrl state definition.

And last but not least, it will not be easy for a user to determine which nodes were defined and could be used in a board overlay file.

Document! Read the documentation!

I understand this is about ease of use - but for complex or "huge" things a user can be expected to read the documentation.

If we want to go a non-standard way

Pinctrl is very versatile, documented, covered by some presentations and talks. Creating one more way to do the same thing should only be done if it is not the same thing (aka. the concept of a pin controller does not fit to the SoC).

@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR area: Device Tree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Jun 29, 2020
@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 1, 2020
@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 1, 2020
@galak galak removed area: Boards dev-review To be discussed in dev-review meeting labels Jul 9, 2020
@nashif nashif added the Stale label Sep 10, 2020
@github-actions github-actions bot closed this Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Boards area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: Pinmux DNM This PR should not be merged (Do Not Merge) has-conflicts Issue/PR has conflicts with another issue/PR platform: NXP NXP Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants