devicetree-org / dt-schema

Devicetree schema tools
http://www.devicetree.org
BSD 2-Clause "Simplified" License
67 stars 64 forks source link

schemas: bootph: Add check for bootph tags in parent nodes #99

Open robherring opened 1 year ago

robherring commented 1 year ago

When a node has boot phase tags, the parent node is implicitly included and a bootph tag is not needed. Add a schema to check for and disallow bootph tags in both parent and child nodes. It's not the greatest logic and resulting error message, but that's what works for json-schema.

sjg20 commented 1 year ago

If I understand it correctly, this will break all existing devicetree files in U-Boot, since the build system does not support this yet. I need to get the fdtgrep tool upstreamed and then enhance it to handle this, then update U-Boot. I believe it if feasible but it is going to take some time.

robherring commented 1 year ago

If I understand it correctly, this will break all existing devicetree files in U-Boot, since the build system does not support this yet. I need to get the fdtgrep tool upstreamed and then enhance it to handle this, then update U-Boot. I believe it if feasible but it is going to take some time.

It only breaks them if you run the checks. Nothing really breaks, just adds to the pile of warnings many platforms have. Does anyone actually run dtschema on u-boot copies of DTs?

I'm assuming the order here is enhance fdtgrep and u-boot first, then submit bootph tags to Linux copies of dts files. So this change is primarily to check the Linux copies. And from the look of it, some platforms may not need to wait as they don't have this issue.

sjg20 commented 1 year ago

We don't run the schema in U-Boot, but would like to one day. But once we have things in Linux, your warning is going to advise people to change the tags, and then U-Boot won't work. It will be very confusing.

Are you saying that you won't accept DTs into Linux until the fdtgrep stuff is done? That is really putting pressure on all of this...

robherring commented 1 year ago

Are you saying that you won't accept DTs into Linux until the fdtgrep stuff is done? That is really putting pressure on all of this...

Yes, that's my position. The dts files in Linux are loosely coupled to u-boot compared to dts files in u-boot tree. IOW, it's an ABI. Plus, why would we add some tags and then later remove them. Just wait until things are stable. However, I don't review (typically) nor apply dts patches (have to pick my firehoses). I do look at overall warnings by platform families though.

sjg20 commented 1 year ago

OK, well perhaps initially I will just upstream what we have and worry about tidying up later.

I had a bit of a look at what it would take to implement your scheme. Options I came up with:

  1. Build a table of tags in each node and look that up when scanning the properties of a node, processing the inferred tags too (without emitting them)
  2. Preprocess the tree to insert tags in parents
  3. Move to a recursive approach for emitting the tree

In terms of development time, probably 2 is best.

If you want to have a look, see fdt_next_region() where it has the 'case FDT_PROP'. The h_include() function is in fdtgrep.c

Obviously fdtgrep needs to know that the bootph things must be propagated upwards. That could be done by adding a new flag in addition to --include-node-with-prop (such as --include-tree-with-prop)

nmenon commented 2 weeks ago

did we decide to pull this in?

sjg20 commented 1 week ago

Quote reply

Well I implemented option 2 in fdtgrep in U-Boot, so it is working. The commit is:

7a06cc2027c fdtgrep: Allow propagating properties up to supernodes

which landed in v2024.04

nmenon commented 1 week ago

Yes, and I would like to catch places where we can clean up in the kernel if we have this as part of a formal release.