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