Verilog case statements

While reviewing the code that the FPGA group has produced, I saw
something that looks bad.  It is not likely to affect the
functionality, but it is not good coding style and may use extra
resources.

They are using Verilog which is not my first HDL language and I am not
as familiar with it as I am VHDL.  But because the case statement is
not fully specified the code below appears to me to produce more
complex logic than needed.

always @ (negedge rst_n or posedge clk) begin
  if (!rst_n)begin
    mask_wr      <= 1'b0;
    clear_wr      <= 1'b0;
    end
  else begin
    if (wr == 1'b1)begin
      case (adr [2:1])
        MASK:     mask_wr      <= 1'b1;
        CLEAR:   clear_wr      <= 1'b1;
        endcase
      end
    else begin
      mask_wr      <= 1'b0;
      clear_wr      <= 1'b0;
    end
  end
end

The real code is controlling 20 or so signals and is very large so this
is very simplified.  At first I didn't notice the enclosing "always"
statement and thought it would produce a latch.  Now I realize that it
will make enabled registers when what is really desired is just
decoders followed by registers.  I guess in the grand scheme of things
this is not a big issue.  But wouldn't it make a more streamlined
result to code the logic separate from the register?  Then the logic
would just be decodes of the address bus and the control signal and the
register would not have separate enables.

"rickman" <spamgoeshere4@yahoo.com> wrote in message 
news:1154099086.215982.7080@m79g2000cwm.googlegroups.com...
> While reviewing the code that the FPGA group has produced, I saw
> something that looks bad.  It is not likely to affect the
> functionality, but it is not good coding style and may use extra
> resources.
>
> They are using Verilog which is not my first HDL language and I am not
> as familiar with it as I am VHDL.  But because the case statement is
> not fully specified the code below appears to me to produce more
> complex logic than needed.
>
> always @ (negedge rst_n or posedge clk) begin
>  if (!rst_n)begin
>    mask_wr      <= 1'b0;
>    clear_wr      <= 1'b0;
>    end
>  else begin
>    if (wr == 1'b1)begin
>      case (adr [2:1])
>        MASK:     mask_wr      <= 1'b1;
>        CLEAR:   clear_wr      <= 1'b1;
>        endcase
>      end
>    else begin
>      mask_wr      <= 1'b0;
>      clear_wr      <= 1'b0;
>    end
>  end
> end
>
> The real code is controlling 20 or so signals and is very large so this
> is very simplified.  At first I didn't notice the enclosing "always"
> statement and thought it would produce a latch.  Now I realize that it
> will make enabled registers when what is really desired is just
> decoders followed by registers.  I guess in the grand scheme of things
> this is not a big issue.  But wouldn't it make a more streamlined
> result to code the logic separate from the register?  Then the logic
> would just be decodes of the address bus and the control signal and the
> register would not have separate enables.

It may be cleaner to have simpler logic where the style
  mask_wr <= wr & adr[2:1]==MASK;
is used for several declarations.  This looks very similar to code I'm used 
to seeing where you could substitute the write strobes with the actual 
register write of the mask and clear registers in the case statement for and 
exclude the else.  It may be the engineer used a common block and pared it 
down to produce just the strobes.

Synthesizers tend to imply less about the type of flop - enabled, 
synchronous reset - from the code than they used to, usually ending up with 
a well-optimized result.  Sometimes you have to kick the synthesizer in the 
@$$.

I'd rewrite the always block if the code was something I became responsible 
for.  It works fine, simulates fine, synthesizes fine; it's just us humans 
that are uncomfortable with the style.

- John_H 

rickman wrote:

> always @ (negedge rst_n or posedge clk) begin
>   if (!rst_n)begin
>     mask_wr      <= 1'b0;
>     clear_wr      <= 1'b0;
>     end
>   else begin
>     if (wr == 1'b1)begin
>       case (adr [2:1])
>         MASK:     mask_wr      <= 1'b1;
>         CLEAR:   clear_wr      <= 1'b1;
>         endcase
>       end
>     else begin
>       mask_wr      <= 1'b0;
>       clear_wr      <= 1'b0;
>     end
>   end
> end

process(rst_n,clk)
begin
if (rst_n='0') then
    mask_wr<='0';
    clear_wr<='0';
elsif rising_edge(clk) then
    if (wr='1') then
       case adr(2 downto 1) is
       when MASK  => mask_wr<='1';
       when CLEAR => clear_wr<='1';
       when others => null;
       end case;
    else
       mask_wr<='0';
       clear_wr<='0';
    end if;
end if;
end process;

(Translated manually without checking.)

I don't see anthything inefficient. 2 Flipflops with async reset.


Ralf

Ralf Hildebrandt wrote:
> rickman wrote:
>
> > always @ (negedge rst_n or posedge clk) begin
> >   if (!rst_n)begin
> >     mask_wr      <= 1'b0;
> >     clear_wr      <= 1'b0;
> >     end
> >   else begin
> >     if (wr == 1'b1)begin
> >       case (adr [2:1])
> >         MASK:     mask_wr      <= 1'b1;
> >         CLEAR:   clear_wr      <= 1'b1;
> >         endcase
> >       end
> >     else begin
> >       mask_wr      <= 1'b0;
> >       clear_wr      <= 1'b0;
> >     end
> >   end
> > end
>
> process(rst_n,clk)
> begin
> if (rst_n='0') then
>     mask_wr<='0';
>     clear_wr<='0';
> elsif rising_edge(clk) then
>     if (wr='1') then
>        case adr(2 downto 1) is
>        when MASK  => mask_wr<='1';
>        when CLEAR => clear_wr<='1';
>        when others => null;
>        end case;
>     else
>        mask_wr<='0';
>        clear_wr<='0';
>     end if;
> end if;
> end process;
>
> (Translated manually without checking.)
>
> I don't see anthything inefficient. 2 Flipflops with async reset.

Two FFs and four logic elements (x15 in the real circuit).  You have to
use a LUT to drive the D input and you have to use a LUT to drive the
enable input.  Of course you can feedback the output to the input and
not use the clock enable, but that will also use more LUTs depending on
if you have the extra inputs or not on the LUTs you are using.  But
this will be up to the tool and many times I see the tool generating
logic to feed the clock enable.

It is not a big deal.  I tend to think of things like this when I code
in an HDL just so I know my logic generation is lean.  Originally I had
not seen the enclosing always statement and was thinking it was
generating a latch.  Once I started looking at it I realize the latch
was not an issue but thought about the extra logic being generated.  I
tend to separate my registers from my logic just to allow me to
optimize this sort of thing.

"rickman" <spamgoeshere4@yahoo.com> wrote in message 
news:1154099086.215982.7080@m79g2000cwm.googlegroups.com...
> While reviewing the code that the FPGA group has produced, I saw
> something that looks bad.  It is not likely to affect the
> functionality, but it is not good coding style and may use extra
> resources.
>
> They are using Verilog which is not my first HDL language and I am not
> as familiar with it as I am VHDL.  But because the case statement is
> not fully specified the code below appears to me to produce more
> complex logic than needed.
>
> always @ (negedge rst_n or posedge clk) begin
>  if (!rst_n)begin
>    mask_wr      <= 1'b0;
>    clear_wr      <= 1'b0;
>    end
>  else begin
>    if (wr == 1'b1)begin
>      case (adr [2:1])
>        MASK:     mask_wr      <= 1'b1;
>        CLEAR:   clear_wr      <= 1'b1;
>        endcase
>      end
>    else begin
>      mask_wr      <= 1'b0;
>      clear_wr      <= 1'b0;
>    end
>  end
> end
>
> The real code is controlling 20 or so signals and is very large so this
> is very simplified.  At first I didn't notice the enclosing "always"
> statement and thought it would produce a latch.  Now I realize that it
> will make enabled registers when what is really desired is just
> decoders followed by registers.  I guess in the grand scheme of things
> this is not a big issue.  But wouldn't it make a more streamlined
> result to code the logic separate from the register?  Then the logic
> would just be decodes of the address bus and the control signal and the
> register would not have separate enables.
>

Rick -

This looks fine to me. It's easy to read and will function correctly, plus 
the case statement and use of parameters make it easy to modify. Given what 
I've heard of your relationship with the FPGA group, I would not flag this 
if I were you. It would be nitpicking.

Rob 

rickman wrote:

>> process(rst_n,clk)
>> begin
>> if (rst_n='0') then
>>     mask_wr<='0';
>>     clear_wr<='0';
>> elsif rising_edge(clk) then
>>     if (wr='1') then
>>        case adr(2 downto 1) is
>>        when MASK  => mask_wr<='1';
>>        when CLEAR => clear_wr<='1';
>>        when others => null;
>>        end case;
>>     else
>>        mask_wr<='0';
>>        clear_wr<='0';
>>     end if;
>> end if;
>> end process;

> Two FFs and four logic elements (x15 in the real circuit).  You have to
> use a LUT to drive the D input and you have to use a LUT to drive the
> enable input.  Of course you can feedback the output to the input and
> not use the clock enable, but that will also use more LUTs depending on
> if you have the extra inputs or not on the LUTs you are using.

I don't know your target, but I don't thinks a clock-enable will be 
implemented. There will be a mux that selects ('1', '0' or the previous 
value of the FFs) I guess.

If this is the desired functional behavior I don't see any option to 
make it smaller. If there are don't care conditions there may be options.


There will be no difference between the way of coding this (two 
processes for the FFs and the combinational logic). There will be only a 
difference if you find some functional simplification of the problem.


Ralf

"rickman" <spamgoeshere4@yahoo.com> wrote in message 
news:1154101628.534694.304340@s13g2000cwa.googlegroups.com...

> Two FFs and four logic elements (x15 in the real circuit).  You have to
> use a LUT to drive the D input and you have to use a LUT to drive the
> enable input.  Of course you can feedback the output to the input and
> not use the clock enable, but that will also use more LUTs depending on
> if you have the extra inputs or not on the LUTs you are using.  But
> this will be up to the tool and many times I see the tool generating
> logic to feed the clock enable.
>
> It is not a big deal.  I tend to think of things like this when I code
> in an HDL just so I know my logic generation is lean.  Originally I had
> not seen the enclosing always statement and was thinking it was
> generating a latch.  Once I started looking at it I realize the latch
> was not an issue but thought about the extra logic being generated.  I
> tend to separate my registers from my logic just to allow me to
> optimize this sort of thing.

You're taking the code structure as a literal guide for synthesis.  There 
will be two FFs and two LUTs without a clock enable.  Synthesis typically 
knows what simple logic breaks down to and how to best implement it.  If you 
go into the technology view of your synthesizer to look at one of the flops, 
expect to see only one LUT driving it, no clock enable involved. 

RobJ wrote:
> "rickman" <spamgoeshere4@yahoo.com> wrote in message
> news:1154099086.215982.7080@m79g2000cwm.googlegroups.com...
> > While reviewing the code that the FPGA group has produced, I saw
> > something that looks bad.  It is not likely to affect the
> > functionality, but it is not good coding style and may use extra
> > resources.
> >
> > They are using Verilog which is not my first HDL language and I am not
> > as familiar with it as I am VHDL.  But because the case statement is
> > not fully specified the code below appears to me to produce more
> > complex logic than needed.
> >
> > always @ (negedge rst_n or posedge clk) begin
> >  if (!rst_n)begin
> >    mask_wr      <= 1'b0;
> >    clear_wr      <= 1'b0;
> >    end
> >  else begin
> >    if (wr == 1'b1)begin
> >      case (adr [2:1])
> >        MASK:     mask_wr      <= 1'b1;
> >        CLEAR:   clear_wr      <= 1'b1;
> >        endcase
> >      end
> >    else begin
> >      mask_wr      <= 1'b0;
> >      clear_wr      <= 1'b0;
> >    end
> >  end
> > end
> >
> > The real code is controlling 20 or so signals and is very large so this
> > is very simplified.  At first I didn't notice the enclosing "always"
> > statement and thought it would produce a latch.  Now I realize that it
> > will make enabled registers when what is really desired is just
> > decoders followed by registers.  I guess in the grand scheme of things
> > this is not a big issue.  But wouldn't it make a more streamlined
> > result to code the logic separate from the register?  Then the logic
> > would just be decodes of the address bus and the control signal and the
> > register would not have separate enables.
> >
>
> Rick -
>
> This looks fine to me. It's easy to read and will function correctly, plus
> the case statement and use of parameters make it easy to modify. Given what
> I've heard of your relationship with the FPGA group, I would not flag this
> if I were you. It would be nitpicking.

I never said I was going to "flag" it.  I was just asking for some help
understanding how Verilog worked.  If it were VHDL I would know for
certain how the synthesizer would handle it.  But I am pretty confident
that this will be less efficient than a properly constructed
description.

That is how I code differently from a lot of people.  Many designers
"program" in the HDL.  I design my hardware in block diagrams and use
the language to describe my hardware.  The above code certainly
functions correctly, but it would be just as clear, if not more so to
separate the logic and the register.  A register description is very,
very simple and small.  These can be put after the logic or they can be
lumped together in a register bunch at the end of the module.

The above program coded as logic would have a separate assignment for
each signal.  These assignments would not have any unspecified
conditions and so would implement very efficiently and of course no
latches.  Expand the above code to 12 address lines and 25 signals and
you will see how inefficient this could get.

John_H wrote:
> "rickman" <spamgoeshere4@yahoo.com> wrote in message
> news:1154101628.534694.304340@s13g2000cwa.googlegroups.com...
>
> > Two FFs and four logic elements (x15 in the real circuit).  You have to
> > use a LUT to drive the D input and you have to use a LUT to drive the
> > enable input.  Of course you can feedback the output to the input and
> > not use the clock enable, but that will also use more LUTs depending on
> > if you have the extra inputs or not on the LUTs you are using.  But
> > this will be up to the tool and many times I see the tool generating
> > logic to feed the clock enable.
> >
> > It is not a big deal.  I tend to think of things like this when I code
> > in an HDL just so I know my logic generation is lean.  Originally I had
> > not seen the enclosing always statement and was thinking it was
> > generating a latch.  Once I started looking at it I realize the latch
> > was not an issue but thought about the extra logic being generated.  I
> > tend to separate my registers from my logic just to allow me to
> > optimize this sort of thing.
>
> You're taking the code structure as a literal guide for synthesis.  There
> will be two FFs and two LUTs without a clock enable.  Synthesis typically
> knows what simple logic breaks down to and how to best implement it.  If you
> go into the technology view of your synthesizer to look at one of the flops,
> expect to see only one LUT driving it, no clock enable involved.

With this simple example you are right, it will require a single
4-input LUTs even if you don't use the CE.  The logic functions only
has four inputs and so can be done in a single 4-input LUT.  They are
wr, the two address bits and the feedback from the given register.  But
if there is one more address line the feedback will push it to a pair
of LUTs.  Like I said, it is not a huge difference, but with the large
number of signals in the real code, this could have been done very
slightly different and would have been optimal.

I realized that a simple change would not be any more typing and would
fully specify the assignments to eliminate the need for the feedback
signal.  At least if this works like VHDL it would work correctly.
Like I said, I am not up to speed in Verilog anymore.

always @ (negedge rst_n or posedge clk) begin
  if (!rst_n)begin
    mask_wr      <= 1'b0;
    clear_wr      <= 1'b0;
    end
  else begin
    mask_wr      <= 1'b0;
    clear_wr      <= 1'b0;
    if (wr == 1'b1)begin
      case (adr [2:1])
        MASK:     mask_wr      <= 1'b1;
        CLEAR:   clear_wr      <= 1'b1;
        endcase
      end
  end
end

Rather than specify the default condition in the ELSE of the if
condition, it is specified first as the default.  Then any signal that
is not assigned in the case statement will still be defined.  The above
is the intended function rather than holding the previous state of the
signal.  But then like I said in another post, I don't code behavior
and let the tools determine the logic, I describe logic which has the
behavior I want.

On Fri, 28 Jul 2006 08:04:46 -0700, rickman wrote:

> While reviewing the code that the FPGA group has produced, I saw
> something that looks bad.  It is not likely to affect the
> functionality, but it is not good coding style and may use extra
> resources.
> 
> They are using Verilog which is not my first HDL language and I am not
> as familiar with it as I am VHDL.  But because the case statement is
> not fully specified the code below appears to me to produce more
> complex logic than needed.
> 
> always @ (negedge rst_n or posedge clk) begin
>   if (!rst_n)begin
>     mask_wr      <= 1'b0;
>     clear_wr      <= 1'b0;
>     end
>   else begin
>     if (wr == 1'b1)begin
>       case (adr [2:1])
>         MASK:     mask_wr      <= 1'b1;
>         CLEAR:   clear_wr      <= 1'b1;
>         endcase
>       end
>     else begin
>       mask_wr      <= 1'b0;
>       clear_wr      <= 1'b0;
>     end
>   end
> end
> 
> The real code is controlling 20 or so signals and is very large so this
> is very simplified.  At first I didn't notice the enclosing "always"
> statement and thought it would produce a latch.  Now I realize that it
> will make enabled registers when what is really desired is just
> decoders followed by registers.  I guess in the grand scheme of things
> this is not a big issue.  But wouldn't it make a more streamlined
> result to code the logic separate from the register?  Then the logic
> would just be decodes of the address bus and the control signal and the
> register would not have separate enables.

The only problem with that code is the lack of a default statement.
Without a default statement the synthesizer will screw it up because the
undefined cases won't be treated as do nothing. If you add a default
statement then the synthesizer will do the right thing.