Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dts: drop HAS_DTS #63696

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Conversation

gmarull
Copy link
Member

@gmarull gmarull commented Oct 9, 2023

HAS_DTS has become a redundant option. All Zephyr architectures now select this option, and no out-of-tree architectures are supported, meaning devicetree has become a de-facto requirement. There are actually many Zephyr areas that are not guarded, and expect devicetree to always be available regardless of HAS_DTS.

Depends on #63799

carlocaione
carlocaione previously approved these changes Oct 9, 2023
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

afaik there are still users not having DTS downstream.

Would like review from @mped-oticon / @daor-oti first.

@gmarull
Copy link
Member Author

gmarull commented Oct 9, 2023

afaik there are still users not having DTS downstream.

Would like review from @mped-oticon / @daor-oti first.

Considering that all architectures require DTS, and that we do not support oot archs, I suspect this must be a heavily patched Zephyr. What this PR shows is that Zephyr as is now, can live without this option because it's doing nothing in practice.

@aescolar
Copy link
Member

aescolar commented Oct 9, 2023

and that we do not support oot archs

We support out of tree architectures :)

fabiobaltieri
fabiobaltieri previously approved these changes Oct 9, 2023
Copy link
Member

@fabiobaltieri fabiobaltieri left a comment

Choose a reason for hiding this comment

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

Hard to justify keeping this in if nothing upstream builds without it. Worth running it through the arch meeting I guess but it feels like a reasonable requirement.

@mped-oticon
Copy link
Collaborator

@tejlmand thanks for the heads-up and inclusion!

@gmarull Yes, there are are indeed platforms without HAS_DTS, e.g. ours.
The option may not do much right now, but my concern is over the long-term. DTS being assumed, can leak into many parts of Zephyr and thus making it mandatory to have DTS.

(That said, I think DTS is great and we internally should use DTS -- but that would require some non-trivial work from our side. And that would require some more official longer-runway announcement; perhaps brought up in TSC)

Copy link
Contributor

@daor-oti daor-oti left a comment

Choose a reason for hiding this comment

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

As @aescolar writes, out of tree architectures are supported and used. We would not be able to use Zephyr without this.
We also cannot support DTS and therefor the HAS_DTS configure option is important to us, so please do not remove that.

@fabiobaltieri
Copy link
Member

@gmarull Yes, there are are indeed platforms without HAS_DTS, e.g. ours. The option may not do much right now, but my concern is over the long-term. DTS being assumed, can leak into many parts of Zephyr and thus making it mandatory to have DTS.

Well but that's already the case since all platforms enables the option, if there's a regression related to HAS_DTS=n it'll get caught downstream, this would be a source of regressions detected late and backports.

If the option is supported there should be some test cases to ensure a minimum level or stability, but if all platforms have been moved it may make sense to make up a deadline to mark it as required and move on.

@daor-oti
Copy link
Contributor

@fabiobaltieri That would be an ideal solution, unfortunately we have a custom toolchain consisting if in-house developed tools, which we cannot share. Unless I am misunderstanding what you are suggesting? We are though looking into the issues and trying to see if we can come up with a solution that will allow us to not block this PR for too long, or possibly suggest a workaround.

@fabiobaltieri
Copy link
Member

@fabiobaltieri That would be an ideal solution, unfortunately we have a custom toolchain consisting if in-house developed tools, which we cannot share. Unless I am misunderstanding what you are suggesting?

Not really, I understand that some tooling is custom but stuff like the Z_DEVICE_DT_DEV_ID breakage could certainly be reproduced upstream with a custom board in a test or something similar. I'm sure you could not cover everything, but you could certainly cover something.

We are though looking into the issues and trying to see if we can come up with a solution that will allow us to not block this PR for too long, or possibly suggest a workaround.

Sure, it's just that if we can have an upstream test that reproduces the specific condition in isolation we could have some confidence that it won't break again.

Also, is there a linker error you could paste here?

@daor-oti
Copy link
Contributor

There are no linker errors and the target seems to be running, but is apparently missing the device drivers. We are investigating and will update here as soon as we have anything useful. Sorry for the holdup and the delays.

coreboot-org-bot pushed a commit to coreboot/zephyr-cros that referenced this pull request Oct 12, 2023
When we have an empty Devicetree, ie,

```
/dts-v1/;

/ {

};
```

The node's dep_ordinal is never initialized because the node graph is
empty. This ends up with invalid ordinal tokens (-1) in
devicetree_generated.h which in turn produce some cryptic compiler
errors, see e.g.

```
token
   95 | #define Z_DEVICE_DT_DEV_ID(node_id) _CONCAT(dts_ord_,
      DT_DEP_ORD(node_id))

...

include/zephyr/devicetree.h:2498:41:
 2498 | #define DT_FOREACH_STATUS_OKAY_NODE(fn)
      DT_FOREACH_OKAY_HELPER(fn)
            |
	    ^~~~~~~~~~~~~~~~~~~~~~
include/zephyr/device.h:1022:1:
     1022 |
	  DT_FOREACH_STATUS_OKAY_NODE(Z_MAYBE_DEVICE_DECLARE_INTERNAL)

```

(devicetree_generated.h)

```
...
 #define DT_N_ORD -1
 #define DT_N_ORD_STR_SORTABLE 000-1
...
```

This patch makes sure root node is always inserted (without any target)
so that it gets initialized later.

Discovered as part of
zephyrproject-rtos/zephyr#63696

(cherry picked from commit 7772cec)

Original-error: pasting "dts_ord_" and "-" does not give a valid preprocessing
Original-note: in expansion of macro 'DT_FOREACH_OKAY_HELPER'
Original-note: in expansion of macro 'DT_FOREACH_STATUS_OKAY_NODE'
Original-Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>
GitOrigin-RevId: 7772cec
Change-Id: I8a3717a58e889c0c87734fc73e27a40eaf7c6f4c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4933942
Commit-Queue: Keith Short <keithshort@chromium.org>
Tested-by: ChromeOS Prod (Robot) <chromeos-ci-prod@chromeos-bot.iam.gserviceaccount.com>
Tested-by: Keith Short <keithshort@chromium.org>
Reviewed-by: Keith Short <keithshort@chromium.org>
By providing a devicetree stub file, we make sure some internal macros
required by devicetree.h are generated in devicetree_generated.h. This
makes sure that systems without devicetree can continue working without
extra ifdeffery.

Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>
HAS_DTS has become a redundant option. All Zephyr architectures now
select this option, meaning devicetree has become a de-facto
requirement.  In fact, if any board does not provide a devicetree
source, the build system uses an empty stub, meaning the devicetree
machinery always runs.

Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>
@gmarull
Copy link
Member Author

gmarull commented Oct 16, 2023

There are no linker errors and the target seems to be running, but is apparently missing the device drivers. We are investigating and will update here as soon as we have anything useful. Sorry for the holdup and the delays.

Please provide any logs or information, so it can be investigated. It looks like a bug that was being papered by HAS_DT, same as the one that this PR fixes.

Copy link
Contributor

@daor-oti daor-oti left a comment

Choose a reason for hiding this comment

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

Thanks once again for your patience.
We seem to have resolved our issues and all should be good from our side.

@mbolivar-ampere mbolivar-ampere merged commit 915cb05 into zephyrproject-rtos:main Oct 20, 2023
24 checks passed
@gmarull gmarull deleted the has-dts-drop branch October 20, 2023 20:16
microbuilder pushed a commit to microbuilder/zephyr that referenced this pull request Oct 30, 2023
When we have an empty Devicetree, ie,

```
/dts-v1/;

/ {

};
```

The node's dep_ordinal is never initialized because the node graph is
empty. This ends up with invalid ordinal tokens (-1) in
devicetree_generated.h which in turn produce some cryptic compiler
errors, see e.g.

```
error: pasting "dts_ord_" and "-" does not give a valid preprocessing
token
   95 | #define Z_DEVICE_DT_DEV_ID(node_id) _CONCAT(dts_ord_,
      DT_DEP_ORD(node_id))

...

include/zephyr/devicetree.h:2498:41:
note: in expansion of macro 'DT_FOREACH_OKAY_HELPER'
 2498 | #define DT_FOREACH_STATUS_OKAY_NODE(fn)
      DT_FOREACH_OKAY_HELPER(fn)
            |
	    ^~~~~~~~~~~~~~~~~~~~~~
include/zephyr/device.h:1022:1:
note: in expansion of macro 'DT_FOREACH_STATUS_OKAY_NODE'
     1022 |
	  DT_FOREACH_STATUS_OKAY_NODE(Z_MAYBE_DEVICE_DECLARE_INTERNAL)

```

(devicetree_generated.h)

```
...
 #define DT_N_ORD -1
 #define DT_N_ORD_STR_SORTABLE 000-1
...
```

This patch makes sure root node is always inserted (without any target)
so that it gets initialized later.

Discovered as part of
zephyrproject-rtos#63696

Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet