[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-05-21T00:50:50Z

DV changes are open for review now.

SW changes will be reviewable once #565 and #535 are merged

martin-velay · 2026-05-29T09:12:11Z

@@ -0,0 +1,66 @@
+// Copyright lowRISC contributors (OpenTitan project).


Suggested change

// Copyright lowRISC contributors (OpenTitan project).

// Copyright lowRISC contributors (COSMIC project).

martin-velay · 2026-05-29T09:16:23Z

+  str = {str, $sformatf("\n    timing_cfg.tSetupStart       : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tSetupStart)};
+  str = {str, $sformatf("\n    timing_cfg.tHoldStart        : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tHoldStart)};
+  str = {str, $sformatf("\n    timing_cfg.tClockStart       : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tClockStart)};
+  str = {str, $sformatf("\n    timing_cfg.tClockLow         : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tClockLow)};
+  str = {str, $sformatf("\n    timing_cfg.tSetupBit         : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tSetupBit)};
+  str = {str, $sformatf("\n    timing_cfg.tClockPulse       : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tClockPulse)};
+  str = {str, $sformatf("\n    timing_cfg.tHoldBit          : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tHoldBit)};
+  str = {str, $sformatf("\n    timing_cfg.tClockStop        : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tClockStop)};
+  str = {str, $sformatf("\n    timing_cfg.tSetupStop        : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tSetupStop)};
+  str = {str, $sformatf("\n    timing_cfg.tHoldStop         : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tHoldStop)};
+  str = {str, $sformatf("\n    timing_cfg.tTimeOut          : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tTimeOut)};
+  str = {str, $sformatf("\n    timing_cfg.enbTimeOut        : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.enbTimeOut)};
+  str = {str, $sformatf("\n    timing_cfg.tStretchHostClock : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tStretchHostClock)};
+  str = {str, $sformatf("\n    timing_cfg.tSdaUnstable      : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tSdaUnstable)};
+  str = {str, $sformatf("\n    timing_cfg.tSdaInterference  : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tSdaInterference)};
+  str = {str, $sformatf("\n    timing_cfg.tSclInterference  : %d",
+            cfg.m_i2c_agent_cfg.timing_cfg.tSclInterference)};


Can you do the following, as we have the right to go up to 120 characters per line in Mocha:

Suggested change

str = {str, $sformatf("\n timing_cfg.tSetupStart : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tSetupStart)};

str = {str, $sformatf("\n timing_cfg.tHoldStart : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tHoldStart)};

str = {str, $sformatf("\n timing_cfg.tClockStart : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tClockStart)};

str = {str, $sformatf("\n timing_cfg.tClockLow : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tClockLow)};

str = {str, $sformatf("\n timing_cfg.tSetupBit : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tSetupBit)};

str = {str, $sformatf("\n timing_cfg.tClockPulse : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tClockPulse)};

str = {str, $sformatf("\n timing_cfg.tHoldBit : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tHoldBit)};

str = {str, $sformatf("\n timing_cfg.tClockStop : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tClockStop)};

str = {str, $sformatf("\n timing_cfg.tSetupStop : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tSetupStop)};

str = {str, $sformatf("\n timing_cfg.tHoldStop : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tHoldStop)};

str = {str, $sformatf("\n timing_cfg.tTimeOut : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tTimeOut)};

str = {str, $sformatf("\n timing_cfg.enbTimeOut : %d",

cfg.m_i2c_agent_cfg.timing_cfg.enbTimeOut)};

str = {str, $sformatf("\n timing_cfg.tStretchHostClock : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tStretchHostClock)};

str = {str, $sformatf("\n timing_cfg.tSdaUnstable : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tSdaUnstable)};

str = {str, $sformatf("\n timing_cfg.tSdaInterference : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tSdaInterference)};

str = {str, $sformatf("\n timing_cfg.tSclInterference : %d",

cfg.m_i2c_agent_cfg.timing_cfg.tSclInterference)};

str = {str, $sformatf("\n timing_cfg.tSetupStart : %d", cfg.m_i2c_agent_cfg.timing_cfg.tSetupStart)};

str = {str, $sformatf("\n timing_cfg.tHoldStart : %d", cfg.m_i2c_agent_cfg.timing_cfg.tHoldStart)};

str = {str, $sformatf("\n timing_cfg.tClockStart : %d", cfg.m_i2c_agent_cfg.timing_cfg.tClockStart)};

str = {str, $sformatf("\n timing_cfg.tClockLow : %d", cfg.m_i2c_agent_cfg.timing_cfg.tClockLow)};

str = {str, $sformatf("\n timing_cfg.tSetupBit : %d", cfg.m_i2c_agent_cfg.timing_cfg.tSetupBit)};

str = {str, $sformatf("\n timing_cfg.tClockPulse : %d", cfg.m_i2c_agent_cfg.timing_cfg.tClockPulse)};

str = {str, $sformatf("\n timing_cfg.tHoldBit : %d", cfg.m_i2c_agent_cfg.timing_cfg.tHoldBit)};

str = {str, $sformatf("\n timing_cfg.tClockStop : %d", cfg.m_i2c_agent_cfg.timing_cfg.tClockStop)};

str = {str, $sformatf("\n timing_cfg.tSetupStop : %d", cfg.m_i2c_agent_cfg.timing_cfg.tSetupStop)};

str = {str, $sformatf("\n timing_cfg.tHoldStop : %d", cfg.m_i2c_agent_cfg.timing_cfg.tHoldStop)};

str = {str, $sformatf("\n timing_cfg.tTimeOut : %d", cfg.m_i2c_agent_cfg.timing_cfg.tTimeOut)};

str = {str, $sformatf("\n timing_cfg.enbTimeOut : %d", cfg.m_i2c_agent_cfg.timing_cfg.enbTimeOut)};

str = {str, $sformatf("\n timing_cfg.tStretchHostClock : %d", cfg.m_i2c_agent_cfg.timing_cfg.tStretchHostClock)};

str = {str, $sformatf("\n timing_cfg.tSdaUnstable : %d", cfg.m_i2c_agent_cfg.timing_cfg.tSdaUnstable)};

str = {str, $sformatf("\n timing_cfg.tSdaInterference : %d", cfg.m_i2c_agent_cfg.timing_cfg.tSdaInterference)};

str = {str, $sformatf("\n timing_cfg.tSclInterference : %d", cfg.m_i2c_agent_cfg.timing_cfg.tSclInterference)};

martin-velay · 2026-05-29T11:17:33Z

+  // Read the timing parameters through SW backdoor load
+  sw_symbol_backdoor_read("SysClkPeriodNS", sw_sys_clk_period_ns);
+  sw_symbol_backdoor_read("SCLLowTimeNs", sw_scl_low_time_ns);
+  sw_symbol_backdoor_read("HoldDataTimeNs", sw_data_hold_time_ns);


Where sw_scl_clk_period_ns is populated? I think something like that is missing?
sw_symbol_backdoor_read("SCLClkPeriodNS", sw_scl_clk_period_ns);

This is already there on L33

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?

martin-velay · 2026-05-29T11:24:11Z

+  extern function new(string name="");
+  extern task body();
+  extern virtual task dut_init(string reset_kind = "HARD");
+  extern local function void configure_agent_timing();


Do we really need to restrict this function to be local?

I will probably move it to the base vseq (top_chip_dv_i2c_tx_rx_vseq) and make it protected

martin-velay · 2026-05-29T11:33:06Z

-    // "UM10204" Table 10 (rev. 6) / Table 11 (rev. 7).
-    // Faster modes will require different/adjustable constants and checking that SCL high/low
-    // cycles calculated are sufficient to still allow the clock stretching logic to function.
+    // scl_high_time should be atleast 4 cycles to aid correct clock streching


Suggested change

// scl_high_time should be atleast 4 cycles to aid correct clock streching

// scl_high_time should be at least 4 cycles to aid correct clock stretching

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

+
+  extern function new(string name="");
+
+  // Obtain an integer minimum of timing parameter "a" and round up to the next highest integer.


Could you reformulate, I feel that the word minimum is confusing, but it might be my comprehension

I think you are right. I meant spec minimum

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

martin-velay · 2026-05-29T11:39:31Z

+// specification "UM10204" Table 10 (rev. 6) / Table 11 (rev. 7).
+//
+// 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.


Suggested change

// the values are taken from OT's i2c_host_tx_rx_test.c test.

// the values are taken from OpenTitan's i2c_host_tx_rx_test.c test.

martin-velay · 2026-05-29T11:40:05Z

 #define I2C_FALL_NS (120)

-void i2c_init(i2c_t i2c);
+// All the speed modes supported by OT's I2C block


Suggested change

// All the speed modes supported by OT's I2C block

// All the speed modes supported by OpenTitan's I2C block

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 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 2 times, most recently from a007ff8 to fb2d82e Compare May 20, 2026 21:04

KinzaQamar requested review from elliotb-lowrisc, marnovandermaas and martin-velay May 21, 2026 00:49

elliotb-lowrisc reviewed May 21, 2026

View reviewed changes

Comment thread hw/top_chip/dv/env/seq_lib/top_chip_dv_base_vseq.sv

Comment thread hw/top_chip/dv/env/seq_lib/top_chip_dv_i2c_host_tx_rx_vseq.sv

KinzaQamar force-pushed the i2c_dv branch 2 times, most recently from d44829e to f5bc214 Compare May 21, 2026 17:54

KinzaQamar requested a review from elliotb-lowrisc May 21, 2026 22:54

KinzaQamar force-pushed the i2c_dv branch 10 times, most recently from ddb4891 to 4e61569 Compare May 28, 2026 11:17

martin-velay reviewed May 29, 2026

View reviewed changes

KinzaQamar added 3 commits May 29, 2026 15:22

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

e6175b5

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

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

c6ec9d0

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

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

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

e202744

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 2 times, most recently from f152d20 to e202744 Compare May 29, 2026 14:54

		@@ -0,0 +1,66 @@
		// Copyright lowRISC contributors (OpenTitan project).

	// Copyright lowRISC contributors (OpenTitan project).
	// Copyright lowRISC contributors (COSMIC project).

	// scl_high_time should be atleast 4 cycles to aid correct clock streching
	// scl_high_time should be at least 4 cycles to aid correct clock stretching


		extern function new(string name="");

		// Obtain an integer minimum of timing parameter "a" and round up to the next highest integer.

	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)

	// the values are taken from OT's i2c_host_tx_rx_test.c test.
	// the values are taken from OpenTitan's i2c_host_tx_rx_test.c test.

	// All the speed modes supported by OT's I2C block
	// All the speed modes supported by OpenTitan's I2C block

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 May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

KinzaQamar commented May 21, 2026 •

edited

Loading

KinzaQamar May 29, 2026 •

edited

Loading