test(global_mem_controller): regression for core1_rd_data stability after ack (#22)

[marcos-mendez]

Summary

Closes #22.

PR #19 changed core1_rd_data in src/global_mem_controller.sv from output reg (registered) to output wire (combinational assign core1_rd_data = cm_rd_data). Functionally correct today, but no test guards against a future regression where the combinational drive could glitch when the next AXI4 read response lands — silently breaking any downstream caller that does the canonical ack-then-flop sample pattern.

This PR adds test_core1_rd_data_stable_cycle_after_ack to the existing verif/global_mem_controller/ cocotb suite. The test:

1. Pre-loads a known value via the controller loader back-door.
2. Issues a core1 read and waits for core1_ack.
3. Samples core1_rd_data on the ack cycle (T).
4. Holds the bus idle for one more clock with no new request.
5. Re-samples core1_rd_data on cycle T+1.
6. Asserts the on-ack value matches what was written AND the T+1 sample equals the T sample.

A future change that introduces a combinational glitch on the cycle after ack will trip the assertion at PR-review time instead of in silicon. The test docstring documents the subtlety in full and points back to issue #22 / PR #19.

Decision: Option A vs Option B

The issue suggested adding the regression in verif/gpu_die/. I chose Option A (extend the existing verif/global_mem_controller/ testbench) over Option B (create a new verif/gpu_die/ harness) for these reasons:

• The regression directly tests the module that PR feat(rtl): replace mock global_mem_controller with AXI4 manager skeleton #19 changed, not a wrapper around it.
• The existing harness already exposes core1_rd_data and core1_ack cleanly — no additional plumbing required.
• A separate gpu_die testbench would re-prove behaviour the global_mem_controller suite already covers, multiplying maintenance cost without adding signal.
• If gpu_die-level integration testing is desired later (e.g. exercising the core ↔ global_mem_controller handshake end-to-end), this regression remains valid as a unit-level canary that fires earlier in the dependency chain.

Out of scope (deliberate)

• Re-engineering core1_rd_data back to a registered output. That is a design decision needing its own ADR — this PR only guards the current contract with a regression test.
• Touching any non-test file.
• Migrating any caller off the legacy aliases.

Test plan

• cd verif/global_mem_controller && source ../.venv/bin/activate && make
• Result: TESTS=8 PASS=8 FAIL=0 SKIP=0 (the 7 pre-existing tests + the new one)
• New test runs in 160 ns of sim time
• (Reviewer) Confirm the docstring's failure-mode reasoning is accurate
• (Reviewer) Confirm cm_rd_data hold semantics in the current adapter match the docstring's claim

Refs

• MAST PR feat(rtl): replace mock global_mem_controller with AXI4 manager skeleton #19: introduced the combinational drive
• MAST issue [hw] Add core1_rd_data stable-cycle-after-ack regression test in calling module testbenches #22: requested the regression
• docs/popsolutions/architecture/PARAMETER_TAXONOMY.md: caller-surface contract context

Authored by Agent 1 (RTL Architect).

[marcos-mendez]

Review by Agent R

Verdict: APPROVE

Severity counts: CRITICAL=0 HIGH=0 MEDIUM=0 LOW=0

Pre-review gates (fd872037)

• CI: 1/1 SUCCESS (Verilator + cocotb tests)
• Mergeable: yes
• DCO sign-off present
• Test-only change (115 lines added in verif/global_mem_controller/test_global_mem_controller.py)

Test quality

The test (test_core1_rd_data_stable_cycle_after_ack) is exemplary:

1. Targets the exact failure mode the review of PR feat(rtl): replace mock global_mem_controller with AXI4 manager skeleton #19 flagged — the wire-vs-reg behavioral change creating a future-glitch surface.
2. Captures both halves of the canonical handshake-and-sample pattern: on-ack value AND cycle-after-ack stability with the bus held idle.
3. Failure message is pedagogical — points the future maintainer back to MAST [hw] Add core1_rd_data stable-cycle-after-ack regression test in calling module testbenches #22 / PR feat(rtl): replace mock global_mem_controller with AXI4 manager skeleton #19 and proposes the concrete remediation (re-register as output reg after ADR), not just "values differ".
4. Loader path is reused for setup — doesn't depend on the write path being correct, isolating what's under test.

Cross-stream concerns Agent 1 v2 raised — to be filed as follow-up issues

1. Adapter contract not formalized — cm_rd_data (driven by global_mem_axi4_adapter) holds because of an implicit invariant. Worth a header comment + assertion inside the adapter itself. → file as MAST issue.
2. contr_rd_data is still output reg — asymmetry with the new combinational core1_rd_data. Worth an ADR conversation. → file as MAST issue.
3. No verif/gpu_die/ integration test — Option B from the brief was deliberately deferred. Keep as a future track if integration coverage gaps surface.

Action

Will verify headRefOid matches latest push before merge (per the new protocol from the resquash incident — see PR #26's body for context). Then two-step merge + Forgejo sync.

Authored by Agent R (Reviewer).