VRTD clock related safety improvements - #216
Open
quetric wants to merge 5 commits into
Open
Conversation
Three defects combined to let a design write leave the clock driver addressing a device that no longer exists, and then to report the resulting garbage as a successfully programmed frequency. device_refresh_pf2_after_design_write() closed and reopened the control device and every BAR file after the PF2 remove/rescan, but left the clock driver untouched. The driver holds a non-owning pointer to the control device and its own mapping of the BAR window carrying the clock wizard registers, both established against the pre-PDI PF2. A clock operation issued after a design write therefore drove MMIO through a stale mapping. Tear the driver down before the handle it borrows and recreate it once the fresh PF2 has been reopened, matching the order device_destroy() already uses. The register decode paths clamped a zero multiplier, a zero input divider and a zero output divider to 1. A register window that reads back as zero consequently produced f_out = f_VCO rather than an error, which is how a request for 200 MHz was reported as 4 GHz. Return zero from the decode helpers and reject it at every call site. clock_driver_try_set_rate_hz() logged a warning and returned success when the rate read back disagreed with the request. Compare the reading against the configuration that was just written instead: a disagreement there means the register window did not accept the programming, so the value does not describe a frequency the hardware is producing. Fail with EIO rather than returning it as an achieved rate. Fixes #207
clock_driver_wait_for_lock() read the lock bit from offset 0x33C, which is not the status register. 0x33C is the second word of the CLKOUT0 leaf register pair, holding the high-time count in both of its low bytes, so the predicate tested bit 0 of the output divider's high-time count rather than PLL lock. The status register is at offset 0x04 and reports lock in bit 0, as the upstream Linux clocking wizard driver does. The consequence was that candidate selection rejected any (M, D, O) tuple whose high-time count was even, after stalling for the full 200 ms lock timeout. For a 365 MHz request this discarded the best-ranked candidate and settled on one producing a different VCO frequency; for requests where every candidate has an even high-time count it failed outright. Requests that happened to work, such as 200 MHz, did so only because the first candidate had an odd count. Read the status register instead, both when waiting for lock and when dumping wizard state for diagnostics. The state dump also carried its own copy of the leaf offset computation that still had the off-by-three error corrected in #210; call the shared helper. Fixes #206
The clock driver wrote the M, D and O registers and then trusted whatever it read back. When the register window is not reachable the writes are discarded and the reads return either zeros or all ones, neither of which is distinguishable from a configuration by inspection alone. Confirm each of the six divider registers by reading it back immediately after the write: they are plain read/write storage, so a mismatch identifies a window that did not take the access. Report that as a device fault that aborts the request rather than as a lock timeout that causes the remaining candidates to be tried in turn. Treat a register reading as all ones as a failed access in the query path as well. None of the registers the driver reads has all 32 bits defined, so the pattern is a completion that did not reach the device. Log a failed refresh after a design write. The completion path reported "Design write completed successfully" whenever the transfer itself had succeeded, including when the subsequent PF2 remove, rescan and reopen had failed and left the device without usable handles. Remove the BAR2 fallback in clock_driver_init(). Its stated premise, that the compute platform places the clock wizards on BAR2, does not hold: both shells map clk_wizard_slash to BAR4 + 0x00000000 and clk_wizard_service to BAR4 + 0x00010000, as documented in docs/explanation/bar-address-map.rst. The fallback could therefore never select the right window, and on the service shell BAR2 + 0 is the eth_0 control interface, so it would have directed clock writes at an unrelated peripheral. There is one BAR to open, and failing to open it is a failure to construct the driver. Fixes #207
vrtd tracks the loaded shell entirely in software. device->current_shell is
set from the boot partition after a reset and from the SET_SHELL_STATE
request, and is never confirmed against the device. Any divergence between
that belief and the design actually loaded goes undetected, and the daemon
continues to drive register windows on the assumption that the wrong shell
provides them.
The static shell already publishes its own identity: the build-ID AXI GPIO
at BAR4 + 0x20000 carries the source commit hash and, in bit 28 of the high
word, the shell variant. Each shell's create_project.tcl forces that bit, so
it is valid even for bitstreams built without SLASH_BUILD_ID_HI set. smi
already reads the register to report the build ID; this adds the equivalent
reader to vrtd together with a comparison against the expected shell.
The comparison is applied at the two points where the expectation is formed
or must still hold:
- After a reset, before recording the shell derived from the boot
partition. The partition states the intended shell; a device reporting a
different one did not boot what was asked of it.
- After the PF2 remove/rescan that follows a design write. Partial
reconfiguration targets the user region and cannot change the shell, so
a change in the reported variant means the reopened BAR window is not
addressing the static shell vrtd assumes.
A high word of all ones, the value an MMIO read returns when it does not
reach the device, and any word with the reserved bits set are rejected as
unreadable rather than decoded. The check is skipped when no shell has been
claimed, since there is then nothing to contradict.
The clock wizard's reconfiguration register file resets to zero and only reflects what software has written into it. The frequency a design comes up with is fixed at synthesis and cannot be read back through it, so on a board where nothing has called set-rate the whole M/D/O range reads zero. Clamping a zero multiplier, divider or output divider to 1 turned that state into an arithmetic identity: f_out = f_in * 1 / 1 / 1. Every read of an unprogrammed wizard returned a confident 100 MHz that was really just the reference clock leaking through the clamps, and `clockwiz --get` had never once reported an actual measured rate. Reading a configuration now yields three outcomes rather than two. OK carries a rate. UNCONFIGURED means the registers are cleared and nothing has been programmed; it is an ordinary state, not a failure, and surfaces as a rate of 0 with a successful return. FAULT is reserved for a register window answering 0xFFFFFFFF, which is a window that is not answering at all, and still fails with EIO. v80-smi prints "unconfigured" for a rate of 0 rather than a bare number, since 0 Hz is not a realizable rate and printing one would be inventing it. Verified on a V80: both wizard windows read zero across 0x330-0x39C with their lock bits set, and both regions now report unconfigured where the previous build reported 100000000.
Coverage
Lines covered at 3404c2f, compared with the most recent successful |
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
1 participant
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.
No description provided.