Fix the RX ring descriptor to the format libogc actually parses
The assembler wrote 00 00 len[11:8] len[7:0] with bytes 0 and 1 zero. The real format, read out of libogc's own BBA driver, is a PACKED 32-BIT LITTLE-ENDIAN word: next_packet_ptr in bits 0-11, packet_len in bits 12-23, status in bits 24-31. Under that driver our descriptor for a 342-byte frame decoded as a 12-byte frame with a next-pointer of 0 into the reserved page, so Swiss and anything else built on libogc would have initialised perfectly and then misparsed every received frame. Found by disassembling /opt/devkitpro/libogc/lib/cube/libbba.a (gcif.o, bba_interrupt) rather than by guessing: lwbrx r9,0,r9 # 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 srwi r9,r9,24 # status = word >> 24 and. r9,r9,r18 # r18 = 0x..84 -> errors are 0x80 | 0x04 and the low 12 bits are written straight back to RRP (0x0018/0x0019), which is why next_packet_ptr has to be there at all -- it is the driver's read pointer. The length semantics and addressing were already correct (descriptor at page<<8, body at +4, length including the descriptor); only the packing was wrong and next_packet_ptr was never written. RXFrameAssembler now computes the next page BEFORE writing the descriptor (CALC_NEXT, formerly ADVANCE_RWP, moved ahead of the backfill) and writes bytes 0-3 as WRITE_D0..WRITE_D3. WRITE_D3 fixes a latent bug this exposed: WRITE_PLACEHOLDER issues four writes but the first lands twice, so desc_base+3 was NEVER written. That was harmless while the length write covered byte 3. As the status byte it would have been whatever the previous frame left in that cell, and a stale 0x80 or 0x04 makes the driver discard the frame. Readers updated to decode identically: netboot (which also now takes RRP from the descriptor's own next_packet_ptr, as libogc does, instead of recomputing the page span) and t_dma, including its page map. netboot's +0/+1 offset probe is removed -- the underrun that shifted descriptors is fixed and confirmed on console, and under a packed format a +1 read is not a shifted descriptor but garbage. The assembler's testbench now decodes with libogc_decode(), reproducing the instruction sequence above, so the tests check the DRIVER'S algorithm rather than our own packing. Every module testbench and bba_top at EXI_HALF=1/2/8 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -32,7 +32,7 @@ static u8 s_last_desc[4];
|
||||
/* How often the descriptor was found at each page offset. The assembler is
|
||||
* supposed to put it at +0 always; hardware shows it sometimes landing at +1,
|
||||
* varying frame to frame, so count both rather than assume either. */
|
||||
static u32 s_shift_hits[2];
|
||||
static u32 s_desc_ok;
|
||||
/* ACKs where TI never set -- the frame did not reach the W5100. */
|
||||
static u32 s_tx_timeouts;
|
||||
static u8 s_last_dump[32];
|
||||
@@ -60,7 +60,7 @@ static int ring_next(u8 *buf, u32 cap)
|
||||
{
|
||||
u8 wp[2], hdr[8];
|
||||
u32 rpage, wpage, dlen, flen, addr;
|
||||
u32 shift;
|
||||
u32 desc, nextp;
|
||||
|
||||
if (bba_read(REG_RWP_LO, wp, 2) < 0) return -1;
|
||||
rpage = s_rrp;
|
||||
@@ -87,25 +87,24 @@ static int ring_next(u8 *buf, u32 cap)
|
||||
memcpy(s_last_desc, hdr, 4);
|
||||
s_last_page = rpage;
|
||||
|
||||
/* LOCATE the descriptor rather than assume it is at +0.
|
||||
/* The descriptor is a PACKED 32-BIT LITTLE-ENDIAN WORD -- the format
|
||||
* libogc's own BBA driver reads, so this is what GC software expects:
|
||||
*
|
||||
* It should always be at +0: four bytes [00 00 len_hi len_lo] followed
|
||||
* immediately by the frame. On hardware it sometimes sits at +1 instead,
|
||||
* and which one varies frame to frame -- 18 of 23 frames were rejected
|
||||
* for an implausible length while 5 parsed fine, in the same run. That
|
||||
* is a race in the assembler, not a constant offset, and the gateware bug
|
||||
* behind it is not yet found.
|
||||
* 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 : 0x80 and 0x04 are errors
|
||||
*
|
||||
* Taking the length from whichever offset yields a plausible one lets the
|
||||
* transfer proceed either way AND measures the split, which is the
|
||||
* measurement needed to find the real cause. A length is a strong check:
|
||||
* it must exceed the 4 descriptor bytes and fit a legal frame. */
|
||||
for (shift = 0; shift <= 1; shift++) {
|
||||
dlen = ((u32)hdr[shift + 2] << 8) | hdr[shift + 3];
|
||||
if (dlen > 4 && dlen <= 1518 + 4) break;
|
||||
}
|
||||
if (shift > 1) {
|
||||
dlen = ((u32)hdr[2] << 8) | hdr[3]; /* report the nominal one */
|
||||
* See the note in exi_bba/rx_frame_assembler.py for the disassembly this
|
||||
* was taken from. The old reader assumed [00 00 len_hi len_lo] and
|
||||
* probed offsets +0 and +1 to work around the tx-FIFO-underrun bug that
|
||||
* shifted descriptors; that bug is fixed (confirmed on console, +1=0) and
|
||||
* the probe is gone -- under the packed format a +1 read is not a shifted
|
||||
* descriptor, just garbage. */
|
||||
desc = (u32)hdr[0] | ((u32)hdr[1] << 8) |
|
||||
((u32)hdr[2] << 16) | ((u32)hdr[3] << 24);
|
||||
dlen = (desc >> 12) & 0xFFF;
|
||||
nextp = desc & 0xFFF;
|
||||
if (((desc >> 24) & 0x84) || dlen <= 4 || dlen > 1518 + 4) {
|
||||
/* Only now is a hex dump worth an EXI transaction. Doing it for
|
||||
* EVERY frame (as an earlier version did) added a round trip to the
|
||||
* poll loop that is already losing the race against the writer --
|
||||
@@ -113,7 +112,7 @@ static int ring_next(u8 *buf, u32 cap)
|
||||
bba_stream((u16)addr, s_last_dump, sizeof(s_last_dump));
|
||||
return -3;
|
||||
}
|
||||
s_shift_hits[shift]++;
|
||||
s_desc_ok++;
|
||||
flen = dlen - 4;
|
||||
if (flen > cap) flen = cap;
|
||||
|
||||
@@ -122,16 +121,18 @@ static int ring_next(u8 *buf, u32 cap)
|
||||
* one DMA read covers it. */
|
||||
{
|
||||
static u8 tmp[1536 + 4];
|
||||
u32 want = shift + dlen;
|
||||
u32 want = dlen;
|
||||
if (want > sizeof(tmp)) want = sizeof(tmp);
|
||||
if (bba_stream((u16)addr, tmp, want) < 0) return -1;
|
||||
memcpy(buf, tmp + shift + 4, flen);
|
||||
memcpy(buf, tmp + 4, flen);
|
||||
}
|
||||
|
||||
/* Advance RRP by the pages this frame occupied, wrapping 1..15. */
|
||||
/* Advance RRP to the descriptor's OWN next_packet_ptr, which is what
|
||||
* libogc does, rather than recomputing the page span here. The writer
|
||||
* put that value in the descriptor, so the two cannot disagree. */
|
||||
{
|
||||
u32 pages = (shift + dlen + 255) >> 8;
|
||||
u32 next = ((rpage - 1 + pages) % 15) + 1;
|
||||
u32 next = nextp;
|
||||
if (next < 1 || next > 15) next = 1;
|
||||
if (bba_w8(REG_RRP_LO, (u8)next) < 0) return -1;
|
||||
s_rrp = next;
|
||||
}
|
||||
@@ -381,11 +382,10 @@ int netboot_receive(void *dst, u32 maxlen, void (*status)(const char *msg))
|
||||
if (idle > (got ? 5000u : 30000u)) {
|
||||
if (status) {
|
||||
snprintf(line, sizeof(line),
|
||||
"%s (%u seen, %u bad, desc+0=%u +1=%u, txto=%u)",
|
||||
"%s (%u seen, %u bad, desc ok=%u, txto=%u)",
|
||||
got ? "timeout mid-transfer" : "timeout: no match",
|
||||
(unsigned)seen, (unsigned)bad,
|
||||
(unsigned)s_shift_hits[0],
|
||||
(unsigned)s_shift_hits[1],
|
||||
(unsigned)s_desc_ok,
|
||||
(unsigned)s_tx_timeouts);
|
||||
status(line);
|
||||
}
|
||||
@@ -433,9 +433,8 @@ int netboot_receive(void *dst, u32 maxlen, void (*status)(const char *msg))
|
||||
* W5100) versus sent-but-lost. The host reports the
|
||||
* retries; only the GC knows which kind they were. */
|
||||
snprintf(line, sizeof(line),
|
||||
"desc@+0=%u +1=%u bad=%u lapped=%u txto=%u",
|
||||
(unsigned)s_shift_hits[0],
|
||||
(unsigned)s_shift_hits[1],
|
||||
"desc ok=%u bad=%u lapped=%u txto=%u",
|
||||
(unsigned)s_desc_ok,
|
||||
(unsigned)bad, (unsigned)lapped,
|
||||
(unsigned)s_tx_timeouts);
|
||||
status(line);
|
||||
|
||||
Reference in New Issue
Block a user