SPI prototype in Verilog: areas for code improvement

Question

I'm working on an SPI in Verilog. I will post what I came up with here. This is an educational project compiled according to the general principle from Wikipedia. There is only one mode - exchange along the rising edge of synchronizing pulses. The problem is this: my code is not very good. It does not implement all functions (only the basic one); due to confusion with discontinuous/continuous assignment, a discrepancy arises between the command for selecting peripherals and signals for the slave, etc.

Please tell me how to improve it and what techniques are best to use? I deliberately did not use ready-made projects in order to understand in general how the SPI works.

Master code:

module Master(clock,AIBO,AOBI,periphery,iSS_out,rCount_out);
input clock; //Standart clock
input periphery; //Future command on "you must work with periphery!"
input [3:0] AIBO; //data to periphery
output [3:0] AOBI; //data from periphery
output [3:0] rCount_out; //Count out
output iSS_out; //SS out after //conditional processing
//Registers and starting points
reg [3:0] RA = 4'b0101;
reg [3:0] rCount = 4'b0000;
reg iSS;
always @(negedge periphery) begin //Count restart
rCount = 4'b0000;
end
always @(posedge clock) begin
if ((periphery == 1'b1 || periphery == 1'b0) && rCount < 3) begin //SS conditional processing
iSS <= periphery;
end
else if ((periphery == 1'b1 || periphery == 1'b0) && rCount >= 3) begin
iSS <= 1'b1;
end
if (iSS == 1'b1) begin
RA <= RA;
rCount <= rCount;
end
else if (iSS == 1'b0) begin
RA <= {AIBO[0],RA[3:1]}; //Exchanging
rCount <= rCount + 4'b0001;
end
else if (iSS == 1'b0) begin
RA <= RA;
rCount <= rCount;
end
end
assign AOBI = RA;
assign rCount_out = rCount;
assign iSS_out = iSS;
endmodule

Slave code:

module Slave(clock,BIAO,BOAI,iSS);
input clock;
input iSS;
input [3:0] BIAO;
output [3:0] BOAI;
//Registers
reg [3:0] RB = 4'b1110;
always @(posedge clock) begin
if (iSS == 1'b1) begin
RB <= RB;
end
else if (iSS == 1'b0) begin
RB <= {BIAO[0],RB[3:1]};
end 
end
assign BOAI = RB;
endmodule

Top code (testbench):

`timescale 1ns / 1ps
module Top;
reg clock;
reg periphery;
wire [3:0] A_out;
wire [3:0] B_out;
wire [3:0] rCount_out;
wire iSS_out;
Master uut1(.clock(clock),.AIBO(B_out),.AOBI(A_out),.periphery(periphery),.iSS_out(iSS_out),.rCount_out(rCount_out));
Slave uut2(.clock(clock),.BIAO(A_out),.BOAI(B_out),.iSS(iSS_out));
initial begin
// Initialize Inputs
clock  = 1'b0;
periphery = 1'b1; #5;
periphery = 1'b0; #10;
periphery = 1'b1; #15;
periphery = 1'b0; #10;
periphery = 1'b1; #15;
periphery = 1'b0; #10;
// Wait 100 ns for global reset to finish
end
always #1 clock = ~clock;  
endmodule

The code seems to work, information is exchanged between Master and Slave, but there is a feeling that the code can be significantly improved.

Answer 1

Layout

The code needs indentation. I recommend 4 spaces per indent level.

I also recommend a blank line between always blocks.

It is great that you used connections-by-name for the module instances. I recommend splitting the instantiations out over several lines, one line per port:

Slave uut2 (
    .clock  (clock),
    .BIAO   (A_out),
    .BOAI   (B_out),
    .iSS    (iSS_out)
);

This makes the code easier to understand and maintain.

Ports

Use ANSI-style port declarations to avoid duplicating the list of signals:

module Slave (
    input clock,
    input iSS,
    input  [3:0] BIAO,
    output [3:0] BOAI
);

Self-assign

It is never necessary to assign a signal to itself. It is customary to omit code such as the following:

if (iSS == 1'b1) begin
    RB <= RB;

Naming

You don't use consistent naming styles for your signals and modules. I recommend all lower-case and snake_case. All upper-case is typically reserved for constants, like a Verilog parameter. Here are some examples:

clock
iss
biao
boai

With some identifiers, you do use descriptive names. However, some signals such as BIAO are hard to understand. Either use a longer, more meaningful name, or add comments describing the name.

periphery sounds like a good name, but I don't understand what it means in an SPI context.

The names of the modules should be more unique. Slave could be spi_slave, for example. We typically connect many designs together to form a larger design, and we need the design names to be unique.

Comments

Add comments to describe each module:

/*

SPI slave

add details of functionality here

*/

module spi_slave (

It is great that you added comments for each port in the master module.

Some comments are unnecessary, like:

// Registers

It is obvious from the reg keyword that it is a register.

Reset

It is a good design practice to use a dedicated reset input signal to initialize the registers. While I believe FPGA design flows allow you to reset the logic in an initial block as you are doing, ASIC flows tend not to support that.

Another benefit is that you are not relying on hidden simulator features like:

// Wait 100 ns for global reset to finish

Clock domains

Always strive to use a single clock signal for all logic. Your usage of clock is good, but the following can introduce a second clock domain:

always @(negedge periphery) begin //Count restart
    rCount = 4'b0000;
end

That should be reworked to be sensitive to clock.

Unreachable code

In the master module, the code in the 2nd else if is unreachable because the condition matches the 1st else if:

  if (iSS == 1'b1) begin
     RA <= RA;
     rCount <= rCount;
  end
  else if (iSS == 1'b0) begin
     RA <= {AIBO[0], RA[3:1]}; //Exchanging
     rCount <= rCount + 4'b0001;
  end
  else if (iSS == 1'b0) begin
     RA <= RA;
     rCount <= rCount;
  end

As I mentioned earlier, self-assignments are unnecessary, so the code can simply be deleted.

Also, if the code is intended to be synthesised (RTL), then it is conventional not to explicitly compare 1-bit signals to both 1 and 0. In fact, in this case, the comparison to 0 can be omitted, simplifying the code to:

  if (iSS) begin
     RA <= RA;
     rCount <= rCount;
  end
  else begin
     RA <= {AIBO[0], RA[3:1]}; //Exchanging
     rCount <= rCount + 4'b0001;
  end

Or, more simply:

  if (iSS == 1'b0) begin
     RA <= {AIBO[0], RA[3:1]}; //Exchanging
     rCount <= rCount + 4'b0001;
  end

Similarly, the following expression can be omitted:

(periphery == 1'b1 || periphery == 1'b0)

This is simpler:

if (rCount < 3) begin //SS conditional processing
    iSS <= periphery;
end
else if (rCount >= 3) begin
    iSS <= 1'b1;
end

Partitioning

There is no need to cram all your code into a single always block. I recommend using 2 always blocks in the master module:

always @(posedge clock) begin
    if (rCount < 3) begin //SS conditional processing
        iSS <= periphery;
    end
    else begin
        iSS <= 1'b1;
    end
end

always @(posedge clock) begin
    if (iSS == 1'b0) begin
       RA <= {AIBO[0], RA[3:1]}; //Exchanging
       rCount <= rCount + 4'b0001;
    end
end

Testbench

Consider using a loop to drive the input in the testbench:

initial begin
    clock = 0;
    repeat (3) begin
        periphery = 1;
        #15;
        periphery = 0;
        #10;
    end
    // Wait 100 ns for global reset to finish
end

Also, use one statement per line.

In a testbench, it is simpler to omit the size for 1-bit constants: 0 is simpler than 1'b0.

SystemVerilog

Your code is not currently taking advantage of any SystemVerilog features, but you should eventually consider doing so. Tools offer varying ways to enable these features (if they are not enabled by default); refer to your simulator documentation, for example.

In the testbench, you could use constants with time units which makes the code easier to understand:

#10ns;
#15ns;