I2C serial communication protocol for university project

Question

For now, I have only implemented a simple I2C protocol where only the master transmits the data. Also there is ack_bit for the I2C Address only. For state 5, i.e., data_byte its always acknowledged.

I am just starting with Verilog, and this is my first big project so, please review my code and suggest any changes.

main source code:

 module main(input reset); wire scl,clk; wire sda_line; wire sda_master_enable; master main_master(.reset(reset),.sda_line(sda_line),.scl(scl),.sda_master_enable(sda_master_enable)); slave main_slave(.sda_line(sda_line),.scl(scl),.sda_master_enable(sda_master_enable)); endmodule //////////////////////////////////////////////////////////////////////////////////// module master(input reset, inout sda_line, output reg scl,[2:0]state_out,wire sda_master_enable); wire clk,reset; reg sda,sda_enable = 1; reg [2:0]state = 3'b000; reg [2:0] state_out; reg sda_master_enable; reg [6:0]addr_reg = 7'h69; //69 = 1101001 reg [2:0]count = 3'd6; reg rw_reg = 0; //FOR NOW transmitting data from master; reg [7:0] data_reg = 8'b10001010; reg data_out; reg addr_ack_bit = 1; reg data_ack_bit; reg ack_flag = 0; reg [2:0]data_count = 3'd7; parameter IDLE_STATE = 3'b000, START_STATE = 3'b001, ADDR_STATE = 3'b010, RW_STATE = 3'b011, ADDR_ACK_STATE = 3'b100, DATA_STATE = 3'b101, DATA_ACK_STATE = 3'b110, STOP_STATE = 3'b111; clock_divider master_cd(.i2c_clk(clk)); assign sda_line = (sda_enable) ? sda : 1'bz; // Master drives SDA only when sda_enable = 1 always @(posedge clk) begin if(reset == 0) begin case(state) IDLE_STATE: begin sda<=1;scl<=1; state_out <= IDLE_STATE; state<=START_STATE; end START_STATE: begin sda<=0; state_out <= START_STATE; state<=ADDR_STATE; end ADDR_STATE: begin sda_enable = 1; if(count == 0) begin sda<=addr_reg[count]; state_out <= ADDR_STATE; state<=RW_STATE; count<=3'd6; end else begin sda<=addr_reg[count]; count = count-1; // DATA will work according to sysclk; // you have to configure scl accordingly to match i2c rule; end end RW_STATE: begin sda<=rw_reg; state_out <= RW_STATE; state<=ADDR_ACK_STATE; end ADDR_ACK_STATE: begin sda_enable = 0; state_out <= ADDR_ACK_STATE; end DATA_STATE: begin sda_enable = 1; if(data_count == 0) begin sda<=data_reg[data_count]; state_out <= DATA_STATE; state<=DATA_ACK_STATE; end else begin sda<=data_reg[data_count]; data_count = data_count-1; // DATA will work according to sysclk; // you have to configure scl accordingly to match i2c rule; end end DATA_ACK_STATE: begin sda_enable = 0; data_ack_bit = sda_line; state_out <= DATA_STATE; state <= (data_ack_bit) ? DATA_STATE : STOP_STATE; end STOP_STATE: begin sda_enable <= 1; sda<= 1; scl<=1; state_out <= STOP_STATE; end default: begin sda<=1;scl<=1; end endcase end else if(reset == 1) begin sda<=1; scl <= 1; end state_out <= state; end always @(posedge scl) begin if(state_out == ADDR_ACK_STATE) begin sda = sda_line; // Capture it properly addr_ack_bit = sda; $display("ACK Bit Read: %b", sda); if(addr_ack_bit == 1) begin state <= ADDR_STATE; // Retry address phase if no ACK end else if(addr_ack_bit == 0) begin state <= DATA_STATE; // Proceed to data transmission end end end always @(clk) begin if(state == ADDR_STATE || state == RW_STATE || state == ADDR_ACK_STATE || state == DATA_STATE ||state == DATA_ACK_STATE) begin //Starting of scl after completing starting state; scl <= ~clk; end if(state_out == DATA_ACK_STATE) begin scl <= 1; end end always @(sda_enable) begin sda_master_enable = sda_enable; end endmodule /////////////////////////////////////////////////////////////// module slave(input scl,sda_master_enable,inout sda_line,output addr_data_out,addr_count_out,flag); reg sda,sda_enable = 0; // Controls when slave drives SDA wire clk; reg flag_reg = 1'bz; wire flag; reg [7:0] addr_data = 8'b00000000; reg [7:0] addr_data_out; reg [3:0] addr_count = 4'b1010; //here from 9 to 0 10 states // we require 8 bits (7+1) //+1 bit for starting posedge of scl from Hi-im state; reg [3:0] addr_count_out; reg addr_flag = 0; parameter slave_addr = 8'hD1; assign sda_line = sda_enable ? sda : 1'bz; // To drive the inout net clock_divider master_cd(.i2c_clk(clk)); always @(negedge clk) begin if(flag_reg == 1) begin if(addr_flag == 0) begin if(addr_count <= 9 && addr_count >= 2) begin sda = sda_line; //no non-blocking for sda because we want it right now and here, nd not on next clock cycle addr_data = addr_data | (sda << addr_count-2) ; //same case with addr_data addr_data_out[7:0] <= addr_data[7:0]; addr_count <= addr_count -1; //Some fucked up shit combining bloacking and non blocking is not advisable but for now its giving me correct result so gud!! addr_count_out <= addr_count -1; end else begin addr_count <= addr_count -1; addr_count_out <= addr_count -1; //earlier it was addr_count_out <= addr_count ,,,,which was two step process first addt_count updates in one cycle and then addr_count_out; end end end end always @(negedge sda_line) begin #1; if(scl == 1) flag_reg <= 1; //Starting condition detected end always @(posedge sda_line) begin #1; if(scl == 1) flag_reg <= 0; //stopping condition detected; end always @(negedge sda_master_enable) begin if(addr_flag == 0) begin if(sda_master_enable == 0) begin sda_enable = 1; if (addr_data == slave_addr) begin sda = 1'b0; addr_data <= 8'b00000000; addr_data_out[7:0] <= 8'b00000000; addr_count <= 4'b1010; addr_count_out <= 4'b10; addr_flag<=1; end else begin sda = 1'b1; //NACK addr_data <= 8'b00000000; addr_data_out[7:0] <= 8'b00000000; addr_count <= 4'b1010; addr_count_out <= 4'b1010; addr_flag<=0; //If NACK then resend data from master and re store it in slave end end end else if(addr_flag == 1) begin sda_enable = 1; sda<=1'b0; end end always @(posedge sda_master_enable) begin if(sda_master_enable == 1) begin sda_enable = 0; end end assign flag = flag_reg; //Do notconfuse non blocking /blocking sign to assign sign here "=" simply means they are equal so when ever the reg changes the wire changes; //Above is the good choice to see reg output in testbench waveform since testbench only takes wires:) endmodule //////////////////////////////////////////////////////////// module clock_signal ( output reg val ); initial begin val = 0; forever #5 val = ~val; // 10 nanosecond main clock ;Frequency = 100MHz gud!! // BUT For standard i2c speed we want 100KHz or 10us clock end endmodule ///////////////////////////////////////////////////////////////////////////////// module clock_divider(output i2c_clk); wire ref_clk; wire reset; reg i2c_clk = 1; reg [10:0] counter = 0; clock_signal cd_cs(.val(ref_clk)); always @(posedge ref_clk) begin if(counter == (500)) begin i2c_clk = ~ i2c_clk; counter = 0; end else begin counter = counter + 1; // up counter is always efficient than down counter bcus ripple carry is gud!! end end endmodule ///////////////////////////////////////// 

testbench code:

 module testbench(); wire main_clk; wire i2c_clk; reg reset; wire sda_line, scl; wire [2:0]state_out; wire [3:0] addr_count_out; wire sda_master_enable; wire sda_master_enable; wire scl,sda_line; wire [7:0] addr_data_out; wire flag; // Instantiate the main module main dut1( .reset(reset) ); master dut2(.reset(reset),.sda_line(sda_line),.scl(scl),.state_out(state_out),.sda_master_enable(sda_master_enable)); slave dut5(.scl(scl),.sda_line(sda_line),.addr_data_out(addr_data_out),.addr_count_out(addr_count_out),.flag(flag),.sda_master_enable(sda_master_enable)); clock_signal dut3(.val(main_clk)); clock_divider dut4(.i2c_clk(i2c_clk)); // Test Stimulus initial begin reset = 1; #27000 reset = 0; // Trigger Start Condition #300000 $finish; end // Monitor Outputs initial begin $dumpfile("waveform.vcd"); $dumpvars(0, testbench); end endmodule 

Answer 1

Overview

You have done a good job with:

• Partitioning the design and testbench into separate files
• Dumping waveforms from your testbench to verify and help debug your design
• Using consistent naming style for your signals
• Using parameter types for your constants
• Indenting the code

It is great that you provided a complete code example so we can easily run a simulation.

Portability

It is a good practice to try to compile and run the code on different simulators. Free simulators are available on the EDA Playground website, for example.

On one simulator I tried, I get a compile error on this line:

module master(input reset, inout sda_line, output reg scl,[2:0]state_out,wire sda_master_enable); 

Some simulation software is more forgiving than others; the simulator you used is too forgiving in this case because it should not have allowed you to declare sda_master_enable as both a wire and as a reg a few lines later:

reg sda_master_enable; 

In this case reg is the correct type. This can be fixed by removing wire:

module master(input reset, inout sda_line, output reg scl,[2:0]state_out, sda_master_enable); 

I get a compile warning on this line:

wire clk,reset; 

You already declared reset as an input. There is no need to declare it as a wire as well, and you can remove it from the line:

wire clk; 

Similarly for these signals:

reg [2:0] state_out; reg sda_master_enable; 

These lines should be deleted. This requires more changes which I will discuss below.

In the slave module, these lines should be deleted also:

wire flag; reg [7:0] addr_data_out; reg [3:0] addr_count_out; 

In the testbench, you have 2 redundant lines:

wire sda_line, scl; wire scl,sda_line; 

You should delete one of them.

This line is repeated twice:

wire sda_master_enable; 

Layout

It would be better to collapse multiple consecutive blank lines into a single blank line. I find it easier to understand code if I can see more of it on my screen at once. For example, in the slave module, there is no need for multiple blank lines before the endmodule line.

It is great that you use the ANSI-style for module port declarations because it reduces the redundancy of maintaining separate signals lists using the older style.

Instead of having a very long like of code for the ports, I recommend splitting them onto separate lines. This is a common good practice for Verilog. For example:

module slave ( input scl, input sda_master_enable, inout sda_line, output reg [7:0] addr_data_out, output reg [3:0] addr_count_out, output flag ); 

While this is more verbose since it repeats the input and output keywords for each signal, it does make the code more explicit, and it avoids one of the most common Verilog pitfalls associated with range specifications, like you have in the master module. You should change the code to:

module master ( input reset, inout sda_line, output reg scl, output reg [2:0] state_out, output reg sda_master_enable ); 

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. In testbench you did this for the dut1 instance already. Let's do it for all instances, for example:

slave dut5 ( .scl (scl), .sda_line (sda_line), .addr_data_out (addr_data_out), .addr_count_out (addr_count_out), .flag (flag), .sda_master_enable (sda_master_enable) ); 

This makes the code easier to understand and maintain.

Comments

Add comments to describe each module:

/* I2C slave add details of functionality here: - what features are implemented - what features are not yet implemented */ module slave ( 

Some comments which appear at the end of lines of code like this:

sda = sda_line; //no non-blocking for sda because we want it right now and here, nd not on next clock cycle 

lead to lines that are too long. We should not be forced to do a lot of horizontal scrolling since it detracts from understanding the code. The comment should be moved to a separate line above the code.

// no non-blocking for sda because we want it right now and here, and not on next clock cycle sda = sda_line; 

Naming

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

main is a vague name for a module. Also, it is not clear why it is even included because it seems redundant with the master and slave instances in testbench. If you still need main, you should give it a more meaningful name and add comments describing its purpose.

Reset

In the master module, you use the reset input signal to reset some of the logic. This is the recommended practice since it allows the design to be portable to both FPGA and ASIC design flows.

You should add the signal as an input to the clock_divider module as well. You have it as an unused wire now.

It is common for FPGA flows to initialize registers in the declaration line as you do for the counter signal, but it would be better to use reset.

As far as partitioning goes, it would be better to remove the clock_signal instance out of the clock_divider, and add a clock input port. Then connect the clock_signal output to the new clock_divider input in the testbench. The clock_signal module is not synthesizable, and it really only belongs in the testbench. This change allows the clock_divider to be synthesizable.

Nonblocking

It is considered good practice to use nonblocking assignments (<=) to model sequential logic. This avoids Verilog simulation race conditions. For example, in clock_divider:

always @(posedge ref_clk) begin if(counter == 500) begin counter <= 0; end else begin counter <= counter + 1; end end 

Blocking assignments (=) should only be used to model combinational logic.

Master

Here are some suggestions for the master module.

This sensitivity list is good:

always @(sda_enable) begin 

But, it is a good practice to use the implicit sensitivity list for combinational logic:

always @* begin 

This avoids another common Verilog pitfall of forgetting to add signals that should be there.

This sensitivity list is unusual:

always @(clk) begin 

It implies that you want to describe combinational logic, but it will simulate more like sequential logic. I recommend using the more common:

always @(posedge clk) begin 

This makes it clear that it infers sequential logic.

This line creates a second clock domain in the module:

always @(posedge scl) begin 

This is always a tricky situation. Great care needs to be taken to design with multiple domains. However, in this case, there should be no need to do so. You should be able to restructure all your logic to use the same clock, namely clk:

always @(posedge clk) begin 

It is not clear why you have a state_out signal as well as a state signal. It is common practice to design an FSM using 2 always blocks:

• curr_state: Sequential logic: always @(posedge clk)
• next_state: Combinational logic: always @*

---

Designing is an iterative process, especially for a design like I2C, whose specification may seem simple at first, but there are many subtleties to it. It is great that you have only implemented the limited set of features you need for your design.

What next, you ask? One approach at this point is to try to implement some of these suggestions. The primary goal for now is to simplify the code so that it is a little easier to understand.

Since you are new to this site, your first instinct might be to update the code in your question. Please refrain from doing so. Instead, follow the advice in the Help Center. For example, you could post a follow-up question.