diff --git a/exi_bba/rx_frame_assembler.py b/exi_bba/rx_frame_assembler.py index f69abfb..82b70ed 100644 --- a/exi_bba/rx_frame_assembler.py +++ b/exi_bba/rx_frame_assembler.py @@ -1,61 +1,52 @@ -"""RX frame assembler — sync domain (24 MHz). +"""RX frame assembler — takes received ethernet frames into the SPRAM ring. -Receives raw ethernet frames from W5500SPIMaster and writes them into the SPRAM -ring buffer in MX98730EC format. +Runs entirely in the `sync` domain (24 MHz). Accepts a byte stream from +whichever ethernet master is built (`W5100ParallelMaster` by default, or +`W5500SPIMaster`) and lays each frame out in the ring the way the GameCube's +own BBA driver expects to find it. -Ring buffer layout (SPRAM byte addresses) ------------------------------------------- -0x0100–0x0FFF 15 pages × 256 bytes = 3840 bytes -Pages 0x01–0x0F; page 0x00 is reserved. -Page wrap: after 0x0F → 0x01 (skip 0x00). +Ring layout +----------- +SPRAM bytes 0x0100-0x0FFF, as 15 pages of 256 bytes. Pages 0x01-0x0F are +usable and page 0x00 is reserved for TX, so the wrap is 0x0F → 0x01. The GC +can narrow the ring through its BP/RHBP registers; see `_ring_bounds`. -Frame descriptor (4 bytes at page start) ------------------------------------------------ -A PACKED 32-BIT LITTLE-ENDIAN WORD, not a byte-wise layout: +A frame occupies consecutive pages from its own page start, and the next frame +begins on the next free page -- frames are never split across the wrap, which +is why the writer reserves a worst-case 6 pages before starting one. - bits 0–11 : next_packet_ptr — the PAGE at which the NEXT frame starts - bits 12–23 : packet_len — total bytes, INCLUDING these 4 - bits 24–31 : status — errors are 0x80 and 0x04; 0x00 means good +Frame descriptor +---------------- +Four bytes at each page start, forming a PACKED 32-BIT LITTLE-ENDIAN WORD. +This is not a byte-wise layout, and it must not be "simplified" into a plain +big-endian length: -so the four bytes at the page start are + bits 0-11 next_packet_ptr the PAGE the NEXT frame starts at + bits 12-23 packet_len total bytes, INCLUDING these 4 + bits 24-31 status 0x00 is good; 0x80 and 0x04 are errors - Byte 0: next_packet_ptr[7:0] - Byte 1: packet_len[3:0] << 4 | next_packet_ptr[11:8] - Byte 2: packet_len[11:4] - Byte 3: status - Bytes 4+: raw ethernet frame +which puts on the wire: -⚠️ This is NOT a guess and must not be "simplified" back to a plain -big-endian length. It was read out of libogc's own BBA driver -(`/opt/devkitpro/libogc/lib/cube/libbba.a`, `gcif.o`, `bba_interrupt`), which -is the driver GC software actually runs: + byte 0 next_packet_ptr[7:0] + byte 1 packet_len[3:0] << 4 | next_packet_ptr[11:8] + byte 2 packet_len[11:4] + byte 3 status + byte 4+ the raw ethernet frame - lwbrx r9,0,r9 # load the 4 bytes as a LITTLE-ENDIAN word - rlwinm r4,r9,20,20,31 # packet_len = (word >> 12) & 0xFFF - addi r4,r4,-4 # payload = packet_len - 4 - cmpwi r4,1540 # ... which must be <= 1540 - srwi r9,r9,24 # status = word >> 24 - and. r9,r9,r18 # r18 = 0x...84, so errors are 0x80 | 0x04 - -and it writes the low 12 bits straight back to RRP as the next read pointer: - - lwz r9,cur_descr # byte 0 -> register 0x0018 (RRP_LO) - rlwinm r9,r31,24,28,31 # bits 11:8 -> register 0x0019 (RRP_HI) - -The length semantics and addressing already matched (descriptor at page<<8, -body at +4, length including the descriptor); the PACKING did not, and -`next_packet_ptr` was never written at all. Our previous -`00 00 len[11:8] len[7:0]` decodes under that driver as packet_len = 16 and -next_packet_ptr = 0 for a 342-byte frame — a 12-byte frame and a read pointer -into the reserved page. +The driver takes the low 12 bits of that word and writes them straight to its +RRP register, so `next_packet_ptr` is load-bearing and not informational. The +layout was read out of libogc's own compiled driver rather than inferred; the +disassembly is recorded in CLAUDE.md. Write sequence -------------- -1. Issue 4 SPRAM writes of 0x00 (placeholder descriptor). -2. For each byte received from W5500, issue one SPRAM write. -3. After EOF: compute the next page, then rewrite descriptor bytes 0, 1 and 2 - with the packed next_packet_ptr + packet_len. Byte 3 (status) stays 0x00. -4. Advance RWP, push to rx_wptr FIFO, pulse rx_irq. +1. Write four placeholder 0x00 bytes where the descriptor will go. +2. Write each received byte straight through, one SPRAM write per byte. +3. On EOF, compute the next page, then write descriptor bytes 0-3 for real. +4. Advance RWP, push it to the register file, and pulse `rx_irq`. + +The descriptor is written last because neither the length nor the next page is +known until the frame has ended. """ from amaranth import * @@ -72,24 +63,47 @@ _MAX_FRAME_PAGES = 6 class RXFrameAssembler(Elaboratable): """Writes incoming ethernet frames into the SPRAM ring buffer. - W5500 streaming interface (sync domain) + Ethernet byte stream in + ----------------------- + rx_data, rx_valid, rx_ready + Standard ready/valid handshake. `rx_ready` is LOW while the + descriptor placeholder is being written, so the master must honour it. + rx_sof, rx_eof + Frame delimiters, asserted in the same cycle as `rx_valid`. + + SPRAM write port out (to `SPRAMArbiter`) ---------------------------------------- - rx_data / rx_valid / rx_ready : byte stream - rx_sof / rx_eof : frame delimiters (same cycle as rx_valid) + eth_wr_addr, eth_wr_data, eth_wr_valid, eth_wr_ready - SPRAM write interface (to SPRAMArbiter, sync domain) - ----------------------------------------------------- - eth_wr_addr / eth_wr_data / eth_wr_valid / eth_wr_ready + To the register file + ------------------- + rx_wptr_w_data, rx_wptr_w_en, rx_wptr_w_rdy + The new write page after each stored frame, through a `SyncFIFO`. + rx_irq + One-cycle pulse: a frame is in the ring. Becomes IR[1] (RI). + rx_overflow + One-cycle pulse: a frame was DROPPED because the ring was full. + Becomes IR[7] (RBFI). - CDC outputs (wired by BBATop) - ----------------------------- - rx_wptr_w_data / rx_wptr_w_en / rx_wptr_w_rdy - rx_irq : 1-cycle pulse → PulseSynchronizer input - rx_enabled : controlled by NCRA SR bit (from BBARegisterFile) + From the register file + --------------------- + rx_enabled + NCRA's SR bit. Frames are ignored entirely while it is low. + rrp + The GC's read pointer, so free ring space can be computed. + bp, rhbp + Ring geometry the GC asked for; see `_ring_bounds`. + ncra_rst + The GC's NCRA reset; see `_ncra_reset`. + geom_honoured + High while `bp`/`rhbp` are usable and in force, low while they have + been rejected and the default layout is standing in. Nothing consumes + it in the design -- it exists so the tests can tell the two cases + apart, which is otherwise unobservable from outside. """ def __init__(self): - # W5500 stream in + # Ethernet stream in self.rx_data = Signal(8) self.rx_valid = Signal() self.rx_ready = Signal() @@ -102,112 +116,75 @@ class RXFrameAssembler(Elaboratable): self.eth_wr_valid = Signal() self.eth_wr_ready = Signal() - # RWP FIFO write-side (sync→exi) + # RWP hand-off to the register file self.rx_wptr_w_data = Signal(8) self.rx_wptr_w_en = Signal() self.rx_wptr_w_rdy = Signal() - # rx_irq pulse (→ PulseSynchronizer) - self.rx_irq = Signal() + self.rx_irq = Signal() + self.rx_overflow = Signal() - # RX gate from NCRA SR bit self.rx_enabled = Signal() - - # ── Ring-overflow protection (RBFI) ─────────────────────────────── - # The GC's read pointer, so we can tell how much of the ring is still - # unread. Without it this module overwrote pages the GC had not read - # yet: a traffic burst corrupted whatever frame the driver was midway - # through, with no indication that it had happened. - self.rrp = Signal(8, init=_RX_PAGE_FIRST) - # ── Ring geometry, from the GC's own BP / RHBP registers ────────── - # compat TODO item 2. These were hardcoded, so a driver configuring a - # SMALLER RX ring was ignored and we kept writing pages it would never - # read -- silent corruption with nothing reporting an error. libogc2 - # writes BP = 0x0001 and RHBP = 0x000F, which happens to match the old - # hardcoded layout exactly, which is why it never showed up. + self.rrp = Signal(8, init=_RX_PAGE_FIRST) self.bp = Signal(12, init=_RX_PAGE_FIRST) # first RX page self.rhbp = Signal(12, init=_RX_PAGE_LAST) # last RX page - # Pulses when a frame had to be DROPPED because the ring was full. - # Drives IR[7] (RBFI), which the GC's IMR enables (0x86). - self.rx_overflow = Signal() - # High when the driver's BP/RHBP were usable and are being honoured; - # low means they were rejected and the hardcoded layout is in use, so - # a mismatch is observable rather than silent. self.geom_honoured = Signal() - - # The GC's NCRA reset. MUST reset `rwp` here as well as the register - # file's copy: they are two copies of the same pointer, and resetting - # only one leaves them disagreeing about where the ring is. A driver - # that then syncs RRP to the register file's value is pointing - # somewhere the writer is not, the free-space calculation shrinks with - # every frame, and once it falls below _MAX_FRAME_PAGES EVERY frame is - # dropped -- silently, because a dropped frame never reaches the ring - # to be counted. Hardware symptom: "0 seen, 0 bad desc, 0 lapped". self.ncra_rst = Signal() def elaborate(self, platform): m = Module() - # ── Ring-buffer state ───────────────────────────────────────────── - rwp = Signal(8, init=_RX_PAGE_FIRST) # current RX write page (1–15) + # The current RX write page, shared by the free-space calculation, the + # frame FSM and the NCRA reset. + rwp = Signal(8, init=_RX_PAGE_FIRST) - # Write address within current frame - wr_addr = Signal(16) - # Number of frame data bytes received - data_ctr = Signal(12) - # Total length = data_ctr + 4 - total_len = Signal(12) - # The page the NEXT frame will start at. Computed before the - # descriptor is written, because the descriptor carries it as - # next_packet_ptr -- libogc writes those 12 bits straight to RRP. - next_page = Signal(12) + # All three outputs are one-cycle pulses, so they default low and the + # FSM below overrides them for the cycle it needs. + m.d.sync += [self.rx_irq.eq(0), + self.rx_wptr_w_en.eq(0), + self.rx_overflow.eq(0)] - # Descriptor base (rwp*256) — saved when frame starts - desc_base = Signal(16) + pg = self._ring_bounds(m) + have_room = self._free_space(m, rwp, pg) + self._frame_fsm(m, rwp, pg, have_room) + self._ncra_reset(m, rwp, pg) + return m - # Placeholder descriptor byte counter (0..3) - desc_ctr = Signal(2) + def _ring_bounds(self, m): + """Sanitise the GC's requested ring geometry. - # Number of pages consumed by this frame (rounded up) - pages_used = Signal(5) + Returns a `(first, last, total)` tuple of signals describing the ring + actually in use. - # Default: no pulses - m.d.sync += self.rx_irq.eq(0) - m.d.sync += self.rx_wptr_w_en.eq(0) - m.d.sync += self.rx_overflow.eq(0) + `bp`/`rhbp` are honoured only when they describe a usable ring inside + SPRAM; otherwise the default 1-15 layout stands and `geom_honoured` + goes low so the disagreement is observable. Page 0 holds the TX + buffer, so BP must be at least 1, and a garbage write (BP = 0, or RHBP + below BP) would otherwise give a zero or negative ring size and send + the writer outside it. - # ── Sanitised ring bounds ───────────────────────────────────────── - # A driver's BP/RHBP are used only when they describe a usable ring - # inside SPRAM; otherwise the hardcoded layout stands. Without this a - # garbage write (BP = 0, or RHBP below BP) would give a zero or - # negative ring size and the assembler would write outside it. Page 0 - # is reserved for the TX buffer, so BP must be at least 1. - # ⚠️ REGISTERED, not combinational. Computing these every cycle put - # bp/rhbp -> comparators -> Mux -> subtract -> pg_total -> add -> raw - # -> compare -> free -> compare -> have_room all in ONE combinational - # path, where it used to be constant-folded. `clk` fmax collapsed - # from ~29 MHz to ~17 MHz and ALL EIGHT SEEDS FAILED the 24 MHz - # target. bp/rhbp only change when the GC writes BP/RHBP, so one - # cycle of staleness costs nothing and splits the path in two. - # ⚠️ REGISTERED AND 4 BITS WIDE, both deliberately. - # - # Computing these combinationally put bp/rhbp -> comparators -> Mux -> - # subtract -> pg_total -> add -> raw -> compare -> free -> compare -> - # have_room in ONE path, where it used to be constant-folded. `clk` - # fmax collapsed from ~29 MHz to ~17 MHz and ALL EIGHT SEEDS FAILED - # the 24 MHz target. Registering split it, but 8-bit variable-vs- - # variable comparators are carry chains where variable-vs-CONSTANT - # folds into a couple of LUTs, so it was still only ~20 MHz. - # - # Ring pages are 1-15, so 4 bits is the natural width; geom_ok - # guarantees bp < rhbp <= 0x0F, which is what makes the narrow slice - # safe. bp/rhbp change only when the GC writes BP/RHBP, so a cycle of - # staleness costs nothing. + ⚠️ These are REGISTERED and 4 BITS WIDE, both deliberately, and both + for timing rather than function. + + Computing them combinationally put bp/rhbp → comparators → Mux → + subtract → total → add → compare → free → compare → have_room in ONE + path, where the constants it replaced had been folded away. `clk` + fmax collapsed from ~29 MHz to ~17 MHz and ALL EIGHT SEEDS FAILED the + 24 MHz target. Registering split the path, but 8-bit variable-vs- + variable comparators are carry chains where variable-vs-constant folds + into a couple of LUTs, so it was still only ~20 MHz. + + Ring pages are 1-15, so 4 bits is the natural width, and the + `geom_ok` test guarantees bp < rhbp <= 0x0F -- which is what makes the + narrow slice safe. `bp`/`rhbp` only change when the GC writes BP or + RHBP, so a cycle of staleness costs nothing. + """ geom_ok = Signal() geom_ok_c = Signal() pg_first = Signal(4, init=_RX_PAGE_FIRST) pg_last = Signal(4, init=_RX_PAGE_LAST) pg_total = Signal(5, init=_PAGES_TOTAL) + m.d.comb += geom_ok_c.eq((self.bp >= _RX_PAGE_FIRST) & (self.rhbp <= _RX_PAGE_LAST) & (self.rhbp > self.bp)) @@ -219,19 +196,35 @@ class RXFrameAssembler(Elaboratable): _PAGES_TOTAL)), ] m.d.comb += self.geom_honoured.eq(geom_ok) + return pg_first, pg_last, pg_total + + def _free_space(self, m, rwp, pg): + """How many pages may still be consumed. Returns `have_room`. + + `rwp == rrp` means EMPTY, so writing the whole ring would wrap back to + looking empty and lose the lot. Usable is therefore + `(rrp - rwp - 1) mod total`, which is `total - 1` on an empty ring. + + A frame's length is unknown until EOF, so starting one reserves the + WORST case -- 1518 bytes plus the descriptor, six pages. Beginning a + frame with less room than that risks running into unread data partway + through, which is worse than refusing it outright. + + ⚠️ `have_room` and the clamped `rrp` are REGISTERED, for timing. The + free-space sum is a mod-15 subtract plus a comparator, and feeding + that straight into the FSM's branch condition cost real depth in + `sync`, the domain with the least margin in this design (`clk` fell + 27.8-32.2 → 27.0-28.0 MHz). A cycle of staleness is harmless: `rwp` + only moves at end-of-frame and `rrp` only when the GC writes it, so + both are many cycles old by the time the next SOF arrives. + """ + pg_first, pg_last, pg_total = pg - # ── How many pages may we still consume? ────────────────────────── - # rwp == rrp means EMPTY, so writing the whole ring would wrap back to - # "empty" and lose the lot. Usable = (rrp - rwp - 1) mod pg_total, - # which is pg_total-1 when the ring is empty. - # rrp_s is REGISTERED too: the clamp is two comparisons against the - # (now variable) bounds, and the GC writes RRP about as often as it - # writes BP/RHBP. `free` already reaches the FSM through a register - # (`have_room`), so this adds no behavioural latency that matters. rrp_s = Signal(4, init=_RX_PAGE_FIRST) m.d.sync += rrp_s.eq(Mux((self.rrp >= pg_first) & (self.rrp <= pg_last), self.rrp[0:4], pg_first)) + raw = Signal(5) free = Signal(5) with m.If(rrp_s >= rwp): @@ -240,21 +233,59 @@ class RXFrameAssembler(Elaboratable): m.d.comb += raw.eq(rrp_s + pg_total - rwp) m.d.comb += free.eq(Mux(raw == 0, pg_total - 1, raw - 1)) - # A frame's length is not known until EOF, so reserve the worst case: - # 1518 bytes + a 4-byte descriptor is 6 pages. Starting a frame with - # less room than that risks running into unread data partway through, - # which is worse than refusing it outright. - # REGISTERED, not combinational. The free-space calculation is a - # mod-15 subtract plus a comparator, and feeding that into the FSM's - # branch condition cost real depth in `sync` -- the domain with the - # least margin in this design (clk fell 27.8-32.2 -> 27.0-28.0 MHz - # when this was comb). One cycle of staleness is harmless: rwp only - # moves at end-of-frame and rrp only when the GC writes it, so both - # are many cycles old by the time the next SOF arrives. have_room = Signal() m.d.sync += have_room.eq(free >= _MAX_FRAME_PAGES) + return have_room + + def _frame_fsm(self, m, rwp, pg, have_room): + """Store one frame per pass through the ring. + + States, in the order a frame visits them: + + IDLE wait for SOF; branch on whether the ring has + room for a worst-case frame + DROP_FRAME ring full: consume to EOF and raise rx_overflow + WRITE_PLACEHOLDER four 0x00 bytes where the descriptor will go, + so the body starts at page+4 + RECV_AND_WRITE stream each received byte into SPRAM + WAIT_LAST_WRITE let the final body write be accepted, and work + out how many pages the frame used + CALC_NEXT compute the page the NEXT frame will start at + WRITE_D0..D3 write the real descriptor, now that length and + next page are both known + ADVANCE_RWP move the write pointer to that next page + PUSH_WPT hand RWP to the register file and pulse rx_irq + + ⚠️ EVERY write state advances on `valid & ready`, never on `ready` + alone. `eth_wr_valid` is scheduled with `m.d.sync` and so is LOW for + the whole first cycle in a state, while `ready` is tested immediately + -- and `ready` is usually already high on entry. Testing `ready` + alone fires `valid.eq(1)` and `valid.eq(0)` in the same cycle, the + later assignment wins, and the write is never issued. + + That bug existed twice, and neither instance was visible to a + testbench that held `ready` high: + + * in the placeholder, only THREE bytes went out, so the body began + at page+3 and EVERY FRAME WAS SHIFTED ONE BYTE EARLY. On hardware + the ring filled and RWP advanced normally while the GC's + ethertype test read the wrong offset and silently dropped + everything. + * in the descriptor, the length write was dropped, leaving byte 2 at + its placeholder -- hardware read a descriptor of 00 00 00 01 and + rejected the ring. Intermittent, because it depended on whether + the arbiter happened to hold `ready` high on the entry cycle. + """ + pg_first, pg_last, pg_total = pg + + wr_addr = Signal(16) # next SPRAM byte address to write + data_ctr = Signal(12) # frame data bytes received so far + total_len = Signal(12) # data_ctr + the 4 descriptor bytes + next_page = Signal(12) # page the NEXT frame will start at + desc_base = Signal(16) # this frame's page start (rwp * 256) + desc_ctr = Signal(2) # placeholder bytes written, 0..3 + pages_used = Signal(5) - # Combinatorial outputs m.d.comb += total_len.eq(data_ctr + 4) with m.FSM(domain="sync", name="rx_fsm"): @@ -264,9 +295,6 @@ class RXFrameAssembler(Elaboratable): m.d.sync += self.eth_wr_valid.eq(0) with m.If(self.rx_valid & self.rx_sof & self.rx_enabled & ~have_room): - # Ring full: discard this frame rather than overwrite data - # the GC has not read. This is what the real BBA's RBFI - # reports, and what this module previously lacked. m.next = "DROP_FRAME" with m.Elif(self.rx_valid & self.rx_sof & self.rx_enabled): frame_base = Signal(16) @@ -278,40 +306,23 @@ class RXFrameAssembler(Elaboratable): m.next = "WRITE_PLACEHOLDER" with m.State("DROP_FRAME"): - # Consume and discard to EOF. rx_ready MUST stay high here: - # stalling the ethernet master instead would back pressure into - # the W5100's own buffer and turn a dropped frame into a wedged - # receiver. + # Discard to EOF rather than overwrite data the GC has not + # read. This is what the real BBA reports through RBFI. + # + # rx_ready MUST stay high: stalling the ethernet master + # instead would back-pressure into the W5100's own buffer and + # turn a dropped frame into a wedged receiver. m.d.comb += self.rx_ready.eq(1) with m.If(self.rx_valid & self.rx_eof): m.d.sync += self.rx_overflow.eq(1) m.next = "IDLE" with m.State("WRITE_PLACEHOLDER"): - # Write 4 bytes of 0x00 as placeholder descriptor. - # - # ⚠️ Advance on `valid & ready`, NOT on `ready` alone. - # `eth_wr_valid` is scheduled with m.d.sync and so is LOW for - # the whole first cycle in this state, while `ready` is tested - # immediately -- and `ready` is usually already high here. - # Testing `ready` alone therefore counted a write that was - # never issued: only THREE placeholder bytes went out and - # wr_addr ended at desc_base+3, so the frame body started at - # descriptor byte 3 and EVERY RECEIVED FRAME WAS SHIFTED ONE - # BYTE EARLY. - # - # Hardware symptom (2026-10-09): the ring filled normally and - # RWP advanced, but the GC's ethertype test read the wrong - # offset and silently dropped every frame -- "no ACK" from the - # host with no other sign of trouble. The matching - # descriptor bug (same cause, in the length write) showed up as a - # descriptor of 00 00 00 01. - # - # The first byte is written twice as a consequence of - # eth_wr_addr trailing wr_addr by a cycle, and desc_base+3 is - # left unwritten -- both harmless, because every descriptor - # byte is either zero anyway or overwritten by WRITE_LEN_*. - # What matters is that wr_addr ends at desc_base+4. + # The first byte is written twice, because eth_wr_addr trails + # wr_addr by a cycle, and desc_base+3 is never reached. Both + # are harmless -- every descriptor byte is overwritten by + # WRITE_D0..D3 later. What matters is that wr_addr ends at + # desc_base+4, so the frame body starts there. m.d.sync += self.eth_wr_addr .eq(wr_addr) m.d.sync += self.eth_wr_data .eq(0x00) m.d.sync += self.eth_wr_valid.eq(1) @@ -324,7 +335,9 @@ class RXFrameAssembler(Elaboratable): m.d.sync += desc_ctr.eq(desc_ctr + 1) with m.State("RECV_AND_WRITE"): - # Accept bytes from W5500 and write each to SPRAM immediately + # One SPRAM write per received byte, with no intermediate + # buffering: accept a byte whenever the write port can take + # the previous one. m.d.comb += self.rx_ready.eq(~self.eth_wr_valid | self.eth_wr_ready) with m.If(self.rx_valid & (~self.eth_wr_valid | self.eth_wr_ready)): m.d.sync += self.eth_wr_addr.eq(wr_addr) @@ -338,34 +351,15 @@ class RXFrameAssembler(Elaboratable): m.d.sync += self.eth_wr_valid.eq(0) with m.State("WAIT_LAST_WRITE"): - # Wait for the last data byte write to be accepted with m.If(~self.eth_wr_valid | self.eth_wr_ready): m.d.sync += self.eth_wr_valid.eq(0) - # Compute pages used: ceil((data_ctr + 4) / 256) - # = (total_len + 255) >> 8 = total_len[11:8] + (total_len[7:0] != 0) + # pages = ceil(total_len / 256) m.d.sync += pages_used.eq(total_len[8:12] + (total_len[:8] != 0)) m.next = "CALC_NEXT" - # ⚠️ All three descriptor writes must advance on `valid & ready`, - # NOT on `ready` alone. `valid` is scheduled with m.d.sync and so is low - # for the whole first cycle in the state, while `ready` is tested - # immediately -- and coming out of WAIT_LAST_WRITE `ready` is - # usually already high. Testing `ready` alone therefore fired both - # valid.eq(1) and valid.eq(0) in the same cycle, the later won, the - # write was never issued, and the length byte was SILENTLY DROPPED. - # - # Hardware symptom (2026-10-09): a descriptor of 00 00 00 01 -- - # byte 2 still at its placeholder 0x00 because the HI write was - # dropped -- so the GC read a 1-byte frame length and rejected the - # ring. Intermittent by nature: it depends on whether the arbiter - # happens to hold `ready` high on the entry cycle, which is why - # bring-up's 342-byte DHCP frame read a correct 00 00 01 5A and - # this went unnoticed. with m.State("CALC_NEXT"): - # The descriptor carries next_packet_ptr, so the next page must - # be known BEFORE the descriptor is written -- this state used - # to come after it (as ADVANCE_RWP). - # next_rwp = ((rwp - 1 + pages_used) % 15) + 1 + # The descriptor carries next_packet_ptr, so the next page has + # to be known BEFORE the descriptor is written. next_rwp_raw = Signal(8) m.d.comb += next_rwp_raw.eq(rwp + pages_used) with m.If(next_rwp_raw > pg_last): @@ -375,7 +369,7 @@ class RXFrameAssembler(Elaboratable): m.next = "WRITE_D0" with m.State("WRITE_D0"): - # Descriptor byte 0 = next_packet_ptr[7:0] + # next_packet_ptr[7:0] m.d.sync += self.eth_wr_addr.eq(desc_base + 0) m.d.sync += self.eth_wr_data.eq(next_page[0:8]) m.d.sync += self.eth_wr_valid.eq(1) @@ -384,7 +378,7 @@ class RXFrameAssembler(Elaboratable): m.next = "WRITE_D1" with m.State("WRITE_D1"): - # Descriptor byte 1 = packet_len[3:0] << 4 | next_ptr[11:8]. + # packet_len[3:0] << 4 | next_packet_ptr[11:8]. # Cat puts its FIRST argument in the low bits. m.d.sync += self.eth_wr_addr.eq(desc_base + 1) m.d.sync += self.eth_wr_data.eq( @@ -395,8 +389,7 @@ class RXFrameAssembler(Elaboratable): m.next = "WRITE_D2" with m.State("WRITE_D2"): - # Descriptor byte 2 = packet_len[11:4]. Byte 3 (status) keeps - # the placeholder 0x00, which libogc reads as "no error". + # packet_len[11:4] m.d.sync += self.eth_wr_addr.eq(desc_base + 2) m.d.sync += self.eth_wr_data.eq(total_len[4:12]) m.d.sync += self.eth_wr_valid.eq(1) @@ -405,19 +398,16 @@ class RXFrameAssembler(Elaboratable): m.next = "WRITE_D3" with m.State("WRITE_D3"): - # Descriptor byte 3 = status = 0x00 ("no error": libogc tests - # it against 0x84). + # status = 0x00, "no error": the driver tests this byte + # against 0x84. # - # ⚠️ This MUST be written explicitly and cannot rely on the - # placeholder. WRITE_PLACEHOLDER issues four writes but the - # first lands twice (eth_wr_addr trails wr_addr by a cycle), - # so desc_base+3 is NEVER written. That was harmless while - # the length write covered byte 3; now byte 3 is the status - # and leaving it would hand libogc whatever the previous - # frame -- or power-on -- left in that SPRAM cell, and any - # frame landing on a stale 0x80 or 0x04 would be discarded as - # an error. The simulation model reports it as an unwritten - # cell; real SPRAM has no such tell. + # ⚠️ This MUST be written explicitly and cannot lean on the + # placeholder, because WRITE_PLACEHOLDER never reaches + # desc_base+3 (see its note). Leaving it would hand the + # driver whatever the previous frame -- or power-on -- left in + # that SPRAM cell, and a stale 0x80 or 0x04 discards the frame + # as an error. Simulation reports an unwritten cell; real + # SPRAM has no such tell. m.d.sync += self.eth_wr_addr.eq(desc_base + 3) m.d.sync += self.eth_wr_data.eq(0x00) m.d.sync += self.eth_wr_valid.eq(1) @@ -436,27 +426,29 @@ class RXFrameAssembler(Elaboratable): m.d.sync += self.rx_irq.eq(1) m.next = "IDLE" - # ── NCRA reset: the write pointer goes back to the ring start ────── - # Placed AFTER the FSM so it takes priority over ADVANCE_RWP in the - # same cycle. - # - # This MUST happen here as well as in the register file: they are two - # copies of the same pointer, and resetting only one leaves them - # disagreeing about where the ring is. A driver that then syncs RRP to - # the register file's value points somewhere the writer is not, the - # free-space calculation shrinks with every frame, and once it drops - # below _MAX_FRAME_PAGES EVERY frame is dropped -- silently, because a - # dropped frame never reaches the ring to be counted. Hardware - # symptom: "timeout: no match (0 seen, 0 bad desc, 0 lapped)". - # - # No in-flight frame needs abandoning: the same NCRA write that pulses - # this also clears the SR bit, so rx_enabled is low and the FSM is in - # IDLE. + def _ncra_reset(self, m, rwp, pg): + """Return the write pointer to the ring start on the GC's NCRA reset. + + Must be elaborated AFTER the FSM so that it takes priority over + ADVANCE_RWP in the same cycle. + + This has to happen here as well as in the register file: the two are + copies of the same pointer, and resetting only one leaves them + disagreeing about where the ring is. A driver that then syncs RRP to + the register file's value points somewhere the writer is not, the + free-space sum shrinks with every frame, and once it falls below + _MAX_FRAME_PAGES EVERY frame is dropped -- silently, since a dropped + frame never reaches the ring to be counted. The hardware symptom was + "0 seen, 0 bad desc, 0 lapped". + + No in-flight frame needs abandoning: the same NCRA write that pulses + this also clears the SR bit, so `rx_enabled` is low and the FSM is + already in IDLE. + """ + pg_first, _pg_last, _pg_total = pg with m.If(self.ncra_rst): m.d.sync += rwp.eq(pg_first) - return m - # ── Testbench ───────────────────────────────────────────────────────────── @@ -490,28 +482,23 @@ if __name__ == "__main__": errors.append(name) async def testbench(ctx): - # Final contents of every SPRAM address the DUT wrote. - # - # The old testbench declared a writes list and never filled it, and - # drove the RX stream OPEN-LOOP -- one byte per tick, never checking - # rx_ready. Both of the 2026-10-09 handshake bugs were therefore - # invisible to it: it only ever asserted on rwp and rx_irq, so a - # dropped descriptor byte or a frame body written one byte early - # changed nothing it looked at. + # Final contents of every SPRAM address the DUT wrote, plus the order + # the writes were issued in. mem = {} order = [] # rx_irq and rx_overflow are 1-CYCLE pulses, and rx_overflow fires on # the very tick that consumes the EOF byte -- i.e. inside send_frame. - # Polling for it afterwards misses it entirely, which first showed up - # as a dropped frame looking like a stall. Latch both in tick(). + # Polling for it afterwards misses it entirely, which showed up as a + # dropped frame looking like a stall, so tick() latches both. pulse = {"irq": None, "ovf": False} - # eth_wr_ready stall pattern. Held constantly HIGH by every test this - # file ever had, which is a serious blind spot: on hardware the SPRAM - # arbiter stalls the write port constantly, because the GC is reading - # the ring over EXI while frames are being written into it. Any + + # eth_wr_ready stall pattern: assert ready 1 cycle in stall[0]. + # Holding it constantly high is a serious blind spot, because on + # hardware the SPRAM arbiter stalls this port constantly -- the GC + # reads the ring over EXI while frames are being written into it. Any # address bookkeeping that depends on ready timing is invisible until - # ready actually goes low. stall[0] = assert ready 1 cycle in N. + # ready actually goes low. stall = [1] cyc = [0] @@ -545,24 +532,22 @@ if __name__ == "__main__": tag = ["?"] async def send_frame(frame): - pulse["irq"] = None - pulse["ovf"] = False """Push a frame RESPECTING BACK-PRESSURE, the way the real ethernet master does. rx_ready is LOW through IDLE and the whole - placeholder-descriptor write, so a master that advances anyway - simply loses those bytes -- which is what the old testbench did, - and why it could not see the frame body land at the wrong - offset.""" + placeholder write, so a master that advances anyway simply loses + those bytes -- which is how a frame body landing at the wrong + offset went unnoticed.""" + pulse["irq"] = None + pulse["ovf"] = False for i, b in enumerate(frame): ctx.set(dut.rx_data, b) ctx.set(dut.rx_valid, 1) ctx.set(dut.rx_sof, 1 if i == 0 else 0) ctx.set(dut.rx_eof, 1 if i == len(frame) - 1 else 0) - # Apply ready, THEN read rx_ready, THEN clock -- so the - # value the DUT decides on is the one this loop checked. - # Polling before applying had the loop waiting on a byte the - # DUT had already accepted: the frame stored correctly while - # send_frame reported rx_ready never asserted. + # Apply ready, THEN read rx_ready, THEN clock -- so the value + # the DUT decides on is the one this loop checked. Polling + # before applying left the loop waiting on a byte the DUT had + # already accepted. guard = 0 while True: apply_ready() @@ -603,7 +588,7 @@ if __name__ == "__main__": return [mem.get(base + i) for i in range(4)] def pack_desc(total, next_ptr, status=0): - """The 4 descriptor bytes libogc's driver expects.""" + """The 4 descriptor bytes the GC's driver expects.""" word = (status << 24) | ((total & 0xFFF) << 12) | (next_ptr & 0xFFF) return [(word >> (8 * i)) & 0xFF for i in range(4)] @@ -611,9 +596,9 @@ if __name__ == "__main__": """Decode exactly as libogc's bba_interrupt does, so these tests check the driver's ALGORITHM rather than our own packing: - lwbrx -> little-endian word from the 4 bytes - rlwinm r4,r9,20,20,31 -> packet_len = (word >> 12) & 0xFFF - srwi r9,r9,24 -> status = word >> 24 + lwbrx -> little-endian word from the 4 bytes + rlwinm r4,r9,20,20,31 -> packet_len = (word >> 12) & 0xFFF + srwi r9,r9,24 -> status = word >> 24 word & 0xFFF -> next_packet_ptr Returns (payload_len, next_ptr, status_is_error).""" @@ -641,19 +626,15 @@ if __name__ == "__main__": check("T1 descriptor via libogc decode", libogc_decode(descriptor(1)), (len(f1), 2, False)) # The body MUST start at base+4. This is the check that catches a - # placeholder write being dropped: with only three placeholder bytes - # written the body landed at base+3 and every frame was shifted one - # byte early, which on hardware made the GC drop every frame. + # dropped placeholder write: with only three written, the body landed + # at base+3 and the GC dropped every frame. check("T1 body at base+4", body(1, len(f1)), f1) await tick(4) - # ── T2: frame LONGER THAN 255 BYTES, so the descriptor's HIGH length - # byte is non-zero. This is the case that catches a dropped - # WRITE_LEN_HI: with the high byte left at its placeholder 0x00 the GC - # read an impossible length (hardware saw 00 00 00 01) and rejected - # the ring. Every earlier test used a short frame, where a dropped - # high byte is indistinguishable from a correct zero. ─────────────── + # ── T2: frame LONGER THAN 255 BYTES, so the descriptor's high length + # nibble is non-zero. A short frame cannot distinguish a dropped + # high-byte write from a correct zero. ────────────────────────────── f2 = [(i * 7 + 3) & 0xFF for i in range(300)] tag[0] = "T2" await send_frame(f2) @@ -671,11 +652,8 @@ if __name__ == "__main__": # ── T4: ring overflow must DROP, not overwrite ─────────────────── # rrp is held still, as if the GC had stopped reading. The writer # must refuse new frames once the ring is nearly full and leave the - # unread ones intact. Before this existed the writer simply lapped - # the reader, so a traffic burst corrupted whatever frame the driver - # was midway through -- with no indication it had happened. That is - # what produced a "descriptor" of 00 00 00 01 on hardware: frame data - # from the frame that had replaced the one being read. + # unread ones intact; lapping the reader corrupts whatever frame the + # driver is midway through, with nothing reporting it. ctx.set(dut.rrp, 4) # GC has consumed up to page 4 await tick(2) f3 = [(i * 3 + 1) & 0xFF for i in range(300)] # 2 pages each @@ -710,10 +688,9 @@ if __name__ == "__main__": # ── T5: the same frame WITH THE WRITE PORT STALLING ────────────── # Models arbiter contention: ready asserted only 1 cycle in 3. The - # descriptor and body offset must be identical to the unstalled case. - # This is the case hardware runs in and no previous test ever did -- - # hardware reported a frame whose body was one byte late while - # simulation with ready held high was byte-perfect. + # descriptor and body offsets must match the unstalled case exactly. + # This is the condition hardware runs in, and holding ready high hid a + # frame body landing one byte late. ctx.set(dut.rrp, 1) await tick(2) stall[0] = 3 @@ -734,11 +711,9 @@ if __name__ == "__main__": await tick(4) # ── T6: NCRA reset must return the WRITE pointer to page 1 ─────── - # The register file resets its own copy of RWP on an NCRA reset; if - # this module does not reset its own, the two disagree and the GC ends - # up pointing somewhere the writer is not. The free-space sum then - # shrinks with every frame until every frame is dropped -- silently, - # which on hardware read as "0 seen, 0 bad desc, 0 lapped". + # If this module keeps its own RWP while the register file resets its + # copy, the two disagree, the free-space sum shrinks with every frame, + # and eventually every frame is dropped silently. ctx.set(dut.rrp, 1) await tick(2) ctx.set(dut.ncra_rst, 1) @@ -767,11 +742,10 @@ if __name__ == "__main__": ctx.set(dut.rx_sof, 0) check("T3 no writes while rx disabled", len(mem), before) - # ── T7: the GC's BP/RHBP are HONOURED (compat TODO item 2) ─────── - # These were hardcoded to pages 1-15, so a driver configuring a - # SMALLER ring was ignored and we kept writing pages it would never - # read -- silent corruption. libogc2 writes BP=1/RHBP=15, exactly the - # old hardcode, which is why it never showed up. + # ── T7: the GC's BP/RHBP are HONOURED ──────────────────────────── + # A driver configuring a SMALLER ring must not be ignored, or we keep + # writing pages it will never read. libogc2 happens to write + # BP=1/RHBP=15, so only a different geometry exercises this. ctx.set(dut.rx_enabled, 1) ctx.set(dut.rrp, 1) ctx.set(dut.bp, 1) @@ -814,7 +788,7 @@ if __name__ == "__main__": check(f"T7b reject {why}", "honoured", "rejected") check("T7b garbage geometry rejected", ctx.get(dut.geom_honoured), 0) - # ... and with it rejected, the hardcoded layout still works. + # ... and with it rejected, the default layout still works. ctx.set(dut.rrp, 1) await tick(4) mem.clear()