RMII: explore fixes for Rx corruption and document it all

.gitignore

@@ -26,4 +26,4 @@
 go.work
 go.sum
 .vscode
-local
+local*

metastability-filtering.diff

@@ -0,0 +1,18 @@
+diff --git a/rp2-pio/piolib/rmii-rx-extclk.go b/rp2-pio/piolib/rmii-rx-extclk.go
+index 5f35925..6885067 100644
+--- a/rp2-pio/piolib/rmii-rx-extclk.go
++++ b/rp2-pio/piolib/rmii-rx-extclk.go
+@@ -102,8 +102,11 @@ func (r *RMIIRx) Configure(PIO *pio.PIO, cfg RMIIRxConfig) error {
+ 	var rxPinMsk uint32 = 0b111 << rxPin
+ 	rxSM.SetPindirsMasked(0, rxPinMsk)
+
+-	// Optional: bypass input synchronizers for lower latency
+-	PIO.SetInputSyncBypassMasked(rxPinMsk, rxPinMsk)
++	// Input synchronizers left enabled — the datasheet default ("If in doubt,
++	// leave this register as all zeroes", RP2040 §3.5.6.3). Hygiene, not a
++	// fix: the 2-flop sync resolves metastable levels but cannot correct a
++	// wrong-phase sample of the marginal RXD1 edge, so no error-rate change
++	// is expected from this alone. See rx-edge-fix-revalidation.md.
+ 	r.dma.helperEnableDMA(true)
+ 	r.sm = rxSM
+ 	r.rxOff = rxoff

refclk-align.diff

@@ -0,0 +1,84 @@
+diff --git a/rp2-pio/piolib/rmii-rx-extclk.go b/rp2-pio/piolib/rmii-rx-extclk.go
+index 5f35925..2bd0528 100644
+--- a/rp2-pio/piolib/rmii-rx-extclk.go
++++ b/rp2-pio/piolib/rmii-rx-extclk.go
+@@ -20,6 +20,11 @@ type RMIIRxConfig struct {
+ 	IRQ uint8
+ 	// IRQSource is the triggering source for state machine. Varies between 0..3 on RP2040 and extends to 0..7 on RP2350.
+ 	IRQSourceIndex uint8
++	// RefClk is the PHY's 50 MHz RMII reference clock pin (required, 1-31).
++	// RX dibits are sampled on RefClk edges instead of free-running the state
++	// machine, locking the sample phase to the clock the PHY drives data on.
++	// See rx-edge-fix-revalidation.md.
++	RefClk machine.Pin
+ }
+
+ // RMIIRx is a PIO-based RMII receiver. It samples RX0, RX1 and CRS_DV at
+@@ -42,17 +47,23 @@ func (r *RMIIRx) Configure(PIO *pio.PIO, cfg RMIIRxConfig) error {
+ 		return errors.New("IRQSource index out of range (0-7)")
+ 	}
+
+-	whole, frac, err := pio.ClkDivFromFrequency(cfg.Baud, machine.CPUFrequency())
+-	if err != nil {
+-		return err
++	if cfg.RefClk == 0 || cfg.RefClk > 31 {
++		return errors.New("RMIIRx: RefClk pin must be set and in range 1-31")
++	}
++	// The read loop is 4 instructions per 20 ns dibit (wait 1 + wait 0 + in +
++	// jmp), so the state machine needs at least 4 cycles per RefClk period.
++	const rmiiClk = 50_000_000
++	if machine.CPUFrequency() < 4*rmiiClk {
++		return errors.New("RMIIRx: CPU frequency below 200 MHz cannot keep up with RefClk-aligned RX")
+ 	}
+ 	const (
+ 		idxRX0 = iota
+ 		idxRX1
+ 		idxCRSDV
+
+-		polRising = true
+-		labelLoop = 2
++		polRising  = true
++		polFalling = false
++		labelLoop  = 3
+ 	)
+
+ 	asm := pio.AssemblerV0{SidesetBits: 0}
+@@ -63,8 +74,18 @@ func (r *RMIIRx) Configure(PIO *pio.PIO, cfg RMIIRxConfig) error {
+ 			Copyright (c) 2021 Sandeep Mistry
+ 		*/
+ 		asm.WaitPin(polRising, idxCRSDV).Encode(),
+-		asm.WaitPin(polRising, idxRX1).Delay(1).Encode(), // Delay modified from rscott version, yields better results.
+-		labelLoop:// main read loop while CRSDV is high at byte boundary.
++		// Preamble dibits are all 01; RX1 first rises on the final SFD dibit
++		// (11), so this wait sets byte alignment independent of CRS_DV timing.
++		asm.WaitPin(polRising, idxRX1).Encode(),
++		// Consume the rest of the SFD cell so the loop's first sample is the
++		// first data dibit, entered on a known RefClk phase.
++		asm.WaitGPIO(polFalling, uint8(cfg.RefClk)).Encode(),
++		labelLoop:// One dibit per RefClk period; 4 cycles per 20 ns at 200 MHz, zero slack.
++		// The PHY drives RXD on the rising edge. Sampling after the falling
++		// edge (mid-cell) gives the marginal RXD1 rise the most settle time
++		// before the sample instant.
++		asm.WaitGPIO(polRising, uint8(cfg.RefClk)).Encode(),
++		asm.WaitGPIO(polFalling, uint8(cfg.RefClk)).Encode(),
+ 		asm.In(pio.InSrcPins, 2).Encode(),
+ 		asm.Jmp(pio.JmpPinInput, labelLoop).Encode(),
+ 		// Pull in another dibit just in case we desynced by a tidbit. If no desync happened is itty bitty harmless.
+@@ -89,13 +110,16 @@ func (r *RMIIRx) Configure(PIO *pio.PIO, cfg RMIIRxConfig) error {
+ 		pin := rxPin + i
+ 		pin.Configure(pinCfg)
+ 	}
++	cfg.RefClk.Configure(pinCfg)
+ 	// Create state machine configuration
+ 	rxcfg := asm.DefaultStateMachineConfig(rxoff, rxprog[:])
+ 	rxcfg.SetInPins(rxPin, 2)         // IN pins: RX0, RX1 at rxPin
+ 	rxcfg.SetJmpPin(rxPin + idxCRSDV) // JMP pin: CRS_DV at rxPin+2
+ 	rxcfg.SetInShift(true, true, 8)   // In shift: right shift, autopush enabled, threshold 8 bits (1 byte)
+ 	rxcfg.SetFIFOJoin(pio.FifoJoinRx)
+-	rxcfg.SetClkDivIntFrac(whole, frac)
++	// Full CPU clock: pacing comes from the RefClk waits, so there is no
++	// baud-derived divider and no fractional-divider jitter.
++	rxcfg.SetClkDivIntFrac(1, 0)
+ 	// Initialize SM at start of program
+ 	rxSM.Init(rxoff, rxcfg)
+ 	// Set RX pins as inputs (pindirs = 0 for input)

reject-frac-divider.diff

@@ -0,0 +1,19 @@
+diff --git a/rp2-pio/piolib/rmii-rx-extclk.go b/rp2-pio/piolib/rmii-rx-extclk.go
+index 5f35925..08e3098 100644
+--- a/rp2-pio/piolib/rmii-rx-extclk.go
++++ b/rp2-pio/piolib/rmii-rx-extclk.go
+@@ -46,6 +46,14 @@ func (r *RMIIRx) Configure(PIO *pio.PIO, cfg RMIIRxConfig) error {
+ 	if err != nil {
+ 		return err
+ 	}
++	if frac != 0 {
++		// A fractional divider stretches individual PIO cycles, jittering the
++		// sample instant dibit-to-dibit on top of an already-marginal RXD1
++		// edge. RMIITxExtClk rejects non-multiple-of-50MHz CPU clocks for the
++		// same reason. Integer-divider CPU frequencies at 100 Mbit are
++		// {100, 200, 300} MHz. See rx-edge-fix-revalidation.md.
++		return errors.New("RMIIRx: CPU frequency yields a fractional RX clock divider, not supported")
++	}
+ 	const (
+ 		idxRX0 = iota
+ 		idxRX1

rx-edge-defect.md

@@ -0,0 +1,163 @@
+# RMII RX: RXD1 loses simultaneous rising edges
+
+Status: root-caused from field data, fix not yet attempted.
+Affects `rp2-pio/piolib/rmii-rx-extclk.go` (`RMIIRx`). TX (`RMIITxExtClk`) is unaffected.
+
+## Symptom
+
+On a LAN8720 breakout driven by `RMIIRx` + `RMIITxExtClk` (RP2040 @ 200 MHz, 100M full duplex),
+**1.14 % of received IPv4 frames arrive with a corrupted header** (3 234 of 283 546 measured over
+6.3 days of continuous uptime). Corruption is bit-level, 1–14 bytes per frame, and lands on
+whatever byte happens to be vulnerable — MAC addresses, IP addresses, length fields.
+
+**Zero corruption on TX** (0 of 122 229 transmitted frames). That asymmetry is the first clue:
+TX aligns every dibit to the PHY's RefClk, RX does not use RefClk at all.
+
+Downstream this produces silently dropped frames (corrupted destination MAC/IP), stack demux
+errors, and truncated/invalid-length frame errors.
+
+## Root cause
+
+**When RXD1 and RXD0 rise on the same RMII clock edge, RXD1 is sampled low ~1 % of the time.**
+
+Every dibit transition, by observed error rate:
+
+| prev → cur | RXD1 | RXD0 | observations | errors | rate |
+|---|---|---|---|---|---|
+| **`00` → `11`** | **rise** | **rise** | 366 954 | **3 663** | **0.998 %** |
+| `01` → `11` | rise | — (high) | 366 954 | 250 | 0.068 % |
+| `11` → `00` | fall | fall | 519 477 | 269 | 0.052 % |
+| `01` → `10` | rise | fall | 244 636 | 54 | 0.022 % |
+| `10` → `01` | fall | rise | 611 590 | 119 | 0.020 % |
+| `00` → `10` | rise | — (low) | 856 226 | 96 | 0.011 % |
+| `11` → `01` | fall | — | 366 954 | 28 | 0.008 % |
+| `00` → `01` | — | rise | 1 100 862 | 25 | 0.002 % |
+| `10` → `00` | fall | — | 519 744 | 6 | 0.001 % |
+| no transition (5 rows) | — | — | 32 353 348 | 133 | 0.0004 % |
+
+**3 663 of 3 982 errors (92 %) are the single `00` → `11` transition.** The failure is always the
+same direction: the dibit reads back `01`, i.e. RXD1 lost and RXD0 won.
+
+Summarised:
+
+- RXD1 rising **with** RXD0: 0.998 %
+- RXD1 rising **without** RXD0: 0.027 % — **37× lower**
+- No edge at all: 0.0004 % — the floor
+
+Secondary effects, both consistent with a marginal RXD1 rising edge:
+
+- RXD1 rising while RXD0 sits high (`01`→`11`, 0.068 %) is 6× worse than RXD1 rising while RXD0
+  sits low (`00`→`10`, 0.011 %).
+- Simultaneous **falling** (`11`→`00`, 0.052 %) is 7–40× worse than single falls, but 20× better
+  than simultaneous rising. Rise is the weak direction.
+- At a *fixed* byte offset and dibit position, the error rate scales with how long RXD1 sat low
+  first — 0.84 % at 3 dibits low, 1.42 % at 6, 2.24 % at 8. A slow rise that starts from a
+  more-discharged line.
+
+This is the signature of **simultaneous-switching sensitivity plus a slow rising edge on RXD1**,
+sampled at a phase that is too early in the bit cell to tolerate it.
+
+## What is ruled out
+
+Worth recording so nobody re-derives it:
+
+- **Not a fractional clock divider.** `Configure` computes the divider from `machine.CPUFrequency()`
+  (`rmii-rx-extclk.go:45`). This TinyGo target runs RP2040 at **200 MHz**
+  (`machine_rp2_2040.go:12`), so `ClkDivFromFrequency(100e6, 200e6)` → `whole=2, frac=0`. Integer,
+  no dither. *(It would not be integer at 125 MHz or 150 MHz — see "Guard rails" below.)*
+- **Not plesiochronous drift.** The PIO samples on its own 100 MHz clock while the PHY runs off its
+  own crystal, but the measured frames are 102 bytes = 408 dibits = 8.16 µs. At ±100 ppm that is
+  <1 ns of accumulated drift against a 20 ns bit cell. Cannot explain ~1 % error rates.
+  *(It does become significant on 1518-byte frames: ~12 ns, 60 % of a bit cell.)*
+- **Not depth into the frame.** Byte offset 26 is worse than offset 30 at identical run length —
+  non-monotonic, so nothing is accumulating.
+- **Not the wire.** IP header checksums are internally consistent with the *uncorrupted* packet
+  (checksum deltas of −1, not 0x8000), so the PHY delivered good data and corruption happened at
+  or after the pin. Corruption is also RX-only.
+
+## Code-level suspects
+
+In `rp2-pio/piolib/rmii-rx-extclk.go`:
+
+1. **`PIO.SetInputSyncBypassMasked(rxPinMsk, rxPinMsk)` — line 106.** Bypasses the 2-flop input
+   synchronizer on RXD0/RXD1/CRS_DV. The SM then samples the raw asynchronous pad with no
+   metastability filtering, and effectively samples earlier in the bit cell. This is the single
+   most suspicious line, and the cheapest thing to change.
+
+2. **Sample phase is set once per frame and only to 10 ns granularity — line 66.**
+   `asm.WaitPin(polRising, idxRX1).Delay(1)` aligns on the SFD, then the 2-instruction loop
+   (`In` + `Jmp`, lines 68–69) free-runs at exactly one dibit per iteration. With the SM at
+   100 MHz, one `Delay` unit is 10 ns = **half a bit cell**, so the phase is only tunable in
+   half-bit steps and lands wherever the SFD edge fell within a 10 ns window. A frame that
+   resyncs badly is sampled near the data edge for its entire length.
+
+   Note this also means the alignment reference is **RXD1's rising edge** — the one signal that is
+   demonstrably marginal. A late SFD edge poisons the phase for the whole frame.
+
+3. **RX ignores RefClk entirely.** `RMIIRxConfig` has no `RefClk` field, while `RMIITxExtClk`
+   requires one and waits on its falling edge for every dibit (`rmii-tx-extclk.go`, and its own
+   doc comment contrasts itself with the free-running variant). TX has zero errors; RX has 1.14 %.
+
+## Fix plan
+
+Ordered by cost. Each step has the same pass/fail metric (below), so they can be evaluated
+independently.
+
+### 1. Stop bypassing the input synchronizers (one line, minutes)
+
+Drop or make optional the `SetInputSyncBypassMasked` call at line 106. Costs 2 cycles of input
+latency — which shifts the effective sample point later in the bit cell, which is the direction we
+want anyway. If this alone fixes it, done.
+
+### 2. Sweep the sample phase (small, mechanical)
+
+Make the `Delay(1)` on line 66 a config field rather than a hardcoded constant, and sweep it. The
+existing comment ("Delay modified from rscott version, yields better results") says this was already
+hand-tuned once by eye; tune it against the error-rate table instead.
+
+To get finer than half-a-bit granularity, run the SM at 200 MHz (`clkdiv = 1`) with a 4-cycle loop
+instead of 100 MHz with a 2-cycle loop — `In` + `Jmp` + 2 delay cycles. That gives 5 ns phase
+steps. This requires `CPUFrequency() == 200 MHz`, which is already the target's value.
+
+### 3. Align RX to RefClk, as TX does (the structural fix)
+
+Add `RefClk machine.Pin` to `RMIIRxConfig` and `wait` on a defined RefClk edge for each dibit,
+mirroring `RMIITxExtClk`. This removes the per-frame phase lottery and the plesiochronous drift on
+long frames in one change, and makes the sample point a deliberate design parameter rather than an
+artifact of when the SFD arrived.
+
+This is the only option that also fixes 1518-byte frames, where drift alone eats 60 % of a bit cell.
+
+### 4. Hardware, if the above does not close it
+
+Sensitivity to *simultaneous* switching specifically points at shared impedance rather than at one
+trace: ground return path and supply decoupling on the breakout, then series termination and
+capacitive load on RXD1 relative to RXD0. Scope RXD1's rise time at the RP2040 pin, triggering on a
+`00`→`11` dibit, and compare against RXD0.
+
+### Guard rails worth adding regardless
+
+`Configure` silently accepts a fractional divider. `RMIITxExtClk.Configure` already rejects a CPU
+frequency that is not a multiple of 50 MHz; RX should likewise reject `frac != 0`, since a dithered
+PIO clock would reintroduce exactly this class of bug. Of the TX-legal frequencies
+{100, 150, 200, 250, 300} MHz, only **100, 200 and 300** give an integer RX divider at 100 Mbit.
+
+## How to measure (pass/fail)
+
+The metric is the transition table above, rebuilt from captured traffic. Method used to produce it:
+
+1. Capture RX frames with the pcap printer for a few hours on a live LAN.
+2. Select router→device ICMP echo requests of a fixed size (102 B here). Every byte of these is
+   known a priori except the IP id, the two checksums, and the ping timestamp, so the *expected*
+   frame can be reconstructed exactly and compared byte-for-byte against what was received.
+3. Convert expected and received to RMII dibits (LSB-first: `d[n] = (byte >> 2n) & 3`) and bin every
+   dibit by `(previous expected dibit, current expected dibit)`.
+4. Report error rate per bin.
+
+**Pass condition:** the `00`→`11` bin drops to the no-transition floor (~0.0004 %) and no bin
+exceeds ~0.01 %. Partial credit is measurable — a fix that halves it will show as a halved rate,
+so steps 1–3 can be evaluated one at a time.
+
+Sample size matters: at 0.001 % you need ~10⁶ transitions of a given type to see anything, which is
+roughly 100 k frames. An hour of ping traffic at 1 Hz is not enough; flood-ping or a traffic
+generator is.

rx-edge-fix-proposal.md

@@ -0,0 +1,58 @@
+# Proposal: RefClk-aligned RMII RX
+
+Fixes the `00`→`11` simultaneous-rise corruption in `rmii-rx-extclk.go` (see `rx-edge-defect.md`).
+Root cause is that RX samples the async RXD pads on a free-running PIO clock at a phase set once
+per frame off RXD1's own (marginal) rising edge. TX has zero errors because the PHY reclocks TX on
+RefClk; RX has no equivalent reclock.
+
+## Change
+
+Land in cost order. Each step is independently measurable against the `00`→`11` bin (pass = drops
+to ~0.0004 % floor).
+
+### 1. Stop bypassing input synchronizers — 1 line
+
+Remove/gate `PIO.SetInputSyncBypassMasked` at `rmii-rx-extclk.go:106`.
+
+**Why:** bypass samples the raw async pad with no metastability filter, right where a slow RXD1
+rising edge is still settling. Costs 2 cycles latency. Cheapest test — do first, but do not bank on
+it (sync also delays the alignment trigger, so relative phase may not move; the real win here is
+metastability filtering, not phase).
+
+### 2. Make sample phase configurable + finer — small
+
+Promote the hardcoded `Delay(1)` at `rmii-rx-extclk.go:66` to a config field. To get sub-half-dibit
+granularity, run the SM at 200 MHz (`clkdiv=1`) with a 4-cycle read loop (`In` + `Jmp` + 2 delay
+cycles) instead of 100 MHz / 2-cycle. That gives 5 ns phase steps vs the current 10 ns (= half a
+dibit).
+
+**Why:** current phase lands wherever the SFD edge fell in a 10 ns window. Sweeping against the
+error-rate table lets us move the sample point off the data edge deliberately.
+
+### 3. Align RX to RefClk, mirroring TX — structural fix
+
+Add `RefClk machine.Pin` to `RMIIRxConfig`. `wait` on a defined RefClk edge per dibit, as
+`RMIITxExtClk` does at frame start. Run the SM at CPU clock, integer `cpuFreq/50MHz` cycles per
+dibit.
+
+**Why:** removes the per-frame phase lottery *and* plesiochronous drift in one change. Only option
+that also fixes 1518-byte frames, where drift alone eats ~60 % of a bit cell (12 ns). Makes the
+sample point a design parameter, not an artifact of SFD arrival time.
+
+### 4. Guard rail — regardless of the above
+
+Reject `frac != 0` in `Configure` (`rmii-rx-extclk.go:45`), same as `RMIITxExtClk` rejects non-50MHz
+multiples. A dithered PIO clock reintroduces exactly this class of edge-sampling bug. Of TX-legal
+{100,150,200,250,300} MHz, only **100 / 200 / 300** give an integer RX divider at 100 Mbit.
+
+## Recommendation
+
+Ship **#1 + #4 now** (minutes, low risk). Measure. If `00`→`11` still above floor, do **#3** — it is
+the only structural fix and the only one covering long frames. **#2** is a fallback if RefClk wiring
+is unavailable on a given board.
+
+## Validation
+
+Reuse the `rx-edge-defect.md` method: capture fixed-size router→device ICMP echoes, reconstruct the
+expected frame, bin dibits by `(prev, cur)`, report per-bin error rate. Need ~100 k frames
+(flood-ping) for statistical power. Pass: `00`→`11` bin at floor, no bin > 0.01 %.