Fix AxiLiteAsync replaying errored transactions after remote reset recovery - #1474
Fix AxiLiteAsync replaying errored transactions after remote reset recovery#1474ruck314 wants to merge 4 commits into
Conversation
…eset With COMMON_CLK_G = false, an access issued while the master domain was held in reset was answered with AXI_ERROR_RESP_G on the slave side and queued at the same time, so it executed on the master side once mAxiClk and mAxiClkRst recovered. A write could modify hardware after software had been told the access failed, and a register implemented as a command strobe could fire long after its caller gave up. Five defects in the GEN_ASYNC branch: - The request FIFO write enables were not gated by the remote reset, so a rejected request was queued anyway. - The request FIFOs were reset only by s2mRst, which carries sAxiClkRst, so a request queued before mAxiClkRst asserted also survived. The response FIFOs had the mirror-image problem and were not flushed by a slave-domain reset. - AW and W cross in separate FIFOs, so a write straddling the reset boundary left an address queued with no data behind it, and the next write's data was then committed to that abandoned address. - RVALID and BVALID were forced to a constant '1', so a response could be presented with no matching request and ahead of its request handshake. - Every reset comparison was written against '0' rather than RST_POLARITY_G, which inverted the fail-fast sense for active LOW resets. All five FIFOs now share one reset, asserted while either domain is in reset or while the bridge is still draining an abandoned transaction, so the bridge is empty whenever either side is reset. Each rejected access is answered from per-channel pending state, so exactly one response follows each accepted request and a write response waits for both AW and W. The bridge now also holds one transaction in flight per channel, matching AxiLiteCrossbar, whose per-slot state machine does not release a slot until rvalid and rready, and AxiLiteMaster, which runs one transaction at a time. The bound is enforced by the ready outputs rather than assumed, because the bridge otherwise accepts four or more outstanding and one pending flag per channel could answer only one of them. Sizing the tracking to the FIFO capacity instead would have needed three 7-bit counters, costing 45 LUTs and 22 flip-flops against the 4 flip-flops this uses. No port or generic changes.
…he bound The sweep previously ran COMMON_CLK_G = true only, so the asynchronous branch of the bridge had no coverage at all. It now runs five cases: common-clock plus asynchronous active-high, active-low, RST_ASYNC_G = true and PIPE_STAGES_G = 2. The bench drives mAxiClk from a gateable coroutine so a test can hold the master domain still while the slave domain keeps running, records an ordered log of the handshakes the downstream slave accepts, counts slave-side handshakes so every response can be matched to an accepted request, and bounds each slave-side access so a missing fail-fast response fails the test instead of hanging the regression. Some tests drive the slave port directly rather than through cocotbext-axi, because the channel ordering cases need AW and W presented independently. New tests: - remote_reset_ghost_test: an access rejected while the remote domain is reset is never replayed downstream after recovery. - remote_reset_write_order_test: a local write response waits for both AW and W, a read response waits for an accepted AR, and neither repeats. - remote_reset_inflight_flush_test: a request queued before the remote reset is discarded rather than replayed. - remote_reset_orphan_pairing_test: an abandoned write address does not pair with a later write data beat and misdirect it. - source_reset_stale_response_test: a response queued when the slave domain resets is discarded instead of being consumed by the next read. - source_reset_clears_outstanding_test: a slave-domain reset clears the pending state, so a later remote reset does not answer an abandoned transaction. This is what exercises the reset path of the registered logic, since a RST_ASYNC_G = true case on its own compiled the branch without checking it. - single_outstanding_bound_test: one transaction per channel is accepted, in normal operation and during a remote reset. Each was confirmed to fail when only its corresponding RTL fix is reverted. reset_behavior_test now waits out the cross-domain reset release before expecting a successful access, which the asynchronous cases require.
Capture the goal, the defects found, the design decisions and their rejected alternatives, the measured Vivado resource and timing comparison, the defect-to-test mapping, the validation run, and the remaining gaps.
There was a problem hiding this comment.
Findings
-
Blocker — unsafe FIFO-reset CDC topology. The previous
fifoRstwas formed with combinational logic from reset/state indications that did not all originate in one clock domain, then fed into the reset synchronizers inside all five asynchronous FIFOs. Synchronizing reset deassertion at each FIFO domain is necessary, but it does not make combinational or multi-clock fan-in ahead of those synchronizers safe. A transition or glitch can asynchronously assert the FIFO resets, and Vivado can report this structure as CDC-10/CDC-12. The polarity conversion also needed to be kept out of the registered reset fanout. -
Blocker — unaccepted AXI beats could still enter the FIFOs. The new READY logic limits the bridge to one outstanding AR, AW, and W beat, but the corresponding FIFO write enables did not use those READY signals. A master that held VALID while READY was low could therefore enqueue the same unaccepted beat on every clock. That could replay a rejected address or leave a ghost W beat to pair with a later AW.
I pushed commit 20116083f to address these findings.
Reset/CDC changes
- Removed the combinational OR of reset indications from different clock domains ahead of the FIFO reset synchronizers.
mAxiClkRstis first synchronized into thesAxiClkcontrol domain. It is then combined with the localerrModestate on the synchronous data input of a one-bitRegisterVector.sAxiClkRstpasses through a localRstSyncand asynchronously asserts that register, preserving reset assertion whensAxiClkis stopped while keeping release synchronous.- The registered output is natively active-high and directly drives all five
FifoAsync.rstinputs. Those FIFO instances use an active-high reset internally, so there is no polarity-select LUT between the reset-request flop and the per-domain reset synchronizers insideFifoAsync.
This gives the reset path one registered source before fanout. The FIFO-local RstSync instances still provide asynchronous assertion and synchronous deassertion independently in the read and write clock domains. The associated contract is that an unavailable AXI clock has its corresponding reset asserted; a source-domain reset abandons its outstanding transactions, while a master-domain reset with the source domain still running produces the local error response required by issue #1467.
Handshake changes
- AR, AW, and W FIFO write enables now require the actual source-side VALID/READY handshake.
- This prevents a master holding VALID while READY is low from inserting unaccepted duplicate or stale requests into a FIFO.
Test updates
single_outstanding_bound_testnow proves that blocked AR, AW, and W beats never appear at the downstream slave.- The five-case sweep covers common-clock operation plus asynchronous active-high, active-low, asynchronous-reset, and pipelined-FIFO configurations.
- Final validation:
5 passed, VSG reported zero violations, andgit diff --checkwas clean.
Vivado is not available in this environment, so report_cdc -details -all_checks_per_endpoint should still be rerun to confirm that the former FIFO-reset CDC-10/CDC-12 entries are gone.
Description
AxiLiteAsyncno longer replays transactions it already rejected.With
COMMON_CLK_G = false, an access issued while the master domain was held in reset was answered withAXI_ERROR_RESP_Gon the slave side but queued at the same time, so it executed on the master side oncemAxiClkandmAxiClkRstrecovered. A write could modify hardware after software had been told the access failed, and a register implemented as a command strobe could fire long after its caller gave up.Changes:
AWthat misdirected the next write's data. It likewise stops a stale response surviving a slave-domain reset.AWandW. PreviouslyRVALIDandBVALIDwere forced high with no matching request.RST_POLARITY_Ginstead of'0', fixing fail-fast behaviour for active-low resets.AxiLiteCrossbar.No port or generic changes.
Fixes #1467.
Details
Behaviour change. The one-transaction bound is enforced by the ready outputs rather than assumed. The old bridge accepted four or more outstanding transactions (measured), so a master that pipelines is now throttled to one. Correctness is unaffected. This matters because
AxiLiteAsyncIpIntegratorexposes the slave port to external masters. The bound is what allows the tracking to be one flag per channel: without it a flag-based responder answers only 1 of 4 abandoned reads and hangs the master, which is why the rejected alternative used counters at +45 LUTs.Cost. Vivado 2025.2, out-of-context synthesis and implementation,
xcku040,sAxiClk2.5 ns andmAxiClk2.0 ns: 309 CLB LUTs versus 312 at baseline, 477 registers versus 473. So 3 LUTs smaller than the unfixed baseline for 4 flip-flops. Both clocks meet timing and no handshake path gained a logic level.Tests. The sweep goes from 1 case to 5: common-clock plus asynchronous active-high, active-low,
RST_ASYNC_G = trueandPIPE_STAGES_G = 2, with 9 cocotb tests each. Every defect has a test confirmed to fail when only its own fix is reverted. Full library run: 1017 passed, 29 skipped.Caveats. The request-enqueue gating is redundant now that the shared reset covers the whole local-answer window, so it is kept for explicitness but is not independently covered by a test. Synthesis numbers are out-of-context on a single part.
docs/plans/axilite-async-remote-reset/has the full design notes and measurements; drop those commits if a plans entry is not wanted for a bugfix.