1
\$\begingroup\$

I'm trying to optimize a verilog code and I found something that I don't feel it's correct. I found a module that has an output and it's using that output value as a condition in a case statement. There is an example:

module TT
(   
    output  reg                             Busy        =   1'b0
,   output  reg                             Finished    =   1'b0

,   input   wire                            clk
,   input   wire                            rst
,   input   wire                            start
,   input   wire    [   5 - 1   :   0   ]   freq
);

always @ (posedge clk)
begin
    if(!rst)
    begin
        case(Busy)
            1'b0:
            begin
                //do some logic...
                if(something)
                    Busy = 1'b1;
            end

            1'b1:
            begin
                //do some logic...
                if(something_else)
                    Busy = 1'b0;
            end
        endcase
    end
end
endmodule

Is it good practice to do this? If not, why? Thanks a lot.

\$\endgroup\$
  • 1
    \$\begingroup\$ looks just like feeding the Q output of a D-type flip flop around to the D input (via some other logic) to me. \$\endgroup\$ – user1582568 Jan 29 '16 at 11:47
  • \$\begingroup\$ Missing elsecan cause latch. Use of non blocking statements is required. \$\endgroup\$ – sharvil111 Jan 29 '16 at 12:32
  • \$\begingroup\$ There's nothing wrong with it, and it's more concise than creating a separate internal signal for the module. Older versions of VHDL (and software tools) required such a signal, but that has since gone away. \$\endgroup\$ – Dave Tweed Jan 29 '16 at 13:32
  • \$\begingroup\$ I think my doubt is clarified. I'll keep this configuration for now, but I'm still going to make some tests with a different approach just for learning what is really happening inside the FPGA... Thanks ;) \$\endgroup\$ – Rui Cunha Jan 29 '16 at 15:19
4
\$\begingroup\$

Seems fine.

Three suggestions/recommendations:

  1. Use non-blocking assignments for registers

  2. Use an if-else instead of the case.

  3. You are missing the reset clause - how does it know what value to take when in reset.

So the code would become:

always @ (posedge clk) begin
    if (rst) begin
        Busy <= 1'b0; //Reset busy with the reset signal?
    end else begin
        if (!Busy && something) begin
            //When not busy and something happens
            Busy <= 1'b1;
        end

        if (Busy && something_else) begin
            //When busy and something else happens
            Busy <= 1'b0;
        end
    end
end

Worth mentioning in the above it is fine having two separate if statements controlling the same register because it is quite clear that they will never both happen at the same time (one happens on !Busy the other on Busy so they are mutually exclusive).

You can also see here that I have used non-blocking statements. This is the preferred approach for always blocks. Where possible try to use non-blocking, using blocking assignments only in functions and tasks. There are some places where blocking can be useful, but more often than not there is no reason to use them.

If you were doing more stuff, you may find it easier to have it in an if-else format, such as:

if (!Busy) begin
   //Logic when not busy. E.g.
   if (something) begin
       Busy <= 1'b1;
   end
end else begin
   //Logic when busy. E.g.
   if (something_else) begin
       Busy <= 1'b0;
   end
end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.