Skip to content

Conversation

@pabigot
Copy link
Contributor

@pabigot pabigot commented Sep 28, 2019

Device tree nodes have a dependency on their parents, as well as other nodes. To ensure driver instances are initialized in the proper we need to identify these dependencies and construct a total order on nodes that ensures no node is initialized before something it depends on.

Add a Graph class that calculates a partial order on nodes, taking into account the potential for cycles in the dependency graph.

When generating devicetree value headers calculate the dependency graph and document the order and dependencies in the derived files. In the future these dependencies may be expressed in binding data.

Example of content added by this:

# Devicetree node: /soc/gpio@50000000
# Binding (compatible = nordic,nrf-gpio): /mnt/devel/external/zp/zephyr/dts/bindings/gpio/nordic,nrf-gpio.yaml
# Binding description: This is a representation of the NRF GPIO nodes
# Ordinal: 4
# Requires: 3 /soc
# Supports: 5 /buttons/button_0
# Supports: 27 /soc/i2c@40003000/hts221@5f
...
# Devicetree node: /soc/i2c@40003000/hts221@5f
# Binding (compatible = st,hts221): /mnt/devel/external/zp/zephyr/dts/bindings/sensor/st,hts221.yaml
# Binding description: This binding gives a base representation of HTS221 humidity and temperature sensor
# Ordinal: 27
# Requires: 4 /soc/gpio@50000000
# Requires: 10 /soc/i2c@40003000

@zephyrbot
Copy link

zephyrbot commented Sep 28, 2019

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@pabigot pabigot changed the title DNM WIP: add dependency information to edtlib devicetree: add dependency information to edtlib Sep 28, 2019
@pabigot pabigot marked this pull request as ready for review September 28, 2019 15:52
@pabigot pabigot requested a review from galak as a code owner September 28, 2019 15:52
Copy link
Contributor

@pizi-nordic pizi-nordic left a comment

Choose a reason for hiding this comment

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

I fully support all work towards proper handling of dependencies in Zephyr :)

@pizi-nordic pizi-nordic requested a review from anangl September 30, 2019 11:41
@pizi-nordic
Copy link
Contributor

FYI: @barsok

Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Some nits and a defaultdict tip.

Feels like a simpler algo might be possible if all that's needed is ordering + dependency loop detection, but shouldn't hurt at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

Accidental blank line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could call it init_order maybe, to give a better hint about what it's about. I like it better than init_ordinal, even if the node would never get initialized.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could have add_edge(self, source, target), etc., to be consistent with the rest of the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could use a set literal:

self.__roots = {root}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could rephrase to be less academicy maybe.

Copy link
Contributor

@ulfalizer ulfalizer Sep 30, 2019

Choose a reason for hiding this comment

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

Instead of reset, maybe there could be an internal _dirty flag or the like that gets set whenever the graph is updated.

Wonder if the caching here might be a bit overkill though.

Copy link
Contributor

@ulfalizer ulfalizer Sep 30, 2019

Choose a reason for hiding this comment

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

Rest of the code uses format(), which is a bit less quirky. Makes it easier to switch to f-strings later too.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the raise Exception within tarjan() is kept, then it should be transformed into an EDTError exception here, so that the library consistently raises that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space before not.

@pabigot
Copy link
Contributor Author

pabigot commented Sep 30, 2019

@ulfalizer OK, done with updates, please re-check.

Many of your comments (style, features) are due to "grutil" being derived from a more general dependency module I wrote in Python 2 over ten years ago. I've done what I can to convert the code to your preferred style.

One thing I have not done is changed the term ordinal. At the moment, this PR introduces the only order on EDT Nodes present in the system: a stable total order that satisfies the relations in the dependency graph.

I don't want to call this "init(ialization) order" because that's not what it is. Zephyr partitions initialization into four phases: PRE_KERNEL_1, PRE_KERNEL_2, POST_KERNEL, and APPLICATION. Initialization order is currently defined by Kconfig priorities within each group.

There is a relation between initialization order and the partial order defined by dependencies, and that order should ultimately be provided by the devicetree scripting, but it won't be the same as this order.

I also don't want to call the Node attribute "order" because that's not what it is either. It's the position of a node within a sequence, i.e. its ordinal.

@ulfalizer
Copy link
Contributor

One thing I have not done is changed the term ordinal. At the moment, this PR introduces the only order on EDT Nodes present in the system: a stable total order that satisfies the relations in the dependency graph.

What about something like dep_ordinal, to give some hint at least? Even if you know what "ordinal" means (I was shaky on it), it's not obvious what the sequence is.

Personally, I try hard to avoid using a word/term when I suspect most people might not know what it means. It's a bit more effort and less auto-pilot, but can make stuff more efficient for people reading the code.

@pabigot pabigot force-pushed the dts/order branch 2 times, most recently from 34cf70a to b2abb50 Compare September 30, 2019 16:36
@pabigot
Copy link
Contributor Author

pabigot commented Sep 30, 2019

@ulfalizer ok exception handling and attribute rename done.

Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Markup in docstrings and __ variables aren't used elsewhere, so that's a bit inconsistent, but not super important.

@ulfalizer
Copy link
Contributor

Remembered another thing.

Might be good to generate edges for Node.interrupts as well. That one's still hardcoded, because it doesn't work like any other property (and deals with both interrupts and interrupts-extended).

@pabigot
Copy link
Contributor Author

pabigot commented Nov 3, 2019

Rebased and updated generated text to be consistent with new documentation format:

/*
 * Devicetree node:
 *   /soc/gpio@50000000
 *
 * Binding (compatible = nordic,nrf-gpio):
 *   $ZEPHYR_BASE/dts/bindings/gpio/nordic,nrf-gpio.yaml
 *
 * Dependency Ordinal: 4
 *
 * Requires:
 *   3 /soc
 *
 * Supports:
 *   5 /buttons/button_0
 *   28 /soc/i2c@40003000/hts221@5f
 *   31 /soc/i2c@40004000/lis2dh12@19
 *
 * Description:
 *   This is a representation of the NRF GPIO nodes
 */

Device tree nodes have a dependency on their parents, as well as other
nodes.  To ensure driver instances are initialized in the proper we
need to identify these dependencies and construct a total order on
nodes that ensures no node is initialized before something it depends
on.

Add a Graph class that calculates a partial order on nodes, taking
into account the potential for cycles in the dependency graph.

When generating devicetree value headers calculate the dependency
graph and document the order and dependencies in the derived files.
In the future these dependencies may be expressed in binding data.

Signed-off-by: Peter A. Bigot <[email protected]>
@galak galak merged commit ea956f4 into zephyrproject-rtos:master Nov 5, 2019
@pabigot pabigot deleted the dts/order branch November 12, 2019 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants