RTL coding style that scales

Two versions of one reloading timer agree on all 506 cycles of their testbench and synthesise to 42 and 43 cells, so a chip would not care which one it got. Verilator’s lint reports 4 warnings for the first and none for the second, and only the second shows at a glance which signals are registers and where each one gets its next value. RTL is read far more often than it is written, and style is what keeps it readable and checkable as a design and its team grow.

You will apply one naming and file convention, split every block into next-value logic and registers, catch combinational loops and second writers before they reach a chip, and finish each change with a definition of done that a script can run. The conventions are this track’s own, informed by lowRISC’s public style guide; the checks use Verilator 5.052, Yosys 0.69 and Icarus Verilog 13.0.

Names and files that say what each signal is

timer_before.sv is the first version. Its ports are clk, rstn, en, load, reload, count and expired, the module is called tmr in a file called timer_before.sv, and a signal named spare is declared and never used. No name says which signals are registers or which are ports, and only the n of rstn hints that the reset is active low. Lint sees two of those problems:

// A reloading down-counter, written the way many first versions are written.
module tmr(clk, rstn, en, load, reload, count, expired);
  input clk, rstn, en, load;
  input [7:0] reload;
  output [7:0] count;
  output expired;
  reg [7:0] count;
  reg expired;
  reg zero;
  reg [3:0] spare;

  always @(count) zero = (count == 0);

  always @(posedge clk or negedge rstn)
    if (!rstn) begin
      count = 0;
      expired = 0;
    end else begin
      case ({load, en})
        2'b10, 2'b11: begin count <= reload; expired <= 0; end
        2'b01: if (zero) begin count <= reload; expired <= 1; end
               else begin count <= count - 1; expired <= 0; end
        2'b00: expired <= 0;
      endcase
    end
endmodule

// A reloading down-counter: counts down from reload_i while en_i is 1 and pulses expired_o for one cycle when it
// reloads from 0. load_i loads reload_i at once and wins over en_i.
module timer #(
  parameter int unsigned Width = 8
) (
  input  logic             clk_i,
  input  logic             rst_ni,
  input  logic             en_i,
  input  logic             load_i,
  input  logic [Width-1:0] reload_i,
  output logic [Width-1:0] count_o,
  output logic             expired_o
);
  if (Width < 2) begin : g_width_check
    $error("timer: Width must be at least 2");
  end

  logic [Width-1:0] count_d, count_q;
  logic             expired_d, expired_q;

  // Next values: computed from the registers and the inputs, nothing else.
  always_comb begin
    count_d   = count_q;
    expired_d = 1'b0;
    if (load_i) begin
      count_d = reload_i;
    end else if (en_i) begin
      if (count_q == '0) begin
        count_d   = reload_i;
        expired_d = 1'b1;
      end else begin
        count_d = count_q - 1'b1;
      end
    end
  end

  // Registers: reset values, then the next values.
  always_ff @(posedge clk_i or negedge rst_ni) begin
    if (!rst_ni) begin
      count_q   <= '0;
      expired_q <= 1'b0;
    end else begin
      count_q   <= count_d;
      expired_q <= expired_d;
    end
  end

  assign count_o   = count_q;
  assign expired_o = expired_q;
endmodule

from session import run

# Verilator lint of both timers: the first version (module tmr in timer_before.sv), then the refactored timer.sv.
# grep keeps the first line of each message.
run("""
verilator --lint-only -Wall --quiet timer_before.sv 2>&1 | grep "^%"
verilator --lint-only -Wall --quiet timer.sv
""")

# Verilator 5.052
$ verilator --lint-only -Wall --quiet timer_before.sv 2>&1 | grep "^%"
%Warning-DECLFILENAME: timer_before.sv:2:8: Filename 'timer_before' does not match MODULE name: 'tmr'
%Warning-BLKSEQ: timer_before.sv:16:13: Blocking assignment '=' in sequential logic process
%Warning-BLKSEQ: timer_before.sv:17:15: Blocking assignment '=' in sequential logic process
%Warning-UNUSEDSIGNAL: timer_before.sv:10:13: Signal is not driven, nor used: 'spare'
$ verilator --lint-only -Wall --quiet timer.sv

DECLFILENAME and UNUSEDSIGNAL are style warnings that -Wall turns on: the module’s name does not match its file, and spare does nothing. The two BLKSEQ warnings come from the reset branch, which the next section deals with. timer.sv produces no message at all, because it follows a convention that answers the questions a reader asks of a name:

lowRISC’s guide uses the same suffixes and combines them in the same order, the polarity before the direction, as in rst_ni. Which public guide a team follows matters less than everybody writing it the same way, because the payoff is that nobody has to open a module to know what a name means.

Compute next values in one block, load them in another

The first version computes everything inside one clocked block, the case on {load, en}, the count down and the reload, and its reset branch uses blocking assignments, which Verilator flagged as BLKSEQ. timer.sv splits the same behaviour into two blocks, the two-process styletwo-process style: Writing a block as one always_comb that computes each register's next value, the _d signals, from the registers and the inputs, and one always_ff that loads those values into the registers, the _q signals, at the clock edge.. The always_comb block computes count_d and expired_d from the registers and the inputs, with defaults first, so that every path gives both a value. The always_ff block only loads: reset values while rst_ni is 0, the next values otherwise. Step through one reload of the testbench’s directed start:

The testbench drives both versions with the same inputs, checks them against a model of the timer for 506 cycles, and then each version is synthesised:

// Drives the refactored timer and the first version (module tmr) with the same inputs, and checks both against a
// model of what the timer must do: a directed start, then 500 cycles of seeded random inputs.
module tb_timer;
  logic       clk = 0, rst_n = 0, en = 0, load = 0;
  logic [7:0] reload = 0;
  logic [7:0] count_new, count_old, count_want = 0;
  logic       expired_new, expired_old, expired_want = 0;
  int         errors = 0, seed = 1;

  timer dut (.clk_i(clk), .rst_ni(rst_n), .en_i(en), .load_i(load), .reload_i(reload),
             .count_o(count_new), .expired_o(expired_new));
  tmr   old (.clk(clk), .rstn(rst_n), .en(en), .load(load), .reload(reload),
             .count(count_old), .expired(expired_old));

  always #5 clk = ~clk;

  // The model: what the timer must load at each rising edge.
  always @(posedge clk) begin
    if (!rst_n)          begin count_want <= 0;          expired_want <= 0; end
    else if (load)       begin count_want <= reload;     expired_want <= 0; end
    else if (en && count_want == 0) begin count_want <= reload; expired_want <= 1; end
    else if (en)         begin count_want <= count_want - 1; expired_want <= 0; end
    else                 expired_want <= 0;
  end

  task automatic step(input logic e, input logic l, input logic [7:0] r, input bit show);
    en = e; load = l; reload = r;
    @(posedge clk); #1;
    if (count_new !== count_want || expired_new !== expired_want) errors++;
    if (count_old !== count_want || expired_old !== expired_want) errors++;
    if (show) $display("en=%b load=%b reload=%0d | timer %0d %b | tmr %0d %b", e, l, r,
                       count_new, expired_new, count_old, expired_old);
  endtask

  initial begin
    @(posedge clk); #1 rst_n = 1;
    step(0, 1, 3, 1);                       // load 3
    repeat (4) step(1, 0, 3, 1);            // 2, 1, 0, then reload to 3 with expired
    step(1, 0, 3, 1);                       // 2 again, expired back to 0
    repeat (500) step($random(seed) % 4 != 0, $random(seed) % 8 == 0, 8'($random(seed) % 6), 0);
    $display("%0d cycles checked, %0d mismatches with the model", 506, errors);
    if (errors == 0) $display("PASS"); else $fatal(1, "FAIL");
    $finish;
  end
endmodule

from session import run

# Both timers on one testbench, checked against its model, then the hardware each one synthesises to.
run("""
iverilog -g2012 -Wall -o sim.vvp tb_timer.sv timer.sv timer_before.sv
vvp -n sim.vvp
yosys -q -p "read_verilog -sv timer.sv; synth -top timer; tee -q -o timer.txt stat"
yosys -q -p "read_verilog -sv timer_before.sv; synth -top tmr; tee -q -o tmr.txt stat"
grep -H -E "cells|DFF" timer.txt tmr.txt
""")

# Icarus Verilog 13.0 (stable) · Yosys 0.69
$ iverilog -g2012 -Wall -o sim.vvp tb_timer.sv timer.sv timer_before.sv
$ vvp -n sim.vvp
en=0 load=1 reload=3 | timer 3 0 | tmr 3 0
en=1 load=0 reload=3 | timer 2 0 | tmr 2 0
en=1 load=0 reload=3 | timer 1 0 | tmr 1 0
en=1 load=0 reload=3 | timer 0 0 | tmr 0 0
en=1 load=0 reload=3 | timer 3 1 | tmr 3 1
en=1 load=0 reload=3 | timer 2 0 | tmr 2 0
506 cycles checked, 0 mismatches with the model
PASS
tb_timer.sv:43: $finish called at 5066 (1s)
$ yosys -q -p "read_verilog -sv timer.sv; synth -top timer; tee -q -o timer.txt stat"
$ yosys -q -p "read_verilog -sv timer_before.sv; synth -top tmr; tee -q -o tmr.txt stat"
$ grep -H -E "cells|DFF" timer.txt tmr.txt
timer.txt:       42 cells
timer.txt:        8   $_DFFE_PN0P_
timer.txt:        1   $_DFF_PN0_
tmr.txt:       43 cells
tmr.txt:        8   $_DFFE_PN0P_
tmr.txt:        1   $_DFF_PN0_

The two agree on every cycle, the reload with its one-cycle pulse included, and synthesise to 42 and 43 cells with the same nine flip-flopsflip-flop: A storage element that takes the value of its input at a clock edge and holds it until the next edge. A register is a row of flip-flops clocked together. More in “Sequential logic, always_ff and nonblocking”: the refactor changed the code and not the circuit. What it bought is a shape in which the next mistakes are hard to write. Every register sits in one always_ff block with one reset value, so a register without a reset stands out. The defaults at the top of always_comb mean no path can leave a value unassigned, so no latch can appear. And always_ff computes nothing, so no statement in it can read a value that another statement of the same block has just changed.

The habit this shape removes most often is the one Verilator flagged in the first version’s reset branch:

Loops and second writers: what lint and synthesis catch

Two mistakes that the two-process style keeps out are easy to write without it. loop_demo.sv computes p from q and q from p through two gates with no register between them, a combinational loopcombinational loop: A path from a signal back to itself through gates alone, with no register on it, such as p = a & q with q = b | p. It can hold a value or oscillate, so lint and synthesis checks report it.. two_writers.sv writes count from an always_ff block and also, while clr is 1, from an always_comb block. The session simulates the loop first, then runs both files through the three tools:

// p needs q and q needs p: a loop through two gates with no register in it.
module loop_demo (
  input  logic a,
  input  logic b,
  output logic y
);
  logic p, q;
  assign p = a & q;
  assign q = b | p;
  assign y = q;
endmodule

// What the loop of loop_demo.sv does when b falls while a is 1.
module tb_loop;
  logic a = 1, b = 1, y;
  loop_demo dut (.a(a), .b(b), .y(y));
  initial begin
    #1 $display("a=%b b=%b: y=%b", a, b, y);
    b = 0;
    #1 $display("a=%b b=%b: y=%b", a, b, y);
    a = 0;
    #1 $display("a=%b b=%b: y=%b", a, b, y);
    a = 1;
    #1 $display("a=%b b=%b: y=%b", a, b, y);
    $finish;
  end
endmodule

// count is written by a clocked block and by a combinational block.
module two_writers (
  input  logic       clk,
  input  logic       clr,
  output logic [3:0] count
);
  always_ff @(posedge clk) count <= count + 1'b1;
  always_comb if (clr) count = '0;
endmodule

from session import run

# A loop through two gates, simulated and then checked, and a variable written by two blocks, on all three tools.
# Icarus Verilog compiles both files without a message.
run("""
iverilog -g2012 -Wall -o loop.vvp tb_loop.sv loop_demo.sv
vvp -n loop.vvp
verilator --lint-only -Wall --quiet loop_demo.sv 2>&1 | grep "^%"
!yosys -q -p "read_verilog -sv loop_demo.sv; proc; check -assert"
iverilog -g2012 -Wall -o writers.vvp two_writers.sv
verilator --lint-only -Wall --quiet two_writers.sv 2>&1 | grep "^%"
!yosys -q -p "read_verilog -sv two_writers.sv; proc; check -assert"
""")

# Icarus Verilog 13.0 (stable) · Verilator 5.052 · Yosys 0.69
$ iverilog -g2012 -Wall -o loop.vvp tb_loop.sv loop_demo.sv
$ vvp -n loop.vvp
a=1 b=1: y=1
a=1 b=0: y=1
a=0 b=0: y=0
a=1 b=0: y=0
tb_loop.sv:13: $finish called at 4 (1s)
$ verilator --lint-only -Wall --quiet loop_demo.sv 2>&1 | grep "^%"
%Warning-UNOPTFLAT: loop_demo.sv:5:16: Signal unoptimizable: Circular combinational logic: 'y'
$ yosys -q -p "read_verilog -sv loop_demo.sv; proc; check -assert"
Warning: found logic loop in module loop_demo:
    cell $or$loop_demo.sv:9$2 ($or) source: loop_demo.sv:9.14-9.19
      B[0] --> Y[0]
    cell $and$loop_demo.sv:8$1 ($and) source: loop_demo.sv:8.14-8.19
      B[0] --> Y[0]
ERROR: Found 1 problems in 'check -assert'.
[exit status 1]
$ iverilog -g2012 -Wall -o writers.vvp two_writers.sv
$ verilator --lint-only -Wall --quiet two_writers.sv 2>&1 | grep "^%"
%Warning-MULTIDRIVEN: two_writers.sv:8:24: Variable written to in always_comb also written by other process (IEEE 1800-2023 9.2.2.2): 'count'
%Warning-MULTIDRIVEN: two_writers.sv:8:24: Variable also written to in always_ff (IEEE 1800-2023 9.2.2.4): 'count'
%Warning-LATCH: two_writers.sv:8:3: Latch inferred for signal 'count' (not all control paths of combinational always assign a value)
%Error-BLKANDNBLK: two_writers.sv:5:22: Unsupported: Blocking and non-blocking assignments to potentially overlapping bits of same packed variable: 'count'
$ yosys -q -p "read_verilog -sv two_writers.sv; proc; check -assert"
ERROR: Latch inferred for signal `\two_writers.\count' from always_comb process `\two_writers.$proc$two_writers.sv:8$3'.
[exit status 1]

The simulation shows why a loop is trouble. With a = 1 and b = 0 the output was 1 on one line and 0 on another: the loop holds whatever it had, which is storage that nobody declared and no testbench expects. Here is what each tool said:

Icarus Verilog compiled both without a word, so a regression that runs only there would carry both on. Verilator’s MULTIDRIVEN messages quote the rules of IEEE 1800-2023 that the second file breaks, sections 9.2.2.2 and 9.2.2.4: a variable that an always_comb or always_ff block writes may be written by no other process. The two-process style prevents both mistakes by construction. Next values come only from registers and inputs, so every path inside the module that returns to where it started passes through a flip-flop, and each register has exactly one writer, its always_ff block.

A definition of done that a script can check

Style rules hold only when a machine checks them on every change. This track’s definition of done for an RTL change has five parts, and the first three run in seconds:

1. verilator --lint-only -Wall prints nothing. A warning that is really harmless gets a lint waiverlint waiver: A comment or a configuration line that turns off one lint warning at one named place in the code, with the reason for it. A flag that turns a warning off for a whole design hides every later instance of it too. on its one line, with the reason, never a -Wno-… flag for the whole design.
2. Yosys synthesises the module without a latch error, and check -assert finds no loop, no second driver and no undriven wire.
3. A parameter value that would build a broken module stops elaboration with $error.
4. A self-checking testbench passes and ends with an exit status a script can read.
5. Every interface carries assertions, which the assertions module of this track writes.

from session import run

# The definition of done for timer.sv, one check per line: lint, then synthesis with its structural checks; then the
# parameter check refusing a Width the timer cannot have, on all three tools. The testbench is cycle_session.py's run.
run("""
verilator --lint-only -Wall --quiet timer.sv
yosys -q -p "read_verilog -sv timer.sv; synth -top timer; check -assert"
!verilator --lint-only -Wall --quiet -GWidth=1 timer.sv
!iverilog -g2012 -Wall -Ptimer.Width=1 -o bad.vvp timer.sv
!yosys -q -p "read_verilog -sv timer.sv; chparam -set Width 1 timer; synth -top timer"
""")

# Verilator 5.052 · Yosys 0.69 · Icarus Verilog 13.0 (stable)
$ verilator --lint-only -Wall --quiet timer.sv
$ yosys -q -p "read_verilog -sv timer.sv; synth -top timer; check -assert"
$ verilator --lint-only -Wall --quiet -GWidth=1 timer.sv
%Warning-USERERROR: timer.sv:15:5: timer: Width must be at least 2
                                 : ... note: In instance 'timer'
   15 |     $error("timer: Width must be at least 2");
      |     ^~~~~~
                    ... For warning description see https://verilator.org/warn/USERERROR?v=5.052
                    ... Use "/* verilator lint_off USERERROR */" and lint_on around source to disable this message.
[exit status 1]
$ iverilog -g2012 -Wall -Ptimer.Width=1 -o bad.vvp timer.sv
ERROR: timer.sv:15: timer: Width must be at least 2
       During elaboration  Scope: g_width_check
1 error(s) during elaboration.
[exit status 1]
$ yosys -q -p "read_verilog -sv timer.sv; chparam -set Width 1 timer; synth -top timer"
timer.sv:15: ERROR: timer: Width must be at least 2.
[exit status 1]

timer.sv passes the first two checks with no output at all, which is what a clean result looks like. The parameter check is the if (Width < 2) at the top of the module body, a generate block that elaboration works out for each instance. Given Width = 1 through each tool’s override, -G for Verilator, -P for Icarus Verilog and chparam for Yosys, all three stop with the module’s own message before anything is built. tb_timer.sv, the testbench of the two-block section, is the fourth check: it ends with PASS or with $fatal(1, …), so a regression script needs to read only its exit status.

Practise: model the timer in two processes

The exercise builds the timer’s two blocks in Python, as separate pieces: a function that computes the next values from the registers and the inputs, as always_comb does, and a loop that loads them once per clock edge, as always_ff does. Its first sample test replays the directed start of tb_timer.sv.

def timer_next(count_q, en, load, reload, width=8):
    """The always_comb block of timer.sv: (count_d, expired_d) from the register and the inputs."""
    count_d = count_q
    expired_d = 0
    return count_d, expired_d


def run_timer(inputs, width=8):
    """The always_ff block: the (count_q, expired_q) after each rising edge, for one (en, load, reload) per edge."""
    return []

from timer import run_timer, timer_next


def test_directed_start():
    """replays the directed start of tb_timer.sv: load 3, count down, reload with one expired pulse"""
    edges = [(0, 1, 3)] + [(1, 0, 3)] * 5
    assert run_timer(edges) == [(3, 0), (2, 0), (1, 0), (0, 0), (3, 1), (2, 0)]


def test_next_values_only():
    """timer_next computes the next values and changes nothing else"""
    assert timer_next(5, 1, 0, 9) == (4, 0)
    assert timer_next(0, 1, 0, 9) == (9, 1)
    assert timer_next(5, 0, 0, 9) == (5, 0)


def test_load_wins():
    """load loads reload even when en is set and the count is 0, without an expired pulse"""
    assert timer_next(0, 1, 1, 7) == (7, 0)
    assert run_timer([(1, 1, 7), (1, 0, 7)]) == [(7, 0), (6, 0)]


def test_pause_clears_pulse():
    """with en at 0 the count holds and expired goes back to 0 after its one cycle"""
    edges = [(0, 1, 1), (1, 0, 1), (1, 0, 1), (0, 0, 1), (0, 0, 1)]
    assert run_timer(edges) == [(1, 0), (0, 0), (1, 1), (1, 0), (1, 0)]


def test_reset_values_and_width():
    """starts from reset, gives no values without edges, and keeps only width bits of reload"""
    assert run_timer([]) == []
    assert run_timer([(1, 0, 0)]) == [(0, 1)]
    assert run_timer([(0, 1, 5)], width=2) == [(1, 0)]

The self-check covers names, the two blocks, loops and second writers, and the done check. Its questions begin with the idea your answers have handled worst, get harder as you get them right, and repeat a missed one before the end:

Interview questions

Warm-up (fresher): what is the two-process coding style, and what does it buy you?

It splits each block into one always_comb, which computes every register’s next value, the _d signals, from the registers and the inputs, and one always_ff, which loads those values into the registers, the _q signals, at the clock edge after their reset values. It buys three things. Every register has one writer and one reset, so a missing reset or a second driver stands out. The combinational block starts with defaults, so no path leaves a value unassigned and no latch appears. And the clocked block only loads, so blocking and nonblocking assignments never mix in it. It changes the code, not the hardware: in this track’s timer the two-process version synthesised to 42 cells against 43 for the one-block version, with the same nine flip-flops.

Next come two questions for experienced engineers: cleaning up an inherited module, and writing a team’s definition of done. Sketch your opening move for each before you look at its first step:

Key takeaways

• Names carry what a reader needs: _q and _d for a register and its next value, _n for active low, _i and _o for ports, one module per file. The first timer, tmr in timer_before.sv with an unused spare, earned two of its four warnings from names and files alone.
• In the two-process style always_comb computes every next value, defaults first, and always_ff only loads them; the refactored timer agreed with the first one on all 506 cycles and synthesised to 42 cells against 43, with the same nine flip-flops.
• Icarus Verilog compiled a combinational loop and a variable with two writers without a word; Verilator’s lint and Yosys’s checks caught both, and the loop held a value that nobody declared.
• A definition of done is a list that a script can run: lint with no warnings, synthesis without a latch and with a passing check -assert, $error on impossible parameters, and a self-checking testbench.