4
\$\begingroup\$

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

waveform for ACK)

waveform for NACK):

\$\endgroup\$
0

1 Answer 1

Reset to default
3
\$\begingroup\$

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.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.