cancel
Showing results for 
Show  only  | Search instead for 
Did you mean: 
YannZ80
Visitor
Visitor
398 Views
Registered: ‎09-26-2020

Question about synthesis results

Jump to solution

Hello,

I have a question regarding 2 versions of a small VHDL code.

I don't understand what's the exact rules for writing this kind of code.

The aim is to implement a state machine with a synchronous part and a combinatioral part. In order to simplify the issue, here is an example of my concern:

Version 1:

entity Essai_sm is
port (
    clk : in std_logic;
    RESET : in std_logic;
    A : out unsigned(2 downto 0);
    B : in std_logic
);
end Essai_sm;

architecture Behavioral of Essai_sm is

signal A_new, i_A : unsigned(2 downto 0);

begin

    combinatioral_process : process(i_A, RESET)
    begin
        if RESET = '1' then
            A_new <= (others => '0');
        else
            if B = '1' then
                A_new <= i_A + 1;
            else
                A_new <= i_A;
            end if;
        end if;
    end process;
    
    sync_process : process(clk)
    begin
        if rising_edge(clk) then
            i_A <= A_new;
        end if;
    end process;
    
    A <= i_A;
    
end Behavioral;

When looking at the implemented design, it's like this:

YannZ80_0-1627461435542.png

And, when looking at the report timing summary, there is no warning inside "no_clock" part:

YannZ80_1-1627461519894.png

Version 2:

The else/endif related to B='1' has been removed:

----------------------------------------------------------------------------------
-- Company: 
-- Engineer: 
-- 
-- Create Date: 25.07.2021 21:54:37
-- Design Name: 
-- Module Name: Essai_sm - Behavioral
-- Project Name: 
-- Target Devices: 
-- Tool Versions: 
-- Description: 
-- 
-- Dependencies: 
-- 
-- Revision:
-- Revision 0.01 - File Created
-- Additional Comments:
-- 
----------------------------------------------------------------------------------

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use ieee.std_logic_unsigned.all;
use ieee.std_logic_arith.all;

-- Uncomment the following library declaration if using
-- arithmetic functions with Signed or Unsigned values
--use IEEE.NUMERIC_STD.ALL;

-- Uncomment the following library declaration if instantiating
-- any Xilinx leaf cells in this code.
--library UNISIM;
--use UNISIM.VComponents.all;

entity Essai_sm is
port (
    clk : in std_logic;
    RESET : in std_logic;
    A : out unsigned(2 downto 0);
    B : in std_logic
);
end Essai_sm;

architecture Behavioral of Essai_sm is

signal A_new, i_A : unsigned(2 downto 0);

begin

    combinatioral_process : process(i_A, RESET)
    begin
        if RESET = '1' then
            A_new <= (others => '0');
        else
            if B = '1' then
                A_new <= i_A + 1;
            end if;
        end if;
    end process;
    
    sync_process : process(clk)
    begin
        if rising_edge(clk) then
            i_A <= A_new;
        end if;
    end process;
    
    A <= i_A;
    
end Behavioral;

 

Now, the implemented design, is like this:

YannZ80_2-1627461716197.png

A new latch is inserted in between the adder and the flip-flop.

And, when looking at the timing report, there are now 3 critical warnings:

YannZ80_3-1627461873697.png

I don't understand why and what's the best way to do this kind of VHDL.

Do we always need to fill any "if" with "else" in this case ?

Is there any documentation / rules about this ?

Thanks for your help,

Yann

 

 

0 Kudos
1 Solution

Accepted Solutions
seamusbleu
Voyager
Voyager
364 Views
Registered: ‎08-12-2008

You don't need to separate the comb and sync logic, just write it in a sync process.  In this way, it's the most concise, you won't get any inadvertent latches, and you don't have to worry about missing terms in the sensitivity list.

   regs_p : process (Clk)
   begin --  regs_p
      if (RISING_EDGE(Clk)) then
         if (RESET = '1') then
            i_A <= (others => '0');
         elsif B = '1' then
            i_A <= i_A + 1;
-- These two lines are redundant to default behaviour
--       else                 
--          i_A <= i_A;
         end if;
      end if;
   end process regs_p;

 

In your first example, you should include all terms used on the right-hand side of an equation in the process to the sensitivity list - you are missing 'B'.  Synthesis ignores that, and VHDL-2008 can use the term "all" in place of all terms, but it's best to just have all terms there so your synthesis and simulation results agree.

In your second example, you are describing a latch since A_new isn't assigned a value in all logic paths.  The results of that latch are then registered.

<== If this was helpful, please feel free to give Kudos, and accept as Solution if it answers your question ==>

View solution in original post

3 Replies
seamusbleu
Voyager
Voyager
365 Views
Registered: ‎08-12-2008

You don't need to separate the comb and sync logic, just write it in a sync process.  In this way, it's the most concise, you won't get any inadvertent latches, and you don't have to worry about missing terms in the sensitivity list.

   regs_p : process (Clk)
   begin --  regs_p
      if (RISING_EDGE(Clk)) then
         if (RESET = '1') then
            i_A <= (others => '0');
         elsif B = '1' then
            i_A <= i_A + 1;
-- These two lines are redundant to default behaviour
--       else                 
--          i_A <= i_A;
         end if;
      end if;
   end process regs_p;

 

In your first example, you should include all terms used on the right-hand side of an equation in the process to the sensitivity list - you are missing 'B'.  Synthesis ignores that, and VHDL-2008 can use the term "all" in place of all terms, but it's best to just have all terms there so your synthesis and simulation results agree.

In your second example, you are describing a latch since A_new isn't assigned a value in all logic paths.  The results of that latch are then registered.

<== If this was helpful, please feel free to give Kudos, and accept as Solution if it answers your question ==>

View solution in original post

avrumw
Expert
Expert
342 Views
Registered: ‎01-23-2009

The "no_clock" warning is not an RTL issue, it is a constraint issue.

The tools flag a "no_clock" check when there is a clocked object (i.e. a flip-flop, RAM, DSP, etc... ) whose clock pin does not have a static timing clock that is propagate on it. In other words, the tools are complaining that there is no "create_clock" or "create_generated_clock" upstream of the clock pin on this flip-flop.

In this case, it is a problem with the unintentional latch (which @seamusbleu already explained). This unintended latch has a G input (the gate) which is considered a "clock pin" by the tools. However, since this G pin is connected to a net that carries logic (does not carry a clock) you get the "no_clock" warning for it. If you remove the unintentional latch, this message will go away.

If you do want to use the two process state machine style (which many people avoid), then you need to have the default assignments that have the "next_state" be the "current_state". In your code you would need to put the following line either at the top of your combinatorial process or in the missing else branch of the if.

  a_new <= i_A;

Also, your sensitivity list is not complete for the combinatorial process - the signal 'B' is used, but isn't in the sensitivity list.

(And, by the way, these are "Elaborated netlists", not "Implemented netlists")

Avrum

bassman59
Historian
Historian
212 Views
Registered: ‎02-25-2008

I agree with seamusbleu that you should just write one synchronous process. (The two-process style is a vestige of the ancient Synopsys FPGA Compiler II from the last millennium — and it was junk!)

The immediate problem with your code is that your combinatorial process has the statement if B = ‘1’ then but B is not on the process’s sensitivity list.

----------------------------Yes, I do this for a living.
0 Kudos