From 4dcf9b579bb5a11b33b559ac3c5bc5ac6da40531 Mon Sep 17 00:00:00 2001 From: Kinza Qamar Date: Sun, 7 Jun 2026 00:45:41 +0100 Subject: [PATCH 1/4] [i2c, sw] Fixed typos and made compute_timing_parameter() static compute_timing_parameters() is only used by i2c_init(). Signed-off-by: Kinza Qamar --- sw/device/lib/hal/i2c.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sw/device/lib/hal/i2c.c b/sw/device/lib/hal/i2c.c index bf2bde6fc..1341505cc 100644 --- a/sw/device/lib/hal/i2c.c +++ b/sw/device/lib/hal/i2c.c @@ -26,7 +26,7 @@ uint16_t i2c_calc_scl_high_cycles(uint16_t rise_cycles, uint16_t fall_cycles, uint16_t scl_period_cycles, uint16_t scl_low_cycles, uint16_t scl_high_cycles_min) { - // 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 scl_high_cycles_min = (scl_high_cycles_min < 4u) ? 4u : scl_high_cycles_min; // An SCL period duration is divided into 4 segments: @@ -51,8 +51,8 @@ uint16_t i2c_calc_scl_high_cycles(uint16_t rise_cycles, uint16_t fall_cycles, // 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. -i2c_timing_params_t compute_minimum_timing_paramaters(i2c_speed_mode_t speed) +// the values are taken from OpenTitan's i2c_host_tx_rx_test.c test. +static i2c_timing_params_t compute_minimum_timing_parameters(i2c_speed_mode_t speed) { switch (speed) { case i2c_speed_mode_standard: @@ -104,7 +104,7 @@ i2c_timing_params_t compute_minimum_timing_paramaters(i2c_speed_mode_t speed) void i2c_init(i2c_t i2c, i2c_speed_mode_t speed_mode) { - i2c_timing_params_t timing_params = compute_minimum_timing_paramaters(speed_mode); + i2c_timing_params_t timing_params = compute_minimum_timing_parameters(speed_mode); timing_params.scl_high_cycles = i2c_calc_scl_high_cycles(timing_params.rise_cycles, timing_params.fall_cycles, From 38ec242f8fd448adf3f654de75ba2aa5f1bdfe8c Mon Sep 17 00:00:00 2001 From: Kinza Qamar Date: Sun, 7 Jun 2026 00:53:00 +0100 Subject: [PATCH 2/4] [i2c, sw] Made the VOLATILE_READ to status register part of the check Signed-off-by: Kinza Qamar --- sw/device/lib/hal/i2c.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/sw/device/lib/hal/i2c.c b/sw/device/lib/hal/i2c.c index 1341505cc..6a1c178bd 100644 --- a/sw/device/lib/hal/i2c.c +++ b/sw/device/lib/hal/i2c.c @@ -200,8 +200,7 @@ bool i2c_wait_write_finish(i2c_t i2c) return false; // Transaction failed } if (i2c_intr_state_reg & i2c_intr_cmd_complete) { - i2c_status i2c_status_reg = VOLATILE_READ(i2c->status); - if (i2c_status_reg & i2c_status_fmtempty) { + if (VOLATILE_READ(i2c->status) & i2c_status_fmtempty) { return true; // Transaction succeeded } } @@ -223,8 +222,7 @@ bool i2c_wait_read_finish(i2c_t i2c) // treat a halt event as a failure, we intentionally skip clearing it. return false; // Transaction failed } - i2c_status i2c_status_reg = VOLATILE_READ(i2c->status); - if (i2c_status_reg & i2c_status_fmtempty) { + if (VOLATILE_READ(i2c->status) & i2c_status_fmtempty) { return true; } } From a1b90c4b16c59eb076a34c624d4304f0cb8a76b6 Mon Sep 17 00:00:00 2001 From: Kinza Qamar Date: Sun, 7 Jun 2026 00:58:14 +0100 Subject: [PATCH 3/4] [i2c, sw] Renaming i2c/smoketest to i2c/temperature_sensor_test Now that we are chip level I2C Host/Device tests, renaming existing i2c/smoketest with the functionality of the test seems reasonable. Signed-off-by: Kinza Qamar --- sw/device/tests/CMakeLists.txt | 2 +- sw/device/tests/i2c/{smoketest.c => temperature_sensor_test.c} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename sw/device/tests/i2c/{smoketest.c => temperature_sensor_test.c} (100%) diff --git a/sw/device/tests/CMakeLists.txt b/sw/device/tests/CMakeLists.txt index 460f6efcb..2b1069256 100644 --- a/sw/device/tests/CMakeLists.txt +++ b/sw/device/tests/CMakeLists.txt @@ -16,7 +16,7 @@ mocha_add_test(NAME gpio_reg_access_test SOURCES gpio/reg_access_test.c LIBRARIE # Can't run this test on FPGA or verilator tops and this test thinks that the GPIO inputs can be # 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_temperature_sensor_test SOURCES i2c/temperature_sensor_test.c LIBRARIES ${LIBS}) mocha_add_test(NAME mailbox_smoketest SOURCES mailbox/smoketest.c LIBRARIES ${LIBS}) mocha_add_test(NAME plic_smoketest SOURCES plic/smoketest.c LIBRARIES ${LIBS}) mocha_add_test(NAME pwrmgr_smoketest SOURCES pwrmgr/smoketest.c LIBRARIES ${LIBS}) diff --git a/sw/device/tests/i2c/smoketest.c b/sw/device/tests/i2c/temperature_sensor_test.c similarity index 100% rename from sw/device/tests/i2c/smoketest.c rename to sw/device/tests/i2c/temperature_sensor_test.c From 14d15525aa7fa74a0e3cd13a97e1e32762178182 Mon Sep 17 00:00:00 2001 From: Kinza Qamar Date: Sun, 7 Jun 2026 01:06:02 +0100 Subject: [PATCH 4/4] [i2c, sw] Use the same functions to check read / write transfer status The only difference b/w them was that i2c_wait_read_finish() wasn't checking cmd_complete field of the intr_state register. In host mode, this interrupt is raised if the host issues a repeated START or terminates the transaction by issuing STOP. But OT's programming guide doesn't say anything about checking that field in case of a read transfer. I think it is better to check that field in case of Reads as it basically means that the transfer is completed. This also reduce the need of having 2 separate functions with just a difference of not checking cmd_complete field of the intr_state register. Signed-off-by: Kinza Qamar --- sw/device/examples/i2c.c | 4 ++-- sw/device/lib/hal/i2c.c | 23 +------------------ sw/device/lib/hal/i2c.h | 8 +++---- sw/device/tests/i2c/temperature_sensor_test.c | 4 ++-- 4 files changed, 8 insertions(+), 31 deletions(-) diff --git a/sw/device/examples/i2c.c b/sw/device/examples/i2c.c index f13c6d20a..5824158d5 100644 --- a/sw/device/examples/i2c.c +++ b/sw/device/examples/i2c.c @@ -31,9 +31,9 @@ int main(void) uint8_t w_data = 0; // Read current temperature from an AS6212 I^2C-bus sensor and print the value i2c_write_bytes(i2c, 0x48u, &w_data, 1); - if (i2c_wait_write_finish(i2c)) { // select TVAL reg; also a presence check + if (i2c_wait_transfer_finish(i2c)) { // select TVAL reg; also a presence check i2c_read_bytes(i2c, 0x48u, 1); - if (i2c_wait_read_finish(i2c)) { + if (i2c_wait_transfer_finish(i2c)) { uint16_t sensor_reading = i2c_rdata_byte(i2c); // read TVAl reg uprintf(uart, "Temperature: 0x%x degC\n", (sensor_reading << 1)); // no decimal printf diff --git a/sw/device/lib/hal/i2c.c b/sw/device/lib/hal/i2c.c index 6a1c178bd..c854da576 100644 --- a/sw/device/lib/hal/i2c.c +++ b/sw/device/lib/hal/i2c.c @@ -184,7 +184,7 @@ void i2c_read_bytes(i2c_t i2c, uint8_t addr, uint8_t num_bytes) VOLATILE_WRITE(i2c->fdata, fdata_reg); } -bool i2c_wait_write_finish(i2c_t i2c) +bool i2c_wait_transfer_finish(i2c_t i2c) { // Wait for transaction to complete and report simple succeed / fail while (true) { @@ -207,27 +207,6 @@ bool i2c_wait_write_finish(i2c_t i2c) } } -bool i2c_wait_read_finish(i2c_t i2c) -{ - // Wait for transaction to complete and report simple succeed / fail - while (true) { - i2c_intr i2c_intr_state_reg = VOLATILE_READ(i2c->intr_state); - if (i2c_intr_state_reg & i2c_intr_controller_halt) { - // Reset FMT FIFO as controller's FSM is in halt - i2c_fifo_ctrl fifo_ctrl_reg = { .fmtrst = 1u }; - VOLATILE_WRITE(i2c->fifo_ctrl, fifo_ctrl_reg); - - // According to programmer's guide, the CONTROLLER_EVENTS register would be cleared - // here to acknowledge the controller halt interrupt. However, since we want to - // treat a halt event as a failure, we intentionally skip clearing it. - return false; // Transaction failed - } - if (VOLATILE_READ(i2c->status) & i2c_status_fmtempty) { - return true; - } - } -} - void i2c_enable_controller_mode(i2c_t i2c) { VOLATILE_WRITE(i2c->ctrl, i2c_ctrl_enablehost); diff --git a/sw/device/lib/hal/i2c.h b/sw/device/lib/hal/i2c.h index d2db556ff..ac7190c44 100644 --- a/sw/device/lib/hal/i2c.h +++ b/sw/device/lib/hal/i2c.h @@ -118,11 +118,9 @@ void i2c_write_bytes(i2c_t i2c, uint8_t addr, const uint8_t *data, uint8_t num_b // Receive multiple bytes from the target void i2c_read_bytes(i2c_t i2c, uint8_t addr, uint8_t num_bytes); -// Wait for the read transfer to complete by checking interrupt state and status register fields -bool i2c_wait_read_finish(i2c_t i2c); - -// Wait for the write transfer to complete by checking interrupt state and status register fields -bool i2c_wait_write_finish(i2c_t i2c); +// Wait for the write / read transfer to complete by checking interrupt state and status register +// fields. +bool i2c_wait_transfer_finish(i2c_t i2c); // Enable I2C in controller mode void i2c_enable_controller_mode(i2c_t i2c); diff --git a/sw/device/tests/i2c/temperature_sensor_test.c b/sw/device/tests/i2c/temperature_sensor_test.c index f590e570a..191ca6c5d 100644 --- a/sw/device/tests/i2c/temperature_sensor_test.c +++ b/sw/device/tests/i2c/temperature_sensor_test.c @@ -17,7 +17,7 @@ static bool as6212_test(i2c_t i2c) i2c_write_bytes(i2c, 0x48u, &w_data, 1); // Check if the write was successful - if (!i2c_wait_write_finish(i2c)) { + if (!i2c_wait_transfer_finish(i2c)) { return false; } @@ -25,7 +25,7 @@ static bool as6212_test(i2c_t i2c) i2c_read_bytes(i2c, 0x48u, 1); // Check if the read was successful - if (!i2c_wait_read_finish(i2c)) { + if (!i2c_wait_transfer_finish(i2c)) { return false; }