Skip to content

Commit db4a6f3

Browse files
committed
Fix snapshot csr_data to prevent TX PEC/bus data race
Add load_csr_data delayed pulse to capture csr_data one cycle after csr_sel updates, ensuring csr_data_next reads from the settled selector. Previously csr_data tracked live register values every clock, allowing FW writes to change tx_data_o between bus capture and PEC computation. The original gating attempt (load_csr_sel || inc_csr_sel) had a pipeline mismatch: both csr_sel and csr_data loaded on the same edge, but csr_data_next is case(csr_sel) which still held the old value. Also simplify common.mk FSDB generation by removing VERDI_PLI toggle in favor of VCS native FSDB support, and add cocotb test test_recovery_tx_pec_csr_data_race that exercises the race by toggling RECOVERY_STATUS from FW during concurrent I3C reads.
1 parent 101b6d7 commit db4a6f3

8 files changed

Lines changed: 736 additions & 16 deletions

File tree

doc/source/ccc.md

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,42 @@ limits on Private Read or Private Write transfers. See {doc}`firmware_guide` for
7878
details on firmware responsibilities.
7979
```
8080

81+
## Multi-Byte CCC Register Atomicity
82+
83+
Several Direct GET CCCs return multi-byte values. The CCC state machine
84+
transmits these byte-by-byte, selecting each byte from the source signal
85+
combinationally as it is needed. There is no snapshot or shadow register;
86+
the source is re-read on every byte. If the underlying CSR is modified
87+
mid-transfer, remaining bytes will reflect the new value while
88+
already-transmitted bytes retain the old value.
89+
90+
| CCC | Bytes | Source | Updated By |
91+
|-----|-------|--------|------------|
92+
| GETBCR | 1 | `STBY_CR_DEVICE_CHAR.BCR_*` | FW |
93+
| GETDCR | 1 | `STBY_CR_DEVICE_CHAR.DCR` | FW |
94+
| GETSTATUS | 2 | `TTI.INTERRUPT_STATUS`, `TTI.STATUS` | HW |
95+
| GETMWL | 2 | Internal flop | SETMWL CCC |
96+
| GETMRL | 3 | Internal flop | SETMRL CCC |
97+
| GETPID | 6 | `STBY_CR_DEVICE_CHAR.PID_HI` + `STBY_CR_DEVICE_PID_LO.PID_LO` | FW |
98+
99+
**GETPID**: The 48-bit PID is assembled combinationally from two CSR
100+
registers (`STBY_CR_DEVICE_CHAR` and `STBY_CR_DEVICE_PID_LO`). If firmware
101+
writes either register while a GETPID CCC is in progress, the byte being
102+
transmitted in that cycle and all subsequent bytes will use the new register
103+
value. Firmware should write both PID registers before the Target receives
104+
a dynamic address (before ENTDAA / SETDASA / SETAASA), since GETPID is
105+
only issued after address assignment.
106+
107+
**GETMWL / GETMRL**: These read internal flops that are only written by the
108+
SETMWL / SETMRL CCC handler. Since only one CCC can be active on the bus at
109+
a time, GET and SET cannot overlap. These registers are read-only to firmware
110+
(`sw = r`).
111+
112+
For SET CCCs, **SETMRL** commits MRL (bytes 0-1) and IBIL (byte 2) to
113+
`STBY_CR_MRL` on separate clock cycles. A firmware read of `STBY_CR_MRL`
114+
between those two updates sees the new MRL with the stale IBIL. **SETMWL**
115+
commits both MWL bytes to `STBY_CR_MWL` in a single cycle.
116+
81117
## CCC Error Handling
82118

83119
The core detects errors during CCC processing:

src/recovery/recovery_receiver.sv

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -353,8 +353,9 @@ module recovery_receiver
353353
logic capture_pec;
354354
logic set_cmd_is_rd;
355355
logic latch_pec_from_len;
356-
logic load_csr_sel;
357-
logic inc_csr_sel;
356+
logic load_csr_sel; // Loads csr_sel/csr_end (CmdDispatch)
357+
logic inc_csr_sel; // Advances csr_sel at DWORD boundaries
358+
logic load_csr_data; // Delayed pulse: captures csr_data 1 cycle after csr_sel settles
358359
logic load_csr_length;
359360

360361
//----------------------------------------------------------------------------
@@ -1210,11 +1211,16 @@ module recovery_receiver
12101211
if (!rst_ni) begin
12111212
csr_sel <= CSR_INVALID;
12121213
csr_end <= CSR_INVALID;
1214+
load_csr_data <= '0;
12131215
end else if (load_csr_sel) begin
12141216
csr_sel <= csr_sel_next;
12151217
csr_end <= csr_end_next;
1218+
load_csr_data <= 1'b1;
12161219
end else if (inc_csr_sel && (csr_sel < csr_end)) begin
12171220
csr_sel <= csr_e'(csr_sel + 8'd1);
1221+
load_csr_data <= 1'b1;
1222+
end else begin
1223+
load_csr_data <= '0;
12181224
end
12191225
end
12201226

@@ -1438,10 +1444,12 @@ module recovery_receiver
14381444
endcase
14391445
end
14401446

1447+
// Capture csr_data one cycle after csr_sel settles to ensure
1448+
// csr_data_next (driven by case(csr_sel)) sees the correct selector.
14411449
always_ff @(posedge clk_i or negedge rst_ni) begin
14421450
if (!rst_ni) begin
14431451
csr_data <= '0;
1444-
end else begin
1452+
end else if (load_csr_data) begin
14451453
csr_data <= csr_data_next;
14461454
end
14471455
end

verification/cocotb/common.mk

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,21 +50,13 @@ ifeq ($(SIM), verilator)
5050
EXTRA_ARGS += -Wno-DECLFILENAME -Wno-TIMESCALEMOD
5151
endif
5252

53-
# Switch between Verdi FSDB PLI and "classic" waveform dumping
54-
VERDI_PLI ?= 1
55-
5653
ifeq ($(SIM), vcs)
5754
COMPILE_ARGS += -assert svaext
5855
COMPILE_ARGS += -Xcflags='-Wno-error=implicit-function-declaration -Wno-error=int-conversion'
5956
COMPILE_ARGS += -kdb
60-
ifeq ($(VERDI_PLI), 1)
61-
COMPILE_ARGS += -P $(VERDI_HOME)/share/PLI/VCS/LINUX64/novas.tab $(VERDI_HOME)/share/PLI/VCS/LINUX64/pli.a
62-
else
63-
COMPILE_ARGS += -debug_access+all +vcs+fsdbon
64-
endif
57+
COMPILE_ARGS += -debug_access+all +vcs+fsdbon
6558
ifeq ($(WAVES), 1)
66-
# Sim args seem to work for both wave dumping types (?)
67-
SIM_ARGS += +fsdbfile+dump.fsdb +fsdb+all=on +fsdb+mda=on
59+
SIM_ARGS += +fsdbfile+dump.fsdb +fsdb+all=on +fsdb+mda=on
6860
endif
6961
EXTRA_ARGS += +vcs+lic+wait
7062

verification/cocotb/top/lib_i3c_top/common.py

Lines changed: 234 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
# SPDX-License-Identifier: Apache-2.0
22
"""Shared constants and helpers for I3C Target cocotb tests."""
33

4+
import random
5+
46
import cocotb
57
from cocotb.triggers import Timer
68

7-
# Canonical I3C address list — single source of truth
9+
from ccc import CCC
10+
11+
# Canonical I3C address list -- single source of truth
812
# 0x23 is excluded: it is hardcoded as the I3CTarget sim model address
913
# and selecting it for DUT addresses causes bus contention.
1014
VALID_I3C_ADDRESSES = (
@@ -28,3 +32,232 @@ def log_seed(dut):
2832
"""Log the random seed for reproducibility."""
2933
seed = cocotb.plusargs.get("seed", None)
3034
dut._log.info(f"Random seed: {seed or 'unknown (set via RANDOM_SEED plusarg)'}")
35+
36+
37+
# =============================================================================
38+
# CCC helpers
39+
#
40+
# Generic wrappers around common CCC commands. Each function takes an
41+
# i3c_controller and target address(es) explicitly so they can be reused
42+
# from any test file.
43+
#
44+
# GET helpers return the raw data bytes (bytearray) and assert ACK.
45+
# SET helpers return True/False indicating whether the target ACKed.
46+
# All helpers log the CCC name, address, and returned data.
47+
# =============================================================================
48+
49+
# Valid event-enable bit patterns for ENEC / DISEC
50+
ENEC_DISEC_PATTERNS = [0x01, 0x02, 0x08, 0x03, 0x09, 0x0A, 0x0B]
51+
52+
53+
async def do_getstatus(i3c_controller, addr):
54+
"""Send directed GETSTATUS. Returns 2-byte status (bytearray)."""
55+
responses = await i3c_controller.i3c_ccc_read(
56+
ccc=CCC.DIRECT.GETSTATUS, addr=addr, count=2)
57+
ack, data = responses[0]
58+
assert ack, f"GETSTATUS NACK addr=0x{addr:02X}"
59+
cocotb.log.info(f"GETSTATUS addr=0x{addr:02X} -> {data.hex()}")
60+
return data
61+
62+
63+
async def do_getbcr(i3c_controller, addr):
64+
"""Send directed GETBCR. Returns 1-byte BCR (bytearray)."""
65+
responses = await i3c_controller.i3c_ccc_read(
66+
ccc=CCC.DIRECT.GETBCR, addr=addr, count=1)
67+
ack, data = responses[0]
68+
assert ack, f"GETBCR NACK addr=0x{addr:02X}"
69+
cocotb.log.info(f"GETBCR addr=0x{addr:02X} -> {data.hex()}")
70+
return data
71+
72+
73+
async def do_getdcr(i3c_controller, addr):
74+
"""Send directed GETDCR. Returns 1-byte DCR (bytearray)."""
75+
responses = await i3c_controller.i3c_ccc_read(
76+
ccc=CCC.DIRECT.GETDCR, addr=addr, count=1)
77+
ack, data = responses[0]
78+
assert ack, f"GETDCR NACK addr=0x{addr:02X}"
79+
cocotb.log.info(f"GETDCR addr=0x{addr:02X} -> {data.hex()}")
80+
return data
81+
82+
83+
async def do_getmwl(i3c_controller, addr):
84+
"""Send directed GETMWL. Returns 2-byte MWL (bytearray)."""
85+
responses = await i3c_controller.i3c_ccc_read(
86+
ccc=CCC.DIRECT.GETMWL, addr=addr, count=2)
87+
ack, data = responses[0]
88+
assert ack, f"GETMWL NACK addr=0x{addr:02X}"
89+
cocotb.log.info(f"GETMWL addr=0x{addr:02X} -> {data.hex()}")
90+
return data
91+
92+
93+
async def do_getmrl(i3c_controller, addr):
94+
"""Send directed GETMRL. Returns 3-byte MRL (bytearray)."""
95+
responses = await i3c_controller.i3c_ccc_read(
96+
ccc=CCC.DIRECT.GETMRL, addr=addr, count=3)
97+
ack, data = responses[0]
98+
assert ack, f"GETMRL NACK addr=0x{addr:02X}"
99+
cocotb.log.info(f"GETMRL addr=0x{addr:02X} -> {data.hex()}")
100+
return data
101+
102+
103+
async def do_getpid(i3c_controller, addr):
104+
"""Send directed GETPID. Returns 6-byte PID (bytearray)."""
105+
responses = await i3c_controller.i3c_ccc_read(
106+
ccc=CCC.DIRECT.GETPID, addr=addr, count=6)
107+
ack, data = responses[0]
108+
assert ack, f"GETPID NACK addr=0x{addr:02X}"
109+
cocotb.log.info(f"GETPID addr=0x{addr:02X} -> {data.hex()}")
110+
return data
111+
112+
113+
async def do_getcaps(i3c_controller, addr):
114+
"""Send directed GETCAPS (no defining byte, 3 bytes). Returns bytearray."""
115+
responses = await i3c_controller.i3c_ccc_read(
116+
ccc=CCC.DIRECT.GETCAPS, addr=addr, count=3)
117+
ack, data = responses[0]
118+
assert ack, f"GETCAPS NACK addr=0x{addr:02X}"
119+
cocotb.log.info(f"GETCAPS addr=0x{addr:02X} -> {data.hex()}")
120+
return data
121+
122+
123+
async def do_setmwl_direct(i3c_controller, addr, val=None):
124+
"""Send directed SETMWL. Returns True if ACKed, False otherwise."""
125+
if val is None:
126+
val = random.randint(0, 0xFFFF)
127+
acks = await i3c_controller.i3c_ccc_write(
128+
ccc=CCC.DIRECT.SETMWL,
129+
directed_data=[(addr, [(val >> 8) & 0xFF, val & 0xFF])])
130+
ok = bool(acks and acks[0])
131+
cocotb.log.info(f"SETMWL direct addr=0x{addr:02X} val=0x{val:04X} ack={ok}")
132+
return ok
133+
134+
135+
async def do_setmrl_direct(i3c_controller, addr, val=None, ibil=None):
136+
"""Send directed SETMRL. Returns True if ACKed, False otherwise."""
137+
if val is None:
138+
val = random.randint(0, 0xFFFF)
139+
if ibil is None:
140+
ibil = random.randint(0, 0xFF)
141+
acks = await i3c_controller.i3c_ccc_write(
142+
ccc=CCC.DIRECT.SETMRL,
143+
directed_data=[(addr, [(val >> 8) & 0xFF, val & 0xFF, ibil])])
144+
ok = bool(acks and acks[0])
145+
cocotb.log.info(
146+
f"SETMRL direct addr=0x{addr:02X} mrl=0x{val:04X} "
147+
f"ibil=0x{ibil:02X} ack={ok}")
148+
return ok
149+
150+
151+
async def do_enec_direct(i3c_controller, addr, pattern=None):
152+
"""Send directed ENEC. Returns True if ACKed, False otherwise."""
153+
if pattern is None:
154+
pattern = random.choice(ENEC_DISEC_PATTERNS)
155+
acks = await i3c_controller.i3c_ccc_write(
156+
ccc=CCC.DIRECT.ENEC, directed_data=[(addr, [pattern])])
157+
ok = bool(acks and acks[0])
158+
cocotb.log.info(f"ENEC direct addr=0x{addr:02X} pattern=0x{pattern:02X} ack={ok}")
159+
return ok
160+
161+
162+
async def do_disec_direct(i3c_controller, addr, pattern=None):
163+
"""Send directed DISEC. Returns True if ACKed, False otherwise."""
164+
if pattern is None:
165+
pattern = random.choice(ENEC_DISEC_PATTERNS)
166+
acks = await i3c_controller.i3c_ccc_write(
167+
ccc=CCC.DIRECT.DISEC, directed_data=[(addr, [pattern])])
168+
ok = bool(acks and acks[0])
169+
cocotb.log.info(
170+
f"DISEC direct addr=0x{addr:02X} pattern=0x{pattern:02X} ack={ok}")
171+
return ok
172+
173+
174+
async def do_rstact_direct(i3c_controller, addr, defining_byte=None):
175+
"""Send directed RSTACT. Returns True if ACKed, False otherwise."""
176+
if defining_byte is None:
177+
defining_byte = random.choice([0x00, 0x01, 0x02])
178+
acks = await i3c_controller.i3c_ccc_write(
179+
ccc=CCC.DIRECT.RSTACT, defining_byte=defining_byte,
180+
directed_data=[(addr, [])], stop=True)
181+
ok = bool(acks and acks[0])
182+
cocotb.log.info(
183+
f"RSTACT direct addr=0x{addr:02X} db=0x{defining_byte:02X} ack={ok}")
184+
return ok
185+
186+
187+
async def do_setmwl_bcast(i3c_controller, val=None):
188+
"""Send broadcast SETMWL. Returns True (broadcast has no per-target ACK)."""
189+
if val is None:
190+
val = random.randint(0, 0xFFFF)
191+
await i3c_controller.i3c_ccc_write(
192+
ccc=CCC.BCAST.SETMWL,
193+
broadcast_data=[(val >> 8) & 0xFF, val & 0xFF])
194+
cocotb.log.info(f"SETMWL bcast val=0x{val:04X}")
195+
return True
196+
197+
198+
async def do_setmrl_bcast(i3c_controller, val=None, ibil=None):
199+
"""Send broadcast SETMRL. Returns True (broadcast has no per-target ACK)."""
200+
if val is None:
201+
val = random.randint(0, 0xFFFF)
202+
if ibil is None:
203+
ibil = random.randint(0, 0xFF)
204+
await i3c_controller.i3c_ccc_write(
205+
ccc=CCC.BCAST.SETMRL,
206+
broadcast_data=[(val >> 8) & 0xFF, val & 0xFF, ibil])
207+
cocotb.log.info(f"SETMRL bcast mrl=0x{val:04X} ibil=0x{ibil:02X}")
208+
return True
209+
210+
211+
async def do_enec_bcast(i3c_controller, pattern=None):
212+
"""Send broadcast ENEC. Returns True (broadcast has no per-target ACK)."""
213+
if pattern is None:
214+
pattern = random.choice(ENEC_DISEC_PATTERNS)
215+
await i3c_controller.i3c_ccc_write(
216+
ccc=CCC.BCAST.ENEC, broadcast_data=[pattern])
217+
cocotb.log.info(f"ENEC bcast pattern=0x{pattern:02X}")
218+
return True
219+
220+
221+
async def do_disec_bcast(i3c_controller, pattern=None):
222+
"""Send broadcast DISEC. Returns True (broadcast has no per-target ACK)."""
223+
if pattern is None:
224+
pattern = random.choice(ENEC_DISEC_PATTERNS)
225+
await i3c_controller.i3c_ccc_write(
226+
ccc=CCC.BCAST.DISEC, broadcast_data=[pattern])
227+
cocotb.log.info(f"DISEC bcast pattern=0x{pattern:02X}")
228+
return True
229+
230+
231+
def build_ccc_stress_table(i3c_controller, targets):
232+
"""Build a weighted CCC command table for stress testing.
233+
234+
Returns a list of zero-argument coroutine functions. Each picks a random
235+
target from *targets* (for directed CCCs) and sends one CCC.
236+
SET CCCs appear twice for higher weight (more race opportunity).
237+
"""
238+
get_table = [
239+
lambda c=i3c_controller, t=targets: do_getstatus(c, random.choice(t)),
240+
lambda c=i3c_controller, t=targets: do_getbcr(c, random.choice(t)),
241+
lambda c=i3c_controller, t=targets: do_getdcr(c, random.choice(t)),
242+
lambda c=i3c_controller, t=targets: do_getmwl(c, random.choice(t)),
243+
lambda c=i3c_controller, t=targets: do_getmrl(c, random.choice(t)),
244+
lambda c=i3c_controller, t=targets: do_getpid(c, random.choice(t)),
245+
lambda c=i3c_controller, t=targets: do_getcaps(c, random.choice(t)),
246+
]
247+
248+
set_directed = [
249+
lambda c=i3c_controller, t=targets: do_setmwl_direct(c, random.choice(t)),
250+
lambda c=i3c_controller, t=targets: do_setmrl_direct(c, random.choice(t)),
251+
lambda c=i3c_controller, t=targets: do_enec_direct(c, random.choice(t)),
252+
lambda c=i3c_controller, t=targets: do_disec_direct(c, random.choice(t)),
253+
lambda c=i3c_controller, t=targets: do_rstact_direct(c, random.choice(t)),
254+
]
255+
256+
set_bcast = [
257+
lambda c=i3c_controller: do_setmwl_bcast(c),
258+
lambda c=i3c_controller: do_setmrl_bcast(c),
259+
lambda c=i3c_controller: do_enec_bcast(c),
260+
lambda c=i3c_controller: do_disec_bcast(c),
261+
]
262+
263+
return get_table + set_directed * 2 + set_bcast * 2

0 commit comments

Comments
 (0)