Fix the +1 descriptor offset: drop the arbiter's response register
SPRAMArbiter latched the SPRAM word into rsp_buf and raised exi_rsp_valid a cycle later. BBARegisterFile already took the response the cycle it arrived, so the buffer bought nothing and cost a 24 MHz cycle (~42 ns) inside a deadline that was short by about that much. Both are now combinational in the cycle SPRAM's output is valid. The data sampling is unchanged -- the old code read spram_dout in that same cycle, it just registered it before handing it over -- so only the handoff moved. bba_top T9 goes from 1/6 to 6/6 byte-exact at EXI_HALF=1, and stays 6/6 at 2 and 8. The whole suite passes at the hardware rate. Nothing else changed: BBARegisterFile is untouched. Found by measuring instead of guessing, after four falsified attempts. A counter in ExiCapture recording tx_load & ~tx_fifo.r_rdy, with a shift-register history of which byte boundary underran, showed good and bad reads BOTH underrun at pop 3 (harmless, mid-header) while a bad read adds pop 4 -- the pop that loads data slot 0. So the late byte is the FIRST data byte, which rules out every "prefetch further ahead" idea, because byte 0's request cannot be issued before RHDR_LO where the low address byte arrives. That makes the deadline arithmetic with no contention in it: rx CDC ~125ns + arbiter 42 + SPRAM 42 + rsp_buf 42 + tx CDC ~110 = ~361ns against a 296ns byte time. Over budget idle, which is why fair arbitration changed nothing -- ETH contention was only what pushed a marginal path over on some frames. Two further changes were implemented, measured and then removed as unnecessary: issuing byte 1's request during the pad byte, and two requests in flight with a 3-deep response hold. With rsp_buf gone, T9 is 6/6 with both removed, with either alone, and with the register file reverted completely. They were real pipelining but not the constraint, and they cost logic on sync, the binding timing domain. CLAUDE.md records them as do-not-re-add, along with the trap that sank the earlier 2-outstanding attempt: incrementing `outstanding` in the FSM and decrementing it in the response handler are two m.d.sync assignments that Amaranth resolves by PRIORITY, not arithmetic, so the increment is silently discarded when both fire in one cycle. Arbiter TB updated (the response is a cycle earlier) and mutation-tested: re-registering the response fails T1/T2/T4, swapping the byte lanes fails T1/T2. New T4 pins exi_rsp_valid to exactly one cycle. Not yet confirmed on hardware; netboot's reader still probes both offsets. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -91,15 +91,21 @@ SPRAM/DMA. The EXI front end runs at the real 27 MHz hardware rate.
|
||||
(`Raw ID dump` reports SKIP once an earlier suite has issued the NCRA reset
|
||||
that clears `id_phase` — reflash or reset the FPGA to restore it.)
|
||||
|
||||
**Timing closes on EVERY seed (2026-10-03).** `python -m exi_bba.synth
|
||||
--seeds 8`:
|
||||
**Timing closes on EVERY seed (re-measured 2026-10-10).** `python -m
|
||||
exi_bba.synth --seeds 8`:
|
||||
|
||||
| domain | target | result |
|
||||
|---|---|---|
|
||||
| `clk` (sync, HFOSC) | 24 MHz | 28.5–30.7 MHz — **PASS on all 8 seeds** |
|
||||
| EXI pin domain | 27 MHz | 41.6–49.7 MHz — **PASS on all 8 seeds** |
|
||||
| `clk` (sync, HFOSC) | 24 MHz | 28.1–30.8 MHz — **PASS on all 8 seeds** |
|
||||
| EXI pin domain | 27 MHz | 57.7–66.8 MHz — **PASS on all 8 seeds** |
|
||||
|
||||
Best seed **3**: `clk` 30.68 MHz, EXI 49.14 MHz. Flash `build/seed<N>/top.bin`.
|
||||
Best seed **6**: `clk` 30.83 MHz, EXI 57.74 MHz. Flash `build/seed<N>/top.bin`.
|
||||
|
||||
Measured after making the SPRAM response combinational (the `+1` descriptor
|
||||
fix), which put SPRAM's output → byte mux → `tx_fifo` write data on `sync`'s
|
||||
combinational path — the binding domain. It cost nothing measurable: `clk` was
|
||||
28.5–30.7 before. The EXI figures replace a stale 41.6–49.7 in this table,
|
||||
which predated the full-cycle-early MISO launch.
|
||||
|
||||
⚠️ **fmax is NOT the only constraint — MISO clock-to-pad matters more.** MISO
|
||||
is source-synchronous: it leaves on the FALLING edge and the GameCube samples
|
||||
@@ -286,12 +292,22 @@ Details and the recovery ladder are in `gc_bba_test/README.md`. Key points:
|
||||
the drain consumes nothing for a whole transaction. **Do not raise it
|
||||
without deepening `tx_bytes` or giving the TX path real back-pressure.**
|
||||
|
||||
### ⚠️ OPEN: ring descriptor sometimes lands at page offset +1
|
||||
### ✅ FIXED 2026-10-10 — ring descriptor at page offset +1 (tx FIFO underrun)
|
||||
|
||||
On console the SPRAM ring descriptor is at page+0 for ~23% of frames and
|
||||
page+1 for ~77% (`desc@+0=100 +1=343` in one run), **varying frame to frame**.
|
||||
`netboot`'s reader probes both offsets and takes whichever yields a plausible
|
||||
length, so it is worked around, not fixed.
|
||||
**The fix is one deleted register.** `SPRAMArbiter` latched the SPRAM word
|
||||
into `rsp_buf` and raised `exi_rsp_valid` a cycle later; it now drives both
|
||||
COMBINATIONALLY in the cycle SPRAM's output is valid. `BBARegisterFile` was
|
||||
already taking the response the cycle it arrived, so the buffer bought nothing
|
||||
and cost 42 ns inside a deadline that was short by about that much.
|
||||
`bba_top` **T9 is now 6/6 at `EXI_HALF=1`, `2` and `8`**, and nothing else in
|
||||
the design changed — the register file is untouched.
|
||||
|
||||
⚠️ **Not yet confirmed on hardware.** `netboot`'s reader still probes both
|
||||
offsets; leave that in until a console run shows `+1=0`.
|
||||
|
||||
Original symptom: on console the SPRAM ring descriptor was at page+0 for ~23%
|
||||
of frames and page+1 for ~77% (`desc@+0=100 +1=343` in one run), **varying
|
||||
frame to frame**.
|
||||
|
||||
What has been ruled out:
|
||||
- **Not ring overflow** — `lapped=0` with RBFI active.
|
||||
@@ -331,10 +347,8 @@ SPRAM data. It only appears at 27 MHz because the read must finish inside the
|
||||
protocol's turnaround window while ETH (which has absolute arbiter priority) is
|
||||
preempting it.
|
||||
|
||||
⚠️ **`bba_top` therefore FAILS at `EXI_HALF=1` until this is fixed.** That is
|
||||
deliberate: the failure is real and the test is the reproduction. Do not
|
||||
"fix" it by relaxing T9. `EXI_HALF=8` and `EXI_HALF=2` still pass, so use
|
||||
those while working on unrelated changes.
|
||||
T9 is now the regression test for the fix: it must stay 6/6 at all three
|
||||
rates. Do not relax it.
|
||||
|
||||
**MECHANISM, established by reading the frame body instead of the descriptor.**
|
||||
Reading from 0x104 (first byte a known `0xDE`) rather than 0x100 (first byte
|
||||
@@ -365,18 +379,60 @@ latency is never hidden.
|
||||
| **Allow 2 outstanding requests** to pipeline the latency | **0/6 at every rate**, including `HALF=8` which previously passed. With no response FIFO, responses must be consumed the cycle they are valid; two in flight loses bytes. The existing comment was right. |
|
||||
| **Return both SPRAM bytes per request** (16-bit response + `exi_rsp_odd`, two pushes per round trip) | **WORSE: 6/6 / 3/6 / 1/6** at `HALF=8/2/1` against the baseline's 6/6 / 6/6 / 1/6 — it broke `HALF=2`, left the `DE DE` duplication at `HALF=1`, and introduced a new bug where the FIRST read after an idle period returns all zeros. Reverted. |
|
||||
|
||||
**Why doubling the payload did not work, which is the useful part.** The budget
|
||||
per round trip doubles to 592 ns only if the next request issues while the
|
||||
second byte is still being pushed. It cannot: the second byte lives in a
|
||||
holding register (`sp_buf`/`sp_cnt`) and the prefetch guard must wait for
|
||||
`sp_cnt == 0`, or the next response overwrites a byte that has not been pushed
|
||||
yet. So the sequence is still request → response → push → push → request, and
|
||||
the *latency* per round trip is unchanged while the deadline for the first byte
|
||||
of each pair is exactly as tight as before. **The constraint is round-trip
|
||||
latency that nothing hides, not bytes per round trip** — so any fix must either
|
||||
overlap requests with pushes (which needs a real response FIFO, the thing whose
|
||||
absence killed the 2-outstanding attempt) or remove the round trip from the
|
||||
critical path altogether.
|
||||
Every one of those tried to carry MORE DATA or WAIT LESS per round trip. None
|
||||
of them shortened the round trip, which is what the deadline is made of.
|
||||
|
||||
**What the measurement showed, and why it ended the guessing.** A counter in
|
||||
`ExiCapture` recording `tx_load & ~tx_fifo.r_rdy` per byte boundary, with a
|
||||
shift-register history of which pop underran:
|
||||
|
||||
```
|
||||
[0] DE AD BE EF 01 02 ur=1 pops=10 hist=0010000000 <- good
|
||||
[1] DE DE AD BE EF 01 ur=2 pops=10 hist=0011000000 <- bad
|
||||
```
|
||||
|
||||
Ten pops per read (4 header + 6 data). Good and bad reads BOTH underrun at
|
||||
pop 3 — harmless, mid-header, nothing is listening. A bad read adds **pop 4**,
|
||||
the pop that loads **data slot 0**. So the late byte is the FIRST data byte,
|
||||
not a later one, which kills every "prefetch further ahead" idea: byte 0's
|
||||
request cannot be issued before `RHDR_LO`, because that is where the low
|
||||
address byte arrives.
|
||||
|
||||
That makes the deadline pure arithmetic, with no contention in it at all:
|
||||
|
||||
| stage | cost |
|
||||
|---|---|
|
||||
| rx `AsyncFIFO` CDC (address byte → FSM) | ~125 ns |
|
||||
| arbiter accepts the request | 42 ns |
|
||||
| SPRAM read | 42 ns |
|
||||
| **response register (`rsp_buf`)** | **42 ns** |
|
||||
| tx `AsyncFIFO` CDC | ~110 ns |
|
||||
| **total** | **~361 ns vs a 296 ns byte time** |
|
||||
|
||||
Over budget by ~65 ns *idle*. That is why fair arbitration changed nothing —
|
||||
ETH contention was never the cause, only what pushed a marginal path over on
|
||||
some frames. Deleting `rsp_buf` removes 42 ns and the CDCs' phase jitter covers
|
||||
the rest.
|
||||
|
||||
⚠️ **Two things were implemented, measured, and then REMOVED as unnecessary** —
|
||||
do not add them back without evidence:
|
||||
- **issuing byte 1's request during the pad byte** (priming the pipeline), and
|
||||
- **two requests in flight with a 3-deep response hold** (`SP_LIMIT=2`).
|
||||
|
||||
Ablation: with the `rsp_buf` deletion in place, T9 is 6/6 at every rate with
|
||||
BOTH removed, with either alone, and with the register file reverted
|
||||
completely. They were real pipelining and they were not the constraint. They
|
||||
also cost logic on `sync`, which is the binding timing domain.
|
||||
|
||||
⚠️ And a trap worth knowing if anyone does revisit multiple outstanding
|
||||
requests: `outstanding` was incremented by the FSM and decremented by the
|
||||
response handler as two separate `m.d.sync` assignments. Amaranth resolves
|
||||
those by PRIORITY, not arithmetic — the later one wins outright and the
|
||||
increment is silently discarded. With one request in flight they can never
|
||||
coincide; with two they coincide constantly, the count drifts down, and
|
||||
responses get dropped by the `outstanding != 0` guard. Symptom: DMA reads with
|
||||
bytes MISSING rather than duplicated (`00 00 00 0C DE EF 01 ...`). Give such a
|
||||
counter exactly one writer fed by comb intent signals.
|
||||
|
||||
### ⚠️ Simulation rate — always re-run at the hardware rate
|
||||
|
||||
|
||||
+44
-19
@@ -41,8 +41,9 @@ class SPRAMArbiter(Elaboratable):
|
||||
exi_req_addr : 16-bit byte address to read
|
||||
exi_req_valid : FIFO r_rdy — a request is waiting
|
||||
exi_req_ready : FIFO r_en — pop the request (asserted when serviced)
|
||||
exi_rsp_data : 8-bit result byte
|
||||
exi_rsp_valid : FIFO w_en — push result when valid
|
||||
exi_rsp_data : 8-bit result byte — COMBINATIONAL, valid only while
|
||||
exi_rsp_valid is high; the consumer must take it that cycle
|
||||
exi_rsp_valid : result is valid this cycle
|
||||
|
||||
ETH write interface (from RXFrameAssembler)
|
||||
-------------------------------------------
|
||||
@@ -120,7 +121,17 @@ class SPRAMArbiter(Elaboratable):
|
||||
|
||||
read_pending = Signal() # a read address was issued last cycle
|
||||
read_was_odd = Signal() # byte address bit 0 of the pending read
|
||||
rsp_buf = Signal(8) # registered response byte; valid when exi_rsp_valid
|
||||
|
||||
# The response is COMBINATIONAL in the cycle SPRAM's output is valid,
|
||||
# not registered for one more. That register cost a 24 MHz cycle
|
||||
# (~42 ns) inside the EXI read's deadline: the request for data byte 0
|
||||
# cannot be issued before the header's low address byte has crossed
|
||||
# the rx CDC, leaving ~296 ns for rx CDC (~125 ns) + this round trip +
|
||||
# the tx CDC (~110 ns). With the extra register that totals ~361 ns
|
||||
# and the engine's pop for data slot 0 found tx_fifo empty, resending
|
||||
# the stale head (the duplicated-first-byte underrun). The consumer
|
||||
# takes the response the cycle it is valid, so the buffer bought
|
||||
# nothing.
|
||||
|
||||
# Combinatorial defaults
|
||||
m.d.comb += [
|
||||
@@ -130,13 +141,12 @@ class SPRAMArbiter(Elaboratable):
|
||||
spram_addr .eq(0),
|
||||
self.exi_req_ready.eq(0),
|
||||
self.eth_wr_ready .eq(0),
|
||||
self.exi_rsp_data .eq(rsp_buf), # always sourced from registered buffer
|
||||
self.exi_rsp_valid.eq(read_pending),
|
||||
self.exi_rsp_data .eq(Mux(read_was_odd,
|
||||
spram_dout[8:16], spram_dout[0:8])),
|
||||
]
|
||||
# Registered defaults
|
||||
m.d.sync += [
|
||||
self.exi_rsp_valid.eq(0),
|
||||
read_pending .eq(0),
|
||||
]
|
||||
m.d.sync += read_pending.eq(0)
|
||||
|
||||
# ETH write has priority
|
||||
with m.If(self.eth_wr_valid):
|
||||
@@ -167,14 +177,6 @@ class SPRAMArbiter(Elaboratable):
|
||||
read_was_odd.eq(self.exi_req_addr[0]),
|
||||
]
|
||||
|
||||
# Capture SPRAM output into registered buffer after 1-cycle latency
|
||||
with m.If(read_pending):
|
||||
with m.If(read_was_odd):
|
||||
m.d.sync += rsp_buf.eq(spram_dout[8:16])
|
||||
with m.Else():
|
||||
m.d.sync += rsp_buf.eq(spram_dout[0:8])
|
||||
m.d.sync += self.exi_rsp_valid.eq(1)
|
||||
|
||||
return m
|
||||
|
||||
|
||||
@@ -206,8 +208,12 @@ if __name__ == "__main__":
|
||||
ctx.set(dut.exi_req_valid, 1)
|
||||
await ctx.tick("sync").repeat(1) # clock A: read issued, read_pending=1
|
||||
ctx.set(dut.exi_req_valid, 0)
|
||||
await ctx.tick("sync").repeat(1) # clock B: SPRAM output captured, valid=1
|
||||
# Check HERE — exi_rsp_valid is 1 for exactly this one cycle
|
||||
# Check HERE. The response is COMBINATIONAL: during the cycle after
|
||||
# the issue, read_pending is high and SPRAM's output is valid, so
|
||||
# exi_rsp_valid and exi_rsp_data are live RIGHT NOW. Waiting another
|
||||
# tick reads zeros -- which is the regression this asserts against,
|
||||
# since a registered response would need that extra tick and would
|
||||
# cost the EXI read 42 ns it does not have.
|
||||
|
||||
rdata = ctx.get(dut.exi_rsp_data)
|
||||
rvalid = ctx.get(dut.exi_rsp_valid)
|
||||
@@ -231,9 +237,10 @@ if __name__ == "__main__":
|
||||
ctx.set(dut.exi_req_valid, 1)
|
||||
await ctx.tick("sync").repeat(1)
|
||||
ctx.set(dut.exi_req_valid, 0)
|
||||
await ctx.tick("sync").repeat(1)
|
||||
|
||||
rdata = ctx.get(dut.exi_rsp_data)
|
||||
if not ctx.get(dut.exi_rsp_valid):
|
||||
errors.append("T2 exi_rsp_valid not set")
|
||||
if rdata != 0xCD:
|
||||
errors.append(f"T2 odd addr read-back: expected 0xCD, got 0x{rdata:02X}")
|
||||
print(f"T2 odd addr read-back: data=0x{rdata:02X}")
|
||||
@@ -260,6 +267,24 @@ if __name__ == "__main__":
|
||||
errors.append("T3 ETH priority: EXI read was not blocked")
|
||||
print(f"T3 ETH priority: eth_won={eth_won} exi_blocked={exi_blocked}")
|
||||
|
||||
await ctx.tick("sync").repeat(2)
|
||||
|
||||
# T4: exi_rsp_valid is high for EXACTLY the one cycle after the issue.
|
||||
# The consumer (BBARegisterFile) relies on that to count responses
|
||||
# against requests in flight; a valid that lingered would be counted
|
||||
# twice and insert a byte into the GC's read.
|
||||
ctx.set(dut.exi_req_addr, 0x0100)
|
||||
ctx.set(dut.exi_req_valid, 1)
|
||||
await ctx.tick("sync").repeat(1)
|
||||
ctx.set(dut.exi_req_valid, 0)
|
||||
v_now = ctx.get(dut.exi_rsp_valid)
|
||||
await ctx.tick("sync").repeat(1)
|
||||
v_after = ctx.get(dut.exi_rsp_valid)
|
||||
if not v_now or v_after:
|
||||
errors.append(f"T4 rsp_valid window: now={v_now} after={v_after} "
|
||||
f"(want 1 then 0)")
|
||||
print(f"T4 rsp_valid one cycle only: now={v_now} after={v_after}")
|
||||
|
||||
sim = Simulator(dut)
|
||||
sim.add_clock(Period(MHz=24), domain="sync")
|
||||
sim.add_testbench(testbench)
|
||||
|
||||
Reference in New Issue
Block a user