One option to produce code with less lines would be this :

always @(posedge clk or negedge rst_n) begin
        mask_wr <= !rst_n ? 1'b0 : (adr[2:1] == MASK) ;
        clear_wr  <= !rst_n ? 1'b0 : (adr[2:1] == CLEAR) ;
end


This way you reduce 16 lines of code to 4

-Deepak Lala


rickman wrote:
> 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.

rickman wrote:


> Actually, I just looked a bit harder at your code and I don't
> understand the notation
>    mask_wr <= #1 mask_wr_in;
> Without looking in one of my Verilog books, what is the "#1" for?

Delay by 1 time step. (The step size is defined with the 'timescale 
directive.)

Although Verilog does not offer delta delays like VHDL it is possible to 
get almost the same behavior using blocked signal assignments (the "<=" 
operator). And then there is no need for manually written delays. 
Furthermore it is dangerous, because one can easily code a behavior that 
would never happen in functional simulation (Verilog has an event queue) 
nor after synthesis.
IMHO it is much better to write good Verilog code than using these 
delays. See Cliff Cummings "Verilog Coding styles that kill" - which 
describes Verilog coding very similar to VHDL code.

Ralf

Yep, that is pretty much what I would have written.  I think this would
be the simplest and clearest description of the circuit that is
desired.  But after pushing it around a bit, I did come up with an
always statement (shown later in this thread) that was about the same
amount of code and could be said to be pretty much as clear as your
code.  At that point it just becomes a matter of preference.

Actually, I just looked a bit harder at your code and I don't
understand the notation
   mask_wr <= #1 mask_wr_in;
Without looking in one of my Verilog books, what is the "#1" for?

luiguo wrote:
> Coding style like this?
>
> wire mask_wr_in = (adr[2:1] == MASK) & wr;
> wire clear_wr_in = (adr[2:1] == CLEAR) & wr;
>
> always @(posedge clk or negedge rst_n)
> if(~rst_n) begin
>    mask_wr <= #1 1'b0;
>    clear_wr<= #1 1'b0;
> end
> else begin
>    mask_wr <= #1 mask_wr_in;
>    clear_wr <= #1 clear_wr_in;
> end
>
> At Fri, 28 Jul 2006 16:34:46 -0700,rickman wrote:
>
> > 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

Coding style like this?

wire mask_wr_in = (adr[2:1] == MASK) & wr;
wire clear_wr_in = (adr[2:1] == CLEAR) & wr;

always @(posedge clk or negedge rst_n)
if(~rst_n) begin
   mask_wr <= #1 1'b0;
   clear_wr<= #1 1'b0;
end
else begin
   mask_wr <= #1 mask_wr_in;
   clear_wr <= #1 clear_wr_in;
end

At Fri, 28 Jul 2006 16:34:46 -0700&#4294967295;&#4294967295;rickman wrote&#4294967295;&#4294967295;

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

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
> >     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
>
> But this is different to the initially posted code example! In the
> inital example both FFs hold their old values in some circumstances
> while now everytime a value is assigned to their inputs. Therefore this
> is a functional simplification which can only be done if the resulting
> behavior is acceptable.
>
> If acceptable it seems to me to be a good way to code this.

That was the point.  The initial code was not coded correctly for the
intended behavior.  It only worked in the application (so far) because
the address did not change.  This is just a set of address decodes
feeding into a register.  It is intended to be clocked on every clock
cycle.  In fact if I gave it a bit of thought, I would remove the
enclosing IF and make the signal assignments from the WR signal...

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;
    case (adr [2:1])
      MASK:    mask_wr     <= wr;
      CLEAR:   clear_wr    <= wr;
      endcase
  end
end

Now the code is doing exactly what it should be doing, anding the
addresses signals with the wr to produce a decode which is then
registered without an enable.  This is totally clear and will produce
the optimally minimum logic.  Another advantage is that you can easily
combine the read and the write strobes in the same block if the design
were small enough to make that useful.  There are many registers in
this design so it would produce too large of a list to combine them.

On Sat, 29 Jul 2006 04:11:30 +0000, RobJ wrote:

> "Josh Rosen" <bjrosen@polybusPleaseDontSPAMme.com> wrote in message 
> news:pan.2006.07.29.01.04.03.95311@polybusPleaseDontSPAMme.com...
>>
>> 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.
>>
> 
> Gotta disagree, Josh. In a clocked process it is understood (and properly 
> implemented by synthesis tools) that for undefined cases the last state of 
> each register is held, which is what you want. You only need to explicitly 
> code transitions. As long as the registers are initialized, which they are 
> here by an async reset, then you're good to go. Even that is mainly a 
> simulation issue. A synthesis tool can even handle no initialization, but 
> you have to know what you're getting. In a non-clocked process you must spec 
> defaults (either before the case statement or inside the case statement), or 
> latches are inferred. In my opinion there is absolutely nothing wrong the 
> code block Rickman posted other than asthetics (I don't care for how the 
> begin/ends are indented).
> 
> Rob

I can tell you from first hand experience that the current version
(8.2sp1) of XST mishandles case statements that don't have defaults, I
just ran into the problem. I've seen this happen before with other
synthesizers. It's just good practice to include a default in a case
statement. That default can be empty but it's presence tell the
synthesizer that the states don't change for the un-specified cases.

RobJ wrote:
> "Josh Rosen" <bjrosen@polybusPleaseDontSPAMme.com> wrote in message
> news:pan.2006.07.29.01.04.03.95311@polybusPleaseDontSPAMme.com...
> >
> > 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.
> >
>
> Gotta disagree, Josh. In a clocked process it is understood (and properly
> implemented by synthesis tools) that for undefined cases the last state of
> each register is held, which is what you want. You only need to explicitly
> code transitions. As long as the registers are initialized, which they are
> here by an async reset, then you're good to go. Even that is mainly a
> simulation issue. A synthesis tool can even handle no initialization, but
> you have to know what you're getting. In a non-clocked process you must spec
> defaults (either before the case statement or inside the case statement), or
> latches are inferred. In my opinion there is absolutely nothing wrong the
> code block Rickman posted other than asthetics (I don't care for how the
> begin/ends are indented).

Ah, but the unspecified cases are where the code is not doing what was
intended.  In the code posted, the previous value will be held.
Instead the intent was that the value should always be 0 except for the
decodes specified in the case statement.  It was just luck that the
input conditions never create a failure.  As long as the address
remains constant while the wr strobe is asserted, it will not have to
"remember" a value.  That is why I said this would not work so well for
the read decodes.

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

But this is different to the initially posted code example! In the 
inital example both FFs hold their old values in some circumstances 
while now everytime a value is assigned to their inputs. Therefore this 
is a functional simplification which can only be done if the resulting 
behavior is acceptable.

If acceptable it seems to me to be a good way to code this.


Ralf

"Josh Rosen" <bjrosen@polybusPleaseDontSPAMme.com> wrote in message 
news:pan.2006.07.29.01.04.03.95311@polybusPleaseDontSPAMme.com...
>
> 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.
>

Gotta disagree, Josh. In a clocked process it is understood (and properly 
implemented by synthesis tools) that for undefined cases the last state of 
each register is held, which is what you want. You only need to explicitly 
code transitions. As long as the registers are initialized, which they are 
here by an async reset, then you're good to go. Even that is mainly a 
simulation issue. A synthesis tool can even handle no initialization, but 
you have to know what you're getting. In a non-clocked process you must spec 
defaults (either before the case statement or inside the case statement), or 
latches are inferred. In my opinion there is absolutely nothing wrong the 
code block Rickman posted other than asthetics (I don't care for how the 
begin/ends are indented).

Rob 

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.