[SOLVED] Post PAR Simulation fails in this FSM logic, please help me review

[xtcx]

Hi friends
Just I could not understand why XST (Xilinx synthesis tool) synthesizes this piece of code and removes everything (almost everything) with some warnings saying : xxx has constant value and hence removing. The logic, is simple.

A timer counts for some time and generates a pulse which triggers the FSM logic to write 64-bit data into FIFO by generating Fifo write signal.Once it writes all 41 QWords twice, the loop repeats

I know this is not a good way of writing an FSM using an integer as state selector. Out of my interests, I did this on purpose to see how it behaves. The behavioral simulation ran correct as expected, but in hardware all I found is nothing other than GRND driving to DL_Data and DL0_Fifo_Wr. All logics are removed.

[B]some warnings [/B]
WARNING:Xst:737 - Found 1-bit latch for signal <next_SS<3>>. Latches may be generated from incomplete case or if statements. We do not recommend the use of latches in FPGA/CPLD designs, as they may lead to timing problems.
WARNING:Xst:737 - Found 1-bit latch for signal <next_SS<1>>. Latches may be generated from incomplete case or if statements. We do not recommend the use of latches in FPGA/CPLD designs, as they may lead to timing problems.
WARNING:Xst:737 - Found 1-bit latch for signal <next_SS<0>>. Latches may be generated from incomplete case or if statements. We do not recommend the use of latches in FPGA/CPLD designs, as they may lead to timing problems.

WARNING:Xst:1293 - FF/Latch <next_SS_0> has a constant value of 0 in block <DL_Data_Pump>. This FF/Latch will be trimmed during the optimization process.
WARNING:Xst:1293 - FF/Latch <next_SS_3> has a constant value of 0 in block <DL_Data_Pump>. This FF/Latch will be trimmed during the optimization process.
WARNING:Xst:1293 - FF/Latch <next_SS_1> has a constant value of 0 in block <DL_Data_Pump>. This FF/Latch will be trimmed during the optimization process.

WARNING:Xst:1895 - Due to other FF/Latch trimming, FF/Latch <DL_Data_45> (without init value) has a constant value of 0 in block <DL_Data_Pump>. This FF/Latch will be trimmed during the optimization process.
WARNING:Xst:1895 - Due to other FF/Latch trimming, FF/Latch <DL_Data_44> (without init value) has a constant value of 0 in block <DL_Data_Pump>. This FF/Latch will be trimmed during the optimization process.
WARNING:Xst:1895 - Due to other FF/Latch trimming, FF/Latch <DL_Data_43> (without init value) has a constant value of 0 in block <DL_Data_Pump>. This FF/Latch will be trimmed during the optimization process.

however when I changed the next state process, when others => next_ss <= 0;
I just got my code synthesized. All I want to know is, after all covering all states, what does it have to do with completely removing hardware??..Latches were removed, but how good does that solve this case?.

Actual Code:-

-- Target Devices: Virtex 6, 240 LXT
-- Tool versions:  ISE 13.2
-- Description:    This module will generate 2 DL packets and send them continuously to GTX encoder.
--
-- Dependencies: 
--
-- Revision: 
-- Revision 0.01 - File Created
-- Additional Comments: 
-- 
----------------------------------------------------------------------------------
library IEEE;
use IEEE.STD_LOGIC_1164.ALL;


entity DL_Data_Pump is
--    generic ( timeout   : integer := 500);--5187);
    Port ( Clk          : in  STD_LOGIC;
           Reset        : in  STD_LOGIC;
           DL0_Fifo_wr  : out  STD_LOGIC;
           DL1_Fifo_wr  : out  STD_LOGIC;
           DL_Data      : out  STD_LOGIC_VECTOR (63 downto 0)
          );
end DL_Data_Pump;

architecture Behavioral of DL_Data_Pump is

  type array_big is array (0 to 40) of std_logic_vector(63 downto 0);
  constant cluster_config_data	: array_big:=( x"2324262A2A262423",
                                                x"0018300041054104", x"0001006407800001", 	
                                                x"000700c000060002",	x"000500d000020064",
                                                x"000b395f000a0001",	x"0009000000080144",
                                                x"000fc821000e0003",	x"000dffff000c0001",
                                                x"0013000000120003",	x"0011ffff00100003",
                                                x"001900d800180048",	x"00179ff400140000",
                                                x"078400030783ffff",	x"0782000107813935",
                                                x"0788fac8078795c0",	x"0786000007850000",
                                                x"078cff00078b9966",	x"078a969607890000",
                                                x"07906996078f0ff0",	x"078e6666078dff00",
                                                x"0794f0f007930ff0",	x"079200000791f0f0",
                                                x"0798000007979696",	x"0796666607959966",
                                                x"079d00e5079c00e5",	x"079b6ac0079a28d8",
                                                x"07a1038307a00000",	x"079f0000079e0000",
                                                x"07a53a0007a40000",	x"07a3001007a20000",
                                                x"07a9014407a80001",	x"07a7000007a60000",
                                                x"07ad100007ac7fff",	x"07ab100007aa0066",
                                                x"07b2000007b00000",	x"07af000607ae8000",
                                                x"0000000307b60000",	x"07b5000007b40000",
                                                x"0780000300000001",x"2A2624232324262A"
                                                );
signal timer_cnt           : integer range 0 to 65535 := 0;
signal timer_trigger,timer_enable       : std_logic:='0';
signal present_SS,next_SS  : integer range 0 to 15 := 0;
signal timeout             : integer range 0 to 65535:=0;
signal i                   : integer range 0 to 63 := 0;


begin
----------------------
-- Local timer 
----------------------
timer_mod : process(clk)
begin

   if rising_edge(clk) then
      if reset = '1' or timer_enable = '0' then
         timer_cnt       <= 0;
         timer_trigger   <= '0';
      else
         timer_cnt <= timer_cnt + 1;
         if (timer_cnt < timeout) then
            timer_trigger <= '0';
         else
            timer_cnt      <= 0;
            timer_trigger   <= '1';
         end if;
      end if;
   end if;      

end process;
--------------------------------------
-- Sequential Present state process
-------------------------------------
process(clk)
begin

   if rising_edge(clk) then
      if reset = '1' then
         DL_Data     <= (others => '0');
         DL0_fifo_wr     <= '0';
         present_SS  <= 0;
         i           <= 0;
         DL_Data   <= (others => '0');
         DL1_fifo_wr     <= '0';
      else
         present_SS <= next_SS;
         case present_SS is
            when 0   => timer_enable   <= '1';                                      -- Enable timer      
                        timeout        <= 5000;                                      -- Load timeout value
            when 1   => timer_enable   <= '0';                                      -- Disable timer      
                        i <= i + 1;                                                 -- Array counters      
                        if (i = 0) then
                           DL0_fifo_wr    <= '1';
                           DL_Data        <= cluster_config_data(i);
                        elsif (i > 0) and (i < 41) then
                           DL_Data        <= cluster_config_data(i);
                        else
                           DL0_fifo_wr             <= '0';                         -- Disable FIFO WRite
                           i                       <= 0;
                        end if;
            when 2   => timer_enable   <= '1';                                     -- Enable Timer
                        timeout        <= 50;                                      -- Load timeout value
            when 3   => timer_enable   <= '0';                                     -- Disable Timer
                        i <= i + 1;                                                 -- Array counters      
                        if (i = 0) then
                           DL1_fifo_wr           <= '1';
                           DL_Data        <= cluster_config_data(i);
                        elsif (i > 0) and (i < 41) then
                           DL_Data        <= cluster_config_data(i);
                        else
                           DL1_fifo_wr           <= '0';                          -- Disable FIFO WRite
                           i                     <= 0;
                        end if;
            when others => null;            
         end case;
      end if;      
   end if;   

end process;
---------------------------------
-- Combo State change process
---------------------------------
process(next_ss, timer_trigger, i)
begin
   
   case next_ss is
      when  0  => if timer_trigger = '1' then
                     next_SS <= 1;
                  else
                     next_SS <= 0;
                  end if;
      when 1   => if (i > 40) then
                     next_SS <= 2;
                  else
                     next_SS <= 1;
                  end if;   
      when 2   => if timer_trigger = '1' then
                     next_SS <= 3;
                  else
                     next_SS <= 2;
                  end if;
      when 3   => if (i > 40) then
                     next_SS <= 0;
                  else
                     next_SS <= 3;
                  end if; 
      when others => null;
   end case;
 
 end process;     
      



end Behavioral;

 

[mrflibble]

Disclaimer: no habla VHDL senor!

Working under the assumption that vhdl case statements are more or less finicky in the same way that they are in verilog. Your next_ss is an integer range 0 to 15, but you only specify cases for case numero 0,1,2,3. So you have values 4 - 15 unspecified. And I vaguely recall that the "when others" is the same as the "default" statement in verilog. So the "when others" statement is needed to specify what should be done for those other value 4-15. I know you want to say "but I dont use those values". Yes indeed, but logic synthesis does not do mind reading. So either you can specify all the when 4 ... through when 15 cases, which might be a bit much. If you don't do that then you need a default way of handling all the other values that you do not explicitely handle.

And that matches perfectly with it simulating just fine, but giving problems with synthesis. In simulation runtime there is no problem, as long as you make sure that you start the FSM at a known state (probably 0) and then stay within the 0,1,2,3 values. But during synthesis the tools really need to know what all the possible 0 ... 15 values are supposed to do. Either explicitly (when 1 => etc) or implicitly (when others => etc).

 

[xtcx]

Disclaimer: no habla VHDL senor!

Working under the assumption that vhdl case statements are more or less finicky in the same way that they are in verilog. Your next_ss is an integer range 0 to 15, but you only specify cases for case numero 0,1,2,3. So you have values 4 - 15 unspecified. And I vaguely recall that the "when others" is the same as the "default" statement in verilog. So the "when others" statement is needed to specify what should be done for those other value 4-15. I know you want to say "but I dont use those values". Yes indeed, but logic synthesis does not do mind reading. So either you can specify all the when 4 ... through when 15 cases, which might be a bit much. If you don't do that then you need a default way of handling all the other values that you do not explicitely handle.

And that matches perfectly with it simulating just fine, but giving problems with synthesis. In simulation runtime there is no problem, as long as you make sure that you start the FSM at a known state (probably 0) and then stay within the 0,1,2,3 values. But during synthesis the tools really need to know what all the possible 0 ... 15 values are supposed to do. Either explicitly (when 1 => etc) or implicitly (when others => etc).

Well, Thanks. But do you mean to say, that without an INIT or default statement, the hardware may jump start from 3 or 4 instead of 0 ...??

 

[mrflibble]

No. I mean that without some way of knowing what to do with those values 4 - 15, the synthesizer has no way of knowing how you would like to have things handled.

Regarding an INIT statement ... definitely use that if you target fpga's. IMO for fpga's it's a good habbit that will prevent all sorts of fun. But for this particular problem (case statements) it will not solve the "synthesizer problem".

Anyways, lets turn the problem around ... what do you expect the synthesizer to do with a 4-bit input to a case statement, and you have only specified what it should do in 4 out of 16 cases. What should it do for those other 12 cases? And specifically, what should it write to ss_next?

Because as you notice, when you do not specify all cases, then you will infer latches. Which is probably not what you want.

I am currently moderately lazy, so you'll have to search a bit ...

http://www.sunburst-design.com/papers/

This guy has some damn good papers, and specifically he has 1 or 2 papers on best practices for case statements and for FSMs. You may find those useful.

 

[TrickyDicky]

Why have you got the states specified at a 0 to 15 integer, when only 0 to 3 are possible? why not spec the present_SS and next_SS to a 0 to 3 integer?
Secondly - you should be checking the present_SS, not the next_SS in the state change process and case statement. This is what is causing the latches and possible logic loops, because you're asking a signal to change its state based on itself. Also, like mr flibble said, you should always specify what should happen in ALL cases in an asynchronous process. the when others => null; bit is telling the next_SS to hold its state, which creates the latch (and according to your logic, if it gets into an illegal state, it will never recover).

The best way to deal with this is create a single synchronous process state machine. 2 process state machines are a thing of tha past and old text books.

 

[xtcx]

No. I mean that without some way of knowing what to do with those values 4 - 15, the synthesizer has no way of knowing how you would like to have things handled.

Regarding an INIT statement ... definitely use that if you target fpga's. IMO for fpga's it's a good habbit that will prevent all sorts of fun. But for this particular problem (case statements) it will not solve the "synthesizer problem".

Anyways, lets turn the problem around ... what do you expect the synthesizer to do with a 4-bit input to a case statement, and you have only specified what it should do in 4 out of 16 cases. What should it do for those other 12 cases? And specifically, what should it write to ss_next?

Because as you notice, when you do not specify all cases, then you will infer latches. Which is probably not what you want.

I am currently moderately lazy, so you'll have to search a bit ...

http://www.sunburst-design.com/papers/

This guy has some damn good papers, and specifically he has 1 or 2 papers on best practices for case statements and for FSMs. You may find those useful.

Yeah I understood, this not a safe FSM. 4\16 cases. Well, what I don't understand is, why would the hardware be removed completely after all I did not complete the case statement.... I'm just trying to know what is happening in hardware, that's all.

Thanks for the info.

---------- Post added at 13:49 ---------- Previous post was at 13:41 ----------

Why have you got the states specified at a 0 to 15 integer, when only 0 to 3 are possible? why not spec the present_SS and next_SS to a 0 to 3 integer?
Secondly - you should be checking the present_SS, not the next_SS in the state change process and case statement. This is what is causing the latches and possible logic loops, because you're asking a signal to change its state based on itself. Also, like mr flibble said, you should always specify what should happen in ALL cases in an asynchronous process. the when others => null; bit is telling the next_SS to hold its state, which creates the latch (and according to your logic, if it gets into an illegal state, it will never recover).

The best way to deal with this is create a single synchronous process state machine. 2 process state machines are a thing of tha past and old text books.

Thanks, and sorry that it was a typo. Somehow I forgetfully checked the next_ss and hence all these issues. Yeah I received a combo loop back warning and was just wondering why it poped up. This typo is the problem...
Thanks for pointing.

Ofcourse that should be 0 to 3, somehow I wanted to add more states at later point, so just left that...

But I quite dont agree why would you say

The best way to deal with this is create a single synchronous process state machine. 2 process state machines are a thing of tha past and old text books.

I practiced myself with this 2 process FSM and what is so old about it?.

 

[mrflibble]

Well, what I don't understand is, why would the hardware be removed completely after all I did not complete the case statement.... I'm just trying to know what is happening in hardware, that's all.

I could make all sorts of stuff, but in the end it boils down to it being a hardware description language. So if your description of A is incomplete it will not infer A. And as you noticed, sometimes incomplete descriptions of A are still valid descriptions of B. And before you know it you are now the proud owner of some latches. With case statements, one of the first things I do is plug in a default, so that I will not randomly forget about it.

The best way to deal with this is create a single synchronous process state machine. 2 process state machines are a thing of tha past and old text books.

I practiced myself with this 2 process FSM and what is so old about it?.

I was wondering the same thing. In the past I have done both the 1 process and 2 process thing. I find the 2 process FSMs to be a bit more readible. Maybe that's an artifact of VHDL vs verilog, but I doubt it since the concepts are exactly the same. Or maybe it's just me. Anyways, what's the big advantage of a 1 process FSM? All I can think of is that the total lines of code will be less for a 1 process FSM, and that's about it...

 

[TrickyDicky]

From my understanding, older synthesisor would only accept 2 process state machines, hence, like std_logic_arith/unsigned/signed, they became the norm and all older text books teach this method. The problem with them is you risk latch generation when you forget to assign the next state in all branches, and problems with simulation when you forget a signal in the sensitivity list. It is a method of coding that separates out async logic and sync logic into separate processes, and nowadays everyone always tries to avoid async logic. If you're going to code like this, why not make ALL code 2 processes - gates in one process and registers in the other, I dont think anyone would advise that, so why do it with a state machine?

A 1 process state machine doesnt care about the sensitivity list (as the only one in there is clock, and maybe reset), and there is no danger of generating latches. You also assign the state machine directly, rather than having a current state and next state signal.

 

[xtcx]

Thanks for your suggestions mr.flibble and tricky, point is I saw some example in xilinx templates for FSM long time back and hence I decided this is the standard or best\optimized way of writing FSM. However, writing single process is easier and less confusing, but 2 process is easier to read. If at all these two are just the same with 2 process increasing the latch if written wrongly, then I'd better stick with 1 process.

Thanks friends.