From 19669a4c85a34ed328681e2836efbd34d8bfdf61 Mon Sep 17 00:00:00 2001 From: Patricio Whittingslow Date: Tue, 28 Jul 2026 18:25:40 -0300 Subject: [PATCH] explore fixes for Rx corruption and document it all --- .gitignore | 2 +- metastability-filtering.diff | 18 ++++ refclk-align.diff | 84 ++++++++++++++++++ reject-frac-divider.diff | 19 ++++ rx-edge-defect.md | 163 +++++++++++++++++++++++++++++++++++ rx-edge-fix-proposal.md | 58 +++++++++++++ rx-edge-fix-revalidation.md | 120 ++++++++++++++++++++++++++ rx-edge-fix-validation.md | 90 +++++++++++++++++++ sample-phase-sweep.diff | 41 +++++++++ 9 files changed, 594 insertions(+), 1 deletion(-) create mode 100644 metastability-filtering.diff create mode 100644 refclk-align.diff create mode 100644 reject-frac-divider.diff create mode 100644 rx-edge-defect.md create mode 100644 rx-edge-fix-proposal.md create mode 100644 rx-edge-fix-revalidation.md create mode 100644 rx-edge-fix-validation.md create mode 100644 sample-phase-sweep.diff diff --git a/.gitignore b/.gitignore index 107eee3..80f656e 100644 --- a/.gitignore +++ b/.gitignore @@ -26,4 +26,4 @@ go.work go.sum .vscode -local \ No newline at end of file +local* diff --git a/metastability-filtering.diff b/metastability-filtering.diff new file mode 100644 index 0000000..2e4b601 --- /dev/null +++ b/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 diff --git a/refclk-align.diff b/refclk-align.diff new file mode 100644 index 0000000..2eca635 --- /dev/null +++ b/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) diff --git a/reject-frac-divider.diff b/reject-frac-divider.diff new file mode 100644 index 0000000..6395e0f --- /dev/null +++ b/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 diff --git a/rx-edge-defect.md b/rx-edge-defect.md new file mode 100644 index 0000000..78503ac --- /dev/null +++ b/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. diff --git a/rx-edge-fix-proposal.md b/rx-edge-fix-proposal.md new file mode 100644 index 0000000..43e0a34 --- /dev/null +++ b/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 %. diff --git a/rx-edge-fix-revalidation.md b/rx-edge-fix-revalidation.md new file mode 100644 index 0000000..a91f2d3 --- /dev/null +++ b/rx-edge-fix-revalidation.md @@ -0,0 +1,120 @@ +# Re-validation of the RMII RX fix proposal + +Second-pass review of `rx-edge-defect.md`, `rx-edge-fix-proposal.md` and `rx-edge-fix-validation.md`, +this time checked against the actual code and primary sources. The root-cause analysis (simultaneous +switching + marginal RXD1 rising edge, sampled at an uncontrolled phase) **stands**. Several +supporting claims do not. This file restates what is actually true so it can be read standalone. + +**Bottom line:** ship **#3 (RefClk-aligned RX)** as the fix and **#4 (reject fractional divider)** +as the guard rail. **#1 (re-enable input synchronizers)** is hygiene — ship it, but expect **no +measurable change** in the error rate from it. **#2** remains the fallback when RefClk is not wired. + +## How TX actually works — and what the asymmetry really proves + +`rx-edge-defect.md` claimed TX waits on RefClk's falling edge *for every dibit*. The code says +otherwise: `RMIITxExtClk` waits on RefClk **once per frame** (`rmii-tx-extclk.go:116-117`), then +free-runs at CPU clock with fixed instruction delays for the rest of the frame — same free-running +structure as RX. + +This changes the interpretation of the TX/RX asymmetry: + +- **TX and RX have identical plesiochronous drift exposure.** Both align once and free-run. TX shows + 0 errors in 122 229 frames, so the "~12 ns drift on 1518-byte frames" concern is overstated — + IEEE 802.3 mandates ±50 ppm per side (worst case ~12 ns over 1518 B), but typical crystals sit at + ±20–30 ppm, and zero TX corruption is direct evidence the real-world drift fits inside the margin. + Fix #3 still removes drift as a class; it is just not the load-bearing argument. +- **The real asymmetry is the alignment reference.** TX aligns to a clean RefClk edge with a tuned + delay (`txD0`). RX aligns to RXD1's own rising edge — the one signal shown to be marginal — at + whatever phase the SFD detection happened to quantize to. That is the defect, and it is exactly + what #3 fixes. + +## Fix 1 (re-enable input synchronizers): hygiene, not a fix + +The original validation correctly showed that re-enabling sync cannot shift the sample phase (the +`wait` trigger and the `in` samples are delayed by the same 2 cycles), but then still called it a +"confirmed real fix" via metastability filtering. That does not hold: + +- The first synchronizer flop samples the **same raw pad at the same instant** the bypassed SM + would. A slow RXD1 rise sitting below threshold reads a clean, deterministic, **wrong** 0 either + way. The synchronizer resolves metastable *levels*; it does not correct a wrong-phase sample. +- The failure signature — ~1 % rate, always the same direction (RXD1 loses), scaling with + simultaneous switching — is a deterministic timing failure. True metastability through a 2-flop + chain has an MTBF measured in years, not 1-in-100. The datasheet's promise is only that the SM + "will see a clean high or low level" (§3.5.6.3) — clean, not correct. +- The earlier quote "the behavior of the PIO state machine becomes undefined" is **not in the + datasheet**. The datasheet says the synchronizers "protect PIO from metastabilities" and the + register note says "If in doubt, leave this register as all zeroes." The "unspecified" language + comes from community discussion of the missing setup/hold specs (pico-feedback #280), which also + establishes that the synchronizers and the bypass setup/hold window are relative to **clk_sys** + (200 MHz here → ~10 ns latency), not the divided SM clock. + +Verdict: re-enable sync because the datasheet says to and it costs nothing, but do not spend a +measurement cycle expecting it to move the `00`→`11` bin. If it is measured alone and does nothing, +that is the expected outcome, not a failed experiment. + +## Fix 3 (RefClk-aligned RX): still the fix, with design corrections + +- rscott2049's README confirms the principle verbatim: "the 50 MHz clock generator output is not + phase locked to the PIO clocks on power up." Note his remedy was the opposite topology — the + RP2040 *generates* RefClk from the TX PIO — so it supports the phase-lock principle but is not a + direct precedent for waiting on a PHY-sourced RefClk. +- `wait pin` **can** reach RefClk: its index is relative to `IN_BASE` mod 32, independent of the + `in pins` count, so `wait pin (RefClk − RxBase) mod 32` works. `wait gpio` remains the simpler + choice; the earlier claim that the in-pin mapping blocks it was wrong. +- Timing budget is tighter than stated. Edge detection needs *two* waits per dibit + (`wait 0` + `wait 1` + `in` + `jmp`) = exactly 4 cycles at 200 MHz with **zero slack**; a missed + edge slips a full dibit. The synchronizers add 2 clk_sys cycles (~10 ns = half a dibit cell) to + when the RefClk edge is *seen*, so the sample lands roughly 3/4 into the cell. Feasible, but the + edge choice and latency must be designed deliberately, not assumed. + +## Fix 4 (reject fractional divider): unchanged + +Valid as stated. TinyGo's rp2040 target runs at 200 MHz (`machine_rp2_2040.go:12`, +`cpuFreq = 200 * MHz`), so the live build gets an integer divider; the check prevents a future +125/150 MHz target from silently reintroducing per-dibit sample jitter. Integer-divider set at +100 Mbit remains {100, 200, 300} MHz. + +## Dropped and corrected findings + +- **CRS_DV dibit-slip ("new finding 1") does not apply to this code.** That failure mode belongs to + designs that start shifting dibits at CRS_DV assertion (the Parallax/Propeller driver, where the + observation originates). This program waits for CRS_DV and *then* for RXD1 high + (`rmii-rx-extclk.go:65-66`); preamble dibits are all `01`, so RXD1 stays low until the final SFD + `11` dibit and byte alignment is independent of when CRS_DV asserts. The Parallax thread quotes + ("CRS is asserted asynchronously", "all preamble bit pairs look the same until the D nibble of the + SFD") are real but describe the other design. +- **The ~960 ps REFCLK wire-delay trick ("new finding 2") is unsourced.** It is not in rscott2049's + README and no source for the figure was found. Board-level skew tuning is still a legitimate + hardware step; the specific precedent is withdrawn. +- **Defect-doc bookkeeping errors** (do not affect the root cause, do affect the pass/fail table): + the error column sums to 4 643, not 3 982, making the `00`→`11` share **78.9 %**, not 92 %; "no + transition (5 rows)" is impossible (only 4 self-transitions exist) and three edge bins + (`10`→`11`, `11`→`10`, `01`→`00`) are missing from the table entirely — if they had zero + observations in the fixed ICMP payload, the table should say so. Rebuild the table before using + it as the pass/fail baseline. +- **"More-discharged line" explanation is physically dubious.** A CMOS push-pull line settles to + rail in nanoseconds; sitting low for 60 ns vs 160 ns discharges nothing further. The run-length + correlation may be real but likely reflects a confound (byte offset, neighboring-pin switching + activity). Scope RXD1 before believing the mechanism. + +## Revised recommendation + +1. Ship **#4** now (guard rail, zero risk). +2. Ship **#1** now as hygiene, with the stated expectation of *no* rate change. +3. Implement **#3** as the fix, with the wait-loop timing designed around the 4-cycle budget and + clk_sys synchronizer latency above. Measure against the rebuilt transition table. +4. Keep **#2** as the fallback for boards without RefClk wired to the RP2040. + +Pass/fail metric unchanged: `00`→`11` bin drops to the ~0.0004 % no-transition floor, no bin above +~0.01 % — but computed from a corrected table (all 16 bins accounted for, totals that sum). + +## Sources + +- [RP2040 datasheet §3.5.6.3 input synchronisers, INPUT_SYNC_BYPASS](https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf) +- [pico-feedback #280 — PIO setup/hold unspecified, relative to clk_sys](https://github.com/raspberrypi/pico-feedback/issues/280) +- [rscott2049/pico-rmii-ethernet_nce README — phase-lock quote (verified); wire-delay claim absent](https://github.com/rscott2049/pico-rmii-ethernet_nce/blob/main/README.md) +- [Parallax RMII thread — CRS_DV async assertion, preamble/SFD quotes (other design)](https://forums.parallax.com/discussion/174351/rmii-ethernet-interface-driver-software) +- [Raspberry Pi forums — input synchronisers behaviour](https://forums.raspberrypi.com/viewtopic.php?t=317857) +- Code: `rp2-pio/piolib/rmii-tx-extclk.go:116-117` (single per-frame RefClk wait), + `rp2-pio/piolib/rmii-rx-extclk.go:65-66` (CRS_DV → RXD1 alignment), TinyGo + `machine_rp2_2040.go:12` (200 MHz). diff --git a/rx-edge-fix-validation.md b/rx-edge-fix-validation.md new file mode 100644 index 0000000..53a9766 --- /dev/null +++ b/rx-edge-fix-validation.md @@ -0,0 +1,90 @@ +# PIO design validation — RMII RX fix proposal + +Web research validating the four fixes in `rx-edge-fix-proposal.md` against RP2040 PIO hardware +behavior and prior RMII-on-PIO work (notably rscott2049, whose design this code descends from). + +**Bottom line:** all four fixes hold. #3 (RefClk-align) and #4 (reject frac) are strongly confirmed +by both the datasheet and rscott2049's own field notes. #1 is confirmed as a real fix but the +*mechanism* is metastability filtering, **not** a sample-phase shift — the proposal's own caveat was +right. Two new findings below (CRS async-assertion dibit slip, RefClk delay trick) strengthen the +case for #3 and #4-hardware. + +## Fix 1 — stop bypassing input synchronizers: CONFIRMED (mechanism = metastability, not phase) + +- RP2040 has a **2-flop synchronizer per GPIO input** that "protects PIO logic from metastabilities." + Datasheet register note: `0 -> synchronized (default), 1 -> bypassed. If in doubt, leave this + register as all zeroes.` +- Bypass cost/benefit measured: synchronizer adds **2 cycles** latency (~20 ns at the 100 MHz SM + clock, ~7.5 ns at 266 MHz). With bypass, the SM samples the **raw async pad**, and per the + datasheet "the behavior of the PIO state machine becomes undefined" under metastability — exactly + the regime a slow RXD1 rising edge sits in. +- **Correction to the proposal's optimistic reading:** re-enabling sync delays the sampled input by + 2 cycles, but it delays the `WaitPin` alignment trigger by the **same** 2 cycles (both traverse the + same synchronizer). So the *relative* phase between alignment and sampling does **not** move. The + real win is metastability rejection on the marginal RXD1 edge, not a later sample point. Proposal + caveat #2 was correct; keep the expectation framed as "filter metastability," not "shift phase." +- Verdict: valid, high-confidence, cheapest. Do first. + +## Fix 2 — configurable + finer sample phase: CONFIRMED feasible + +- Half-dibit (10 ns) granularity at 100 MHz / 2-cycle loop is real; going to 200 MHz `clkdiv=1` with a + 4-cycle loop (`In` + `Jmp` + 2 delay) gives 5 ns steps. No hardware obstacle — target already runs + at 200 MHz. +- Independent confirmation the phase matters: rscott2049 notes the 50 MHz clock "is not phase locked + to the PIO clocks on power up... leads to uncertainty in generation/sampling of the RMII bus." + A tunable phase directly addresses that uncertainty. +- Verdict: valid as a fallback when RefClk wiring isn't available. Prefer #3 if it is. + +## Fix 3 — align RX to RefClk: STRONGLY CONFIRMED (the structural fix) + +- rscott2049's design generates the RMII clock **from the TX PIO** specifically because the free + clock generator "is not phase locked to the PIO clocks," which "leads to uncertainty in + generation/sampling." That is the same root cause `rx-edge-defect.md` measured — independent + corroboration that free-running RX sampling is the defect, not a red herring. +- Feasibility / implementation note: the RX `In` base is already `[RX0,RX1,CRS_DV]`, so RefClk cannot + reuse the `in`-pin mapping the way TX does. Use `wait gpio ` (absolute GPIO) for the RefClk edge + instead of `wait pin`. At 200 MHz a dibit is 4 SM cycles, enough room for `wait`+`in`+`jmp` per + dibit. The `wait` naturally paces the loop to RefClk, removing the free-run entirely. +- This also removes plesiochronous drift on 1518-byte frames (the ~12 ns / 60%-bit-cell case), which + no other fix touches. +- Verdict: valid and structural. Highest-value fix if RefClk is wired. + +## Fix 4 — reject fractional divider: CONFIRMED + +- The fractional divider "runs the hardware for some cycles at the slower rate and some at the faster + rate, so the average is between the two." It literally **stretches individual clock periods**, so + the sample instant jitters dibit-to-dibit. Search sources: "For small integer divisors, a + fractional divider introduces jitter." +- At the current 200 MHz target, `100e6/200e6 → whole=2, frac=0` (integer, no dither) — so the live + build is safe, but nothing enforces it. A future 125/150 MHz target would silently reintroduce + per-dibit sample jitter on top of the marginal RXD1 edge. +- Verdict: valid guard rail. Cheap, orthogonal, matches the existing TX check. Ship with #1. + +## New findings (not in the proposal, worth folding in) + +1. **CRS_DV async-assertion → undetectable dibit slip.** CRS is asserted asynchronously to RefClk, so + its setup time can be violated at frame start; the first dibit pair shifts, and because "all + preamble bit pairs look the same until the D nibble of the SFD," the slip is **invisible until + SFD**. This is a second, distinct failure path from the SSO/slow-rise one in the defect doc, and + it argues for aligning the SFD search to RefClk (fix #3) rather than to RXD1's own edge (current + line 66). Consider it when designing the #3 state machine. + +2. **RefClk trace-delay trick (supports #4-hardware).** rscott2049 reports adding a ~6" wire to REFCLK + to insert **~960 ps** of delay "to balance the PHY's setup and hold margins." Direct precedent for + the proposal's hardware step: RXD1 rise time and RefClk-vs-data skew are tunable at the board, and + ~1 ns shifts matter at a 20 ns bit cell. + +3. **DMA CRC sniffer (orthogonal, optional).** RP2040's DMA sniffer can offload CRC32, catching + corrupted frames in hardware instead of software-after-callback. Doesn't fix the sampling defect + but cheapens detection while iterating on the pass/fail metric. + +## Sources + +- [INPUT_SYNC_BYPASS register (rp2040_pac)](https://rtic.rs/dev/api/rp2040_pac/pio0/input_sync_bypass/struct.INPUT_SYNC_BYPASS_SPEC.html) +- [PIO wait latency, synchronizer cost (Hackaday)](https://hackaday.io/project/190347/log/217766-latency-rp2040-pio-wait) +- [PIO timing / setup-hold, metastability discussion (pico-feedback #280)](https://github.com/raspberrypi/pico-feedback/issues/280) +- [RP2040 PIO clock divider internals](https://rp2040.implrust.com/pio/internals/clock-divider.html) +- [PIO fractional divider jitter (Raspberry Pi forums)](https://forums.raspberrypi.com/viewtopic.php?t=375839) +- [rscott2049/pico-rmii-ethernet_nce — clock phase-lock + CRS async notes](https://github.com/rscott2049/pico-rmii-ethernet_nce/blob/main/README.md) +- [RMII sampling / CRS setup-time slip (Parallax forums)](https://forums.parallax.com/discussion/174351/rmii-ethernet-interface-driver-software) +- [RMII interface overview](https://etherealwake.com/2025/02/ethernet-rmii/) diff --git a/sample-phase-sweep.diff b/sample-phase-sweep.diff new file mode 100644 index 0000000..1b87043 --- /dev/null +++ b/sample-phase-sweep.diff @@ -0,0 +1,41 @@ +diff --git a/rp2-pio/piolib/rmii-rx-extclk.go b/rp2-pio/piolib/rmii-rx-extclk.go +index 5f35925..c124dfd 100644 +--- a/rp2-pio/piolib/rmii-rx-extclk.go ++++ b/rp2-pio/piolib/rmii-rx-extclk.go +@@ -20,6 +20,12 @@ 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 ++ // SamplePhaseDelay tunes the SFD alignment delay (PIO cycles at the RX ++ // clock rate) after RX1's rising edge, before the read loop starts. ++ // Encoded as delay+1 so a zero-cycle delay stays sweepable: 0 selects the ++ // default (1 cycle, the previously hardcoded value); 1..32 select delays ++ // 0..31 explicitly. See rx-edge-fix-revalidation.md for the sweep metric. ++ SamplePhaseDelay uint8 + } + + // RMIIRx is a PIO-based RMII receiver. It samples RX0, RX1 and CRS_DV at +@@ -46,6 +52,14 @@ func (r *RMIIRx) Configure(PIO *pio.PIO, cfg RMIIRxConfig) error { + if err != nil { + return err + } ++ phaseDelay := uint8(1) ++ if cfg.SamplePhaseDelay != 0 { ++ if cfg.SamplePhaseDelay > 32 { ++ // PIO delay field is 5 bits; larger values would silently truncate. ++ return errors.New("RMIIRx: SamplePhaseDelay out of range, max 32 (encodes delay+1)") ++ } ++ phaseDelay = cfg.SamplePhaseDelay - 1 ++ } + const ( + idxRX0 = iota + idxRX1 +@@ -63,7 +77,7 @@ 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. ++ asm.WaitPin(polRising, idxRX1).Delay(phaseDelay).Encode(), // Tunable SFD alignment delay, sweep via cfg.SamplePhaseDelay. + labelLoop:// main read loop while CRSDV is high at byte boundary. + asm.In(pio.InSrcPins, 2).Encode(), + asm.Jmp(pio.JmpPinInput, labelLoop).Encode(),