devicetree: Update generation/API

[galak]

There are cases like having a reserved memory node that will not having
a compatible string and thus will not match a binding. However we might
set a chosen property like zephyr,sram or zephyr,ipc_shm to such a node.
Thus we should generate defines for such nodes.

Signed-off-by: Kumar Gala [email protected]

[mbolivar-nordic]

I really think we need to discuss this in detail with an issue that describes the impact on the API as discussed on slack.

[galak]

I really think we need to discuss this in detail with an issue that describes the impact on the API as discussed on slack.

what impact will this have on the API? This isn't related to "status".

[galak]

#24773

[zephyrbot]

All checks are passing now.

checkpatch (informational only, not a failure)

-:218: WARNING:LONG_LINE: line over 80 characters
#218: FILE: tests/lib/devicetree/src/main.c:212:
+	zassert_equal(DT_NODE_HAS_STATUS(DT_NODELABEL(test_no_status), disabled),

-:222: WARNING:IF_0: Consider removing the code enclosed by this #if 0 and its #endif
#222: FILE: tests/lib/devicetree/src/main.c:216:
+#if 0

- total: 0 errors, 2 warnings, 204 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

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

[ioannisg]

This solves the problem of having new-style DT defines generated for nodes without the compatible property, which seems required in several cases. Looks fine to me, and I don't see any negative impact (but my DT expertise is limited)

[pabigot]

I'm OK with the change the PR is proposed to do, but not the secondary one that discards one of the main roles of compatible.

commit causing concern was removed

[pabigot]

I added these to /:

       special = "this";
       subspecial {
          bar = "that";
       };

The subordinate node was generated, but neither /special nor /subspecial/bar were generated

Is that what was intended? Generating the node without its properties seems odd; will this satisfy the need identified in #24460 (comment)

[mbolivar-nordic]

I really think we need to discuss this in detail with an issue that describes the impact on the API as discussed on slack.

what impact will this have on the API? This isn't related to "status".

Here are some ways I find this PR leading to inconsistent or wrong results. Some of them do relate to status.

DT_HAS_NODE()

This is now returning true for any node that exists in the tree, regardless of whether it has status okay and a matching binding, which contradicts the docstring:

 * Tests whether a node identifier refers to a node which:
 *
 * - exists in the devicetree, and
 * - is enabled (has status property "okay"), and
 * - has a matching binding

I think that docstring is probably already wrong, since we just need status != "disabled", but that's because of the weird way we deal with status right now, which is maybe a bug now that I look at it.

DT_NODE_HAS_PROP()

This PR makes it possible to add a node n which has no status, and which has no matching binding, and get DT_HAS_NODE(n) == 1, but DT_NODE_HAS_PROP(n, some_random_property_defined_on_n) == 0, which doesn't make sense to me.

(This is because edtlib refuses to initialize Node.props for a node without a matching binding.)

REG and IRQ APIs

These behave in inconsistent ways between themselves and depending on the status of the node. For example, if some node foo@0 has no matching binding, these tests can all pass:

	zassert_false(DT_NODE_HAS_PROP(DT_PATH(foo_0), reg), "");
	zassert_equal(DT_REG_ADDR(DT_PATH(foo_0)), 0, "");
	zassert_equal(DT_NUM_REGS(DT_PATH(foo_0)), 1, "");

In other words, even though the API claims foo@0 has no reg property, the number of regs can be 1 and you can read a reg address. (This is because edtlib knows what to do with a reg for any node.)

But if you disable the node using status, you can't read its reg anymore, and DT_REG_ADDR is a build error.

Without this PR, things are more consistent. For example, DT_NODE_HAS_PROP(foo, reg) returns 0, and using DT_REG_ADDR results in a build error, even when the node isn't explicitly disabled, as long as it doesn't have a matching binding.

Similar comments should apply to IRQ properties, though I didn't check explicitly.

[mbolivar-nordic]

DT_HAS_NODE works on nodes with or without bindings, and with status enabled or disabled

Though this may take a global search/replace on nodelabel-based device access in samples and drivers.

[pabigot]

I agree that the test cases should verify that this does what's expected. An example based on the use case it's supposed to solve would make this more compelling.

The behavior now is just as I'd described at #24769 (review) except that I'm seeing a few more things generated as well (e.g. compatibles properties for the buttons and LEDs and a few other root nodes like aliases). I understand the concern about generating properties without a binding, but I remain skeptical of the value of generating an empty node. I guess that can be extended in the future.

@mbolivar not sure what specifically you want my view on. I would like DT_HAS_foo to have the semantics proposed in #24769 (comment) before 2.3 release. I'm okay with DT_INST_FOREACH only working on enabled nodes as long as the documentation is consistent with "enabled" meaning "whatever condition triggers generation of data structures", e.g. DEFINE_DEVICE*. Today that might be "status okay" (which I think is really "status != disabled"), but in the future it must change to potentially support all status values for #19448.

[galak]

@mbolivar-nordic are wanting this PR to handle both issues of generating properties for nodes w/o binding matches and the status change?

[mbolivar-nordic]

are wanting this PR to handle both issues of generating properties for nodes w/o binding matches and the status change?

They're interrelated IMO, given the way status can affect REG and IRQ with this change as I detailed in #24769 (comment)

@mbolivar not sure what specifically you want my view on

Sound like you're just wanting it done by release and don't have a strong view on which PRs should have things done in. OK.

[mbolivar-nordic]

So if we're moving to #25002, I think this should be removed too, right? We had discussed being able to read any property we have semantics for (at least 'reg' and 'interrupts') regardless of status.

[mbolivar-nordic]

I also think things like compat2enabled in the EDT object may need changes, since we're changing the semantics of "enabled" in C to be status == "okay" instead of status != "disabled" and adding macros like DT_HAS_COMPAT_STATUS_OKAY in #25002. So it seems like we should be doing compat2statusokay or something instead.

In general I'm having trouble keeping all the pull requests together and I don't see enough Python changes to accompany all the C changes. What am I missing?

[galak]

I reworked how enabled was defined. If we want to leave that alone we can create a new 'available' which would be similar to the linux concept https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/base.c#n581

[b0661]

edtlib: populate standard properties for nodes without bindings

If a devicetree node doesn't have a matching binding we will at least
populate a common standard set ...:

	compatible
	status
	reg
	reg-names
	label
	interrupt
	interrupts-extended
	interrupt-names
	interrupt-controller

This allows us to handle cases like memory nodes ...

Why not just assign a default.yaml binding to active nodes without a binding. The binding can contain any property of above or just include base.yaml.

The explicit addition of default properties in the current solution does not scale well and also makes edtlib.py less generic.

[galak]

Why not just assign a default.yaml binding to active nodes without a binding. The binding can contain any property of above or just include base.yaml.

The explicit addition of default properties in the current solution does not scale well and also makes edtlib.py less generic.

I was torn about this, since it makes an assumption about there being a specific file in one of the binding dirs.

[mbolivar-nordic]

Leaving a comment here since we've merged ifdeffed out tests before, to make sure it doesn't happen this time

[b0661]

Why not just assign a default.yaml binding to active nodes without a binding. The binding can contain any property of above or just include base.yaml.
The explicit addition of default properties in the current solution does not scale well and also makes edtlib.py less generic.

I was torn about this, since it makes an assumption about there being a specific file in one of the binding dirs.

If the specific binding file default.yaml is not found it does not hurt. If it is found it is available on purpose (like e.g. base.yaml) and the default properties will be generated.

With the current solution you get the opposite situation - every user of edtlib.py gets the default properties generated. This may not be a problem within Zephyr but usability outside of Zephyr may be reduced.

Using default.yaml gives a choice and does not reduce usability in any way.

[mbolivar-nordic]

Using default.yaml gives a choice and does not reduce usability in any way.

I've chosen a different strategy with a similar effect in #25009. Please have a look.

[galak]

I get the desire @b0661 and would say we can add some way to optionally extend the base set of properties if someone desires that. However, while I'm not super happy about hard coding a list of properties into EDT, I think its the most reasonable tradeoff for the set of properties that are defined as part of the base DTS spec.