Discard SPRAM responses with no request outstanding
SPRAMArbiter gives ETH writes ABSOLUTE priority -- eth_wr_ready is asserted
combinationally whenever eth_wr_valid is high, so the write port is never
stalled. Two consequences worth recording:
- The T5 stall model added to rx_frame_assembler does NOT reflect hardware.
Its value was eliminating a hypothesis, not modelling reality.
- EXI READS are what get preempted, so a read response can be delayed --
including past the end of the transaction that asked for it.
The register file did not guard against that. The response handler pushed
whatever arrived, and HEADER0 reset `outstanding` and `sp_req` but not
`sp_pending`. So a late response was pushed into tx_fifo anyway, a held byte
leaked across the transaction boundary, and `outstanding - 1` from zero
underflowed a 4-bit counter to 15 -- which then broke the prefetch bound for
the rest of that transaction.
Now guarded on (outstanding != 0), and sp_pending is cleared in HEADER0.
WHAT THIS DOES NOT CLAIM. I cannot demonstrate that this causes the +1
descriptor offset seen on hardware (17 of 22 frames, varying frame to frame).
A test written in bba_register_file passes with AND without the guard --
checked both ways -- because that testbench models TX as a capture list per
transaction, with no persistent tx FIFO and no header drain pops, which is
precisely the mechanism by which a leaked byte would shift the next read. A
test that cannot fail is worse than no test, so it is deleted rather than
banked, and the gap is documented where it was.
THE MISSING TEST, recorded in bba_register_file.py: a bba_top-level test that
performs an EXI DMA read from the SPRAM ring WHILE the ethernet side writes
frames into it. Only under that overlap can a response be delayed past its
transaction. No simulation does this today, which is why the offset is still
formally unexplained -- the fix here is a real latent bug, not a proven cause.
--seeds 8: clk 28.1-30.3 MHz, EXI 54.9-62.3 MHz, 8/8 PASS. Best seed 7
(clk 30.26, EXI 61.09), flashed. Gateware only -- .dol unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -343,7 +343,12 @@ class BBARegisterFile(Elaboratable):
|
|||||||
with m.If(self.rx_rdy):
|
with m.If(self.rx_rdy):
|
||||||
m.d.comb += self.rx_en.eq(1)
|
m.d.comb += self.rx_en.eq(1)
|
||||||
m.d.sync += [is_write.eq(self.rx_data[6]), prefetched.eq(0),
|
m.d.sync += [is_write.eq(self.rx_data[6]), prefetched.eq(0),
|
||||||
byte_ctr.eq(0), sp_req.eq(0), outstanding.eq(0)]
|
byte_ctr.eq(0), sp_req.eq(0), outstanding.eq(0),
|
||||||
|
# sp_pending too: a byte still held from the
|
||||||
|
# PREVIOUS transaction would be pushed as this
|
||||||
|
# one's first response and shift the whole
|
||||||
|
# read by one.
|
||||||
|
sp_pending.eq(0)]
|
||||||
with m.If(self.rx_data[7]): # register space
|
with m.If(self.rx_data[7]): # register space
|
||||||
m.d.sync += cmd_space.eq(0)
|
m.d.sync += cmd_space.eq(0)
|
||||||
m.next = "RHDR_HI"
|
m.next = "RHDR_HI"
|
||||||
@@ -589,10 +594,26 @@ class BBARegisterFile(Elaboratable):
|
|||||||
# it outside the FSM: push straight to tx_fifo if it has room, else
|
# it outside the FSM: push straight to tx_fifo if it has room, else
|
||||||
# hold it one cycle. SPRAM reads and register reads never overlap, so
|
# hold it one cycle. SPRAM reads and register reads never overlap, so
|
||||||
# driving tx here cannot collide with the FSM's own pushes.
|
# driving tx here cannot collide with the FSM's own pushes.
|
||||||
with m.If(self.spram_rsp_valid & self.tx_rdy & ~sp_pending):
|
# ⚠️ A response is only OURS if a request is actually in flight.
|
||||||
|
#
|
||||||
|
# ETH writes preempt EXI reads in SPRAMArbiter (ETH has absolute
|
||||||
|
# priority and asserts eth_wr_ready combinationally), so a read
|
||||||
|
# response can arrive later than expected -- including after the
|
||||||
|
# transaction that asked for it has ended. Pushing it then INSERTS A
|
||||||
|
# BYTE at the start of the NEXT DMA read and shifts everything the GC
|
||||||
|
# reads by one. It also underflowed `outstanding` (4-bit: 0 - 1 = 15),
|
||||||
|
# which broke the prefetch bound for the rest of that transaction.
|
||||||
|
#
|
||||||
|
# Hardware symptom (2026-10-09): the SPRAM ring descriptor appearing at
|
||||||
|
# page offset +1 instead of +0 for 17 of 22 frames, VARYING frame to
|
||||||
|
# frame, while the assembler was writing it correctly all along. It
|
||||||
|
# only happens when the GC reads the ring while frames are being
|
||||||
|
# written into it, which no simulation did.
|
||||||
|
with m.If(self.spram_rsp_valid & (outstanding != 0)
|
||||||
|
& self.tx_rdy & ~sp_pending):
|
||||||
m.d.comb += [self.tx_data.eq(self.spram_rsp_data), self.tx_en.eq(1)]
|
m.d.comb += [self.tx_data.eq(self.spram_rsp_data), self.tx_en.eq(1)]
|
||||||
m.d.sync += outstanding.eq(outstanding - 1)
|
m.d.sync += outstanding.eq(outstanding - 1)
|
||||||
with m.Elif(self.spram_rsp_valid):
|
with m.Elif(self.spram_rsp_valid & (outstanding != 0)):
|
||||||
m.d.sync += [sp_hold.eq(self.spram_rsp_data), sp_pending.eq(1),
|
m.d.sync += [sp_hold.eq(self.spram_rsp_data), sp_pending.eq(1),
|
||||||
outstanding.eq(outstanding - 1)]
|
outstanding.eq(outstanding - 1)]
|
||||||
with m.Elif(sp_pending & self.tx_rdy):
|
with m.Elif(sp_pending & self.tx_rdy):
|
||||||
@@ -899,6 +920,29 @@ if __name__ == "__main__":
|
|||||||
errors.append(f"T7 NWAYS after DMA: got {result}")
|
errors.append(f"T7 NWAYS after DMA: got {result}")
|
||||||
print(f"T7 NWAYS after DMA read: 0x{result[0]:02X}")
|
print(f"T7 NWAYS after DMA read: 0x{result[0]:02X}")
|
||||||
|
|
||||||
|
# NO T8 FOR THE LATE-RESPONSE LEAK -- and that is deliberate.
|
||||||
|
#
|
||||||
|
# The guard added to the response handler (outstanding != 0) and the
|
||||||
|
# sp_pending clear in HEADER0 fix a real latent bug: a response that
|
||||||
|
# arrives after its transaction ended used to be pushed anyway, and
|
||||||
|
# `outstanding - 1` from zero underflowed a 4-bit counter to 15,
|
||||||
|
# breaking the prefetch bound for the rest of the transaction.
|
||||||
|
#
|
||||||
|
# But this testbench CANNOT demonstrate the consequence. It models the
|
||||||
|
# TX side as tx_rdy/tx_en with a fresh capture list per transaction --
|
||||||
|
# there is no persistent tx FIFO and no header drain pops, which is
|
||||||
|
# exactly the mechanism by which a leaked byte shifts the NEXT read. A
|
||||||
|
# test written here passes whether or not the guard is present (checked
|
||||||
|
# both ways), and a test that cannot fail is worse than no test.
|
||||||
|
#
|
||||||
|
# WHAT IS ACTUALLY MISSING: a bba_top-level test that performs an EXI
|
||||||
|
# DMA read from the SPRAM ring WHILE the ethernet side writes frames
|
||||||
|
# into it. ETH has absolute priority in SPRAMArbiter, so only then can
|
||||||
|
# a read response be delayed past the end of its transaction. No
|
||||||
|
# simulation does this today, which is why the +1 descriptor offset
|
||||||
|
# (17 of 22 frames on hardware, 2026-10-09) is still unexplained.
|
||||||
|
|
||||||
|
|
||||||
sim = Simulator(reg)
|
sim = Simulator(reg)
|
||||||
sim.add_clock(Period(MHz=24), domain="sync")
|
sim.add_clock(Period(MHz=24), domain="sync")
|
||||||
sim.add_testbench(testbench)
|
sim.add_testbench(testbench)
|
||||||
|
|||||||
Reference in New Issue
Block a user