Repository navigation
frontend: Drive the inst64 buffer stall events - #230
Merged
Merged
Conversation
#187 fixed r_stall and w_stall, which were each assigned twice with opposite polarity, but it deleted the buffer expressions instead of moving them to the fields they describe. Nothing has driven buf_w_stall and buf_r_stall since, so the Snitch cluster performance counters 0xe and 0xf read zero. The expressions are the ones the cluster documents for those counters: w_ready high with w_valid low, and r_valid high with r_ready low. Integrators whose dma_events_t lacks the two members will not elaborate, so the inst64 testbench package gains them here.
DanielKellerM
requested review from
micprog and
thommythomaso
as code owners
September 16, 2026 10:02
There was a problem hiding this comment.
🟡 Changes recommended
Regression coverage does not verify either new buffer-stall field.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds AXI buffer-stall event generation for inst64 performance counters and updates the testbench event type.
Changes:
- Drives
buf_r_stallandbuf_w_stallfrom AXI handshake conditions. - Adds both fields to the testbench
dma_events_t.
File summaries
| File | Summary | Review note |
|---|---|---|
test/frontend/idma_inst64_tb_pkg.sv |
Extends the testbench event structure. | Add pin-level checks for both buffer-stall fields. |
src/frontend/inst64/idma_inst64_events.sv |
Drives buffer-stall event signals. | Add per-cycle checks with stimulus covering relevant ready/valid combinations. |
Review details
Suppressed comments (1)
src/frontend/inst64/idma_inst64_events.sv:75
- The new buffer-stall outputs are not covered by
tb_idma_inst64_axi_copy: itsev_field_e/ev_field_oktable stops atr_stall/w_stall, so either new expression could be removed or inverted and the advertised regression would still pass. Add per-cycle checks for bothbuf_r_stallandbuf_w_stall, with stimulus that reaches their relevant ready/valid combinations.
events_o.buf_r_stall = !axi_req_i.r_ready && axi_rsp_i.r_valid;
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+106
to
+107
| logic r_valid, r_ready, r_done, r_bw, r_stall, buf_r_stall; | ||
| logic w_valid, w_ready, w_done, w_stall, buf_w_stall; |
DanielKellerM
added a commit
that referenced
this pull request
Sep 16, 2026
The inst64 frontend drives every member of dma_events_t from idma_inst64_events, but each integrator declares the struct itself, so a member iDMA stopped driving or never drove goes unnoticed. Publish the driven set, including the two buffer stall fields #230 restores.
tb_idma_inst64_axi_copy left buf_r_stall and buf_w_stall out of the per-field pin comparison, so either assignment could be stuck at zero and the sim still passed. The sim memory answers a read and accepts a write every cycle, so the DMA buffer never fills or drains and neither event ever asserts. The harness gains an optional antiphase R/W stall between the DUT and the memory, following the shim in tb_idma_mxrand: the read buffer alternately backs up (the DUT drops r_ready under r_valid) and runs dry (the memory holds w_ready with no w_valid). A go bit only falls after its channel handshake, so no valid is withdrawn. Both events now assert, are compared against the pins every cycle, and a stuck-at-zero count fails the test.
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.
buf_w_stallandbuf_r_stallhave been undriven since #187, so the Snitch cluster performance countersdma_buf_w_stall(0xe) anddma_buf_r_stall(0xf) read zero in every cluster.#187 fixed a real bug:
r_stallandw_stallwere each assigned twice in the samealways_combwith opposite polarity, and the second pair won. That second pair was computing the buffer metrics, so removing it restored the documented stall polarity but left the buffer fields with no driver.The expressions here are the ones
snitch_cluster_peripheral_reg.rdldocuments for those counters:dma_events_tis aparameter type, so writing a member an integrator's struct does not have is an elaboration error.idma_inst64_tb_pkg.svgains the two members here; snitch_cluster already declares them.Verified:
idma_lint_sv,idma_lint_inst64andtb_idma_inst64_axi_copy(Questa, 0 errors) pass.Relates to snitch_cluster#330, where the ownership of
dma_events_tis being discussed.