32-bit counter and test bench

Question

How could this VHDL counter and its test bench be improved? I am interested in anything you see that could be done better, but especially in the test bench:

• Is wait for 10 ns better or worse than any other time delay?
• The test is very minimal. Should it do more?

counter32.vhdl:

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity counter32 is
  port (
    clk: in std_ulogic;
    ena: in std_ulogic;
    rst: in std_ulogic;
    q  : out std_ulogic_vector(31 downto 0));
end entity;

architecture rtl of counter32 is

  signal count : unsigned(31 downto 0);

begin

  process(clk, rst)
  begin
    if rst = '1' then
      count <= X"00000000";
    elsif rising_edge(clk) then
      if ena = '1' then
        count <= count + 1;
      end if;
    end if;
  end process;

  q <= std_ulogic_vector(count);

end architecture;

counter32_tb.vhdl:

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity counter32_tb is
end counter32_tb;

architecture tb of counter32_tb is

  component counter32
    port (
      clk: in std_ulogic;
      ena: in std_ulogic;
      rst: in std_ulogic;
      q  : out std_ulogic_vector(31 downto 0));
  end component;

  signal clk: std_ulogic;
  signal ena: std_ulogic;
  signal rst: std_ulogic;
  signal q  : std_ulogic_vector(31 downto 0);

begin

  dut : counter32 port map (
    clk => clk,
    ena => ena,
    rst => rst,
    q => q);

  process
  begin

    -- Reset
    clk <= '0';
    ena <= '1';
    rst <= '1';
    wait for 10 ns;
    rst <= '0';
    wait for 10 ns;

    -- Counts on clock leading edge if enabled
    clk <= '1';
    wait for 10 ns;
    assert q = x"00000001";

    -- Does not count on clock trailing edge
    clk <= '0';
    wait for 10 ns;
    assert q = x"00000001";

    -- Does not count when not enabled
    ena <= '0';
    clk <= '1';
    wait for 10 ns;
    assert q = x"00000001";

    -- Clears on reset
    rst <= '1';
    wait for 10 ns;
    assert q = x"00000000";

    wait;

  end process;

end tb;

Shell script to run the test bench in GHDL:

OPTS=--workdir=build
ghdl -a ${OPTS} *.vhdl
ghdl -e ${OPTS} counter32_tb
ghdl -r ${OPTS} counter32_tb --vcd=counter32.vcd

Answer 1

Nice with some HDL on Code Review. I'll contribute with my opinions. All-in-all, I find the code all well-written.

Opinions not already covered in previous answers

• A more standard way of setting count at reset would be (others => '0').

  • That is, as opposed to hard-coding a bit/hex literal - even if you for some reason hard-code the width as you have done, (others => '0') is more standard in this case.

• You might have a good reason for asynchronous reset, but if not, synchronous reset is a better habit.

  • You often see these asynchronous resets, it is unfortunately a more common habit, since that's what is (or was) most often taught.
  • See my motivations here: http://www.fpga-dev.com/resets-make-them-synchronous-and-local/

Opinions already covered/partly covered

Somewhat in order of importance (in my subjective opinion).

• Define the roll-over behaviour.

  • The behaviour is actually defined for an unsigned in numeric_std: any carry bits will be ignored so the unsigned will roll-over. However, neither simulators nor synthesis tools should be trusted on following this. (And even less, humans reading the code.) Also; it is good to really think about what behaviour you desire, or is the most stable: roll-over or saturate? I'll take the chance to give two comments on this choice:
    • For synthesis, roll-over rather than saturation will yield less logic and improved timing. The tool will be able to use a counter macro right-off.
    • If reaching max count is expected never to happen, add an assertion so a simulation will flag in that case. Then still code the RTL behaviour explicitly to roll-over (less logic, better timing) if there aren't stability reasons to code it to saturate.

• Parameterize the width. Put it in a constant or generic.

• Generate the clock separately (e.g. clk <= not(clk) after 10 ns;, and make sure to init clk to '1'.

  • This is more standard and more readable.
  • Then, when coding your stimuli, do not dead-reckon the time for stimuli assignments, instead, use wait until rising_edge(clk); etc. This makes code more stable, and gives deterministic and proper times for the stimuli changes (not only in regards to time, but also to simulation deltas, which is equally important).

• Use entity instantiation

  • More details and motivation: http://www.fpga-dev.com/leaner-vhdl-with-entity-instantiation/

Simulation performance

• There's nothing wrong in coding the RTL of the counter like this, but it could be noted that there is a simulation performance gain possible: Make count a variable, local to the process, do tests and arithmetics on this variable, and in the end of the process drive the variable to a counter signal (defined in the architecture).

Others

• Thumbs up for self-checking test benches.

Answer 2

Contrary to what Aseem Bansal states, there is no need to use std_logic alone. Your use of std_ulogic is fine.

One place where there is a trade-off to be pondered is on your top-level entity (the one which defines the whole chip). The tools that generate the output will create you a simulation model with all the gates and delays in, but it will have std_logic on the IO pins. This can be plugged straight into your existing testbench if you've already used std_logic at the top.

You can avoid lots of duplicated typing by using direct instantiation. You don't need this at all:

component counter32
    port (
      clk: in std_ulogic;
      ena: in std_ulogic;
      rst: in std_ulogic;
      q  : out std_ulogic_vector(31 downto 0));
  end component;

if you do this:

  dut : entity work.counter32 port map (
    clk => clk,
    ena => ena,
    rst => rst,
    q => q);

This is the preferred method these days.

In your testbench, I would separate out the clock generation to its own process. Or even to a single line:

clk <= not clock after 10 ns when finished /= '1' else '0';

Create a signal called finished which is std_logic. You can then drive it low from every source and data-checking entity in the testbench, and drive it to '1' as each of them finish. Once they have all finished, the signal will finally resolve to '1' and the clock will stop.

You can then use wait for rising_edge(clk) in your checking process, to wait for the appropriate clock edges (I'm sure you can guess how to wait for a falling edge :)

I would also add a test to check it rolls over correctly. This can be a pain for such a big counter, so you might want to have a generic to control the width and then you can test a smaller counter to start with. You can wait for multiple clock ticks with a for loop with a wait for rising_edge inside.

While I'm here, some good stuff you've already done:

• Full marks for using numeric_std.
• And for std_ulogic - it helps avoid multiple driver mistakes very early on!
• Creating a self-checking testbench is something often overlooked, it's a great habit to get into.