combinational loops

Discussion in 'VHDL' started by alb, Sep 20, 2013.

  1. alb

    alb Guest

    Dear all,

    during P&R Actel Designer barfs this information (not a warning):

    > While analyzing gated clock network, ambiguities have been found on gates
    > temperature_1/tfsm_1/main.nwr_v_RNI3GSJ:Y, temperature_1/tfsm_1/main.nwr_v_RNIOVCF:Y,
    > master_clock_routing_RNO:Y, temperature_1/tfsm_1/main.nrd_v_RNI12QJ:Y.
    > The timing models of these gates have been simplified for Static Timing Analysis.


    All gates except 'master_clock_routing' are defined in the following
    code and they are the ones I'm interested into:

    <code>
    begin -- procedure autoread
    case state_v is
    when IDLE =>
    cnt_v := 0;
    state_v := SET_CLKRATE;

    when READ_BGN =>
    state_v := READ_END; -- this is one clock cycle
    nRD_v := '0';
    when READ_END =>
    state_v := nstate_v;
    nRD_v := '1';
    DATA_v := DATA;
    when WRITE_BGN =>
    state_v := WRITE_END; -- this is one clock cycle
    nWR_v := '0';
    when WRITE_END =>
    state_v := nstate_v;
    nWR_v := '1';
    DATA_v := (others => 'Z'); -- release the bus
    </code>

    and the procedure is called at each clock cycle in a template like this:

    <code>
    -------------------------------------------------------------------------------
    procedure update_regs is
    begin -- purpose: call the procedures above in the desired order
    autoread;
    end procedure update_regs;
    -------------------------------------------------------------------------------
    begin
    if RST = '1' then
    init_regs;
    elsif rising_edge(CLK) then
    update_regs;
    end if;
    update_ports;
    end process main;
    </code>

    It sim'ed ok, both pre/post-synthesis and post-layout. STA does not
    report timing violations. Why there're combinational loops???

    Al

    --
    A: Because it fouls the order in which people normally read text.
    Q: Why is top-posting such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
     
    alb, Sep 20, 2013
    #1
    1. Advertising

  2. alb

    alb Guest

    Correction to my previous post in this thread:

    On 20/09/2013 12:33, alb wrote:
    []
    > It sim'ed ok, both pre/post-synthesis and post-layout. STA does not
    > report timing violations.


    STA *does* report timing violations.
     
    alb, Sep 20, 2013
    #2
    1. Advertising

  3. alb

    rickman Guest

    On 9/20/2013 6:33 AM, alb wrote:
    > Dear all,
    >
    > during P&R Actel Designer barfs this information (not a warning):
    >
    >> While analyzing gated clock network, ambiguities have been found on gates
    >> temperature_1/tfsm_1/main.nwr_v_RNI3GSJ:Y, temperature_1/tfsm_1/main.nwr_v_RNIOVCF:Y,
    >> master_clock_routing_RNO:Y, temperature_1/tfsm_1/main.nrd_v_RNI12QJ:Y.
    >> The timing models of these gates have been simplified for Static Timing Analysis.

    >
    > All gates except 'master_clock_routing' are defined in the following
    > code and they are the ones I'm interested into:
    >
    > <code>
    > begin -- procedure autoread
    > case state_v is
    > when IDLE =>
    > cnt_v := 0;
    > state_v := SET_CLKRATE;
    >
    > when READ_BGN =>
    > state_v := READ_END; -- this is one clock cycle
    > nRD_v := '0';
    > when READ_END =>
    > state_v := nstate_v;
    > nRD_v := '1';
    > DATA_v := DATA;
    > when WRITE_BGN =>
    > state_v := WRITE_END; -- this is one clock cycle
    > nWR_v := '0';
    > when WRITE_END =>
    > state_v := nstate_v;
    > nWR_v := '1';
    > DATA_v := (others => 'Z'); -- release the bus
    > </code>
    >
    > and the procedure is called at each clock cycle in a template like this:
    >
    > <code>
    > -------------------------------------------------------------------------------
    > procedure update_regs is
    > begin -- purpose: call the procedures above in the desired order
    > autoread;
    > end procedure update_regs;
    > -------------------------------------------------------------------------------
    > begin
    > if RST = '1' then
    > init_regs;
    > elsif rising_edge(CLK) then
    > update_regs;
    > end if;
    > update_ports;
    > end process main;
    > </code>
    >
    > It sim'ed ok, both pre/post-synthesis and post-layout. STA does not
    > report timing violations. Why there're combinational loops???


    I'm not very happy with the way you have separated the register code
    from the output code. For example the procedure autoread is invoked
    from the registered section of the clocked process. This means any
    signals assigned a value in this procedure will be registered.

    The last line shown in this procedure assigns a high impedance value to
    what I assume is a signal, that's not realizable, or at least it isn't
    the way I would do it.

    Unless you show us all your code it will be hard to pin point your
    combinatorial loop. The problem is most likely in init_regs or
    update_ports as these are not registered.

    --

    Rick
     
    rickman, Sep 20, 2013
    #3
  4. alb

    alb Guest

    On 20/09/2013 22:56, rickman wrote:
    [see end of this post for the entire code]
    >
    > I'm not very happy with the way you have separated the register code
    > from the output code. For example the procedure autoread is invoked
    > from the registered section of the clocked process. This means any
    > signals assigned a value in this procedure will be registered.


    I do not have signals, I have only variables and they are synthesized to
    registers or wires according to whether they need to retain a value or not.

    > The last line shown in this procedure assigns a high impedance value to
    > what I assume is a signal, that's not realizable, or at least it isn't
    > the way I would do it.


    The DATA_v variable is not the main issue here, I'm concerned about
    combinational loops on nRD_v and nWR_v.
    OT, on the 'Z' assignment, it is clear that while internal resources of
    an FPGA do not provide tri-state logic, they can easily be transformed
    to a set of multiplexers which logically provide the same functionality.

    It is clear that no condition should exist for multiple drivers on the
    line. On a side note, shouldn't be better to use std_ulogic_vector to
    spot multiple drivers on the bus instead of std_logic_vector? I'm
    naively assuming that an unresolved type would help me spotting multiple
    drivers at compilation time...

    >
    > Unless you show us all your code it will be hard to pin point your
    > combinatorial loop. The problem is most likely in init_regs or
    > update_ports as these are not registered.


    I believe that not posting the whole code was a bad idea so it follows
    at the end of this post, but let me add that init_regs does not mean
    they are 'not registered'. The clocked process has an asynchronous reset
    which triggers the 'initialization' of everything needs to be
    initialized, while the update_ports only 'connects' the variables to the
    entity ports (no register involved here).

    While I cannot be 100% sure that the problem is in the snippet I
    previously posted, I'm quite confident that the rest of the code should
    not play an important role. In 'pkg_temp' there are some constant
    definitions only.

    <code>
    > library ieee;
    > use ieee.std_logic_1164.all;
    > use ieee.std_logic_unsigned.all;
    > use ieee.numeric_std.all;
    >
    > library work;
    > use work.pkg_temp.all;
    >
    > -------------------------------------------------------------------------------
    >
    > entity tfsm is
    > port (
    > ADD_i : in std_logic_vector(2 downto 0);
    > nRD_i : in std_logic;
    > nWR_i : in std_logic;
    > ADD_o : out std_logic_vector(2 downto 0);
    > nRD_o : out std_logic;
    > nWR_o : out std_logic;
    > DATA : inout std_logic_vector(7 downto 0);
    > TEMP : out std_logic_vector(15 downto 0);
    > SRNO : out std_logic_vector(63 downto 0);
    > ST : in std_logic;
    > CLK : in std_logic;
    > RST : in std_logic);
    >
    > end tfsm;
    >
    > -------------------------------------------------------------------------------
    >
    > architecture tfsm_arch of tfsm is
    > -- Note: No signal declarations.
    > begin -- tfsm_arch
    >
    > main : process (CLK, RST) is
    > -------------------------------------------------------------------------------
    > type state_t is (
    > SET_CLKRATE,
    > SET_CTRLREG,
    > CLR_INTREG,
    > SET_INEREG,
    > SEND_READ_ROM,
    > SEND_SKIP_ROM,
    > SEND_WRITE_SCRATCHPAD,
    > SEND_CONVERT,
    > SEND_READ_SCRATCH,
    >
    > -- reusable states
    > SEND_RESET,
    > TEST_PRESENCE,
    >
    > SEND_CMD,
    > TEST_CMD,
    >
    > SEND_DATA,
    > TEST_DATA,
    >
    > WRITE_BGN,
    > WRITE_END,
    >
    > READ_BGN,
    > READ_END,
    > IDLE
    > );
    >
    > variable state_v : state_t; -- current state
    > variable nstate_v : state_t; -- next state after write/read
    > variable mstate_v : state_t; -- next state after macro command
    > variable lstate_v : state_t; -- next state after skip rom
    >
    > variable cmd_v : integer; -- command to send
    > variable dat_v : integer; -- data to send
    > variable cnt_v : integer; -- counter
    >
    > -------------------------------------------------------------------------------
    > -- Data Registers
    > subtype bus_t is std_logic_vector(DATA'range); -- match port width
    > variable DATA_v : bus_t;
    >
    > -- Address registers
    > subtype add_t is std_logic_vector(ADD_i'range); -- match port width
    > variable ADD_v : add_t;
    >
    > -- control wires
    > subtype ctr_t is std_logic;
    > variable nRD_v : ctr_t;
    > variable nWR_v : ctr_t;
    >
    > -- sensor id type
    > type sens_t is array (7 downto 0) of std_logic_vector (7 downto 0);
    >
    > -- sensor temperature type
    > type temp_t is array (1 downto 0) of std_logic_vector (7 downto 0);
    >
    > -- scratchpad type
    > type scratch_t is array (2 downto 0) of integer;
    >
    > -- temperature registers
    > -- temp_count_v: will count each time there's a new temperature available
    > -- temp_valid_v: will be set to one when ID AND first temperature readout
    > -- are both valid
    >
    > variable temp_count_v : std_logic_vector(2 downto 0);
    > variable temp_valid_v : std_logic;
    > variable temp_value_v : temp_t;
    >
    > -- sensor id register
    > variable sens_id_v : sens_t;
    >
    > -- scratchpad register
    > variable scratchpad_v : scratch_t;
    >
    > -------------------------------------------------------------------------------
    > procedure bus_init is -- used in init_regs
    > begin
    > DATA_v := (others => 'Z');
    > ADD_v := (others => '0');
    > nRD_v := '1'; -- inactive
    > nWR_v := '1'; -- inactive
    >
    > temp_count_v := "000";
    > temp_valid_v := '0';
    > temp_value_v(0) := x"00";
    > temp_value_v(1) := x"00";
    >
    > sens_id_v(0) := x"28"; -- for test only
    > sens_id_v(1) := x"DE"; -- for test only
    > sens_id_v(2) := x"D5"; -- for test only
    > sens_id_v(3) := x"45"; -- for test only
    > sens_id_v(4) := x"03"; -- for test only
    > sens_id_v(5) := x"00"; -- for test only
    > sens_id_v(6) := x"00"; -- for test only
    > sens_id_v(7) := x"C0"; -- crc
    >
    > end procedure bus_init;
    >
    > -------------------------------------------------------------------------------
    > procedure fsm_init is -- used in init_regs
    > begin
    > state_v := IDLE;
    > nstate_v := IDLE;
    > mstate_v := IDLE;
    > lstate_v := IDLE;
    >
    > cmd_v := 0;
    > cnt_v := 0;
    > dat_v := 0;
    >
    > scratchpad_v (0) := 40; -- x"28" -> +40 degrees
    > scratchpad_v (1) := 226; -- x"E2" -> -30 degrees
    > scratchpad_v (2) := 127; -- x"7F" -> I have no idea!
    > end procedure fsm_init;
    >
    > -------------------------------------------------------------------------------
    > procedure init_regs is
    > begin
    > bus_init;
    > fsm_init;
    > end procedure init_regs;
    >
    > -------------------------------------------------------------------------------
    > procedure update_ports is
    > begin -- purpose: synthesize a wire from the register to the port
    >
    > -- ST is a register bit that allows start/stop of the automatic
    > -- temperature readout procedure. It is set in the control register of
    > -- the DS1WM and propagated up to here (see one_wire_interface.vhd)
    >
    > mux : if ST then
    > DATA <= DATA_v;
    > nRD_o <= nRD_v;
    > nWR_o <= nWR_v;
    > ADD_o <= ADD_v;
    > else
    > DATA <= (others => 'Z');
    > nRD_o <= nRD_i;
    > nWR_o <= nWR_i;
    > ADD_o <= ADD_i;
    > end if;
    >
    > TEMP <= temp_count_v &
    > temp_valid_v &
    > temp_value_v(1)(3 downto 0) &
    > temp_value_v(0);
    >
    > sensor_id : for i in 0 to 7 loop
    > SRNO(8*(i+1)-1 downto 8*i) <= sens_id_v(i);
    > end loop sensor_id;
    >
    > end procedure update_ports;
    > -------------------------------------------------------------------------------
    > procedure autoread is
    > -- local procedures for statements used more than once:
    > procedure write_ds1wm (
    > add : in add_t;
    > val : in integer) is
    > begin
    > state_v := WRITE_BGN;
    > ADD_v := add;
    > DATA_v := std_logic_vector(to_unsigned(val, DATA_v'length));
    > end write_ds1wm;
    > --
    > procedure read_ds1wm (
    > add : in add_t) is
    > begin
    > state_v := READ_BGN;
    > ADD_v := add;
    > DATA_v := (others => 'Z');
    > end read_ds1wm;
    >
    > procedure inc_tic_count is
    > begin
    > cnt_v := cnt_v + 1;
    > end procedure inc_tic_count;
    >
    > procedure inc_tem_count is
    > begin
    > temp_count_v := temp_count_v + '1';
    > end procedure inc_tem_count;
    >
    > begin -- procedure autoread
    > if ST then
    > case state_v is
    > when IDLE =>
    > cnt_v := 0;
    > state_v := SET_CLKRATE;
    >
    > -------------------------------------------------------------------------------
    > -- Write and Read states are special ones since they are called several times
    > -- by every other state, it is therefore necessary to store the value of the
    > -- next state once the write/read steps have been completed, in order to
    > -- continue with the fsm.
    > -------------------------------------------------------------------------------
    > when READ_BGN =>
    > state_v := READ_END; -- this is one clock cycle
    > nRD_v := not nRD_v;
    > when READ_END =>
    > state_v := nstate_v;
    > nRD_v := not nRD_v;
    > DATA_v := DATA;
    > when WRITE_BGN =>
    > state_v := WRITE_END; -- this is one clock cycle
    > nWR_v := not nWR_v;
    > when WRITE_END =>
    > state_v := nstate_v;
    > nWR_v := not nWR_v;
    > DATA_v := (others => 'Z'); -- release the bus
    >
    > -------------------------------------------------------------------------------
    > -- Init part for the DS1WM. Set clock rate, control register, clean
    > -- interrupt register and configure interrupt enable register
    > -------------------------------------------------------------------------------
    > when SET_CLKRATE =>
    > nstate_v := SET_CTRLREG;
    > write_ds1wm(CLKREG, 138); -- clock enabled + 20 MHz
    > when SET_CTRLREG =>
    > nstate_v := CLR_INTREG;
    > write_ds1wm(CTLREG, 2**STB); -- keep STB active!
    > when CLR_INTREG =>
    > nstate_v := SET_INEREG;
    > read_ds1wm(INFREG);
    > when SET_INEREG =>
    > nstate_v := SEND_RESET;
    > mstate_v := SEND_READ_ROM;
    > write_ds1wm(INEREG, 2**EPD + 2**ETBE + 2**ERBF);
    >
    > -------------------------------------------------------------------------------
    > -- Start DS1WM transactions with a 1wire reset. Each 1-wire transaction has
    > -- always three phases:
    > -- 1. initialization (RESET)
    > -- 2. ROM command
    > -- 3. FUNCTION command
    > -------------------------------------------------------------------------------
    >
    > when SEND_RESET =>
    > nstate_v := TEST_PRESENCE;
    > write_ds1wm(CMDREG, 2**OneWR);
    >
    > when TEST_PRESENCE =>
    > -- we should foresee a timeout...!!!
    > if DATA_v(PD) and not DATA_v(PDR) then
    > state_v := mstate_v;
    > else
    > nstate_v := state_v;
    > read_ds1wm(INFREG);
    > end if;
    >
    > -------------------------------------------------------------------------------
    > -- SEND command cmd_v. This sequence of states is referenced as a 'macro' as it
    > -- will depend on the mstate_v defined by the calling state.
    > -------------------------------------------------------------------------------
    > when SEND_CMD =>
    > nstate_v := TEST_CMD;
    > write_ds1wm(DATBUF, cmd_v);
    >
    > when TEST_CMD =>
    > -- we should foresee a timeout...!!!
    > if DATA_v(RBF) then
    > nstate_v := mstate_v;
    > read_ds1wm(DATBUF); -- empty data buffer!
    > else
    > nstate_v := state_v;
    > read_ds1wm(INFREG);
    > end if;
    >
    > -------------------------------------------------------------------------------
    > -- Read cnt_v data times to register. This sequence of states is references as
    > -- a 'macro' as it will depend on the mstate_v defined by the calling state.
    > -------------------------------------------------------------------------------
    >
    > when SEND_DATA =>
    > nstate_v := TEST_DATA;
    > write_ds1wm(DATBUF, dat_v);
    >
    > when TEST_DATA =>
    > -- we should foresee a timeout...!!!
    > if DATA_v(RBF) then
    > nstate_v := mstate_v;
    > read_ds1wm(DATBUF);
    > else
    > nstate_v := state_v;
    > read_ds1wm(INFREG);
    > end if;
    >
    > -------------------------------------------------------------------------------
    >
    > when SEND_READ_ROM =>
    > mstate_v := SEND_READ_ROM;
    > -- read data after the first SEND_DATA is over (cnt_v = 2)
    > if cnt_v > 1 then
    > sens_id_v(cnt_v - 2) := DATA_v;
    > end if;
    >
    > if cnt_v = 0 then
    > cmd_v := READ_ROM;
    > state_v := SEND_CMD;
    > inc_tic_count;
    > elsif cnt_v <= 8 then
    > dat_v := GET_DATA;
    > state_v := SEND_DATA;
    > inc_tic_count;
    > else
    > cnt_v := 0;
    > state_v := SEND_WRITE_SCRATCHPAD;
    > end if;
    >
    > -------------------------------------------------------------------------------
    >
    > when SEND_WRITE_SCRATCHPAD =>
    > mstate_v := SEND_WRITE_SCRATCHPAD;
    > if cnt_v = 0 then
    > cmd_v := WRITE_SCRATCH;
    > state_v := SEND_CMD;
    > inc_tic_count;
    > elsif cnt_v <= 3 then
    > dat_v := scratchpad_v(cnt_v - 1);
    > state_v := SEND_DATA;
    > inc_tic_count;
    > else
    > cnt_v := 0;
    > mstate_v := SEND_SKIP_ROM;
    > lstate_v := SEND_CONVERT;
    > state_v := SEND_RESET;
    > end if;
    >
    > -------------------------------------------------------------------------------
    >
    > when SEND_SKIP_ROM =>
    > cmd_v := SKIP_ROM;
    > state_v := SEND_CMD;
    > mstate_v := lstate_v;
    >
    > -------------------------------------------------------------------------------
    >
    > when SEND_CONVERT =>
    > mstate_v := SEND_CONVERT;
    >
    > if cnt_v = 0 then
    > cmd_v := CONVERT;
    > state_v := SEND_CMD;
    > inc_tic_count;
    > elsif cnt_v = WAIT_CONVERSION_TIME then
    > dat_v := GET_DATA;
    > state_v := SEND_DATA;
    > inc_tic_count;
    > elsif cnt_v = WAIT_CONVERSION_TIME+1 then
    > cnt_v := 1;
    > if DATA_v = x"FF" then
    > cnt_v := 0;
    > mstate_v := SEND_SKIP_ROM;
    > lstate_v := SEND_READ_SCRATCH;
    > state_v := SEND_RESET;
    > end if;
    > else
    > inc_tic_count;
    > end if;
    >
    > -------------------------------------------------------------------------------
    >
    > when SEND_READ_SCRATCH =>
    > mstate_v := SEND_READ_SCRATCH;
    > -- read data after the first SEND_DATA is over (cnt_v = 2)
    > if cnt_v > 1 then
    > temp_value_v(cnt_v - 2) := DATA_v;
    > end if;
    >
    > if cnt_v = 0 then
    > cmd_v := READ_SCRATCH;
    > state_v := SEND_CMD;
    > inc_tic_count;
    > elsif cnt_v <= 2 then
    > dat_v := GET_DATA;
    > state_v := SEND_DATA;
    > inc_tic_count;
    > else
    > inc_tem_count;
    > temp_valid_v := '1';
    > cnt_v := 0;
    > mstate_v := SEND_SKIP_ROM;
    > lstate_v := SEND_CONVERT;
    > state_v := SEND_RESET;
    > end if;
    >
    > when others => null;
    > end case;
    > else
    > state_v := IDLE;
    > end if;
    > end procedure autoread;
    >
    > -------------------------------------------------------------------------------
    > procedure update_regs is
    > begin -- purpose: call the procedures above in the desired order
    > autoread;
    > end procedure update_regs;
    >
    > -------------------------------------------------------------------------------
    > -- Synchronous Templates for Synthesis: The following templates use
    > -- all of the declarations above to make a UART. With other
    > -- declarations, the template could make anything else. Relative
    > -- synthesis performance will be compared below for each template.
    > -- If I were not making comparisons, the first template would be
    > -- pasted into the main process.
    > -------------------------------------------------------------------------------
    > procedure template_v_rst is
    > begin -- a_rst is logically equivalent
    > if RST = '1' then -- Assumes synched trailing edge RST pulse
    > init_regs; -- reg_v := init_c; Variables only, ports below.
    > elsif rising_edge(CLK) then
    > update_regs; -- reg_v := f(reg_v);Variables only, ports below.
    > end if; -- Synchronous init optional (state_v = idle_c)
    > update_ports; -- will infer port wires ok for RST and CLK
    > end procedure template_v_rst; -- out_port <= reg_v; ports only, no signals
    >
    > -------------------------------------------------------------------------------
    > begin -- process main
    > template_v_rst;
    > end process main;
    >
    > -------------------------------------------------------------------------------
    >
    > end tfsm_arch;




    </code>
     
    alb, Sep 23, 2013
    #4
  5. alb

    rickman Guest

    On 9/23/2013 4:03 AM, alb wrote:
    > nRD_v := not nRD_v;


    Is this your culpret?

    I didn't read all your code because of the volume, but if nRD_v is not
    registered, this statement creates a latch.

    You seem to understand this stuff pretty well, so it puzzles me that you
    missed this. Or is there something else I didn't catch about your code?

    --

    Rick
     
    rickman, Sep 23, 2013
    #5
  6. alb

    alb Guest

    On 23/09/2013 15:20, rickman wrote:
    > On 9/23/2013 4:03 AM, alb wrote:
    >> nRD_v := not nRD_v;

    >
    > Is this your culpret?


    Not really, considering the way the procedure is called (in update_regs)
    it will be like doing:

    <code>
    process (clk, rst)
    begin
    if rst = '1' then
    nRD_s <= '1'; -- nRD_s is a signal instead of a variable
    elsif rising_edge(clk) then
    if <condition> then
    nRD_s <= not nRD_s;
    end if;
    end if;
    end process;
    </code>

    Indeed even if I change the previous variable assignment to the following:

    nRD_v := '0'; -- or '1' according to conditions

    the P&R still complains for combinational loops.
    Variables can be used to infer flip-flop just as well as signals. I like
    the following code example (from Martin Thompson):

    <code>
    > process (clk)
    > variable something : std_logic;
    > if rising_edge(clk) then
    > if reset = '1' then
    > something := '0';
    > else
    > output_b <= something or input c; -- using the previous clock's value of 'something' infers a register
    > something := input_a and input_b; -- comb. logic for a new value
    > output_a <= something or input_c; -- which is used immediately, not registered here
    > end if;
    > end if;
    > end process;

    </code>

    > I didn't read all your code because of the volume, but if nRD_v is not
    > registered, this statement creates a latch.


    I can imagine, that was the reason why I didn't want to post it all in
    the first place.

    A latch is inferred if you write a process where an output is not
    assigned under all possible input conditions:

    <code>
    process (clk, rst)
    begin
    if rst = '1' then
    nRD_s <= '1'; -- nRD_s is a signal instead of a variable
    foo_s <= nRD_s; -- foo_s is not assigned under all possible inputs
    -- of the process therefore a latch is needed to
    -- retain its value.
    elsif rising_edge(clk) then
    if <condition> then
    nRD_s <= not nRD_s;
    end if;
    end if;
    end process;
    </code>

    So my code should not infer a latch (and indeed it does not).

    >
    > You seem to understand this stuff pretty well, so it puzzles me that you
    > missed this. Or is there something else I didn't catch about your code?


    Believe me, if I knew this stuff 'pretty well' I won't be asking ;-).
    I'm actually exploring the usage of variables to a greater extent,
    together with a new 'procedural' approach and I'm afraid I'm misusing it
    somehow.

    Funny enough the code simulates just as I want it, maybe I've only been
    lucky...
     
    alb, Sep 23, 2013
    #6
  7. alb

    rickman Guest

    On 9/23/2013 10:34 AM, alb wrote:
    > On 23/09/2013 15:20, rickman wrote:
    >> On 9/23/2013 4:03 AM, alb wrote:
    >>> nRD_v := not nRD_v;

    >>
    >> Is this your culpret?

    >
    > Not really, considering the way the procedure is called (in update_regs)
    > it will be like doing:
    >
    > <code>
    > process (clk, rst)
    > begin
    > if rst = '1' then
    > nRD_s<= '1'; -- nRD_s is a signal instead of a variable
    > elsif rising_edge(clk) then
    > if<condition> then
    > nRD_s<= not nRD_s;
    > end if;
    > end if;
    > end process;
    > </code>
    >
    > Indeed even if I change the previous variable assignment to the following:
    >
    > nRD_v := '0'; -- or '1' according to conditions
    >
    > the P&R still complains for combinational loops.
    > Variables can be used to infer flip-flop just as well as signals. I like
    > the following code example (from Martin Thompson):
    >
    > <code>
    >> process (clk)
    >> variable something : std_logic;
    >> if rising_edge(clk) then
    >> if reset = '1' then
    >> something := '0';
    >> else
    >> output_b<= something or input c; -- using the previous clock's value of 'something' infers a register
    >> something := input_a and input_b; -- comb. logic for a new value
    >> output_a<= something or input_c; -- which is used immediately, not registered here
    >> end if;
    >> end if;
    >> end process;

    > </code>
    >
    >> I didn't read all your code because of the volume, but if nRD_v is not
    >> registered, this statement creates a latch.

    >
    > I can imagine, that was the reason why I didn't want to post it all in
    > the first place.


    But if you omit the full code, you can't expect anyone to even begin
    finding the problem, too many possibilities.


    > A latch is inferred if you write a process where an output is not
    > assigned under all possible input conditions:
    >
    > <code>
    > process (clk, rst)
    > begin
    > if rst = '1' then
    > nRD_s<= '1'; -- nRD_s is a signal instead of a variable
    > foo_s<= nRD_s; -- foo_s is not assigned under all possible inputs
    > -- of the process therefore a latch is needed to
    > -- retain its value.
    > elsif rising_edge(clk) then
    > if<condition> then
    > nRD_s<= not nRD_s;
    > end if;
    > end if;
    > end process;
    > </code>
    >
    > So my code should not infer a latch (and indeed it does not).


    Your premise is not complete. A latch is inferred as well if you create
    a feedback loop. nRD_v := not nRD_v; creates feedback if it is not
    registered. It appears it should be registered however. So the
    questions are,

    1) Is it registered?

    2) If it is registered, how is the message being generated?

    3) If it is not registered, why?

    BTW, you *don't* assign a value to nRD_v under all conditions.

    when IDLE =>
    cnt_v := 0;
    state_v := SET_CLKRATE;
    when READ_BGN =>
    state_v := READ_END; -- this is one clock cycle
    nRD_v := not nRD_v;

    So if nRD_v is not registered, it will be a latch anyway.

    You need to focus on why nRD_v is not registered I would say.


    >> You seem to understand this stuff pretty well, so it puzzles me that you
    >> missed this. Or is there something else I didn't catch about your code?

    >
    > Believe me, if I knew this stuff 'pretty well' I won't be asking ;-).
    > I'm actually exploring the usage of variables to a greater extent,
    > together with a new 'procedural' approach and I'm afraid I'm misusing it
    > somehow.


    Yes, I have never used variables so much myself. That's why I missed
    that nRD_v was a variable. I missed both the := and the _v ... duh!

    I also don't use procedures so much. I should try to work with them
    more so I understand them better. There are times when they are useful
    for decomposition of code into reusable modules.


    > Funny enough the code simulates just as I want it, maybe I've only been
    > lucky...


    *Something* is amiss. Has this error message been generated all along?
    If not, see what you changed. If it has I would say simplify the code
    to something that doesn't give the error and figure out what is different.

    --

    Rick
     
    rickman, Sep 23, 2013
    #7
  8. alb

    rickman Guest

    Ok, timeout!

    This is the first time you have compiled the code isn't it? So you have
    not done any testing on the code at all so far?

    Where are your parameter lists for the procedures such as update_ports?

    Rick


    On 9/23/2013 4:03 AM, alb wrote:
    > On 20/09/2013 22:56, rickman wrote:
    > [see end of this post for the entire code]
    >>
    >> I'm not very happy with the way you have separated the register code
    >> from the output code. For example the procedure autoread is invoked
    >> from the registered section of the clocked process. This means any
    >> signals assigned a value in this procedure will be registered.

    >
    > I do not have signals, I have only variables and they are synthesized to
    > registers or wires according to whether they need to retain a value or not.
    >
    >> The last line shown in this procedure assigns a high impedance value to
    >> what I assume is a signal, that's not realizable, or at least it isn't
    >> the way I would do it.

    >
    > The DATA_v variable is not the main issue here, I'm concerned about
    > combinational loops on nRD_v and nWR_v.
    > OT, on the 'Z' assignment, it is clear that while internal resources of
    > an FPGA do not provide tri-state logic, they can easily be transformed
    > to a set of multiplexers which logically provide the same functionality.
    >
    > It is clear that no condition should exist for multiple drivers on the
    > line. On a side note, shouldn't be better to use std_ulogic_vector to
    > spot multiple drivers on the bus instead of std_logic_vector? I'm
    > naively assuming that an unresolved type would help me spotting multiple
    > drivers at compilation time...
    >
    >>
    >> Unless you show us all your code it will be hard to pin point your
    >> combinatorial loop. The problem is most likely in init_regs or
    >> update_ports as these are not registered.

    >
    > I believe that not posting the whole code was a bad idea so it follows
    > at the end of this post, but let me add that init_regs does not mean
    > they are 'not registered'. The clocked process has an asynchronous reset
    > which triggers the 'initialization' of everything needs to be
    > initialized, while the update_ports only 'connects' the variables to the
    > entity ports (no register involved here).
    >
    > While I cannot be 100% sure that the problem is in the snippet I
    > previously posted, I'm quite confident that the rest of the code should
    > not play an important role. In 'pkg_temp' there are some constant
    > definitions only.
    >
    > <code>
    >> library ieee;
    >> use ieee.std_logic_1164.all;
    >> use ieee.std_logic_unsigned.all;
    >> use ieee.numeric_std.all;
    >>
    >> library work;
    >> use work.pkg_temp.all;
    >>
    >> -------------------------------------------------------------------------------
    >>
    >> entity tfsm is
    >> port (
    >> ADD_i : in std_logic_vector(2 downto 0);
    >> nRD_i : in std_logic;
    >> nWR_i : in std_logic;
    >> ADD_o : out std_logic_vector(2 downto 0);
    >> nRD_o : out std_logic;
    >> nWR_o : out std_logic;
    >> DATA : inout std_logic_vector(7 downto 0);
    >> TEMP : out std_logic_vector(15 downto 0);
    >> SRNO : out std_logic_vector(63 downto 0);
    >> ST : in std_logic;
    >> CLK : in std_logic;
    >> RST : in std_logic);
    >>
    >> end tfsm;
    >>
    >> -------------------------------------------------------------------------------
    >>
    >> architecture tfsm_arch of tfsm is
    >> -- Note: No signal declarations.
    >> begin -- tfsm_arch
    >>
    >> main : process (CLK, RST) is
    >> -------------------------------------------------------------------------------
    >> type state_t is (
    >> SET_CLKRATE,
    >> SET_CTRLREG,
    >> CLR_INTREG,
    >> SET_INEREG,
    >> SEND_READ_ROM,
    >> SEND_SKIP_ROM,
    >> SEND_WRITE_SCRATCHPAD,
    >> SEND_CONVERT,
    >> SEND_READ_SCRATCH,
    >>
    >> -- reusable states
    >> SEND_RESET,
    >> TEST_PRESENCE,
    >>
    >> SEND_CMD,
    >> TEST_CMD,
    >>
    >> SEND_DATA,
    >> TEST_DATA,
    >>
    >> WRITE_BGN,
    >> WRITE_END,
    >>
    >> READ_BGN,
    >> READ_END,
    >> IDLE
    >> );
    >>
    >> variable state_v : state_t; -- current state
    >> variable nstate_v : state_t; -- next state after write/read
    >> variable mstate_v : state_t; -- next state after macro command
    >> variable lstate_v : state_t; -- next state after skip rom
    >>
    >> variable cmd_v : integer; -- command to send
    >> variable dat_v : integer; -- data to send
    >> variable cnt_v : integer; -- counter
    >>
    >> -------------------------------------------------------------------------------
    >> -- Data Registers
    >> subtype bus_t is std_logic_vector(DATA'range); -- match port width
    >> variable DATA_v : bus_t;
    >>
    >> -- Address registers
    >> subtype add_t is std_logic_vector(ADD_i'range); -- match port width
    >> variable ADD_v : add_t;
    >>
    >> -- control wires
    >> subtype ctr_t is std_logic;
    >> variable nRD_v : ctr_t;
    >> variable nWR_v : ctr_t;
    >>
    >> -- sensor id type
    >> type sens_t is array (7 downto 0) of std_logic_vector (7 downto 0);
    >>
    >> -- sensor temperature type
    >> type temp_t is array (1 downto 0) of std_logic_vector (7 downto 0);
    >>
    >> -- scratchpad type
    >> type scratch_t is array (2 downto 0) of integer;
    >>
    >> -- temperature registers
    >> -- temp_count_v: will count each time there's a new temperature available
    >> -- temp_valid_v: will be set to one when ID AND first temperature readout
    >> -- are both valid
    >>
    >> variable temp_count_v : std_logic_vector(2 downto 0);
    >> variable temp_valid_v : std_logic;
    >> variable temp_value_v : temp_t;
    >>
    >> -- sensor id register
    >> variable sens_id_v : sens_t;
    >>
    >> -- scratchpad register
    >> variable scratchpad_v : scratch_t;
    >>
    >> -------------------------------------------------------------------------------
    >> procedure bus_init is -- used in init_regs
    >> begin
    >> DATA_v := (others => 'Z');
    >> ADD_v := (others => '0');
    >> nRD_v := '1'; -- inactive
    >> nWR_v := '1'; -- inactive
    >>
    >> temp_count_v := "000";
    >> temp_valid_v := '0';
    >> temp_value_v(0) := x"00";
    >> temp_value_v(1) := x"00";
    >>
    >> sens_id_v(0) := x"28"; -- for test only
    >> sens_id_v(1) := x"DE"; -- for test only
    >> sens_id_v(2) := x"D5"; -- for test only
    >> sens_id_v(3) := x"45"; -- for test only
    >> sens_id_v(4) := x"03"; -- for test only
    >> sens_id_v(5) := x"00"; -- for test only
    >> sens_id_v(6) := x"00"; -- for test only
    >> sens_id_v(7) := x"C0"; -- crc
    >>
    >> end procedure bus_init;
    >>
    >> -------------------------------------------------------------------------------
    >> procedure fsm_init is -- used in init_regs
    >> begin
    >> state_v := IDLE;
    >> nstate_v := IDLE;
    >> mstate_v := IDLE;
    >> lstate_v := IDLE;
    >>
    >> cmd_v := 0;
    >> cnt_v := 0;
    >> dat_v := 0;
    >>
    >> scratchpad_v (0) := 40; -- x"28" -> +40 degrees
    >> scratchpad_v (1) := 226; -- x"E2" -> -30 degrees
    >> scratchpad_v (2) := 127; -- x"7F" -> I have no idea!
    >> end procedure fsm_init;
    >>
    >> -------------------------------------------------------------------------------
    >> procedure init_regs is
    >> begin
    >> bus_init;
    >> fsm_init;
    >> end procedure init_regs;
    >>
    >> -------------------------------------------------------------------------------
    >> procedure update_ports is
    >> begin -- purpose: synthesize a wire from the register to the port
    >>
    >> -- ST is a register bit that allows start/stop of the automatic
    >> -- temperature readout procedure. It is set in the control register of
    >> -- the DS1WM and propagated up to here (see one_wire_interface.vhd)
    >>
    >> mux : if ST then
    >> DATA<= DATA_v;
    >> nRD_o<= nRD_v;
    >> nWR_o<= nWR_v;
    >> ADD_o<= ADD_v;
    >> else
    >> DATA<= (others => 'Z');
    >> nRD_o<= nRD_i;
    >> nWR_o<= nWR_i;
    >> ADD_o<= ADD_i;
    >> end if;
    >>
    >> TEMP<= temp_count_v&
    >> temp_valid_v&
    >> temp_value_v(1)(3 downto 0)&
    >> temp_value_v(0);
    >>
    >> sensor_id : for i in 0 to 7 loop
    >> SRNO(8*(i+1)-1 downto 8*i)<= sens_id_v(i);
    >> end loop sensor_id;
    >>
    >> end procedure update_ports;
    >> -------------------------------------------------------------------------------
    >> procedure autoread is
    >> -- local procedures for statements used more than once:
    >> procedure write_ds1wm (
    >> add : in add_t;
    >> val : in integer) is
    >> begin
    >> state_v := WRITE_BGN;
    >> ADD_v := add;
    >> DATA_v := std_logic_vector(to_unsigned(val, DATA_v'length));
    >> end write_ds1wm;
    >> --
    >> procedure read_ds1wm (
    >> add : in add_t) is
    >> begin
    >> state_v := READ_BGN;
    >> ADD_v := add;
    >> DATA_v := (others => 'Z');
    >> end read_ds1wm;
    >>
    >> procedure inc_tic_count is
    >> begin
    >> cnt_v := cnt_v + 1;
    >> end procedure inc_tic_count;
    >>
    >> procedure inc_tem_count is
    >> begin
    >> temp_count_v := temp_count_v + '1';
    >> end procedure inc_tem_count;
    >>
    >> begin -- procedure autoread
    >> if ST then
    >> case state_v is
    >> when IDLE =>
    >> cnt_v := 0;
    >> state_v := SET_CLKRATE;
    >>
    >> -------------------------------------------------------------------------------
    >> -- Write and Read states are special ones since they are called several times
    >> -- by every other state, it is therefore necessary to store the value of the
    >> -- next state once the write/read steps have been completed, in order to
    >> -- continue with the fsm.
    >> -------------------------------------------------------------------------------
    >> when READ_BGN =>
    >> state_v := READ_END; -- this is one clock cycle
    >> nRD_v := not nRD_v;
    >> when READ_END =>
    >> state_v := nstate_v;
    >> nRD_v := not nRD_v;
    >> DATA_v := DATA;
    >> when WRITE_BGN =>
    >> state_v := WRITE_END; -- this is one clock cycle
    >> nWR_v := not nWR_v;
    >> when WRITE_END =>
    >> state_v := nstate_v;
    >> nWR_v := not nWR_v;
    >> DATA_v := (others => 'Z'); -- release the bus
    >>
    >> -------------------------------------------------------------------------------
    >> -- Init part for the DS1WM. Set clock rate, control register, clean
    >> -- interrupt register and configure interrupt enable register
    >> -------------------------------------------------------------------------------
    >> when SET_CLKRATE =>
    >> nstate_v := SET_CTRLREG;
    >> write_ds1wm(CLKREG, 138); -- clock enabled + 20 MHz
    >> when SET_CTRLREG =>
    >> nstate_v := CLR_INTREG;
    >> write_ds1wm(CTLREG, 2**STB); -- keep STB active!
    >> when CLR_INTREG =>
    >> nstate_v := SET_INEREG;
    >> read_ds1wm(INFREG);
    >> when SET_INEREG =>
    >> nstate_v := SEND_RESET;
    >> mstate_v := SEND_READ_ROM;
    >> write_ds1wm(INEREG, 2**EPD + 2**ETBE + 2**ERBF);
    >>
    >> -------------------------------------------------------------------------------
    >> -- Start DS1WM transactions with a 1wire reset. Each 1-wire transaction has
    >> -- always three phases:
    >> -- 1. initialization (RESET)
    >> -- 2. ROM command
    >> -- 3. FUNCTION command
    >> -------------------------------------------------------------------------------
    >>
    >> when SEND_RESET =>
    >> nstate_v := TEST_PRESENCE;
    >> write_ds1wm(CMDREG, 2**OneWR);
    >>
    >> when TEST_PRESENCE =>
    >> -- we should foresee a timeout...!!!
    >> if DATA_v(PD) and not DATA_v(PDR) then
    >> state_v := mstate_v;
    >> else
    >> nstate_v := state_v;
    >> read_ds1wm(INFREG);
    >> end if;
    >>
    >> -------------------------------------------------------------------------------
    >> -- SEND command cmd_v. This sequence of states is referenced as a 'macro' as it
    >> -- will depend on the mstate_v defined by the calling state.
    >> -------------------------------------------------------------------------------
    >> when SEND_CMD =>
    >> nstate_v := TEST_CMD;
    >> write_ds1wm(DATBUF, cmd_v);
    >>
    >> when TEST_CMD =>
    >> -- we should foresee a timeout...!!!
    >> if DATA_v(RBF) then
    >> nstate_v := mstate_v;
    >> read_ds1wm(DATBUF); -- empty data buffer!
    >> else
    >> nstate_v := state_v;
    >> read_ds1wm(INFREG);
    >> end if;
    >>
    >> -------------------------------------------------------------------------------
    >> -- Read cnt_v data times to register. This sequence of states is references as
    >> -- a 'macro' as it will depend on the mstate_v defined by the calling state.
    >> -------------------------------------------------------------------------------
    >>
    >> when SEND_DATA =>
    >> nstate_v := TEST_DATA;
    >> write_ds1wm(DATBUF, dat_v);
    >>
    >> when TEST_DATA =>
    >> -- we should foresee a timeout...!!!
    >> if DATA_v(RBF) then
    >> nstate_v := mstate_v;
    >> read_ds1wm(DATBUF);
    >> else
    >> nstate_v := state_v;
    >> read_ds1wm(INFREG);
    >> end if;
    >>
    >> -------------------------------------------------------------------------------
    >>
    >> when SEND_READ_ROM =>
    >> mstate_v := SEND_READ_ROM;
    >> -- read data after the first SEND_DATA is over (cnt_v = 2)
    >> if cnt_v> 1 then
    >> sens_id_v(cnt_v - 2) := DATA_v;
    >> end if;
    >>
    >> if cnt_v = 0 then
    >> cmd_v := READ_ROM;
    >> state_v := SEND_CMD;
    >> inc_tic_count;
    >> elsif cnt_v<= 8 then
    >> dat_v := GET_DATA;
    >> state_v := SEND_DATA;
    >> inc_tic_count;
    >> else
    >> cnt_v := 0;
    >> state_v := SEND_WRITE_SCRATCHPAD;
    >> end if;
    >>
    >> -------------------------------------------------------------------------------
    >>
    >> when SEND_WRITE_SCRATCHPAD =>
    >> mstate_v := SEND_WRITE_SCRATCHPAD;
    >> if cnt_v = 0 then
    >> cmd_v := WRITE_SCRATCH;
    >> state_v := SEND_CMD;
    >> inc_tic_count;
    >> elsif cnt_v<= 3 then
    >> dat_v := scratchpad_v(cnt_v - 1);
    >> state_v := SEND_DATA;
    >> inc_tic_count;
    >> else
    >> cnt_v := 0;
    >> mstate_v := SEND_SKIP_ROM;
    >> lstate_v := SEND_CONVERT;
    >> state_v := SEND_RESET;
    >> end if;
    >>
    >> -------------------------------------------------------------------------------
    >>
    >> when SEND_SKIP_ROM =>
    >> cmd_v := SKIP_ROM;
    >> state_v := SEND_CMD;
    >> mstate_v := lstate_v;
    >>
    >> -------------------------------------------------------------------------------
    >>
    >> when SEND_CONVERT =>
    >> mstate_v := SEND_CONVERT;
    >>
    >> if cnt_v = 0 then
    >> cmd_v := CONVERT;
    >> state_v := SEND_CMD;
    >> inc_tic_count;
    >> elsif cnt_v = WAIT_CONVERSION_TIME then
    >> dat_v := GET_DATA;
    >> state_v := SEND_DATA;
    >> inc_tic_count;
    >> elsif cnt_v = WAIT_CONVERSION_TIME+1 then
    >> cnt_v := 1;
    >> if DATA_v = x"FF" then
    >> cnt_v := 0;
    >> mstate_v := SEND_SKIP_ROM;
    >> lstate_v := SEND_READ_SCRATCH;
    >> state_v := SEND_RESET;
    >> end if;
    >> else
    >> inc_tic_count;
    >> end if;
    >>
    >> -------------------------------------------------------------------------------
    >>
    >> when SEND_READ_SCRATCH =>
    >> mstate_v := SEND_READ_SCRATCH;
    >> -- read data after the first SEND_DATA is over (cnt_v = 2)
    >> if cnt_v> 1 then
    >> temp_value_v(cnt_v - 2) := DATA_v;
    >> end if;
    >>
    >> if cnt_v = 0 then
    >> cmd_v := READ_SCRATCH;
    >> state_v := SEND_CMD;
    >> inc_tic_count;
    >> elsif cnt_v<= 2 then
    >> dat_v := GET_DATA;
    >> state_v := SEND_DATA;
    >> inc_tic_count;
    >> else
    >> inc_tem_count;
    >> temp_valid_v := '1';
    >> cnt_v := 0;
    >> mstate_v := SEND_SKIP_ROM;
    >> lstate_v := SEND_CONVERT;
    >> state_v := SEND_RESET;
    >> end if;
    >>
    >> when others => null;
    >> end case;
    >> else
    >> state_v := IDLE;
    >> end if;
    >> end procedure autoread;
    >>
    >> -------------------------------------------------------------------------------
    >> procedure update_regs is
    >> begin -- purpose: call the procedures above in the desired order
    >> autoread;
    >> end procedure update_regs;
    >>
    >> -------------------------------------------------------------------------------
    >> -- Synchronous Templates for Synthesis: The following templates use
    >> -- all of the declarations above to make a UART. With other
    >> -- declarations, the template could make anything else. Relative
    >> -- synthesis performance will be compared below for each template.
    >> -- If I were not making comparisons, the first template would be
    >> -- pasted into the main process.
    >> -------------------------------------------------------------------------------
    >> procedure template_v_rst is
    >> begin -- a_rst is logically equivalent
    >> if RST = '1' then -- Assumes synched trailing edge RST pulse
    >> init_regs; -- reg_v := init_c; Variables only, ports below.
    >> elsif rising_edge(CLK) then
    >> update_regs; -- reg_v := f(reg_v);Variables only, ports below.
    >> end if; -- Synchronous init optional (state_v = idle_c)
    >> update_ports; -- will infer port wires ok for RST and CLK
    >> end procedure template_v_rst; -- out_port<= reg_v; ports only, no signals
    >>
    >> -------------------------------------------------------------------------------
    >> begin -- process main
    >> template_v_rst;
    >> end process main;
    >>
    >> -------------------------------------------------------------------------------
    >>
    >> end tfsm_arch;

    >
    >
    >
    > </code>



    --

    Rick
     
    rickman, Sep 23, 2013
    #8
  9. alb

    Andy Guest

    Al,

    I have had problems in synthesis (synplify pro) when calling the same procedure under multiple conditions (e.g. states in a state machine) from a clocked process.

    Synthesizer did not complain, but the HW did not work, and the post-synth netlist simulation did not match the RTL simulation. The post-synth simulation was consistent with behavior observed in the lab (we could not observe the exact failure in the lab, but the overall results were consistent). P&R STA passed with no errors/warnings.

    To work around it, I created a variable flag that was set anytime I needed to run the procedure, then below the fsm code (in the same clocked process or update_regs procedure in your case), I called the procedure within an if-statement when the flag was set. Note that since the flag was written every clock cycle before read to determine whether to run the procedure, the flag was combinatorial, and the procedure ran in the same clock cycle that set the flag.

    I have not tried to create a boiled-down test case to submit to Synopsys yet. My procedure used an inout variable parameter, and updated a CRC checksum based on data provided in the procedure call. Updating an inout variable parameter would behave identically to updating an external (but in-scope) variable.

    Hope this helps,

    Andy
     
    Andy, Sep 23, 2013
    #9
  10. On Friday, September 20, 2013 3:42:31 AM UTC-7, alb wrote:
    > > It sim'ed ok, both pre/post-synthesis and post-layout.



    In an init,update,output single process design using variables,
    you can ignore warnings about variable reuse if synthesis completes without error and sim is ok.

    -- Mike Treseler
     
    Mike Treseler, Sep 23, 2013
    #10
  11. alb

    alb Guest

    Hi Mike,

    On 23/09/2013 18:51, Mike Treseler wrote:
    > On Friday, September 20, 2013 3:42:31 AM UTC-7, alb wrote:
    >>> It sim'ed ok, both pre/post-synthesis and post-layout.

    >
    >
    > In an init,update,output single process design using variables,
    > you can ignore warnings about variable reuse if synthesis completes without error and sim is ok.


    there are a couple of things that do not make me feel happy even if sim
    is ok:

    1. the amount of logic inferred is exceeding my predictions (this is
    just an FSM with some registers and a couple of counters and it takes
    12% of a 250k gates FPGA!).

    2. in the lab it does not work.

    Concerning 1. I'll need to investigate the RTL and see if there's
    something wrong going on (the Synplify Pro RTL viewer should be
    sufficient to do so).

    About 2., this is an FSM to read out a 1-wire bus in an infinite loop
    and I believe that at power up the 'Reset' on the 1-wire is sent too
    early (the line is still not stable at 'H') and the presence pulse is
    not sampled properly, causing the FSM to loop indefinitely in a state
    waiting for the presence bit to show up. I think this can be easily
    fixed waiting for the line to be stable first.
     
    alb, Sep 24, 2013
    #11
  12. alb

    alb Guest

    On 23/09/2013 17:11, rickman wrote:
    []
    >> A latch is inferred if you write a process where an output is not
    >> assigned under all possible input conditions:
    >>
    >> <code>
    >> process (clk, rst)
    >> begin
    >> if rst = '1' then
    >> nRD_s<= '1'; -- nRD_s is a signal instead of a variable
    >> foo_s<= nRD_s; -- foo_s is not assigned under all possible inputs
    >> -- of the process therefore a latch is needed to
    >> -- retain its value.
    >> elsif rising_edge(clk) then
    >> if<condition> then
    >> nRD_s<= not nRD_s;
    >> end if;
    >> end if;
    >> end process;
    >> </code>
    >>
    >> So my code should not infer a latch (and indeed it does not).

    >
    > Your premise is not complete. A latch is inferred as well if you create
    > a feedback loop. nRD_v := not nRD_v; creates feedback if it is not
    > registered.


    On a clocked process the assignment you quote is perfectly legal and
    does not create any latch (it may still create a feedback mux if nRD_v
    is not reset).

    > It appears it should be registered however. So the
    > questions are,
    >
    > 1) Is it registered?


    yes

    >
    > 2) If it is registered, how is the message being generated?


    Assuming the message you refer to is the OP's note issued by Designer,
    that is an unfair question to ask since I asked it first ;-)

    > BTW, you *don't* assign a value to nRD_v under all conditions.
    >
    > when IDLE =>
    > cnt_v := 0;
    > state_v := SET_CLKRATE;
    > when READ_BGN =>
    > state_v := READ_END; -- this is one clock cycle
    > nRD_v := not nRD_v;


    I know the code is a bit long to go through, but the 'autorun' procedure
    is executed during 'update_reg', which is called in the 'if
    rising_edge(clk)' branch of the -only - process.

    The variable is also properly initialized in bus_init, called during
    init_regs, in the 'other' branch of the clocked process. Therefore the
    variable nRD_v *is* assigned under all possible conditions which will
    trigger the process to run.

    []
    >> Believe me, if I knew this stuff 'pretty well' I won't be asking ;-).
    >> I'm actually exploring the usage of variables to a greater extent,
    >> together with a new 'procedural' approach and I'm afraid I'm misusing it
    >> somehow.

    >
    > Yes, I have never used variables so much myself. That's why I missed
    > that nRD_v was a variable. I missed both the := and the _v ... duh!


    I keep the _v reminder in the name which may seem redundant since it is
    clear that I cannot assign a variable with a non-blocking assignment.
    But generally I tend to use no reminders for the entity ports to
    increase readability and therefore I find it natural to use the _v
    internally.

    > I also don't use procedures so much. I should try to work with them
    > more so I understand them better. There are times when they are useful
    > for decomposition of code into reusable modules.


    After having been contaminated by software development for three years I
    started to see the power of using subprograms and IMO VHDL has all the
    language constructs to allow a high level of abstraction, resulting in
    fewer lines of code (-> less bugs [1]), greater readability and reuse.

    >
    >> Funny enough the code simulates just as I want it, maybe I've only been
    >> lucky...

    >
    > *Something* is amiss. Has this error message been generated all along?
    > If not, see what you changed. If it has I would say simplify the code
    > to something that doesn't give the error and figure out what is different.


    I can indeed start to remove bits and pieces to see what really fools
    the tool, but I'm not happy about the flow. How can it be that only the
    P&R has noticed the combinational loop?

    [1] Halstead, Maurice H. (1977). Elements of Software Science.
    Amsterdam: Elsevier North-Holland, Inc.
     
    alb, Sep 24, 2013
    #12
  13. alb

    alb Guest

    Hi Rick,

    On 23/09/2013 17:22, rickman wrote:
    []
    > This is the first time you have compiled the code isn't it? So you have
    > not done any testing on the code at all so far?


    As I mentioned in the beginning the code has been sim'ed and it works
    properly in post-synth and post-layout (even though STA shows timing
    violations on unrelated parts of the code).

    Unfortunately it does not work in the lab, but I think is more related
    to the power-up rather than a functional problem (at power-up the 1-wire
    line is pulled 'H' through a pull-up resistor and the sequence to talk
    on the line starts too early).

    > Where are your parameter lists for the procedures such as update_ports?


    No parameter lists. All variables have global scope in the process.

    This is indeed something I do not completely like about this approach
    since I haven't found a way to 'protect' the access to variables and you
    may end up with variable assignments all scattered, killing the benefit
    of subprogram partitioning.
     
    alb, Sep 24, 2013
    #13
  14. alb

    alb Guest

    Hi Andy,

    On 23/09/2013 18:44, Andy wrote:
    []
    > I have had problems in synthesis (synplify pro) when calling the same
    > procedure under multiple conditions (e.g. states in a state machine)
    > from a clocked process.


    This is no good news! :-/

    > Synthesizer did not complain, but the HW did not work, and the
    > post-synth netlist simulation did not match the RTL simulation. The
    > post-synth simulation was consistent with behavior observed in the
    > lab (we could not observe the exact failure in the lab, but the
    > overall results were consistent). P&R STA passed with no
    > errors/warnings.


    The HW does not work for me as well, but I'm afraid for a different
    reason. Conversely to your case, my code sim ok, post-synth and
    post-layout. P&R STA reports issues but on a different part of the design.

    What about the RTL viewer? Did it show something reasonable or not?

    > To work around it, I created a variable flag that was set anytime I
    > needed to run the procedure, then below the fsm code (in the same
    > clocked process or update_regs procedure in your case), I called the
    > procedure within an if-statement when the flag was set. Note that
    > since the flag was written every clock cycle before read to determine
    > whether to run the procedure, the flag was combinatorial, and the
    > procedure ran in the same clock cycle that set the flag.


    something similar to this:

    <code>
    -- declarative part of global variables:
    variable flag: std_logic;
    -- ...
    -------------------------------------------------------------------------------
    procedure update_regs is
    begin -- purpose: call the procedures above in the desired order
    if flag then
    autoread;
    flag := '0'; -- reset flag.
    end if;
    end procedure update_regs;
    -------------------------------------------------------------------------------
    procedure template_v_rst is
    begin -- a_rst is logically equivalent
    if RST = '1' then
    init_regs;
    elsif rising_edge(CLK) then
    flag := '1'; -- set flag.
    update_regs;
    end if;
    update_ports;
    end procedure template_v_rst;
    -- ...

    </code>

    I'll try it and post results.

    >
    > I have not tried to create a boiled-down test case to submit to
    > Synopsys yet.


    I'll see what I can come up with, but I'm a bit under pressure to
    complete and I'm not sure I'll have the time to find a test case.
     
    alb, Sep 24, 2013
    #14
  15. alb

    alb Guest

    This is a correction to my previous post.

    On 24/09/2013 10:19, alb wrote:
    []
    >> BTW, you *don't* assign a value to nRD_v under all conditions.
    >>
    >> when IDLE =>
    >> cnt_v := 0;
    >> state_v := SET_CLKRATE;
    >> when READ_BGN =>
    >> state_v := READ_END; -- this is one clock cycle
    >> nRD_v := not nRD_v;

    >
    > I know the code is a bit long to go through, but the 'autorun' procedure
    > is executed during 'update_reg', which is called in the 'if
    > rising_edge(clk)' branch of the -only - process.
    >
    > The variable is also properly initialized in bus_init, called during
    > init_regs, in the 'other' branch of the clocked process. Therefore the
    > variable nRD_v *is* assigned under all possible conditions which will
    > trigger the process to run.


    You are right, the variable nRD_v *is not* assigned under all possible
    branches, but it is assigned in a clocked process therefore is
    synthesized to a dff.

    Sorry for the confusion.
     
    alb, Sep 24, 2013
    #15
  16. On Tuesday, September 24, 2013 12:32:07 AM UTC-7, alb wrote:
    >
    > there are a couple of things that do not make me feel happy even if sim
    > is ok:
    > 1. the amount of logic inferred is exceeding my predictions (this is
    > just an FSM with some registers and a couple of counters and it takes
    > 12% of a 250k gates FPGA!).


    Have a look on the rtl viewer. Some variable array is too large or some
    block ram does not fit the device.

    If you don't have a viewer, start out with the free quartus tools.

    > 2. in the lab it does not work.


    Start with with just the reset procedure
    check the reset pulse and all static outputs.

    Add a clock and counter and check that. etc.

    > Concerning 1. I'll need to investigate the RTL and see if there's
    > something wrong going on (the Synplify Pro RTL viewer should be
    > sufficient to do so).


    I never go near the bench until the RTL view is as expected.

    > About 2., this is an FSM to read out a 1-wire bus in an infinite loop
    > and I believe that at power up the 'Reset' on the 1-wire is sent too
    > early (the line is still not stable at 'H') and the presence pulse is
    > not sampled properly, causing the FSM to loop indefinitely in a state
    > waiting for the presence bit to show up. I think this can be easily
    > fixed waiting for the line to be stable first.


    I would read a burst at at time and check sims that the bus port has Z,1,0
    at the right times.

    Good Luck,

    -- Mike Treseler
     
    Mike Treseler, Sep 24, 2013
    #16
  17. alb

    Andy Guest

    Al,

    I think you need flags to control calling the sub-procedures within autoread, not calling autoread itself. It looks like you would also need variablesto hold the parameters you are going to pass to some of your sub-procedures.

    After reviewing your FSM though, I think you might do better with separate,smaller state machine(s) for the reusable states, and implement a hierarchical state machine. There is no point in combining all the reusable and unique states into the same FSM.

    Does Synplify actually recognize your FSM as an FSM (what does it look likein the RTL viewer or FSM explorer)? Sometimes if you assign the next statefrom the contents of a register other than the current state, Synplify will not treat it as a state machine, which then excludes optimizations normally performed on FSMs.

    As for my example that illuminated the synthesis problem, the procedure's logic was complex enough that any abnormalities would not have been easy to spot in RTL view. Unfortunately, Synplify unrolls functions and procedures for RTL view.

    Andy
     
    Andy, Sep 24, 2013
    #17
  18. alb

    rickman Guest

    On 9/24/2013 4:29 AM, alb wrote:
    > Hi Rick,
    >
    > On 23/09/2013 17:22, rickman wrote:
    > []
    >> This is the first time you have compiled the code isn't it? So you have
    >> not done any testing on the code at all so far?

    >
    > As I mentioned in the beginning the code has been sim'ed and it works
    > properly in post-synth and post-layout (even though STA shows timing
    > violations on unrelated parts of the code).
    >
    > Unfortunately it does not work in the lab, but I think is more related
    > to the power-up rather than a functional problem (at power-up the 1-wire
    > line is pulled 'H' through a pull-up resistor and the sequence to talk
    > on the line starts too early).
    >
    >> Where are your parameter lists for the procedures such as update_ports?

    >
    > No parameter lists. All variables have global scope in the process.
    >
    > This is indeed something I do not completely like about this approach
    > since I haven't found a way to 'protect' the access to variables and you
    > may end up with variable assignments all scattered, killing the benefit
    > of subprogram partitioning.


    Ok, at this point I am showing my ignorance of using procedures in VHDL.
    I never realized that scope would work this way. In fact, this sounds
    very unlike VHDL. I'm still a bit confused about a couple of things.

    The procedures init_regs and update_ports may be in a clocked process,
    but they are *not* in the clocked region of the clocked process and so
    can generate combinatorial logic. Again, I have not looked at the code
    in detail as that would require me to copy to a text file and open it in
    a decent editor rather than this limited newsreader. Are you sure there
    are no issues in one of those two procedures?

    As to the scope issue, I don't think you *have* to use the global scope.
    You can pass parameters into the procedures if you choose to. It
    seems to me that using procedures provides limited advantages over
    non-modular code by not using parameters.

    --

    Rick
     
    rickman, Sep 25, 2013
    #18
  19. alb

    alb Guest

    Hi Rick,

    On 25/09/2013 06:52, rickman wrote:
    []
    >>> Where are your parameter lists for the procedures such as update_ports?

    >>
    >> No parameter lists. All variables have global scope in the process.
    >>
    >> This is indeed something I do not completely like about this approach
    >> since I haven't found a way to 'protect' the access to variables and you
    >> may end up with variable assignments all scattered, killing the benefit
    >> of subprogram partitioning.

    >
    > Ok, at this point I am showing my ignorance of using procedures in VHDL.
    > I never realized that scope would work this way. In fact, this sounds
    > very unlike VHDL. I'm still a bit confused about a couple of things.


    There's only one process in the entire entity. Variables have local
    scope in the process, but since there's nothing except one process they
    can be considered 'global' in the entity. Moreover every procedure
    defined within the process has access to all local variables as well.

    > The procedures init_regs and update_ports may be in a clocked process,
    > but they are *not* in the clocked region of the clocked process and so
    > can generate combinatorial logic.


    the init_regs procedure is called in the reset branch of the clocked
    process, therefore will run to 'reset' all initial states of any locally
    defined variable.

    In update_ports all appropriate variables are 'wired' to ports (no
    signals). Meaning that every time the process is triggered they will be
    assigned to a port. Since update_ports is only called when the process
    is triggered I'm not sure if you can have an output port which is mapped
    to a pure combinatorial logic of a set of inputs.

    > Again, I have not looked at the code
    > in detail as that would require me to copy to a text file and open it in
    > a decent editor rather than this limited newsreader. Are you sure there
    > are no issues in one of those two procedures?


    Yeah I apologize for the code format, I try to keep everything under 80
    characters for this very purpose but I'm not always so strict :-/

    I'm trying to remove most of the stuff and synthesize piece by piece.
    Honestly I do not see how the init_regs and update_ports procedures can
    be broken.

    >
    > As to the scope issue, I don't think you *have* to use the global scope.
    > You can pass parameters into the procedures if you choose to. It seems
    > to me that using procedures provides limited advantages over non-modular
    > code by not using parameters.


    AFAIK vhdl passes arguments by value, making a local copy of the
    parameter which has local scope to the subprogram, in this case I do not
    know how I can have my 'state' variable retaining the value through
    clock cycles.
     
    alb, Sep 25, 2013
    #19
  20. alb

    alb Guest

    On 24/09/2013 19:21, Andy wrote:
    > Al,
    >
    > I think you need flags to control calling the sub-procedures within
    > autoread, not calling autoread itself. It looks like you would also
    > need variables to hold the parameters you are going to pass to some
    > of your sub-procedures.


    flags to control sub-procedures calling are just the state of the FSM,
    while sub-procedures operate on global variables.

    >
    > After reviewing your FSM though, I think you might do better with
    > separate, smaller state machine(s) for the reusable states, and
    > implement a hierarchical state machine. There is no point in
    > combining all the reusable and unique states into the same FSM.


    I started off by 'encapsulating' the reusable states in separate
    procedures, but I was not sure how to 'wait' the end of it, that is why
    I ended up putting everything under a single FSM.

    > Does Synplify actually recognize your FSM as an FSM (what does it
    > look like in the RTL viewer or FSM explorer)? Sometimes if you assign
    > the next state from the contents of a register other than the current
    > state, Synplify will not treat it as a state machine, which then
    > excludes optimizations normally performed on FSMs.


    That is true indeed. Apparently the FSM explorer does not recognize an
    FSM. Definitely not good, a hierarchical approach becomes definitely
    mandatory.

    >
    > As for my example that illuminated the synthesis problem, the
    > procedure's logic was complex enough that any abnormalities would not
    > have been easy to spot in RTL view. Unfortunately, Synplify unrolls
    > functions and procedures for RTL view.


    The fact that my RTL looks messier than what I think should have been a
    clear hint that something is wrong.
     
    alb, Sep 25, 2013
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. ALuPin

    Combinational Loop?

    ALuPin, Aug 30, 2004, in forum: VHDL
    Replies:
    5
    Views:
    6,890
    Ken Smith
    Aug 31, 2004
  2. GuitarNerd
    Replies:
    8
    Views:
    3,820
    Andy Peters
    May 19, 2005
  3. Taras_96
    Replies:
    2
    Views:
    950
    Taras_96
    Aug 22, 2005
  4. john
    Replies:
    13
    Views:
    3,328
    Mike Treseler
    May 3, 2006
  5. Me
    Replies:
    2
    Views:
    249
Loading...

Share This Page