[chip, I2C, DV] Top level I2C host test by KinzaQamar · Pull Request #473 · lowRISC/mocha

KinzaQamar · 2026-04-27T11:46:24Z

Close #507

KinzaQamar · 2026-06-01T11:09:30Z

Refer to the note about why host_test.c is failing on verilator507#issuecomment-4569471032). I've suggest few options but I need few opinions as well.

KinzaQamar · 2026-06-02T14:37:29Z

Refer to the note about why host_test.c is failing on verilator507#issuecomment-4569471032). I've suggest few options but I need few opinions as well.

@marnovandermaas suggested to skip verilator and FPGA images for host_test

elliotb-lowrisc

Generally looking decent to me. I have a few comments on making the SW test a bit clearer and in keeping with other tests

elliotb-lowrisc · 2026-06-02T15:39:17Z

I think we should rename this and the existing smoketest.c to better capture what they do. In this case, it is testing reading and writing to a memory-like I^2C device. The existing "smoketest" does similar, only to a temperature sensor I^2C device. I think having test names that capture whether the i2c block is operating in host/device mode and what it is attempting to communicate with would be much clearer.

martin-velay · 2026-06-02T16:04:53Z

    uvm_config_db#(virtual clk_rst_if)::set(null, "*", "sys_clk_if", sys_clk_if);
    uvm_config_db#(virtual uart_if)::set(null, "*.env.m_uart_agent*", "vif", uart_if);
    uvm_config_db#(virtual pins_if #(NUM_GPIOS))::set(null, "*.env", "gpio_vif", gpio_pins_if);
+    uvm_config_db#(virtual i2c_if)::set(null, "*.env.m_i2c_agent", "vif", i2c_if);


Probably best to add a wildcard as if a sub-component of i2c agent tries to get this handle it will fail. But it doesn't hurt us to give this possibility in case:

Suggested change

uvm_config_db#(virtual i2c_if)::set(null, "*.env.m_i2c_agent", "vif", i2c_if);

uvm_config_db#(virtual i2c_if)::set(null, "*.env.m_i2c_agent*", "vif", i2c_if);

martin-velay · 2026-06-02T16:08:24Z

+  // Set I2C agent config object for I2C agent
+  uvm_config_db#(i2c_agent_cfg)::set(this, "m_i2c_agent", "cfg", cfg.m_i2c_agent_cfg);
+
+  // Create I2C agent
+  m_i2c_agent = i2c_agent::type_id::create("m_i2c_agent", this);
+
  // Instantiate UART agent
  m_uart_agent = uart_agent::type_id::create("m_uart_agent", this);
  uvm_config_db#(uart_agent_cfg)::set(this, "m_uart_agent*", "cfg", cfg.m_uart_agent_cfg);


Nit - for consistency:

Suggested change

// Set I2C agent config object for I2C agent

uvm_config_db#(i2c_agent_cfg)::set(this, "m_i2c_agent", "cfg", cfg.m_i2c_agent_cfg);

// Create I2C agent

m_i2c_agent = i2c_agent::type_id::create("m_i2c_agent", this);

// Instantiate UART agent

m_uart_agent = uart_agent::type_id::create("m_uart_agent", this);

uvm_config_db#(uart_agent_cfg)::set(this, "m_uart_agent*", "cfg", cfg.m_uart_agent_cfg);

// Instantiate UART agent

m_uart_agent = uart_agent::type_id::create("m_uart_agent", this);

uvm_config_db#(uart_agent_cfg)::set(this, "m_uart_agent*", "cfg", cfg.m_uart_agent_cfg);

// Instantiate I2C agent

m_i2c_agent = i2c_agent::type_id::create("m_i2c_agent", this);

uvm_config_db#(i2c_agent_cfg)::set(this, "m_i2c_agent", "cfg", cfg.m_i2c_agent_cfg);

martin-velay · 2026-06-02T16:12:51Z

+  super.dut_init(reset_kind);
+endtask
+
+task top_chip_dv_i2c_host_tx_rx_vseq::body();


You should add a call to the super in case there's something added there in the future:
super.body();

Also, for consistency, you can have these lines at the very beginning like we do for other vseqs:

`DV_WAIT(cfg.sw_test_status_vif.sw_test_status == SwTestStatusInTest); `uvm_info(`gfn, "Starting I2C Host TX-RX test", UVM_LOW)

KinzaQamar · 2026-06-04T15:14:05Z

SW comments addressed

Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

Controller writes multiple bytes to the target's receiver and then reads multiple bytes from the same address. At the end of both read and write transfer, the test checks if the transfer was succesful. If yes, then the test compares all the read bytes with the data bytes that was sent as part of write transfer. Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

This vseq: ** reads the SW symbols to get the timing values ** calculates relevant timing parameter use by the agent to send Ack, Nack and Rdata. ** start the reactive sequence Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

ziuziakowska

Just a few nits.

ziuziakowska · 2026-06-05T10:54:35Z

+static bool host_tx_rx_test(i2c_t i2c, uint8_t addr, uint8_t num_bytes)
+{
+    // Data bytes to send to the target's receiver.
+    uint8_t wr_data_bytes[num_bytes];


I realise now that this here seems a bit ambiguous - num_bytes could refer to either the function parameter or the enum constant 😅, I think it would be best to prefix the enum values with test, e.g test_i2c_address, test_num_bytes, and just using those values directly in the function by changing the signature to be static bool host_tx_rx_test(i2c_t i2c)

ziuziakowska · 2026-06-05T10:54:42Z

+    num_bytes = 0x8,
+};
+
+static bool write_transfer(i2c_t i2c, uint8_t addr, const uint8_t *data, uint8_t num_wr_bytes)


Like the other functions in hal/i2c.h, maybe this should be num_bytes?

ziuziakowska · 2026-06-05T10:56:45Z

+// Below symbols are going to be read by top_chip_dv_i2c_host_tx_rx_vseq in order to calculate agent
+// timing parameters.
+//
+// The values below are standard mode minimums


Suggested change

// Below symbols are going to be read by top_chip_dv_i2c_host_tx_rx_vseq in order to calculate agent

// timing parameters.

//

// The values below are standard mode minimums

// The below symbols are used by top_chip_dv_i2c_host_tx_rx_vseq in order to calculate agent timing parameters.

// The values are the minimums for standard mode.

KinzaQamar marked this pull request as draft April 27, 2026 11:46

KinzaQamar force-pushed the i2c_dv branch 3 times, most recently from fd42f2e to 0c033ce Compare April 28, 2026 13:40

KinzaQamar changed the title ~~[I2C, dv] Initial I2C top level DV setup~~ [chip, I2C, DV] Top level I2C host test Apr 28, 2026

KinzaQamar force-pushed the i2c_dv branch 2 times, most recently from dbb8d58 to 16ba0d4 Compare April 30, 2026 11:19

KinzaQamar force-pushed the i2c_dv branch 3 times, most recently from 4da05c4 to d5d15ad Compare May 7, 2026 15:03

KinzaQamar mentioned this pull request May 7, 2026

I2C DV -- Host TX-RX test #507

Open

KinzaQamar force-pushed the i2c_dv branch 10 times, most recently from f33a9e9 to 3a25ff6 Compare May 12, 2026 12:38

KinzaQamar assigned KinzaQamar and unassigned KinzaQamar May 12, 2026

KinzaQamar force-pushed the i2c_dv branch 4 times, most recently from d5393fb to a90365e Compare May 18, 2026 18:47

KinzaQamar force-pushed the i2c_dv branch 2 times, most recently from 6d98478 to e89e1dc Compare May 19, 2026 19:32

KinzaQamar force-pushed the i2c_dv branch 4 times, most recently from 1911b12 to 8d476f5 Compare June 1, 2026 11:05

KinzaQamar requested review from engdoreis, martin-velay and ziuziakowska June 1, 2026 11:06

KinzaQamar force-pushed the i2c_dv branch 2 times, most recently from 1c9cec2 to 8987db7 Compare June 1, 2026 15:58

elliotb-lowrisc reviewed Jun 2, 2026

View reviewed changes

martin-velay reviewed Jun 2, 2026

View reviewed changes

Comment thread hw/top_chip/dv/tb/tb.sv Outdated

martin-velay reviewed Jun 2, 2026

View reviewed changes

Comment thread sw/device/tests/CMakeLists.txt Outdated

martin-velay reviewed Jun 2, 2026

View reviewed changes

KinzaQamar force-pushed the i2c_dv branch 5 times, most recently from 48917e0 to 0823c30 Compare June 3, 2026 23:26

engdoreis reviewed Jun 4, 2026

View reviewed changes

Comment thread sw/device/lib/hal/i2c.c Outdated

Comment thread sw/device/tests/i2c/host_test.c Outdated

Comment thread sw/device/tests/i2c/host_test.c Outdated

Comment thread sw/device/tests/i2c/host_test.c Outdated

Comment thread sw/device/tests/i2c/host_test.c Outdated

ziuziakowska reviewed Jun 4, 2026

View reviewed changes

KinzaQamar added 4 commits June 4, 2026 22:53

[chip, I2C, DV] Top level wiring of I2C ports with the interface

6af272e

Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

[chip, I2C, DV] Wired up I2C agent in the chip environment

dcae399

Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

[chip, I2C, DV] Added Host TX-RX vseq

da2a4f5

This vseq: ** reads the SW symbols to get the timing values ** calculates relevant timing parameter use by the agent to send Ack, Nack and Rdata. ** start the reactive sequence Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

ziuziakowska reviewed Jun 5, 2026

View reviewed changes

	uvm_config_db#(virtual i2c_if)::set(null, "*.env.m_i2c_agent", "vif", i2c_if);
	uvm_config_db#(virtual i2c_if)::set(null, ".env.m_i2c_agent", "vif", i2c_if);

Conversation

KinzaQamar commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KinzaQamar commented Jun 1, 2026

Uh oh!

KinzaQamar commented Jun 2, 2026

Uh oh!

elliotb-lowrisc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elliotb-lowrisc Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

martin-velay Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

martin-velay Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

martin-velay Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KinzaQamar commented Jun 4, 2026

Uh oh!

ziuziakowska left a comment

Choose a reason for hiding this comment

Uh oh!

ziuziakowska Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

ziuziakowska Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

ziuziakowska Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

KinzaQamar commented Apr 27, 2026 •

edited

Loading