rx_frame_assembler: document the ring and descriptor, drop stale references

844 -> 818 lines, 30% -> 12% comment-only.  The descriptor bit layout is kept
in FULL, because it is the one interface in this module an external driver
parses; what went was the disassembly listing that proves it (CLAUDE.md holds
that) and the per-bug narrative around each fix.

Stale references fixed:

  * The module claimed to receive frames "from W5500SPIMaster".  The W5100
    parallel master is the default back-end; the stream interface is
    deliberately identical, so the text now names neither.
  * "RWP FIFO write-side (sync->exi)" and "rx_irq : 1-cycle pulse ->
    PulseSynchronizer input" both described the old split clock domains.  The
    exi domain was merged into sync; that FIFO is a SyncFIFO and the pulse is
    a plain wire.
  * The module docstring's write sequence said descriptor "bytes 0, 1 and 2"
    are rewritten and "byte 3 (status) stays 0x00", and WRITE_D2's comment
    said byte 3 "keeps the placeholder".  Both predate WRITE_D3, which exists
    precisely because the placeholder NEVER reaches desc_base+3 -- so the
    file contradicted its own next state, and in the direction that sounds
    reassuring.

Two comment blocks were duplicated outright: the registered-ring-bounds
timing note appeared twice in a row with slightly different wording, and the
NCRA-reset rationale appeared both on the port and again at the logic.

Structure: elaborate() is now _ring_bounds, _free_space, _frame_fsm and
_ncra_reset, with _ncra_reset still elaborated last so it keeps priority over
ADVANCE_RWP.  The FSM's docstring lists all eleven states in visit order, and
the valid-&-ready rule that both 2026-10-09 handshake bugs violated is stated
once for every write state instead of twice at two of them.

Also fixed a real (if harmless) bug in the testbench: send_frame's docstring
sat after two statements, making it a no-op expression rather than a
docstring.

VERIFIED EQUIVALENT: generated RTLIL identical, 1930 lines, after stripping
\src attributes.  Full module suite passes, bba_top T9 6/6 at EXI_HALF=1, 2
and 8, and this module's own 25 checks all pass including T5's stalled-write
case and T7's small-ring wrap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-10-10 20:02:34 +02:00
parent 41ba716759
commit 20d64ac416
+281 -307
View File
@@ -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 Runs entirely in the `sync` domain (24 MHz). Accepts a byte stream from
ring buffer in MX98730EC format. 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) Ring layout
------------------------------------------ -----------
0x0100–0x0FFF 15 pages × 256 bytes = 3840 bytes SPRAM bytes 0x0100-0x0FFF, as 15 pages of 256 bytes. Pages 0x01-0x0F are
Pages 0x01–0x0F; page 0x00 is reserved. usable and page 0x00 is reserved for TX, so the wrap is 0x0F → 0x01. The GC
Page wrap: after 0x0F → 0x01 (skip 0x00). can narrow the ring through its BP/RHBP registers; see `_ring_bounds`.
Frame descriptor (4 bytes at page start) 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
A PACKED 32-BIT LITTLE-ENDIAN WORD, not a byte-wise layout: 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 Frame descriptor
bits 12–23 : packet_len — total bytes, INCLUDING these 4 ----------------
bits 24–31 : status — errors are 0x80 and 0x04; 0x00 means good 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] which puts on the wire:
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
⚠️ This is NOT a guess and must not be "simplified" back to a plain byte 0 next_packet_ptr[7:0]
big-endian length. It was read out of libogc's own BBA driver byte 1 packet_len[3:0] << 4 | next_packet_ptr[11:8]
(`/opt/devkitpro/libogc/lib/cube/libbba.a`, `gcif.o`, `bba_interrupt`), which byte 2 packet_len[11:4]
is the driver GC software actually runs: byte 3 status
byte 4+ the raw ethernet frame
lwbrx r9,0,r9 # load the 4 bytes as a LITTLE-ENDIAN word The driver takes the low 12 bits of that word and writes them straight to its
rlwinm r4,r9,20,20,31 # packet_len = (word >> 12) & 0xFFF RRP register, so `next_packet_ptr` is load-bearing and not informational. The
addi r4,r4,-4 # payload = packet_len - 4 layout was read out of libogc's own compiled driver rather than inferred; the
cmpwi r4,1540 # ... which must be <= 1540 disassembly is recorded in CLAUDE.md.
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.
Write sequence Write sequence
-------------- --------------
1. Issue 4 SPRAM writes of 0x00 (placeholder descriptor). 1. Write four placeholder 0x00 bytes where the descriptor will go.
2. For each byte received from W5500, issue one SPRAM write. 2. Write each received byte straight through, one SPRAM write per byte.
3. After EOF: compute the next page, then rewrite descriptor bytes 0, 1 and 2 3. On EOF, compute the next page, then write descriptor bytes 0-3 for real.
with the packed next_packet_ptr + packet_len. Byte 3 (status) stays 0x00. 4. Advance RWP, push it to the register file, and pulse `rx_irq`.
4. Advance RWP, push to rx_wptr FIFO, 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 * from amaranth import *
@@ -72,24 +63,47 @@ _MAX_FRAME_PAGES = 6
class RXFrameAssembler(Elaboratable): class RXFrameAssembler(Elaboratable):
"""Writes incoming ethernet frames into the SPRAM ring buffer. """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 eth_wr_addr, eth_wr_data, eth_wr_valid, eth_wr_ready
rx_sof / rx_eof : frame delimiters (same cycle as rx_valid)
SPRAM write interface (to SPRAMArbiter, sync domain) To the register file
----------------------------------------------------- -------------------
eth_wr_addr / eth_wr_data / eth_wr_valid / eth_wr_ready 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) From the register file
----------------------------- ---------------------
rx_wptr_w_data / rx_wptr_w_en / rx_wptr_w_rdy rx_enabled
rx_irq : 1-cycle pulse → PulseSynchronizer input NCRA's SR bit. Frames are ignored entirely while it is low.
rx_enabled : controlled by NCRA SR bit (from BBARegisterFile) 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): def __init__(self):
# W5500 stream in # Ethernet stream in
self.rx_data = Signal(8) self.rx_data = Signal(8)
self.rx_valid = Signal() self.rx_valid = Signal()
self.rx_ready = Signal() self.rx_ready = Signal()
@@ -102,112 +116,75 @@ class RXFrameAssembler(Elaboratable):
self.eth_wr_valid = Signal() self.eth_wr_valid = Signal()
self.eth_wr_ready = 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_data = Signal(8)
self.rx_wptr_w_en = Signal() self.rx_wptr_w_en = Signal()
self.rx_wptr_w_rdy = 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() self.rx_enabled = Signal()
self.rrp = Signal(8, init=_RX_PAGE_FIRST)
# ── 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.bp = Signal(12, init=_RX_PAGE_FIRST) # first RX page self.bp = Signal(12, init=_RX_PAGE_FIRST) # first RX page
self.rhbp = Signal(12, init=_RX_PAGE_LAST) # last 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() 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() self.ncra_rst = Signal()
def elaborate(self, platform): def elaborate(self, platform):
m = Module() m = Module()
# ── Ring-buffer state ───────────────────────────────────────────── # The current RX write page, shared by the free-space calculation, the
rwp = Signal(8, init=_RX_PAGE_FIRST) # current RX write page (1–15) # frame FSM and the NCRA reset.
rwp = Signal(8, init=_RX_PAGE_FIRST)
# Write address within current frame # All three outputs are one-cycle pulses, so they default low and the
wr_addr = Signal(16) # FSM below overrides them for the cycle it needs.
# Number of frame data bytes received m.d.sync += [self.rx_irq.eq(0),
data_ctr = Signal(12) self.rx_wptr_w_en.eq(0),
# Total length = data_ctr + 4 self.rx_overflow.eq(0)]
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)
# Descriptor base (rwp*256) — saved when frame starts pg = self._ring_bounds(m)
desc_base = Signal(16) 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) def _ring_bounds(self, m):
desc_ctr = Signal(2) """Sanitise the GC's requested ring geometry.
# Number of pages consumed by this frame (rounded up) Returns a `(first, last, total)` tuple of signals describing the ring
pages_used = Signal(5) actually in use.
# Default: no pulses `bp`/`rhbp` are honoured only when they describe a usable ring inside
m.d.sync += self.rx_irq.eq(0) SPRAM; otherwise the default 1-15 layout stands and `geom_honoured`
m.d.sync += self.rx_wptr_w_en.eq(0) goes low so the disagreement is observable. Page 0 holds the TX
m.d.sync += self.rx_overflow.eq(0) 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 ───────────────────────────────────────── ⚠️ These are REGISTERED and 4 BITS WIDE, both deliberately, and both
# A driver's BP/RHBP are used only when they describe a usable ring for timing rather than function.
# inside SPRAM; otherwise the hardcoded layout stands. Without this a
# garbage write (BP = 0, or RHBP below BP) would give a zero or Computing them combinationally put bp/rhbp → comparators → Mux →
# negative ring size and the assembler would write outside it. Page 0 subtract → total → add → compare → free → compare → have_room in ONE
# is reserved for the TX buffer, so BP must be at least 1. path, where the constants it replaced had been folded away. `clk`
# ⚠️ REGISTERED, not combinational. Computing these every cycle put fmax collapsed from ~29 MHz to ~17 MHz and ALL EIGHT SEEDS FAILED the
# bp/rhbp -> comparators -> Mux -> subtract -> pg_total -> add -> raw 24 MHz target. Registering split the path, but 8-bit variable-vs-
# -> compare -> free -> compare -> have_room all in ONE combinational variable comparators are carry chains where variable-vs-constant folds
# path, where it used to be constant-folded. `clk` fmax collapsed into a couple of LUTs, so it was still only ~20 MHz.
# 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 Ring pages are 1-15, so 4 bits is the natural width, and the
# cycle of staleness costs nothing and splits the path in two. `geom_ok` test guarantees bp < rhbp <= 0x0F -- which is what makes the
# ⚠️ REGISTERED AND 4 BITS WIDE, both deliberately. narrow slice safe. `bp`/`rhbp` only change when the GC writes BP or
# RHBP, so a cycle of staleness costs nothing.
# 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.
geom_ok = Signal() geom_ok = Signal()
geom_ok_c = Signal() geom_ok_c = Signal()
pg_first = Signal(4, init=_RX_PAGE_FIRST) pg_first = Signal(4, init=_RX_PAGE_FIRST)
pg_last = Signal(4, init=_RX_PAGE_LAST) pg_last = Signal(4, init=_RX_PAGE_LAST)
pg_total = Signal(5, init=_PAGES_TOTAL) pg_total = Signal(5, init=_PAGES_TOTAL)
m.d.comb += geom_ok_c.eq((self.bp >= _RX_PAGE_FIRST) m.d.comb += geom_ok_c.eq((self.bp >= _RX_PAGE_FIRST)
& (self.rhbp <= _RX_PAGE_LAST) & (self.rhbp <= _RX_PAGE_LAST)
& (self.rhbp > self.bp)) & (self.rhbp > self.bp))
@@ -219,19 +196,35 @@ class RXFrameAssembler(Elaboratable):
_PAGES_TOTAL)), _PAGES_TOTAL)),
] ]
m.d.comb += self.geom_honoured.eq(geom_ok) 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) rrp_s = Signal(4, init=_RX_PAGE_FIRST)
m.d.sync += rrp_s.eq(Mux((self.rrp >= pg_first) m.d.sync += rrp_s.eq(Mux((self.rrp >= pg_first)
& (self.rrp <= pg_last), & (self.rrp <= pg_last),
self.rrp[0:4], pg_first)) self.rrp[0:4], pg_first))
raw = Signal(5) raw = Signal(5)
free = Signal(5) free = Signal(5)
with m.If(rrp_s >= rwp): 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 += raw.eq(rrp_s + pg_total - rwp)
m.d.comb += free.eq(Mux(raw == 0, pg_total - 1, raw - 1)) 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() have_room = Signal()
m.d.sync += have_room.eq(free >= _MAX_FRAME_PAGES) 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) m.d.comb += total_len.eq(data_ctr + 4)
with m.FSM(domain="sync", name="rx_fsm"): with m.FSM(domain="sync", name="rx_fsm"):
@@ -264,9 +295,6 @@ class RXFrameAssembler(Elaboratable):
m.d.sync += self.eth_wr_valid.eq(0) m.d.sync += self.eth_wr_valid.eq(0)
with m.If(self.rx_valid & self.rx_sof & self.rx_enabled with m.If(self.rx_valid & self.rx_sof & self.rx_enabled
& ~have_room): & ~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" m.next = "DROP_FRAME"
with m.Elif(self.rx_valid & self.rx_sof & self.rx_enabled): with m.Elif(self.rx_valid & self.rx_sof & self.rx_enabled):
frame_base = Signal(16) frame_base = Signal(16)
@@ -278,40 +306,23 @@ class RXFrameAssembler(Elaboratable):
m.next = "WRITE_PLACEHOLDER" m.next = "WRITE_PLACEHOLDER"
with m.State("DROP_FRAME"): with m.State("DROP_FRAME"):
# Consume and discard to EOF. rx_ready MUST stay high here: # Discard to EOF rather than overwrite data the GC has not
# stalling the ethernet master instead would back pressure into # read. This is what the real BBA reports through RBFI.
# the W5100's own buffer and turn a dropped frame into a wedged #
# receiver. # 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) m.d.comb += self.rx_ready.eq(1)
with m.If(self.rx_valid & self.rx_eof): with m.If(self.rx_valid & self.rx_eof):
m.d.sync += self.rx_overflow.eq(1) m.d.sync += self.rx_overflow.eq(1)
m.next = "IDLE" m.next = "IDLE"
with m.State("WRITE_PLACEHOLDER"): with m.State("WRITE_PLACEHOLDER"):
# Write 4 bytes of 0x00 as placeholder descriptor. # The first byte is written twice, because eth_wr_addr trails
# # wr_addr by a cycle, and desc_base+3 is never reached. Both
# ⚠️ Advance on `valid & ready`, NOT on `ready` alone. # are harmless -- every descriptor byte is overwritten by
# `eth_wr_valid` is scheduled with m.d.sync and so is LOW for # WRITE_D0..D3 later. What matters is that wr_addr ends at
# the whole first cycle in this state, while `ready` is tested # desc_base+4, so the frame body starts there.
# 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.
m.d.sync += self.eth_wr_addr .eq(wr_addr) 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_data .eq(0x00)
m.d.sync += self.eth_wr_valid.eq(1) 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) m.d.sync += desc_ctr.eq(desc_ctr + 1)
with m.State("RECV_AND_WRITE"): 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) 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)): with m.If(self.rx_valid & (~self.eth_wr_valid | self.eth_wr_ready)):
m.d.sync += self.eth_wr_addr.eq(wr_addr) 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) m.d.sync += self.eth_wr_valid.eq(0)
with m.State("WAIT_LAST_WRITE"): 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): with m.If(~self.eth_wr_valid | self.eth_wr_ready):
m.d.sync += self.eth_wr_valid.eq(0) m.d.sync += self.eth_wr_valid.eq(0)
# Compute pages used: ceil((data_ctr + 4) / 256) # pages = ceil(total_len / 256)
# = (total_len + 255) >> 8 = total_len[11:8] + (total_len[7:0] != 0)
m.d.sync += pages_used.eq(total_len[8:12] + (total_len[:8] != 0)) m.d.sync += pages_used.eq(total_len[8:12] + (total_len[:8] != 0))
m.next = "CALC_NEXT" 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"): with m.State("CALC_NEXT"):
# The descriptor carries next_packet_ptr, so the next page must # The descriptor carries next_packet_ptr, so the next page has
# be known BEFORE the descriptor is written -- this state used # to be known BEFORE the descriptor is written.
# to come after it (as ADVANCE_RWP).
# next_rwp = ((rwp - 1 + pages_used) % 15) + 1
next_rwp_raw = Signal(8) next_rwp_raw = Signal(8)
m.d.comb += next_rwp_raw.eq(rwp + pages_used) m.d.comb += next_rwp_raw.eq(rwp + pages_used)
with m.If(next_rwp_raw > pg_last): with m.If(next_rwp_raw > pg_last):
@@ -375,7 +369,7 @@ class RXFrameAssembler(Elaboratable):
m.next = "WRITE_D0" m.next = "WRITE_D0"
with m.State("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_addr.eq(desc_base + 0)
m.d.sync += self.eth_wr_data.eq(next_page[0:8]) m.d.sync += self.eth_wr_data.eq(next_page[0:8])
m.d.sync += self.eth_wr_valid.eq(1) m.d.sync += self.eth_wr_valid.eq(1)
@@ -384,7 +378,7 @@ class RXFrameAssembler(Elaboratable):
m.next = "WRITE_D1" m.next = "WRITE_D1"
with m.State("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. # 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_addr.eq(desc_base + 1)
m.d.sync += self.eth_wr_data.eq( m.d.sync += self.eth_wr_data.eq(
@@ -395,8 +389,7 @@ class RXFrameAssembler(Elaboratable):
m.next = "WRITE_D2" m.next = "WRITE_D2"
with m.State("WRITE_D2"): with m.State("WRITE_D2"):
# Descriptor byte 2 = packet_len[11:4]. Byte 3 (status) keeps # packet_len[11:4]
# the placeholder 0x00, which libogc reads as "no error".
m.d.sync += self.eth_wr_addr.eq(desc_base + 2) 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_data.eq(total_len[4:12])
m.d.sync += self.eth_wr_valid.eq(1) m.d.sync += self.eth_wr_valid.eq(1)
@@ -405,19 +398,16 @@ class RXFrameAssembler(Elaboratable):
m.next = "WRITE_D3" m.next = "WRITE_D3"
with m.State("WRITE_D3"): with m.State("WRITE_D3"):
# Descriptor byte 3 = status = 0x00 ("no error": libogc tests # status = 0x00, "no error": the driver tests this byte
# it against 0x84). # against 0x84.
# #
# ⚠️ This MUST be written explicitly and cannot rely on the # ⚠️ This MUST be written explicitly and cannot lean on the
# placeholder. WRITE_PLACEHOLDER issues four writes but the # placeholder, because WRITE_PLACEHOLDER never reaches
# first lands twice (eth_wr_addr trails wr_addr by a cycle), # desc_base+3 (see its note). Leaving it would hand the
# so desc_base+3 is NEVER written. That was harmless while # driver whatever the previous frame -- or power-on -- left in
# the length write covered byte 3; now byte 3 is the status # that SPRAM cell, and a stale 0x80 or 0x04 discards the frame
# and leaving it would hand libogc whatever the previous # as an error. Simulation reports an unwritten cell; real
# frame -- or power-on -- left in that SPRAM cell, and any # SPRAM has no such tell.
# 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.
m.d.sync += self.eth_wr_addr.eq(desc_base + 3) 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_data.eq(0x00)
m.d.sync += self.eth_wr_valid.eq(1) 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.d.sync += self.rx_irq.eq(1)
m.next = "IDLE" m.next = "IDLE"
# ── NCRA reset: the write pointer goes back to the ring start ────── def _ncra_reset(self, m, rwp, pg):
# Placed AFTER the FSM so it takes priority over ADVANCE_RWP in the """Return the write pointer to the ring start on the GC's NCRA reset.
# same cycle.
# Must be elaborated AFTER the FSM so that it takes priority over
# This MUST happen here as well as in the register file: they are two ADVANCE_RWP in the same cycle.
# copies of the same pointer, and resetting only one leaves them
# disagreeing about where the ring is. A driver that then syncs RRP to This has to happen here as well as in the register file: the two are
# the register file's value points somewhere the writer is not, the copies of the same pointer, and resetting only one leaves them
# free-space calculation shrinks with every frame, and once it drops disagreeing about where the ring is. A driver that then syncs RRP to
# below _MAX_FRAME_PAGES EVERY frame is dropped -- silently, because a the register file's value points somewhere the writer is not, the
# dropped frame never reaches the ring to be counted. Hardware free-space sum shrinks with every frame, and once it falls below
# symptom: "timeout: no match (0 seen, 0 bad desc, 0 lapped)". _MAX_FRAME_PAGES EVERY frame is dropped -- silently, since a dropped
# frame never reaches the ring to be counted. The hardware symptom was
# No in-flight frame needs abandoning: the same NCRA write that pulses "0 seen, 0 bad desc, 0 lapped".
# this also clears the SR bit, so rx_enabled is low and the FSM is in
# IDLE. 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): with m.If(self.ncra_rst):
m.d.sync += rwp.eq(pg_first) m.d.sync += rwp.eq(pg_first)
return m
# ── Testbench ───────────────────────────────────────────────────────────── # ── Testbench ─────────────────────────────────────────────────────────────
@@ -490,28 +482,23 @@ if __name__ == "__main__":
errors.append(name) errors.append(name)
async def testbench(ctx): async def testbench(ctx):
# Final contents of every SPRAM address the DUT wrote. # Final contents of every SPRAM address the DUT wrote, plus the order
# # the writes were issued in.
# 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.
mem = {} mem = {}
order = [] order = []
# rx_irq and rx_overflow are 1-CYCLE pulses, and rx_overflow fires on # 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. # the very tick that consumes the EOF byte -- i.e. inside send_frame.
# Polling for it afterwards misses it entirely, which first showed up # Polling for it afterwards misses it entirely, which showed up as a
# as a dropped frame looking like a stall. Latch both in tick(). # dropped frame looking like a stall, so tick() latches both.
pulse = {"irq": None, "ovf": False} 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 # eth_wr_ready stall pattern: assert ready 1 cycle in stall[0].
# arbiter stalls the write port constantly, because the GC is reading # Holding it constantly high is a serious blind spot, because on
# the ring over EXI while frames are being written into it. Any # 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 # 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] stall = [1]
cyc = [0] cyc = [0]
@@ -545,24 +532,22 @@ if __name__ == "__main__":
tag = ["?"] tag = ["?"]
async def send_frame(frame): async def send_frame(frame):
pulse["irq"] = None
pulse["ovf"] = False
"""Push a frame RESPECTING BACK-PRESSURE, the way the real """Push a frame RESPECTING BACK-PRESSURE, the way the real
ethernet master does. rx_ready is LOW through IDLE and the whole ethernet master does. rx_ready is LOW through IDLE and the whole
placeholder-descriptor write, so a master that advances anyway placeholder write, so a master that advances anyway simply loses
simply loses those bytes -- which is what the old testbench did, those bytes -- which is how a frame body landing at the wrong
and why it could not see the frame body land at the wrong offset went unnoticed."""
offset.""" pulse["irq"] = None
pulse["ovf"] = False
for i, b in enumerate(frame): for i, b in enumerate(frame):
ctx.set(dut.rx_data, b) ctx.set(dut.rx_data, b)
ctx.set(dut.rx_valid, 1) ctx.set(dut.rx_valid, 1)
ctx.set(dut.rx_sof, 1 if i == 0 else 0) ctx.set(dut.rx_sof, 1 if i == 0 else 0)
ctx.set(dut.rx_eof, 1 if i == len(frame) - 1 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 # Apply ready, THEN read rx_ready, THEN clock -- so the value
# value the DUT decides on is the one this loop checked. # the DUT decides on is the one this loop checked. Polling
# Polling before applying had the loop waiting on a byte the # before applying left the loop waiting on a byte the DUT had
# DUT had already accepted: the frame stored correctly while # already accepted.
# send_frame reported rx_ready never asserted.
guard = 0 guard = 0
while True: while True:
apply_ready() apply_ready()
@@ -603,7 +588,7 @@ if __name__ == "__main__":
return [mem.get(base + i) for i in range(4)] return [mem.get(base + i) for i in range(4)]
def pack_desc(total, next_ptr, status=0): 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) word = (status << 24) | ((total & 0xFFF) << 12) | (next_ptr & 0xFFF)
return [(word >> (8 * i)) & 0xFF for i in range(4)] 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 """Decode exactly as libogc's bba_interrupt does, so these tests
check the driver's ALGORITHM rather than our own packing: check the driver's ALGORITHM rather than our own packing:
lwbrx -> little-endian word from the 4 bytes lwbrx -> little-endian word from the 4 bytes
rlwinm r4,r9,20,20,31 -> packet_len = (word >> 12) & 0xFFF rlwinm r4,r9,20,20,31 -> packet_len = (word >> 12) & 0xFFF
srwi r9,r9,24 -> status = word >> 24 srwi r9,r9,24 -> status = word >> 24
word & 0xFFF -> next_packet_ptr word & 0xFFF -> next_packet_ptr
Returns (payload_len, next_ptr, status_is_error).""" 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)), check("T1 descriptor via libogc decode", libogc_decode(descriptor(1)),
(len(f1), 2, False)) (len(f1), 2, False))
# The body MUST start at base+4. This is the check that catches a # The body MUST start at base+4. This is the check that catches a
# placeholder write being dropped: with only three placeholder bytes # dropped placeholder write: with only three written, the body landed
# written the body landed at base+3 and every frame was shifted one # at base+3 and the GC dropped every frame.
# byte early, which on hardware made the GC drop every frame.
check("T1 body at base+4", body(1, len(f1)), f1) check("T1 body at base+4", body(1, len(f1)), f1)
await tick(4) await tick(4)
# ── T2: frame LONGER THAN 255 BYTES, so the descriptor's HIGH length # ── T2: frame LONGER THAN 255 BYTES, so the descriptor's high length
# byte is non-zero. This is the case that catches a dropped # nibble is non-zero. A short frame cannot distinguish a dropped
# WRITE_LEN_HI: with the high byte left at its placeholder 0x00 the GC # high-byte write from a correct zero. ──────────────────────────────
# 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. ───────────────
f2 = [(i * 7 + 3) & 0xFF for i in range(300)] f2 = [(i * 7 + 3) & 0xFF for i in range(300)]
tag[0] = "T2" tag[0] = "T2"
await send_frame(f2) await send_frame(f2)
@@ -671,11 +652,8 @@ if __name__ == "__main__":
# ── T4: ring overflow must DROP, not overwrite ─────────────────── # ── T4: ring overflow must DROP, not overwrite ───────────────────
# rrp is held still, as if the GC had stopped reading. The writer # 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 # must refuse new frames once the ring is nearly full and leave the
# unread ones intact. Before this existed the writer simply lapped # unread ones intact; lapping the reader corrupts whatever frame the
# the reader, so a traffic burst corrupted whatever frame the driver # driver is midway through, with nothing reporting it.
# 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.
ctx.set(dut.rrp, 4) # GC has consumed up to page 4 ctx.set(dut.rrp, 4) # GC has consumed up to page 4
await tick(2) await tick(2)
f3 = [(i * 3 + 1) & 0xFF for i in range(300)] # 2 pages each 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 ────────────── # ── T5: the same frame WITH THE WRITE PORT STALLING ──────────────
# Models arbiter contention: ready asserted only 1 cycle in 3. The # Models arbiter contention: ready asserted only 1 cycle in 3. The
# descriptor and body offset must be identical to the unstalled case. # descriptor and body offsets must match the unstalled case exactly.
# This is the case hardware runs in and no previous test ever did -- # This is the condition hardware runs in, and holding ready high hid a
# hardware reported a frame whose body was one byte late while # frame body landing one byte late.
# simulation with ready held high was byte-perfect.
ctx.set(dut.rrp, 1) ctx.set(dut.rrp, 1)
await tick(2) await tick(2)
stall[0] = 3 stall[0] = 3
@@ -734,11 +711,9 @@ if __name__ == "__main__":
await tick(4) await tick(4)
# ── T6: NCRA reset must return the WRITE pointer to page 1 ─────── # ── 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 # If this module keeps its own RWP while the register file resets its
# this module does not reset its own, the two disagree and the GC ends # copy, the two disagree, the free-space sum shrinks with every frame,
# up pointing somewhere the writer is not. The free-space sum then # and eventually every frame is dropped silently.
# shrinks with every frame until every frame is dropped -- silently,
# which on hardware read as "0 seen, 0 bad desc, 0 lapped".
ctx.set(dut.rrp, 1) ctx.set(dut.rrp, 1)
await tick(2) await tick(2)
ctx.set(dut.ncra_rst, 1) ctx.set(dut.ncra_rst, 1)
@@ -767,11 +742,10 @@ if __name__ == "__main__":
ctx.set(dut.rx_sof, 0) ctx.set(dut.rx_sof, 0)
check("T3 no writes while rx disabled", len(mem), before) check("T3 no writes while rx disabled", len(mem), before)
# ── T7: the GC's BP/RHBP are HONOURED (compat TODO item 2) ─────── # ── T7: the GC's BP/RHBP are HONOURED ────────────────────────────
# These were hardcoded to pages 1-15, so a driver configuring a # A driver configuring a SMALLER ring must not be ignored, or we keep
# SMALLER ring was ignored and we kept writing pages it would never # writing pages it will never read. libogc2 happens to write
# read -- silent corruption. libogc2 writes BP=1/RHBP=15, exactly the # BP=1/RHBP=15, so only a different geometry exercises this.
# old hardcode, which is why it never showed up.
ctx.set(dut.rx_enabled, 1) ctx.set(dut.rx_enabled, 1)
ctx.set(dut.rrp, 1) ctx.set(dut.rrp, 1)
ctx.set(dut.bp, 1) ctx.set(dut.bp, 1)
@@ -814,7 +788,7 @@ if __name__ == "__main__":
check(f"T7b reject {why}", "honoured", "rejected") check(f"T7b reject {why}", "honoured", "rejected")
check("T7b garbage geometry rejected", ctx.get(dut.geom_honoured), 0) 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) ctx.set(dut.rrp, 1)
await tick(4) await tick(4)
mem.clear() mem.clear()