Implementation of interface within state machine

This particular example is verilog, but my question is more about the state machine structuring, which would be relevant to both VHDL and verilog.

So if I have a state machine, this one is fairly simple:

always@(posedge m_axi_clk) begin        
if(m_axi_resetn == 0) begin
  current_state   <= S_IDLE;
end
else begin
  if ((ps_axi_busy == 0) && (ps_wr_en_i == 0)) begin
      case(current_state)
    S_IDLE: current_state <= S_WAIT_FOR_CONFIG;
    S_WAIT_FOR_CONFIG:
      if (config_done == 1)
        current_state <= S_WAIT_FOR_DMA;
      else
        current_state <= S_WAIT_FOR_CONFIG;
            ...
    S_HALTED:         current_state <= S_HALTED;
    default:          current_state <= S_HALTED;
      endcase 
  end 
end
end

If I want to perform, for example, a BRAM write triggered from the state machine state, I usually split the processes up, so I'll duplicate the entire process and then do the following:

always@(posedge m_axi_clk) begin        
if(m_axi_resetn == 0) begin
  ps_wr_addr_i  <= 0;
  ps_wr_data_i  <= 0;
  ps_wr_en_i    <= 0;
end
else begin
  if ((ps_axi_busy == 0) && (ps_wr_en_i == 0))
    case(current_state)
      S_IDLE:
    begin       
        ps_wr_addr_i  <= `INPUT_NEXT;
        ps_wr_data_i  <= ones;
        ps_wr_en_i    <= 1;
    end
      S_WAIT_FOR_CONFIG:
    begin
        ps_wr_addr_i  <= 0;
        ps_wr_data_i  <= 0;
        ps_wr_en_i    <= 0;     
        if (config_done == 1) begin
          ps_wr_addr_i  <= `INPUT_DATA;
          ps_wr_data_i  <= ones;
          ps_wr_en_i    <= 1;
        end
    end
          ...
      default:
    begin
        ps_wr_addr_i  <= 0;
        ps_wr_data_i <= 0;
        ps_wr_en_i <= 0;
    end
    endcase // case (current_state)
  else begin
      ps_wr_addr_i  <= 0;
      ps_wr_data_i <= 0;
      ps_wr_en_i <= 0;      
  end // else: !if(ps_axi_busy == 0)
end // else: !if(m_axi_resetn == 0

I used to do one big process, where I merge everything together, but that gets big fast f

As with all questions on codereview, some of the comments below is just opinion and there are always several ways of doing things. However my personal style preferences have evolved over many years of debugging my own (and other people's) code!

First some style comments on the code itself:

always@(posedge m_axi_clk) begin

Unless you're stuck with very out-dated tools it's better to use always_ff @(posedge m_axi_clk) begin.

case(current_state)
  S_IDLE:
begin

Personally I find your placement of begin a bit inconsistent - same line for if statements but a newline for case entries? The indentation makes it slightly harder to follow.

ps_wr_addr_i  <= 0;
    ps_wr_data_i <= 0;

Do you actually need to drive the addr and data lines low when not doing a write? Often they're qualified by the en so this generates superfluous logic.

General comments


If I want to perform, for example, a BRAM write triggered from the state machine state, I usually split the processes up, so I'll duplicate the entire process

The "duplicate the entire process" raises a red flag here. Code duplication is generally wrong and should be avoided if there's a better way to do things.


I used to do one big process, where I merge everything together, but that gets big fast for multiple interfaces and multiple states. Then it's also difficult to sift through the signals to check that each write is in the right place.

So here we get to the crux of the problem. Your current style is making a single process unwieldy, but splitting into multiple processes forces you to make compromises.

My advice would be to avoid multiple processes unless necessary. It's very difficult to debug code where many processes are interacting. It also becomes hard to maintain - when adding code where does it go? What if I need a new synchonisation flag to communicate between processes? Even if it starts off well, over time it will grow into bad code!

My advice would be to use the following style:

• Only have a single case statement for your state machine

• Stick to a single process if possible

• Easier to debug, modify, maintain

• Will also improve simulation speed (marginally)

• Only resort to 2 processes if you need to drive outputs asynchronously

• Collect related signals together in structures (or interfaces)

• Factor conditions that are used multiple times outside the state-machine

• Re-factor common actions to the end of the process

• use blocking assignments to indicate which actions to take

The main goal is to simplify the state machine - ideally you want it to read naturally. Here is an example showing some of these in action:

typedef struct packed {
    logic [31:0]                data;
    logic [7:0]                 addr;
    logic                       en;
} write_t;

write_t                         write;

assign some_condition = (bing & bong) || (ding & ~dong);

always_ff @(posedge clk) begin

    // Defaults
    if (write_accepted)
        write.en <= 1'b0;

    case (state)
        IDLE: begin
            if (some_condition)
                state           <= WAIT_FOR_CONFIG;
        end

        WAIT_FOR_CONFIG: begin

            if (some_condition)
                state           <= ANOTHER_STATE;
            else if (some_other_condition)
                state           <= THE_OTHER_STATE;
            else begin
                state           <= SOMETHING_ELSE;
                write.addr      <= that_address;
                do_write        = 1'b1;             // Blocking assignment
            end

        end

        ...

    endcase

    // Common actions
    if (do_write) begin
        write.en        <= 1'b1;
        ...
    end

end

StackExchange Code Review Q#54786, answer score: 4