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

• stanri: Why is it better to use begin on a new line after the always?
• Chiggs: @StaceyAnne That's just a formatting artefact from in-line markup. The improvement is moving from always @ to always_ff @ which tell the tools more about your intentions and allow them to perform additional checks for you.
• Chiggs: Yes, it's SystemVerilog, but plain Verilog is effectively deprecated having been merged into the SV standard. Only stick with Verilog if have very good reason to stay decades behind (i.e. forced to use some very old tools).
• Chiggs: ISE has been maintenance only for 6 months, it's effectively deprecated by Vivado which has been around for over 2 years and has reasonable SV support. For new designs you should consider switching from ISE to Vivado, but as ever the client is always right :)
• stanri: Vivago only supports Gen7 and up, though. Spartan/virtex 6 and below still only are develop-able in ISE, So you're stuck if you want to use a device that's more than a couple years old.

Answer 2

FSM styles

Your code and the previous answer implement the FSM in a single always block, and this is certainly a valid way to do so.

Another common practice is to use two always blocks:

• A sequential logic block for the current state
• A combinational logic block for the next state logic

Sequential

The sequential logic block for the current state is very simple:

always @(posedge m_axi_clk) begin 
 if (m_axi_resetn == 0) begin
 current_state <= S_IDLE;
 end else begin
 current_state <= next_state;
 end
end

As in your code, this uses nonblocking assignments (<=), which is good coding practice for sequential logic.

Combinational

The combinational logic block for the next state logic uses the implicit sensitivity list:

always @*

This avoids a common Verilog pitfall of forgetting to add signals to the sensitivity list that should be there.

Consider using always_comb instead of always *:

always_comb

This syntax requires you to enable SystemVerilog features in your simulator, if they are not already enabled by default. The advantage is that it better conveys the design intent and it provides some built-in checks. This is why the previous answer recommended using always_ff.

Here is what the combinational block would look like:

always @* begin 
 next_state = current_state;
 if ((ps_axi_busy == 0) && (ps_wr_en_i == 0)) begin
 case (current_state)
 S_IDLE : next_state = S_WAIT_FOR_CONFIG;
 S_WAIT_FOR_CONFIG : next_state = (config_done) ? S_WAIT_FOR_DMA : S_WAIT_FOR_CONFIG;
 // ...
 S_HALTED : next_state = S_HALTED;
 default : next_state = S_HALTED;
 endcase 
 end 
end 

You would declare next_state the same way you declared current_state, using a reg type with the same bit width.

There are a few things to note about this style. Since you change state only when enabled by the if condition, you need to make sure next_state is assigned under all conditions, even when the if is false. This is accomplished by the first statement:

next_state = current_state;

This technique avoids unwanted latch behavior during simulation and latch inference during design synthesis.

The next thing to notice is that it is good practice to use blocking assignments (=).

For simple next-state logic, like you have shown, you can have one line per item in the case statement. Instead of using the if/else, you can use the ternary operator (() ? :).

If your logic is more complicated than shown, then it would be better to use begin/end on multiple lines for all case items, for consistency.

Deadlock

When I see a line like this:

S_HALTED : next_state = S_HALTED;

it could be a sign that the state machine could get stuck in that state. Typically, an FSM always has a path to a different state.

Scaling

How do I handle scaling this state machine for a more complicated protocol

If the FSM becomes too unwieldy, it may make sense to split the FSM into multiple state machines.

Partitioning

It is generally a good practice to split the logic up into multiple always blocks. As you have discovered, too much Verilog code in a single block can become difficult to understand and maintain.