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