Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 22 additions & 19 deletions scripts/dts/python-devicetree/src/devicetree/edtlib.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (c) 2019 Nordic Semiconductor ASA

Check warning on line 1 in scripts/dts/python-devicetree/src/devicetree/edtlib.py

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

License may not be allowed

scripts/dts/python-devicetree/src/devicetree/edtlib.py:1 License file for 'BSD-3-Clause' not found in /LICENSES. Please check https://docs.zephyrproject.org/latest/contribute/guidelines.html#components-using-other-licenses.

Check warning on line 1 in scripts/dts/python-devicetree/src/devicetree/edtlib.py

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

License may not be allowed

scripts/dts/python-devicetree/src/devicetree/edtlib.py:1 License file for 'BSD-3-Clause' not found in /LICENSES. Please check https://docs.zephyrproject.org/latest/contribute/guidelines.html#components-using-other-licenses.

Check warning on line 1 in scripts/dts/python-devicetree/src/devicetree/edtlib.py

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

License may not be allowed

scripts/dts/python-devicetree/src/devicetree/edtlib.py:1 License file for 'BSD-3-Clause' not found in /LICENSES. Please check https://docs.zephyrproject.org/latest/contribute/guidelines.html#components-using-other-licenses.
# Copyright (c) 2019 Linaro Limited
# Copyright 2025 NXP
# SPDX-License-Identifier: BSD-3-Clause
Expand Down Expand Up @@ -2404,30 +2404,32 @@
"""
# A Node depends on any Nodes present in 'phandle',
# 'phandles', or 'phandle-array' property values.
#
# When a phandle target is a descendant of root_node, skip adding
# the dependency edge: the tree already has a child-parent edge
# for every child, so adding parent-child would create a cycle.
# The dependency is implicit because the parent already contains it.

def is_child_node(child_node):
while child_node is not None:
if root_node is child_node:
return True
child_node = child_node.parent

return False

for prop in props_node.props.values():
if prop.type == 'phandle':
# According to the DT spec, a property named 'phy-handle' is required when
# the Ethernet device is connected a physical layer device (PHY).
# But the 'phy-handle' property can point to a child node of the Ethernet device,
# so we need to check for that and not add a dependency in that case, otherwise
# we'll get a cycle in the graph.
if prop.name == "phy-handle":
def _is_child(parent_node: Node, child_node: Optional[Node]) -> bool:
if child_node is None:
return False
if parent_node is child_node:
return True
return _is_child(parent_node, child_node.parent)
if TYPE_CHECKING:
assert isinstance(prop.val, Node)
if _is_child(props_node, prop.val):
continue
self._graph.add_edge(root_node, prop.val)
if TYPE_CHECKING:
assert isinstance(prop.val, Node)
if not is_child_node(prop.val):
self._graph.add_edge(root_node, prop.val)
elif prop.type == 'phandles':
if TYPE_CHECKING:
assert isinstance(prop.val, list)
for phandle_node in prop.val:
self._graph.add_edge(root_node, phandle_node)
if not is_child_node(phandle_node):
self._graph.add_edge(root_node, phandle_node)
elif prop.type == 'phandle-array':
if TYPE_CHECKING:
assert isinstance(prop.val, list)
Expand All @@ -2436,7 +2438,8 @@
continue
if TYPE_CHECKING:
assert isinstance(cd, ControllerAndData)
self._graph.add_edge(root_node, cd.controller)
if not is_child_node(cd.controller):
self._graph.add_edge(root_node, cd.controller)

# A Node depends on whatever supports the interrupts it
# generates.
Expand Down
62 changes: 62 additions & 0 deletions scripts/dts/python-devicetree/tests/test_edtlib.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (c) 2019 Nordic Semiconductor ASA

Check warning on line 1 in scripts/dts/python-devicetree/tests/test_edtlib.py

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

License may not be allowed

scripts/dts/python-devicetree/tests/test_edtlib.py:1 License file for 'BSD-3-Clause' not found in /LICENSES. Please check https://docs.zephyrproject.org/latest/contribute/guidelines.html#components-using-other-licenses.

Check warning on line 1 in scripts/dts/python-devicetree/tests/test_edtlib.py

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

License may not be allowed

scripts/dts/python-devicetree/tests/test_edtlib.py:1 License file for 'BSD-3-Clause' not found in /LICENSES. Please check https://docs.zephyrproject.org/latest/contribute/guidelines.html#components-using-other-licenses.

Check warning on line 1 in scripts/dts/python-devicetree/tests/test_edtlib.py

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

License may not be allowed

scripts/dts/python-devicetree/tests/test_edtlib.py:1 License file for 'BSD-3-Clause' not found in /LICENSES. Please check https://docs.zephyrproject.org/latest/contribute/guidelines.html#components-using-other-licenses.
# SPDX-License-Identifier: BSD-3-Clause

import contextlib
Expand Down Expand Up @@ -939,6 +939,68 @@
assert edt.get_node("/child-binding/child-1/grandchild") in dep_node.required_by
assert edt.get_node("/child-binding/child-2") in dep_node.required_by

def test_child_phandle_circular_dependency(tmp_path):
'''Test parent phandles to child states do not create dependency cycles.'''

binding_dir = tmp_path / "bindings"
binding_dir.mkdir()

binding_file = binding_dir / "test-stm.yaml"
binding_file.write_text("""
description: Generic child state dependency test

compatible: "test,stm"

properties:
states:
type: phandles

child-binding:
description: state node
properties:
next-state:
type: int
""", encoding="utf-8")

dts_file = tmp_path / "child-descendant-ref.dts"
dts_file.write_text("""
/dts-v1/;

/ {
test_stm {
compatible = "test,stm";
states = <&state1 &state2 &state3>;

state1: state1 {
next-state = <2>;
};

state2: state2 {
next-state = <3>;
};

state3: state3 {
next-state = <1>;
};
Comment on lines +970 to +984
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm guessing that the test won't pass without the edtlib patch, but this is exactly the sort of thing that we intend to capture as a circular dependency. I know that there are some unpleasant limitations in our current approach, but this is not something I am open to revisiting in a piecemeal fashion, use-case-by-use-case, like is being done here.

For now I would suggest refactoring your DT to avoid this. If you want to re-start the larger conversation about how zephyr tracks DT dependencies, I would request that you file a much more detailed report and plan. It would be a stability and maintainability nightmare in my opinion if zephyr had subtle but minor differences in how it managed DT dependencies between releases, which is the path we are going down if we start to merge PRs like this one.

Copy link
Copy Markdown
Contributor Author

@ifx-rmcs ifx-rmcs Mar 31, 2026

Choose a reason for hiding this comment

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

Yes, I am currently working around this issue by placing my DT in a way that it doesn't encounter the circular dependency. This PR is to improve the end user experience. I figured that generalizing the existing solution for the Ethernet would be beneficial for others also.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't improve the end user experience in my opinion. It seems to directly contradict our specification for how dependencies are supposed to work:

https://docs.zephyrproject.org/latest/build/dts/api/api.html#inter-node-dependencies

I understand your idea but please close this PR if you don't have the resources to try to tackle the more general problem

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks to me like the existing Ethernet phy-handle is a piecemeal or a case-by-case solution. Perhaps I don't understand the general issue related to circular dependency and the test I added is too specific? I can close the PR if there's some other bigger picture I'm not seeing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Part of the bigger picture that I'm referring to is that this PR seems to completely break the algorithm we have documented for how dependency tracking is meant to work. If that's not clear, let's just close this for now. Thanks for your contribution but changes to this code have a huge blast radius and I don't believe you've worked through the details enough for me to accept this. I appreciate your effort, though.

};
};
""", encoding="utf-8")

edt = edtlib.EDT(os.fspath(dts_file), [os.fspath(binding_dir)])

parent = edt.get_node("/test_stm")
states = [
edt.get_node("/test_stm/state1"),
edt.get_node("/test_stm/state2"),
edt.get_node("/test_stm/state3"),
]

assert parent.props["states"].val == states
for state in states:
assert parent not in state.required_by
assert state not in parent.depends_on
assert parent in state.depends_on

def test_slice_errs(tmp_path):
'''Test error messages from the internal _slice() helper'''

Expand Down
Loading