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):
    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
     
    alb, Sep 20, 2013
    #1
    1. Advertisements

  2. alb

    alb Guest

    Correction to my previous post in this thread:

    On 20/09/2013 12:33, alb wrote:
    []
    STA *does* report timing violations.
     
    alb, Sep 20, 2013
    #2
    1. Advertisements

  3. alb

    rickman Guest

    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.
     
    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 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 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...
    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>
     
    alb, Sep 23, 2013
    #4
  5. alb

    rickman Guest

    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?
     
    rickman, Sep 23, 2013
    #5
  6. alb

    alb Guest

    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):

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

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

    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.

    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.

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


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

    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:
    []
    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).
    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 ;-)
    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.

    []
    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.
    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.
    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:
    []
    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).
    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:
    []
    This is no good news! :-/
    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?
    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'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:
    []
    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. 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.
    Start with with just the reset procedure
    check the reset pulse and all static outputs.

    Add a clock and counter and check that. etc.
    I never go near the bench until the RTL view is as expected.
    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

    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.
     
    rickman, Sep 25, 2013
    #18
  19. alb

    alb Guest

    Hi Rick,

    On 25/09/2013 06:52, rickman wrote:
    []
    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 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.
    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.
    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

    flags to control sub-procedures calling are just the state of the FSM,
    while sub-procedures operate on global variables.
    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.
    That is true indeed. Apparently the FSM explorer does not recognize an
    FSM. Definitely not good, a hierarchical approach becomes definitely
    mandatory.
    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. Advertisements

Ask a Question

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

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.