[WIP] scripts: extract_dts_includes.py: enhance #6761

b0661 · 2018-03-23T07:07:23Z

Overview

To ease the maintenance and ongoing development extract_dts_includes.py is refactored.

To manage devices efficiently extract_dts_includes.py is extended/ amended:

to extract the compatibe directive
to extract [device-type]-controller directives (e.g. gpio-controller, pin-controller, ...)
to apply some heuristics to detect device controller nodes by the node type of the binding (yaml file). Such nodes are added to the list of device controllers for a SoC the same way as a [device-type]-controller directive.
to extract all clock related directives

To support pinctrl devices extract_dts_includes.py is extended:

to extract new pinctrl and gpio-ranges directives as described in the Linux pinctrl-bindings.txt.

The PR is a cut-out from PR #5799 for review but can be independently applied.

Motivation for or Use Case

The prime motivation is to be able to create pin controller drivers that can configure the pins on startup by the information from the device tree in a standardized way. Therefor all relevant device tree information must be available in generated_dts_boards.[h,conf] in a way that can easily be used to generate structured data at build time.

Generation of structured data is done by CodeGen that reads generated_dts_boards.conf

Design Details

To allow modularization of the monolitic script all global data and functions are factorized to a python module that can be imported by other modules.

extract/globals.py

The extraction of device tree directives is added as separate python modules that are derived from the common base class DTDirective:

extract/pinctrl.py: pinctrl-[x] and pinctrl-names directives
extract/gpioranges.py: gpio-ranges directive
extract/interrupts.py: interrupts directive
extract/reg.py: reg directive
extract/flash.py: reg directive in partitions and partitions chosen
extract/controller.py: [device-type]-controller directive
extract/compatible.py: compatible directive
extract/clocks.py: clock related directives
extract/heuristics.py: handling of heuristics; expecially for device controller detection
extract/default.py: default used for all directives not covered by any other module

Test Strategy

This PR includes a unit test that tests the correct generation of defines for the pinctrl directive in device trees. It also tests that the default behaviour for the pinctrl directive stays the same.

A working proof of concept for STM32F0x drivers using the extracted device tree information is available in PR #5799.

Signed-off-by: Bobby Noelte b0661n0e17e@gmail.com

codecov-io · 2018-03-23T07:28:27Z

Codecov Report

Merging #6761 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6761   +/-   ##
=======================================
  Coverage   52.36%   52.36%           
=======================================
  Files         195      195           
  Lines       24696    24696           
  Branches     5128     5128           
=======================================
  Hits        12931    12931           
  Misses       9695     9695           
  Partials     2070     2070

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5356ee5...9384a46. Read the comment docs.

b0661 · 2018-05-17T11:08:59Z

recheck

b0661 · 2018-05-21T09:27:00Z

As there seems now consensus that inline code generation (PR #6760 ) is acceptable to Zephyr I remove the [RFC] tag also here. PR #6760 uses the device tree info generated by this PR.

Please start the PR review

Add a common binding for pinctrl devices to be included by SoC specific bindings. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

Add a common binding for GPIO devices with pinctrl backend. To be included by SoC specific bindings. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

Add generic bindings for clock consumers and clock providers. Add a common binding for fixed clocks. To be included by SoC specific bindings. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

Extend pinctrl client device bindings to allow pinctrl to replace the pin initialisation currently done by the SoCs/boards pinmux.c: - UART - SPI - I2C - CAN Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

Variables 'names' is copying value from 'reduced' dict and gives illusion that any computation could be done on copied data. Problem is that later computation on 'names' will modify data in 'reduced'. Use deepcopy in order to avoid erasing thing in reduced. Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

Use EDTS database to create and access DTS info. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

Extract compatible info and make it available by defines. This way the compatible info of all activated devices can be used in drivers. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

Support more clocks related directives (besides clocks): - clock-names - clock-output-names - clock-indices - clock-ranges - clock-frequency - clock-accuracy - oscillator - assigned-clocks - assigned-clock-parents - assigned-clock-rates Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

For pin-controller and clients the following additional information is generated: Pin controller nodes -------------------- For a pin controller node five data sets are extracted from the device tree and provided in generated_dts_board.h. - FUNCTION For all active client nodes that reference a pin-controller by pinctrl-x(&pinctrl, ...) FUNCTION defines are generated. The function equals the client node. - STATE_NAME For all pinctrl names of pinctrl-x directives of active client nodes STATE_NAME defines are generated. This is mostly to allow deduplication of commonly used state names such as e.g. “default”. - PINCTRL_STATE For all pinctrl-x directives of active client nodes PINCTRL_STATE defines are generated. The pinctrl state references the FUNCTION (aka. node) it is given in. The pinctrl state also references the STATE_NAME given in the pinctrl-names directive associated to the pinctrl-x directive. All the pins that a pinctrl state controls are regarded to be a pin group. The name of the pin group is the same as the name of the pinctrl state. - PINCTRL For all subnodes of pin configurations referenced by active client nodes - pinctrl-x(&xxx, pinconf_a, ...) - PINCTRL defines are generated. The pinctrl references the FUNCTION (aka. node) it is referenced from. The pinctrl also references the PINCTRL_STATE it is referenced from. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

GPIO ranges info is added to pin controller nodes nad GPIO nodes: Pin controller nodes -------------------- - GPIO_RANGE For all gpio-ranges directives of active GPIO nodes GPIO_RANGE defines are generated. GPIO nodes ---------- - GPIO_RANGE For all gpio-ranges directives of active GPIO nodes GPIO_RANGE defines are generated. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

Extract info of all devices activated. Add this info to the top level node to allow drivers to use this info. Info of devices activated is either taken from the <device>-controller directive (e.g. gpio-controller, pin-controller) or detected by heuristics if the <device>-controller directive is not foreseen for a device in the DT specification or Linux. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

'defs' variable is used and passed as a parameter in the vast majority of the functions of extract_dts_includes script. Set this variable global Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org> Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

For testing the default dts bindings can now be replaced by test versions. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

Test correct generation of defines for the pinctrl directive in device trees. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

erwango

While there are definitely some good things, I think that PR should not be merged as is as it introduced too much new concepts and that needs to be discussed one by one.
Blocking for now.

erwango · 2018-07-05T09:39:51Z

Here are my main concerns regarding this PR (among good things):

About the way of working:
1- This PR introduces several new concepts of dts information extractions and #define's generation. Each of this new feature deserve a dedicated discussion, so this PR should be split per concept.

2- It is difficult to apprehend/review/comment each one of these concepts without an example of its use in actual code. Besides, w/o illustration, interest of the feature is unclear to the reader. Each new addendum should come with an example of usage in a driver (in a dedicated PR), so the usage and the usefulness is demonstrated.

EDIT: Just to be clear: my point is not to block the introduction of new features, but really to fasten the discussions by enabling the review process which could not take place today. Then, some parts maybe accepted as is, some rejected and some accepted after modification. But today, there is no review.

About the content:
3- The new features introduce numerous new #define's while we're aiming at removing the need for .fixup files and attempt of introducing codegen should help to generate code w/o need for #define generation. Some #define might still be needed for some part (like aliases), but could theoretically be removed in their vast majority. Having an example of usage of these defines would help to understand the need (cf point 2)

4- One commit introduce dev- directive to establish top level nodes as controllers even when not existing in DT specification. This is conflicting with bus-master concept already present in Zephyr code base and might create confusion with extisting dts -controllers. As is, the need for this addition is unclear, would require and example (cf point 2)

b0661 · 2018-07-05T10:47:25Z

@erwango

Thank you for the review.

This PR will change as most of the defines can be replaced by EDTS. This is a work in progress. I will move from defines to EDTS.

What really would help is if we can come up with:

a common EDTSDatabase class in this PR and in PR Generated driver instantiation prototype #8561. That is why I referenced this from Generated driver instantiation prototype #8561.
a concept how to structure the information in the EDTSDatabase
a concept what info shall go to defines besides going to EDTS

1- This PR introduces several new concepts of dts information extractions and #define's generation. Each of this new feature deserve a dedicated discussion, so this PR should be split per concept.

The full picture is in #5799 as always. This is a cut out specifically for review.

2- It is difficult to apprehend/review/comment each one of these concepts without an example of its use in actual code. Besides, w/o illustration, interest of the feature is unclear to the reader. Each new addendum should come with an example of usage in a driver (in a dedicated PR), so the usage and the usefulness is demonstrated

The full picture is in #5799 as always. But I will see how to even split that in more PRs after the change to EDTS.

3- The new features introduce numerous new #define's while we're aiming at removing the need for .fixup files and attempt of introducing codegen should help to generate code w/o need for #define generation. Some #define might still be needed for some part (like aliases), but could theoretically be removed in their vast majority. Having an example of usage of these defines would help to understand the need (cf point 2)

I agree (see above). The only unclear thing is what should remain a define also? Is there a concept?

4- One commit introduce dev- directive to establish top level nodes as controllers even when not existing in DT specification. This is conflicting with bus-master concept already present in Zephyr code base and might create confusion with extisting dts -controllers. As is, the need for this addition is unclear, would require and example (cf point 2)

I think the defines for device controllers can be removed alltogether. They were necessary before EDTS. The info can now be queried from the EDTS when needed.

b0661 · 2018-07-05T10:54:41Z

@erwango

a common EDTSDatabase class in this PR and in PR #8561.

Can you switch from insert_structs to edts.insert_device_property ?

a concept how to structure the information in the EDTSDatabase

I would closely follow the Device Tree layout e.g. "clocks/controller", "clocks/0/bus", .... insert_device_property allows to specify the property_path when inserting info into EDTS.

a concept what info shall go to defines besides going to EDTS

I´m quite unshure here. At least the legacy defines that are currently generated have to be generated. About new ones I don´t have a clear view.

erwango · 2018-07-05T12:29:32Z

scripts/dts/edtsdatabase.py

+    # @param device_id
+    # @param property_path Path of the property to access
+    #                      (e.g. 'reg/0', 'interrupts/prio', 'label', ...)
+    # @param property_value value


The way it works today is that we enter a top level property and a dict associated.
For instance:
'deflabel: interrupts'
("new_structs: [{'data': [33, 0], 'labels': ['irq', 'priority']}, {'data': "
"[34, 0], 'labels': ['irq', 'priority']}]")
''
Would be easier to stick to that in a first iteration

This would become:

device_id = edts.device_id(compatible, ....) # or other helper function - TBD edts.device_insert_property(device_id, 'interrupts', [{'data': [33, 0], 'labels': ['irq', 'priority']}, {'data': " "[34, 0], 'labels': ['irq', 'priority']}])

erwango · 2018-07-05T12:35:24Z

scripts/dts/edtsdatabase.py

+    ##
+    # @brief Insert property value for the device of the given device id.
+    #
+    # @param device_id


Using device_id will impose to generate device id each time before calling insert_device_property.
It would be easier to set node_address + compat* in the arguments and then compute device_id inside the function

*: compatible could be computed from node_address, but requires to know 'reduced'

Using device_id will impose to generate device id each time before calling insert_device_property.

On filling the EDTS database the device_id should be be generated at the beginning of extract_ and then re-used. reduced for 'compatible' is available here anyway.

def extract_<whatever>(node_address, ...) node = .... device_id = edts.device_id(node[props][...], ...)

There may also be a helper function in extract_dts_includes:

def extract_device_id(node_address): # extract necessary values from reduced return etds.device_id(compatible, base_address, offset)

On using the ETDS database the device id will be queried. Reduced is not necessary (and not available).

for device_id in edts.compatible_devices_id(compatible): # do something, eg label = edts.device_property(device_id, 'label')

It would be easier to set node_address + compat* in the arguments and then compute device_id inside the function

This can not be part of EDTSDatabase as this requires reduced to be available which breaks data encapsulation. There can well be a function in extract_dts_includes that provides this, e.g.

def extract_edts_device_property(node_address, property_path, property_value): # extract necessary values from reduced device_id = etds.device_id(compatible, base_address, offset) etds.insert_device_property(device_id, property_path, property_value)

erwango · 2018-07-05T12:38:17Z

scripts/dts/edtsdatabase.py

+    # @return property value
+    #
+    def device_property(self, device_id, property_path, default="<unset>"):
+        property_value = self._edts['devices'][device_id]


To be updated vs the latest version.
(And will likely require a more efficient (and iterative) parsing function)

erwango · 2018-07-05T12:51:33Z

scripts/dts/edtsdatabase.py

+        s = s.upper()
+        return s
+
+    def device_id(self, compatible, base_address, offset):


What is 'offset'?

I think the function should take node_address and compatible into account.
node_address should look like: /soc/i2c@40005800/hts221@5f
From this is should be possible to build device_id: <compatible>_40005800_5F

And while I don't have any example yet, I think there could be more address level than that, so it should be taken into account upfront: <compatible>_<addr_1>_<addr_2>_<...>_<addr_n>

I took offset only as an assumption from the string you provided as @galak ´s request. You may also call it index, ... Maybe @galak can explain what he would like device id to be build from and look like.

I don´t think it is important how device id will be generated as long as it is unique. My first proposal was to just use the 'label' property as this is unique for all activated devices.

In my view device_id is just a unique reference to a device struct. It should not be used for other purposes (like generating define labels from it).

I'll let @galak provide more information here

erwango · 2018-07-05T12:53:35Z

scripts/dts/edtsdatabase.py

+# device-type: any of ['GPIO', 'SPI', 'CAN', ...] - 'id' from <binding>.yaml
+# label: any of ['UART_0', 'SPI_11', ...] - label directive from DTS
+#
+class EDTSDatabase(Mapping):


For clarity, it would be nice to split between producer and consumer functions.

For clarity, it would be nice to split between producer and consumer functions.

I tried to by the function names:

Producer

insert_device_property

save

Consumer

device_property

compatible_devices

compatible_devices_id

load

I´m open to better method names.

So a clear split in the file would be good. get_... for consumer could be nice as well
What about sub classes?

What about sub classes?

OK will go for mixin classes like codegen uses to separate the different functions.

erwango · 2018-07-05T13:24:09Z

@b0661 , thanks for the feedback. I made some comments to edts class.
Let me try to give my opinion on some other points:

a common EDTSDatabase class in this PR and in PR #8561. That is why I referenced this from #8561.

Added some comments. In a first step it would be easier to stick with existing APIs (or at least something close).

a concept how to structure the information in the EDTSDatabase

Once EDTSDatabase are in use, we can update internal database structure.
But structure will depends on the needs expressed.
For now, only needs expressed are the ones for drivers instantiation, and they are satisfied by current structure (I've not identified any limitation yet). I'm open to changing the structure, as long as the drivers instantiation still works with current API (using a shim layer if needed)
While no other needs are expressed, it's hard to guess a better structure.

a concept what info shall go to defines besides going to EDTS

My rough view is that we should target to keep #define's generated from 'aliases' and CONFIG_xxx.
The other one that we generate today should be kept until the drivers do not need them any more (replaced by codegen device instantiation).

b0661 · 2018-07-05T16:07:04Z

@erwango

In a first step it would be easier to stick with existing APIs (or at least something close).

Replace

insert_structs(node_address, prop, prop_values)

by

edts_insert_device_property(node_address, property_path, property_value)

with helper function:

def edts_insert_device_property(node_address, property_path, property_value):
    device_id = edts_device_id(node_address)
    etds.insert_device_property(device_id, property_path, property_value)

I think this is pretty close.

But structure will depends on the needs expressed.

There should still be a convention how to pack DTS info into EDTS. You already created one with the conversion to dict entries and lists.

b0661 · 2018-07-26T15:24:40Z

Defines are not used anymore to generate DTS information for codegen. #6762 will be updated to include DTS generation for codegen using a json database file as @erwango already uses in #8561.

b0661 requested review from nashif and SebastianBoe as code owners March 23, 2018 07:07

b0661 mentioned this pull request Mar 23, 2018

drivers: pinctrl device interface (Linux-like) and STM32F0 driver #5799

Closed

b0661 mentioned this pull request Mar 23, 2018

sanitycheck in shippable states faillure but reports 0 errors #6759

Closed

b0661 force-pushed the pr_pinctrl_extract branch from f66bae9 to 9b3c148 Compare March 27, 2018 20:01

b0661 force-pushed the pr_pinctrl_extract branch 2 times, most recently from 80ed611 to a6089c5 Compare April 5, 2018 12:17

b0661 force-pushed the pr_pinctrl_extract branch from a6089c5 to cf2830a Compare May 10, 2018 14:44

b0661 mentioned this pull request May 10, 2018

scripts: add Zephyr inline code generation with Python #6762

Closed

b0661 force-pushed the pr_pinctrl_extract branch 4 times, most recently from af8b56a to bd49e17 Compare May 15, 2018 06:59

This was referenced May 15, 2018

tests: kernel.timer: fails on riscv32 #7536

Closed

Generate device instance defines from dts #7550

Closed

b0661 force-pushed the pr_pinctrl_extract branch 2 times, most recently from dcb4a6f to ae6017a Compare May 21, 2018 08:14

b0661 changed the title ~~[RFC] scripts: extract_dts_includes.py: extract pinctrl and related directives~~ scripts: extract_dts_includes.py: extract pinctrl and related directives May 21, 2018

b0661 force-pushed the pr_pinctrl_extract branch 2 times, most recently from 9694980 to b79938f Compare June 17, 2018 19:02

b0661 changed the title ~~scripts: extract_dts_includes.py: extract pinctrl and related directives~~ scripts: extract_dts_includes.py: refactor and enhance Jun 17, 2018

b0661 force-pushed the pr_pinctrl_extract branch 2 times, most recently from 35de025 to e96bb35 Compare June 23, 2018 12:59

b0661 mentioned this pull request Jun 23, 2018

scripts: extract_dts_includes.py: refactor for better maintenance #8541

Merged

b0661 force-pushed the pr_pinctrl_extract branch from e96bb35 to 2db8a16 Compare June 27, 2018 20:26

b0661 requested review from andrewboie and dbkinder as code owners June 27, 2018 20:26

b0661 and others added 14 commits July 4, 2018 19:04

dts: bindings: add generic binding for pinctrl devices

2f201b2

Add a common binding for pinctrl devices to be included by SoC specific bindings. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

dts: bindings: add generic binding for gpio-pinctrl devices

ba4efe5

Add a common binding for GPIO devices with pinctrl backend. To be included by SoC specific bindings. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

dts: bindings: add generic binding for clock consumer and provider

50e363c

Add generic bindings for clock consumers and clock providers. Add a common binding for fixed clocks. To be included by SoC specific bindings. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

dts: bindings: extend by pinctrl and related directives

5c1c642

Extend pinctrl client device bindings to allow pinctrl to replace the pin initialisation currently done by the SoCs/boards pinmux.c: - UART - SPI - I2C - CAN Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

scripts: extract_dts_includes.py: add EDTS database

7f596d6

Use EDTS database to create and access DTS info. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

scripts: extract_dts_includes.py: add compatible directive support

36a1808

Extract compatible info and make it available by defines. This way the compatible info of all activated devices can be used in drivers. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

cmake: dts: allow default dts bindings dir to be overwritten

154137f

For testing the default dts bindings can now be replaced by test versions. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

test: scripts: extract_dts_includes.py pinctrl directive

9384a46

Test correct generation of defines for the pinctrl directive in device trees. Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

b0661 force-pushed the pr_pinctrl_extract branch from a3a1d8f to 9384a46 Compare July 4, 2018 17:06

b0661 mentioned this pull request Jul 4, 2018

Generated driver instantiation prototype #8561

Closed

b0661 changed the title ~~scripts: extract_dts_includes.py: refactor and enhance~~ scripts: extract_dts_includes.py: enhance Jul 4, 2018

erwango requested changes Jul 5, 2018

View reviewed changes

b0661 changed the title ~~scripts: extract_dts_includes.py: enhance~~ [WIP] scripts: extract_dts_includes.py: enhance Jul 5, 2018

erwango requested changes Jul 5, 2018

View reviewed changes

erwango reviewed Jul 5, 2018

View reviewed changes

b0661 closed this Jul 26, 2018

b0661 mentioned this pull request Oct 26, 2018

scripts: inline code generation with cogeno (and edts) #10885

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] scripts: extract_dts_includes.py: enhance #6761

[WIP] scripts: extract_dts_includes.py: enhance #6761

b0661 commented Mar 23, 2018 •

edited

Loading

codecov-io commented Mar 23, 2018 •

edited

Loading

b0661 commented May 17, 2018

b0661 commented May 21, 2018

erwango left a comment

erwango commented Jul 5, 2018 •

edited

Loading

b0661 commented Jul 5, 2018

b0661 commented Jul 5, 2018

erwango Jul 5, 2018

b0661 Jul 5, 2018 •

edited

Loading

erwango Jul 5, 2018

b0661 Jul 5, 2018 •

edited

Loading

erwango Jul 5, 2018

erwango Jul 5, 2018

b0661 Jul 5, 2018

erwango Jul 5, 2018

erwango Jul 5, 2018

b0661 Jul 5, 2018 •

edited

Loading

erwango Jul 5, 2018

b0661 Jul 5, 2018

erwango commented Jul 5, 2018

b0661 commented Jul 5, 2018

b0661 commented Jul 26, 2018

[WIP] scripts: extract_dts_includes.py: enhance #6761

[WIP] scripts: extract_dts_includes.py: enhance #6761

Conversation

b0661 commented Mar 23, 2018 • edited Loading

Overview

Motivation for or Use Case

Design Details

Test Strategy

codecov-io commented Mar 23, 2018 • edited Loading

Codecov Report

b0661 commented May 17, 2018

b0661 commented May 21, 2018

erwango left a comment

Choose a reason for hiding this comment

erwango commented Jul 5, 2018 • edited Loading

b0661 commented Jul 5, 2018

b0661 commented Jul 5, 2018

Choose a reason for hiding this comment

b0661 Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b0661 Jul 5, 2018 • 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

b0661 Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwango commented Jul 5, 2018

b0661 commented Jul 5, 2018

b0661 commented Jul 26, 2018

b0661 commented Mar 23, 2018 •

edited

Loading

codecov-io commented Mar 23, 2018 •

edited

Loading

erwango commented Jul 5, 2018 •

edited

Loading

b0661 Jul 5, 2018 •

edited

Loading

b0661 Jul 5, 2018 •

edited

Loading

b0661 Jul 5, 2018 •

edited

Loading