Skip to content

test: Add an inst64 frontend testbench driven over the accelerator bus - #185

Merged
DanielKellerM merged 5 commits into
pulp-platform:develfrom
DanielKellerM:test/inst64-tb
Aug 13, 2026
Merged

test: Add an inst64 frontend testbench driven over the accelerator bus#185
DanielKellerM merged 5 commits into
pulp-platform:develfrom
DanielKellerM:test/inst64-tb

Conversation

@DanielKellerM

Copy link
Copy Markdown
Collaborator

The tightly-coupled inst64 frontend had no public testbench. Nothing exercised the DMA ISA decode, the transfer id and status path, or the datapath below it; only syntax analysis reached those sources.

Ports the existing harness (idma_inst64_tb_pkg, idma_inst64_drv_if, idma_inst64_base) and adds a directed AXI-to-AXI copy test driving dmsrc/dmdst/dmcpyi and polling dmstati.

Verified

  • Questa 2023.4: compiles 0 errors, reaches $finish, TEST PASSED: 4096 B copied, ar=1 aw=1 beats.
  • Non-vacuity: corrupting one destination byte after the wait gives payload mismatch at offset 777: expected 0xa9, got 0xba and TEST FAILED. The TB also sentinels the destination and adds guard bands, so an unwritten byte or an overrun fails too.
  • make idma_sim_tb_idma_inst64_axi_copy gates on both a negative Error:|Fatal: grep and a positive TEST PASSED grep, because Questa does not propagate $fatal to the exit code and a sim that never starts would pass a negative-only gate.

Note on the port

The upstream harness builds on DMOPC, which is not part of the public ISA. It is not merely absent: DMOPC decodes bit-identically to the public DMINIT, so carrying it over would have launched a real zero-fill transfer, burning a transfer id and bumping completed_id, desynchronising the completion wait so it could return before the copy retired. The opcode and its callers are therefore dropped rather than stubbed. Head arguments are dropped for the same reason: the public decoder ignores data_argb[12:5], so passing them would be a silent no-op.

The tightly-coupled frontend had no public testbench, so nothing exercised
the DMA ISA decode, the id/status path or the datapath below it.

Port the existing harness and driver, dropping the vidma-only ISA: DMOPC
decodes bit-identically to the public DMINIT, so carrying it over would have
launched a real zero-fill transfer and desynchronised the completion wait.
Copilot AI lite review requested due to automatic review settings August 13, 2026 05:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The event outputs feed downstream performance counters but nothing verified
them; the testbench observed the same handshakes independently and ignored
them.

Compare all 26 observable fields against the pins every cycle and anchor the
totals, keeping the independent sniff as the reference so a wrong counter
cannot agree with itself.

@DanielKellerM DanielKellerM left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

Comment thread test/frontend/idma_inst64_drv_if.sv Outdated
Comment on lines +9 to +11
/// only (DMSRC/DMDST/DMSTR/DMREP/DMCPY/DMCPYI/DMSTAT). The vidma-only DMOPC/ALU/MX
/// opcodes are deliberately absent: DMOPC's encoding aliases public DMINIT and would
/// silently launch a real zero-fill transfer.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove references to vidma The vidma-only DMOPC/ALU/MX
/// opcodes are deliberately absent: DMOPC's encoding aliases public DMINIT and would
/// silently launch a real zero-fill transfer.

Comment thread test/frontend/idma_inst64_tb_pkg.sv Outdated
Comment on lines +11 to +12
/// Concrete type binding for the `idma_inst64_top` testbenches. The types mirror
/// `test/idma_inst64_lint.sv`, the elaboration-proven binding of the frontend.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this comments as they might be obsolete

Comment on lines +45 to +62
// `events_o` is exported for downstream performance counters but nothing verified it.
// Every field is a zero-latency recode of the DUT's own top-level pins, so the TB
// rebuilds the expected value from those pins and compares it per cycle. This checks
// the recode (mapping, polarity, gating, width), not the bus traffic; the geometry
// anchors below are what keep it from passing on a run where nothing happened. The
// hand-counted AR/AW sniff stays the independent source of truth and is untouched.
//
// Deliberately not checked here: obi_wr_req/obi_rd_req (a_no_obi_traffic pins both
// sides to 0, so an equality proves nothing; needs a TCDM-window TB with an OBI
// read/write mix) and the per-channel index in gen_events (NumChannels == 1).
//
// Polarity note: aw_stall/ar_stall are valid && !ready, but idma_inst64_events
// overrides its own w_stall/r_stall at lines 95/96, so w_stall is a W-side bubble
// (w_ready && !w_valid) and r_stall is !r_ready && r_valid; the earlier assignments
// are dead code. Measured gap: none of the four stall fields ever asserts here, since
// the sim memories neither backpressure nor bubble. Their equality still catches a
// polarity flip or a spurious assert, but a stuck-at-0 stall would pass; that needs a
// ready-throttling harness, not a weaker check.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verbose explanation, reduce to 3 o4 4 lines

Comment on lines +191 to +194
// Sentinel over the destination and its guard bands: an unwritten byte must fail
// the compare rather than accidentally match, and an overrun must be visible.
// Offsets stay non-negative; a signed offset added to a 64-bit unsigned address is
// zero-extended, not sign-extended.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verbose

Comment thread idma.mk Outdated
Comment on lines +427 to +429
# Tightly-coupled inst64 frontend driven over the snitch accelerator bus.
# Run with the Questa SEPP wrapper:
# make idma_sim_tb_idma_inst64_axi_copy VSIM="questa-2023.4 vsim"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comments

@DanielKellerM

Copy link
Copy Markdown
Collaborator Author

Addressed in a1612d2.

  • idma_inst64_drv_if.sv: dropped the vidma/DMOPC references; the docstring now just says what the driver covers.
  • idma_inst64_tb_pkg.sv: removed the header comment. It pointed at test/idma_inst64_lint.sv, which ci: Verify the backend variants publicly with verilator and slang #186 deletes, so it was already going stale.
  • tb_idma_inst64_axi_copy.sv:62: 18 comment lines down to 5.
  • tb_idma_inst64_axi_copy.sv:194: 4 lines down to 2.
  • idma.mk: removed the target header block; no other sim target in the file carries one.

Re-verified after the edits: make idma_sim_tb_idma_inst64_axi_copy still passes, with the cross-check reporting the same 26 fields and totals.

One thing the edits surfaced, separate from this PR: lint-sv runs with paths: src, and the tree-scoped verible job in #186 also only walks src/, so nothing lints test/. Running verible over this testbench by hand reports always-ff-non-blocking on the counter updates, including ones that predate the cross-check. I have not touched them here since the blocking assignments are deliberate for the same-tick sampling, and changing them would need re-verification. Happy to extend the tree-scoped job to test/ and clean these up in a follow-up if you want that coverage.

The goldens mirrored the RTL rather than the counter spec, so both sides read
zero for the whole transfer and the four stall comparisons could not fail.
@DanielKellerM

Copy link
Copy Markdown
Collaborator Author

Note on merge order: this now expects the documented stall polarity in the event cross-check, so #187 has to land first. On today's devel RTL the goldens for r_stall/w_stall would mismatch.

Background: an audit of #187 against snitch_cluster_peripheral_reg.rdl showed the RTL had the two stall fields overwritten by the buffer metrics, so both the RTL and this testbench's goldens were encoding the wrong polarity. Since the goldens mirrored the RTL, all four stall comparisons were 0 === 0 and could not fail. With #187 in place the corrected r_stall asserts 44 of 108 cycles and the comparison discriminates - proven by flipping the golden and watching it fail.

…-check"

The matching RTL fix is not on devel yet; this expectation moves to the pull
request that changes the polarity, so the two land together.
@DanielKellerM
DanielKellerM merged commit f387093 into pulp-platform:devel Aug 13, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants