RTL Design Review — Master and Slave

Modules 17 and 18 built a master and a target, and every design decision in them was argued for as it was made. Reviewing is the opposite situation. The code is finished, the author is confident, the schedule is short, and you have perhaps an hour.

The question this chapter answers is not "what should I look for". Lists of things to look for are easy to write and almost useless, because the reviewer who reads one still has to decide whether the thing they are looking at is wrong. The question is:

Which review questions can actually be settled, and by what observation?

A review question is worth asking only if there is something you could see — in the code, in a simulation, on a board — that would answer it either way. A question whose only possible answer is the author saying "yes, we handled that" has not been asked; it has been raised.

1. Four Kinds of Review Question, and Only Three Are Useful

Sort every question you are about to ask into one of these before you ask it.

The fourth kind is not a failure. It is the most important row in the table, because a question that the available tools cannot settle has to be written down as unresolved rather than nodded through. Chapter 19.4 makes that case for metastability specifically: no amount of RTL simulation produces evidence about a settling-time failure rate, so the review records the depth chosen and the reasoning, and does not pretend to have measured anything.

What makes a review go wrong is not asking too few questions. It is asking structural questions experimentally, experimental questions structurally, and unanswerable questions as though a confident answer settled them.

2. Where the Questions Attach

An I²C block has a small number of places where a review question is worth spending time, and they are not evenly distributed. Most of the code is a shift register and a counter and is nearly impossible to get wrong in an interesting way. The interesting places are the seams: where the block meets the wire, where it meets the clock, and where it decides to stop waiting.

Each of the four sections below follows the same shape, and the shape is the method:

1. the code as submitted, and the alternative that survives review;
2. the review question, in one line;
3. the plausible test — the one a competent engineer writes first, which passes for both;
4. the discriminating test — the observation that separates them;
5. what actually happened when both were run.

Step 3 is the one usually skipped, and it is the step that tells you whether the defect would have shipped.

3. Specimen A — Can Anything Drive the Line High?

This is the first structural question on any I²C review, and it takes about four seconds to answer.

   // The version submitted for review.
   module sda_stage_sub (input wire drive_low, output wire sda);
      assign sda = drive_low ? 1'b0 : 1'b1;
   endmodule

   // The version that survives review.
   module sda_stage_ok (input wire drive_low, output wire sda);
      assign sda = drive_low ? 1'b0 : 1'bz;
   endmodule

The difference is one character. Chapter 19.1 is the chapter that explains why 1'bz is the correct half of that ternary and what synthesis does with it; the review question is narrower and purely structural:

Is there any assignment in this block that can place a 1 on SDA or SCL?

Now the part that matters. Here is the test almost everybody writes first — one device on a bus with a pull-up, driving low and releasing.

   pullup (sda_sub);
   pullup (sda_ok);
   sda_stage_sub u_sub (.drive_low(d), .sda(sda_sub));
   sda_stage_ok  u_ok  (.drive_low(d), .sda(sda_ok));

   // ... chk(), which compares both stages against the expected value and
   //     counts a mismatch on each ...

   initial begin
      d = 1'b1; chk(1'b0, 1'b0);     // pulled low
      d = 1'b0; chk(1'b1, 1'b1);     // released -> pull-up wins
      d = 1'b1; chk(1'b0, 1'b0);
      d = 1'b0; chk(1'b1, 1'b1);
      // ... then the pass/fail report ...
   end

TEST1 single-participant : SUB PASS , OK PASS

Both pass, and they pass for a reason worth stating precisely: with one participant on the bus, driving high and releasing to a pull-up are indistinguishable. The pull-up and the transistor produce the same logic level. Every waveform looks right. A regression built on single-device tests will be green forever.

The discriminating test adds the thing I²C is actually made of — a second device pulling low while the first has released.

   // The second device, modelled exactly like a real one: pull low or release.
   assign sda_sub = other ? 1'b0 : 1'bz;
   assign sda_ok  = other ? 1'b0 : 1'bz;

   // The protocol rule, stated once: the bus is the wired-AND of every
   // participant. LOW if anybody pulls low, HIGH only if all have released.
   function expected(input a, input b);
      expected = !(a | b);
   endfunction

  SUB d=0 other=1 bus=x expected=0
TEST2 two-participant    : SUB FAIL , OK PASS

One of the four input combinations separates them: the stage has released, and somebody else is pulling low. The submitted version puts the bus into x — contention — where the protocol says 0.

That combination is not a corner case. It is every acknowledge, every clock stretch, and every arbitration bit, which is to say it is most of the protocol. The reason the defect is nevertheless easy to ship is that it is invisible in exactly the configuration people build first.

4. Specimen B — Is the Resolved Bus Authoritative?

The same confusion, one level up, in the verification code rather than the design. Two monitors, both passive, both eight bits MSB first, both sampling in the middle of the high phase. They differ in one port connection.

   // Submitted for review: sampling the master's own drive intent.
   module mon_intent (input wire scl, input wire intent, output reg [7:0] byte_o,
                      output reg done);
      integer n; reg [7:0] sh;
      initial begin n = 0; sh = 0; done = 0; byte_o = 0; end
      always @(posedge scl) begin
         sh = {sh[6:0], intent};
         n  = n + 1;
         if (n == 8) begin byte_o = sh; done = 1; n = 0; end
      end
   endmodule

   // Survives review: sampling the resolved line.
   module mon_bus (input wire scl, input wire bus, output reg [7:0] byte_o,
                   output reg done);
      integer n; reg [7:0] sh;
      initial begin n = 0; sh = 0; done = 0; byte_o = 0; end
      always @(posedge scl) begin
         sh = {sh[6:0], bus};
         n  = n + 1;
         if (n == 8) begin byte_o = sh; done = 1; n = 0; end
      end
   endmodule

Does every checker in this environment read the resolved line, or does one of them read what somebody meant to transmit?

With one master, intent and wire are the same signal wearing two names:

TEST1 one master : on-the-wire=0xa5  intent-monitor=0xa5  bus-monitor=0xa5
TEST1 one master : intent-monitor AGREES , bus-monitor AGREES

Now add a second master. Master A sends 0xA5, master B sends 0xA1; wired-AND resolves each bit, and A loses at the bit where it released and B pulled low. The oracle is computed from the stimulus by the protocol rule — tx_a & tx_b — and never read from either monitor.

TEST2 two masters: A intended=0xa5  B intended=0xa1  wired-AND truth=0xa1
TEST2 two masters: intent-monitor=0xa5  bus-monitor=0xa1
TEST2 two masters: intent-monitor DISAGREES , bus-monitor AGREES

The intent monitor reports 0xA5 — a byte that was never on the wire at any instant. It is not approximately wrong or late; it is a clean, confident report of a transfer that did not happen.

Chapter 21.5 builds the monitor that gets this right and explains the sampling discipline behind it. The review question is the cheap structural version of the same concern, and the thing to notice is that it is answerable by reading a port map. You do not need to understand the monitor's reconstruction logic to ask what its input is connected to.

5. Specimen C — What Legal Pulse Does This Filter Delete?

Specimens A and B are structural. This one is not: no amount of reading settles it, because the answer is a number and the number depends on three parameters that live in different files.

   module agree_filter #(parameter integer N_SAMP = 6)
      (input wire clk, input wire rst_n, input wire pin, output reg out);
      integer cnt;
      always @(posedge clk or negedge rst_n) begin
         if (!rst_n) begin
            cnt <= 0;
            out <= 1'b1;            // idle high, like a released bus line
         end else if (pin == out) begin
            cnt <= 0;               // agrees with the held value: nothing to do
         end else if (cnt == N_SAMP - 1) begin
            out <= pin;             // N consecutive disagreeing samples: accept
            cnt <= 0;
         end else begin
            cnt <= cnt + 1;
         end
      end
   endmodule

The review question people ask here is "is the filter deep enough", and it is the wrong question — it has only one direction and therefore only one answer, which is always "make it deeper". The question with two directions is:

What is the narrowest pulse this design must still see, and does the threshold pass it?

A filter has a stop-band and a pass-band. The specification fixes both ends: tSP = 50 ns is the spike width a compliant input must reject (11.8), and tHIGH(min) is the narrowest legal high phase of the chosen speed mode — 260 ns at Fast-mode Plus (11.2). At a 100 MHz sampling clock those are 5 and 26 samples.

Here is the plausible test: inject a 50 ns spike and confirm it is rejected. Everyone writes this one, because it is the filter's stated purpose.

TEST1 spike 50ns  (tSP, MUST be rejected): N=6 rejected , N=32 rejected

Green, at both depths, including the deep one. Now the discriminating test — the narrowest pulse that must survive:

TEST2 pulse 260ns (tHIGH min at Fm+, MUST survive): N=6 survived (latency 59 ns) , N=32 DELETED

At N_SAMP = 32 the filter silently removes a legal Fast-mode Plus clock high phase. Sweeping the parameter against both bounds gives the window directly:

SWEEP: largest N that still PASSES the 50 ns spike    = 5
SWEEP: smallest N that DELETES the 260 ns legal pulse = 27
SWEEP: legal window for N_SAMP at 100 MHz, Fm+       = 6 .. 26

Two things follow, and the second is the reason this specimen is here.

The window is a design output, not a preference. At 100 MHz and Fast-mode Plus, N_SAMP must be between 6 and 26. That is a derived constraint with two named causes, and it belongs in the block's documentation next to the parameter.

"More filtering is safer" is false, with a number attached. Depth costs observation latency on every edge — 59 ns at N_SAMP = 6 in the run above — and past a threshold it starts deleting legal protocol. Chapter 19.5 derives the architecture and the latency; what a review adds is the check that somebody actually evaluated the upper bound for the speed mode this product ships in.

6. Specimen D — Is Every Wait Bounded?

The last specimen has a different character from the first three. Its defect corrupts nothing.

   module stretch_wait_sub                      // submitted for review
     (input wire clk, input wire rst_n, input wire start, input wire scl_pin,
      output reg done, output reg timeout);
      reg busy;
      always @(posedge clk or negedge rst_n) begin
         if (!rst_n)      begin busy <= 0; done <= 0; timeout <= 0; end
         else begin
            done <= 0; timeout <= 0;
            if (!busy && start)       busy <= 1;
            else if (busy && scl_pin) begin busy <= 0; done <= 1; end
         end
      end
   endmodule

   module stretch_wait_ok #(parameter integer TIMEOUT = 2000)
     (input wire clk, input wire rst_n, input wire start, input wire scl_pin,
      output reg done, output reg timeout);
      reg busy; integer cnt;
      always @(posedge clk or negedge rst_n) begin
         if (!rst_n)      begin busy <= 0; done <= 0; timeout <= 0; cnt <= 0; end
         else begin
            done <= 0; timeout <= 0;
            if (!busy && start)       begin busy <= 1; cnt <= 0; end
            else if (busy && scl_pin) begin busy <= 0; done <= 1; end
            else if (busy && cnt == TIMEOUT - 1) begin busy <= 0; timeout <= 1; end
            else if (busy)            cnt <= cnt + 1;
         end
      end
   endmodule

Both are correct about stretching. Both release SCL, watch the pin, and refuse to proceed until the target lets it rise — which is the behavior 12.2 requires and the thing a stretching test is written to check. They differ only in what happens when the pin never rises.

TEST1 legal 300-clk stretch : sub done=1 timeout=0 | ok done=1 timeout=0
TEST2 SCL held low forever  : sub done=0 timeout=0 | ok done=0 timeout=1
TEST2 data mismatches observed by a data-integrity checker: 0
TEST2 verdict: sub REPORTED NOTHING -- still waiting , ok reported an outcome

Read the third line again. Zero data mismatches. Every byte the submitted design transferred was correct, because it transferred none. A liveness defect produces no wrong data, so no data-integrity checker — no scoreboard, no reference model, no protocol assertion about byte contents — can see it. The only check that fires is a check on completion, and that check has to be written deliberately because nothing about the failure suggests it.

For every wait in this block: what ends it, and what happens if that never arrives?

There is a second, sharper version of the question for reviewers who have been burned once: does the testbench itself contain an unbounded wait? A bench that hangs alongside the design reports neither a pass nor a fail, and a regression that reports nothing is usually read as a machine problem rather than a design one. The harness above bounds its own observation window at 6000 clocks for exactly this reason, and that bound is part of the test, not an implementation detail of it.

7. The Four Results Together

The column that carries the lesson is the fourth. In all four cases a competent, purposeful test passes both versions. Not a lazy test — a test written by somebody who understood the feature and aimed at it. Specimen A's author tested driving and releasing. Specimen C's author tested spike rejection, which is what the filter is for. Specimen D's author tested clock stretching, which is the hard feature.

That is the single most useful thing a reviewer can internalize: the tests that exist were written by someone aiming at the feature, and the defects that survive are the ones that are invisible from where they aimed. So the productive review question is rarely "did you test this feature" — the answer is nearly always yes. It is:

What would this test still pass with?

8. What These Results Do Not Establish

Four specimens, run in one simulator, on deliberately small designs. Being exact about the limits is part of the method.

• Every result is SIMULATED. Specimen A's x is a simulator's model of contention. On real silicon two CMOS drivers fighting produce a voltage somewhere between the rails, a current through both transistors, and a level that may read as either value at different receivers on the same bus. The simulator's x is a warning that the value is not determined, not a measurement of what the board does. The review conclusion — never drive high — is unchanged, but the mechanism on hardware is 2.2's, not the simulator's.
• Specimen C's numbers are tied to 100 MHz and Fast-mode Plus. They are arithmetic about sample counts, and they move with either parameter.
• None of this addresses metastability. Specimen B concerns what is observed, not whether the observation settled. RTL simulation cannot produce evidence about settling time at all, which 19.4 states plainly and this chapter does not improve on.
• The specimens are small on purpose. They isolate one defect each. A real block has all four seams at once and the interactions between them, which is what an integration bench is for and what 17.12 §6 measures on the assembled master.

9. A Review Checklist That Encodes Reasoning

A checklist is worth having only if each line names a failure mode rather than a topic. "Check the FSM" is a topic. The lines below are questions whose wrong answer is a specific bug.

Line ownership and the wire

• Does any assignment place a 1 on SDA or SCL, anywhere, including in reset and in the testbench? (19.1)
• When the block has released a line, what do the existing tests have pulling it low?
• Does every checker read the resolved line rather than a drive intent?
• Does reset leave both lines released? A block that comes out of reset holding SDA low has hung a bus it does not own. (18.11)

Clock, reset and observation

• Which clock samples the pins, and how many stages before any logic uses the value?
• What is the total observation latency from pin edge to protocol decision, in clocks and in nanoseconds, and what fraction of a bit period is that at the fastest supported mode?
• Is the synchronizer's output used anywhere before the last stage? A chain whose intermediate node feeds logic is not a synchronizer.

Filtering and sampling

• What is the narrowest legal pulse at the fastest supported speed mode, and does the filter pass it?
• What is the widest illegal pulse, and does the filter reject it?
• Is there a parameter combination for which those two requirements conflict?

Framing and protocol state

• Where are START and STOP detected, and can that detector fire on a data bit?
• What happens on a repeated START to state that was mid-transfer — and is each piece of state's behavior a decision somebody wrote down, or whatever the code happens to do? (18.10)
• On a NACK, who stops driving, and when?

Waiting and recovery

• List every wait. For each: what ends it, and what happens if that never arrives?
• Is a timeout a per-bit bound or a per-transfer bound, and which was intended? (24.6 measures the difference)
• After a timeout fires, what state is the bus left in, and can the block recover it? (15.4)

Parameters

• What is each parameter's legal range, and where is it checked?
• Which values have actually been elaborated, as opposed to declared legal?
• Does any legal-looking value fail to elaborate? (19.9 found one in this curriculum's own target.)

10. Common Misconceptions

"The code is simple, so a careful read is enough." Specimens C and D are simple and a careful read does not settle either. C's defect is a relationship between three numbers in different files; D's is an absence, and absences do not attract attention while reading. Sorting questions by §1's four kinds is what prevents an hour spent re-reading a shift register while the unbounded wait goes unnoticed.

"If it passes the feature's own test, the feature works." Every specimen here passes its feature's own test. The test was written by someone who understood the feature and aimed at it; that is precisely why the defect is in the part they were not aiming at.

"An x in simulation is a simulation artifact." Sometimes. Specimen A's x is the simulator correctly refusing to guess, and the underlying condition — two drivers, opposite directions — is a real current path on a real board. Reviewing away an x without finding out which of those two it is turns a loud defect into a quiet one.

"Deeper filtering is a safe default." It is a default with a measured upper bound: 26 samples at 100 MHz for Fast-mode Plus. Above it, legal protocol disappears with no error anywhere.

"A liveness bug will show up in the regression." Only if something checks completion. Specimen D produced zero data mismatches while transferring nothing.

// An I2C target, reviewed and approved. The reviewer opened the regression
// waveforms, stepped through a write transaction, and saw:
//
//    SCL  ___|‾|___|‾|___ ...     nine clean clocks
//    SDA  ‾‾|_______|‾‾‾‾ ...     address, R/W, then the target's ACK low
//
// Every edge in the right place. Every byte correct. ACK where it belongs.
//
// The block drives SDA with:
//
//    assign sda = ack_drive ? 1'b0 : 1'b1;
//
// which is Specimen A. It was invisible in the waveform because the ONLY
// other participant in the testbench was a master that had released SDA
// during the ACK slot -- so nothing was pulling against the target's 1.

The structural question, asked of the source rather than the waveform:

  grep -n "sda.*<= *1'b1\|sda.*= *1'b1" rtl/*.v

  assign sda = ack_drive ? 1'b0 : 1'bz;   // and the same for scl

and the discriminating test, which is the one the testbench lacked: put a
SECOND participant on the modelled bus and let it pull low while the DUT has
released. That one addition turns an invisible defect into a failing test --
and it is the same addition Specimen A needed.

// Reviewer:  "What happens if a target holds SCL low forever?"
// Author:    "There's a timeout -- TIMEOUT_CLKS, it's a parameter."
// Reviewer:  "Good."
//
// Both statements are true. The block has a timeout, it is a parameter, and
// it fires. What neither of them said is WHERE THE COUNTER IS CLEARED:
//
//    else if (!stretched) cnt <= 0;     // restarts at every SCL release
//
// so TIMEOUT_CLKS bounds ONE stretch, not the transfer. A target that
// stretches by just under the limit at every one of nine bits never trips it,
// and the bus is held for nine times the number in the parameter.

Ask the question that has more than two answers:

  "Where is the timeout counter cleared, and what does that make it bound --
   one stretch, or the whole transfer?"

Then ask the consequence, which is where the engineering is:

  "A target stretching just under the limit at every bit is legal. How long
   can it hold the bus, and is that within what the rest of the system can
   tolerate?"

Neither question can be answered 'yes'.

11. Reason It Through

A. You are reviewing an I²C target. The author shows you a regression with 340 passing tests, including directed tests for address match, NACK on a wrong address, clock stretching, repeated START, and the full register map. You have forty minutes and you may run one simulation. What do you run, and why that rather than reading the FSM?

The suite is dense and purposeful, so the defects that survived it are the ones invisible from where it aimed — and §7's column four says those are the ones needing a second participant, an unended wait, or a boundary value. Reading the FSM is the lowest-yield use of the forty minutes: FSMs are where the author's attention already was. The highest-yield single simulation is the one that adds what the suite structurally lacks. If the bus model has one master and one target, add a second device that pulls SDA low during the target's released slots. If it already has two, sweep a parameter to a legal boundary and see whether it elaborates.

B. The block passes a 50 ns spike-rejection test and a 1.3 µs Standard-mode clock test. The product ships in Standard-mode only. Is N_SAMP = 32 at 100 MHz acceptable? What would you need to know before answering?

At Standard-mode, tHIGH(min) is 4.0 µs — 400 samples at 100 MHz — so a 32-sample filter is nowhere near deleting a legal high phase, and the answer is yes for the protocol. But two things are still open. First, the filter's latency is now 320 ns on every edge, which has to fit inside the setup margin the protocol decisions depend on; that is a timing question, not a protocol one. Second, "ships in Standard-mode only" is a claim about the product, not the block, and it is exactly the claim that stops being true when the block is reused. The reviewable output is not "yes" but "yes at Standard-mode; the upper bound is 400 samples here and 26 at Fm+, and that belongs in the parameter's documentation."

C. A reviewer proposes adding an assertion that SDA never changes while SCL is high. The author objects that it will fire on every START and STOP. Who is right, and what does the disagreement tell you about where the rule belongs?

Both are right about the facts, and the disagreement is really about the property's antecedent. The data-valid rule has framing as a deliberate exception — that is what makes START and STOP distinguishable from data at all (4.2). An assertion that ignores the exception is not too strict; it is wrong, and it will be switched off within a day, which is worse than not having it. The useful output of the disagreement is that the rule cannot be written without a notion of "we are inside a byte", which means it needs framing state — and that tells you it belongs somewhere that tracks framing rather than in a stateless check on two wires.

D. Reviewing a monitor, you find it takes the target's internal addr_match signal as an input, and the author explains this is much simpler than decoding the address from the bus. What is the cost, and is there a case where the decision is defensible?

The cost is that the monitor can no longer detect a wrong address match, because it is being told the answer by the thing it is checking. Any defect in the address comparator is now invisible to every check downstream of that signal — the same circularity as Specimen B, one level up. It is defensible only in a bring-up or debug configuration whose purpose is triage rather than verification, and only if that configuration cannot be the one that signs the block off. The reviewable question is not "is this ever acceptable" but "which claims in the test plan depend on this signal", and every one of those claims is now unproven.

12. Understanding Check

13. What 24.1 Settled

A review question earns its place only if some observation answers it either way. Sorting questions into structural, experimental, evidential and unanswerable before asking them is what stops an hour being spent re-reading code while an unbounded wait goes unnoticed — and it makes the fourth category, the questions this project cannot settle, something that gets written down rather than nodded through.

The existing tests are aimed, so the surviving defects are off-aim. All four specimens pass a purposeful test of the feature they break. The productive question is not "did you test this" but "what would this test still pass with".

Three defect shapes account for all four specimens. Something that only misbehaves when a second participant is on the bus; something that only misbehaves at a parameter boundary nobody evaluated; and something whose failure produces no data at all, so that every data check is silent. A reviewer who checks only those three is using their hour well.

Simulation settles less than it appears to. The x in Specimen A is a warning, not a measurement; the window in Specimen C is arithmetic about one clock frequency; and nothing here bears on metastability. Saying which is which is not pedantry — it is the difference between a review that produces a decision and one that produces a feeling.

The next chapter turns the same discipline on the testbench. The design review above asked what a test would still pass with; a verification review asks the sharper version of that question — whether the environment is capable of failing at all. Chapter 24.2 — Verification Completeness Review.