combinational loops

[alb]

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]

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.

 

[rickman]

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

 

[alb]

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>

 

[Andy]

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

 

[Mike Treseler]

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

 

[rickman]

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

 

[alb]

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...

 

[rickman]

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]

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

 

[alb]

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]

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]

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]

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.

 

[Mike Treseler]

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

 

[alb]

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.

 

[Andy]

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.

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 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.

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

 

[rickman]

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

 

[Andy]

Al,

I checked your update_ports procedure. In a clocked process, you cannot have a combinatorial in-to-out path. I'm surprised it is not flagged as a warnging from the synthesis tool. I really don't know what circuitry you might get from using input ports in the update_ports procedure, but it is possible that, on a clock-cycle basis, the circuitry might still match simulation.

All outputs driven by update_ports must be functions only of process variables or constants/generics, NOT input ports or signals. The references to variables in update_ports are to the registered values thereof (remember, update_ports also runs on the falling edge of the clock), and any logic (decision making, expression, etc.) in the procedure is combinatorial after the registers.

Just like in a combinatorial process, you also have to make sure that any ports that may be assigned in update_ports are always assigned, or else a latch will be inferred to remember the last value assigned to the port.

If update_ports were called inside the clocked if-then-else, the cycle-based timing of the outputs would be identical, but the input variables would be combinatorial, along with any logic in the procedure, and the ports would be registered. In this case, you would also need to reset the output port registers.

The synchronous process template with three procedures (init_regs, update_regs, update_ports) works well within limits, but those limits must be observed, and are not always well understood. I never really bought into the idea that anything was improved by using procedures here; I tend to see the three regions of the clocked process as having those three distinct functions anyway, procedure or not. Reuse of a procedure multiple times would be a good reason, if not for the synthesis bug I mentioned.

Andy

 

[alb]

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.