diff --git a/CLAUDE.md b/CLAUDE.md index e64c376..9152577 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1065,25 +1065,51 @@ REGISTER space (16-bit reg) 4-byte header, then a SEPARATE data phase write: [0xC0, reg>>8, reg&0xFF, 0x00] then N bytes written ``` -⚠️ **Command space is a 2-byte header + data phase — corrected 2026-10-10.** -This file previously said "one 4-byte READWRITE transfer, -`[0x40|reg, 00, 00, data]`", and the decoder followed it: it waited for a -fourth byte, consumed the DATA byte as header byte 2, and **silently dropped -every command-space write.** libogc's `__bba_init` writes command register 2 -(its interrupt mask) at both the start and the end of init, so none of that -would have taken effect. The real shape, from the disassembly: +⚠️ **COMMAND SPACE HAS TWO FORMS AND BOTH ARE REAL — settled 2026-10-10 by +reading all three drivers.** Register space is the same everywhere; command +space is not: -``` -EXI_Imm (len=2, write) -> 42 00 the WHOLE header -EXI_Sync -EXI_ImmEx(len=1, write) -> 00 the data -``` +| contract | Swiss patch | libogc (stock) | libogc2 | +|---|---|---|---| +| RX descriptor | packed LE bitfield | packed LE bitfield | packed LE bitfield | +| register space | 4-byte header + data phase | same | same | +| **command space** | **ONE 4-byte transfer**, value/response in **byte 3** | 2-byte header + data phase, **byte 2** | 2-byte header + data phase, **byte 2** | +| TWD on TX | **never written** | never written | never written | +| TX trigger | NCRA **ST1** | NCRA ST1 (`0x04`) | NCRA ST1 | -Three bytes between CS fall and CS rise, not four. `0x4200 = 0x4000 | -(reg << 8)`. The device-ID read survived the wrong decode only because -`SPIMode3Slave` answers it on a fast path independent of the FSM — which is -exactly why no test caught it. `bba_register_file` **T10** now writes command -register 2 and reads it back, and is mutation-tested against the 4-byte decode. +Sources, so this can be re-checked rather than re-guessed: +- Swiss — `cube/patches/bba/bba.c`, `bba_cmd_out8`: + `exi_imm_read_write(0x40<<24 | (reg&0x3F)<<24 | val, 4)`, and `bba_cmd_in8` + returns the **low byte** of that 32-bit result. +- libogc2 — `lwip/arch/gc/netif/gcif.c`, `bba_cmd_outsnosel`: + `req = (reg<<8)|0x4000; EXI_ImmEx(&req, 2, WRITE); EXI_ImmEx(val, len, WRITE)`. +- stock libogc — the same, read out of `libbba.a` (`gcif.o`, `__bba_init`): + `EXI_Imm(len=2)` of `42 00`, then `EXI_ImmEx(len=1)` of the data. + +**How one decoder serves both:** they share bytes 0 and 1, and differ only in +how many bytes follow. So the device takes the **LAST data byte before CS +rise** as the write value (Swiss sends `[00, val]`, libogc sends `[val]` — +last is `val` either way), and **pushes a read response TWICE** so byte 2 and +byte 3 both carry it. Swiss clocks 4 bytes and pops both copies; libogc clocks +3 and leaves one, which the next transaction's header pops drain — a 2-byte +header gives exactly one drain slot, so the leftover is bounded at one and +cannot accumulate. + +⚠️ **There must be NO auto-increment of `cmd_reg` across the data phase.** It +would apply Swiss's padding `0x00` to the register and `val` to `reg+1`. This +is how the libogc-only version broke Swiss. + +History worth keeping, because the failure mode repeated: this file first had +Swiss's 4-byte form only, so libogc's writes were dropped (the decoder waited +for a fourth byte and ate the data byte as header byte 2). It was then +"fixed" to libogc's form only, which would have dropped Swiss's writes. Only +reading all three drivers showed that neither is "the" driver. The device-ID +read masked the original bug throughout, because `SPIMode3Slave` answers it on +a fast path independent of this FSM. + +Tests: `bba_register_file` **T10** (libogc form) and **T12** (Swiss form, +asserting bytes 2 AND 3), both mutation-tested — re-adding the auto-increment +fails T12, and pushing the response once fails T12's byte 3. NCRA, IR, NWAYS, RWP, RRP, PAR… all live in the REGISTER space (`bba_in8`/`bba_out8`/`bba_ins`/`bba_outs`). Only the device-ID query uses a short header — the single case where the two formats coincide. @@ -1250,8 +1276,13 @@ parallel master is the default — see "W5100 vs W5500".) | 2 | read reg `0x0000` (NCRA) | | 3 | write reg `0x0000` = `0x04` (NCRA ST) | -⚠️ **There is no `TWD_LO`/`TWD_HI` write anywhere**, so the device has to -COUNT the bytes it was handed. `BBARegisterFile` now keeps `txdata_cnt`, +⚠️ **No driver writes `TWD_LO`/`TWD_HI` — not Swiss, not libogc, not +libogc2** (Swiss: `bba_output` is `bba_outs(BBA_WRTXFIFOD, data, size)` then +NCRA ST1; libogc2: `bba_outsregister(BBA_WRTXFIFOD)` then NCRA ST1). So the +device has to COUNT the bytes it was handed. The TWD path is OUR test app's +convention, not a driver's — which is why the "GC sets TWD before streaming" +note in the TWD fix was self-confirming: `t_tx` writes TWD because we wrote +`t_tx`. `BBARegisterFile` now keeps `txdata_cnt`, incremented per byte accepted at `0x0048`, and the NCRA ST fallback queues **that** rather than `tx_frame_len`. Previously it queued `tx_frame_len`, which a driver that skips TWD never sets — so the frame would have gone out @@ -1265,6 +1296,14 @@ before streaming is what lets `TXFrameDrain` consume concurrently and keeps `tx_bytes` a rate-matching buffer instead of whole-frame storage (see the TWD fix note). Swiss writes TWD; libogc does not. Support both. +⚠️ **Swiss rounds the DMA length UP to 32 bytes** (`exi_dma_write` sets +`EXI[..][2] = OSRoundUp32B(len)`), so TXDATA receives up to 31 trailing bytes +beyond the frame. With a counted length the frame goes out that much longer. +Real hardware must behave the same way, since Swiss works on it — trailing +padding past the IP total length is ignored by receivers. Worth knowing before +treating a slightly-too-long TX frame as a bug. Note a 1514-byte frame rounds +to 1536, over the 1518 maximum. + ⚠️ **libogc streams the payload with `EXI_Dma`** — the 32-byte-aligned bulk via `EXI_Dma` after a `PPCDCacheStoreAsync`, then `EXI_ImmEx` for the remainder. This file says elsewhere that EXI DMA *writes* are avoided because the GC's diff --git a/exi_bba/bba_register_file.py b/exi_bba/bba_register_file.py index e693c67..3020dc5 100644 --- a/exi_bba/bba_register_file.py +++ b/exi_bba/bba_register_file.py @@ -257,6 +257,8 @@ class BBARegisterFile(Elaboratable): cmd_space = Signal() # this transaction is a command access cmd_reg = Signal(6) cmd_wreg = Signal(6) # target of a pending command-space write + cmd_have = Signal() # at least one data byte seen this txn + cmd_pushes = Signal(2) # command-read responses pushed byte_ctr = Signal(3) # response-byte index (device-ID sequence) cmd_wr = Signal() # command-space write pending prefetched = Signal() # first response byte already pushed @@ -378,7 +380,8 @@ class BBARegisterFile(Elaboratable): # PREVIOUS transaction would be pushed as this # one's first response and shift the whole # read by one. - sp_pending.eq(0)] + sp_pending.eq(0), + cmd_have.eq(0), cmd_pushes.eq(0)] with m.If(self.rx_data[7]): # register space m.d.sync += cmd_space.eq(0) m.next = "RHDR_HI" @@ -448,25 +451,39 @@ class BBARegisterFile(Elaboratable): m.d.comb += [self.tx_data.eq(rd_val), self.tx_en.eq(1)] m.d.sync += [eff_addr.eq(eff_addr + 1), prefetched.eq(1)] - # ── command space: a 2-BYTE header, then a data phase ───────── + # ── command space: 2-byte header, then a data phase ─────────── # - # [0x40|reg, 0x00] + N data bytes written, or - # [reg, 0x00] + N data bytes read. + # ⚠️ THE TWO REAL DRIVERS DISAGREE ON THE LENGTH, and both must + # work. Checked against the source, not guessed: # - # ⚠️ NOT a single 4-byte transfer. This file used to decode - # [reg, 00, 00, data] and wait for a fourth byte, taking the DATA - # byte as header byte 2 -- so every command-space WRITE was - # silently dropped. libogc's own driver (see the init map in - # CLAUDE.md) does, for a write of 0x00 to command register 2: + # Swiss (cube/patches/bba/bba.c, bba_cmd_out8 / bba_cmd_in8): + # exi_imm_read_write(0x40<<24 | reg<<24 | val, 4) + # -> ONE 4-byte transfer [0x40|reg, 00, 00, val], and a read + # takes the LOW byte of the 32-bit result, so the response + # must land in BYTE 3. # - # EXI_Imm (len=2, write) -> 42 00 the whole header - # EXI_Sync - # EXI_ImmEx(len=1, write) -> 00 the data + # libogc (libbba.a, gcif.o, __bba_init): + # EXI_Imm (len=2, write) -> 42 00 the whole header + # EXI_ImmEx(len=1, write) -> 00 the data + # -> THREE bytes, data in byte 2, and a read takes byte 2. # - # i.e. three bytes between CS fall and CS rise, not four. The - # device-ID read survived the old decode only because - # SPIMode3Slave answers it on the fast path, independently of - # this FSM, which is why nothing caught it. + # Both share bytes 0 and 1, so one decoder serves both: + # + # WRITE: latch every data byte and apply the LAST one at CS + # rise. Swiss sends [00, val] so the last is val; + # libogc sends [val] so the last is val. Hence NO + # auto-increment of cmd_reg -- that would apply Swiss's + # padding 00 to reg and val to reg+1. + # READ : push the response TWICE so byte 2 AND byte 3 carry it. + # Swiss clocks 4 bytes and pops both; libogc clocks 3 and + # leaves one, which the next transaction's header pops + # drain. A 2-byte header gives exactly one drain slot, + # so the leftover is bounded at one and cannot build up. + # + # This file previously had only the 4-byte form (Swiss's), and the + # decoder waited for a fourth byte -- so libogc's writes were + # dropped. Changing it to libogc's form alone then broke Swiss. + # Neither driver is "the" driver; support both. with m.State("CHDR_1"): with m.If(self.rx_rdy): m.d.comb += self.rx_en.eq(1) @@ -479,29 +496,37 @@ class BBARegisterFile(Elaboratable): with m.Elif(is_write): m.next = "CMD_WRITE" with m.Else(): - # Read: the response was prefetched below and the pop - # at the end of this byte lands it in data slot 0. - m.next = "TRAILER" - with m.Elif(~is_write & self.tx_rdy & ~prefetched): - # Ordinary command read: push during header byte 1 so the - # byte-boundary pop presents it in data slot 0. + m.next = "CMD_READ" + with m.Elif(~is_write & self.tx_rdy & (cmd_pushes == 0)): + # Push #1, during header byte 1, so the byte-boundary pop + # presents it in byte 2 -- what libogc reads. m.d.comb += [self.tx_data.eq(rd_val), self.tx_en.eq(1)] - m.d.sync += prefetched.eq(1) + m.d.sync += cmd_pushes.eq(1) + + with m.State("CMD_READ"): + # Push #2 lands in byte 3 -- what Swiss reads. It has a whole + # byte-time to get there, unlike push #1. + with m.If(self.tx_rdy & (cmd_pushes < 2)): + m.d.comb += [self.tx_data.eq(rd_val), self.tx_en.eq(1)] + m.d.sync += cmd_pushes.eq(cmd_pushes + 1) + with m.If(self.rx_rdy): + m.d.comb += self.rx_en.eq(1) # Swiss's padding bytes + with m.If(~self.cs_active): + m.next = "HEADER0" with m.State("CMD_WRITE"): - # Data phase. Each byte writes the running command register, - # which auto-increments so a multi-byte command write behaves - # like the register-space path. cmd_wreg latches the target - # separately because the handler below applies the write a - # cycle later, by which time cmd_reg has already moved on. + # Data phase. Latch each byte; the LAST one before CS rise is + # the value, which is what lets Swiss's 4-byte form and + # libogc's 3-byte form both decode correctly. with m.If(self.rx_rdy): m.d.comb += self.rx_en.eq(1) - m.d.sync += [cmd_wr.eq(1), - cmd_wdata.eq(self.rx_data), + m.d.sync += [cmd_wdata.eq(self.rx_data), cmd_wreg.eq(cmd_reg), - cmd_reg.eq(cmd_reg + 1)] + cmd_have.eq(1)] with m.If(~self.cs_active): - m.next = "TRAILER" + with m.If(cmd_have): + m.d.sync += cmd_wr.eq(1) + m.next = "HEADER0" with m.State("TRAILER"): # Soak up anything else the GC clocks, then wait for CS to rise. @@ -882,6 +907,32 @@ if __name__ == "__main__": await ctx.tick("sync").repeat(6) return result + async def cmd_write_swiss(ctx, creg, val): + """Swiss's command write: ONE 4-byte transfer + [0x40|reg, 0x00, 0x00, val] -- exi_imm_read_write(..., 4).""" + ctx.set(reg.cs_active, 1) + await push_rx(ctx, 0x40 | (creg & 0x3F)) + await push_rx(ctx, 0x00) + await push_rx(ctx, 0x00) # padding, NOT the value + await push_rx(ctx, val) # the value is the LAST byte + ctx.set(reg.cs_active, 0) + await ctx.tick("sync").repeat(6) + + async def cmd_read_swiss(ctx, creg): + """Swiss's command read: a 4-byte read/write whose LOW byte (byte 3) + is the response. Returns (byte2, byte3).""" + tx_log.clear() + ctx.set(reg.cs_active, 1) + await push_rx(ctx, creg & 0x3F) + for _ in range(4): + sample_tx(ctx) + await ctx.tick("sync") + await push_rx(ctx, 0x00) + got = await collect_tx(ctx, 2) + ctx.set(reg.cs_active, 0) + await ctx.tick("sync").repeat(6) + return got + # SPRAM contents the streaming-read test reads back (byte i = 0xA0+i). spram_mem = {0x100 + i: (0xA0 + i) & 0xFF for i in range(64)} @@ -1146,6 +1197,27 @@ if __name__ == "__main__": ctx.set(reg.tx_irq_i, 0) await ctx.tick("sync").repeat(2) + # ── T12: Swiss's command-space form, which DIFFERS from libogc's ── + # Swiss sends ONE 4-byte transfer with the value as byte 3 and reads + # the response from byte 3; libogc sends three bytes with the value as + # byte 2 and reads byte 2. Both are real drivers, so both must work: + # taking the LAST data byte as the value serves both, and pushing the + # read response twice puts it in byte 2 AND byte 3. + # + # Supporting only one of them is not a theoretical risk -- this file + # had Swiss's form only (libogc's writes were dropped), then briefly + # had libogc's form only (Swiss's writes would have been dropped). + await cmd_write_swiss(ctx, 0x02, 0x5A) + both = await cmd_read_swiss(ctx, 0x02) + print(f"T12 Swiss 4-byte cmd write 0x5A -> bytes 2,3 = " + f"{[hex(x) for x in both]}") + if len(both) < 2 or both[1] != 0x5A: + errors.append(f"T12 Swiss cmd form: byte 3 = " + f"{[hex(x) for x in both]}, want 0x5A") + if len(both) >= 1 and both[0] != 0x5A: + errors.append(f"T12 libogc byte-2 slot also wrong: {hex(both[0])}") + await cmd_write_swiss(ctx, 0x02, 0x00) # restore + frame11 = [0x40 + i for i in range(24)] await exi_write(ctx, _TXDATA, frame11) # the frame, in ONE write await exi_write(ctx, _NCRA, [0x04]) # ST trigger, no TWD diff --git a/gc_bba_test/source/bba.c b/gc_bba_test/source/bba.c index b148daf..d1e76a7 100644 --- a/gc_bba_test/source/bba.c +++ b/gc_bba_test/source/bba.c @@ -139,6 +139,35 @@ int bba_cmd_read(u8 creg) return (int)v; } +/* SWISS's command-space form: ONE 4-byte transfer, value in byte 3, and a + * read takes byte 3 (the low byte of the 32-bit result). Swiss's + * bba_cmd_out8/in8 use exi_imm_read_write(..., 4). + * + * This DIFFERS from bba_cmd_write/bba_cmd_read above, which use libogc's + * 2-byte-header form. Both are real drivers and the device must serve both, + * so both shapes are exercised. */ +int bba_cmd_write4(u8 creg, u8 val) +{ + int rc; + u32 w = ((u32)(0x40 | (creg & 0x3F)) << 24) | val; + if ((rc = begin_raw()) < 0) return rc; + if (EXI_Imm(BBA_EXI_CHAN, &w, 4, EXI_WRITE, NULL) <= 0) { end_raw(); return -3; } + if (EXI_Sync(BBA_EXI_CHAN) <= 0) { end_raw(); return -4; } + end_raw(); + return 0; +} + +int bba_cmd_read4(u8 creg) +{ + int rc; + u32 w = ((u32)(creg & 0x3F) << 24); + if ((rc = begin_raw()) < 0) return rc; + if (EXI_Imm(BBA_EXI_CHAN, &w, 4, EXI_READWRITE, NULL) <= 0) { end_raw(); return -3; } + if (EXI_Sync(BBA_EXI_CHAN) <= 0) { end_raw(); return -4; } + end_raw(); + return (int)(w & 0xFF); /* Swiss reads the LOW byte = byte 3 */ +} + int bba_r8(u16 addr) { u8 v; diff --git a/gc_bba_test/source/bba.h b/gc_bba_test/source/bba.h index a82e444..579d293 100644 --- a/gc_bba_test/source/bba.h +++ b/gc_bba_test/source/bba.h @@ -82,6 +82,12 @@ void end_raw(void); int bba_cmd_write(u8 creg, u8 val); int bba_cmd_read(u8 creg); +/* Swiss's command-space form: ONE 4-byte transfer, value/response in byte 3. + * Differs from bba_cmd_write/read above (libogc's 2-byte-header form); the + * device must serve both. */ +int bba_cmd_write4(u8 creg, u8 val); +int bba_cmd_read4(u8 creg); + /* Convenience single-byte helpers. bba_r8 returns the byte, or <0 on error. */ int bba_r8(u16 addr); int bba_w8(u16 addr, u8 val); diff --git a/gc_bba_test/source/tests.c b/gc_bba_test/source/tests.c index 81ac4b0..41b0c34 100644 --- a/gc_bba_test/source/tests.c +++ b/gc_bba_test/source/tests.c @@ -421,6 +421,30 @@ static int t_cmd_write(char *m, size_t n) return rb == 0x00 ? 0 : 1; } +static int t_cmd_write_swiss(char *m, size_t n) +{ + int rc, rb; + /* Swiss sends ONE 4-byte transfer with the value as byte 3 and reads the + * response from byte 3; libogc (and libogc2) send a 2-byte header plus a + * data phase, value in byte 2. Both are real drivers. The device takes + * the LAST data byte as the value and pushes a read response twice, so + * bytes 2 and 3 both carry it -- this test is the Swiss half. */ + if ((rc = bba_cmd_write4(0x02, 0x5A)) < 0) { + snprintf(m, n, "Swiss cmd write failed: EXI error %d", rc); return 1; + } + if ((rb = bba_cmd_read4(0x02)) < 0) { + snprintf(m, n, "Swiss cmd read failed: EXI error %d", rb); return 1; + } + if (rb != 0x5A) { + snprintf(m, n, "Swiss 4-byte form: wrote 0x5A, byte 3 = 0x%02X", rb); + bba_cmd_write4(0x02, 0x00); + return 1; + } + bba_cmd_write4(0x02, 0x00); + snprintf(m, n, "4-byte form ok (byte 3 = 0x%02X)", rb); + return 0; +} + static int t_mac_is_readable(char *m, size_t n) { u8 mac[6]; @@ -654,6 +678,7 @@ static test_t s_spram[] = { static test_t s_init[] = { {"cmd-space write","2-byte header + data phase, as libogc sends it", t_cmd_write, T_PENDING, {0}}, + {"cmd-space Swiss","4-byte transfer, value in byte 3, as Swiss sends", t_cmd_write_swiss, T_PENDING, {0}}, {"MAC readable", "libogc READS the MAC from PAR0-5; it must be sane", t_mac_is_readable, T_PENDING, {0}}, {"init replay", "libogc's 30 init writes, in order, do no harm", t_init_sequence, T_PENDING, {0}}, {"TX without TWD","libogc never writes TWD; length must be counted", t_tx_no_twd, T_PENDING, {0}}, @@ -666,7 +691,7 @@ suite_t g_suites[] = { {"EXI integrity", "the EXI link is error-free, and how fast", s_exi, 6}, {"Ethernet", "frames get to and from the W5100", s_eth, 3}, {"SPRAM/DMA", "bulk reads stream correctly from the ring", s_spram, 2}, - {"libogc init", "the real driver's init sequence and shapes", s_init, 4}, + {"driver shapes", "Swiss + libogc init sequences and transfer shapes", s_init, 5}, }; const int g_nsuites = sizeof(g_suites) / sizeof(g_suites[0]);