Help with good verilog practices

[Sink0]

Hi, i have written the following verilog code. Its basically a WB master to read and write from OC PCI bridge core. Can you guys point me what is "ugly" on my code, or what is a bad practice? I just want to learn what is a good practice programming with verilog, and why.

`include "network_controller_constants.v"

module NETWORK_CONTROLLER_WB_MASTER
(
	CLK_I,            
   RST_I,

	MINT_O,
	MADR_O,
	MDAT_O,
	MDAT_I,
	MSEL_O,
	MCYC_O,
	MSTB_O,
	MWE_O,
	MCAB_O,
	MACK_I,
	MRTY_I,
   MERR_I,
	 
	tx_b1_offset,
	tx_b2_offset,
	rx_b1_offset,
	rx_b2_offset,
	
	r_cnt_reg,
	r_cmnd_flag,
	
	tx_b_1_int,
	tx_b_2_int,
	rx_b_1_int,
	rx_b_2_int,
	
	tx_b_1_over,
	tx_b_2_over,
	rx_b_1_over,
	rx_b_2_over,
	
	r_counter_empty,
	counter_loaded,
	
	w_cmnd_flag,
	w_counter_loaded,
	w_cnt_reg,
	
	leds,
	
	wb_reading,
	wb_r_data
);


input			   	CLK_I;            
input             RST_I;

output				MINT_O;
output   [31:0]  	MADR_O;
input    [31:0]  	MDAT_I;
output   [31:0]  	MDAT_O;
output   [3:0]   	MSEL_O;
output            MCYC_O;
output            MSTB_O;
output            MWE_O;
output            MCAB_O;
input            	MACK_I;
input            	MRTY_I;
input            	MERR_I;

input		[31:0]	tx_b1_offset;
input		[31:0]	tx_b2_offset;
input		[31:0]	rx_b1_offset;
input		[31:0]	rx_b2_offset;

output				tx_b_1_over;
output				tx_b_2_over;
output				rx_b_1_over;
output				rx_b_2_over;

input		[31:0]	r_cnt_reg;
input					r_cmnd_flag;

input					tx_b_1_int;
input					tx_b_2_int;
input					rx_b_1_int;
input					rx_b_2_int;

output				r_counter_empty;

output	[31:0]	wb_r_data;
output				wb_reading;

input					w_cmnd_flag;
output				w_counter_loaded;
input		[31:0]	w_cnt_reg;

output	[3:0]		leds;

output				counter_loaded;

parameter WB_IDLE	= 	  4'b0001;	
parameter WB_WRITING	= 4'b0010;
parameter WB_READING	= 4'b0100;
parameter WB_W_WAIT	= 4'b1000;

reg		[31:0]	MADR_O = 32'h00000000;
wire		[31:0]	MDAT_O;
wire		[31:0]	MDAT_I;
wire		[3:0]		MSEL_O;
reg					MCYC_O = 0;
reg					MSTB_O = 0;
wire					MWE_O;
reg					MCAB_O;
wire					MACK_I;
wire					MRTY_I;
wire					MINT_O;

reg		[31:0]	r_counter = 0;
reg		[3:0]		state_machine = WB_IDLE;
reg		[3:0]		next_state = WB_IDLE;
reg					write_done = 0;
wire					r_counter_empty;
wire					wb_int_gen;


reg					tx_b_1_over = 0;
reg					tx_b_2_over = 0;
reg					rx_b_1_over = 0;
reg					rx_b_2_over = 0;

reg		[31:0]	w_counter = 0;
wire					w_counter_empty;

wire		[31:0]	wb_r_data;
wire					wb_reading;

reg		[30:0]	r_w_addr = 0;

//###########################################################
//							DEBUG VARIABLES
reg 		[3:0]		MRTY_C = 0;
reg		[3:0]		MACK_C = 0;	
reg		[3:0]		MINT_C = 0;
reg 		[3:0]		WRITE_C = 0;
reg		[3:0]		leds;	

//###########################################################	

assign MSEL_O = 4'b1111;
assign MINT_O = wb_int_gen;

assign wb_int_gen = 		tx_b_1_int||
								tx_b_2_int||
								rx_b_1_int||
								rx_b_2_int;

/*##################################################################################
############################ state_machine CONTROL #################################
##################################################################################*/

always@(*)
begin
	tx_b_1_over = 1'b0;
	tx_b_2_over = 1'b0;
	rx_b_1_over = 1'b0;
	rx_b_2_over = 1'b0;
	next_state = state_machine;
	case (state_machine)
		WB_IDLE : 
			begin
				if(r_counter > 32'h0)
				begin
						next_state = WB_READING;
				end
				else
				begin
					if(w_counter > 32'h0)
					begin
						next_state = WB_WRITING;
					end
				end
			end
		WB_READING :
			begin
				if(r_counter == 32'h0)
				begin
					next_state = WB_IDLE;
					rx_b_1_over = 1'b1;
				end
			end
		WB_WRITING :
		begin
				if(w_counter == 32'h0)
					next_state = WB_W_WAIT;
		end
		WB_W_WAIT :
			begin
				if(write_done)
				begin
					next_state = WB_IDLE;
					tx_b_1_over = 1'b1;
				end
			end
		default:begin
				next_state = WB_IDLE ; 
            end 
	endcase
end


/*##################################################################################
############################# write_done CONTROL ###################################
##################################################################################*/

always@(posedge CLK_I)
begin
	write_done = 1'b0;
	if(state_machine == WB_W_WAIT)
	begin
		if(MCYC_O && MACK_I)
			write_done = 1'b1;
	end
end


/*##################################################################################
############################ state_machine TIMING ##################################
##################################################################################*/

always@(posedge CLK_I)
begin
		state_machine <= next_state;
end


always@(posedge CLK_I)
begin
		case (state_machine)
			WB_IDLE : begin
				if(r_cmnd_flag)
				begin
					r_counter <= r_cnt_reg;
					r_w_addr <= 30'h0;
				end
				else
				begin
				if(w_cmnd_flag && (~tx_b_1_int || ~tx_b_2_int))
				begin
					w_counter <= w_cnt_reg;
					r_w_addr <= 30'h0;
				end
				end
			end
			WB_READING : begin
				if(MCYC_O && MACK_I)
				begin
					if(r_counter > 0)
					begin
						r_counter <= r_counter - 32'h1;
						r_w_addr <= r_w_addr + 30'h4;
					end
				end
			end
			WB_WRITING : begin
				if(MCYC_O && MACK_I)
				begin
					if(w_counter > 0)
					begin
						w_counter <= w_counter - 32'h1;
						r_w_addr <= r_w_addr + 30'h4;
					end
				end
			end
		endcase
end

assign MDAT_O = w_counter;
assign MWE_O = (next_state == WB_WRITING)? 1'b1 : 1'b0;
assign wb_r_data = MDAT_I;

always@(*)
begin
	case(next_state)
		WB_IDLE: begin
			MADR_O[30:0] <= r_w_addr + tx_b1_offset[30:0];
			MADR_O[31] <= 1'b0;
			MCAB_O <= 1;
			end
		WB_READING: begin
			MADR_O[30:0] <= r_w_addr + tx_b1_offset[30:0];
			MADR_O[31] <= 1'b0;
			MCAB_O <= 1;
			end
		WB_WRITING: begin
			MADR_O[30:0] <= r_w_addr + rx_b1_offset[30:0];
			MADR_O[31] <= 1'b1;
			MCAB_O <= 1;
			end
		WB_W_WAIT: begin
			MADR_O[30:0] <= tx_b1_offset[30:0] + `DUMMY_READ_ADDR;
			MADR_O[31] <= 1'b0;
			MCAB_O <= 0;
			end
		default: begin
			MADR_O[31:0] <= 0;
			MCAB_O <= 1;
			end
	endcase
end
 
always@(*)
begin
	leds[0] <= state_machine[0];
	leds[1] <= state_machine[1];
	leds[2] <= MINT_O;
	
	if(next_state == WB_IDLE || next_state != state_machine)
	begin
		MSTB_O <= 1'b0;
		MCYC_O <= 1'b0;
	end
	else 
	begin
		MSTB_O <= 1'b1;
		MCYC_O <= 1'b1;
	end
end

assign wb_reading		  = (state_machine == WB_READING) ? 1'b1 : 1'b0;
assign r_counter_empty = (r_counter > 0)? 1'b0 : 1'b1;
assign w_counter_empty = (w_counter > 0)? 1'b0 : 1'b1;

pulse_gen read_ld_pulse
(
	.Trigger				(r_counter_empty),
	.Pulse_Out			(counter_loaded),
	.Clock				(CLK_I)
);

pulse_gen write_ld_pulse
(
	.Trigger				(w_counter_empty),
	.Pulse_Out			(w_counter_loaded),
	.Clock				(CLK_I)
);

endmodule

Thank you!!

 

[mrflibble]

Well, for one ... the indentation as it is makes it a bit unpleasant to read.

Another thing to reduce chances of implicit nets due to typos or whatever...

Start the file with

`default_nettype none

And end the file with

`default_nettype wire

So something like this:

`default_nettype none

module NETWORK_CONTROLLER_WB_MASTER (
    input  wire CLK_I,
    input  wire RST_I,
    output wire MINT_O,
...
    );

...

endmodule // NETWORK_CONTROLLER_WB_MASTER
`default_nettype none

Did I mention using spaces for indentation instead of tabs? One of many reasons being that spaces survive cut & paste into other environments. Such as a forum. With tabs it's always hit and miss...

 

[kulkarni_saurabh]

Nice suggestion by mrflibble.

A few more suggestions:

1. Restrict the use of all-uppercase names to constants and parameters. That will reduce ambiguity from the code and constants/parameters become readily identifiable.

So you could do:

module network_controller_wb_master(
    input  wire clk_i,
    input  wire rst_i,
    output wire mint_o,
...
    );

...

2. Use Verilog-2001 style of port-list (direction + port name) instead of name and direction separately as is in Verilog-1995. It creates more compact and readable code. It has already been done in the above example by mrflibble, but just wish to state it explicitly.

3. Initialize registers at reset. This will synthesize registers with known initial state making the design more robust and deterministic.

So

always@(posedge CLK_I)
begin
		state_machine <= next_state;
end

becomes

always@(posedge CLK_I or posedge rst_i) begin
  if(rst_i) begin
    state_machine <= 'd0;
  end else begin
    state_machine <= next_state;
  end
end

3. Try to employ clock-gating to reduce power. I don't know your code well enough, but the most trivial case would be:

always@(posedge CLK_I or posedge rst_i) begin
  if(rst_i) begin
    state_machine <= 'd0;
  end else begin
    if(state_machine ^ next_state) state_machine <= next_state; //update state only when current_state and next_state are different
  end
end

4. Try to reduce levels of if. It will help the synthesis tool to better clock-gate the register.

always@(posedge CLK_I)
begin
	write_done = 1'b0;
	if(state_machine == WB_W_WAIT)
	begin
		if(MCYC_O && MACK_I)
			write_done = 1'b1;
	end
end

becomes

always@(posedge CLK_I or posedge rst_i) begin
  if(rst_i) begin
    write_done = 1'b0;
  end else if((state_machine == WB_W_WAIT) && MCYC_O && MACK_I) begin
    write_done = 1'b1;
  end else if(write_done) begin
    //Added to match the stated functionality of asserting write_done only as long as 
    //(state_machine == WB_W_WAIT) && MCYC_O && MACK_I condition is true
    write_done = 1'b0;
  end
end

5. Use shorthand notations.
So

assign wb_reading		  = (state_machine == WB_READING) ? 1'b1 : 1'b0;
assign r_counter_empty = (r_counter > 0)? 1'b0 : 1'b1;
assign w_counter_empty = (w_counter > 0)? 1'b0 : 1'b1;

becomes

assign wb_reading = (state_machine == WB_READING);
assign r_counter_empty = (r_counter == 0); //Could have also used ~|r_counter
assign w_counter_empty = (w_counter == 0); //Could have also used ~|w_counter

Not to be nit-picking on your code. But these practices will go a long way in creating robust designs and aid quick debug of your code. :wink:

Hope this helps.

All the best!

Regards,
Saurabh