FPGA design rules - Using a Module Output Register Value Internaly

Question

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.

Answer 1

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