From 68f3c5e4034922e6ed60bc0a54b14da70e5a342e Mon Sep 17 00:00:00 2001 From: Michele Bigi Date: Sun, 6 Sep 2026 17:33:45 +0200 Subject: [PATCH] fix(v2): resolve ERR-0025 Part B - SRAM read timing bug in weight/activation memory Root-causes and fixes the real, disclosed defect left open at the end of the previous STEP20 commit: the board-level SPI host interface produced wrong compute results when jobs were dispatched with realistic (widely time-separated) pacing, even though job registration itself was already confirmed correct at the dependency_manager handshake. Root cause: nms_weight_packed.v and nms_activation_replicated.v both used a REGISTERED SRAM read (rd_data_reg <= mem[addr], one full clock of latency), but nms_memory_manager_stream_wide.v's own read-ahead pipeline (its `rd_pending` bit) is designed around a COMBINATIONAL read -- a request issued this cycle produces data already valid to capture the very next cycle. A busy, multi-tile job (e.g. the STEP19 D-Stress regression, 16 tiles/neuron) never exposes the mismatch, since its own weight/activation prefetch always runs far enough ahead that any given tile has been sitting stable in the SRAM for many cycles by the time it's actually consumed. An uncontested single-tile job has zero such margin: its one tile's read fires on the exact edge the data nominally becomes ready, landing squarely on the missing cycle and permanently latching stale/zero data. Fixed by making both SRAMs' reads combinational, with an explicit same-cycle fill/read address-match bypass for the one hazard a plain combinational read alone would still miss. No FSM, arbiter, or SDRAM controller logic was touched. Verified (Verilator, per this project's own standing DEC-0004 protocol): - tb_fpga_neural_v2_top_smoke.v: 11/11 PASS -- single job, back-to-back jobs, a realistic ~85us-gap job pair, and a parametric sweep of inter-job gaps (100ns/5000ns/50000ns). - STEP19 D-Stress N=2: 49788 cycles, 256/256 bit-exact -- identical cycle count to before this fix (zero regression). - STEP19 D-Stress N=4: 49771 cycles, 256/256 bit-exact -- identical cycle count to before this fix (zero regression). - tb_sdram_unified_backend.v (40/40) and tb_spi_host_bridge.v (18/18) reconfirmed unaffected. The physical SPI host interface is now verified correct end-to-end. Real synthesis/P&R of the board-level top (fpga_neural_v2_top.v) is the deliberate next step, not yet performed this round. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013xXuuRUWZScuo1DeYJxs3v --- hardware/v2/docs/CHIP_READINESS.md | 35 ++-- hardware/v2/docs/OPEN_ITEMS.md | 58 +++--- hardware/v2/logs/decisions.log | 33 ++++ hardware/v2/logs/errors.log | 62 ++++++ .../v2/nms/rtl/nms_activation_replicated.v | 35 +++- hardware/v2/nms/rtl/nms_weight_packed.v | 25 ++- .../v2/nms/sim/tb_fpga_neural_v2_top_smoke.v | 178 ++++++++++++------ 7 files changed, 329 insertions(+), 97 deletions(-) diff --git a/hardware/v2/docs/CHIP_READINESS.md b/hardware/v2/docs/CHIP_READINESS.md index e44d623..9035f8e 100644 --- a/hardware/v2/docs/CHIP_READINESS.md +++ b/hardware/v2/docs/CHIP_READINESS.md @@ -22,10 +22,15 @@ masked. (39 signals) REALLY assigned from the official Lattice CSV and P&R-confirmed; host bus (110 signals) deliberately unassigned (see below) -[ ] clock defined -- oscillator-vs-PLL decision NOT made +[ ] clock defined -- real EHXPLLL RTL now exists (STEP20, + ecp5_pll_sys_clk.v, 16MHz->64MHz), NOT yet + confirmed by synthesis/P&R of the board top [ ] power defined -- rail voltages known; regulators NOT selected [ ] FPGA configuration defined -- standard pins identified; flash NOT chosen -[ ] host interface defined -- BLOCKER: 110-pin raw bus, no real protocol +[x] host interface defined -- STEP20: real SPI protocol engine + (spi_host_bridge.v), verified correct + end-to-end (11/11, board-level smoke test), + zero regression to the STEP19 baseline [x] schematic requirements complete -- SCHEMATIC_READINESS.md's own block diagram and interconnection list are complete [x] first-power-on test defined -- FIRST_POWER_ON.md's own 12-step procedure @@ -37,17 +42,20 @@ masked. real board-programmable artifact ``` -**7 of 14 items checked. HARDWARE READY = NO.** +**8 of 14 items checked. HARDWARE READY = NO.** -**STEP20 update:** a real SPI host interface RTL now exists -(`spi_host_bridge.v` + `fpga_neural_v2_top.v`), narrowing item "host -interface defined" from "does not exist" to "exists, protocol-correct -in isolation, but NOT yet proven correct end-to-end" (errors.log -ERR-0025 Part B, real and unresolved) — still unchecked, for a more -specific reason than before. The STEP19 core (raw `reg_*` interface) -remains bit-exact verified and was reconfirmed fresh this session via -Verilator after an unrelated Icarus Verilog v13.0 toolchain regression -was found and ruled out (ERR-0024). +**STEP20 update:** a real SPI host interface (`spi_host_bridge.v` + +`fpga_neural_v2_top.v`) now exists AND is verified correct end-to-end +(errors.log's "ERR-0025 Part B — RESOLUTION": a registered- vs +combinational-read SRAM timing bug, found via the board-level smoke +test, root-caused, fixed with zero regression to the STEP19 baseline) +— "host interface defined" is now checked. A real EHXPLLL clock +wrapper also now exists (`ecp5_pll_sys_clk.v`) but has not yet been +through synthesis/P&R of the board-level top, so "clock defined" +remains unchecked for that specific, narrower reason. The STEP19 core +(raw `reg_*` interface) remains bit-exact verified and was reconfirmed +fresh this session via Verilator after an unrelated Icarus Verilog +v13.0 toolchain regression was found and ruled out (ERR-0024). ## Why each unchecked item is unchecked (no vague language) @@ -56,10 +64,9 @@ was found and ruled out (ERR-0024). | N4 timing ≥80MHz | 8 real P&R seeds measured; only 1 (81.84MHz) clears 80MHz. This is a real MARGINAL result, not a PASS, per the governing spec's own explicit classification rule (some seeds pass, most do not). | | Constraints complete | `v2_unified.lpf` real and P&R-verified for 39/149 signals (clock frequency + clk/rst + the full 37-signal SDRAM bus, sourced from the real Lattice pinout CSV found at `~/Downloads/` during this step's own pre-commit review). The 110-signal host bus is deliberately left unassigned. | | Pinout complete | Signal inventory is complete (149, exactly matching real P&R); SDRAM+clk/rst (39 signals, 26%) are now really assigned and P&R-confirmed; the 110-signal host bus is unassigned, not because pin data is missing, but because that bus is not yet a real physical protocol (see below) — assigning it balls now would be premature. | -| Clock defined | No PLL exists in the RTL (confirmed: 0/4 EHXPLLL in every synthesis run); the board's real oscillator frequency (16MHz per prior project memory) does not match the RTL's own 80MHz requirement, and neither "new oscillator" nor "add a PLL" has been decided. | +| Clock defined | A real EHXPLLL wrapper now exists (`ecp5_pll_sys_clk.v`, STEP20, real Project Trellis `ecppll`-generated parameters, 16MHz->64MHz) and is instantiated in the board-level top, but has NOT yet been confirmed by synthesis/P&R of that top — deliberately deferred until ERR-0025 Part B was resolved (decisions.log DEC-0037). | | Power defined | Rail VOLTAGES are known from real datasheets; regulator SELECTION, CURRENT budget, and decoupling are not — no real power-estimation tool was run, and the previously-referenced board power-tree design is not accessible this session to confirm as a concrete plan. | | FPGA configuration defined | Standard ECP5 config pins (TDI/TDO/TCK/TMS/PROGRAMN/INITN/DONE/CCLK) are correctly identified as existing and standard, but no configuration-flash part number or SPI-vs-JTAG-only bring-up approach has been chosen for V2 specifically. | -| Host interface defined | The RTL's own "host" ports are a 110-pin raw parallel test-harness bus (`reg_valid`/`reg_node_id`/etc.), not a real board protocol. No serializing bridge RTL exists. This is the single largest real gap between "simulated/synthesized" and "physically buildable." | | Bitstream reproducible | Every P&R run this project has performed used free (unconstrained) I/O placement — a real, valid way to prove the design FITS the package, but not a way to produce a bitstream a real board's own fixed wiring could actually use. | ## What this means, precisely diff --git a/hardware/v2/docs/OPEN_ITEMS.md b/hardware/v2/docs/OPEN_ITEMS.md index 2d4fc2b..04baf72 100644 --- a/hardware/v2/docs/OPEN_ITEMS.md +++ b/hardware/v2/docs/OPEN_ITEMS.md @@ -7,24 +7,36 @@ FUTURE. ## BLOCKER (impede la realizzazione o il funzionamento del chip) -0. **STEP20 update:** a real SPI host interface (`spi_host_bridge.v`) - was implemented and is protocol-correct in isolation (18/18, - `tb_spi_host_bridge.v`), but a real, disclosed, UNRESOLVED defect - (errors.log ERR-0025 Part B) produces wrong compute results when - jobs are dispatched through it with realistic (widely time- - separated) pacing — root cause not yet isolated. This SUPERSEDES - item 1 below with a more specific, code-level blocker: the physical - host interface RTL now exists, but is not yet proven correct. -1. **No physical host interface exists.** The RTL's own "host" ports - are a 110-pin raw parallel job-registration bus - (`reg_valid`/`reg_node_id`/`reg_required`/`reg_producer_ids`/ - `reg_x_base`/`reg_w_base`/`reg_n_tiles`/`reg_result_addr`) — a - simulation/testbench convenience, not a real board protocol. No - RTL exists to serialize it (e.g. SPI, matching V1's own - `spi_neuron_top.v` precedent). -2. **The 110-pin host/registration bus has no real ball assignment** - (deliberately — it is not yet a real physical protocol, see item 1). - The SDRAM bus (37 signals) and clk/rst (2 signals) now DO have a +0. **RESOLVED (STEP20).** A real SPI host interface (`spi_host_bridge.v`) + is now implemented, protocol-correct in isolation (18/18, + `tb_spi_host_bridge.v`), AND verified correct end-to-end through the + full SPI→dependency_manager→compute→SDRAM→result path under both + tight and realistic (widely time-separated) job pacing (11/11, + `tb_fpga_neural_v2_top_smoke.v`) — see errors.log's own "ERR-0025 + Part B — RESOLUTION" entry for the full root-cause writeup (a + registered- vs combinational-read latency mismatch in the shared + weight/activation SRAMs, fixed with zero regression to the STEP19 + baseline). This item is CLOSED — kept here only for the historical + record; item 1 below is likewise no longer a real blocker in the + sense of "the RTL doesn't exist" — it remains open only for real + pinout/board-connector work (see item 1's own updated text). +1. **RESOLVED (STEP20).** The RTL's own internal "host" ports (`reg_valid` + /`reg_node_id`/`reg_required`/`reg_producer_ids`/`reg_x_base`/ + `reg_w_base`/`reg_n_tiles`/`reg_result_addr`) remain a simulation/ + testbench-only bus for `nms_neural_multiprocessor_sdram_unified.v` + in isolation, but the board-level top (`fpga_neural_v2_top.v`) now + drives these SAME internal ports from `spi_host_bridge.v`, a real, + verified SPI protocol engine (WRITE_JOB/WRITE_MEM/READ_MEM/STATUS/ + RESET), matching V1's own `spi_neuron_top.v` precedent. The 110-pin + bus is no longer exposed as a physical top-level port at all in + `fpga_neural_v2_top.v` — only 4 real SPI pins (sclk/mosi/miso/cs_n) + are. +2. **The 110-pin host/registration bus has no real ball assignment** — + moot now (see item 1): it is an internal signal, not a top-level + port, in the board-level top. The 4 real SPI pins likewise have no + ball assignment yet, since `fpga_neural_v2_top.v` has not been + through P&R this round (see the next open item). The SDRAM bus (37 + signals) and clk/rst (2 signals) now DO have a real, sourced, P&R-verified assignment (`hardware/v2/constraints/ v2_unified.lpf`, from the real Lattice pinout CSV found at `~/Downloads/FPGA-SC-02034-3-0-ECP5U-45-Pinout.csv` during this @@ -42,9 +54,13 @@ FUTURE. `first_ready_idx`/`reg_ready` chain) — the regression is attributed to added overall die/routing pressure from consolidation, not a new RTL defect, but it is real and unresolved. -2. **Clock source/oscillator gap.** The RTL requires a direct ≥80MHz - clock (no PLL exists anywhere in the hierarchy — confirmed via - `EHXPLLL: 0/4` in every real synthesis run). Prior project memory +2. **Clock source/oscillator gap -- PARTIALLY ADDRESSED (STEP20).** A + real EHXPLLL wrapper (`ecp5_pll_sys_clk.v`, real Project Trellis + `ecppll`-generated parameters, 16MHz->64MHz) now exists and is + instantiated in `fpga_neural_v2_top.v`. NOT YET confirmed by real + synthesis/P&R of that board-level top this round (deliberately + deferred until ERR-0025 Part B was resolved -- see decisions.log + DEC-0037) -- this is the immediate next real step. Prior project memory records a 16MHz board oscillator. Neither "source an 80MHz+ oscillator" nor "add a real PLL to the RTL" has been decided. 3. **Two physical memories were required through STEP18** — RESOLVED diff --git a/hardware/v2/logs/decisions.log b/hardware/v2/logs/decisions.log index bf7b322..8d43be9 100644 --- a/hardware/v2/logs/decisions.log +++ b/hardware/v2/logs/decisions.log @@ -2107,3 +2107,36 @@ deliberately NOT attempted this round -- doing so before the functional defect is resolved would produce a real bitstream/timing report for RTL known to compute wrong answers under realistic host timing, which is not a meaningful use of that real toolchain work. + +DEC-0037 + +DATE: 2026-09-06 + +DECISION: +ERR-0025 Part B (real, downstream defect blocking the physical SPI +host interface's own end-to-end correctness) is RESOLVED. The fix +(combinational reads in nms_weight_packed.v / nms_activation_ +replicated.v, see errors.log's own "ERR-0025 Part B -- RESOLUTION" +entry for the full root-cause writeup) is verified to introduce ZERO +regression against the STEP19 baseline (N=2/N=4 D-Stress, identical +cycle counts, still bit-exact) while making the NEW STEP20 board-level +integration path (SPI -> dependency_manager -> compute -> SDRAM -> +result) correct under both tight and realistic-gap job dispatch. + +This REVISES DEC-0036's own "V2 HARDWARE RELEASE: FAIL" conclusion: +the single blocking item DEC-0036 identified is now closed. The +physical SPI host interface is no longer just "protocol-correct in +isolation" -- it is now verified correct end-to-end, matching the same +bit-exact standard already established for the raw reg_* interface. + +NEXT STEP (not yet performed this round, per the governing spec's own +explicit sequencing -- "prepara il repository per il passo successivo: +synthesis/P&R del vero fpga_neural_v2_top... NON eseguire benchmark +finche' ERR-0025 Part B non e' completamente chiuso e la V2 non ha +superato synthesis/P&R"): real synthesis and P&R of fpga_neural_v2_top.v +against a real, ball-assigned LPF, to determine the real, achievable +operating frequency for the board-level top (including the SPI bridge +and the new host-arb arbitration layer) -- this was deliberately not +attempted before now, since running the real toolchain against RTL +known to compute wrong answers would not have been a meaningful +result. That reason no longer applies. diff --git a/hardware/v2/logs/errors.log b/hardware/v2/logs/errors.log index 98be2c7..b9f51c9 100644 --- a/hardware/v2/logs/errors.log +++ b/hardware/v2/logs/errors.log @@ -1182,3 +1182,65 @@ timing is NOT yet established. Does not affect the STEP19 baseline remains bit-exact verified. Requires dedicated follow-up before this step's own board-level top (fpga_neural_v2_top.v) can be considered hardware-release-ready. + +ERR-0025 Part B -- RESOLUTION (STEP20, ERR-0025 Part B closed) +DATE: 2026-09-06 +ROOT CAUSE: nms_weight_packed.v and nms_activation_replicated.v both +used a REGISTERED read port (`rd_data_reg <= mem[addr]`, gated by +rd_en -- a full clock cycle of latency from address/enable to valid +output), but nms_memory_manager_stream_wide.v's own pipelined read- +ahead consumer (`rd_pending`) is designed around a COMBINATIONAL read +(issue this cycle -> already-valid data captured next cycle). Traced +via a full internal signal walk (dependency_manager -> neural_director +-> per-slot memory_manager -> weight_prefetch_engine_wide/nms_weight_ +packed -> nms_activation_fill_ctrl_v3/nms_activation_replicated): +job registration, slot dispatch, and per-job context capture (w_base_ +reg/result_addr_reg) were ALL confirmed correct at every stage: the +corruption traced all the way down to the exact cycle where `buf_ +weight`/`buf_input` are captured, which used the PRE-edge (stale) +value of the SRAM's own registered output -- one real cycle before +that register's own update (from the read issued the SAME cycle) +actually committed. A busy, multi-tile job (e.g. STEP19's D-Stress, +128 inputs/16 tiles per neuron) never exposes this: its own weight/ +activation prefetch runs far enough ahead (PREFETCH_DISTANCE=8) that +by the time any given tile is actually consumed, that tile's data has +already been sitting stable in the SRAM for many cycles, masking the +extra latency completely. An uncontested single-tile job (STEP20's own +board-level SPI smoke test, tb_fpga_neural_v2_top_smoke.v) has ZERO +such margin: its one-and-only tile's read fires on the exact edge the +data nominally becomes "ready", landing squarely on the missing cycle +-- permanently latching stale/zero weight and activation data. This +explains BOTH observed symptoms exactly: neuron 0 (first job, first +real weight fetch of the whole session) read back 0 (all-zero SRAM +reset content); neuron 1 (second job, reusing the same physical slot) +read back 100 -- neuron 0's own TRUE value, one full cycle "behind" +where it should have been. +FIX: made both SRAMs' read ports combinational (`assign rd_data = mem +[addr]`, replacing the registered `always @(posedge clk) rd_data_reg +<= mem[addr]`), with an explicit same-cycle fill/read address-match +bypass (forwarding fill_data directly) for the one hazard a plain +combinational read alone would still miss -- a fill and a read to the +IDENTICAL address landing on the IDENTICAL edge, where mem[] itself +would not yet reflect that same-edge write. Files changed: hardware/ +v2/nms/rtl/nms_weight_packed.v, hardware/v2/nms/rtl/nms_activation_ +replicated.v. No change to any FSM, arbiter, SDRAM controller, or +dependency-tracking logic -- this is a pure, minimal, two-file SRAM +read-timing fix. +VERIFICATION (all via Verilator, the trusted tool per DEC-0004): + - tb_fpga_neural_v2_top_smoke.v: 11/11 PASS -- single job alone, + two jobs back-to-back, two jobs with a ~85us realistic SPI-paced + gap, and a parametric sweep of inter-job gaps (100ns/5000ns/ + 50000ns), all bit-exact. + - tb_nms_dstress_sdram_unified.v N=2: 49788 cycles, 256/256 + bit-exact -- IDENTICAL cycle count to before this fix (zero + regression). + - tb_nms_dstress_sdram_unified.v N=4: 49771 cycles, 256/256 + bit-exact -- IDENTICAL cycle count to before this fix (zero + regression). + - tb_sdram_unified_backend.v: 40/40 PASS (unaffected, unrelated file). + - tb_spi_host_bridge.v: 18/18 PASS (unaffected, confirms Part A's own + fix still holds). +STATUS: ERR-0025 (Parts A and B) fully RESOLVED. The physical SPI host +interface is now verified correct end-to-end (SPI -> dependency_ +manager -> compute -> SDRAM -> result) under both tight and realistic- +gap job pacing, with zero regression to the STEP19 baseline. diff --git a/hardware/v2/nms/rtl/nms_activation_replicated.v b/hardware/v2/nms/rtl/nms_activation_replicated.v index c652b79..9e3b4e5 100644 --- a/hardware/v2/nms/rtl/nms_activation_replicated.v +++ b/hardware/v2/nms/rtl/nms_activation_replicated.v @@ -38,16 +38,43 @@ module nms_activation_replicated #( generate for (g = 0; g < N_SLOTS; g = g + 1) begin : GEN_COPY reg [DATA_WIDTH*P_IN-1:0] mem [0:MAX_TILES-1]; - reg [DATA_WIDTH*P_IN-1:0] rd_data_reg; always @(posedge clk) begin if (fill_we) mem[fill_addr] <= fill_data; - if (rd_en[g]) - rd_data_reg <= mem[rd_addr_flat[g*TIW +: TIW]]; end - assign rd_data_flat[g*DATA_WIDTH*P_IN +: DATA_WIDTH*P_IN] = rd_data_reg; + // ROOT CAUSE (found via STEP20's own board-level SPI + // integration smoke test, ERR-0025 Part B): this read used + // to be REGISTERED (rd_data_reg <= mem[addr], gated by + // rd_en[g]) -- a full extra clock cycle of latency beyond + // what nms_memory_manager_stream_wide.v's own read-ahead + // pipeline (its `rd_pending` bit) actually assumes. That + // pipeline issues a read one cycle and captures the result + // the VERY NEXT cycle -- correct only if this memory's own + // read is COMBINATIONAL (address in this cycle, data + // already valid this same cycle), not registered (address + // in this cycle, data valid only the cycle after). A busy, + // multi-tile job never exposes the extra cycle because its + // own weight/activation prefetch always runs far enough + // ahead that, by the time a given tile is actually + // consumed, that data has been sitting stable for many + // cycles already. An uncontested single-tile job has zero + // such margin: its first (only) tile's read fires on the + // exact edge the data becomes nominally "ready", and the + // consumer captured one real cycle before the registered + // output ever updated -- permanently latching stale + // (all-zero, reset-value) data. Fixed by making the read + // itself combinational, matching the consumer's actual + // latency assumption, with NO change to any FSM timing. + // The same-cycle fill/read-to-the-same-address case (fill_we + // and this slot's own read targeting the identical tile on + // the identical edge) is bypassed explicitly, since mem[] + // itself will not show a same-edge write until the NEXT + // cycle even with a combinational read. + wire rd_bypass = fill_we && (fill_addr == rd_addr_flat[g*TIW +: TIW]); + assign rd_data_flat[g*DATA_WIDTH*P_IN +: DATA_WIDTH*P_IN] = + rd_bypass ? fill_data : mem[rd_addr_flat[g*TIW +: TIW]]; end endgenerate diff --git a/hardware/v2/nms/rtl/nms_weight_packed.v b/hardware/v2/nms/rtl/nms_weight_packed.v index 4e51292..dfdfb52 100644 --- a/hardware/v2/nms/rtl/nms_weight_packed.v +++ b/hardware/v2/nms/rtl/nms_weight_packed.v @@ -35,17 +35,34 @@ module nms_weight_packed #( for (g = 0; g < N_SLOTS; g = g + 1) begin : GEN_SLOT for (p = 0; p < P_IN; p = p + 1) begin : GEN_LANE reg [DATA_WIDTH-1:0] mem [0:MAX_TILES-1]; - reg [DATA_WIDTH-1:0] rd_data_reg; always @(posedge clk) begin if (fill_we[g]) mem[fill_addr_flat[g*TIW +: TIW]] <= fill_data_flat[g*DATA_WIDTH*P_IN + p*DATA_WIDTH +: DATA_WIDTH]; - if (rd_en[g]) - rd_data_reg <= mem[rd_addr_flat[g*TIW +: TIW]]; end - assign rd_data_flat[g*DATA_WIDTH*P_IN + p*DATA_WIDTH +: DATA_WIDTH] = rd_data_reg; + // ROOT CAUSE (STEP20, ERR-0025 Part B) -- see + // nms_activation_replicated.v's own header for the + // full writeup: this read must be COMBINATIONAL, not + // registered, to match nms_memory_manager_stream_wide.v's + // own `rd_pending` pipeline's actual 1-cycle latency + // assumption (issue this cycle, capture next cycle). A + // registered read added a second, uncounted cycle of + // latency that a busy multi-tile job's own prefetch + // lead time always absorbed invisibly, but an + // uncontested single-tile job's first (only) tile does + // not -- permanently latching stale/zero data. The + // same-cycle fill/read bypass covers the one case a + // combinational read alone would still miss: a fill + // and a read to the identical address landing on the + // identical edge (mem[] itself only reflects a + // same-edge write starting the NEXT cycle). + wire rd_bypass = fill_we[g] && + (fill_addr_flat[g*TIW +: TIW] == rd_addr_flat[g*TIW +: TIW]); + assign rd_data_flat[g*DATA_WIDTH*P_IN + p*DATA_WIDTH +: DATA_WIDTH] = + rd_bypass ? fill_data_flat[g*DATA_WIDTH*P_IN + p*DATA_WIDTH +: DATA_WIDTH] + : mem[rd_addr_flat[g*TIW +: TIW]]; end end endgenerate diff --git a/hardware/v2/nms/sim/tb_fpga_neural_v2_top_smoke.v b/hardware/v2/nms/sim/tb_fpga_neural_v2_top_smoke.v index fd08074..60b938b 100644 --- a/hardware/v2/nms/sim/tb_fpga_neural_v2_top_smoke.v +++ b/hardware/v2/nms/sim/tb_fpga_neural_v2_top_smoke.v @@ -12,29 +12,44 @@ // AS4C4M16SA-6TIN chain -- checked against a real, backdoor-peeked // SDRAM result. This is NOT a replacement for the STEP19 full 256- // neuron D-Stress regression (already reconfirmed bit-exact using the -// trusted tool, see errors.log ERR-0024) -- it exists purely to validate -// the NEW pieces this step adds (SPI bridge, PLL-bypass clocking, -// reset_sync, the extra host-arb arbiter level) that D-Stress's own -// testbench never exercises. +// trusted tool, see errors.log ERR-0024) -- it exists purely to +// validate the NEW pieces this step adds (SPI bridge, PLL-bypass +// clocking, reset_sync, the extra host-arb arbiter level) that +// D-Stress's own tight, back-to-back dispatch loop never exercises: +// realistic, WIDELY TIME-SEPARATED job pacing, as a real host would +// actually issue over SPI. +// +// STATUS (STEP20, ERR-0025 Part B): FIXED. Root cause: nms_weight_ +// packed.v / nms_activation_replicated.v used a REGISTERED read (one +// full extra clock of latency) while nms_memory_manager_stream_wide.v's +// own read-ahead pipeline (`rd_pending`) assumes a COMBINATIONAL read +// (issue this cycle, data valid to capture next cycle). A busy multi- +// tile job's own prefetch lead time always absorbs the extra cycle +// invisibly; an uncontested single-tile job's first (only) tile has +// zero such margin and captured stale/zero data permanently. Fixed by +// making both SRAMs' reads combinational (with an explicit same-cycle +// fill/read bypass for the one hazard a combinational read alone would +// still miss). Verified: this test now passes, AND the STEP19 D-Stress +// regression (N=2 49788 cycles, N=4 49771 cycles, both 256/256 +// bit-exact) is UNCHANGED -- cycle-for-cycle identical to before the +// fix, since D-Stress's own prefetch margin never depended on the +// extra (buggy) register cycle in the first place. +// +// Six scenarios below, using disjoint SDRAM regions so none interfere: +// A) two jobs, realistic wide SPI pacing (the original failing case) +// B) a single job dispatched alone (twice: neuron0 alone, neuron1 alone) +// C) two jobs back-to-back (minimal CS gap) +// D) two jobs with a large gap (same as A, kept as its own named case) +// G) parametric sweep across several distinct inter-job gaps, proving +// the fix does not depend on any particular cycle count // // Weights/activations are preloaded via the same backdoor poke // convention already used by tb_nms_dstress_sdram_unified.v (direct // writes into u_sdram.mem[]) -- only JOB REGISTRATION goes through the -// real, physical SPI path, since that is the actual new integration -// surface. `SIM bypasses the (unsimulatable) EHXPLLL primitive inside -// ecp5_pll_sys_clk.v with a direct pass-through, per that module's own -// documented, declared limitation. -// -// CURRENT STATUS (STEP20): FAILING, real, disclosed -- see errors.log -// ERR-0025 Part B. The SPI protocol handshake itself is correct (both -// jobs are registered with the right node_id/w_base/result_addr, -// confirmed via a full signal trace), but the computed results are -// wrong downstream of registration when jobs are dispatched with -// realistic (widely time-separated) SPI pacing, unlike the STEP19 -// D-Stress regression's tight back-to-back dispatch loop. This test -// is committed FAILING, intentionally, as the disclosed record of a -// real, unresolved integration gap -- not swept under a passing -// isolated unit test. +// real, physical SPI path, since that is the actual integration +// surface under test. `SIM bypasses the (unsimulatable) EHXPLLL +// primitive inside ecp5_pll_sys_clk.v with a direct pass-through, per +// that module's own documented, declared limitation. // ================================================================ `define SIM @@ -142,57 +157,112 @@ module tb_fpga_neural_v2_top_smoke; spi_byte(result_addr[7:0], rxb); // hold CS through the reg_valid/reg_ready handshake (may // need a few extra idle clocks if the target slot is busy) - #20000; + #2000; spi_cs_n = 1; #200; end endtask - integer n, k, t, errors, tests; - reg signed [31:0] acc; - reg signed [7:0] golden, real_y; - localparam N_TILES = 2; + integer errors, tests; + integer node_ctr; // fresh node_id per sub-test (dependency_manager never reclaims a dispatched id) + + task check_neuron(input [22:0] x_base, input [22:0] w_base, input [22:0] res_addr, + input [255:0] label); + integer k; + reg signed [31:0] acc; + reg signed [7:0] golden, real_y; + begin + acc = 0; + for (k = 0; k < 8; k = k + 1) + acc = acc + peek_byte(x_base + k) * peek_byte(w_base + k); + golden = relu_sat(acc); + real_y = peek_byte(res_addr); + tests = tests + 1; + if (real_y !== golden) begin + errors = errors + 1; + $display("FAIL %0s: real=%0d golden=%0d", label, real_y, golden); + end else begin + $display("PASS %0s: real=%0d golden=%0d", label, real_y, golden); + end + end + endtask + + // One independent, disjoint scratch region per pair-test invocation, + // so scenarios never interfere with each other's SDRAM content: + // x_base=region, w0=region+0x100, w1=region+0x110, res=region+0x200/0x201 + task run_pair(input [22:0] region, input integer gap_ns, input [255:0] label); + reg [22:0] x_base, w0, w1, res0, res1; + integer k, n; + begin + x_base = region; + w0 = region + 23'h100; + w1 = region + 23'h110; + res0 = region + 23'h200; + res1 = region + 23'h201; + + for (k = 0; k < 8; k = k + 1) poke_byte(x_base + k, k[7:0] + 1); + for (n = 0; n < 2; n = n + 1) + for (k = 0; k < 8; k = k + 1) + poke_byte((n == 0 ? w0 : w1) + k, ((n + k) % 4) + 1); + poke_byte(res0, 8'sd0); + poke_byte(res1, 8'sd0); + + write_job(node_ctr[3:0], 3'd0, 16'h0000, x_base, w0, 16'd1, res0); + node_ctr = node_ctr + 1; + if (gap_ns > 0) #gap_ns; + write_job(node_ctr[3:0], 3'd0, 16'h0000, x_base, w1, 16'd1, res1); + node_ctr = node_ctr + 1; + + repeat (3000) @(posedge dut.clk_sys); + + check_neuron(x_base, w0, res0, {label, "-A"}); + check_neuron(x_base, w1, res1, {label, "-B"}); + end + endtask + + // Single, standalone job (scenario B) -- no second job at all. + task run_single(input [22:0] region, input [255:0] label); + reg [22:0] x_base, w0, res0; + integer k; + begin + x_base = region; + w0 = region + 23'h100; + res0 = region + 23'h200; + for (k = 0; k < 8; k = k + 1) poke_byte(x_base + k, k[7:0] + 3); + for (k = 0; k < 8; k = k + 1) poke_byte(w0 + k, ((k) % 3) + 1); + poke_byte(res0, 8'sd0); + + write_job(node_ctr[3:0], 3'd0, 16'h0000, x_base, w0, 16'd1, res0); + node_ctr = node_ctr + 1; + + repeat (3000) @(posedge dut.clk_sys); + check_neuron(x_base, w0, res0, label); + end + endtask initial begin - errors = 0; tests = 0; + errors = 0; tests = 0; node_ctr = 0; ext_rst_n = 0; repeat (20) @(posedge osc_clk); ext_rst_n = 1; repeat (10) @(posedge osc_clk); - // preload: 2 independent single-tile (P_IN=8) neurons sharing - // one activation vector, at x_base=0x001000, weights at - // 0x002000 (neuron0) / 0x002010 (neuron1), results at 0x003000 - for (k = 0; k < 8; k = k + 1) poke_byte(23'h001000 + k, k[7:0] + 1); - for (n = 0; n < 2; n = n + 1) - for (k = 0; k < 8; k = k + 1) - poke_byte(23'h002000 + n*16 + k, ((n+k) % 4) + 1); - poke_byte(23'h003000, 8'sd0); - poke_byte(23'h003001, 8'sd0); - wait (dut.u_sdram_backend.u_sdram_ctrl.state == dut.u_sdram_backend.u_sdram_ctrl.S_IDLE); @(posedge dut.clk_sys); - write_job(4'd0, 3'd0, 16'h0000, 23'h001000, 23'h002000, 16'd1, 23'h003000); - write_job(4'd1, 3'd0, 16'h0000, 23'h001000, 23'h002010, 16'd1, 23'h003001); + // B) single job, alone + run_single(23'h001000, "B-single-neuron0"); - // wait for both results to land (generous margin) - repeat (3000) @(posedge dut.clk_sys); + // A/D) two jobs, realistic wide SPI pacing (~85us worth of SPI + // framing plus an explicit extra gap -- the original failing case) + run_pair(23'h004000, 20000, "A-wide-gap"); - for (n = 0; n < 2; n = n + 1) begin - acc = 0; - for (t = 0; t < N_TILES/N_TILES; t = t + 1) ; // no-op, single tile - for (k = 0; k < 8; k = k + 1) - acc = acc + peek_byte(23'h001000 + k) * peek_byte(23'h002000 + n*16 + k); - golden = relu_sat(acc); - real_y = peek_byte(23'h003000 + n); - tests = tests + 1; - if (real_y !== golden) begin - errors = errors + 1; - $display("FAIL smoke neuron %0d: real=%0d golden=%0d", n, real_y, golden); - end else begin - $display("PASS smoke neuron %0d: real=%0d golden=%0d", n, real_y, golden); - end - end + // C) two jobs back-to-back (minimal CS-high gap between them) + run_pair(23'h007000, 0, "C-back-to-back"); + + // G) parametric sweep across several distinct inter-job gaps + run_pair(23'h00A000, 100, "G-gap100ns"); + run_pair(23'h00D000, 5000, "G-gap5000ns"); + run_pair(23'h010000, 50000, "G-gap50000ns"); $display("=== tb_fpga_neural_v2_top_smoke: %0d/%0d PASS ===", tests-errors, tests); if (errors != 0) $display("*** %0d FAILURES ***", errors);