Add Versal SysMon support - #815
Conversation
|
Thanks for the patch. The I ran the suites locally against this branch and they are clean: 1099 passed / 4 One question on the Every sysmon node in Second thing, more about future-proofing than a defect. The vck190 sysmon nodes have
A unit test in |
|
@dileepkumarnagavarapu : can you please address the bruce review comments |
…t compatible entry Update zephyr_supported_comp.yaml to include xlnx,versal-sysmon compatibility so that SysMon nodes are not filtered out during domain DTS generation. Include the required properties needed for Zephyr Devicetree generation, such as compatible, reg, status, interrupts, and xlnx,numchannels. This enables Zephyr sensor driver integration for Versal SysMon using Lopper-generated DTS. Signed-off-by: Dileep Kumar Nagavarapu <DileepKumar.Nagavarapu@amd.com>
bb1e1f8 to
2716ab6
Compare
zeddii
left a comment
There was a problem hiding this comment.
Thanks for the updates: three of the four things I raised are fixed.
I checked the things the new code depends on and they're all in place: re, LopperProp and delete_unused_props are imported, and node.delete(child) does remove a child node (I tested that one rather than assume it), since LopperNode.delete is documented for properties.
Three small things inline, all in the new code.
On the test: I still think this should have one, and unlike some other assists the scaffolding already exists here. tests/test_xlnx_gen_domain.py has the pattern: build a tree, wrap it in SimpleNamespace(tree=tree, outdir=...), call the generator. pull request #824 add two tests in that file this week doing exactly that. A sysmon node with two supply children and one xlnx,aie-temp child would cover the count, the AIE drop, the rename and the collision path in one go. That is the whole of this change, and none of it is exercised by anything in the tree today.
| child.name = rail_name | ||
| child["xlnx,name"] = rail_name | ||
| else: | ||
| child.name = f"supply@{reg_val}" |
There was a problem hiding this comment.
The unnamed fallback gives the node a unit address while reg has just been removed on line 781 and replaced with xlnx,register. So the output carries supply@10 { xlnx,register = <0x10>; ... } — a unit address with no reg property, which is the case dtc flags as unit_address_vs_reg ("node has a unit name, but no reg or ranges property").
The named branch above gets this right by accident of naming: VCC_SOC has no @, and no reg, so it's consistent. It's only the fallback that ends up mismatched.
f"supply_{reg_val}" for both the node name and xlnx,name would keep the two branches consistent with each other and quiet the warning ... you already use that exact form for the property on the next line.
| else: | ||
| child.name = f"supply@{reg_val}" | ||
| child["xlnx,name"] = f"supply_{reg_val}" | ||
| used_names.add(child.name) |
There was a problem hiding this comment.
used_names.add() sits inside the if reg_val ... block, so a supply child that has no reg (or whose reg failed the int() conversion and hit the continue on line 780) never registers its name.
That child also keeps its original SDT name, since the rename is in the same block. If that original name happens to match a rail name generated later in the loop, the collision check on line 788 won't see it and you get two siblings with the same node name.
Narrow, and it needs a malformed SDT to reach. Registering every child's final name, rather than only the renamed ones, would close it.
| child.name = f"supply@{reg_val}" | ||
| child["xlnx,name"] = f"supply_{reg_val}" | ||
| used_names.add(child.name) | ||
| delete_unused_props(child, ["xlnx,register", "xlnx,name"], False) |
There was a problem hiding this comment.
This runs for every non-AIE child, including ones that took the continue on line 780 or never entered the if reg_val block at all. Those children never had xlnx,register created, so the keep-list strips them down to xlnx,name alone.
A supply child with nothing but a name is unlikely to be useful to the Zephyr driver, and it's silent ... the node stays in the output looking plausible. Either the reg-less case is worth skipping outright, or it's worth a warning, since reaching it means the SDT wasn't what this code expects.
Add support to enables extraction and inclusion of SysMon nodes from the SDT into generated domain DTS, ensuring proper representation of supply monitoring hardware for Versal platforms. Signed-off-by: Dileep Kumar Nagavarapu <DileepKumar.Nagavarapu@amd.com>
2716ab6 to
ba3fb18
Compare
This patch series adds support for AMD/Xilinx Versal SysMon in Lopper.
The first patch updates the Zephyr supported compatibility list to
include the "xlnx,versal-sysmon" compatible entry, allowing proper
recognition of the SysMon node during device tree processing.
The second patch extends the gen_domain_dts assist script to handle
Versal SysMon nodes and generate appropriate domain-specific DTS
entries.
Tested on the vck190 platform with Lopper-generated dts