# I2C slave debug retrospective: six bugs, two fixes, and a reusable verification method A supplementary I2C slave simulation found six bugs, the worst hidden by a testbench acting as a friendly master. This note gives the diagnosis, margin accounting and re-verification matrix. ## 1. Bottom line The device under test is an I2C slave that oversamples SCL/SDA with a high-frequency clock (~70 MHz) and locates its sampling instants through four stages of synchroniser delay compensation. The original simulation "passed completely". Supplementary simulation produced six problems and two fixes: | ID | Problem | Mechanism in one line | Severity | Status | | -- | ---------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------- | --------------------------------------- | | A | The write path fails completely with a real master | The R/W bit is sampled from the raw input `sda_i`; the sampling instant lands about 112 ns after the SCL falling edge (during SCL low) | High (fails with any compliant master) | Fixed (4 sites now use `sda_i_sync_d4`) | | B | An address that is not its own is still ACKed | `sda_oe` drives unconditionally on the 8th address bit | High | Fixed (`dev_chk_pass` gating added) | | C | The read address after a repeated START is not checked | Only `i2c_ack` is tested, not `dev_chk_pass` → a foreign address is ACKed and data is driven | High (bus conflict) | Open | | D | Repeated START + write address seizes the bus | `RESTART` jumps to `SLV_ADDR_RD` unconditionally, ignoring the R/W bit of the first byte after the restart | High | Open | | E | A 40 ns glitch on SCL causes a spurious register write | The edge detector has no glitch filter; one glitch = one extra bit | Medium | Open | | F | A current-address read after reset silently rewrites the pointer | The read address is treated as an address byte of the write flow, so the following 8 read clocks are parsed as a register address (all ones → 0xFF) | Medium | Open | In one sentence: both fixes are one-line changes about **when to sample** and **when to drive**. The cost was not in writing them but in first quantifying the behaviour until it proved those lines were wrong. ## 2. Problem zero: why the original simulation could not see the worst bug The master BFM of the original testbench **keeps driving SDA after every byte, holding it until ACK ends** (`tb_sda_oe` is never released). That is exactly the non-standard "the master keeps the data valid past the SCL falling edge" case, so Bug A never fires. ```systemverilog task i2c_wr_byte(...); // wait ack tb_scl_o = 1'b0; // only SCL goes low; SDA is still driven by the master with the last bit #T tb_scl_o = 1'b1; ``` **Lesson**: a verification environment bakes the DUT's defects into "normal behaviour". Before trusting a simulation of a slow serial slave, check three things about the BFM: (1) is SDA released after the 8th bit of each byte; (2) is there any skew between SDA and SCL; (3) is only a single bit period ever tried. ## 3. The diagnostic environment: parameterise the master, add three probes The new regression environment turns master timing into four parameters: | Parameter | Meaning | Sweep | | --------- | ------------------------------------------------------------------------------------------ | ---------------------------------- | | `T` | Quarter of a bit period (one bit = 4T) | bus 100 kHz ~ 1 MHz | | `THD` | Delay from the SCL falling edge to SDA changing, i.e. the master's `t_HD;DAT` | 0 ~ 625 ns (aligned with the spec) | | `TS_BYTE` | 1 = release SDA after the 8th bit (a real master); 0 = keep driving as the original TB did | both are run | | `MSCL_D` | Arrival delay of SCL on the master side, to model skew | 0 ~ 120 ns | Three observation means were what made this qualitative-to-quantitative: 1. **Scoreboard**: every register write the DUT actually drives (`wr_en` sampled on its edge) is compared item by item with the expected write sequence — address, data, order, count. "Not a single register was written" then becomes a check result, not an impression. 2. **Illegal-drive probe**: if `sda_oe=1` appears outside the read-data/ACK window, `[ILLEGAL-DRIVE]` is printed and the case counts as a bus conflict; the counts are per sampled point (the 699 and 88 quoted below are this counter). 3. **Sampling-instant probe**: prints the exact moment the R/W bit is decided, together with `sda_i` / `d2` / `d4` / the bus SDA / whether the master is still driving. 17 cases run with a single `./run.sh`, one log per case at `logs/{case}.log`; `EXTRA="+TV=130"` sets the master's `t_HD;DAT`. (Later, targeted cases were added — an address decode table, restart transactions, a direct read — bringing the full run to 21.) ## 4. Bug A: one sampling instant, quantify before touching the code ### 4.1 Locating it The three places that decide the address R/W bit (the "write" decision, the "read" decision, and the RDDATA continuation) used the **raw, unsynchronised, uncompensated input** `sda_i`. The shift register uses `sda_i_sync_d4` (four stages of delay compensation) and the address comparison uses `d2`. Of the three, only `sda_i` falls in the SCL **low** period. ### 4.2 Measured sampling instant Aligned with the waveform, the R/W decision happens about **112 ns ≈ 4 clock cycles** after the SCL falling edge at the slave pin — the fixed delay of "2-stage synchroniser + 2-stage edge detect". By that time a compliant master has long released SDA (the spec's lower bound for `t_HD;DAT` is 0 ns), so the slave samples the released value 1: - the write address 0x40 is classified as a read (`wr_op=0`) → after the address byte is ACKed the FSM reaches the register-address state and returns to IDLE; - every following data byte is **NACKed**, and not a single register is written; - every read sequence returns 0xFF (restart detection depends on `wr_op`, so a misclassified write address means the restart is never recognised). > One trap when reading the numbers: the probe prints "+140.9 ns", one clock later than the real decision cycle (112 ns), because the probe only triggers once the ACK has updated. **Probe time ≠ decision time**; margins must be computed on the decision cycle, otherwise the error in that line gets attributed to something else. ### 4.3 Threshold sweep: turn "a guess" into "a number" With the master genuinely releasing SDA (`TS_BYTE=1`), sweeping `t_HD;DAT`: | `t_HD;DAT` | Result | | ----------------------------------------------- | --------------------------------------------------------------- | | 625 / 300 / 200 / 150 / 130 / 120 / 115 ns | all pass (write + read-back correct) | | 110 / 105 / 100 / 95 / 90 / 85 / 80 / 70 / 0 ns | all fail (data bytes NACKed, 0 register writes, read-back 0xFF) | The threshold sits between **110 and 115 ns**, matching the 112 ns sampling instant exactly. The key **control case**: let the master hold SDA throughout (`TS_BYTE=0`) and sweep `t_HD;DAT` from 625 down to 0 — **everything passes**. That rules out the wrong attribution to "rate / bit period / phase": failure depends only on **when the master releases SDA**. ### 4.4 Margin accounting (the most reusable part of this note) Convert everything in simulation to a **bus-side window relative to the SCL falling edge**, then compare it against the spec: | Sampling source | Equivalent sampling instant (relative to the SCL falling edge) | Requirement this imposes on the master | Compatibility with the spec | | ---------------------------------------------------- | -------------------------------------------------------------- | --------------------------------------------------------------- | ------------------------------------------------------------------------- | | `sda_i` (original) | **after** +86 … +114 ns (SCL low period) | master must hold SDA for ≥115 ns | **conflicts with `t_HD;DAT ≥ 0`: fails 100% with a compliant master** | | `d2` (address compare, still in use after round one) | **before** 0 … 29 ns | almost no margin, correct only by luck of being before the edge | thin | | `d4` (shift register / the fixed decision sites) | **before** 57 … 86 ns (inside the SCL high period) | only requires SDA to be stable ~86 ns before the falling edge | that is the spec's `t_SU;DAT` (250 ns class), met by any compliant master | The acceptance criterion is not "it passed after the change" but "after the change the slave no longer demands anything the spec does not require of the master". ### 4.5 The fix and its re-verification ```verilog // round one: R/W decision + RDDATA continuation 194: else if (bit_cnt == 4'd8 && ~sda_i_sync_d4) i2c_ns = RDDATA; 266: else if (cs == SLV_ADDR_WR && bit_cnt == 4'd7 && sda_i_sync_d4 == 1'b0) wr_op <= 1; 282: else if (cs == SLV_ADDR_RD && bit_cnt == 4'd7 && sda_i_sync_d4 == 1'b1) ... // round two: A0 bit comparison, same sampling instant everywhere 223: if (bit_cnt == 4'd6 && {p_dat[5:0], sda_i_sync_d4} == addr) ``` Re-verified against the harshest criterion, `t_HD;DAT = 0`: | Case | Content | Result | | --------------------------------- | ------------------------------------------------------------- | ------------------------------------------- | | `mon` | full write + read flow with the sampling-instant probe | PASS | | `wr_ts` | 16 points of `t_HD;DAT` (625 → 0 ns) | PASS, 192 checks, 0 failures at every point | | `thd0` | THD=0, 8 rates from 100 k to 1 MHz | PASS, 96 checks | | `thd0_rd16` | THD=0, 20 consecutive bytes (including wrap at 0xFF) | PASS | | `thd0_ph` | clock phase swept over 8/15 points | all PASS | | `addr_a0` | slave address A0=1 (address bytes 0x42/0x43), 15 phase points | all PASS | | regression `wr_hold` / `rd_basic` | the old "master holds SDA" usage | PASS → the old usage was not broken | ## 5. Bug B: ACKing an address that is not its own — one gate On the 8th bit of `SLV_ADDR_WR`, `sda_oe`/`i2c_ack` pulled the bus low **unconditionally**, without testing `dev_chk_pass`. On a multi-slave bus an unselected device must NACK, otherwise the master concludes "the device exists". The fix (one gated term): ```verilog if (bit_cnt == 4'd7 && (((cs == SLV_ADDR_WR) && dev_chk_pass) || cs == REG_ADDR || cs == WRDATA || cs == RESTART || cs == SLV_ADDR_RD)) sda_oe <= 1'b1; ``` Besides the original foreign-address case, an **address decode table** was added (DUT 7-bit address 0x20; send START + address one byte at a time and watch ACK): | Master sends | 0x40 | 0x41 | 0x42 | 0x43 | 0x20 | 0x21 | 0x60 | 0x61 | 0xA0 | 0xA1 | 0xBF | 0xFE | | ------------ | ---- | ---- | ---- | ---- | ---- | ---- | ---- | ---- | ---- | ---- | ---- | ---- | | Expected | ACK | ACK | NACK | NACK | NACK | NACK | NACK | NACK | NACK | NACK | NACK | NACK | | Measured | ACK | ACK | NACK | NACK | NACK | NACK | NACK | NACK | NACK | NACK | NACK | NACK | 12/12 correct, including 0x42/0x43, which differ from the slave address by the A0 bit alone. Full regression, 21 cases: **16 PASS, and the 5 failures are exactly the still-unfixed C/D/E/F plus one skew case that is expected to fail** — that is what "no regression" looks like as evidence. One inconsistency left behind: the `i2c_ack` register itself still pulls low unconditionally (it only selects the FSM branch `SLV_ADDR_WR → REG_ADDR/IDLE`, it does not drive the bus). Externally the behaviour is now right, but the internal state disagrees with the bus; gating it as well is an optional cleanup, not a bug. ## 6. The four still open: symptom, mechanism, criterion | ID | Measured symptom | Mechanism | Suggested fix | | -- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | C | restart + foreign read address 0xA1: the slave ACKs and drives `mem` data (0x3C was returned) | the read-address state tests `i2c_ack` but not `dev_chk_pass`, and `dev_chk_pass` is cleared on START | gate `dev_chk_pass` in the read-address state as well | | D | write data → restart → write address (a combined transaction): the slave enters RDDATA unconditionally, 699 sampled points of illegal drive, and the master's register-address byte is NACKed | `RESTART → SLV_ADDR_RD` is a fixed transition; the R/W bit of the first byte after the restart is ignored | parse the address byte after a restart first, then branch on R/W | | E | a 40 ns low pulse on SCL (while high): a premature ACK and one spurious write `mem[0x40] <= 0xA0`, with `bit_cnt` left stuck at 3 | an edge is accepted as soon as the 2-stage synchroniser sees it (≥ ~29 ns), with no glitch filter | require the level to be stable for 3 cycles before accepting an edge, or give SCL the same glitch immunity as SDA (the spec already asks for 50 ns class suppression) | | F | current-address read after reset (address 0x41): the address is ACKed correctly but 0xFF is read back and the internal pointer is changed to 0xFF | in the address-byte state, "address matched" always leads to the register-address state regardless of R/W, so the following 8 read clocks are parsed as a register address | branch on R/W after the address matches, or document that only "write address + register address + [restart + read address]" is supported | For C/D specifically: on a single-slave bus they are only wasteful, **but with several devices on the bus they are a real conflict** — they drive this slave's data onto someone else's address and break access to the other parts. ## 7. Three non-bug risks noticed along the way 1. `sda_o` is a purely combinational output (selected directly from the register-file read data); read data is not output-registered → register-file read delay plus routing must fit inside half a bit period. Registering the read data out during SCL low would be safer. 2. One internal read-enable signal is generated but never brought out as a port: dead logic. 3. The ~86 ns delay compensation of the four-stage synchroniser chain is **hard-wired to the clock frequency**: change the clock from 70 MHz and every sampling constant must be recomputed. Parameterise it, or rewrite the scheme as "sample at the middle of the SCL high period" so it does not depend on an absolute delay — a structural issue, out of scope for these bug fixes. ## 8. Pitfalls hit along the way (process, not RTL) | Pitfall | Symptom | Handling | | --------------------- | ------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------- | | Environment variables | running the script directly reports the tools as not found | `module load vcs/2023 verdi/2023` first, or point at the tool installation explicitly | | Wrong attribution | first assumed the bus rate was too high | a control case with the master holding SDA ruled out rate/phase and pinned it to "when is SDA released" | | Misleading probe | the instant printed by `[MON]` is one clock later than the decision cycle | compute margins on the decision cycle, and state the one-clock difference in the report | | Half a fix | round one changed 3 decision sites and missed the address comparison still on `d2` (0~29 ns of margin) | round two moved all 4 sampling sites to `d4` and re-ran everything | | Incomplete gating | after fixing B the bus behaviour was right but internal `i2c_ack` still pulled low unconditionally | recorded as "externally correct, internally to be unified", not counted as fixed | | Report readability | markdown tables do not line up in a terminal or in emacs | wrote an md→HTML converter (tables with borders) and regenerate it from the source md | ## 9. Reusable checklist (verifying a slow serial slave) 1. Audit the BFM first: is it permanently a "friendly master" (always driving, one rate, no skew)? 2. Parameterise all master timing, and sweep `t_HD;DAT` / `t_SU;DAT` down to the spec's **lower bound**. 3. All three probes are mandatory: scoreboard (did the write actually land), illegal drive (bus conflict), sampling instant (margin). 4. Write the conclusion as **margin accounting**: convert simulation instants into a bus-side window relative to the SCL falling edge and compare with the spec; never stop at "it passes after the change". 5. Every "failure" needs a control case that rules out the most tempting wrong attribution. 6. After each fix, run the related cases plus one full regression, and claim "no regression" with counts — total / PASS / FAIL, every FAIL explained. 7. State explicitly which failures are **outside legal master behaviour** (e.g. SDA changing while SCL is high is itself a START/STOP condition, not a bug), otherwise they become noise that buries the real issue. ## 10. Numbers at a glance | Item | Value | | --------------------------------- | --------------------------------------------------------- | | Sampling instant of the raw input | about 112 ns ≈ 4 clock cycles after the SCL falling edge | | Failure threshold for `t_HD;DAT` | between 110 and 115 ns (115 passes, 110 fails) | | Original sampling window | +86 … +114 ns after the falling edge | | Fixed sampling window (`d4`) | 57 … 86 ns before the falling edge | | Regression size | 21 cases, 16 PASS, 5 FAIL (4 unfixed + 1 expected) | | Address decode table | 12/12 correct | | Illegal-drive counts (D/E) | 699 sampled points / 88 sampled points | ## 11. Glossary - **`t_HD;DAT` (data hold time)**: how long data must stay valid after the SCL falling edge; the spec's lower bound is 0 ns. The central parameter of this note. - **`t_SU;DAT` (data setup time)**: how long data must be stable before the SCL rising edge (250 ns class in standard/fast mode). - **current address read**: a transaction that reads from the current pointer without sending a register-address byte. - **repeated START**: another START without an intervening STOP, to change direction or target. - **combined transaction**: write + repeated START + write/read in one transfer; I2C allows it. - **open-drain / wired-AND**: the bus has only "pull low" and "release"; devices can only pull low and pull-ups restore the high level. - **oversampling**: sampling SCL/SDA with a clock much faster than the bus, locating the sampling instant through a synchroniser chain. It saves a clock domain but ties the sampling instant to the clock frequency. - **2-stage synchroniser / 4-stage delay compensation**: the clock-domain-crossing chain whose depth decides where the sampling instant sits relative to the edge — the source of the margins in this note. - **scoreboard**: the checker that compares DUT output against expectations item by item; "did it actually get written" has to be proven by it. - **BFM (bus functional model)**: the model of the master inside the testbench; its habits decide which DUT defects can be seen at all.