frontend: Fix reg next_id launch deadlock#153
Open
DanielKellerM wants to merge 2 commits into
Open
Conversation
DanielKellerM
commented
Jul 8, 2026
Comment on lines
+102
to
+107
| // launch-stall pending state (see gen_nxt_read_pending in gen_core_regs). The PeakRDL | ||
| // reg_top masks the CPUIF read req the cycle after an ungranted external read | ||
| // (external_pending), so next_id[c].req is a single-cycle pulse. nxt_read_pending holds | ||
| // the launch across the stall so arb_valid/rd_ack/stream_idx_o stay asserted until the | ||
| // arbiter grants (matching the canonical reggen .re strobe); else the launch is dropped | ||
| // and the reg block deadlocks. |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments
Comment on lines
+108
to
+109
| logic [NumRegs-1:0][NumStreams-1:0] nxt_read_seen; | ||
| logic [NumRegs-1:0][NumStreams-1:0] nxt_read_pending_q; |
Collaborator
Author
There was a problem hiding this comment.
explain why we need extra signals.
Comment on lines
+187
to
+190
| // launch-stall: hold the arbiter request and the reg read-ack across the CPUIF read stall | ||
| // until the arbiter accepts the launch. nxt_read_seen is the single-cycle req pulse; | ||
| // nxt_read_pending_q holds it until arb_ready. rd_ack is driven from these below (see | ||
| // gen_hw2reg_connections), not from the maskable req — matching the canonical held handshake. |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments
Comment on lines
+8
to
+19
| // Self-checking testbench for the iDMA register frontend `idma_reg32_3d` | ||
| // (config-bus CPUIF = apb4-flat, the default). It drives the APB config slave | ||
| // directly, owns the transfer-id counter via `idma_transfer_id_gen`, and models | ||
| // a controllable backend stub on `req_ready_i` so the launch handshake can be | ||
| // stalled at will. | ||
| // | ||
| // The core regression is the next_id launch deadlock: PeakRDL's external-register | ||
| // handshake masks the CPUIF read req the cycle after an ungranted next_id read | ||
| // (external_pending), so without the pending latch the launch rd_ack never fires, | ||
| // external_pending never clears, and the whole reg block freezes. A cycle watchdog | ||
| // `$fatal`s if any next_id (APB) read stalls beyond a generous bound, so an unfixed | ||
| // template trips the watchdog while the fixed one completes cleanly. |
Collaborator
Author
There was a problem hiding this comment.
Verbose comments. This should be a clear header
Comment on lines
+389
to
+396
| # Self-checking register-frontend regression. Drives idma_reg32_3d's apb4-flat | ||
| # config slave, owns the id counter (idma_transfer_id_gen), and models a | ||
| # controllable backend on req_ready_i. Test 2 is the next_id launch-deadlock gate: | ||
| # a per-read watchdog $fatals if a next_id read freezes under backpressure. Runs | ||
| # once with NumStreams=1 and once with NumStreams=2 (multi-stream stream_idx hold). | ||
| # Requires the apb4-flat reg top (the default IDMA_REG_CPUIF) — compile.tcl regens | ||
| # it. Run with the Questa SEPP wrapper: | ||
| # make idma_sim_tb_idma_reg_frontend VSIM="questa-2023.4 vsim" |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments
The PeakRDL external-register handshake masks the CPUIF read req the cycle after an ungranted next_id read (external_pending), collapsing next_id.req to a single-cycle pulse. The launch rd_ack (req & ~req_is_wr & arb_ready) then can never fire once req is masked, external_pending never clears, and the whole reg block freezes. Hold the launch across the stall in nxt_read_pending_q: set on the seen read, clear on arb_ready, and drive arb_valid, rd_ack and stream_idx_o from the latch instead of the maskable req. This replicates the canonical reggen held-handshake (the stable .re strobe) exactly: the read stalls until the arbiter grants, completes the cycle arb_ready is high with no added latency, and returns this launch's id. status/done_id are untouched (their ungated rd_ack never latches external_pending).
The register frontend had no directed testbench. tb_idma_reg_frontend drives idma_reg32_3d's apb4-flat config slave, owns the transfer-id counter via idma_transfer_id_gen, and models a controllable backend on req_ready_i so the next_id launch handshake can be stalled at will. A per-read cycle watchdog fatals if a next_id APB read fails to complete under backpressure, so the test detects the launch deadlock: it passes against the pending-latch fix and fatals (DEADLOCK) against the pre-fix template. Covers a basic launch, the backpressure/deadlock regression, multi-stream stream_idx hold under stall (NumStreams=2), and back-to-back monotonic ids. Wired into the idma_test Bender target and a new idma_sim_tb_idma_reg_frontend make target that runs both NumStreams=1 and NumStreams=2 in Questa.
Collaborator
Author
|
Addressed the review — dropped the verbose comment blocks in |
DanielKellerM
force-pushed
the
frontend/reg-deadlock-fix
branch
from
d87610c to
13ba549
Compare
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.
What
Fixes a launch deadlock in the register frontend's
next_idread — introduced by the SystemRDL/PeakRDL migration (#73) and inherited by devel/#148 — and adds the first directed reg-frontend testbench.Root cause
next_idis a PeakRDLexternalregister: reading it launches a transfer and the read stalls (blocking) until the backend arbiter grants (arb_ready). When the launch isn't granted in its single active cycle, the reg_top latchesexternal_pendingand masks its own CPUIF req — sonext_id.reqbecomes a one-cycle pulse. The #134 gaterd_ack = req & ~req_is_wr & arb_readystill references that maskablereq, so oncereqdrops,rd_ackcan never fire,external_pendingnever clears, and the whole reg block freezes.Fix
Reconstruct the canonical (pre-#73 reggen) held read-strobe with a per-
[reg][stream]pending latch, entirely in the CPUIF-agnostic launch layer:nxt_read_seen= the single-cycle req pulse;nxt_read_pending_qsets onseen & ~arb_ready, clears onpending & arb_readyarb_valid = |(seen | pending)— holds the arbiter request across the stall (satisfiesrr_arb_treeLockIn)rd_ack = (seen | pending) & arb_ready— driven by the latch, not the maskable reqnext_id/status/done_idstayexternal(no RDL change, no CPUIF change), which preserves the canonical blocking read-stall exactly: the fast path accepts in cycle 0 (zero added latency), the slow path holds until the grant. Because the read blocks, SW physically cannot issue a secondnext_idread while one is pending, so the 1-deep latch is sufficient and no launch is ever dropped. Template-only, +30/-7.Also fixes a latent multi-stream bug:
stream_idx_owas keyed on the maskable req and reverted to 0 mid-stall; it is now keyed on(seen | pending).Test
New self-checking
test/tb_idma_reg_frontend.sv(APB DUT + controllable backend stub +idma_transfer_id_gen+ a deadlock watchdog) with anidma_sim_tb_idma_reg_frontendmake target — the first directed reg-frontend test in the repo. Cases: basic launch, backpressure/deadlock regression, multi-streamstream_idxhold, back-to-back monotonic ids.DEADLOCK: next_id read did not complete within 2000 cycles) at exactly theexternal_pendingfreeze — proving the test catches the bug.Notes
rd_swaccrework, which changed the blocking contract to poll-based and was untested).axi_sim_memdata-path test is a good follow-up.