+ Post New Thread
Results 1 to 10 of 10
  1. #1
    Member level 3
    Points: 772, Level: 6

    Join Date
    Jul 2015
    Posts
    61
    Helped
    0 / 0
    Points
    772
    Level
    6

    [Verilog] Testbench Critique - First attempt at a real testbench with verification.

    Hello all,

    A month or so ago I posted about wanting to get better at making testbenches. Previously I just used basic logic to create the signals and respond accordingly. This was super tedious so I've been trying to learn how to do this better. I'm trying to write my code as professionally as possible and to avoid strange looking code. If anyone is inclined I would really appreciate feedback on this testbench(or module for that matter). The module gets data from an ADC(AD7091RBRMZ) and gives it to the module that instantiated it. The test bench drives the module, simulates the functionality of the ADC, and verifies the output.

    Below is the testbench:
    Code Verilog - [expand]
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
    12
    13
    14
    15
    16
    17
    18
    19
    20
    21
    22
    23
    24
    25
    26
    27
    28
    29
    30
    31
    32
    33
    34
    35
    36
    37
    38
    39
    40
    41
    42
    43
    44
    45
    46
    47
    48
    49
    50
    51
    52
    53
    54
    55
    56
    57
    58
    59
    60
    61
    62
    63
    64
    65
    66
    67
    68
    69
    70
    71
    72
    73
    74
    75
    76
    77
    78
    79
    80
    81
    82
    83
    84
    85
    86
    87
    88
    89
    90
    91
    92
    93
    94
    95
    96
    97
    98
    99
    100
    101
    102
    103
    104
    105
    106
    107
    108
    109
    110
    111
    112
    113
    114
    115
    116
    117
    118
    119
    120
    121
    122
    123
    124
    125
    126
    127
    128
    129
    130
    131
    132
    133
    134
    135
    136
    137
    138
    139
    140
    141
    142
    143
    144
    145
    146
    147
    148
    149
    150
    151
    152
    153
    154
    155
    156
    157
    158
    159
    160
    161
    162
    163
    164
    165
    166
    167
    168
    169
    170
    171
    172
    173
    174
    175
    176
    177
    178
    179
    180
    181
    182
    183
    184
    185
    186
    187
    188
    189
    190
    191
    192
    193
    194
    195
    196
    197
    198
    199
    200
    201
    202
    203
    204
    205
    206
    207
    208
    209
    210
    211
    212
    213
    214
    215
    216
    217
    218
    219
    220
    221
    222
    223
    224
    225
    226
    227
    228
    229
    230
    231
    232
    233
    234
    235
    236
    237
    238
    239
    240
    241
    242
    243
    244
    245
    246
    247
    248
    249
    250
    251
    252
    253
    254
    255
    256
    257
    258
    259
    260
    261
    262
    263
    264
    265
    266
    267
    268
    269
    270
    271
    272
    273
    274
    275
    276
    277
    278
    279
    280
    281
    282
    283
    284
    285
    286
    287
    288
    289
    290
    291
    292
    293
    294
    295
    296
    297
    298
    299
    300
    301
    302
    303
    304
    305
    306
    307
    308
    309
    310
    311
    312
    313
    314
    315
    316
    317
    318
    319
    320
    321
    322
    323
    324
    
    `timescale 1ns / 1ps
    //////////////////////////////////////////////////////////////////////////////////
    // Written by:  
    // 
    // Create Date:         02/08/2018 
    // Design Name: 
    // Module Name:     AD7091RBRMZ_TB 
    // Target Devices:  AD7091RBRMZ
    // Description: This module is meant to simulate the action of the AD7091RBRMZ.  It
    //                      will verify very simple timing constraints such as tQuiet, time
    //                      between the conversion input going low to CS going low.  Verification
    //                      of functionality is also tested by driving the module and verifing
    //                      the output.
    //
    // Revision: 1.0
    // Revision 1.0 - Sucessful simulation.
    // Additional Comments: There are three main aspects of the AD7091RBRMZ which needs
    //                              modeling.  First when the module is powered on a soft
    //                                  reset is required.  This is verified by the model.  Next 
    //                                  there are two different modes of operation.  Normal operation
    //                                  and using the busy indicator.  In normal operation the 
    //                                  conversion pin is brought low.  650ns later CS can be brought
    //                                  low to start clocking out data.  When using the busy indicator
    //                                  conversion is brought low.  CS must be brought low within 650ns
    //                                  then the SDO line will be pulled low to indicate the conversion
    //                                  is complete.  Swaping between these modes is supported by this
    //                                  model.  Full details in the data sheet.
    //
    //////////////////////////////////////////////////////////////////////////////////
    module AD7091RBRMZ_TB();
        // Define basic signals
        reg clk, reset;
        
        // Define UUT signals
        wire ADC_conv;  // To ADC
        wire ADC_cs;    // To ADC
        wire ADC_sclk;  // To ADC
        wire ready;     // Tells the user(TB in this case) that another conversion can be started/data from the previous conversion is ready. 
        wire[11:0] data_out; // Data from the module(UUT);
        
        reg ADC_sdo;  // From ADC
        reg convert;   // Input to the UUT that tells it to start a conversion.
        reg use_busy_sig; // Input to the UUT that tells it to use the busy signal.  This takes a single conversion before it takes effect.
        reg [11:0] timing_errors; // Number of timing errors found.
        reg [11:0] verification_errors;  // Number of failures in verification.
        reg [11:0] data_verify;   // Data read from the UUT.
        reg [11:0] data_expected; // Data we expect from the UUT.
        reg [11:0] data_adc;      // Data the model will output.  This will increment after each conversion.
        reg [11:0] data_adc_temp; // Data used to send data out from the model.
        reg [15:0] num_conversions; // Number of conversions we will verify.
        reg use_busy_indicator;   // Denotes whether the busy indicator should be used.  
        
        // Signals used in TB
        reg[15:0] time1, time2;
        reg[15:0] counter;
        
        // UUT instance
        AD7091RBRMZ ADC_UUT(
            .clk(clk),
            .reset(reset),
            .ADC_conv(ADC_conv),
            .ADC_cs(ADC_cs),
            .ADC_sclk(ADC_sclk),
            .ADC_sdo(ADC_sdo),
            .ready(ready),
            .data_out(data_out),
            .convert(convert),
            .use_busy_sig(use_busy_sig)
            );
            
        // clock generation
        initial begin 
            forever #10 clk = ~clk;  // 50 MHz clock
        end
        
        initial begin   // ADC model - This block is meant to model the functionality of the ADC
            // Give initial values to registers
            clk = 0;
            counter = 0;
            ADC_sdo = 1'bz;
            convert = 0;
            data_verify = 0;
            data_adc = 2729;
            data_adc_temp = data_adc;
            data_expected = data_adc;
            num_conversions = 20;
            timing_errors = 0;
            verification_errors = 0;
            use_busy_indicator = 0;
            use_busy_sig = 1;
            time1 = $time;
            time2 = $time;
     
            // Start main process
            // Reset the module
            start_reset();
            
            // Verify soft reset
            wait_for_soft_reset();
            //$display("Soft reset complete!  Device correctly initialized!");
     
            // Wait for conversion forever
            while(1) begin
                @(negedge ADC_conv)   // Conversion received
                start_conversion();   // Start conversion task
                data_adc = data_adc + 1;  // Increment input data.  
            end
        end
     
        initial begin  // This block drives the module to get data then verifies it.
            @(posedge ready)  // Wait for the UUT to be ready
            while(num_conversions != 0) begin  // Get num_conversion about of conversions.
                #20
                convert = 1;
                @(negedge ready)
                #20
                convert = 0;
                if(num_conversions[3])      // Swap the busy signal every once in a while.
                    use_busy_sig = ~use_busy_sig;
                @(posedge ready)
                data_verify = data_out;
                if(data_verify != data_expected) begin
                    verification_errors = verification_errors + 1;      
                end
                data_expected = data_expected + 1;
                num_conversions = num_conversions - 1;
            end
            $display("Verification complete!");
            $display("Timing errors found: %d.", timing_errors);
            $display("Verification errors found: %d.", verification_errors);
        end
     
        task start_reset;  // Reset task.
            begin
                @(posedge clk)
                reset = 0;
                #55
                @(negedge clk)
                reset = 1;
            end
        endtask
     
        task wait_for_soft_reset;   // This task verifies that the power on soft reset is done correctly.  
            begin
                
                @(negedge ADC_conv)
                time1 = $time;
                @(negedge ADC_cs)
                time2 = $time;
                if(time2-time1 < 650) begin
                    $display("Soft reset failed: CS pulled low too early. %d ns", time2-time1);
                    timing_errors = timing_errors + 1;
                end 
                
                ADC_sdo = $random;
                while(counter < 7 && !ADC_cs) begin     // Count how many data bits are clocked out by the module.  This should be 3-7 to initiate a soft reset.
                    @(negedge ADC_sclk or posedge ADC_cs)   // ADC_cs is in this so we can escape the while loop.
                    ADC_sdo = $random;
                    counter = counter + 1;
                end
     
                ADC_sdo = 1'bz;    // ADC_sdo should be highz in between conversions.
                if(counter < 2) begin   // Verify number of data bits clocked out.  Report errors. 
                    $display("CS pulled high too early during soft reset.  Only %d bit(s) were clocked out.", counter + 1);
                    timing_errors = timing_errors + 1;
                end else if(counter > 6) begin
                    $display("CS pulled high too late during soft reset. %d bits were clocked out.", counter + 1);
                    timing_errors = timing_errors + 1;
                end 
                
                // Another conversion must be started to finish the soft reset.  No data is clocked out during this time.  But user must wait the 650ns anyway. 
                @(negedge ADC_conv)  
                time1 = $time;
                time2 = $time;
                @(posedge ADC_conv)
                while(time2 - time1 < 650) begin  // Wait 650ns.
                    #1 time2 <= $time;
                    if(!ADC_conv) begin
                        $display("New conversion started too soon since soft reset.  Only %d ns since end of last conv.", time2-time1);
                        timing_errors = timing_errors + 1;
                    end
                end
                use_busy_indicator = 0;   // At the end of a soft reset the busy indicator is reset to zero. 
            end
        endtask
     
        task soft_reset;  // This is called when a module trys to soft reset the ADC.  It verifys this is done correctly.
            begin
                @(negedge ADC_conv)
                time1 = $time;
                time2 = $time;
                @(posedge ADC_conv)
                while(time2 - time1 < 650) begin
                    #1 time2 <= $time;
                    if(!ADC_conv) begin
                        $display("New conversion started too soon since soft reset.  Only %d ns since end of last conv.", time2-time1);
                        timing_errors = timing_errors + 1;
                    end
                end
                use_busy_indicator = 0;
            end
        endtask
     
        task start_conversion;
            begin
                counter = 0;
                data_adc_temp = data_adc;
                
                // Verify tQuiet timing
                time2 = $time;
                if(time2 - time1 < 50) begin
                    $display("tQuiet timing violated.  Only %d ns since the end of last conversion.  Time: %d ns.", time2-time1, time2);
                    timing_errors = timing_errors + 1;
                end
                
                time1 = $time;
                time2 = $time;
                if(use_busy_indicator) begin    //// The two different conversion methods split here. 
                
                    @(negedge ADC_cs)     // Wait for CS to go low.  
                    time2 = $time;
                    if(time2 - time1 > 650) begin  // If CS goes low after 650ns this means the module wants to turn the busy indicator off.  This will take effect the next conversion.
                        $display("Reseting busy indicator.");
                        use_busy_indicator = 0;
                    end
                    
                    while(time2 - time1 < 650) begin  // Wait for the conversion time to pass. 
                        #1
                        time2 = $time;
                    end
                    #8 ADC_sdo = 0;   // Bring SDO low to indicate the end of the conversion.
                    time1 = $time;
                    time2 = $time;
                    while(!ADC_cs) begin
                        @(negedge ADC_sclk or posedge ADC_cs)  // Clock data out on sclk.  CS is there to escape the while loop.
                        time2 = $time;
                        counter = counter + 1;
                        if(counter > 12) begin  // Once enough data is sent drive SDO to highz.  
                            #8 ADC_sdo = 1'bz;
                        end else begin
                            time1 = $time;
                            #7 ADC_sdo = data_adc_temp[11];
                            data_adc_temp = {data_adc_temp[10:0], data_adc_temp[11]};
                        end
                    end
                    
                    ////  Verify enough data has been sent and check for soft reset request.  
                    if(counter < 3) begin   // CS pulled up to early.  Only 1-2 bits clocked out.  Error.
                        $display("CS pulled low to early during the clocking out of data from conversion.  Only %d bits read out.  Time: %d ns.", counter, time2);
                        timing_errors = timing_errors + 1;
                    end else if(counter < 2 && counter < 8) begin // This initiates a soft reset.
                        $display("Soft reset initiated. Time: %d ns.", time2);
                        soft_reset();
                    end else if(counter > 7 && counter < 14) begin // Counter pulled low too early(too late for soft reset).  Error.
                        $display("CS pulled low to early during the clocking out of data from conversion.  Only %d bits read out.  Time: %d", counter, time2);
                        timing_errors = timing_errors + 1;
                    end else if(counter == 14) begin // Only correct option that isn't a reset.  
                        //$display("Data sucessfuly read from ADC! Time: %d ns.", time2);
                    end else begin  // Counter counted past 13 clock cycles.  Too many.  Error.
                        $display("Too many data bits clocked out during conversion.  %d bits. Time: %d.", counter-1, time2);
                    end
                    time1 = $time - 20; // 20ns subtracted to compensate for this measurment being taken from the rising edge of CS instead of the rising edge of sclk.
                    time2 = $time;
                    
                end else begin
                
                    @(negedge ADC_cs) // There are two reasons CS can go low here.  Either to start clocking data out or set the busy indicator.  This depends on whether 650ns has passed yet.
                    time2 = $time;
                    if(time2 - time1 < 650) begin  // ADC_cs brought low too early.  This could mean they are setting the busy signal or it is a timing violation.
                        @(posedge ADC_cs or posedge ADC_sclk)
                        if(ADC_cs) begin  // This means ADC_cs triggered first.  This would mean they are trying to set the busy signal.
                            @(negedge ADC_cs)
                            use_busy_indicator = 1;
                            time2 = $time;
                            if(time2 - time1 < 650) begin
                                $display("CS pulled low too early while setting busy indicator.  Second edge too early.  CS only held low %d ns.  Time: %d ns.", time2-time1, time2);
                                timing_errors = timing_errors + 1;
                            end
                        end else begin     
                            $display("CS pulled low too early when starting conversion.  Data not valid and simulation may be broken past this point.  CS only held low %d ns.  Time: %d ns.", time2-time1, time2);
                            timing_errors = timing_errors + 1;
                        end
                    end          
                    
                    // Start clocking out data.
                    #18 ADC_sdo = data_adc_temp[11];
                    data_adc_temp = {data_adc_temp[10:0], data_adc_temp[11]};
                    counter = 0;
                    time1 = $time;
                    time2 = $time;
                    while(!ADC_cs) begin
                        @(negedge ADC_sclk or posedge ADC_cs)  // Clock data out on negative edge of ADC_sclk
                        time2 = $time;
                        counter = counter + 1;
                        if(counter > 11) begin  // Drive SDO highz after data.
                            #15 ADC_sdo = 1'bz;
                        end else begin
                            #7 ADC_sdo = data_adc_temp[11];
                            data_adc_temp = {data_adc_temp[10:0], data_adc_temp[11]};
                        end
                    end
                    
                    ////  Verify enough data has been sent and check for soft reset request. 
                    if(counter < 2) begin   // CS pulled up to early.  Only 1-2 bits clocked out.  Error.
                        $display("CS pulled low to early during the clocking out of data from conversion.  Only %d bits read out.  Time: %d ns.", counter, time2);
                        timing_errors = timing_errors + 1;
                    end else if(counter < 1 && counter < 9) begin // This initiates a soft reset.
                        $display("Soft reset initiated. Time: %d ns.", time2);
                        soft_reset();
                    end else if(counter > 6 && counter < 12) begin // Counter pulled low too early(too late for soft reset).  Error.
                        $display("CS pulled low to early during the clocking out of data from conversion.  Only %d bits read out.  Time: %d", counter, time2);
                        timing_errors = timing_errors + 1;
                    end else if(counter == 12) begin // Only correct option that isn't a reset.  
                        //$display("Data sucessfuly read from ADC! Time: %d ns.", time2);
                    end else begin  // Counter counted past 13 clock cycles.  Too many.  Error.
                        $display("Too many data bits clocked out during conversion.  %d bits. Time: %d.", counter, time2);
                    end
                    time1 = $time - 10;  // 10ns subtracted to compensate for this measurment being taken from the rising edge of CS instead of the falling edge of sclk.
                    time2 = $time;
                end
            end
        endtask
     
    endmodule

    Here is the module that the testbench drives, for reference:
    Code Verilog - [expand]
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
    12
    13
    14
    15
    16
    17
    18
    19
    20
    21
    22
    23
    24
    25
    26
    27
    28
    29
    30
    31
    32
    33
    34
    35
    36
    37
    38
    39
    40
    41
    42
    43
    44
    45
    46
    47
    48
    49
    50
    51
    52
    53
    54
    55
    56
    57
    58
    59
    60
    61
    62
    63
    64
    65
    66
    67
    68
    69
    70
    71
    72
    73
    74
    75
    76
    77
    78
    79
    80
    81
    82
    83
    84
    85
    86
    87
    88
    89
    90
    91
    92
    93
    94
    95
    96
    97
    98
    99
    100
    101
    102
    103
    104
    105
    106
    107
    108
    109
    110
    111
    112
    113
    114
    115
    116
    117
    118
    119
    120
    121
    122
    123
    124
    125
    126
    127
    128
    129
    130
    131
    132
    133
    134
    135
    136
    137
    138
    139
    140
    141
    142
    143
    144
    145
    146
    147
    148
    149
    150
    151
    152
    153
    154
    155
    156
    157
    158
    159
    160
    161
    162
    163
    164
    165
    166
    167
    168
    169
    170
    171
    172
    173
    174
    175
    176
    177
    178
    179
    180
    181
    182
    183
    184
    185
    186
    187
    188
    189
    190
    191
    192
    193
    194
    195
    196
    197
    198
    199
    200
    201
    202
    203
    204
    205
    206
    207
    208
    209
    210
    211
    212
    213
    214
    215
    216
    217
    218
    219
    220
    221
    222
    223
    224
    225
    226
    227
    228
    229
    230
    231
    232
    233
    234
    235
    236
    237
    238
    239
    240
    241
    242
    243
    244
    245
    246
    247
    248
    249
    250
    251
    252
    253
    254
    255
    256
    257
    258
    259
    260
    261
    262
    263
    264
    265
    266
    267
    268
    269
    270
    271
    272
    273
    274
    275
    276
    277
    278
    279
    280
    281
    282
    283
    284
    285
    286
    287
    288
    289
    290
    291
    292
    293
    294
    295
    296
    297
    298
    299
    300
    301
    302
    303
    304
    305
    306
    307
    308
    309
    310
    311
    312
    313
    314
    315
    316
    317
    
    `timescale 1ns / 1ps
    //////////////////////////////////////////////////////////////////////////////////
    // Company: 
    // Engineer: 
    // 
    // Create Date:    15:02:33 01/06/2018 
    // Design Name: 
    // Module Name:    AD7091RBRMZ 
    // Project Name: 
    // Target Devices: 
    // Tool versions: 
    // Description: 
    //
    // Dependencies: 
    //
    // Revision: 
    // Revision 0.01 - File Created
    // Additional Comments: 
    //
    //////////////////////////////////////////////////////////////////////////////////
    module AD7091RBRMZ(
            input wire clk,
            input wire reset,
            
            // ADC signals
            output reg ADC_conv,
            output reg ADC_cs,
            output wire ADC_sclk,
            
            input wire ADC_sdo,
            
            // Control signals
            output reg ready,
            output reg[11:0] data_out,
            input wire convert,
            input wire use_busy_sig
        );
        
        // Local parameters
        //localparam use_busy_sig = 1;     // This tells the module to use the busy signal feature or not. 
        
        localparam[4:0]
            start = 0,
            idle = 1,
            convert_start = 2,
            convert_start_busy = 3,
            convert_cs_busy = 4,
            convert_cs = 5,
            convert_wait_busy = 6,
            convert_get_data = 7,
            convert_wait = 8,
            convert_get_data_extra = 9,
            convert_done = 10,
            convert_get_data_delay = 11,
            reset_convert_start = 12,
            reset_convert_wait1 = 13,
            reset_convert_get_data = 14,
            reset_quiet_wait = 15,
            reset_convert_wait2 = 16,
            convert_wait_ce_busy = 17, 
            convert_hold_ce = 18,
            convert_delay = 19,
            convert_stop_busy_sig = 20;
        
        // Define local registers and wires
        reg ADC_conv_nxt, ADC_cs_nxt, ready_nxt, ADC_ce, ADC_ce_nxt;
        reg busy_init, busy_init_nxt;  // This register says whether the busy signal has been initialized.  
       reg busy_sig, busy_sig_nxt;    // This register saves the value of 'use_busy_sig' when a conversion starts.  
        reg[4:0] state, state_nxt;
        reg[5:0] counter, counter_nxt;
        reg[11:0] data_out_nxt;
        
        assign ADC_sclk = (ADC_ce) ? clk : 0;   // ADC clock gating.
    /*  
        always@(posedge clk or negedge clk) begin
            ADC_sclk <= (ADC_ce_nxt) ? clk : busy_init;
        end
    */
     
        always@(negedge clk) begin   // ADC_ce needs to transistion on negative edges to avoid glitches.
            if(!reset) begin
                ADC_ce <= 0;
            end else begin
                ADC_ce <= ADC_ce_nxt;
            end
        end
     
        // Define sequential logic
        always@(posedge clk) begin
            if(!reset) begin
                ADC_conv <= 1;
                ADC_cs <= 1;
                ready <= 0;
                counter <= 0;
                data_out <= 0;
                busy_init <= 0;
                busy_sig <= 0;
                state <= start;
            end else begin
                ADC_conv <= ADC_conv_nxt;
                ADC_cs <= ADC_cs_nxt;
                ready <= ready_nxt;
                counter <= counter_nxt;
                data_out <= data_out_nxt;
                busy_init <= busy_init_nxt;
                busy_sig <= busy_sig_nxt;
                state <= state_nxt;
            end 
        end
        
        // Define combinational logic
        always @* begin
            // Define default transistions
            ADC_conv_nxt = ADC_conv;
            ADC_cs_nxt = ADC_cs;
            ADC_ce_nxt = ADC_ce;
            ready_nxt = ready;
            counter_nxt = counter;
            data_out_nxt = data_out;
            busy_init_nxt = busy_init;
            busy_sig_nxt = busy_sig;
            state_nxt = state;
            
            case(state)
            
                start: begin
                    ADC_conv_nxt = 1;
                    ADC_cs_nxt = 1;
                    ADC_ce_nxt = 0;
                    ready_nxt = 0;
                    counter_nxt = 0;
                    data_out_nxt = 0;
                    busy_init_nxt = 0;
                    busy_sig_nxt = 0;
                    state_nxt = reset_convert_start;
                end
                
                reset_convert_start: begin   // This is the start of the reset process.  Must be done before device can be used.
                    ADC_conv_nxt = 0;
                    state_nxt = reset_convert_wait1;
                end
                
                reset_convert_wait1: begin  // Start conversion and wait for conversion to finish(650ns).
                    ADC_conv_nxt = 1;
                    counter_nxt = counter + 1;
                    if(counter == 33) begin
                        counter_nxt = 0;
                        ADC_ce_nxt = 1;
                        ADC_cs_nxt = 0;
                        state_nxt = reset_convert_get_data;
                    end
                end
                
                reset_convert_get_data: begin  // Start reading data, but pull CS high prematurely(triggers soft reset).
                    counter_nxt = counter + 1;
                    if(counter == 4) begin   // We need to halt the read between the second and eighth bit.
                        ADC_cs_nxt = 1; 
                        ADC_ce_nxt = 0;
                        counter_nxt = 0;
                        state_nxt = reset_quiet_wait;
                    end
                end
                
                reset_quiet_wait: begin  // Wait a few cycles for the quiet wait time.  Then start a new conversion.
                    counter_nxt = counter + 1;
                    if(counter == 4) begin
                        ADC_conv_nxt = 0;
                        state_nxt = reset_convert_wait2;
                    end
                end
                
                reset_convert_wait2: begin  // Wait for conversion to finish then reset is done.
                    ADC_conv_nxt = 1;
                    counter_nxt = counter + 1;
                    if(counter == 34) begin
                        counter_nxt = 0;
                        state_nxt = idle;
                    end
                end
                
                idle: begin
                    ADC_conv_nxt = 1;
                    ADC_cs_nxt = 1;
                    ADC_ce_nxt = 0;
                    ready_nxt = 1;
                    counter_nxt = 0;
                    data_out_nxt = data_out;
                    
                    if(convert) begin        // A conversion request is received
                        ready_nxt = 0;
                        busy_sig_nxt = use_busy_sig;
                        if(busy_init)      // Determine if the busy signal is initialized or not.
                            state_nxt = convert_start_busy;  // States with the prefix 'busy' are the path taken when the busy indicator is used. 
                        else
                            state_nxt = convert_start;
                    end else begin
                        state_nxt = idle;
                    end
                end
                
                convert_start_busy: begin  
                    ADC_conv_nxt = 0;
                    state_nxt = convert_cs_busy;
                end
                
                convert_cs_busy: begin
                    ADC_conv_nxt = 1;
                    if(!busy_sig) begin
                        ADC_cs_nxt = 1;
                        counter_nxt = 0;
                        state_nxt = convert_stop_busy_sig;
                    end else begin
                        ADC_cs_nxt = 0;
                        state_nxt = convert_wait_busy;
                    end
                end
                
                convert_stop_busy_sig: begin  // To reset the busy signal you must bring CS low after the conversion time.  This state waits 650ns, then brings CS low.  
                    counter_nxt = counter + 1;
                    ADC_cs_nxt = 1;
                    if(counter == 33) begin
                        counter_nxt = 0;
                        ADC_cs_nxt = 0;
                        state_nxt = convert_wait_busy;
                    end
                end
                
                convert_wait_busy: begin  // Wait for the busy interupt
                    if(!ADC_sdo) begin   // The ADC finished the conversion.  
                        counter_nxt = 0;
                        state_nxt = convert_wait_ce_busy;
                    end 
                end
                
                convert_wait_ce_busy: begin  // This state is nessesary because CE is triggered off the negative edge.  The time from CS going low to SDO going low is 18ns.  This means the negative edge has been missed.  One extra delay is needed.
                    ADC_ce_nxt = 1;
                    state_nxt = convert_get_data;
                end
                
                convert_start: begin  // This begins the conversion process without using the busy signal. 
                    ADC_conv_nxt = 0;
                    ADC_cs_nxt = 1;
                    state_nxt = convert_cs;
                end
                
                convert_cs: begin  // Brings conv high and keeps CS high unless the busy signal is trying to be initialized.
                    ADC_conv_nxt = 1;
                    ADC_cs_nxt = 1;
                    if(busy_sig)   // CS needs to be brought low once during the conversion time to set the busy signal.
                        ADC_cs_nxt = 0;
                    state_nxt = convert_wait;
                end
                
                convert_wait: begin   // Wait for conversion to finish.
                    counter_nxt = counter + 1;
                    ADC_cs_nxt = 1;
                    if(counter == 33) begin
                        counter_nxt = 0;
                        ADC_cs_nxt = 0;
                        state_nxt = convert_get_data_delay; 
                    end
                end
                
                convert_get_data_delay: begin  // An extra 
                    ADC_ce_nxt = 1;
                    data_out_nxt = {data_out[10:0], ADC_sdo};
                    state_nxt = convert_get_data;
                end
                
                convert_get_data: begin
                    ADC_ce_nxt = 1;
                    data_out_nxt = {data_out[10:0], ADC_sdo};
                    counter_nxt = counter + 1;
                    if(counter == 10) begin  // We have all 12 bits(11 iterations plus the one bit sampled as we entered this state).
                        if(busy_init) begin    // We have to hold CE high for an extra cycle if using busy signal.
                            if(!busy_sig)   // If the busy signal is low and the busy signal is initialized then we want to reset it.  Only the flag is set here.  The work was done earlier.
                                busy_init_nxt = 0;
                            state_nxt = convert_get_data_extra;
                        end else begin
                            if(busy_sig)
                                busy_init_nxt = 1;     // If the busy signal is not initialized but the busy_sig is high then we need to initialize it.  Only the flag is set here.  The work was done earlier.  
                            state_nxt = convert_done;
                        end
                    end 
                end
     
                convert_get_data_extra: begin  // This state gets another bit from the ADC.  This is nessesary because when using the busy signal you have to clock the interupt bit out.
                    data_out_nxt = {data_out[10:0], ADC_sdo};
                    ADC_ce_nxt = 1;
                    state_nxt = convert_hold_ce;  // We need a delay so another conversion doesn't start too soon.  
                end
     
                convert_hold_ce: begin // This state allows the clock to clock one more time to put SDO to high z.
                    ADC_ce_nxt = 0;
                    ADC_cs_nxt = 0;
                    ready_nxt = 0;
                    state_nxt = convert_delay;  // We need a delay so another conversion doesn't start too soon.  This helps me tQuiet.  This one is specificly when using the busy signal.
                end
                
                convert_delay: begin
                    ready_nxt = 0;
                    state_nxt = convert_done;
                end
                
                convert_done: begin  // This is a delay so another conversion doesn't start too soon.
                    ADC_cs_nxt = 1;
                    ADC_ce_nxt = 0;
                    ready_nxt = 1;
                    state_nxt = idle;
                end
                
                default: state_nxt = start; 
            endcase
        end
        
        
    endmodule

    I've attached the datasheet for the ADC in case that is useful. The waveforms of interest are on pages 18, 19, and 21. The code for both of these is a little more complicated than you would expect given an ADC with a SPI interface. Most of that complexity comes from the fact that the ADC can operate in two different modes and you can switch between them. I may have also just made it too complicated.

    I've never seen what a serious testbench looks before. The ones I've seen just drive the clock and reset lines then provide a couple static packets of data. So I have no idea how much testing of timing I should be doing or generally the structure of testbenches. So feel free to comment about things like this if they seem odd. My intention is to possibly show code like this in an interview or display of my work. (Also, I know there are a ton of spelling errors in the comments. I'm garbage at spelling so I fix these at the very end once I'm done making changes.)

    Thank you and I appreciate any advice or input!

  2. #2
    Advanced Member level 4
    Points: 5,363, Level: 17

    Join Date
    Apr 2016
    Posts
    1,126
    Helped
    202 / 202
    Points
    5,363
    Level
    17

    Re: [Verilog] Testbench Critique - First attempt at a real test bench with verificati

    The general structure and organisation look ok to me. This design, however, is not a good candidate for a complex testbench. It's very data-centric instead of control-centric. You can verify it with a model like you did, but I don't see opportunities for more complex testbench features like assertions, functional coverage, constrained random stimuli, BFMs, etc.
    Really, I am not Sam.



    •   Alt13th February 2018, 17:06

      advertising

        
       

  3. #3
    Member level 3
    Points: 772, Level: 6

    Join Date
    Jul 2015
    Posts
    61
    Helped
    0 / 0
    Points
    772
    Level
    6

    Re: [Verilog] Testbench Critique - First attempt at a real test bench with verificati

    Ok, I was sort of thinking that. It seems the only thing to check here is basic timing and whether data makes it from one end to the other. I'll be using this in a larger design so maybe that one requires different strategies like you mention.

    As I mentioned I'm not really sure to what degree things are tested usually. I don't know if you can tell quickly from looking at it but does my testbench seem reasonable for the module that it's testing? Or would more strict testing be done? I wasn't exactly sure how many timing parameters I should test.

    ...for more complex testbench features like assertions, functional coverage, constrained random stimuli, BFMs, etc.
    Most of the things you mention at the end there I only have an extremely basic understanding of. In regards to assertions specifically, I tried to learn about them but with just Verilog it seemed like it required a different language(OVL) or something that I didn't completely understand. But I think I see that System Verilog has assertions natively. So I'm trying to decide whether I should just bite the bullet and learn SV. Would SV be useful in implementing the other more complex methods you mention in your quote? Or can all this be done with Verilog?

    Thanks again!



    •   Alt13th February 2018, 21:28

      advertising

        
       

  4. #4
    Advanced Member level 4
    Points: 5,363, Level: 17

    Join Date
    Apr 2016
    Posts
    1,126
    Helped
    202 / 202
    Points
    5,363
    Level
    17

    Re: [Verilog] Testbench Critique - First attempt at a real test bench with verificati

    Assertions in verilog are outdated. SV is probably the best language to learn right now for all things verification.
    Really, I am not Sam.



    •   Alt13th February 2018, 21:32

      advertising

        
       

  5. #5
    Advanced Member level 5
    Points: 35,434, Level: 45
    Achievements:
    7 years registered

    Join Date
    Jun 2010
    Posts
    6,485
    Helped
    1891 / 1891
    Points
    35,434
    Level
    45

    Re: [Verilog] Testbench Critique - First attempt at a real test bench with verificati

    I'd say its a start. You've moving away from just writing some RTL style testbench to thinking about breaking it up more. But I think it is still too involved and too inflexible.
    Ideally, you would have some sort of control explictly for the interface (maybe even an SV interface) with send_data and receive_data methods. This way you have a generic interface that can send and receive any data from the ADC. From there you could build it up into specific routines of various sequences, so you could have many tests using the same testbench, just using parameters to control which sequence of data to drive in any particular test.

    The more tests you can do, with the least amount of code-rewrite needed, the better and more flexible the test will be.
    Add in constrained random (like putting random delays within your send_data/receive_data methods), assertions and coverage, and you will have enourmous flexability. Randomness will help you find those corner cases that happen all too often in real hardware, that never showed up on your simple testbench because "it should work!". The main goal here is moving from "testing for success" to "testing for failure".

    As to whether your code is a good test as it is - only you can tell us. Does it cover the cases you require? Do you think you've found all the appropriate corner cases? What did you have in the original test plan?


    2 members found this post helpful.

  6. #6
    Member level 3
    Points: 772, Level: 6

    Join Date
    Jul 2015
    Posts
    61
    Helped
    0 / 0
    Points
    772
    Level
    6

    Re: [Verilog] Testbench Critique - First attempt at a real test bench with verificati

    @ThisIsNotSam That's what I thought I was finding while researching this. Almost everyone seems to reference SV. I started looking into it but I can't seem to find a simulator that can simulate non-synthesizable SV.

    @TrickyDicky Thank you for the input.
    But I think it is still too involved and too inflexible.
    I think I understand what you mean by inflexible. All it can really do is start a conversion and all the tests associated are forced to be run. I'm thinking it should be something where there is a task(or maybe something better) for individual tests that you want to verify. So you can just call them under certain conditions or something. Correct me if I'm wrong, it's still sort of vague to me what the general structure is for test benches.

    But I'm not sure what you mean by too involved. Is it just too complicated or am I doing things manually that can be done is an easier or automated way?

    Ideally, you would have some sort of control explictly for the interface (maybe even an SV interface) with send_data and receive_data methods. This way you have a generic interface that can send and receive any data from the ADC. From there you could build it up into specific routines of various sequences, so you could have many tests using the same testbench, just using parameters to control which sequence of data to drive in any particular test.
    This makes a lot of sense. I didn't do the verification part until the end and so I ended up with a dumb method of just initializing both the sending and verifying sides with the same number and always incrementing.

    The more tests you can do, with the least amount of code-rewrite needed, the better and more flexible the test will be.
    Add in constrained random (like putting random delays within your send_data/receive_data methods), assertions and coverage, and you will have enourmous flexability. Randomness will help you find those corner cases that happen all too often in real hardware, that never showed up on your simple testbench because "it should work!". The main goal here is moving from "testing for success" to "testing for failure".
    I was working on the module today to try to convert it to run at twice the frequency today and everything was perfect. And after reading this comment I manually put a delay before the start of a read and it uncovered an error. So I already see the value of adding randomness. In regards to this it seems like a lot of the tools that are useful here are in SV(for example I was trying to generate a random number within a range which apparently Verilog doesn't have but SV does.). But I'm having a lot of trouble finding a simulator that can simulate the non-synthesizable parts of SV. Is there a simulator commonly used or how do people go about this?

    As to whether your code is a good test as it is - only you can tell us. Does it cover the cases you require? Do you think you've found all the appropriate corner cases? What did you have in the original test plan?
    I think I covered most cases and tested most things that make sense to test but it's kind of hard to tell if I'm missing something. It's kind odd with this because all the module can really do wrong is violate timing or just not work at all. Unfortunately I didn't think of a test plan before hand which was certainly a mistake.



  7. #7
    Advanced Member level 5
    Points: 35,434, Level: 45
    Achievements:
    7 years registered

    Join Date
    Jun 2010
    Posts
    6,485
    Helped
    1891 / 1891
    Points
    35,434
    Level
    45

    Re: [Verilog] Testbench Critique - First attempt at a real test bench with verificati

    But I'm not sure what you mean by too involved. Is it just too complicated or am I doing things manually that can be done is an easier or automated way?
    I mean you have everything happening in a single task - too involved. There is no way I could come along and say "Hey pigtwo, any chance I could re-use your code to drive the ADC for my tests"? The answer atm is no

    Is there a simulator commonly used or how do people go about this?
    Modelsim/Questa, ActiveHDL/Riviera, Xilinx XSIM would the be standard ones. What are you using at the moment? A simulator that claims it can do SV shouldnt care whether the code is synthesisable or not. It's either SV capable (sometimes with caveats) or it's not .
    SV used to be an extra licensable feature for some simulators, but I think it comes as standard for them now.


    It's kind odd with this because all the module can really do wrong is violate timing or just not work at all.
    RTL Simulation will help you determine if your design is functionally correct. It cannot check timing. A netlist simulation will show you some timings.
    But nowadays, on FPGA, people generally stick with RTL simulation and Timing Analysis. No need for netlist simulations (and thats a good thing, as netlist sims are sloooow).



    •   Alt15th February 2018, 00:14

      advertising

        
       

  8. #8
    Member level 3
    Points: 772, Level: 6

    Join Date
    Jul 2015
    Posts
    61
    Helped
    0 / 0
    Points
    772
    Level
    6

    Re: [Verilog] Testbench Critique - First attempt at a real test bench with verificati

    I mean you have everything happening in a single task - too involved. There is no way I could come along and say "Hey pigtwo, any chance I could re-use your code to drive the ADC for my tests"? The answer atm is no
    That makes sense. I think the problem here was when I first started I thought the idea was to first build a model of the ADC that would just respond as the ADC would. Then you could run tests against it. But obviously all the tests will be in the model and it becomes very cumbersome. I'm guessing the better way to do this is instead of making a big monolithic model of the device, you create individual tasks that simulate various functions that the model would do. Obviously I to actually attempt this to fully understand it.

    Modelsim/Questa, ActiveHDL/Riviera, Xilinx XSIM would the be standard ones. What are you using at the moment? A simulator that claims it can do SV shouldnt care whether the code is synthesisable or not. It's either SV capable (sometimes with caveats) or it's not .
    SV used to be an extra licensable feature for some simulators, but I think it comes as standard for them now.
    I'm using Modelsim but the one provided from Altera/Intel. I was actually just being dumb here. I googled 'System Verilog simulator' and there were a few places that said modelsim and other simulators couldn't simulate the non-synthesizable SV. So I tried to do '#$urandom_range(100,1);' to see if it could do SV and I got an error. Without thinking I just assume it couldn't do SV. I realize now it just doesn't like that statement not the fact that it used the $urandom_range. This is actually a major releif because I was starting to think that you needed to work for a large company to be able to afford one of these simulators. So that is great news!



  9. #9
    Advanced Member level 5
    Points: 35,434, Level: 45
    Achievements:
    7 years registered

    Join Date
    Jun 2010
    Posts
    6,485
    Helped
    1891 / 1891
    Points
    35,434
    Level
    45

    Re: [Verilog] Testbench Critique - First attempt at a real test bench with verificati

    with modelsim, by default it compiles .v files as verilog, and .sv files as SV. You can force system verilog by using the -sv switch on vlog. In addition, if you want to force a specific SV revision, you can follow it up with -sv05compat (also 09 and 12).

    Quote Originally Posted by Modelsim Reference Manual, Vlog
    -sv
    (optional) Enables SystemVerilog features and keywords. By default ModelSim follows the
    IEEE Std 1364-2001 and ignores SystemVerilog keywords. If a source file has a ".sv"
    extension, ModelSim will automatically parse SystemVerilog keywords.


    1 members found this post helpful.

  10. #10
    Member level 3
    Points: 772, Level: 6

    Join Date
    Jul 2015
    Posts
    61
    Helped
    0 / 0
    Points
    772
    Level
    6

    Re: [Verilog] Testbench Critique - First attempt at a real test bench with verificati

    Ah ok, that's good to know. Thank you again for the advice. I think I have a much better idea of what I should try to do now. I'm going to have to spend some time familiarizing myself with SV. Then I have another module I need to write for this project so hopefully I can apply some of this correctly.



--[[ ]]--