Match nested model keys against a node's own compatible list (fixes #13) - #14
Open
TheArkadiuszGrzelka wants to merge 1 commit into
Open
Match nested model keys against a node's own compatible list (fixes #13)#14TheArkadiuszGrzelka wants to merge 1 commit into
TheArkadiuszGrzelka wants to merge 1 commit into
Conversation
renode_model_overlay()'s nested-key match notation (used for entries like "st,stm32-ethernet") only checked the SoC/board root compatible strings returned by get_overlays(). It never looked at the node's own "compatible" property. This silently breaks any device whose node compat list reuses another SoC family's identifier for a shared IP block. STM32H5's Ethernet MAC is the same Synopsys DWC EQOS IP as STM32H7's, and Zephyr's H5 binding reflects that by listing "st,stm32h7-ethernet" alongside the node's own "st,stm32h5-ethernet"/"st,stm32-ethernet" compat strings. Since the H5 SoC/root compat never contains "st,stm32h7", the nested key in models.json's "st,stm32-ethernet" entry never matched, and every non-H7 STM32 with this Ethernet IP silently fell through to the "_" default model (Network.SynopsysEthernetMAC) instead of the correct Network.SynopsysDWCEthernetQualityOfService. Fix by also matching nested keys against the generating node's own compatible list, passed down from every call site that already has it in scope. Verified against the full current zephyr_sim dashboard replkit (17436 generated dts/board/sample combinations): exactly 145 outputs across 5 STM32 boards (H5, N6 and MP1 Ethernet-capable families) change, all changing only the Ethernet MAC model line in the expected direction (SynopsysEthernetMAC -> SynopsysDWCEthernetQualityOfService with its MTL/DMA regions and clock); zero other differences anywhere in the corpus. Fixes antmicro#13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
renode_model_overlay()'s nested-key match notation (used e.g. by the"st,stm32-ethernet"entry inmodels.json) only checks the SoC/board root compatible strings returned byget_overlays(). It never looks at the generating node's owncompatibleproperty.This silently mis-resolves any device whose node compat list reuses another SoC family's identifier for a shared IP block. STM32H5's Ethernet MAC is the same Synopsys DWC EQOS IP as STM32H7's, and Zephyr's H5 binding reflects that by listing
"st,stm32h7-ethernet"alongside the node's own"st,stm32h5-ethernet"/"st,stm32-ethernet"compat strings. Since the H5 SoC/root compat never contains"st,stm32h7", the nested key inmodels.json's"st,stm32-ethernet"entry never matched, and every non-H7 STM32 with this Ethernet IP silently fell through to the"_"default model,Network.SynopsysEthernetMAC— the wrong model — with no PHY at all. The platform loads fine; the guest simply never gets a link.Reported as #13.
Fix
Match nested keys against the generating node's own compatible list too, in addition to the SoC/root overlays.
compatible/compatswas already in scope at everyrenode_model_overlay()call site, so this only threads it through as an extra parameter — no call site had to look anything up that it didn't already have.Blast radius
I enumerated every nested-key entry in
models.json(all entries whose value is a dict keyed by compat-substrings rather thantype/irq_mappings) and checked whether the node-own-compat lookup could flip resolution for anything besides Ethernet. The only way this fix changes behavior is if a node's owncompatiblelist contains a nested-key substring that the SoC/root overlay does not already contain — i.e. the "borrowed cross-family IP" pattern Ethernet exhibits. I did not find that pattern anywhere else that I could check.I then validated this empirically instead of just by inspection: I generated
.replfiles for every.dtsin the current productionzephyr_simdashboard replkit (17436 board/sample combinations) with and without this patch and diffed all pairs.nucleo_h563zi,stm32h573i_dk,stm32h5f5j_dk(H5),stm32n6570_dk/nucleo_n657x0(N6),stm32mp135f_dk(MP1) — all STM32 families that share this Ethernet IP.Network.SynopsysEthernetMAC→Network.SynopsysDWCEthernetQualityOfServiceplus its MTL/DMABusMultiRegistrationregions andsystemClockFrequency: 50000000(already the correct config declared forst,stm32h7inmodels.json— only the resolution was broken).Network.SynopsysEthernetMACfor all 5 of these boards, confirming this is a live, currently-shipping defect, not a hypothetical one.Testing
python3 -m py_compile dts2repl/dts2repl.py.ci.yml"compare repls" job manually against the realzephyr_simdashboard replkit (see above) — this is the part of that job that does not require building/downloading Renode.renode-test .../load_repls_with_diffs.robotre-verification step that job runs when diffs are found (needs a full Renode + dotnet-sdk-8.0 build, out of scope for what I could stand up here). Flagging this explicitly rather than implying coverage I don't have.96b_aerocore2-hello_worldsample from the CI's own smoke test path: output is byte-identical before/after this patch (the one diff line versus the dashboard reference there is pre-existingsystickFrequencydrift, unrelated to this change).Related: I also looked at #12 (
st,stm32-fdcanhas nomodels.jsonentry at all) but concluded it needs actual code (to synthesize a sizedmessageRamchild object frombosch,mram-cfg), which a declarativemodels.jsonentry cannot express, so I'm not including it here.