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

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

Close #507

martin-velay · 2026-05-29T11:21:22Z

 }

-void i2c_init(i2c_t i2c)
+uint16_t i2c_calc_scl_high_cycles(uint16_t rise_cycles, uint16_t fall_cycles,


Maybe better to compute in uint32_t, in case the sum exceeds 16 bits. Then maybe check if it's not going over 16 bits and return in 16 bits?

I think your comment is about L51:
uint16_t scl_high_cycles = scl_period_cycles - (scl_low_cycles + rise_cycles + fall_cycles);

thinking about it scl_period_cycles should satisfy the equation:
scl_period_cycles = rise_cycles + fall_cycles + high_cycle + low_cycles

scl_period_cycles itself is 16-bits so expression scl_low_cycles + rise_cycles + fall_cycles can't go out of bound 16bits I think

The part where I might expect an overflow is scl_low_cycles + rise_cycles + fall_cycles, before the subtraction.

To check whether this is OK, I suggest you calculate the maximum clock speed (smallest SYSCLK_NS) that could cause the above to overflow with standard mode timing. If it's stupidly fast (e.g. 100 GHz), then it's probably safe.

martin-velay · 2026-05-29T11:38:55Z

+//
+// The values for Rise and Fall times for Fast mode are taken as spec minimum. For Fast plus mode,
+// the values are taken from OT's i2c_host_tx_rx_test.c test.
+i2c_timing_params_t compute_minimum_timing_paramaters(i2c_speed_mode_t speed)


Nit:

Suggested change

i2c_timing_params_t compute_minimum_timing_paramaters(i2c_speed_mode_t speed)

i2c_timing_params_t compute_minimum_timing_parameters(i2c_speed_mode_t speed)

Also, this function shouldn't be in the header file instead?

You are suggesting the same thing?

Only function declarations are present in i2c.h here

GitHub isn't highlighting it well, but "paramaters" is indeed spelt incorrectly here.

Correct me if I'm wrong, but I believe that this function is only used by the init() function, so is fine to be omitted from the header file. You could also add static to tell the compiler (linker?) that is is local-only I believe, but it's not needed as far as I'm aware.

Yes, param**e**ters instead of param**a**ters

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.

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>

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:34:56Z

+#include <stdbool.h>
+#include <stdint.h>
+
+bool i2c_host_wr_xfer(i2c_t i2c, uint8_t addr, const uint8_t *data, uint8_t num_wr_bytes)


No need for the "i2c_host" prefix here or on the read version

elliotb-lowrisc · 2026-06-02T15:36:25Z

+    for (uint8_t i = 0; i < (uint8_t)(sizeof(wr_data_bytes)); i++) {
+        wr_data_bytes[i] = 1u << i;
+    }
+
+    if (host_rx_tx_test(i2c, 0x48u, wr_data_bytes, sizeof(wr_data_bytes))) {
+        uint8_t rd_data_bytes[sizeof(wr_data_bytes)];
+        for (uint8_t i = 0; i < (uint8_t)(sizeof(rd_data_bytes)); i++) {
+            rd_data_bytes[i] = i2c_rdata_byte(i2c);
+            if (wr_data_bytes[i] != rd_data_bytes[i]) {
+                return false;
+            }
+        }
+        success = true;
+    }


I think this would be better outside of test_main. Generally only the peripheral setup is in the test_main so that the test can be reused in a larger test suite more easily (I think?)

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-02T15:48:50Z

+  i2c_if i2c_if (.clk_i(clk),
+                 .rst_ni(rst_n),
+                 .scl_io(scl),
+                 .sda_io(sda));


Nit: to be coherent with the rest of the code

Suggested change

i2c_if i2c_if (.clk_i(clk),

.rst_ni(rst_n),

.scl_io(scl),

.sda_io(sda));

i2c_if i2c_if (

.clk_i (clk ),

.rst_ni (rst_n ),

.scl_io (scl ),

.sda_io (sda )

);

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

 # driven externally
 mocha_add_test(NAME gpio_smoketest SOURCES gpio/smoketest.c LIBRARIES ${LIBS} SKIP_VERILATOR SKIP_FPGA)
 mocha_add_test(NAME i2c_smoketest SOURCES i2c/smoketest.c LIBRARIES ${LIBS})
+mocha_add_test(NAME i2c_host_tx_rx_test SOURCES i2c/host_test.c LIBRARIES ${LIBS})


As you've mentioned, Marno has proposed to skip this test for Verilator and FPGA, remember to update this line:

Suggested change

mocha_add_test(NAME i2c_host_tx_rx_test SOURCES i2c/host_test.c LIBRARIES ${LIBS})

mocha_add_test(NAME i2c_host_tx_rx_test SOURCES i2c/host_test.c LIBRARIES ${LIBS} SKIP_VERILATOR SKIP_FPGA)

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:09:29Z

+                                      sw_sys_clk_period_ns[0]);
+
+  super.dut_init(reset_kind);
+endtask


Nit - for consistency

Suggested change

endtask

endtask : dut_init

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)

martin-velay · 2026-06-02T16:17:27Z

+  str = {str, $sformatf("\n| %-20s | %7d |", "tSdaInterference", timing_cfg.tSdaInterference)};
+  str = {str, $sformatf("\n| %-20s | %7d |", "tSclInterference", timing_cfg.tSclInterference)};
+  `uvm_info(`gfn, $sformatf("%s", str), UVM_MEDIUM);
+endfunction


Nit (applies above too):

Suggested change

endfunction

endfunction : print_i2c_timing_cfg

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 ddb4891 to 4e61569 Compare May 28, 2026 11:17

martin-velay reviewed May 29, 2026

View reviewed changes

KinzaQamar force-pushed the i2c_dv branch 2 times, most recently from 2fe1620 to f152d20 Compare May 29, 2026 14:45

KinzaQamar marked this pull request as ready for review May 29, 2026 14:47

KinzaQamar force-pushed the i2c_dv branch 5 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 added 3 commits June 1, 2026 16:55

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

da4a5af

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

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

c4a8e3d

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

KinzaQamar force-pushed the i2c_dv branch from 8d476f5 to 1c9cec2 Compare June 1, 2026 15:57

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

8987db7

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>

KinzaQamar force-pushed the i2c_dv branch 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

	i2c_timing_params_t compute_minimum_timing_paramaters(i2c_speed_mode_t speed)
	i2c_timing_params_t compute_minimum_timing_parameters(i2c_speed_mode_t speed)

	mocha_add_test(NAME i2c_host_tx_rx_test SOURCES i2c/host_test.c LIBRARIES ${LIBS})
	mocha_add_test(NAME i2c_host_tx_rx_test SOURCES i2c/host_test.c LIBRARIES ${LIBS} SKIP_VERILATOR SKIP_FPGA)

	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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KinzaQamar commented Apr 27, 2026 •

edited

Loading