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
1 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
parametertypes 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.