idma_channel_coupler: don't consume coupled-AW credit when popping a …#98
Open
flaviens wants to merge 1 commit into
Open
idma_channel_coupler: don't consume coupled-AW credit when popping a …#98flaviens wants to merge 1 commit into
flaviens wants to merge 1 commit into
Conversation
…decoupled head
The credit counter `aw_to_send_q` in `proc_credit_cnt` tracks coupled
AWs whose `first` R beat has arrived but that have not yet been
handshaken on the AW master channel. The default
`aw_sent = aw_decoupled_head & aw_valid` correctly pops a decoupled
AW from the head without touching the counter, but the surrounding
if/else-if chain does not check whether the head is decoupled. This
leads to two related bugs:
1. The third else-if (`aw_ready_decoupled & !first & aw_to_send_q != 0`)
decrements the counter and asserts `aw_sent` whenever the downstream
is ready and a credit is pending — even when the AW currently being
popped is a decoupled head. The credit (which was earned by a
*coupled* AW further back in the FIFO) is consumed by the decoupled
pop, so when the coupled AW reaches the head later it has neither a
credit nor an arriving `first` and the AW master channel deadlocks.
2. The first if-branch (`aw_ready_decoupled & first`) consumes a
`first` even when the head is decoupled. The `first` belonged to a
future coupled AW; it is silently lost.
Reproducer (seclang `examples/idma/i003_verilator/`):
1. Push AW0 (decoupled). FIFO=[AW0(dec)] ready=0
2. first arrives for upcoming AW1. ready=0, credit++ -> 1
3. Push AW1 (coupled). FIFO=[AW0(dec), AW1(coup)] ready=0
4. Release ready=1.
Pre-fix: cycle 0 handshakes AW0 and decrements credit to 0.
cycle 1+ AW1 head, no credit, no `first` -> deadlock.
Post-fix: cycle 0 handshakes AW0 (default path), credit stays at 1.
cycle 1 handshakes AW1, credit drains to 0.
Fix: gate the first-consuming branch and the credit-decrement branch
on `~aw_decoupled_head`, and rephrase the increment branch to fire
on any unhandled `first`. Decoupled AWs are popped only by the
default `aw_sent = aw_decoupled_head & aw_valid` path and do not
interact with the credit counter.
A more comprehensive fix exists upstream on branch
`decouple_rw_fix` (commit a7bbc83, "Fix channel_coupler to handle
decoupled", 2025-08-21) which introduces a separate `aw_to_stall_q`
counter; that branch also touches other parts of the legalizer and
has not been merged. This patch is the minimal change that resolves
the deadlock against `master` without the larger refactor.
flaviens
force-pushed
the
fix/i003-channel-coupler-decoupled
branch
from
72919ba to
cdb3afd
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a deadlock in the AW master channel by ensuring the coupled-AW credit counter is never consumed or decremented when the FIFO head entry is a decoupled AW. This keeps decoupled AW popping on the default path and preserves credits for coupled AWs further back in the queue.
Changes:
- Clarifies the meaning of
aw_to_send_qas a credit counter for coupled AWs only, and documents why decoupled head AWs must not consume credits. - Gates the “send-on-first” and “drain-credit” paths with
~aw_decoupled_headso only coupled head AWs can affect the credit counter.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Hi there!
proc_credit_cnt could incorrectly consume a coupled-AW credit while popping a decoupled AW at the FIFO head. That left the later coupled AW with no saved credit and no arriving first beat, causing the AW master channel to deadlock.
This PR gates the first-consuming and credit-decrement paths on ~aw_decoupled_head, so decoupled AWs are handled only by the default pop path and never modify the coupled-AW credit counter. This is a minimal fix for master; the larger upstream decouple_rw_fix refactor remains separate.
Thanks!
Flavien