Skip to content

Conversation

@pabigot
Copy link
Contributor

@pabigot pabigot commented Sep 1, 2020

Clean up of devicetree tooling removed generation of information present in devicetree in nodes that have no compatible, or for extra properties not defined by a binding. Discussion proposed that these properties should be allowed, but only in a defined node /zephyr,user. For that node infer bindings based on the presence of properties.

Fixes: #25945
Supersedes: #18544, #15020

@pabigot pabigot added dev-review To be discussed in dev-review meeting area: Devicetree Tooling PR modifies or adds a Device Tree tooling labels Sep 1, 2020
@pabigot pabigot requested a review from ulfalizer as a code owner September 1, 2020 15:29
Copy link
Contributor

@mbolivar-nordic mbolivar-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'd really like to see this fleshed out in the form of test cases in tests/lib/devicetree/api and documentation in the user guides.

I know where this is going, and I'm for it, but I'd like to see the t's crossed and i's dotted. Let me know if you don't have cycles and want help here.

@b0661
Copy link
Contributor

b0661 commented Sep 2, 2020

I look at this without knowing the discussions in the TSC.

I understand that you create a "dynamic" binding for a virtual compatible (infer by properties) that is associated only to the node with the special path "/zephyr,user".

The mechanism is Zephyr special - it does no exist in the DT specification -, it is hidden - you have to know about the special handling of the node "/zephyr,user" - and it is hard coded - the path has to be "/zephyr,user".

My thoughts are whether the same effect can be achieved without being hard coded and without being hidden. The idea is to introduce the compatible "infer,by_properties". edtlib could detect this special compatible for "dynamic" bindings and do the same thing as it does in this solution. Using a compatible also allows to apply this compatible to nodes where the normal binding does not extract all of the properties and add binding information for these to the available binding.

I think the benefit of this idea could outweight the disadvantage that you have to add the compatible "infere,by_properties" to the "/zephyr,user" node. Which btw. can be avoided if wanted the same way as it is now done for "/zephyr,user". .

Besides the idea my wish is: do not add specific features hard coded for Zephyr to the generic library edtlib. Alt least make the node path configurable and make it a list. This allows non-Zephyr users of edtlib to have node pathes more appropriate to their environment.

Bobby

@pabigot
Copy link
Contributor Author

pabigot commented Sep 2, 2020

@b0661 The requirements and design discussion for this feature are summarized in #25945 (comment). @galak required that support be isolated to a single node to preserve the integrity of binding validation.

edtlib is where bindings and devicetree structure are combined, so I believe the core capability of inferring a binding from a node has to live there. However, I've updated this so the EDT instance is told the paths to which inference is applied.

Does edtlib exist anywhere outside of Zephyr? If so, who's maintaining it and why has it diverged?

@pabigot pabigot requested a review from nashif as a code owner September 2, 2020 10:11
@pabigot
Copy link
Contributor Author

pabigot commented Sep 2, 2020

I'd really like to see this fleshed out in the form of test cases in tests/lib/devicetree/api and documentation in the user guides.

Agreed about the documentation; I've added something for that.

I don't see how this impacts the devicetree API, though. The change is solely to edtlib where it's fully (I think) tested. I supposed we could define more helper macros, but I believe this is already functionally complete.

What did you have in mind?

@b0661
Copy link
Contributor

b0661 commented Sep 2, 2020

Does edtlib exist anywhere outside of Zephyr?

Yes, it is part of my tool for inline code generation 'Cogeno' that originally was developed to replace the former Zepyhr extract_dts_includes.py script (see #10888, https://cogeno.readthedocs.io/en/latest/) and now is part of our internal tool chain also:

The docs and the gitlab repo is a little bit outdated. Sync to internal version is in preparation.

If so, who's maintaining it

Me

and why has it diverged?

One reason is that Zephyr could not take extensions (e.g. #22255) necessary to edtlib to fulfill my main interest: the implementation of a Pinctrl driver for Zephyr similar to the Linux one.

@galak
Copy link
Contributor

galak commented Sep 2, 2020

I would like to see if we can avoid having zephyr,user in edtlib.py and either be data driven or added to gen_defines.py.

@pabigot
Copy link
Contributor Author

pabigot commented Sep 2, 2020

I would like to see if we can avoid having zephyr,user in edtlib.py and either be data driven or added to gen_defines.py.

@galak zephyr,user isn't in edtlib.py anymore (with my latest update). The ability to infer the binding from the dtlib node data is, with an option to specify which (if any) node paths use it. By default none do, and gen_defines enables it for zephyr,user.

Architecturally it doesn't make sense to do it anywhere else, since it's an integral part of doing the dtlib to EDT conversion.

@mbolivar-nordic
Copy link
Contributor

I supposed we could define more helper macros, but I believe this is already functionally complete.
[...] What did you have in mind?

Something like:

  • a DT_ZEPHYR_USER node identifier macro in include/devicetree/zephyr.h, defined as DT_PATH(zephyr_user)
  • documentation showing how to access zephyr,user properties (at least ad-hoc GPIOs) in the DT HOWTOs, so we have a single link to point people at when they ask the FAQ about this on slack or elsewhere
  • a test case verifying the ad-hoc GPIOs work as documented

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary, or can it be set to None? I'm not fond of faking out values like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As implemented this produces:

 * Devicetree node: /zephyr,user
 *
 * Node's generated path identifier: DT_N_S_zephyr_user
 *
 * Binding (compatible = zephyr,user):
 *   @synthesized/zephyr,user
 *
 * Dependency Ordinal: 9
 *
 * Requires:
 *   0   /
 *   7   /soc/gpio@50000000
 *   8   /soc/gpio@50000300
 */

Setting to None produces:

Traceback (most recent call last):
  File "/mnt/nordic/zp/zephyr/scripts/dts/gen_defines.py", line 744, in <module>
    main()
  File "/mnt/nordic/zp/zephyr/scripts/dts/gen_defines.py", line 82, in main
    write_node_comment(node)
  File "/mnt/nordic/zp/zephyr/scripts/dts/gen_defines.py", line 184, in write_node_comment
    {relativize(node.binding_path)}
  File "/mnt/nordic/zp/zephyr/scripts/dts/gen_defines.py", line 219, in relativize
    return str("$ZEPHYR_BASE" / pathlib.Path(path).relative_to(zbase))
  File "/usr/lib/python3.8/pathlib.py", line 1033, in __new__
    self = cls._from_parts(args, init=False)
  File "/usr/lib/python3.8/pathlib.py", line 674, in _from_parts
    drv, root, parts = self._parse_args(args)
  File "/usr/lib/python3.8/pathlib.py", line 658, in _parse_args
    a = os.fspath(a)
TypeError: expected str, bytes or os.PathLike object, not NoneType

That can be changed if you wish; please provide the text you'd like to see for a synthesized binding.

Copy link
Contributor

Choose a reason for hiding this comment

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

That can be changed if you wish; please provide the text you'd like to see for a synthesized binding.

Tweaking this path to add the appropriate null checks and changing the text to something like: "None, bindings were inferred from property types" would get my +1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some dithering:

 * Binding (compatible = nordic,nrf-gpio):
 *   $ZEPHYR_BASE/dts/bindings/gpio/nordic,nrf-gpio.yaml
...
 * Binding (compatible = zephyr,user):
 *   No yaml (bindings inferred from properties)

OK?

@pabigot pabigot added this to the v2.4.0 milestone Sep 3, 2020
@pabigot pabigot force-pushed the nordic/20200901a branch 2 times, most recently from 4674ba1 to a7e198f Compare September 3, 2020 16:02
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a subnode in here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Subnodes aren't processed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify in here if child of the path are covered or just the path itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

(probably add to the docs as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, but it does say "path", not "path prefix".

Copy link
Contributor

@galak galak left a comment

Choose a reason for hiding this comment

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

In general looks good. Just some clarify and testing improvement for sub-nodes.

Happy to ack we just the doc update on sub-nodes and we can handle the test improvement later if needed.

Clean up of devicetree tooling removed generation of information
present in devicetree in nodes that have no compatible, or for extra
properties not defined by a binding.  Discussion proposed that these
properties should be allowed, but only in a defined node /zephyr,user.
For that node infer bindings based on the presence of properties.

Signed-off-by: Peter Bigot <[email protected]>
Tell the EDT instance that properties of the /zephyr,user node should
be generated based on the binding types inferred from the property
content.

Signed-off-by: Peter Bigot <[email protected]>
Identify the special case of /zephyr,user as a node for which property
values are generated without having to define a binding.

Signed-off-by: Peter Bigot <[email protected]>
@galak galak merged commit 1357696 into zephyrproject-rtos:master Sep 4, 2020
@galak galak removed the dev-review To be discussed in dev-review meeting label Sep 4, 2020
@pabigot pabigot deleted the nordic/20200901a branch September 5, 2020 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

devicetree: support generating symbols for -gpios properties w/o compatible

4 participants