Don't advance the write address across TXDATA; deepen tx_bytes to 2048
Two coupled fixes, both exposed by batching TXDATA writes in the test app. 1. eff_addr advanced on EVERY data byte of a register-space write. But TXDATA (0x48) is a FIFO PORT, not a register: a whole frame is written to that one address. So an 8-byte write put byte 0 in TXDATA and bytes 1..7 into 0x49+, which the switch silently ignores. The frame was an eighth of its declared length, TXFrameDrain blocked waiting for the rest, and TI never set (hardware: txto=16, "no ACK after 20 tries"). SWISS WOULD HAVE HIT THIS. It writes an entire frame with bba_outs to BBA_WRTXFIFOD -- one transaction, one address -- so its TX would have delivered ONE BYTE per frame. The path was never exercised because the test app wrote TXDATA one byte per EXI transaction, giving every byte a fresh header at 0x48. Every other register must still advance, so that bba_outs can write PAR0-5 or MAR0-7 as a block; the exception is TXDATA only. 2. tx_bytes was 16 deep. SPI cannot back-pressure -- there is no way to tell the GC to wait -- so BBARegisterFile silently drops bytes when w_rdy is low, which makes the depth the only thing between a drain stall and a corrupted frame, not a buffering preference. 16 bytes is ~4.7 us at the EXI byte rate, and TXFrameDrain shares the W5100 bus arbiter with RX and the socket-3 UDP test, so any longer stall truncated a frame mid-flight. With (1) fixed, Swiss streams up to 1514 bytes in a single transaction, so 16 could never have held. Now 2048: a whole maximum-size frame plus margin, so a frame cannot be truncated even if the drain makes no progress for the entire transaction. Costs 3 of 27 free BRAMs (ICESTORM_RAM 3/30 -> 6/30, LC 68% -> 70%). Dennis spotted that the 8-byte workaround could not survive whole-frame writes, which is what prompted fixing the depth rather than shipping it. bba_tx_bytes() consequently streams 64-byte chunks (bba_write()'s buffer limit), so a 60-byte ACK is ONE transaction instead of sixty. T8 covers both and is mutation-tested: restoring the increment gives "1/8 bytes MISMATCH -- got [17], want [17, 34, 51, ...]". It now writes 64 bytes so it exercises the deepened FIFO too. --seeds 8: clk 27.6-29.4 MHz, EXI 51.2-64.7 MHz, 8/8 PASS. Best seed 1 (clk 29.35, EXI 60.80), flashed. STILL NO FLOW CONTROL on the TX path: a frame larger than 2048 would drop silently rather than error. Fine for ethernet's 1518 ceiling; revisit if the multiworld socket work ever wants larger payloads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -150,7 +150,25 @@ class BBARegisterFile(Elaboratable):
|
|||||||
# On the SPRAM path that latency exceeded the protocol's turnaround
|
# On the SPRAM path that latency exceeded the protocol's turnaround
|
||||||
# byte and shifted every DMA read by one byte. One slow domain now.
|
# byte and shifted every DMA read by one byte. One slow domain now.
|
||||||
spram_req = SyncFIFO(width=16, depth=4)
|
spram_req = SyncFIFO(width=16, depth=4)
|
||||||
tx_bytes = SyncFIFO(width=8, depth=16)
|
# TX byte FIFO: deep enough for a WHOLE maximum-size frame.
|
||||||
|
#
|
||||||
|
# SPI cannot back-pressure, so BBARegisterFile silently drops bytes
|
||||||
|
# when w_rdy is low -- there is no mechanism to tell the GC to wait.
|
||||||
|
# The depth is therefore not a buffering choice, it is the only thing
|
||||||
|
# standing between a drain stall and a corrupted frame.
|
||||||
|
#
|
||||||
|
# At 16 deep it held 16 bytes = ~4.7 us at the EXI byte rate (296
|
||||||
|
# ns/byte). TXFrameDrain shares the W5100 bus arbiter with RX and the
|
||||||
|
# socket-3 UDP test, so any stall longer than that dropped bytes
|
||||||
|
# mid-frame. Swiss writes an ENTIRE frame in one register-space
|
||||||
|
# transaction (bba_outs to BBA_WRTXFIFOD), so 16 could never work for
|
||||||
|
# real traffic; the on-console test app only got away with it by
|
||||||
|
# writing one byte per transaction, which paced the drain by accident.
|
||||||
|
#
|
||||||
|
# 2048 covers the 1518-byte maximum frame with margin, so a frame
|
||||||
|
# cannot be truncated even if the drain makes no progress at all for
|
||||||
|
# the whole transaction. Costs ~4 of 27 free BRAMs.
|
||||||
|
tx_bytes = SyncFIFO(width=8, depth=2048)
|
||||||
tx_ctrl = SyncFIFO(width=16, depth=4)
|
tx_ctrl = SyncFIFO(width=16, depth=4)
|
||||||
rx_wptr = SyncFIFO(width=8, depth=4)
|
rx_wptr = SyncFIFO(width=8, depth=4)
|
||||||
rx_rptr = SyncFIFO(width=8, depth=4)
|
rx_rptr = SyncFIFO(width=8, depth=4)
|
||||||
@@ -527,7 +545,28 @@ class BBARegisterFile(Elaboratable):
|
|||||||
|
|
||||||
# No length field in the real protocol: consume data
|
# No length field in the real protocol: consume data
|
||||||
# bytes until CS rises.
|
# bytes until CS rises.
|
||||||
m.d.sync += eff_addr.eq(eff_addr + 1)
|
#
|
||||||
|
# ⚠️ TXDATA IS A FIFO PORT, NOT A REGISTER. A whole frame
|
||||||
|
# is written to that ONE address, so the address must not
|
||||||
|
# advance across its data phase. Swiss does exactly this
|
||||||
|
# (bba_outs to BBA_WRTXFIFOD streams the entire frame in a
|
||||||
|
# single register-space write), so incrementing here
|
||||||
|
# delivered only the FIRST byte of every frame and silently
|
||||||
|
# dropped the rest into 0x49+ -- which the switch above
|
||||||
|
# ignores.
|
||||||
|
#
|
||||||
|
# It went unnoticed because the on-console test app wrote
|
||||||
|
# TXDATA one byte per EXI transaction, so every byte got a
|
||||||
|
# fresh header at 0x48. The moment it batched 8 bytes per
|
||||||
|
# transaction, 7 of every 8 vanished: the frame was an
|
||||||
|
# eighth of its declared length, TXFrameDrain blocked
|
||||||
|
# waiting for the rest and TI never set (hardware
|
||||||
|
# 2026-10-09: txto=16, "no ACK after 20 tries").
|
||||||
|
#
|
||||||
|
# Every other register IS a register and must advance, so
|
||||||
|
# that bba_outs can write PAR0-5 or MAR0-7 as a block.
|
||||||
|
with m.If(rd_sel != _TXDATA):
|
||||||
|
m.d.sync += eff_addr.eq(eff_addr + 1)
|
||||||
with m.If(~self.cs_active):
|
with m.If(~self.cs_active):
|
||||||
m.next = "HEADER0"
|
m.next = "HEADER0"
|
||||||
|
|
||||||
@@ -737,6 +776,19 @@ if __name__ == "__main__":
|
|||||||
ctx.set(reg.cs_active, 0)
|
ctx.set(reg.cs_active, 0)
|
||||||
await ctx.tick("sync").repeat(6)
|
await ctx.tick("sync").repeat(6)
|
||||||
|
|
||||||
|
async def drain_tx_bytes(ctx, limit=80):
|
||||||
|
"""Pop everything the register file pushed into tx_bytes."""
|
||||||
|
out = []
|
||||||
|
for _ in range(limit):
|
||||||
|
if not ctx.get(reg.tx_bytes_r_rdy):
|
||||||
|
break
|
||||||
|
out.append(ctx.get(reg.tx_bytes_r_data))
|
||||||
|
ctx.set(reg.tx_bytes_r_en, 1)
|
||||||
|
await ctx.tick("sync").repeat(1)
|
||||||
|
ctx.set(reg.tx_bytes_r_en, 0)
|
||||||
|
await ctx.tick("sync").repeat(1)
|
||||||
|
return out
|
||||||
|
|
||||||
async def exi_devid(ctx):
|
async def exi_devid(ctx):
|
||||||
"""EXI_GetID: write 0x0000 (2 bytes), then clock 4 read bytes."""
|
"""EXI_GetID: write 0x0000 (2 bytes), then clock 4 read bytes."""
|
||||||
tx_log.clear()
|
tx_log.clear()
|
||||||
@@ -920,7 +972,31 @@ 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.
|
# T8: a MULTI-BYTE write to TXDATA must push EVERY byte.
|
||||||
|
#
|
||||||
|
# TXDATA (0x48) is a FIFO port, not a register: a whole frame goes to
|
||||||
|
# that one address. The write address used to advance across the data
|
||||||
|
# phase unconditionally, so byte 0 landed in TXDATA and bytes 1..N-1
|
||||||
|
# went to 0x49+ and were silently ignored.
|
||||||
|
#
|
||||||
|
# Swiss writes an entire frame this way (bba_outs to BBA_WRTXFIFOD), so
|
||||||
|
# its TX would have delivered ONE BYTE per frame. It went unnoticed
|
||||||
|
# because the on-console test app wrote TXDATA one byte per EXI
|
||||||
|
# transaction; the moment it batched 8 bytes per transaction, 7 of
|
||||||
|
# every 8 vanished and TXFrameDrain blocked waiting for a frame that
|
||||||
|
# never completed (hardware: txto=16, no ACK).
|
||||||
|
# 64 bytes, not 8: this also exercises the deepened FIFO and matches
|
||||||
|
# the largest chunk bba_write() can send in one transaction.
|
||||||
|
payload = [(0x11 + i * 3) & 0xFF for i in range(64)]
|
||||||
|
await exi_write(ctx, _TXDATA, payload)
|
||||||
|
got = await drain_tx_bytes(ctx)
|
||||||
|
if got != payload:
|
||||||
|
errors.append(f"T8 multi-byte TXDATA write: got {got}, "
|
||||||
|
f"want {payload}")
|
||||||
|
print(f"T8 multi-byte TXDATA write: {len(got)}/{len(payload)} bytes "
|
||||||
|
f"{'ok' if got == payload else 'MISMATCH'}")
|
||||||
|
|
||||||
|
# NO T9 FOR THE LATE-RESPONSE LEAK -- and that is deliberate.
|
||||||
#
|
#
|
||||||
# The guard added to the response handler (outstanding != 0) and the
|
# The guard added to the response handler (outstanding != 0) and the
|
||||||
# sp_pending clear in HEADER0 fix a real latent bug: a response that
|
# sp_pending clear in HEADER0 fix a real latent bug: a response that
|
||||||
|
|||||||
@@ -125,7 +125,7 @@ int bba_tx_bytes(const u8 *buf, u32 len)
|
|||||||
{
|
{
|
||||||
u32 off = 0;
|
u32 off = 0;
|
||||||
|
|
||||||
/* Stream in 8-byte chunks, ONE EXI transaction each.
|
/* Stream in 64-byte chunks, ONE EXI transaction each.
|
||||||
*
|
*
|
||||||
* Writing byte by byte with bba_w8() costs a full EXI_Lock / EXI_Select /
|
* Writing byte by byte with bba_w8() costs a full EXI_Lock / EXI_Select /
|
||||||
* header / EXI_Deselect / EXI_Unlock PER BYTE -- about sixty transactions
|
* header / EXI_Deselect / EXI_Unlock PER BYTE -- about sixty transactions
|
||||||
@@ -142,14 +142,14 @@ int bba_tx_bytes(const u8 *buf, u32 len)
|
|||||||
* tries"). Byte-by-byte worked only because the per-byte EXI overhead
|
* tries"). Byte-by-byte worked only because the per-byte EXI overhead
|
||||||
* happened to give the drain time.
|
* happened to give the drain time.
|
||||||
*
|
*
|
||||||
* 8 is half the FIFO depth: it cannot overflow even if the drain consumes
|
* tx_bytes is now 2048 deep -- a whole maximum-size frame -- so the old
|
||||||
* nothing for the whole transaction, while still cutting a 60-byte ACK
|
* 8-byte cap is gone and 64 (bba_write()'s buffer limit) sends a 60-byte
|
||||||
* from ~60 transactions to 8. Do not raise it without deepening
|
* ACK in a SINGLE transaction. The depth is what makes this safe: SPI
|
||||||
* tx_bytes or giving the TX path real back-pressure. */
|
* cannot back-pressure, so the FIFO must absorb the entire burst. */
|
||||||
while (off < len) {
|
while (off < len) {
|
||||||
u32 n = len - off;
|
u32 n = len - off;
|
||||||
int rc;
|
int rc;
|
||||||
if (n > 8) n = 8;
|
if (n > 64) n = 64;
|
||||||
rc = bba_write(REG_TXDATA, buf + off, n);
|
rc = bba_write(REG_TXDATA, buf + off, n);
|
||||||
if (rc < 0) return rc;
|
if (rc < 0) return rc;
|
||||||
off += n;
|
off += n;
|
||||||
|
|||||||
Reference in New Issue
Block a user