[top-i2c,dv] Added chip level I2C Device TX_RX test

[KinzaQamar]

Added a host Vseq and device SW. The host will initiate read / write transfer. The device SW waits for the transfers to finish and compare the data bytes in each transfer at the end

[engdoreis]

I left a suggestion, otherwise it LGTM.

[engdoreis]

I suggest making a function to clear interrupts that take the interrupt as argument like uart:

mocha/sw/device/lib/hal/uart.c

Line 36 in 3b3b9e2

void uart_interrupt_enable_clear(uart_t uart, uart_intr intrs)

[KinzaQamar]

Do you mean something like that:

void i2c_clear_intr_state(i2c_t i2c, uint32_t state) 
{
    VOLATILE_WRITE(i2c->intr_state, state);
}

[engdoreis]

Yes, but 'state' would be an enum with the interrupts. It should also accept more then one interrupt per call.

[KinzaQamar]

For this test, I only wanted to clear cmd_complete field of intr_state register. I am not checking / enabling interrupts for smoke tests. But your suggestion is good and I'll do it as part of a separate PR. Thanks!

[martin-velay]

I think these changes also have an impact on top_chip_dv_i2c_host_tx_rx_vseq. Isn't it an issue?

[KinzaQamar]

I don't think that host_tx_rx_test gets affected. I've took these calculations from i2c_base_vseq function get_timing_values(). They look sensible to reason.

[KinzaQamar]

Oh, sorry. I think I didn't answer this question well. These parameters are used to add delays which should depend on the speed of the I2C bus and not on host / device part. So, whatever the speed is just calculate the timing parameter the same way

[martin-velay]

Sorry, I was unclear. The issue I see is that you've created the dut_init task for top_chip_dv_i2c_device_tx_rx_vseq to read some SW symbols and then use them in this configure_agent_timing function, all fine here. But in your implementation of top_chip_dv_i2c_host_tx_rx_vseq::dut_init() should also backdoor read these missing SW symbols.

This also needs to be added in host_tx_rx_test.c as done in device_tx_rx_test.c or in a common shared file?

Something like:

const uint16_t scl_high_time_ns = 4000;
const uint16_t setup_data_time_ns = 250;
const uint16_t setup_start_time_ns = 4700;
const uint16_t hold_start_time_ns = 4000;
const uint16_t setup_stop_time_ns = 4000;
const uint16_t bus_free_time_ns = 4700;
const uint16_t rise_time_ns = I2C_RISE_NS;
const uint16_t fall_time_ns = I2C_FALL_NS;

[KinzaQamar]

No, they are not useful for the host test. For host test, the TB is the device responsible for sending responses (rdata, ack, and nack). This is the function i2c_driver.sv#L50 called by the driver where only device_send_bit(_) is used deep down in i2c_if that majorly uses tClockPulse, tClockLow, and tHoldBit. other parameters like tSdaUnstable and tTimeOut are not neccessary for smoke test

[KinzaQamar]

I've added comments here: changes#diff-e7f07ab57afcc3afbc109eaf390fe9db01a5e5372a8e20b41ccb00e04c9ef14eR37-R51 explaining the we oinly need subset of timing parameters for host test

[martin-velay]

Traced through calc_timing_params_in_cycles() for the host path and I don't think "the remaining parameters default to 0" holds up. round_up_divide(a, b) is ((a - 1) / b) + 1, with a/b as int unsigned. For every parameter host_tx_rx doesn't backdoor-read (scl_high, setup_data, setup_start, hold_start, setup_stop, bus_free, rise, fall), the backing sw_*_time_ns array is never written, so a = 0 when round_up_divide runs. 0 - 1 in unsigned 32-bit arithmetic wraps to 4294967295, not -1, so the result isn't 0 — it's floor(4294967295 / clk_period) + 1 truncated into a 16-bit field. For a 5ns clock period that's 858993460, which truncates to 13108 cycles, not 0. So 8 of the 10 *_cycles fields end up with a clock-period-dependent garbage value, and nearly every field configure_agent_timing() computes (tSetupBit, tClockPulse, tClockLow, tSetupStop, tHoldStop) mixes in at least one of them.

I'd rather we set these explicitly for the host test the same way device_tx_rx_test.c does, rather than leaning on round_up_divide to return 0 for an input it was never written to handle — that keeps both vseqs symmetric and doesn't depend on an accident of integer wraparound working out. Could you add the same 8 consts to host_tx_rx_test.c and the corresponding backdoor reads to top_chip_dv_i2c_host_tx_rx_vseq::dut_init(), same as the device vseq?

[KinzaQamar]

Oh yes, thanks for figuring this out. My intention is just to add stuff that is required for the vseq / test. I can add all the timing parameters in host_tx_rx_test.x but it is not going to be consumed by the i2c_driver when it is driving the device interface. I can go ahead and revert it back to manually calculating scl_low_cycles and scl_hold_cycles. WDYT?

[martin-velay]

Reverting fixes the underflow, but tClockPulse/tSetupBit still end up 0 either way, and tClockPulse is one of the three you called essential. Rather than reason about it further, could you run i2c_host_tx_rx and paste the print_i2c_timing_cfg() output here? If it passes cleanly with tClockPulse/tSetupBit at 0, I'll drop this. If not, let's just add the explicit consts to host_tx_rx_test.c instead.

[KinzaQamar]

but tClockPulse/tSetupBit still end up 0 either way, and tClockPulse is one of the three you called essential.

Oh yes, you are right. Sorry, I am a bit slow today. I'll re-think about it again. Thanks for your careful analysis

[martin-velay]

Sorry, I was unclear. The issue I see is that you've created the dut_init task for top_chip_dv_i2c_device_tx_rx_vseq to read some SW symbols and then use them in this configure_agent_timing function, all fine here. But in your implementation of top_chip_dv_i2c_host_tx_rx_vseq::dut_init() should also backdoor read these missing SW symbols.

This also needs to be added in host_tx_rx_test.c as done in device_tx_rx_test.c or in a common shared file?

Something like:

const uint16_t scl_high_time_ns = 4000;
const uint16_t setup_data_time_ns = 250;
const uint16_t setup_start_time_ns = 4700;
const uint16_t hold_start_time_ns = 4000;
const uint16_t setup_stop_time_ns = 4000;
const uint16_t bus_free_time_ns = 4700;
const uint16_t rise_time_ns = I2C_RISE_NS;
const uint16_t fall_time_ns = I2C_FALL_NS;

[martin-velay]

This PR #600 has been merged now. Could you rebase and remove this workaround?

[martin-velay]

Same here #600 is now merged