Coding for review and debug

Name signals so they say what and when, split registers from the logic that feeds them, and replace numbers and macros with named parameters. Then read Lint's messages until the design is clean.

Lesson: R7.3
Time: 25 min
Runs on: Your browser
Check: 5 questions

A reviewer spent a day on a timer controller whose signals were t1, t2 and flag. Its state and its counter shared one block, and its widths came from macros. The question was whether done could come a cycle late, and nobody could answer it from the code. The design was right, and it took a rename and a lint run to show that.

As it arrived

logic [1:0] t1;
logic [`W-1:0] t2;
always_ff @(posedge clk or negedge rst_n)
  if (!rst_n) begin t1 <= `IDLE; t2 <= 0; end
  else begin
    flag = (t2 == 9);
    case (t1)
      `IDLE: if (go) t1 <= `RUN;
      `RUN:  if (flag) t1 <= `FIN; else t2 <= t2 + 1;
      `FIN:  begin t1 <= `IDLE; t2 <= 0; end
    endcase
  end

Lint prints ten warnings. Yosys 0.70 builds 31 cells, 7 flops, 4 levels of logic on this flow.

Written for a reader

assign at_last = (count_q == LAST);
always_comb begin
  state_d = state_q;
  count_d = count_q;
  case (state_q)
    IDLE:    if (go) state_d = RUN;
    RUN:     if (at_last) state_d = FIN;
             else         count_d = count_q + 1'b1;
    FIN:     begin state_d = IDLE; count_d = '0; end
    default: begin state_d = IDLE; count_d = '0; end
  endcase
end
always_ff @(posedge clk or negedge rst_n)
  if (!rst_n) begin state_q <= IDLE; count_q <= '0; end
  else        begin state_q <= state_d; count_q <= count_d; end

Lint is silent. The bench prints the same five lines. Yosys 0.70 builds 30 cells, 7 flops, 4 levels.

Names that say what and when

RTL is read far more often than it is written: by reviewers, by verifiers, by whoever reads a timing report, and by you a year later. Each rule below makes one kind of reading cheaper.

Rule	Why
`count_q` for a register's output, `count_d` for its next value	a reader sees at once which side of the flop a value is on, and in which cycle
`rst_n` for active-low; a domain prefix such as `wr_` when there are two clocks	the polarity and the clock are in the name, where a waveform shows them
one module, one job, one clock; ports grouped and commented once	a box in the block diagram becomes one module a reviewer can read alone
registers in `always_ff`, next-state logic in `always_comb`	a search for `always_ff` finds every flop
named constants: `localparam LAST = CW'(TICKS - 1)`	one change of `TICKS` resizes the counter and moves the compare
a named signal for each value a waveform or report will show	`at_last` appears in the Waves tab; `t2 == 9` inside a block does not

The house style of this track. A brief may override a default, never the reason behind it.

The controller as a reviewer reads itArrows show values moving through one clock cycle. Each box is one block in the clean version.

Before the edge
The comb block reads go, at_last and the _q values and computes the _d values.
At the edge
The registers load the _d values. Nothing else happens in always_ff.
After the edge
The outputs decode state_q, and count_q feeds the compare for the next cycle.

Read the diagram as text

go: input
at_last: assign: count_q == LAST
Next state: always_comb: state_d, count_d
Registers: always_ff: state_q, count_q
Outputs: assign: busy, done

go → Next state: go
at_last → Next state: at_last
Next state → Registers: _d values
Registers → Outputs: state_q
Registers → at_last: count_q

In the version that arrived, all five boxes were one always_ff block.

Lint, the reviewer who never tires

Lint reads the design without simulating it and reports code that is legal but likely wrong. The Lint button runs Verilator 5.052 with -Wall. Here is what it says about the controller as it arrived.

The ten messages, without the source lines Verilator prints under each.

%Warning-WIDTHTRUNC: main.sv:12:26: Operator ASSIGNDLY expects 2 bits on the Assign RHS, but Assign RHS's CONST '3'h0' generates 3 bits.
%Warning-WIDTHTRUNC: main.sv:16:26: Operator ASSIGNDLY expects 2 bits on the Assign RHS, but Assign RHS's CONST '3'h1' generates 3 bits.
%Warning-WIDTHTRUNC: main.sv:17:29: Operator ASSIGNDLY expects 2 bits on the Assign RHS, but Assign RHS's CONST '3'h2' generates 3 bits.
%Warning-WIDTHTRUNC: main.sv:18:25: Operator ASSIGNDLY expects 2 bits on the Assign RHS, but Assign RHS's CONST '3'h0' generates 3 bits.
%Warning-WIDTHEXPAND: main.sv:15:7: Operator CASE expects 3 bits on the Case expression, but Case expression's VARREF 't1' generates 2 bits.
%Warning-WIDTHEXPAND: main.sv:21:21: Operator EQ expects 3 bits on the LHS, but LHS's VARREF 't1' generates 2 bits.
%Warning-WIDTHEXPAND: main.sv:22:21: Operator EQ expects 3 bits on the LHS, but LHS's VARREF 't1' generates 2 bits.
%Warning-BLKSEQ: main.sv:14:12: Blocking assignment '=' in sequential logic process
%Warning-UNUSEDSIGNAL: main.sv:6:83: Signal is not used: 'cfg'
%Warning-CASEINCOMPLETE: main.sv:15:7: Case values incompletely covered (example pattern 0x3)

Code	What it found here	The fix
`WIDTHTRUNC`	the state macros are 3 bits, `t1` is 2: the top bit is cut	an enum of the right width
`WIDTHEXPAND`	the case and the compares widen `t1` to 3 bits	the same enum
`BLKSEQ`	`flag =` is a blocking assignment in a clocked block; Synthesize adds its own warning that the asynchronous load value for `flag` is missing	a named `assign at_last`
`UNUSEDSIGNAL`	port `cfg` is read nowhere	delete it, or waive it with a reason
`CASEINCOMPLETE`	code 3 of `t1` has no arm	a `default` arm

Lint also reports LATCH (R1.3) and UNDRIVEN (R7.4). It skips DECLFILENAME here, since every design file is named main.sv.

Every one of these warnings was harmless in this design, and that is the trap. A truncation that drops a zero bit today drops a one bit after the next edit. The house rule: a design is lint-clean before review. A warning you keep gets a waiver beside the line, with its reason.

A waiver for one line: Lint drops from ten messages to nine.

// verilator lint_off UNUSEDSIGNAL
input logic [3:0] cfg,   // reserved for a later mode; unused until then
// verilator lint_on UNUSEDSIGNAL

Readability costs no gates

Synthesize both versions. The one that arrived builds 31 cells, 7 flops and 4 levels of logic; the clean one builds 30, 7 and 4. Add the same default arm to the messy version and it builds 30 cells, with the same count of every cell type. The names, the split and the parameters cost nothing. The one cell was the arm that sends the unused code 3 back to IDLE.

Macros are the other half of the cleanup. A text macro, made with define, is global to everything compiled after it, in every file. It also depends on compile order. A localparam lives inside its module and can be computed from a parameter, as LAST is from TICKS. The width warnings above came from macros whose widths nobody tied to t1.

The lab: clean the controller

Design brief

Port	Direction	Width	Meaning
clk	input	1	clock
rst_n	input	1	reset, asynchronous, active-low
go	input	1	starts the timer; ignored while busy
cfg	input	[3:0]	unused
busy	output	1	after `go`, `busy` for 10 cycles
done	output	1	then `done` for one cycle

Clock: rising edge
Reset: rst_n asynchronous, active-low
Parameters: TICKS = 10 in the clean version
Corner cases: Done when: Lint is clean, the bench prints the same five lines, and Synthesize shows 30 cells

Run, Lint and Synthesize as shipped
Two runs of ten busy cycles; the go at edge 6 is ignored. Lint: ten warnings. Gates: 31 cells.
Rename and split
Rename to state_q, count_q, at_last. Move the next-state logic into always_comb with _d values and a default arm.
Replace the macros
Use an enum for the states and localparams from a TICKS parameter. Delete cfg, or waive it.
Check all three
Run prints the same lines, Lint is silent, and Synthesize reports 30 cells, 7 flops, 4 levels.

One clean version of the controller

Lint is silent on this file; the bench prints the five expected lines.

// tmr: after a go pulse, stays busy for TICKS cycles, then pulses done for one cycle.
module tmr #(parameter int TICKS = 10) (
  input  logic clk,
  input  logic rst_n,     // asynchronous, active-low
  input  logic go,        // start a run; ignored while busy
  output logic busy,      // 1 while the run counts
  output logic done       // 1 for the cycle after the last count
);
  localparam int CW = $clog2(TICKS);           // count bits: 4 for 10 ticks
  localparam logic [CW-1:0] LAST = CW'(TICKS - 1);

  typedef enum logic [1:0] {IDLE = 2'd0, RUN = 2'd1, FIN = 2'd2} state_e;
  state_e         state_q, state_d;
  logic [CW-1:0]  count_q, count_d;
  logic           at_last;

  assign at_last = (count_q == LAST);

  // Next state and next count.
  always_comb begin
    state_d = state_q;
    count_d = count_q;
    case (state_q)
      IDLE:    if (go) state_d = RUN;
      RUN:     if (at_last) state_d = FIN;
               else         count_d = count_q + 1'b1;
      FIN:     begin state_d = IDLE; count_d = '0; end
      default: begin state_d = IDLE; count_d = '0; end
    endcase
  end

  // The registers, and nothing else.
  always_ff @(posedge clk or negedge rst_n)
    if (!rst_n) begin state_q <= IDLE; count_q <= '0; end
    else        begin state_q <= state_d; count_q <= count_d; end

  assign busy = (state_q == RUN);
  assign done = (state_q == FIN);
endmodule

Asked in interviews

What makes RTL readable?
Names that say what and when: _q, _d, _n. One job per module, with registers apart from the logic that feeds them. Named constants from parameters, and a named signal for anything a waveform will show.
What does lint catch that simulation does not?
Code that works for the values the bench happens to drive: a truncation that drops a zero bit, an unused input, a missing case arm. Simulation passes; the next edit or the next stimulus fails.
How do you organize a module?
Ports grouped and commented, then parameters and localparams, declarations, combinational next-state logic, the registers in always_ff, and the output decode. One clock per module unless it is a crossing.

Try it

Each run is busy for ten cycles and pulses done once. The go at edge 6 arrives while busy and changes nothing.

The clean version prints the same five lines. Its numbers: 30 cells, 7 flops, 0 latches, 4 levels of logic, against 31 cells as shipped.

RTLIcarus Verilog · in your browser · Lint: Verilator service · Synthesize: Yosys service

Edit the design, then Run, Lint or Synthesize

Expected: e2 go (busy=1) e6 go (busy=1) e12 done after 10 busy cycles e15 go (busy=1) e25 done after 10 busy cycles

Original design · 31 cells · 7 flip-flops · 0 latches · 4 logic levels. Synthesis uses Yosys; counts depend on the toolchain.

Original example source

// tmr: a timer controller as it arrived for review. After go: busy for ten cycles, then done.
`define W    4
`define IDLE 3'd0
`define RUN  3'd1
`define FIN  3'd2
module tmr (input logic clk, input logic rst_n, input logic go, input logic [3:0] cfg,
            output logic busy, output logic done);
  logic [1:0] t1;
  logic [`W-1:0] t2;
  logic flag;
  always_ff @(posedge clk or negedge rst_n)
    if (!rst_n) begin t1 <= `IDLE; t2 <= 0; end
    else begin
      flag = (t2 == 9);
      case (t1)
        `IDLE: if (go) t1 <= `RUN;
        `RUN:  if (flag) t1 <= `FIN; else t2 <= t2 + 1;
        `FIN:  begin t1 <= `IDLE; t2 <= 0; end
      endcase
    end
  assign busy = (t1 == `RUN);
  assign done = (t1 == `FIN);
endmodule

Read included file: tb.sv

`timescale 1ns/1ps
module tb;
  logic clk = 0;
  always #5 clk = ~clk;                   // rising edges at 5, 15, 25, ...
  logic rst_n = 1;                        // falls at 1 ns: a real edge, so an asynchronous reset acts before any clock
  int edge_n = -1;
  always @(posedge clk) edge_n = edge_n + 1;
  logic go = 0;
  logic busy, done;
  int   nbusy = 0;                        // busy cycles since the last done
  tmr dut (.clk(clk), .rst_n(rst_n), .go(go), .busy(busy), .done(done));
  // Advance to the next rising edge, wait for the design to settle, then print one line.
  task automatic tick();
    @(posedge clk); #1;
    begin
      if (busy) nbusy++;
      if (go)   $display("e%0d go   (busy=%b)", edge_n, busy);
      if (done) begin $display("e%0d done after %0d busy cycles", edge_n, nbusy); nbusy = 0; end
    end
  endtask
  initial begin
    #1 rst_n = 0; tick(); tick(); rst_n = 1;   // asserted at 1 ns, edges 0 and 1 in reset, released 1 ns after edge 1
    go = 1; tick(); go = 0;       // start a run
    repeat (3) tick();
    go = 1; tick(); go = 0;       // a second go while busy: ignored
    repeat (8) tick();
    go = 1; tick(); go = 0;       // a new run after done
    repeat (11) tick();
    $finish(0);                             // quiet: no "$finish called" line
  end
endmodule

Check yourself

Predict the outputThe clean controller runs with TICKS = 4. The bench pulses go, then pulses it again two edges later, then waits. It counts the edges where busy and done were 1. What does it print?

// tmr: after a go pulse, stays busy for TICKS cycles, then pulses done for one cycle.
module tmr #(parameter int TICKS = 10) (
  input  logic clk,
  input  logic rst_n,     // asynchronous, active-low
  input  logic go,        // start a run; ignored while busy
  output logic busy,      // 1 while the run counts
  output logic done       // 1 for the cycle after the last count
);
  localparam int CW = $clog2(TICKS);           // count bits: 4 for 10 ticks
  localparam logic [CW-1:0] LAST = CW'(TICKS - 1);

  typedef enum logic [1:0] {IDLE = 2'd0, RUN = 2'd1, FIN = 2'd2} state_e;
  state_e         state_q, state_d;
  logic [CW-1:0]  count_q, count_d;
  logic           at_last;

  assign at_last = (count_q == LAST);

  // Next state and next count.
  always_comb begin
    state_d = state_q;
    count_d = count_q;
    case (state_q)
      IDLE:    if (go) state_d = RUN;
      RUN:     if (at_last) state_d = FIN;
               else         count_d = count_q + 1'b1;
      FIN:     begin state_d = IDLE; count_d = '0; end
      default: begin state_d = IDLE; count_d = '0; end
    endcase
  end

  // The registers, and nothing else.
  always_ff @(posedge clk or negedge rst_n)
    if (!rst_n) begin state_q <= IDLE; count_q <= '0; end
    else        begin state_q <= state_d; count_q <= count_d; end

  assign busy = (state_q == RUN);
  assign done = (state_q == FIN);
endmodule

module tb;
  logic clk = 0, rst_n = 0, go = 0, busy, done;
  always #5 clk = ~clk;
  tmr #(.TICKS(4)) dut (.*);
  int nbusy = 0, ndone = 0;
  always @(posedge clk) begin
    if (busy) nbusy++;
    if (done) ndone++;
  end
  initial begin
    #12 rst_n = 1; go = 1;              // go at the next edge
    @(posedge clk); #1 go = 0;
    @(posedge clk); #1 go = 1;          // go again, two edges later
    @(posedge clk); #1 go = 0;
    repeat (8) @(posedge clk);
    #1 $display("busy=%0d done=%0d", nbusy, ndone);
    $finish(0);
  end
endmodule

Spot the bugEach option writes the controller's two constants. The bench starts a run with TICKS = 10 and expects done ten cycles after go. Which option fails?

localparam int CW = $clog2(TICKS);
  localparam logic [CW-1:0] LAST = TICKS - 1;

localparam int CW = $clog2(TICKS);
  localparam logic [2:0] LAST = TICKS - 1;

localparam int CW = 4;
  localparam logic [CW-1:0] LAST = 4'd9;

localparam int CW = $clog2(TICKS);
  localparam logic [CW-1:0] LAST = CW'(TICKS - 1);

ConceptWhy does the clean controller call its state register state_q and its next value state_d?

The synthesizer reads the suffixes to decide which signals become flip-flopsLint warns about any register whose name does not end in _qA reader can tell which side of the flop each value is on, in the code and in a waveformThe suffixes make the simulator run state_d before state_q in each time step

ConceptWhy replace the macro W with a localparam computed from a parameter?

Macros cannot be used inside a port declaration or a vector rangeA localparam can be overridden from the instance, and a macro cannotA localparam builds fewer cells than a macro holding the same valueA macro is global and order-dependent; a localparam is scoped and tracks the parameter

ConceptThe controller as it arrived passes its bench. What does Lint add?

Code that is wrong for values the bench never drove: a cut bit, an unused port, a missing armNothing new: a design that passes its bench has no warnings worth readingA count of the cells the design needs, before synthesis runsThe cycle in which each output first goes wrong, as in a waveform

Remember

Name for the reader: _q registers, _d next values, _n active-low, a named signal for every value a waveform will show.
Keep registers in always_ff and the logic that feeds them in always_comb; replace numbers and macros with localparams from parameters.
A design is lint-clean before review; a warning you keep gets a waiver beside the line, with its reason.
Readability costs no gates: the controller builds 30 cells messy or clean once both have the same default arm.