Verilog assignment - scheme, code! Optimization suggestions?

[kostanatuka]

Hello!
Don't look at the scheme as it has to have a meaning! It's an assignment to practice what I've learned about verilog for the past week. The code as it is, is working and the test bench is showing the results that I expect. I'm looking for optimization suggestions. Any help will be appreciated!

Scheme:


Code:
valid module:

module valid(
button,
clk,
rstn,
valid_in
    );
	 
input button, clk, rstn;
output valid_in;
reg q1,q2;

assign valid_in = q1 & ~q2;
always @ (posedge clk or negedge rstn)
 begin
  if(!rstn)
   begin
    q1 <= 1'b0;
	 q2 <= 1'b0;
	end
  else
   begin
	 q1 <= button;
	 q2 <= q1;
	end
 end
	
endmodule

counter module:

module counter8(
valid_in,
clk,
rstn,
valid1,
data_8,
stop
    );
	 
input valid_in, clk, rstn, stop;
output valid1, data_8;
wire [7:0] data_8;
reg [7:0] tmpdata_8;
reg valid1;

always @ (posedge clk or negedge rstn)
 begin
  if(!rstn)
   begin
    tmpdata_8 <= 8'b00000000;
	 valid1 <= 1'b0;
	end	
  else
   begin
	 valid1 <= valid_in & ~stop;
	 tmpdata_8 <= tmpdata_8 + 1'b1;
	end
 end
assign data_8 = tmpdata_8;
 
endmodule

shiftreg module:

module shiftreg(
clk,
rstn,
data_8,
valid1,
data_32,
valid_fifo
    );

input valid1, clk, rstn;
input [7:0] data_8;
output wire [31:0] data_32;
output valid_fifo;
reg [31:0] tmp;
reg [1:0] fill;
reg [1:0] go;

assign valid_fifo = &go & fill[1] & ~fill[0];

always @ (posedge clk or negedge rstn)
 if (!rstn)
  fill <= 0;
  else
   fill <= go;
   

always @ (posedge clk or negedge rstn)
 begin
  if(!rstn)
   begin
	 tmp <= 0;
	 
	 go <= 4'b0;
	end
  else 
   if(valid1)
	 begin
	  tmp <= {tmp,data_8};
	  go <= go + 1'b1;
     	  
	 end	 
 end
assign data_32 = tmp;

endmodule

mem driver module:

module mem_fifo(
clk,
rstn,
data_32,
data,
addr,
valid_fifo,
stop,
mem_full,
wr
    );
	 
input clk, rstn, valid_fifo;
input [31:0] data_32;
output stop, mem_full;
output reg [31:0] data;
output reg [6:0] addr;
output reg wr;
reg [2:0] rd_ptr, wr_ptr;
reg [31:0] mem [0:7];
reg [2:0] counter;

assign mem_full = (addr > 63) ? 1 : 0;
assign stop = mem_full;

always @ (posedge clk, negedge rstn)
 begin
  if(!rstn)
   begin
	 rd_ptr <= 0;
	 wr_ptr <= 0;
	 counter <= 0;
	 addr <= 0;
	 data <= 0;
	end
	else if(counter!=7  && valid_fifo)
	 begin
	 mem[wr_ptr] <= data_32;
	 wr_ptr <= wr_ptr + 1;
	 counter <= counter + 1;
	 wr <= 0;
	 end
	 else if(counter!=0 && addr != 64 )
	 begin
	  data <= mem[rd_ptr];
	  rd_ptr <= rd_ptr + 1;
	  counter <= counter - 1;
	  addr <= addr + 1;
	  wr <= 1;
	 end
	end

endmodule

mem module:

module mem(
data,
wr,
addr,
clk,
out
    );
	 
input wr,clk;
input [5:0] addr;
input [31:0] data;
output reg [31:0] out;

reg [31:0] mem[0:63];

always @ (posedge clk) 
 if (wr)
  begin
   mem[addr] <= data;   	
  end
  
always @ (posedge clk) 
 if (!wr)
  begin
   out <= mem[addr];
  end

endmodule

 

[mrflibble]

One suggestion would be to optimize user laziness. Laziness is good, laziness is progress!

If you are allowed (this being an assignment), I'd use some verilog 2001 syntax. That makes your port definitions more readible, and you don't have to repeat yourself in the module body.

I'll use your shiftreg module as an example:

---

Code C - [expand]
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
`default_nettype none
 
module shiftreg(
    input  wire        clk,
    input  wire        rstn,
    input  wire  [7:0] data_8,
    input  wire        valid1,
    output reg  [31:0] data_32 = 0,
    output wire        valid_fifo
    );
 
 
reg [1:0] fill;
reg [1:0] go;
 
assign valid_fifo = &go & fill[1] & ~fill[0];
 
always @ (posedge clk or negedge rstn)
 if (!rstn)
  fill <= 0;
  else
   fill <= go;
   
 
always @(posedge clk or negedge rstn)
 begin
  if(!rstn)
   begin
     data_32 <= 0;
     
     go <= 4'b0;
    end
  else 
   if(valid1)
     begin
      data_32 <= {data_32[23:0],data_8}; // <== Yo, here! Better to be explicit with your bit vector width, and make sure source and target width match.
      go <= go + 1'b1;
          
     end     
 end
 
endmodule
 
`default_nettype wire

---

Notice how you get rid of having to repeat signals in the body that you have already specified in the module port list.

Also notice how you can get rid of temporary registers like "tmp", since you can put registers in the port list. I personally like that a lot, since just by looking at the port list I can see which outputs are registered, and which ones are not.

For example, now I can see that "data_32" is properly registered. And that for whatever design reason valid_fifo is a combinatorial (i.e unregistered) output. This could be considered bad practice by some. Depends a bit on the design. In this particular case, you may want to consider registering the "valid_fifo" output. And in the spirit of optimizations, registering signals like that between modules can help you meet your timing constraints. As in, your design will probably run faster.

And also note the use of indentation in the port list to make things more readible.

As an example notice that you can initialize registers with a default value. The question if this is useful or not is another matter entirely. I'd just thought I'd show you that it can be done. Mainly (for me) it would be a matter of target. If this module for simulation or for synthesis. And after that it becomes a matter of, synthesize for fpga or asic.

In the case of asic you don't want those registers initialized, since there is no hardware equivalent for that.

In the case of an fpga it can certainly make sense, since there IS a hardware equivalent for that. And that being that on configuration the fpga registers can be set to an initial (reset) value. This uses the global reset signal of the fpga, and that way you don't NEED to use precious routing for resets that you only intend to apply at powerup.

Oh yeah, and a final tip is to use "`default_nettype none". This will prevent stupid typo related error. Without this, the default_nettype will be wire. So if you make a typo somewhere and type the wrong signal name, the synthesizer will just assume this is correct and will apply the default nettype (wire) to it. And the rest of the logic will probably compile (and not work). And you will spend lots of time tracing down what the hell went wrong.

Now if you use default_nettype none, verilog will still be stupid like before. You make a typo in your signal name. The synthesizer will still assume this is correct. And it will still appy the default nettype to it. Only this time we made sure that the DEFAULT is value "none". So now you have a mistyped signal with type none. And now the logic you tried to do on nettype "none" will fail horribly, throwing a nice error regarding this typo-ed signal.

Also note how at the end of the module I restore the default nettype to the default default (wire). That way when you have other modules that are less strict (from other people, etc), these will still work. Just your own modules will now be somewhat more robust to human error.

Looks small, but I can tell you that this is the biggest timesaver of all the above. At least for me it is...


Quick links on verilog 2001:
Whats New in Verilog 2001 Part-I
https://www.sutherland-hdl.com/papers/2001-Wescon-tutorial_using_Verilog-2001_part1.pdf
**broken link removed**

The asic-world + sutherland papers have all sorts of useful other info as well.

Hope that helps!

---------- Post added at 17:31 ---------- Previous post was at 17:09 ----------

Additional:

Should you decide to use fpga global resets to take care of resetting the flipflops to a know state at startup (configuration)m then you could do something like this:

---

Code C - [expand]
1
2
3
4
5
6
7
reg [1:0] go = 0;
 
//
// This is now a lot shorter, without the explicit reset stuff. 
//
always @ (posedge clk)
  fill <= go;

---

Again, if you want to do this or not depends. For HDL that is targeting fpga's only, it makes sense IMO.

The above works just fine for the xilinx targets I use this for. I would expect similar behaviour for Altera. Best read the documentation to be sure...

 

[dcreddy1980]

Expect that synthesis tool might do optimization for the below code :

assign mem_full = (addr > 63) ? 1 : 0;

But the above can be rewritten as :

assign mem_full = addr[6];

If "addr" is 64 and beyond, it will be 1. It can 64 and beyond if and only addr[6] is 1.

 

[kostanatuka]

Thanks for the tips. I'll try to get used to and implement them to the assignment! Tomorrow I'll have the opportunity to test the code on a FPGA and see the end result.