lopper: assists: xlnx_overlay_pl_dt: add --user-dtsi option to append user overlay content#718
lopper: assists: xlnx_overlay_pl_dt: add --user-dtsi option to append user overlay content#718arthokal wants to merge 1 commit intodevicetree-org:masterfrom
Conversation
… user overlay content
Add --user-dtsi=<path> option allowing users to append custom overlay
content to the generated pl.dtsi output. The provided file undergoes
lightweight validation: checks for presence of at least one reference
node (&label { ... }) and rejects /dts-v1/ and /plugin/ headers.
Also refactor validate_and_parse_options() to return a named tuple
(OverlayOptions) instead of a bare tuple, and warn on unrecognized
options.
Signed-off-by: Aravind Thokala <aravind.thokala@amd.com>
|
You closed the old pull request without answering my question. Those questions still stand. |
|
Hi @zeddii, Adding questions from previous closed PR here:
Adding @onkarharsh to the discussion. |
|
Hi @zeddii, This is more related to the prototype work that you mentioned above. In this case, the user dtsi must not be merged to the base system device tree, instead this needs to go to the pl overlay as is, which is generated after the assist operation. We were not aware that you already have something of this sort in the front-end. We do need this support especially for creating overlays if this PR is going to be rejected. Do you have a timeline for the proposed solution? This Pull Request is critical for our release timeline. |
|
It may not be ready for a few weeks yet, there are other pressing issues. But so I understand better as to what you are trying to do. Why can't those changes be merged into the main tree, when the overlay's should be created/pulled out of the main tree. Is it that those use supplied additions aren't fully "trusted" ? something else ? What assists do you see / want in the pipeline when this overlay is created ? |
|
Hi @zeddii One use case of not merging changes into main tree is this: Sometimes a user needs to update a PS block (like a display controller) to use clocks coming from the PL side. These PL clocks are provided by IPs in the FPGA design and only become available after the FPGA bitstream is loaded. If these changes are added to the base device tree, Linux will try to use those PL clocks at boot — before the FPGA is programmed — and fail because the clocks don't exist yet. That's why these changes need to go into the PL overlay, so they only take effect after the FPGA is up and the PL clocks are actually available. Beyond PS-to-PL dependencies, it also lets users disable PL nodes, add custom properties or apply any overlay-level tweak. |
|
But if we create an overlay from the base tree, the logic of lopper-core is that those resources should be removed from the base device tree, and hence linux won't see them. If that was the case, would that meet the need. Yes, I'll still do a feature to carry the supporting/secondary trees, but I don't think it is needed in this case. The point is that, if it loads with the main tree, we can validate that it is correct, then pull it out as an overlay and again, validate that it is correct. If it arrives late like that to an assist, we can't do that. |
|
Hi @zeddii Using Three use cases are identified:
Problem with use case 3: After Proposed solution:
Issue: Mixed PS+PL phandles Step 4 has a problem when a property contains both PS and PL phandles. Deleting entire property loses original PS clock reference. This mixed PS+PL phandle use case appears valid(PS clock available at boot, PL clock added later). Could you please let us know how we should handle properties containing both PS and PL phandle references? CC: @onkarharsh , @sivadur |
|
Understood. Let me ask a clarifying question you say: "After -i merges for example &zynqmp_dp = { clocks = <&pl_clk_0> to base tree, PS node has a PL dependency. If this stays in base DT, Linux tries to resolve PL clocks at boot before FPGA loads causing boot failure." So the clock is in the pl (the overlay), lopper by default will delete the clock if the phandle target node isn't present in the device tree. If we pull that clock into the overlay, then it will no longer be resolved. Lopper can delete it. Lopper understands "records" in a property, so it is supposed to only delete the record with the undefined phandle. A secondary pass isn't triggered now, but it could be (with some associated tweaks). If what I describe starts working, would that meet the need ? I know where it is likely going wrong and if you get me some sample device trees and expected results, I can get it working. |
|
Hi @zeddii Lopper's strict mode successfully drops invalid phandle records. But, there are two gaps that I identified: Gap 1: Property synchronization Does strict mode only process phandle-bearing properties and not string arrays? Gap 2: No data capture for overlay |
|
We'll have to write something custom for the -names. Because the actual values of the -names are not part of the device tree spec, they can vary. If we have an input schema, then yes, we can enforce it. But that's why it is left alone. You can't be sure which name to drop. That being said, I can do it by index and be right in almost all cases. That could be something that can something configurable (in case it gets it wrong). Invalid phandles aren't dropped until an output is created. So they are in the tree, with their invalid numeric value, just on output they are resolved and dropped if they are invalid. Remind me of the overlay phandles. Don't they have to be valid to the base device tree ? Or are they referring to resources in other overlays or external information ? |
|
Hi @zeddii, These phandles are rendered invalid in base device tree because they are part of the PL bus and the PL bus is carved out from the DT as part of the overlay. When we are dropping such properties from the base device tree, we need to put them back in the carved overlay so that they come into effect after FPGA programming. Let me try to summarize the complete use case that we are trying to support:
Now, when someone wants to insert an overlay on top of the existing pl.dtbo, we need to manually add an include file in the generated overlay and then check its correctness with DTC. That's where we are trying to make use of lopper.
Based on the above discussions, I see we are trying to focus on below two areas:
We see few issues in achieving step2 with the frontend:
Given the complexity involved here, the current suggestion doesn't seem to work. We only want to check the compatibility of this secondary tree coming from overlay with the system device tree, but we don't want to merge this secondary tree to the primary tree. That's where we thought your secondary tree related prototype may come to our rescue. Another possible solution that I can think is to define a separate bus (say overlay-bus) for all such use cases and use that bus in our assists depending on our use cases. For overlays, that bus will be merged to the PL bus. And since they will be passed to lopper frontend, the phandle and other props syntax will also be in check. Let me know your opinion on this summary and the introduction of this new bus. Any suggestions are most welcome. |
Right. That's the core purpose of the overlay_of() routines, they can pull components out, and remove them from the base device tree. We shouldn't lose anything outside of invalid references in the base tree. If we pull those refs into the overlay, then we'll have the same set as the single tree, just split across the two places, and that's completely valid.
Those match my understanding of what we are trying to do.
That's expected and handled by the core when creating an overlay, the complexity isn't anything than has been designed. Lopper could track nodes and property on the way in, there's other uses of this in the future, but that's a complexity we don't want to add right now.
Can you elaborate on why ? Do you want two overlays in the end ? The base tree + our overlay + the user overlay, all separate ? If so, then yes, we'd either create two separate overlays in the assist, or track the overlays as secondary trees in lopper (which is what I was hinting at before).
It still may, assuming you confirm my understanding above about the two separate overlays. If the result is a single, merged overlay (dtbo), then either approach will work.
It's not a bad idea, but let's see about the secondary/support trees and the other options first. Bruce |
Hi @zeddii , Thanks for the response. I am not sure if I understood this splitting of properties across 2 places correctly. If my overlay has: Dropping the <&pl_clk_0 0x1> from base tree (because we have removed pl clocks from the base tree) and dropping the "pl-clk" from clock names (for which we may need a new implementation as mentioned before) seems fine for the base tree consumption. But, in overlay, we expect both the entries of ps_clk and pl_clk in its given form and not the truncated one (with only pl_clk entries). When these properties are kept at two places (one in the base tree and one in the overlay), the overlay overwrite the base tree property. Therefore, splitting across two places is not what we expect, we want to preserve the overlay instead.
How many such properties we can cater to in the frontend. Removing clk-names based on the phandles if not done right can lead to erroneous trees and those errors will be seen only during run time. I feel, the onus for using the right clocks (and the overlay) should be on the user. We should stick to only dt compatibility check between the overlay and the base tree and not do any modification with the overlays passed.
We don't want two separate overlay files, the user overlay and our overlay have to be in one file. As stated in the first example and the above, I am afraid such manipulations can lead to erroneous trees even when the user overlay was correct and that is not ideal. Let me know if I could answer your queries. Looking forward to your suggestions. Regards,
|
|
On Tue, Mar 24, 2026 at 10:04 PM onkarharsh ***@***.***> wrote:
*onkarharsh* left a comment (devicetree-org/lopper#718)
<#718 (comment)>
Hi @zeddii <https://github.com/zeddii>,
These phandles are rendered invalid in base device tree because they are
part of the PL bus and the PL bus is carved out from the DT as part of the
overlay. When we are dropping such properties from the base device tree, we
need to put them back in the carved overlay so that they come into effect
after FPGA programming.
Right. That's the core purpose of the overlay_of() routines, they can pull
components out, and remove them from the base device tree. We shouldn't
lose anything outside of invalid references in the base tree. If we pull
those refs into the overlay, then we'll have the same set as the single
tree, just split across the two places, and that's completely valid.
Hi @zeddii <https://github.com/zeddii> ,
Thanks for the response. I am not sure if I understood this splitting of
properties across 2 places correctly.
If my overlay has:
&mmi_dc {
clock-names = "ps-clk", "pl-clk";
clocks = <&ps_clk_0 0x0> , <&pl_clk_0 0x1>;
}
Dropping the <&pl_clk_0 0x1> from base tree (because we have removed pl
clocks from the base tree) and dropping the "pl-clk" from clock names (for
which we may need a new implementation as mentioned before) seems fine for
the base tree consumption. But, in overlay, we expect both the entries of
ps_clk and pl_clk in its given form and not the truncated one (with only
pl_clk entries). When these properties are kept at two places (one in the
base tree and one in the overlay), the overlay overwrite the base tree
property. Therefore, splitting across two places is not what we expect, we
want to preserve the overlay instead.
If a single clock is in the overlay, and one remains in the base tree. Then
yes, the semantics were intended to be that the phandle for the overlay
clock is removed from the base tree (and the associated index string), but
in the overlay, phandles for the the base tree clock and the overlay clock
should be valid and not be hidden/removed by lopper.
And when they are applied on the platform, the nodes are merged and you get
both in the dtb seen by the OS. i.e. it sees both the clocks again.
Is that what you are describing when you say "we want to preserve the
overlay instead" ?
Do you have a reference SDT and the lopper command line you are using to
create the overlay ? (and an example of the expected overlay if lopper
isn't creating what you need). Then if you have the sample user overlay
and what you'd expect the combined overlay to be (for which I'd run that
first lopper command, pass the user overlay to lopper, and have a final
overlay generated that is both of them combined). I can do some runs to see
what is wrong.
Let me try to summarize the complete use case that we are trying to
support:
- Existing Usage:
SDT -> overlay assist ->
Outputs two files:
a) linux device tree (without PL bus)
b) an overlay file pl.dtbo (PL bus converted to an overlay)
Now, when someone wants to insert an overlay on top of the existing
pl.dtbo, we need to manually add an include file in the generated overlay
and then check its correctness with DTC. That's where we are trying to make
use of lopper.
- Proposed Usage:
SDT -> overlay assist (with user overlay as input) ->
Outputs two files:
a) linux device tree (without PL bus and without the user overlay)
b) an overlay file pl.dtbo (PL bus converted to an overlay + the user overlay)
Those match my understanding of what we are trying to do.
Based on the above discussions, I see we are trying to focus on below two
areas:
1. The overlay passed by the user must be syntactically correct and
that can only be checked when it is merged with the system device tree and
compiled. This is what lopper front end is doing and it is needed.
2. At the same time, we must preserve the info contained in overlay
and not merge it to the generated linux device tree. This must be added
only to the final PL overlay that we are carving out.
We see few issues in achieving step2 with the frontend:
1. If we first merge the overlay DT to full SDT, removing those
entries from the linux dt output is not easy. The suggestion is to rely on
the frontend checks for the invalid phandle, invalid names etc. This works
for phandles because those phandles are defined in the PL bus which is
removed via the assist. This seems to be a reactive approach as the list of
such properties can be an ever increasing one (after clocks, interrupt
related props can be the next).
That's expected and handled by the core when creating an overlay, the
complexity isn't anything than has been designed.
Lopper could track nodes and property on the way in, there's other uses of
this in the future, but that's a complexity we don't want to add right now.
How many such properties we can cater to in the frontend. Removing
clk-names based on the phandles if not done right can lead to erroneous
trees and those errors will be seen only during run time. I feel, the onus
for using the right clocks (and the overlay) should be on the user. We
should stick to only dt compatibility check between the overlay and the
base tree and not do any modification with the overlays passed.
The index -names are standardized and part of the practical specification,
so they aren't a significant thing to add. It would be data driven, so
something that can easily be adapted. I don't consider that a modification
of the overlay.
2. Removing those entries from linux device tree still seems fine, but
we need the overlay passed by the user in its actual form while creating
the pl.dtbo without any manipulation. And this also seems difficult with
the proposed suggestion.
Can you elaborate on why ? Do you want two overlays in the end ? The base
tree + our overlay + the user overlay, all separate ? If so, then yes, we'd
either create two separate overlays in the assist, or track the overlays as
secondary trees in lopper (which is what I was hinting at before).
We don't want two separate overlay files, the user overlay and our overlay
have to be in one file. As stated in the first example and the above, I am
afraid such manipulations can lead to erroneous trees even when the user
overlay was correct and that is not ideal.
Let me know if I could answer your queries. Looking forward to your
suggestions.
I think I have it.
If you have those sample files, I can check if there's a bug and send you
some tangible examples of what I'm trying (badly) to describe).
Bruce
… Regards,
Onkar
Given the complexity involved here, the current suggestion doesn't seem to
work. We only want to check the compatibility of this secondary tree coming
from overlay with the system device tree, but we don't want to merge this
secondary tree to the primary tree. That's where we thought your secondary
tree related prototype may come to our rescue.
It still may, assuming you confirm my understanding above about the two
separate overlays. If the result is a single, merged overlay (dtbo), then
either approach will work.
Another possible solution that I can think is to define a separate bus
(say overlay-bus) for all such use cases and use that bus in our assists
depending on our use cases. For overlays, that bus will be merged to the PL
bus. And since they will be passed to lopper frontend, the phandle and
other props syntax will also be in check.
Let me know your opinion on this summary and the introduction of this new
bus. Any suggestions are most welcome.
It's not a bad idea, but let's see about the secondary/support trees and
the other options first.
Bruce
—
Reply to this email directly, view it on GitHub
<#718?email_source=notifications&email_token=AARSRK2BAM36QFISONGKHHD4SM5BNA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJSGI3DENBVGAZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4122624503>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARSRK7YSB4XLGRSWHFEXVT4SM5BNAVCNFSM6AAAAACWRGVNACVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCMRSGYZDINJQGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
- Thou shalt not follow the NULL pointer, for chaos and madness await thee
at its end
- "Use the force Harry" - Gandalf, Star Trek II
|
|
I've implemented an alternative approach for the user overlay use case in the Commits (on master-next)Note: The first commit ( How It WorksWhen user overlays (via The new infrastructure:
ExampleUser overlay ( Here Command: ./lopper.py -i mmi_dc.dtsi -O lopper_output -f --enhanced \
system-top.dts system-top-no-pl.dts -- \
xlnx_overlay_pl_dt cortexa78_0 fullOutput pl.dtsi (excerpt showing auto-generated fragment): ValidationThe overlay compiles and applies cleanly: # Compile base and overlay
dtc -I dts -O dtb -@ system-top-no-pl.dts -o base.dtb
dtc -I dts -O dtb -@ pl.dtsi -o pl.dtbo
# Apply overlay
fdtoverlay -i base.dtb -o merged.dtb pl.dtboMerged DTB verification (mmi_dc has the complete clocks property): Optional: --nodes optionThe third commit also adds a # Extract only specific properties from mmi_dc
./lopper.py ... -- xlnx_overlay_pl_dt cortexa78_0 full --nodes="mmi_dc#clocks,clock-names"
# Combine whole nodes with property-specific extraction
./lopper.py ... -- xlnx_overlay_pl_dt cortexa78_0 full --nodes="/amba_pl;mmi_dc#clocks"This completely overrides the default Please review and let me know if you'd like to integrate this approach or if you have questions about the implementation. |
Created a new PR with latest changes.
Add
--user-dtsi=<path>option allowing users to append custom overlay content to the generated pl.dtsi output. The provided file undergoes lightweight validation: checks for presence of at least one reference node (&label { ... }) and rejects /dts-v1/ and /plugin/ headers.Also refactor validate_and_parse_options() to return a named tuple (OverlayOptions) instead of a bare tuple, and warn on unrecognized options.