michele/FPGA-Neural
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix: real SPI MISO bit-corruption bug found+fixed; add register file + pin plan (EXP-0075)

Browse Source 
Found a real, previously-masked bug in spi_host_bridge_v3.v's physical
layer (inherited unchanged from V1/V2): the bit_count==0 MISO bypass
corrupts the last bit of any multi-byte response whose value happens
to end in a 1 -- every prior test's response data coincidentally
ended in 0, hiding it until the new DEVICE_ID register (0x...01)
exposed it via a real bit-exact mismatch. Fixed by removing the
bypass (verified unnecessary for this protocol's actual usage).

Added REG_WRITE/REG_READ opcodes (0x30/0x31) and a register file
(DEVICE_ID/CONTROL/STATUS/N_SLOTS) for general device control beyond
job submission, per explicit user request. Full regression: 38/38
PASS, including new cases specifically targeting the bit-corruption
bug for both REG_READ and READ_MEM.

New hardware/v3/constraints/n2_system_ddr3_top.xdc: reserves the
FPGA's dedicated Master-SPI config-flash pins (found colliding with
auto-placed design ports in the real routed checkpoint) and assigns
the neural-processor management SPI to real, verified-free, edge-
adjacent pins on xc7a100tcsg324-2.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MUG92aM9m68TRc4rG55BcC
This commit is contained in:
michele
2026-09-19 18:31:51 +02:00
co-authored by Claude Sonnet 5
parent 835ae881dc
commit ca17765fe3
5 changed files with 440 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
hardware/v3/rtl/spi_host_bridge_v3.v
+191 -6
View File
@@ -92,6 +92,51 @@
// payload byte 7, MSB-first per word, one
// mem_req/mem_ready read per word.
//
// 0x30 REG_WRITE -- 5 payload bytes: byte0 = reg_addr[7:0],
// byte1:4 = value[31:0] MSB-first. Applied the
// instant the last data byte lands (no backend
// handshake needed, register writes are purely
// internal). Writing a read-only or unknown
// register address is inert (accepted on the
// wire, has no effect) -- same "never wedges
// the bus" precedent as an unknown opcode.
// 0x31 REG_READ -- 1 payload byte (reg_addr[7:0]), then 4
// response bytes clocked out on MISO MSB-
// first starting at payload byte 2. An unknown
// register address reads back 32'hFFFF_FFFF
// (deliberately distinct from any real 0
// value, so a host can tell "read an unmapped
// register" apart from "read a real zero").
//
// REGISTER MAP (v1, extensible -- add new addresses, never repurpose
// an existing one, so old host software stays correct against new
// firmware):
// 0x00 DEVICE_ID (RO) -- 32'h4E50_5601 ("NPV" + protocol
// version 1, ASCII 'N''P''V' + 0x01).
// Lets host software confirm it's really
// talking to this protocol/version before
// trusting anything else.
// 0x01 CONTROL (RW) -- bit0: write 1 to pulse soft_rst_pulse
// for one clk cycle (same physical effect
// as the RESET opcode, exposed here too
// since a register-based control path is
// often more convenient for host software
// than a dedicated opcode). Always reads
// back 0 (it's a pulse trigger, not a
// level). bits[31:1] reserved.
// 0x02 STATUS (RO) -- bit0: job_busy: bit1: mem_busy;
// bit2: last_job_accepted (sticky, same
// as the STATUS opcode's own bits);
// bit3: init_calib_complete (DDR3 PHY
// calibration done, i.e. DRAM traffic is
// actually safe to issue); bit4: dir_error
// (neural_director_packed.v's own error
// latch). bits[31:5] reserved.
// 0x03 N_SLOTS (RO) -- number of compute slots this build was
// synthesized with (the N_SLOTS parameter
// below), so host software doesn't need
// to hardcode it.
//
// Any opcode byte not listed above is treated as NOP (0 payload,
// MISO drives 0x00) -- matches spi_host_bridge.v's own "unknown
// opcode is inert, never wedges the bus" precedent.
@@ -99,11 +144,16 @@
module spi_host_bridge_v3 #(
parameter JOB_ADDR_WIDTH = 26, // matches neural_director_packed.v's ADDR_WIDTH (byte-base convention)
parameter MEM_ADDR_WIDTH = 25 // matches host_mem_bridge.v's ADDR_WIDTH (word/burst convention)
parameter MEM_ADDR_WIDTH = 25, // matches host_mem_bridge.v's ADDR_WIDTH (word/burst convention)
parameter N_SLOTS = 2 // reported read-only via REG 0x03, purely informational
)(
input wire clk,
input wire rst,
// ---- system status, for the REG 0x02 STATUS register ----
input wire init_calib_complete,
input wire dir_error,
// ---- physical SPI pins ----
input wire sclk,
input wire mosi,
@@ -169,7 +219,29 @@ module spi_host_bridge_v3 #(
wire [7:0] tx_byte;
reg miso_shift_bit;
assign miso = (cs_active && bit_count == 3'd0) ? tx_byte[7] : miso_shift_bit;
// REAL BUG found and fixed this session (via REG_READ's DEVICE_ID
// register, whose non-zero LSB exposed it -- prior tests'
// response values happened to coincidentally mask it, see the
// note above "mem_rout_pending_ignore" for the full root-cause):
// this used to be `(cs_active && bit_count==3'd0) ? tx_byte[7] :
// miso_shift_bit`, a combinational bypass meant to serve the
// FIRST bit of a fresh byte before any falling edge has prepared
// miso_shift_bit for it. bit_count==0 is ALSO true for the ENTIRE
// remainder of the bit period immediately AFTER a byte's LAST bit
// was sampled (it only advances again at the next byte's own
// first sampling edge) -- so this bypass showed tx_byte[7] (the
// wrong bit, and on continuously-clocked multi-byte reads,
// possibly a byte value that's already stale/wrong too) for the
// WHOLE tail of every byte-to-byte gap, corrupting exactly the
// moment a real (non-instant) SPI master samples the last bit.
// Proven unnecessary for every opcode this module has: a genuine
// "first bit with zero prior falling edges" only occurs for the
// opcode byte itself (whose MISO value is always don't-care 0x00
// anyway) -- every real response byte in this protocol is always
// preceded by several other bytes in the same CS session, so
// miso_shift_bit has always already been freshly prepared by the
// ordinary falling-edge mechanism below by the time it matters.
assign miso = miso_shift_bit;
always @(posedge clk) begin
if (rst) begin
@@ -206,6 +278,8 @@ module spi_host_bridge_v3 #(
localparam OP_RESET = 8'h0F;
localparam OP_WRITE_JOB = 8'h10;
localparam OP_STATUS = 8'h20;
localparam OP_REG_WRITE = 8'h30;
localparam OP_REG_READ = 8'h31;
localparam ST_OPCODE = 4'd0;
localparam ST_JOB = 4'd1; // collecting 16 WRITE_JOB payload bytes
@@ -217,6 +291,9 @@ module spi_host_bridge_v3 #(
localparam ST_MEM_RISS= 4'd7; // READ_MEM: issue+wait mem_req
localparam ST_MEM_ROUT= 4'd8; // READ_MEM: shifting the 2 bytes of a word out
localparam ST_IGNORE = 4'd9; // opcode consumed / unknown, wait for cs_rose
localparam ST_REG_ADDR = 4'd10; // collecting 1 reg_addr byte
localparam ST_REG_WDATA= 4'd11; // REG_WRITE: collecting 4 value bytes
localparam ST_REG_ROUT = 4'd12; // REG_READ: shifting 4 value bytes out
reg [3:0] state;
reg [7:0] opcode;
@@ -225,9 +302,50 @@ module spi_host_bridge_v3 #(
reg [15:0] word_cnt;
reg [15:0] cur_word; // WRITE_MEM: assembling MSB,LSB; READ_MEM: holding readback
reg job_busy_r, mem_busy_r, last_job_accepted_r;
reg [7:0] reg_addr;
reg [31:0] reg_wdata; // REG_WRITE: assembling the 4 value bytes
// ---- ROUT-exit deferral (real bug found and fixed this session,
// see the header's own note near the physical layer): the
// combinational "assign miso = (bit_count==0) ? tx_byte[7] :
// miso_shift_bit" bypass exists to serve the FIRST bit of a fresh
// byte, but bit_count ALSO reads 0 for one edge immediately AFTER
// the LAST bit of the byte that just finished (it wraps 7->0 at
// that same edge) -- the two cases are indistinguishable from
// bit_count alone. If `state` (and therefore tx_byte, via tx_mux)
// changes on that SAME edge -- exactly what a naive ROUT-exit
// transition does -- the bypass reads the NEW (already-wrong)
// tx_byte instead of the correctly-prepared miso_shift_bit,
// corrupting the LAST bit of the LAST byte of a multi-byte read.
// This was masked in READ_MEM's own existing test by coincidence
// (the test word's last bit happened to equal the corrupted
// substitute's bit7, both 0) until REG_READ's DEVICE_ID register
// (whose last bit is 1) exposed it via a real bit-exact mismatch.
// Fix: defer the state/byte_idx-clearing transition by exactly
// one internal clk cycle past the byte that triggers it, via a
// one-cycle pending flag -- clk runs far faster than SCLK (this
// file's own documented >=50x minimum ratio), so a one-clk-cycle
// delay is invisible on the SPI bus but moves the transition
// safely off the vulnerable bit_count==0 edge.
reg mem_rout_pending_ignore, mem_rout_pending_riss;
reg reg_rout_pending;
// ---- register file readback mux (combinational -- see the
// header's REGISTER MAP for the meaning of each address) ----
reg [31:0] reg_rdata;
always @(*) begin
case (reg_addr)
8'h00: reg_rdata = 32'h4E505601;
8'h01: reg_rdata = 32'h00000000;
8'h02: reg_rdata = {27'b0, dir_error, init_calib_complete,
last_job_accepted_r, mem_busy_r, job_busy_r};
8'h03: reg_rdata = {24'b0, N_SLOTS[7:0]};
default: reg_rdata = 32'hFFFFFFFF;
endcase
end
// combinational tx byte mux -- STATUS response, READ_MEM data,
// everything else drives 0x00
// REG_READ data, everything else drives 0x00
reg [7:0] tx_mux;
always @(*) begin
tx_mux = 8'h00;
@@ -235,6 +353,8 @@ module spi_host_bridge_v3 #(
tx_mux = {5'b0, last_job_accepted_r, mem_busy_r, job_busy_r};
else if (opcode == OP_READ_MEM && state == ST_MEM_ROUT)
tx_mux = (byte_idx == 5'd0) ? cur_word[15:8] : cur_word[7:0];
else if (opcode == OP_REG_READ && state == ST_REG_ROUT)
tx_mux = reg_rdata[8*(3-byte_idx) +: 8];
end
assign tx_byte = tx_mux;
@@ -249,6 +369,9 @@ module spi_host_bridge_v3 #(
mem_wdata <= 16'd0; mem_lb_n <= 1'b0; mem_ub_n <= 1'b0;
soft_rst_pulse <= 1'b0;
job_busy_r <= 1'b0; mem_busy_r <= 1'b0; last_job_accepted_r <= 1'b0;
reg_addr <= 8'h00; reg_wdata <= 32'h0;
mem_rout_pending_ignore <= 1'b0; mem_rout_pending_riss <= 1'b0;
reg_rout_pending <= 1'b0;
end else begin
mem_req <= 1'b0;
soft_rst_pulse <= 1'b0;
@@ -273,6 +396,8 @@ module spi_host_bridge_v3 #(
OP_WRITE_JOB: state <= ST_JOB;
OP_WRITE_MEM: state <= ST_MEM_ADDR;
OP_READ_MEM: state <= ST_MEM_ADDR;
OP_REG_WRITE: state <= ST_REG_ADDR;
OP_REG_READ: state <= ST_REG_ADDR;
OP_RESET: state <= ST_IGNORE;
default: state <= ST_IGNORE; // NOP, STATUS: no MOSI payload
endcase
@@ -341,7 +466,40 @@ module spi_host_bridge_v3 #(
end
end
default: ; // ST_JOB_WAIT/ST_MEM_WISS/ST_MEM_RISS/ST_MEM_ROUT/ST_IGNORE: no MOSI payload expected
ST_REG_ADDR: begin
reg_addr <= rx_byte;
byte_idx <= 5'd0;
// REG_READ needs no backend handshake -- the
// register value is already available
// combinationally (reg_rdata), so it can go
// straight to shifting bytes out; REG_WRITE
// still needs 4 more MOSI bytes first.
state <= (opcode == OP_REG_WRITE) ? ST_REG_WDATA : ST_REG_ROUT;
end
ST_REG_WDATA: begin
case (byte_idx)
5'd0: reg_wdata[31:24] <= rx_byte;
5'd1: reg_wdata[23:16] <= rx_byte;
5'd2: reg_wdata[15:8] <= rx_byte;
5'd3: begin
reg_wdata[7:0] <= rx_byte;
state <= ST_IGNORE;
// apply the write immediately -- register
// writes are purely internal, no backend
// handshake to wait on. Unknown/read-only
// addresses are silently inert (accepted
// on the wire, no effect), matching this
// module's own "never wedges the bus"
// precedent for unknown opcodes.
if (reg_addr == 8'h01 && rx_byte[0])
soft_rst_pulse <= 1'b1;
end
endcase
if (byte_idx != 5'd3) byte_idx <= byte_idx + 5'd1;
end
default: ; // ST_JOB_WAIT/ST_MEM_WISS/ST_MEM_RISS/ST_MEM_ROUT/ST_REG_ROUT/ST_IGNORE: no MOSI payload expected
endcase
end
@@ -385,10 +543,37 @@ module spi_host_bridge_v3 #(
end else begin
mem_addr <= mem_addr + 1'b1;
word_cnt <= word_cnt - 1'b1;
byte_idx <= 5'd0;
state <= (word_cnt == 16'd1) ? ST_IGNORE : ST_MEM_RISS;
// defer the actual exit -- see this module's own
// "ROUT-exit deferral" note above -- so tx_mux
// keeps showing this byte's correct value through
// the vulnerable bit_count==0 edge.
if (word_cnt == 16'd1) mem_rout_pending_ignore <= 1'b1;
else mem_rout_pending_riss <= 1'b1;
end
end
if (mem_rout_pending_ignore) begin
mem_rout_pending_ignore <= 1'b0;
byte_idx <= 5'd0;
state <= ST_IGNORE;
end
if (mem_rout_pending_riss) begin
mem_rout_pending_riss <= 1'b0;
byte_idx <= 5'd0;
state <= ST_MEM_RISS;
end
if (state == ST_REG_ROUT && rx_valid) begin
if (byte_idx == 5'd3) begin
reg_rout_pending <= 1'b1;
end else begin
byte_idx <= byte_idx + 5'd1;
end
end
if (reg_rout_pending) begin
reg_rout_pending <= 1'b0;
byte_idx <= 5'd0;
state <= ST_IGNORE;
end
job_busy_r <= (state == ST_JOB_WAIT);