diff --git a/CLAUDE.md b/CLAUDE.md index e92b937..373a02e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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/top.bin`. +Best seed **6**: `clk` 30.83 MHz, EXI 57.74 MHz. Flash `build/seed/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 diff --git a/exi_bba/spram_arbiter.py b/exi_bba/spram_arbiter.py index f1ddd92..7c17a55 100644 --- a/exi_bba/spram_arbiter.py +++ b/exi_bba/spram_arbiter.py @@ -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)