Skip to content

dts: Add device type handing support #17976

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

Closed
wants to merge 4 commits into from

Conversation

galak
Copy link
Collaborator

@galak galak commented Aug 2, 2019

No description provided.

Copy link
Collaborator

@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.

I think this adds complexity for no value. We already have the compatible property, and this just adds another property that works the same way.

Having cpu nodes without device_type is common in the Linux kernel, if you're worried about existing practice. See this, this, this, and this for example.

The schema you found in #17860 doesn't say that device_type is required for cpu nodes. It says that cpu nodes should have at least one of device_type, reg, or compatible (note the anyOf).

device_type is deprecated in the devicetree specification:

The device_type property was used in IEEE 1275 to describe the device's
FCode programming model. Because DTSpec does not have FCode, new use of the
property is deprecated, and it should be included only on `cpu` and
`memory` nodes for compatibility with IEEE 1275–derived devicetrees.

(IEEE 1275 is a standard from 1994.)

The wording is a bit ambiguous, but I think it's just making concessions for old IEEE 1275-derived devicetrees, by saying that it's okay to have device_type on cpu and memory nodes (but not on any other nodes). It's not saying that you must have device_type on those.

That leaves device_type = "memory", which seems to be put in pretty consistently in the Linux .dts files, and is required by at least one random schema file. I think we should just remove it along with device_type = "cpu" still, since we don't use it. The compatible mechanism is all we need. If we absolutely must have it, we could require it in the binding though, along with a comment that says it's unused.

@galak
Copy link
Collaborator Author

galak commented Aug 2, 2019

I think this adds complexity for no value. We already have the compatible property, and this just adds another property that works the same way.

Having cpu nodes without device_type is common in the Linux kernel, if you're worried about existing practice. See this, this, this, and this for example.

The schema you found in #17860 doesn't say that device_type is required for cpu nodes. It says that cpu nodes should have at least one of device_type, reg, or compatible (note the anyOf).

device_type is deprecated in the devicetree specification:

The device_type property was used in IEEE 1275 to describe the device's
FCode programming model. Because DTSpec does not have FCode, new use of the
property is deprecated, and it should be included only on `cpu` and
`memory` nodes for compatibility with IEEE 1275–derived devicetrees.

(IEEE 1275 is a standard from 1994.)

The wording is a bit ambiguous, but I think it's just making concessions for old IEEE 1275-derived devicetrees, by saying that it's okay to have device_type on cpu and memory nodes (but not on any other nodes). It's not saying that you must have device_type on those.

That leaves device_type = "memory", which seems to be put in pretty consistently in the Linux .dts files, and is required by at least one random schema file. I think we should just remove it along with device_type = "cpu" still, since we don't use it. The compatible mechanism is all we need. If we absolutely must have it, we could require it in the binding though, along with a comment that says it's unused.

I understand your intent in the comments, but my goal is to eventually pull edtlib.py out of Zephyr and have it as a standalone project that can be used by others. As such support for device_type and semantics used by Linux is going to be desired.

Additionally, we've had a long term goal of sharing device tree's across projects (there are already STM32 dts files in Linux for example). So I understand the desire not to clutter things for various semantics we might not like in Zephyr, but I don't think we can fully take that stance.

@galak galak force-pushed the dts-device-type branch 2 times, most recently from 365f823 to 4916910 Compare August 2, 2019 15:31
@pabigot
Copy link
Collaborator

pabigot commented Aug 2, 2019

This is a meta-comment: I have no opinion about the technical aspects of this particular PR.

I'm swayed by the argument of standard conformance to support reusability, applying both here and #17829. So, no default support as currently proposed in #17928.

However: this cuts both ways. If we say we want devicetree source to be compatible with non-Zephyr systems, we should not be using our own schemas but instead have a plan to move to the dt-schema hierarchy for both compatible names and for validation.

And we need to be more careful about using standard properties appropriately. I have not changed my position on Zephyr's (mis)use of label. Zephyr requires something that can be used at runtime in device_get_binding() to reference node information that in a "real" implementation could be accessed by the node path? Fine, but that's a result of an architectural choice made in Zephyr. It's not what label is normally used for, and the property should be something like zephyr,device-id.

But that assumes the schema is extensible so that the presence of environment-specific properties can be validated. (Which also kills the "diagnose unrecognized properties" feature. Annoying!)

I don't plan to look into this so I'm blue-skying here, but if the standard supports it, I think #17928 can be salvaged by naming the YAML entry zephyr,default, which is interpreted specifically by Zephyr's tooling with the semantics that a required property that is absent but has a defined default is synthesized to have the default.

Similarly, things like setting driver init group and priority--which is a critical gap (#17830) for multi-instance devices--are specific to the way Zephyr uses devicetree content. I want to see something like this in the YAML for GPIO [*]:

zephyr,init-group:
    type: string
    category: optional
    zephyr,default: "POST_KERNEL"
    enum:
      - "PRE_KERNEL_1"
      - "PRE_KERNEL_2"
      - "POST_KERNEL"
      - "APPLICATION"

zephyr,init-priority:
    type: int
    category: optional
    zephyr,default: 40

Then, in the devicetree, I can enforce the conditions that an SX1509B driver hanging off an I2C bus has an init priority that places it after the parent bus is initialized. Right now that's done with a Kconfig setting, which is, simply, wrong. [**]

In short: we need be consistent in what we're aiming for, and make sure everybody understands and accepts it, so we stop wasting time working on features that aren't acceptable, and so we end up with a system that meets clearly-defined goals. A good way to start would be to write it down and reference it when assessing whether a proposed solution is appropriate. (It may be written down, but most of the rejections I've seen are independently justified rather than based on reference to a documented plan.)

[*] In fact I really need this, and can't get it without modifying the scripting, so I'm again giving up on a generic regulator to support ultra-low-power sensor applications (see #17830).

[**] IMO the correct way to deal with device initialization is to create a dependency graph based on devicetree node relations, then use a strongly-connected-components algorithm to sort them into an initialization order that is correct, rather than relying on magic numbers sprinkled throughout the infrastructure. You'd still need zephyr,init-group to group into desired initialization contexts, but the sorting could diagnose cross-group dependencies.)

@ulfalizer
Copy link
Collaborator

I understand your intent in the comments, but my goal is to eventually pull edtlib.py out of Zephyr and have it as a standalone project that can be used by others. As such support for device_type and semantics used by Linux is going to be desired.

I don't think we should put any effort into supporting deprecated Linux hacks that we don't need in Zephyr, especially not at this point. device_type is an old and bad way of doing things that's only around for legacy reasons.

The C tools do not special-case device_type in any way by the way. It's just a limitation of how our schema stuff is done.

Additionally, we've had a long term goal of sharing device tree's across projects (there are already STM32 dts files in Linux for example). So I understand the desire not to clutter things for various semantics we might not like in Zephyr, but I don't think we can fully take that stance.

Fixing them up is trivial, and having clean and easy-to-understand devicetree files is much more important than allowing people to blindly copy-paste stuff in.

@galak
Copy link
Collaborator Author

galak commented Aug 2, 2019

However: this cuts both ways. If we say we want devicetree source to be compatible with non-Zephyr systems, we should not be using our own schemas but instead have a plan to move to the dt-schema hierarchy for both compatible names and for validation.

I agree and have stated thats a goal to convert the bindings to use dt-schema in the future.

@galak
Copy link
Collaborator Author

galak commented Aug 2, 2019

And we need to be more careful about using standard properties appropriately. I have not changed my position on Zephyr's (mis)use of label. Zephyr requires something that can be used at runtime in device_get_binding() to reference node information that in a "real" implementation could be accessed by the node path? Fine, but that's a result of an architectural choice made in Zephyr. It's not what label is normally used for, and the property should be something like zephyr,device-id.

I agree with this as well, the first step was the cleanup the base scripts and introduce edtlib.py that we could utilize for code generation. Now that we've got a cleaner base I think it will be easier to move to a model in which we don't need the strings at all and could remove the label usage.

@galak
Copy link
Collaborator Author

galak commented Aug 2, 2019

[**] IMO the correct way to deal with device initialization is to create a dependency graph based on devicetree node relations, then use a strongly-connected-components algorithm to sort them into an initialization order that is correct, rather than relying on magic numbers sprinkled throughout the infrastructure. You'd still need zephyr,init-group to group into desired initialization contexts, but the sorting could diagnose cross-group dependencies.)

I imagine this is need for power management as well.

@galak
Copy link
Collaborator Author

galak commented Aug 2, 2019

I understand your intent in the comments, but my goal is to eventually pull edtlib.py out of Zephyr and have it as a standalone project that can be used by others. As such support for device_type and semantics used by Linux is going to be desired.

I don't think we should put any effort into supporting deprecated Linux hacks that we don't need in Zephyr, especially not at this point. device_type is an old and bad way of doing things that's only around for legacy reasons.

The deprecation in Linux and the DT spec is with regards to new device classes and not propagating the use of device_type. However its a long established practice with device trees that memory nodes for things like DDR/SDRAM don't have a compatible and just a device_type. Additionally there are other nodes like PCI that utilize device_type.

The C tools do not special-case device_type in any way by the way. It's just a limitation of how our schema stuff is done.

Which C tool are you referring too?

Additionally, we've had a long term goal of sharing device tree's across projects (there are already STM32 dts files in Linux for example). So I understand the desire not to clutter things for various semantics we might not like in Zephyr, but I don't think we can fully take that stance.

Fixing them up is trivial, and having clean and easy-to-understand devicetree files is much more important than allowing people to blindly copy-paste stuff in.

The devicetree model in other projects isn't as fluid as it has been in Zephyr, so one has to take some of the baggage along with it. For example since in Linux or other projects a binary DTB get used there contents are far more static and thus you can't just say we can change the dts files.

@ulfalizer
Copy link
Collaborator

The deprecation in Linux and the DT spec is with regards to new device classes and not propagating the use of device_type. However its a long established practice with device trees that memory nodes for things like DDR/SDRAM don't have a compatible and just a device_type.

It's that way for legacy reasons in Linux, not because it's a good way to do things. cpu nodes have started to move towards only having compatible.

The compatible = "mmio-sram", compatible = "arc,iccm", etc., that we have now are a big step up from Linux. It's probably what they will move towards if anyone ever steps up to clean it up (kinda doubtful). We shouldn't remove a decent and go back to an old crappy design.

Supporting copy-paste of files with poorly-designed legacy stuff in them isn't worth the effort I think. Fixing it takes second. A much bigger problem that we have is that our devicetree stuff is unnecessarily complex, and this adds to it.

Additionally there are other nodes like PCI that utilize device_type.

Do we need that? If you want to follow the spec., it disallows it.

Which C tool are you referring too?

dtc... though I guess it doesn't special-case compatible either.

The devicetree model in other projects isn't as fluid as it has been in Zephyr, so one has to take some of the baggage along with it. For example since in Linux or other projects a binary DTB get used there contents are far more static and thus you can't just say we can change the dts files.

I don't get it. If someone puts device_type = "cpu" on a node and edtlib says it's unnecessary and they remove it, then that's it.

However: this cuts both ways. If we say we want devicetree source to be compatible with non-Zephyr systems, we should not be using our own schemas but instead have a plan to move to the dt-schema hierarchy for both compatible names and for validation.

I'm not against moving to something like dt-schema in the long run (I think... haven't looked enough at it), but that's probably a long way off. In the meantime, there are lots of improvements and simplifications that could be made to what we have now. They wouldn't move us any further from dt-schema either.

One minor iffy thing is that we currently do an ugly regex thing to find bindings, inherited from the old code (see

match = re.match(r'\s+constraint:\s*"([^"]*)"', line)
). I've timed it, and it gets too slow (like 15 seconds) without it. dt-schema seems to do something fancier that might not allow that hack.

Don't know if it'd be a problem though. Wonder if we could just match on filename instead.

Another thing is that dt-schema (afaiu) is just for verifying the format of .dts files, and not for output control. Maybe it has what we need for output-control still though.

@galak
Copy link
Collaborator Author

galak commented Aug 2, 2019

It's that way for legacy reasons in Linux, not because it's a good way to do things. cpu nodes have started to move towards only having compatible.

The compatible = "mmio-sram", compatible = "arc,iccm", etc., that we have now are a big step up from Linux. It's probably what they will move towards if anyone ever steps up to clean it up (kinda doubtful). We shouldn't remove a decent and go back to an old crappy design.

Supporting copy-paste of files with poorly-designed legacy stuff in them isn't worth the effort I think. Fixing it takes second. A much bigger problem that we have is that our devicetree stuff is unnecessarily complex, and this adds to it.

I'm not against removing device_type = "memory" for the true cases of mmio-sram. However there are cases like on the i.mx-rt SoCs in which its actually DDR/SRAM in that case mmio-sram doesn't make sense and the prescribed mechanism used by DTS forever has be a device_type="memory" node w/o any compatible.

@pabigot
Copy link
Collaborator

pabigot commented Aug 2, 2019

However: this cuts both ways. If we say we want devicetree source to be compatible with non-Zephyr systems, we should not be using our own schemas but instead have a plan to move to the dt-schema hierarchy for both compatible names and for validation.

I agree and have stated thats a goal to convert the bindings to use dt-schema in the future.

Let me ask more clearly: Is there a written down roadmap of where Zephyr is heading with devicetree? Including the overall goals and constraints, which people reviewing PRs can use as reference in assessing them for architectural consistency? Rather like #17706 that was provided to meet that demand before POSIX changes were acceptable? Except maybe even more detailed, since it's such a large scale and long term activity with multiple intermediate steps.

Because there needs to be such a plan, possibly even a github project to manage it. We can't be expected to know everything that's been said in a comment or slack discussion somewhere. Proposing, preparing, and submitting PRs for workarounds that you're going to reject is a waste of time. And we can't give quality feedback on PRs without an understanding of what they're intended to do and how they advance Zephyr to a desired end state.

This one doesn't even have a description.

@ulfalizer
Copy link
Collaborator

...prescribed mechanism used by DTS forever has be a device_type="memory" node w/o any compatible.

For legacy reasons, not because it's a good way to do things. Linux has reasons to preserve that legacy, because device_type is actually used there.

The writing on the wall in the spec. and devicetree-org/devicetree-specification#29 is that no one would be sad to see device_type go away. We never used it to begin with (probably because it's bad, so using it never came naturally), and starting to use it seems like a step in the wrong direction. We have a chance to remove some crap that other systems have to deal with.

For pure RAM nodes, why not change compatible = "mmio-sram" to compatible = "ram" or the like? The only thing bad about the mmio-sram "hack" is the compatible string. Otherwise, it seems like a great way to do things.

If you look at the pattern in my PRs (lots of documentation, commenting, and code cleanups), a lot of it is about making things simpler to understand and cleaner. I'd be happy to put in a custom message that tells people what to do instead if they put in a device_type = "cpu" or device_type = "memory" or the like. I feel bad when people are confused by stuff I wrote or if I feel like I'm wasting their time, so I always try to avoid that.

We could even put in support for device_type into edtlib later, for compatibility with other systems besides Zephyr (what else are you planning to use it on?). The reason I don't want to do it now is that I'm afraid that it'll be used as an excuse to keep (or even extend) the currently-unused device_type stuff, because "see, we're using it!"

One thing that irks me about even adding support in edtlib though is that it's a step towards the unreadable mess in the old scripts.

Proposing, preparing, and submitting PRs for workarounds that you're going to reject is a waste of time.

The stuff that's getting rejected seems to be exactly the kind of stuff I was planning to work on too, so it's pretty frustrating for me. Our devicetree stuff is too complex and quirky and poorly documented, even with the rewrite, so I want to change it to be simpler. I won't do anything that I think would make people more confused, because that would make me feel bad.

(Family of teachers, so maybe I'm damaged from that. :P)

@galak galak force-pushed the dts-device-type branch from 4916910 to b9093e2 Compare August 6, 2019 09:06
@galak galak changed the title dts: Add device type handing support [TOPIC: DTS] dts: Add device type handing support Aug 13, 2019
@galak galak changed the base branch from master to topic-dts August 13, 2019 12:46
@erwango
Copy link
Member

erwango commented Aug 16, 2019

Agree with @galak here.
I agree device_type might seem superfluous, but removing it or preventing its usage won't be as easy as it sounds.
For zephyr, maybe we'd be able to do w/o it.
For zephyr using Linux files, I would find it much more complex.
But if we think of using edtlib outside of zephyr then I don't see it as feasible.

If we want edtlib to be used outside zephyr, then we need to adapt it to (rather common) dts practices, because we won't be able to impose new usages in dts because of edtlib. As said device tree guys (devicetree-org/devicetree-specification#29 (comment))

While we'd like to drop device_type, we can't

And then, about (in same comment):

Though I guess that could be relaxed on new platforms.

Ok, but if we want to be able to enjoy multiplatform build with zephyr using edtslib, then, we don't fall in that category.

@pabigot
Copy link
Collaborator

pabigot commented Aug 16, 2019

If we want edtlib to be used outside zephyr, then we need to adapt it to (rather common) dts practices, because we won't be able to impose new usages in dts because of edtlib.

Yes. In the same vein, I remain concerned that we seem to be developing parsing/validation code and making policies about what's an error/warning/ignored when dt-schema clearly has the ability to process schema, and to validate both schema and devicetree data. We should be trying to use or integrate with as much of that infrastructure as possible.

I understand this may be a goal that can't be reached now because of past Zephyr-specific decisions, but if there's no plan then the new effort is just going to create more incompatibilities that will have to be worked around in the future.

@galak galak force-pushed the topic-dts branch 2 times, most recently from c4ec776 to 1a7d15a Compare September 7, 2019 13:54
@galak galak closed this Sep 7, 2019
@galak galak reopened this Sep 7, 2019
Rename the helper function _binding_compat to _binding_constraint as its
not limited to matching a constraint on the compatible property but can
also easily work for a constraint on any property.

Signed-off-by: Kumar Gala <[email protected]>
Rename matching_compat to matching_constraint to be more generic since
we could have the constraint be on something other than a compatible
property.

Signed-off-by: Kumar Gala <[email protected]>
Treat device_type similar to compatiable, use it to match a binding if
there isn't a compatiable.

Signed-off-by: Kumar Gala <[email protected]>
Add support for handling generation of defines if the binding match is
based on device_type and not compatiable.  This is primarily for
handling memory nodes which don't typically have a compatiable property.

Signed-off-by: Kumar Gala <[email protected]>
@galak galak changed the title [TOPIC: DTS] dts: Add device type handing support dts: Add device type handing support Sep 9, 2019
@galak galak changed the base branch from topic-dts to master September 9, 2019 14:19
@galak galak added area: Devicetree Tooling PR modifies or adds a Device Tree tooling and removed area: Devicetree labels Jan 30, 2020
@erwango
Copy link
Member

erwango commented May 4, 2020

@galak, should we keep this one ?

@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jun 29, 2020
@galak galak closed this Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Tooling PR modifies or adds a Device Tree tooling has-conflicts Issue/PR has conflicts with another issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants