2
\$\begingroup\$

I'm trying to create an FSM that left shifts a register until the MSB is 1 while counting the number of shifts completed. However, I have an issue with latches because I don't re-assign each register on a state change; namely, the shift and ctr registers (see the "if (shift[3])" statements). To get rid of the latches, I know that I need to assign these registers, but I don't want their values to change at the end--I want them to remain constant.

How do I achieve this?

`define RST 2'b00
`define LSH1 2'b01
`define LSH2 2'b10
`define DONE 2'b11

module leftshift(clk, rst, x, shift);
    input [3:0] x;
    input clk, rst;
    output [3:0] shift;

    reg [1:0] state, nextstate;

    reg [3:0] shift; // reg holding shifted bits
    reg [2:0] ctr; // ctr keeps track of how many shifts until completion

    always @ (posedge clk)
        if (rst) state <= `RST;
        else state <= nextstate;

    always @ (state, x, shift[3])
        case (state)
            `RST: begin
                shift <= x;
                ctr <= 0;
                nextstate <= `LSH1;
            end
            `LSH1: begin
                if (shift[3]) begin
                    //shift <= //don't change;
                    //ctr <= //don't change;
                    nextstate <= `DONE;
                end
                else begin
                    shift <= {shift[2:0], 1'b0};
                    ctr <= ctr+1;
                    nextstate <= `LSH2;
                end
            end
            `LSH2: begin
                if (shift[3]) begin
                    //ctr <= //don't change
                    //shift <= //don't change
                    nextstate <= `DONE;
                end
                else begin
                    shift <= {shift[2:0], 1'b0};
                    ctr <= ctr+1;
                    nextstate <= `LSH1;
                end
            end
            `DONE: begin
                //shift <= //don't change;
                //ctr <= //don't change;
                nextstate <= `DONE;
            end
        endcase
endmodule
\$\endgroup\$
4
  • \$\begingroup\$ @ThePhoton yeah I'm getting latch warnings from Xilinx ISE 14.7 and I'm about to tear by hair out I just want these errors to go away and nothing I've tried is fixing it. edit: ctr <= ctr didn't fix the warning, btw \$\endgroup\$
    – user104243
    Dec 3, 2017 at 20:12
  • \$\begingroup\$ @ThePhoton That exactly creates a latch in a combinational always block. \$\endgroup\$
    – user154136
    Dec 3, 2017 at 20:30
  • \$\begingroup\$ @ahmedus please help I see from your profile that you're a hardware guy \$\endgroup\$
    – user104243
    Dec 3, 2017 at 20:38
  • \$\begingroup\$ @ahmedus, oops. Didn't look at the sensitivity list. \$\endgroup\$
    – The Photon
    Dec 3, 2017 at 20:38

3 Answers 3

4
\$\begingroup\$

What I see is the coding style where you have a registered and a combinatorial section. It is a good coding style but it also only works if you are 100% consistent in your code:
Everything you clock (state, counter, shift) must be in the clock section. and you must only use non-blocking "<=" assignments.
All combinatorial code must be fully! encoded and you must only use blocking "=" assignments.
Start by adding a next_shift and next_cnt and in the clocking section use

always @ (posedge clk)
   if (rst) 
   begin
      state <= `RST_STATE;
      shift <= `RST_SHIFT;
      cnt   <= `RST_CNT;
   end
   else
   begin
      state <= next_state;
      shift <= next_shift;
      cnt   <= next_cnt;
   end
 end

I am not going to re-code your whole combinatorial section (sorry) but there you use:

always @ ( * ) // easiest!
begin
    case (state)
        `RST_STATE: begin
            next_shift = x;
            next_ctr   = 0;
            next_state = `DONE;
        end
        `LSH1: begin
             if (shift[3]) begin
               next_shift = shift; // no change
               next_ctr   = cntr;  // no change
               next_state = `DONE; // 
             end
             else begin 
               next_shift = {shift[2:0], 1'b0};
               next_ctr   = cntr+1;  
               next_state = `LSH2;
             end
  etc...

end

As I said it is a good coding style because it can handle some nasty cases better but the other coding style (I just saw another answer appear) is much easier.

Disclaimer: code not compiled there may be typos in there

\$\endgroup\$
2
  • \$\begingroup\$ I prefer the 2-always block largely because of this paper which compares coding styles in terms of lines of code, synthesis timing, area synthesis area. 2-always blocks (w/ output encoded) wins most scenarios. No coding style is perfect for all situations (there wouldn't be a debate if there was), so it is go to be flexible if it is necessary to switch. \$\endgroup\$
    – Greg
    Dec 4, 2017 at 18:01
  • \$\begingroup\$ I have dropped processor cores from a big processor core supplier into silicon chips. I have seen their code and almost everywhere they follow that coding stile. I suspect it is a mandatory style. If a company of 3000 engineers with many years of experience follows it it can't be that bad. \$\endgroup\$
    – Oldfart
    Dec 9, 2017 at 21:58
2
\$\begingroup\$

Splitting sequential and combinational parts is not efficient for your design. In addition, the combinational always block has non-blocking assignments (<=), which may cause unexpected synthesis results.

A single sequential always block should solve the problem.

always @ (posedge clk) begin
    if (rst) begin
        state <= `RST;
        shift <= 0;
        ctr <= 0;
    end else begin
        case (state)
            `RST: begin
                shift <= x;
                ctr <= 0;
                state <= `LSH1;
            end
            `LSH1: begin
                if (shift[3]) begin
                    //shift <= //don't change;
                    //ctr <= //don't change;
                    state <= `DONE;
                end
                else begin
                    shift <= {shift[2:0], 1'b0};
                    ctr <= ctr+1;
                    state <= `LSH2;
                end
            end
            `LSH2: begin
                if (shift[3]) begin
                    //ctr <= //don't change
                    //shift <= //don't change
                    state <= `DONE;
                end
                else begin
                    shift <= {shift[2:0], 1'b0};
                    ctr <= ctr+1;
                    state <= `LSH1;
                end
            end
            `DONE: begin
                //shift <= //don't change;
                //ctr <= //don't change;
                state <= `DONE;
            end
        endcase
    end
end

Both ctr and shift are flip-flop here. If there is no assignment in a case, they will preserve their values. The assignment ctr <= ctr also does the same.

\$\endgroup\$
1
\$\begingroup\$

Since code specific answers have already been given correctly by others. I just have a general answer to this question.

To avoid latches in a HDL design , two major points you have to keep in mind.

1) Make sure that you cover all possible conditions in - case and if constructs, even though you feel that its not gonna appear in your design or you think is irrelavant. Because synthesizer will always "think" that such a condition will appear in your design and infer a latch for such conditions.

2) Once you did it the above thing correctly, the next step is to make sure that all the output/internal signals of your design get SOME value in every execution cycle, whatever the conditions are. Otherwise synthesizer will again infer latches to hold the previous values of uncovered signals.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.