diff --git a/exi_bba/bba_register_file.py b/exi_bba/bba_register_file.py index c08e443..967bc81 100644 --- a/exi_bba/bba_register_file.py +++ b/exi_bba/bba_register_file.py @@ -81,6 +81,16 @@ class BBARegisterFile(Elaboratable): # through both byte FIFOs. This output is what arms that path. self.id_phase = Signal(init=1) + # ── Ring-overflow protection (RBFI) ─────────────────────────────── + # RRP out to RXFrameAssembler so it can tell how much of the ring is + # still unread and DROP frames instead of overwriting them. + self.rrp = Signal(8, init=_RX_PAGE_FIRST) + # Pulse in: a frame was dropped because the ring was full. Sets + # IR[7] (RBFI), which the GC's IMR = 0x86 enables alongside RI and TI, + # so the driver learns it lost a frame instead of silently reading + # corrupted data. + self.rx_overflow = Signal() + # ── Interrupt (exi domain) ──────────────────────────────────────── self.exi_int_n = Signal(init=1) @@ -597,6 +607,7 @@ class BBARegisterFile(Elaboratable): with m.Case(0x03): m.d.sync += r_cmd_ir.eq(r_cmd_ir & ~cmd_wdata) # ── Interrupt output ───────────────────────────────────────────── + m.d.comb += self.rrp.eq(r_rrp_lo) m.d.sync += self.exi_int_n.eq(~(r_ir & r_imr).any()) # ── Consume RWP updates from sync domain ────────────────────────── @@ -618,6 +629,13 @@ class BBARegisterFile(Elaboratable): with m.If(self.tx_irq_i): m.d.sync += r_ir[2].eq(1) # TI bit + + # RBFI: the ring was full and a frame was discarded. Reported rather + # than hidden -- the previous behaviour was to overwrite unread pages, + # so the driver got corrupted frames with no signal that anything had + # gone wrong. + with m.If(self.rx_overflow): + m.d.sync += r_ir[7].eq(1) m.d.sync += r_ncra[1:3].eq(0) # clear ST bits m.d.sync += tx_queued.eq(0) # ready for the next frame diff --git a/exi_bba/bba_top.py b/exi_bba/bba_top.py index f91a564..f7e543f 100644 --- a/exi_bba/bba_top.py +++ b/exi_bba/bba_top.py @@ -191,6 +191,11 @@ class BBATop(Elaboratable): self.exi_miso.eq(cap.spi_miso), self.exi_miso_next.eq(cap.miso_next), cap.id_phase.eq(reg.id_phase), + # Ring-overflow protection: the assembler needs the GC's read + # pointer to know what it must not overwrite, and reports a + # dropped frame back as RBFI (IR[7]). + asm.rrp.eq(reg.rrp), + reg.rx_overflow.eq(asm.rx_overflow), self.int_n .eq(reg.exi_int_n), ] diff --git a/exi_bba/rx_frame_assembler.py b/exi_bba/rx_frame_assembler.py index fd70847..8e2181f 100644 --- a/exi_bba/rx_frame_assembler.py +++ b/exi_bba/rx_frame_assembler.py @@ -32,6 +32,8 @@ __all__ = ["RXFrameAssembler"] _RX_PAGE_FIRST = 0x01 _RX_PAGE_LAST = 0x0F _PAGES_TOTAL = _RX_PAGE_LAST - _RX_PAGE_FIRST + 1 # 15 +# Worst-case pages one frame can occupy: 1518 + 4 descriptor bytes -> 6 pages. +_MAX_FRAME_PAGES = 6 class RXFrameAssembler(Elaboratable): @@ -78,6 +80,16 @@ class RXFrameAssembler(Elaboratable): # 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) + # 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() + def elaborate(self, platform): m = Module() @@ -103,6 +115,37 @@ class RXFrameAssembler(Elaboratable): # 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) + + # ── How many pages may we still consume? ────────────────────────── + # rwp == rrp means EMPTY, so writing all 15 pages would wrap back to + # "empty" and lose the lot. Usable = (rrp - rwp - 1) mod 15, which is + # 14 when the ring is empty. + rrp_s = Signal(4) + m.d.comb += rrp_s.eq(Mux((self.rrp >= _RX_PAGE_FIRST) + & (self.rrp <= _RX_PAGE_LAST), + self.rrp, _RX_PAGE_FIRST)) + raw = Signal(5) + free = Signal(5) + with m.If(rrp_s >= rwp): + m.d.comb += raw.eq(rrp_s - rwp) + with m.Else(): + m.d.comb += raw.eq(rrp_s + _PAGES_TOTAL - rwp) + m.d.comb += free.eq(Mux(raw == 0, _PAGES_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) # Combinatorial outputs m.d.comb += total_len.eq(data_ctr + 4) @@ -112,7 +155,13 @@ class RXFrameAssembler(Elaboratable): with m.State("IDLE"): m.d.comb += self.rx_ready.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): + # 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) m.d.comb += frame_base.eq(Cat(Const(0, 8), rwp)) m.d.sync += desc_base.eq(frame_base) @@ -121,6 +170,16 @@ class RXFrameAssembler(Elaboratable): m.d.sync += desc_ctr.eq(0) 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. + 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. # @@ -275,6 +334,12 @@ if __name__ == "__main__": # changed nothing it looked at. mem = {} + # 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(). + pulse = {"irq": None, "ovf": False} + async def tick(n=1): for _ in range(n): # A write is issued on a cycle where valid AND ready are both @@ -282,8 +347,14 @@ if __name__ == "__main__": if ctx.get(dut.eth_wr_valid) and ctx.get(dut.eth_wr_ready): mem[ctx.get(dut.eth_wr_addr)] = ctx.get(dut.eth_wr_data) await ctx.tick("sync") + if ctx.get(dut.rx_overflow): + pulse["ovf"] = True + if ctx.get(dut.rx_irq): + pulse["irq"] = ctx.get(dut.rx_wptr_w_data) 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 @@ -309,11 +380,23 @@ if __name__ == "__main__": async def wait_irq(limit=200): for _ in range(limit): + if pulse["irq"] is not None: + return pulse["irq"] await tick() - if ctx.get(dut.rx_irq): - return ctx.get(dut.rx_wptr_w_data) return None + async def wait_done(limit=400): + """Wait for a frame to be either stored (rx_irq) or discarded + (rx_overflow), reading the latches so a pulse that fired during + send_frame is not lost.""" + for _ in range(limit): + if pulse["ovf"]: + return "dropped" + if pulse["irq"] is not None: + return "stored" + await tick() + return "timeout" + def descriptor(page): base = page << 8 return [mem.get(base + i) for i in range(4)] @@ -360,6 +443,43 @@ if __name__ == "__main__": await tick(4) + # ── 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. + 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 + await send_frame(f3) + check("T4 first frame stored", await wait_done(), "stored") + page4 = descriptor(4) + body(4, 32) + + outcomes = [] + for _ in range(12): + await send_frame(f3) + outcomes.append(await wait_done()) + if outcomes[-1] == "dropped": + break + check("T4 ring eventually refuses a frame", + outcomes[-1] if outcomes else "none", "dropped") + # THE property: the frame the GC has not read yet is untouched. + check("T4 unread page preserved", descriptor(4) + body(4, 32), page4) + + # Once the GC catches up, frames must be accepted again -- a full ring + # must not latch the receiver off. The writer is at page 14 here, so + # rrp must move well past it to free the 6 pages a worst-case frame + # needs: free = (rrp - rwp - 1) mod 15, so rrp = 8 gives 8 free. + ctx.set(dut.rrp, 8) + await tick(4) + await send_frame(f3) + check("T4 accepts again after RRP advances", await wait_done(), "stored") + + await tick(4) + # ── T3: RX disabled — SOF must be ignored ──────────────────────── before = len(mem) ctx.set(dut.rx_enabled, 0) diff --git a/gc_bba_test/source/netboot.c b/gc_bba_test/source/netboot.c index 287ffdc..f59c223 100644 --- a/gc_bba_test/source/netboot.c +++ b/gc_bba_test/source/netboot.c @@ -174,6 +174,10 @@ int netboot_receive(void *dst, u32 maxlen, void (*status)(const char *msg)) /* Start receive. Without the SR bit the assembler never writes the * ring, so frames arrive at the W5100 and go nowhere. */ if (bba_w8(REG_NCRA, NCRA_SR) < 0) return -1; + /* Clear any stale interrupt flags, RBFI (bit 7) included: the gateware + * now reports a ring-full drop there, and we would rather see a fresh + * one than inherit a set bit from an earlier run. */ + bba_w8(REG_IR, 0xFF); /* Establish a KNOWN ring state instead of trusting what we inherited. * Swiss initialises the BBA before launching us and writes RRP itself,