[RFC][DNM]Pinmux configuration code generated from Device tree #5021

erwango · 2017-11-16T16:14:10Z

GOAL:
Aim of this PR is to enable generation of pinmux configuration code based on information extracted
from device tree files.
At compilation, board device tree is parsed and a pinmux dedicated header file is generated
under ${PROJECT_BINARY_DIR}/include/generated.
This file holds a pinmux array. It is included by board pinmux code and parsed at pinmux init.
File name and array name are based on pinmux node compatible.

STATUS:

Supports single and mutli pinmux instances
Currently tested on disco_l475_iot1 (single instance pinmux) and frdm_k64f (multi instance pinmux)
Tested on samples/hello_world (both) and samples/sensors/hts221 (disco_l475_iot1)
Limited to UART and I2C nodes (because only thse nodes contrain pinmux information on above boards dts files)

Preliminary work:
Some preliminary work was needed to enable this:

extract_dts_include.py: generate struct dictionary that store all decive nodes information into structs (@agross-linaro)
yaml: Add yaml pinmux base file
dts: nxp: add label to pinmux nodes
nxp: move PORT_PCR_MUX to pinmux_mcux driver

TODO:

So far, extract_dts_includes flies knows it deals with a pinmux node by checking "pin" string is contained in node yaml description. Instanciate a yaml information to store node type as "pinmux", so this type could be checked against.

erwango · 2017-11-21T15:49:59Z

CI faces an error in my code when running sample/synchronisation for target sam_e70_xplained.
I don't face this issue in my environment (tested on python3.4.X and 3.5.3)..

SebastianBoe · 2017-11-21T16:01:04Z

@erwango : Try rebasing onto the latest version of master. CI rebases this branch onto master before testing it.

EDIT: Oh, arm base branch, well, rebase onto arm

erwango · 2017-11-22T08:02:27Z

@SebastianBoe : Thanks for the hint! Unfortunately, I'm already on top of arm branch..

EDIT: BUT... I had a try anyway (rebase on master) and I face the error... So even for arm base branches, rebase is done on top of master.

SebastianBoe · 2017-11-22T09:15:56Z

recheck

erwango · 2017-11-30T17:07:13Z

Don't know if something changed in CI recently, but I now get following error:
"author email (Erwan Gouriou [email protected]) needs to match one of the signed-off-by entries."
Checked all the commits but couldn't find the issue

pfalcon · 2017-11-30T17:28:45Z

@nashif pushed something like that recently, yeah.

E.g. this commit bf4def7 has: Signed-Off-By: Erwan Gouriou <[email protected]> (count u's in email's surname ;-) ).

MaureenHelm · 2017-11-30T17:31:49Z

boards/arm/frdm_k64f/pinmux.c

-	pinmux_pin_set(porte, 25, PORT_PCR_MUX(kPORT_MuxAlt5)
-					| PORT_PCR_ODE_MASK);
+	pinmux_pin_set(porte, 24, kPORT_MuxAlt5 | PORT_PCR_ODE_MASK);
+	pinmux_pin_set(porte, 25, kPORT_MuxAlt5 | PORT_PCR_ODE_MASK);


This won't work whenever we need to set bits outside the MUX field, in this case ODE.

@MaureenHelm: Ho right! 'Find all/replace all/Don't think'...
So we need to have all bindings available in include/dt_bindings with a constraint that dt parser only accept basic #define. enum/struct/.. could not be used.
Providing a new version using set of bindings based on PORT_PCR...

@MaureenHelm : Happy new year and best wishes to you!
Would you mind have a new look to this PR and check if you have some more comments?

Add pinmux.yaml file. Get it included by existing pinctrl yaml bindinds. Signed-Off-By: Erwan Gouriou <[email protected]>

To enable dts based pinmux code generation, add label property to pinmux nodes on all nxp dtsi files Signed-Off-By: Erwan Gouriou <[email protected]>

During yaml collapse step, convert inherited 'id' key to 'gen_type'. With this new 'gen_type', it's is more easy to apply common treatment to all bindings that include the same base yaml file but might not have similar bindings/constraint naming convention. Signed-off-by: Erwan Gouriou <[email protected]>

This commits provide pinctrl initialisation code generation. A new -s option is added to the script which allows code generation in header files located in outdir/<board>/zephyr/include/generated Pinctrl generation is purely based on dts and done when soc dts provides a 'pinmux' node. The script is called with -s option by default in dts.cmake and has no effect if dts pinctrl driver generation is not prepared Signed-off-by: Erwan Gouriou <[email protected]>

Include generated file st_stm32_pinmux_init.h in board pinmux.c. Generated file provides st_stm32_pinmux_pinconf array. This array holds pinmux information for UART and I2C. It is parsed to configure and initialize pins configured in board dts. Signed-Off-By: Erwan Gouriou <[email protected]>

Include generated file nxp_kinetis_pinmux_init.h in board pinmux.c. Generated file provides nxp_kinetis_pinmux_instances array. This array holds pinmux instances information for UART. It is parsed to configure and initialize pins configured in board dts. Signed-off-by: Erwan Gouriou <[email protected]>

In order to be able to configure pin control registers using device tree properties, introduce a pinctrl binding providing defines compatible with current use of pinmux_pin_set function. Provided set of defines is equivalent to PORT_PCR_xxx set provided in nxp hal. Signed-off-by: Erwan Gouriou <[email protected]>

ydamigos

I tested also both UART and I2C on 96b_carbon, and worked fine. LGTM, just some conflicts to be fixed.

mbolivar

I appreciate being asked for review, as I'm excited about the direction this is going in.

Probably due to my lack of understanding of device tree, however, I'm having a hard time understanding what is going on beyond the high level goal.

I don't mean these comments to block the series. I'm just offering feedback on why I find DTS in Zephyr confusing and ideas on how to clear it up. Any explanations appreciated!

mbolivar · 2017-12-20T18:15:09Z

scripts/dts/extract_dts_includes.py

@@ -18,6 +19,8 @@
 aliases = {}
 chosen = {}
 reduced = {}
+defs = {}


I'm having an increasingly hard time following along with this script. The combination of the primary datatype being a nested dictionary with string keys, the reliance on global variables, and the lack of comments/docstrings makes it hard to understand (and makes it hard to understand changes).

When I was looking at this script for the first time, I gave up on reading it and and just ran it in PDB to printline various pieces of state as it went along.

Is there any way this series could start to improve this situation rather than keep going along the same lines? I'm thinking specifically about starting to introduce some classes which define datatypes with well-specified properties, methods, etc., making it into a bit more of a library with a main() routine.

@mbolivar: I can only ack your observations.
Agreed that current version the script is not a state of the art piece of python software but rather a simple script doing job expected from it. Aim of this series was primarily focused on adding functionality to it.
But it would indeed make lot of sense to increase a bit the standards on this script, as I'm sure it could simplify some processing, make it easier to use and more powerful. To do this though, I humbly admit I could use some help, as I'm still a python apprentice. Well, for now, I can at least provide more comments on the code.

Before doing this, maybe one point would be to know where we're going and what functionality is expected from the script. If we agree that long term goal is to generate code that could be used for driver initialisation (driver instances and their configuration data), there are several ways to do that.
I see two trends currently:

Initial and current 'defs' trend, generating pure #defines with machine computable names (which is mostly available today). Then post process these definitions so driver can get rid of Kconfig stuff related to instances/config/... This part is currently an unknown (to me at least)

New 'struct' trend introduced here, that allows to store information from dts/yaml in a struct dict holding parsed information and hence directly ready to generate whatever code is needed (an example of use of such struct dict is here: https://github.com/erwango/zephyr/commits/dts_struct_cleanup generating directly uart driver init code per instance, skipping the #define generation part )

If a rework is required, this should be decided first.
@galak, @nashif, @agross-linaro : maybe you have something in mind already?

Before doing this, maybe one point would be to know where we're going and what functionality is expected from the script.

@erwango I agree this is the main point.

My concern is Zephyr relies on DT more and more (e.g. increasingly complex code generation in this series), but has an important DT script that is hard to understand and modify.

If this is the "last" feature for this script, maybe OK, but if we're going farther from here, this seems worrisome.

But it would indeed make lot of sense to increase a bit the standards on this script, as I'm sure it could simplify some processing, make it easier to use and more powerful. To do this though, I humbly admit I could use some help, as I'm still a python apprentice. Well, for now, I can at least provide more comments on the code.

Totally fair, and I want to repeat I'm not trying to block this series with my comments.

Totally fair, and I want to repeat I'm not trying to block this series with my comments.

No problem, this RFC is meant to be discussed. DTS based code generation will have some impacts throughout zephyr code, it deserves some discussion beforehand.

No problem, this RFC is meant to be discussed. DTS based code generation will have some impacts throughout zephyr code, it deserves some discussion beforehand.

On that note: looking around for Python representations of device trees, I was surprised and pleased to see that Zephyr developer @superna9999 already has a library for managing flattened device trees in Python!

https://github.com/superna9999/pyfdt

It has a great looking set of classes for representing nodes and properties which has thorough documentation:

https://github.com/superna9999/pyfdt/blob/master/pyfdt/pyfdt.py

@superna9999, do you think this is something Zephyr could easily use instead of its current scripts for managing DTS files? I know FDT is of course a different format, but perhaps it's not so hard to either have Zephyr generate a DTB, or perhaps have the library parse DTS?

@mbolivar Hi, thanks for pointing it !

Actually it was already evaluated in the early days on DT in zephyr, but since pyfdt does not have a dts compiler, it needed a painful 'dtc' compilation to dtb then an import into pyfdt.

But then a simple dts parser was written and pyfdt was ditched, but then I always had in mind to use this parser as a pyfdt frontend to compile dts files... even the licence matches !

It could be a good base to import in zephyr code base as a foundation to a brand new code generator from DT.

(it will need some slight work to make it 100% conform to DT spec, the work is WIP in next branch BTW)

@mbolivar , @superna9999, I had a quick look to pyfdt.

While I still agree on the observation that current script should be deeply reworked or replaced for an easier maintenance and future evolutions, I'm not sure pyfdt is the good match, from a functional point of view, IMVHO.
It is definitely a good example, and I don't have better alternative to propose, but I think pyfdt provides a lot of functionalities but only a little that would be useful for code generation once dts is already available.
I'm not convinced we need to compile dts file to be able to parse it. devicetree.py converts .dts into a python dict object that is pretty sufficient for further parsing. Code generation part (what would need to be enhanced, ie 90% of the script) happens after this.

It is definitely a good example, and I don't have better alternative to propose, but I think pyfdt provides a lot of functionalities but only a little that would be useful for code generation once dts is already available.
I'm not convinced we need to compile dts file to be able to parse it. devicetree.py converts .dts into a python dict object that is pretty sufficient for further parsing. Code generation part (what would need to be enhanced, ie 90% of the script) happens after this.

I don't have plans to contribute heavily to this effort so I will happily defer to your opinion.

I would only say that I think the pyfdt library seems to provide good class definitions with good documentation and decent duck-typing for some of the key properties DT seems to rely on. This is a very beginner opinion, and so it's likely I've missed many important details.

I think the nested dicts that Zephyr is using now are very hard to understand, and very hard to typecheck, even at runtime. But this is not necessarily a problem if only a few people who all know what they are doing are interacting with the datatypes.

IMHO converting to dict will be limiting at some point, having a proper object model will get the code maintainable and understandable against the device tree standards.
Copying pyfdt may be a bad idea, but implementing a similar model maybe smart towards to goals we want to achieve with device tree support.
Basically, we are trying to do what is actually done in the linux kernel, in python, at compile time with a limited object representation.
At some point, we should restructure this script into submodules mapping each bindings, i.e. pinctrl, interrupt parsing, i2c bus, spi bus, flash partitions... from a correctly parsed device tree.

@superna9999 , @mbolivar : Sorry, I had missed your comments

IMHO converting to dict will be limiting at some point, having a proper object model will get the code maintainable and understandable against the device tree standards.

Ok, I will trust your experience on this

implementing a similar model maybe smart towards to goals we want to achieve with device tree support.

For sure

At some point, we should restructure this script into submodules mapping each bindings, i.e. pinctrl, interrupt parsing, i2c bus, spi bus, flash partitions... from a correctly parsed device tree.

On this point I'm not so sure. I think (hope) yaml files should provide enough information to have a nearly generic parsing so it's not needed to add a new submodule for each new binding. At least most people should be able to get things working by only adding a yaml file matching their binding. I know this is not 100% feasible, but I think this should be a target.

mbolivar · 2017-12-20T19:11:19Z

dts/bindings/pinctrl/nxp,kinetis-pinmux.yaml

@@ -5,28 +5,20 @@ version: 0.1
 description: >
    This is a representation of the Kinetis Pinmux node

+inherits:


I'm also having trouble understanding how "inherits" works right now. This makes it difficult for me to understand these changes. In particular, I don't understand the schema/semantics for bindings and how it relates to their inheritance structure.

In addition to reading extract_dts_includes.py and playing with it in the debugger, I've read this documentation: http://docs.zephyrproject.org/devices/dts/device_tree.html

I see how defines can get generated from a node's name (along with it's parent's names and addresses), and how the "compatible" constraint and "generation: define" control that.

But there are other things that are less clear to me here especially where inheritance is concerned. For example, the ADC binding for nxp,kinetis-adc16.yaml inherits from adc.yaml in the same way this file is being changed:

https://github.com/zephyrproject-rtos/zephyr/blob/master/dts/bindings/iio/adc/nxp%2Ckinetis-adc16.yaml

However, looking at the base adc.yaml (which corresponds to the pinmux.yaml being introduced in this patch), it defines a "category: required" clocks property:

https://github.com/zephyrproject-rtos/zephyr/blob/master/dts/bindings/iio/adc/adc.yaml

This is confusing because when looking at the dts for frdm_k64f, which has ADC nodes, those same nodes don't have any clocks property (I checked the build/zephyr/frdm_k64f.dts_compiled output as well just to make sure it wasn't in a magic overlay or something):

https://github.com/zephyrproject-rtos/zephyr/blob/master/boards/arm/frdm_k64f/frdm_k64f.dts
https://github.com/zephyrproject-rtos/zephyr/blob/master/dts/arm/nxp/nxp_k6x.dtsi

I would have expected extract_dts_includes.py to raise an error there, since it looks like it's trying to define a required property which is missing, but that doesn't seem to be happening, so I'm not sure what the relationship between parent and child is there (especially since I'm having trouble following extract_dts_includes.py in general).

I did read device_node.yaml.template as well, but that seems to be more descriptive than really enforcing a schema (the way that the pykwalify library is used with the sanitycheck test cases to enforce that the actual yaml files match the schema).

@mbolivar: You're totally right. I think your understanding is correct, problem is that field is not checked. I raised following issue to address this point: #5460
Don't hesitate to comment on this issue.

erwango · 2018-02-21T10:13:39Z

Accidentally closed - Re-opening

erwango requested a review from MaureenHelm as a code owner November 16, 2017 16:14

erwango requested review from agross-oss and galak November 16, 2017 16:14

erwango added area: Devicetree platform: STM32 ST Micro STM32 platform: NXP NXP labels Nov 16, 2017

erwango requested a review from nashif November 16, 2017 16:15

erwango added the area: Pinmux label Nov 17, 2017

nashif added RFC Request For Comments: want input from the community and removed RFC Request For Comments: want input from the community labels Nov 18, 2017

erwango changed the title ~~[RFC]Pinmux configuration code generated from Device tree (Do not merge)~~ [RFC][DNM]Pinmux configuration code generated from Device tree Nov 20, 2017

erwango force-pushed the dev_pinmux_gen branch from 7b10d49 to 6fd897d Compare November 20, 2017 16:57

erwango requested a review from SebastianBoe as a code owner November 20, 2017 16:57

erwango force-pushed the dev_pinmux_gen branch 2 times, most recently from 5819de6 to f284267 Compare November 21, 2017 14:20

erwango changed the base branch from master to arm November 21, 2017 14:33

erwango force-pushed the dev_pinmux_gen branch from f284267 to ba39ac2 Compare November 22, 2017 12:38

erwango requested review from andrewboie, andyross, dbkinder, jhedberg, jukkar, mike-scott and tbursztyka as code owners November 22, 2017 12:38

erwango changed the base branch from arm to master November 22, 2017 12:39

erwango removed the request for review from andyross November 22, 2017 12:40

erwango force-pushed the dev_pinmux_gen branch from ac1875f to 701936b Compare November 30, 2017 16:26

MaureenHelm requested changes Nov 30, 2017

View reviewed changes

erwango added 2 commits December 5, 2017 13:44

dts: yaml: Add base file for pinmux

e240765

Add pinmux.yaml file. Get it included by existing pinctrl yaml bindinds. Signed-Off-By: Erwan Gouriou <[email protected]>

dts: nxp: add label to pinmux nodes

4d6b2b5

To enable dts based pinmux code generation, add label property to pinmux nodes on all nxp dtsi files Signed-Off-By: Erwan Gouriou <[email protected]>

erwango force-pushed the dev_pinmux_gen branch from 701936b to 1b81fde Compare December 5, 2017 12:44

erwango added 5 commits December 5, 2017 15:43

erwango force-pushed the dev_pinmux_gen branch from 1b81fde to bd813a7 Compare December 5, 2017 14:50

SebastianBoe removed their request for review December 5, 2017 14:58

erwango mentioned this pull request Dec 6, 2017

dts: boards: provide generic dtsi file for 'generic' boards #5138

Closed

erwango requested review from mbolivar, ydamigos and superna9999 December 18, 2017 08:32

erwango mentioned this pull request Dec 18, 2017

Add gpio nodes to stm32 based soc/boards #5381

Merged

ydamigos reviewed Dec 18, 2017

View reviewed changes

mbolivar reviewed Dec 20, 2017

View reviewed changes

nashif added the DNM This PR should not be merged (Do Not Merge) label Jan 17, 2018

erwango closed this Feb 12, 2018

erwango deleted the dev_pinmux_gen branch February 12, 2018 13:36

erwango restored the dev_pinmux_gen branch February 21, 2018 10:13

erwango reopened this Feb 21, 2018

erwango mentioned this pull request Feb 21, 2018

Refining Zephyr's Device Driver Model #6293

Closed

erwango closed this Jul 2, 2018

erwango deleted the dev_pinmux_gen branch September 3, 2019 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][DNM]Pinmux configuration code generated from Device tree #5021

[RFC][DNM]Pinmux configuration code generated from Device tree #5021

erwango commented Nov 16, 2017 •

edited

Loading

erwango commented Nov 21, 2017

SebastianBoe commented Nov 21, 2017 •

edited

Loading

erwango commented Nov 22, 2017 •

edited

Loading

SebastianBoe commented Nov 22, 2017

erwango commented Nov 30, 2017

pfalcon commented Nov 30, 2017

MaureenHelm Nov 30, 2017

erwango Dec 5, 2017

erwango Jan 9, 2018

ydamigos left a comment •

edited

Loading

mbolivar left a comment

mbolivar Dec 20, 2017

erwango Dec 21, 2017

mbolivar Dec 21, 2017

erwango Dec 21, 2017

mbolivar Dec 21, 2017

superna9999 Dec 21, 2017 •

edited

Loading

erwango Jan 9, 2018

mbolivar Jan 10, 2018

superna9999 Jan 10, 2018

erwango Jan 24, 2018

mbolivar Dec 20, 2017

erwango Dec 21, 2017

erwango commented Feb 21, 2018

[RFC][DNM]Pinmux configuration code generated from Device tree #5021

[RFC][DNM]Pinmux configuration code generated from Device tree #5021

Conversation

erwango commented Nov 16, 2017 • edited Loading

erwango commented Nov 21, 2017

SebastianBoe commented Nov 21, 2017 • edited Loading

erwango commented Nov 22, 2017 • edited Loading

SebastianBoe commented Nov 22, 2017

erwango commented Nov 30, 2017

pfalcon commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydamigos left a comment • edited Loading

Choose a reason for hiding this comment

mbolivar 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

superna9999 Dec 21, 2017 • edited Loading

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

erwango commented Feb 21, 2018

erwango commented Nov 16, 2017 •

edited

Loading

SebastianBoe commented Nov 21, 2017 •

edited

Loading

erwango commented Nov 22, 2017 •

edited

Loading

ydamigos left a comment •

edited

Loading

superna9999 Dec 21, 2017 •

edited

Loading