How to Rewrite FSM not to use Latches

Question

I have an FSM and it works. The synthesizer, however, complains that there are latches for "acc_x", "acc_y", and "data_out" and I understand why and why it is bad. I have no idea, however, how to rewrite the FSM so the state-part goes to the clocked process. Any ideas where to start from? Here is the code of the FSM:

---

library IEEE;

use IEEE.std_logic_1164.all;
use IEEE.numeric_std.all;

entity storage is
    port
    (
        clk_in                           : in  std_logic;
        reset                            : in  std_logic;
        element_in                       : in  std_logic;
        data_in                          : in  signed(11 downto 0);
        addr                             : in  unsigned(9 downto 0);
        add                              : in  std_logic; -- add = '1' means add to RAM
                                                          -- add = '0' means write to RAM
        dump                             : in  std_logic;
        element_out                      : out std_logic;
        data_out                         : out signed(31 downto 0)
    );
end storage;

architecture rtl of storage is
    component bram is
    port
    (
        clk                              : in  std_logic;
        we                               : in  std_logic;
        en                               : in  std_logic;
        addr                             : in  unsigned(9 downto 0);
        di                               : in  signed(31 downto 0);
        do                               : out signed(31 downto 0)
    );
    end component bram;

    type state is (st_startwait, st_add, st_write);

    signal current_state                 : state := st_startwait;
    signal next_state                    : state := st_startwait;

    signal we                            : std_logic;
    signal en                            : std_logic;
    signal di                            : signed(31 downto 0);
    signal do                            : signed(31 downto 0);

    signal acc_x                         : signed(31 downto 0);
    signal acc_y                         : signed(31 downto 0);
begin
    ram : bram port map
    (
        clk  => clk_in,
        we   => we,
        en   => en,
        addr => addr,
        di   => di,
        do   => do  
    );

    process(clk_in)
    begin
        if rising_edge(clk_in) then
            if (reset = '1') then
                current_state           <= st_startwait;
            else
                current_state           <= next_state;
            end if;
        end if;
    end process;

    process(current_state, element_in, add, dump, data_in, do, acc_x, acc_y)
    begin
        element_out                     <= '0';

        en                              <= '1';
        we                              <= '0';

        di                              <= (others => '0');

        case current_state is
            when st_startwait =>          
                if (element_in = '1') then
                    acc_x               <= resize(data_in, acc_x'length);

                    next_state          <= st_add;
                else
                    next_state          <= st_startwait;
                end if;
            when st_add =>
                if (add = '1') then
                    acc_y               <= acc_x + do;
                else
                    acc_y               <= acc_x;
                end if;

                next_state              <= st_write;
            when st_write =>      
                if (dump = '1') then
                    data_out            <= acc_y;
                    element_out         <= '1';
                else
                    di                  <= acc_y;
                    we                  <= '1';
                end if;

                next_state              <= st_startwait;
        end case;
    end process;  
end rtl;

---

Answer 1

Clearly you need supplemental (clocked) registers (D flip flops).

Your need to ask yourself "what occurs to acc_x if the FSM is in (let say) state st_add ?". Your answer is "I dont want to modify acc_x in this state". So : write it explicitely, using clocked registers (such as the one used for state ; you can augment the clocked process with these supplemental registers). Do that Everywhere. That is the rule. Otherwise, synthesizers will infer transparent latches to memorize the previous value of acc_x : but these transparent latches violate the synchronous design principles. They structurally imply combinatorial loops in your designs, which are bad.

Put another way : ask yourself what is combinatorial and where are the registers ? If you have registers in mind, code them explicitly. Do not make combinatorial signals assigned and read in the same process.

Answer 2

The reason for the inferred latches is that the case in the last process does not drive all signals in all possible combinations of the sensitive signals. So the process can finish without altering some of the output data for some of the signal values to the process. To hold output in this way is the operation of a latch, so latches are therefore inferred by the synthesis tool.

The latches applies only to acc_x, acc_y, and data_out, since all other signals are assigned a default value in the beginning of the process.

You can fix this by either driving a default value for the last 3 signals in the beginning of the process, for example 'X' for all bit to allow synthesis freedom:

data_out <= (others => 'X');
acc_x    <= (others => 'X');
acc_y    <= (others => 'X');

Alternatively can you can ensure that all outputs are driven in all branches of the case, and you should then also add a when others => branch to the case.

I suggest that you use the assign of default value to all signals, since this is easier to write and maintain, instead of having to keep track of assign to all driven signals in all branches of the case.

Answer 3

This is personal preference, but I think most people on here will agree with me on this one... do not use two processes to control your state machine. The whole previous_state next_state thing is total garbage in my opinion. It's really confusing and it tends to make latches - SURPRISE - You found that out. Try rewriting your state machine with a single clocked process and only one state machine signal.

Here's my attempt at rewriting your state machine. Note that I'm not sure the functionality that I have below will work for you. Simulate it to make sure it behaves the way you expect. For example the signal en is always tied to '1', not sure if you want that...

process (clk_in)
begin
  if rising_edge(clk_in) then
    element_out <= '0';
    en <= '1';                      -- this is set to 1 always?
    we <= '0';

    di <= (others => '0');

    case state is

      when st_startwait =>
        if (element_in = '1') then
          acc_x <= resize(data_in, acc_x'length);
          state <= st_add;
        end if;

      when st_add =>
        if (add = '1') then
          acc_y <= acc_x + do;
        else
          acc_y <= acc_x;
        end if;
        state <= st_write;

      when st_write =>
        if (dump = '1') then
          data_out    <= acc_y;
          element_out <= '1';
        else
          di <= acc_y;
          we <= '1';
        end if;
        state <= st_startwait;
    end case;
  end if;
end process;