Implementation of interface within state machine

Question

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 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.

I usually take out all the states where writes don't occur and bunch them up together under default. This works nicely for small state machines, although if I do a write on a state edge, then I will need to maintain that condition through both processes. For example, I only switch out of S_WAIT_FOR_CONFIG (and do the write) when config_done is asserted. This condition is now in twice, and I have to change it in two places (something I'd like to avoid, if possible).

I've also been thinking about creating a delayed state signal and doing an edge detect on that in order to trigger certain BRAM writes, this allows the config_done to trigger the state switch, and then that edge triggers the writes. This is great for writes on state switch edges, but I can't do the same thing for writes that are in the middle of states.

There's also the asynchronous option:

assign ps_wr_addr_i = ((current_state == S_IDLE)             && (ps_axi_busy == 0))                          ? `INPUT_NEXT:
                      ((current_state == S_WAIT_FOR_CONFIG)  && (config_done)        && (ps_axi_busy == 0))  ? `INPUT_DATA:
                      0;
...

I prefer this from a readability/elegance standpoint, but due to the timing implications (no longer pipelined), I don't really use it that often. Also, once again the config_done check needs to be maintained in multiple places.

I understand that there are subtle variations in the behaviour of each of these three, so small adjustments will probably need to be made for similar behaviour overall. Also I think it depends on what is needed at the time as far as pipelining goes. However, I'm more interested behind the principal behind these writes.

Is there a better way of doing this, or is this it? How do I handle scaling this state machine for a more complicated protocol where writes are necessary perhaps at the beginning, middle, or end of a state? Do I make separate states for that, or do I use edge triggers/if statements? How do write this in an elegant way and stop it from just running away into an abomination of a state machine?

TL;DR I usually use a state machine as a way of setting up the framework of what gets done when, and then all the other signals revolve around that. I'm struggling to consolidate this "macro" framework with the "micro" details that involve timing multiple writes during one state. Am I thinking about this wrong and can I be using a state machine in a better way?

Answer 1

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