Parameterized Verilog shift register code

Question

I am a beginner in FPGAs, and I am studying Verilog HDL. Could you please check the quality of my code (a shift register)? Any comments are welcome.

The shift register serializes parallel from wr_data_i data to serial_data_o and has two variations of first serialized bit. It may be most significant bit or least significant bit, which is defined in parameters as TRUE or FALSE.

`timescale 1ns / 1ps
module shift_reg # 
(
 parameter integer DATA_WIDTH = 16,
 parameter integer DO_MSB_FIRST = "TRUE"
)
(
 input wire clk_i,
 input wire s_rst_n_i,
 input wire enable,
 input wire wr_enable,
 
 input wire [DATA_WIDTH - 1 : 0] wr_data_i,
 output wire serial_data_o
);
 localparam integer MSB = DATA_WIDTH - 1;
 localparam integer LSB = 0;
 localparam integer XSB = ("TRUE" == DO_MSB_FIRST) ? MSB : LSB;
 reg [DATA_WIDTH - 1 : 0] parallel_data;
 assign serial_data_o = (1'h1 == enable) ? parallel_data[XSB] : 1'hz;
 
 always @ (posedge clk_i)
 begin
 if (1'h0 == s_rst_n_i)
 begin
 parallel_data <= 'h0;
 end
 else if (1'h1 == wr_enable)
 begin
 parallel_data <= wr_data_i;
 end
 else if (1'h1 == enable)
 begin
 if ("TRUE" == DO_MSB_FIRST)
 begin
 parallel_data <= {parallel_data[MSB - 1 : 0], 1'h0};
 end
 else
 begin
 parallel_data <= {1'h0, parallel_data[MSB: 1]};
 end 
 end
 end
endmodule

Answer 1

I don't see any problems with the code functionally, and the layout follows good practices for the most part. You chose meaningful names for the signals and parameters, and the parameters make your design scalable to different data widths. It also follows good practices using nonblocking assignments for the sequential logic. This is excellent for a beginner.

You should use a sized constant for the data reset value ('h0) in order to avoid synthesis lint warnings from some tools. I prefer the replicated concatenation style for this: {DATA_WIDTH{1'h0}}.

I think it is more conventional to omit the 1'h1 == in comparisons to 1 for 1-bit signals. It makes the code a little cleaner, and all the tools do the right thing. For example: (wr_enable).

Similarly for comparisons to 0, omit the 1'h0 ==, and use either ! or ~ (they are equivalent for 1-bit signals): (!s_rst_n_i)

You did a fantastic job of using a consistent layout style. The following comments are my preferences, but you might consider using them as well.

I use 4-space indentation because I think it is easier to see the different levels of your code's logic.

I prefer to place the begin and end keywords on the same line as the if/else keywords in order to reduce vertical space and get more code on one screen. It also makes the branches of your code more evident.

Here is the code with the above suggestions:

`timescale 1ns / 1ps
module shift_reg # 
(
 parameter integer DATA_WIDTH = 16,
 parameter integer DO_MSB_FIRST = "TRUE"
)
(
 input wire clk_i,
 input wire s_rst_n_i,
 input wire enable,
 input wire wr_enable,
 input wire [DATA_WIDTH - 1 : 0] wr_data_i,
 output wire serial_data_o
);
 localparam integer MSB = DATA_WIDTH - 1;
 localparam integer LSB = 0;
 localparam integer XSB = ("TRUE" == DO_MSB_FIRST) ? MSB : LSB;
 reg [DATA_WIDTH - 1 : 0] parallel_data;
 assign serial_data_o = (enable) ? parallel_data[XSB] : 1'hz;
 
 always @ (posedge clk_i) begin
 if (!s_rst_n_i) begin
 parallel_data <= {DATA_WIDTH{1'h0}};
 end else if (wr_enable) begin
 parallel_data <= wr_data_i;
 end else if (enable) begin
 if ("TRUE" == DO_MSB_FIRST) begin
 parallel_data <= {parallel_data[MSB - 1 : 0], 1'h0};
 end else begin
 parallel_data <= {1'h0, parallel_data[MSB : 1]};
 end 
 end
 end
endmodule