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

[pigtwo]

[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!

 

[ThisIsNotSam]

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.

 

[pigtwo]

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!

 

[ThisIsNotSam]

Assertions in verilog are outdated. SV is probably the best language to learn right now for all things verification.

 

[TrickyDicky]

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?

 

[pigtwo]

@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.

 

[TrickyDicky]

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).

 

[pigtwo]

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!

 

[TrickyDicky]

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).

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

 

[pigtwo]

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.