Simple GPIO design module in SystemVerilog

Question

I am designing a very crude general-purpose input/output (GPIO) module to provide IO pin control to a RISC-V (like) architecture microprocessor I am currently playing with.

This is the code so far:

`timescale 1ns / 1ps

module gpio(
    input clk_i,
    input rst_ni,
    input we_i,
    input [7:0] wdata_i,
    input [7:0] addr_i,
    inout [7:0] port_io,
    output [7:0] rdata_o
    );
    
logic [7:0] io_dir;
logic [7:0] io_val;
logic [7:0] io_int;

integer i;

always_ff @(posedge clk_i, negedge rst_ni) begin
    if(rst_ni==0) begin
        for(i=0; i<8; i=i+1)
            io_dir <= 0;
    end
    else begin
        if(addr_i==0 && we_i==1)
            io_dir <= wdata_i;
    end
end

always_ff @(posedge clk_i, negedge rst_ni) begin
    if(rst_ni==0) begin
        for(i=0; i<8; i=i+1)
            io_val <= 0;
    end
    else begin
        for(i=0; i<8; i=i+1) begin
            if(io_dir[i]==0 && addr_i==1 && we_i==1)
                io_val[i] <= wdata_i[i];
            else
                io_val[i] <= port_io[i];
        end
    end
end

always_comb begin
    for(i=0; i<8; i=i+1) begin
        io_int[i] <= (io_dir[i]==0)? io_val[i] : 1'bz;
    end
end

assign port_io = io_int;
assign rdata_o = io_val;
endmodule

The code seems to be working as expected. The purpose of this question is to get advice on the quality of my code and what to improve.

Answer 1

Generally speaking, the code has consistent layout, is easy to understand and uses descriptive variable names.

You have a good separation between sequential logic and combinational logic, and you take advantage of the additional automatic code check offered by the SystemVerilog always_ff and always_comb keywords. These keywords also convey design intent well.

However, I get compile errors with 2 different simulators due to the integer i declaration. Perhaps your simulator is more forgiving. Regardless, to make your code more portable, I recommend declaring the iterator variable locally within each for loop.

You can easily make the design to be more scalable by using a parameter for the number of I/O signals, instead of using hard-coded numbers (7 and 8). This makes it easier to change the design if you need 16 I/O's, for example. You could use a parameter named NUM and set it to 8:

parameter NUM = 8;

You can also set the parameter to a different value when you create an instance of the module (without even changing the module itself). It is customary to use all capital letters when naming a parameter.

You can simplify the for loop iterator using the increment operator, ++. Change all:

i = i + 1

to

i++

When a line has multiple operators, I prefer to use extra parentheses around each term to avoid potential operator precedence issues and to make the code easier to read. Also, when comparing a 1-bit signal to 1, I prefer to omit the comparison operator. For example, change:

        if(io_dir[i]==0 && addr_i==1 && we_i==1)

to:

        if ((io_dir[i]==0) && (addr_i==1) && we_i)

The `timescale compiler directive is not needed for RTL code since there are no delays specified.

Some synthesis tools may give you trouble with the inout port. It is quite common to split this single port into separate input and output ports.

Here is equivalent code with some of the changes outlined above:

module gpio #(parameter NUM = 8) (
    input clk_i,
    input rst_ni,
    input we_i,
    input [NUM-1:0] wdata_i,
    input [NUM-1:0] addr_i,
    inout [NUM-1:0] port_io,
    output [NUM-1:0] rdata_o
    );
    
logic [NUM-1:0] io_dir;
logic [NUM-1:0] io_val;
logic [NUM-1:0] io_int;

always_ff @(posedge clk_i, negedge rst_ni) begin
    if (~rst_ni) begin
        for (int i=0; i<NUM; i++)
            io_dir <= 0;
    end
    else begin
        if ((addr_i==0) && we_i)
            io_dir <= wdata_i;
    end
end

always_ff @(posedge clk_i, negedge rst_ni) begin
    if (~rst_ni) begin
        for (int i=0; i<NUM; i++)
            io_val <= 0;
    end
    else begin
        for (int i=0; i<NUM; i++) begin
            if ((io_dir[i]==0) && (addr_i==1) && we_i)
                io_val[i] <= wdata_i[i];
            else
                io_val[i] <= port_io[i];
        end
    end
end

always_comb begin
    for (int i=0; i<NUM; i++) begin
        io_int[i] <= (io_dir[i]==0) ? io_val[i] : 1'bz;
    end
end

assign port_io = io_int;
assign rdata_o = io_val;
endmodule

It is also common to code an active-low reset using the negation operator.

if (~rst_ni) begin

Answer 2

Adding my two cents to the already great answer by toolic:

1. Using "integer" as a loop iterator is not recommended, as each bit in integer is a 4 state bit (0, 1, x, z). Since your loop iterator is basically an internal signal that is only used inside the for loop, this is just a waste of simulator memory. This is another benefit of using "int" instead of "integer" --> in "int" each bit is only two states (0, 1).

2. Since you already use system verilog, I will just say that you can simplify the code by replacing:

   if (~rst_ni) begin
   for (int i=0; i<NUM; i++) begin
        somename <= 0;
   end else begin
   

   With:

   if (~rst_ni) begin
      somename <= '0;
   end else begin
   

   The syntax '0 means that you wish to set each bit in the target to 1'b0. Same logic applies to '1 --> Each bit will be set to 1'b1.

   I like it more than looping over the signal since it's portable to all possible dimensions of the target, and it contains less lines.

3. You are coding a GPIO module with actual tristate values (1'bz), so I would recommend you to support tristate values propagation coding style. You can read about it here: https://www.verilogpro.com/systemverilog-verilog-x-optimism/

   For your code it means that I would not recommend to use "if-else" statements, but instead use ternary statements in the next sections:

     end else begin
      io_dir <= (addr_i==0) && we_i ? wdata_i : io_dir;
     end
   
   
   
    end else begin
   for (int i=0; i<NUM; i++) begin
       io_val[i] <= (io_dir[i]==0) && (addr_i==1) && we_i ? wdata_i[i] : port_io[i]; 
   end
   

   This is debatable topic, and really depends on your methodology and verification tools capabilities. I agree that for your GPIO is not really critical, only something that may expand your knowledge on existing coding styles.