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

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

[martin-velay]

Suggested change

	sw_images: ["i2c_device_tx_rx_test_vanilla_sram:5" "bootrom:5"]
	sw_images: ["i2c_device_tx_rx_test_cheri_sram:5" "bootrom:5"]

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

I think this function should be moved in the top_chip_dv_i2c_tx_rx_vseq as I believe top_chip_dv_i2c_host_tx_rx_vseq also need to have the same thing?