exi_capture: document the three pin-clocked domains correctly

308 -> 322 lines, 18% -> 13% comment-only, and the clock-domain comment was
flatly wrong.  It said:

    "Two edges are needed: Mode 3 samples MOSI on the RISING edge and MISO
     must change on the FALLING edge, between the master's sampling points.
     Each edge gets TWO domains with the same clock: *_rst async-reset by
     CS ... (plain) no reset ..."

None of that describes this module.  There are THREE domains, not four; they
are all the same edge, not two; there is no `*_rst` naming; and MISO has not
changed on the falling edge since the full-cycle-early launch.  The docstring
now states what is actually built and, for each domain, what forces its reset
choice: the engine must see CS because its counters restart per transaction,
the FIFO sides must not (resetting one side desynchronises an AsyncFIFO's gray
pointers), and both FIFO sides must be posedge because AsyncFIFO instantiates
a posedge-only reset CDC internally.

Also corrected: "capture domain" and "core domain" in the class docstring,
which are names from the deleted oversampling design; cs_active described as
living in "the exi domain" when it is produced in sync; and a claim that the
engine "hands the byte to its negedge output stage", which no longer exists.

The id_arm comment kept its rule and lost its history: armed permanently
because real hardware answers EXI_GetID whenever asked, and a device that
stops identifying itself reads to the host as ABSENT rather than broken; the
port survives only for the PHASE_METER build.

Structure: elaborate() is now _clock_domains, _bit_engine, _byte_fifos and
_transaction_framing.  _transaction_framing records what is deliberately NOT
synchronised here, since that is the non-obvious part -- CLK is a clock and CS
is an async reset, and synchronising them was the original bug rather than a
safety measure.

VERIFIED EQUIVALENT: generated RTLIL identical for both build variants
(phase_meter False and True, 2120 and 2117 lines) after stripping \src
attributes.  Full module suite passes and bba_top T9 is 6/6 at EXI_HALF=1, 2
and 8.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

exi_bba/exi_capture.py

@@ -1,8 +1,9 @@
-"""ExiCapture — EXI byte-capture front-end, clocked by the GC's EXI clock.
+"""ExiCapture — the EXI front end, clocked by the GameCube's own EXI clock.
 
-Creates the pin-driven clock domains (so the byte FIFOs can name them), wraps
+Owns the pin-driven clock domains (which is why it creates them rather than
-the SPIMode3Slave bit engine, and bridges it to the 24 MHz `sync` domain
+`BBATop`: the byte FIFOs have to name them), wraps the `SPIMode3Slave` bit
-through two AsyncFIFOs:
+engine, and bridges that engine to the 24 MHz `sync` domain through two
+`AsyncFIFO`s:
 
     EXI clock pin (~27 MHz)                  sync (24 MHz, HFOSC)
     ┌─────────────────────┐   rx_fifo  ───►  received bytes (header + data)
@@ -10,36 +11,43 @@ through two AsyncFIFOs:
     │  (bit engine)       │   tx_fifo  ◄───  response bytes to drive on MISO
     └─────────────────────┘   (8-bit, sync→exififo_r)
 
+These two FIFOs are the ONLY clock-domain crossings in the EXI path, and each
+has a whole byte time (~296 ns at 27 MHz) of slack.  That is the point of
+clocking the engine from the pin: oversampling it from a faster domain put
+BIT-level timing across a crossing instead, with roughly 55 ns of synchroniser
+latency against a 37 ns bit, and MISO came out two bits late.
+
 Domains created here
 --------------------
-    exiclk     EXI clock pin, posedge, async-reset by CS  -- samples MOSI
+    exiclk     EXI clock pin, posedge, ASYNC-RESET BY CS.  Hosts the bit
-               and launches MISO one bit early
+               engine: samples MOSI, and launches MISO one bit early.
-    exififo_w  EXI clock pin, posedge, RESET-LESS         -- rx FIFO write side
+    exififo_w  EXI clock pin, posedge, RESET-LESS.  rx FIFO write side.
-    exififo_r  EXI clock pin, posedge, RESET-LESS         -- tx FIFO read side
+    exififo_r  EXI clock pin, posedge, RESET-LESS.  tx FIFO read side.
 
-The FIFO domains must NOT see CS: resetting one side of an AsyncFIFO
+All three are the same clock and the same edge; they differ only in reset,
-desynchronises its gray pointers against the other side.  They are also both
+and each difference is forced:
-posedge, because AsyncFIFO instantiates a posedge-only reset CDC internally and
-cannot host a negedge side.
 
-Why pin-clocked: oversampling the EXI clock from a faster domain put BIT-level
+  * The bit engine MUST be reset by CS, because its bit and byte counters
-timing across a CDC boundary (~55 ns of synchroniser latency against a 37 ns
+    have to restart at every transaction.
-bit), so MISO came out ~2 bits late and the GC read 0x17 as 05 C0.  Clocking
+  * The FIFO sides must NOT see CS.  Resetting one side of an `AsyncFIFO`
-from the pin makes MISO clock-to-Q and leaves these byte FIFOs as the only
+    desynchronises its gray pointers against the other side and corrupts it.
-crossing, each with a full byte-time (~296 ns) of slack.
+  * Both FIFO sides must be POSEDGE, because `AsyncFIFO` instantiates a
+    posedge-only reset CDC internally and cannot host a negedge side.
 
 TX response timing
 ------------------
-The engine pops tx_fifo once per byte boundary, unconditionally -- there is no
+The engine pops `tx_fifo` once per byte boundary, unconditionally -- there is
-header counter and no slot threshold.  The byte popped at the end of byte N is
+no header counter and no slot threshold.  The byte popped at the end of byte N
-presented during byte N+1, so pops during the header drain whatever the
+is presented during byte N+1, which gives the scheme two useful properties:
-previous transaction left in the FIFO (this replaces the old CS-fall flush),
-and the response the register file pushes during the protocol's turnaround
-byte lands in data slot 0.
 
-⚠️ Command space has only ONE drain slot (2-byte header), so a command-space
+  * pops during a header drain whatever the previous transaction left in the
-responder must push exactly its bytes and stop -- see the DEVID note in
+    FIFO, so no CS-fall flush is needed; and
-bba_register_file.py.
+  * the response the register file pushes during the protocol's turnaround
+    byte lands in data slot 0.
+
+⚠️ Command space has only ONE drain slot, because its header is two bytes.  A
+command-space responder must therefore push exactly its bytes and stop; see
+the device-ID note in `bba_register_file.py`.
 """
 
 from amaranth import *
@@ -52,74 +60,77 @@ __all__ = ["ExiCapture"]
 
 
 class ExiCapture(Elaboratable):
-    """EXI front-end: SPI bit engine (capture domain) + byte FIFOs to core.
+    """The EXI bit engine plus the byte FIFOs that connect it to `sync`.
 
-    Physical SPI pins (capture domain)
+    Physical SPI pins, from the GC
-    ----------------------------------
+    ------------------------------
-    spi_clk / spi_mosi / spi_cs_n : raw async inputs from the GC
+    spi_clk, spi_mosi, spi_cs_n
-    spi_miso                       : output to the GC
+        Raw inputs.  `spi_clk` becomes the clock of all three domains above
+        and `spi_cs_n` the async reset of `exiclk`.
+    spi_miso
+        Serial output, driven from the fabric shift register.
+    miso_next
+        The same bit one edge early, for an SB_IO output register; this is
+        what the synthesised build drives the pad from.
 
-    Core-facing RX byte stream (core domain, FWFT read side of rx_fifo)
+    RX byte stream, read side in `sync`
-    ------------------------------------------------------------------
+    -----------------------------------
-    rx_data : current received byte
+    rx_data   the received byte at the head of the FIFO
-    rx_rdy  : a received byte is available
+    rx_rdy    a byte is available
-    rx_en   : pop (assert for one core cycle to consume rx_data)
+    rx_en     pop; assert for one `sync` cycle to consume `rx_data`
 
-    Core-facing TX byte stream (core domain, write side of tx_fifo)
+    TX byte stream, write side in `sync`
-    --------------------------------------------------------------
+    ------------------------------------
-    tx_data : response byte to enqueue
+    tx_data   response byte to enqueue
-    tx_en   : write strobe
+    tx_en     write strobe
-    tx_rdy  : tx_fifo has room
+    tx_rdy    the FIFO has room
+
+    Transaction framing
+    -------------------
+    cs_active
+        High in `sync` while a transaction is in progress.  The register file
+        uses it to stream a variable-length (DMA) read until CS deasserts,
+        since the protocol carries no length field.
     """
 
     def __init__(self, rx_depth=4, tx_depth=2, phase_meter=False):
         self._rx_depth = rx_depth
         self._tx_depth = tx_depth
-        self._phase_meter = phase_meter   # diagnostic, see SPIMode3Slave
+        self._phase_meter = phase_meter   # diagnostic; see SPIMode3Slave
 
-        # Physical SPI (capture domain, wired to pins by BBATop)
         self.spi_clk  = Signal(init=1)
         self.spi_mosi = Signal()
         self.spi_cs_n = Signal(init=1)
         self.spi_miso = Signal()
-        self.miso_next = Signal(init=1)   # for an SB_IO output register
+        self.miso_next = Signal(init=1)
-        # Core-facing RX read side
+
         self.rx_data = Signal(8)
         self.rx_rdy  = Signal()
         self.rx_en   = Signal()
 
-        # Core-facing TX write side
         self.tx_data = Signal(8)
         self.tx_en   = Signal()
         self.tx_rdy  = Signal()
 
-        # Core-facing: high (exi domain) while a transaction is in progress.
-        # The register file uses it to stream variable-length (DMA) reads until
-        # CS deasserts.
         self.cs_active = Signal()
 
     def elaborate(self, platform):
         m = Module()
+        self._clock_domains(m)
+        spi = self._bit_engine(m)
+        self._byte_fifos(m, spi)
+        self._transaction_framing(m)
+        return m
 
-        # ── Clock domains driven by the EXI clock pin ─────────────────────
+    def _clock_domains(self, m):
-        # The bit engine is clocked by EXI CLK itself (pin 44 = die (6,0) =
+        """Create the three pin-clocked domains.
-        # global buffer net 6 on the 5k-sg48, so it can drive a real clock
+
-        # network).  Two edges are needed: Mode 3 samples MOSI on the RISING
+        EXI CLK is pin 44, which sits at die (6,0) on the 5k-sg48 = global
-        # edge and MISO must change on the FALLING edge, between the master's
+        buffer input net 6, so it drives a real clock network with no board
-        # sampling points.
+        change.  See the module docstring for why each domain's reset differs.
-        #
+        """
-        # Each edge gets TWO domains with the same clock:
-        #   *_rst  async-reset by CS -- the bit/byte counters, which must
-        #          restart at every transaction
-        #   (plain) no reset -- the byte FIFOs.  Resetting only ONE side of an
-        #          AsyncFIFO desynchronises its gray pointers and corrupts it,
-        #          so the FIFOs must NOT see CS.
         m.domains.exiclk    = cd_p  = ClockDomain(async_reset=True)
         m.domains.exififo_w = cd_fw = ClockDomain(reset_less=True)
-        # NOTE: both FIFO domains are POSEDGE.  AsyncFIFO instantiates a
-        # posedge-only reset CDC internally, so a negedge domain cannot host a
-        # FIFO side.  The engine pops in `exififo_r` (posedge) and hands the
-        # byte to its negedge output stage through a register.
         m.domains.exififo_r = cd_fr = ClockDomain(reset_less=True)
         m.d.comb += [
             cd_p .clk.eq(self.spi_clk), cd_p .rst.eq(self.spi_cs_n),
@@ -127,22 +138,32 @@ class ExiCapture(Elaboratable):
             cd_fr.clk.eq(self.spi_clk),
         ]
 
+    def _bit_engine(self, m):
+        """Instantiate the bit engine and wire it to the pins."""
         spi = SPIMode3Slave(phase_meter=self._phase_meter)
-        # The device-ID fast path is ARMED PERMANENTLY.  It used to be gated
-        # by the register file's `id_phase`, a one-shot cleared on the GC's
-        # first NCRA reset and crossed in here with an FFSynchronizer -- so
-        # after init the device stopped identifying itself and Swiss's
-        # EXI_GetID returned 0x00000000, i.e. "no adapter".  Real hardware
-        # answers EXI_GetID whenever asked.  `id_arm` stays a port because
-        # the PHASE_METER diagnostic build needs to turn the fast path off
-        # (it would otherwise send ID bytes instead of the constant 0x80).
-        m.d.comb += spi.id_arm.eq(0 if self._phase_meter else 1)
         m.submodules.spi = spi
 
-        # RX bytes cross exiclk -> sync; TX bytes cross sync -> exififo_r.
+        # The device-ID fast path is ARMED PERMANENTLY, because real hardware
-        # These are the ONLY crossings left, and each has a whole byte-time
+        # answers EXI_GetID whenever it is asked -- that is how EXI enumerates
-        # (~296 ns at 27 MHz) of slack -- unlike the old design, where
+        # devices, and a device that stops identifying itself after init reads
-        # bit-level MISO timing crossed a domain with a 37 ns budget.
+        # to the host as absent rather than broken.
+        #
+        # `id_arm` remains a port only for the PHASE_METER diagnostic build,
+        # which must turn the fast path OFF so its constant 0x80 pattern is
+        # not replaced by ID bytes in the slots it measures.
+        m.d.comb += spi.id_arm.eq(0 if self._phase_meter else 1)
+
+        m.d.comb += [
+            spi.spi_clk .eq(self.spi_clk),
+            spi.spi_mosi.eq(self.spi_mosi),
+            spi.spi_cs_n.eq(self.spi_cs_n),
+            self.spi_miso .eq(spi.spi_miso),
+            self.miso_next.eq(spi.miso_next),
+        ]
+        return spi
+
+    def _byte_fifos(self, m, spi):
+        """The two AsyncFIFOs that carry bytes across to and from `sync`."""
         rx_fifo = AsyncFIFO(width=8, depth=self._rx_depth,
                             w_domain="exififo_w", r_domain="sync")
         tx_fifo = AsyncFIFO(width=8, depth=self._tx_depth,
@@ -150,16 +171,7 @@ class ExiCapture(Elaboratable):
         m.submodules.rx_fifo = rx_fifo
         m.submodules.tx_fifo = tx_fifo
 
-        # ── Physical pins ↔ bit engine ───────────────────────────────────
+        # RX: every byte the engine completes goes straight in.
-        m.d.comb += [
-            spi.spi_clk .eq(self.spi_clk),
-            spi.spi_mosi.eq(self.spi_mosi),
-            spi.spi_cs_n.eq(self.spi_cs_n),
-            self.spi_miso.eq(spi.spi_miso),
-            self.miso_next.eq(spi.miso_next),
-        ]
-
-        # ── RX: every received byte → rx_fifo ────────────────────────────
         m.d.comb += [
             rx_fifo.w_data.eq(spi.rx_byte),
             rx_fifo.w_en  .eq(spi.rx_valid),
@@ -168,10 +180,8 @@ class ExiCapture(Elaboratable):
             rx_fifo.r_en  .eq(self.rx_en),
         ]
 
-        # ── TX: core pushes responses; the engine pops one per byte-time ──
+        # TX: `sync` pushes responses and the engine pops one per byte time.
-        # Pops are UNCONDITIONAL (one per byte boundary).  Header pops drain
+        # Those pops are unconditional -- see the module docstring.
-        # whatever the previous transaction left behind, so no CS-fall flush
-        # or slot-threshold logic is needed here any more.
         m.d.comb += [
             tx_fifo.w_data.eq(self.tx_data),
             tx_fifo.w_en  .eq(self.tx_en),
@@ -180,16 +190,21 @@ class ExiCapture(Elaboratable):
             tx_fifo.r_en  .eq(spi.tx_load),
         ]
 
-        # ── Transaction framing, in the slow domain ──────────────────────
+    def _transaction_framing(self, m):
-        # CS is slow relative to `exi`, so a plain synchroniser is correct and
+        """Bring CS into `sync` as a level.
-        # there is no longer any reason to detect it in a fast domain.
+
+        CS changes once per transaction, which is slow relative to `sync`, so
+        a plain two-stage synchroniser is the right primitive.  Note what is
+        deliberately NOT done here: the EXI clock and MOSI are not
+        synchronised anywhere.  CLK *is* a clock and CS *is* an async reset --
+        synchronising them into a fast domain was the original MISO bug, not
+        a safety measure.
+        """
         cs_n_s = Signal(init=1)
         m.submodules.cs_sync = FFSynchronizer(self.spi_cs_n, cs_n_s,
                                               o_domain="sync", init=1)
         m.d.comb += self.cs_active.eq(~cs_n_s)
 
-        return m
-
 
 # ── Testbench ─────────────────────────────────────────────────────────────
 
@@ -200,13 +215,13 @@ if __name__ == "__main__":
     dut = ExiCapture()
     errors = []
 
-    # SPI half-period in capture ticks.  At 54 MHz capture / 27 MHz EXI the real
+    # SPI half-period, in units of HP below.  At 27 MHz EXI the real
     # ratio is ~2; use 4 here for a clean, well-oversampled functional check.
     HALF = 4
 
-    # No capture domain any more: the bit engine is clocked by the EXI pin,
+    # The bit engine is clocked by the EXI pin, so there is no faster domain
-    # so drive the SPI clock with real delays instead of counting ticks.
+    # to count ticks against; the SPI clock is driven with real delays.
-    HP = Period(MHz=54)   # ONE capture-equivalent tick; delays are HP * n
+    HP = Period(MHz=54)   # the unit of delay; half-periods are HP * HALF
 
     async def spi_byte(ctx, mosi_val):
         """Clock one SPI Mode 3 byte; return the assembled MISO byte."""