Skip to content

I2C SW cleanups #605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
KinzaQamar merged 4 commits into from
Jun 8, 2026
Merged

I2C SW cleanups #605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4dcf9b5
[i2c, sw] Fixed typos and made compute_timing_parameter() static
KinzaQamar Jun 6, 2026
38ec242
[i2c, sw] Made the VOLATILE_READ to status register part of the check
KinzaQamar Jun 6, 2026
a1b90c4
[i2c, sw] Renaming i2c/smoketest to i2c/temperature_sensor_test
KinzaQamar Jun 6, 2026
14d1552
[i2c, sw] Use the same functions to check read / write transfer status
KinzaQamar Jun 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sw/device/examples/i2c.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
35 changes: 6 additions & 29 deletions sw/device/lib/hal/i2c.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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);
Comment thread
KinzaQamar marked this conversation as resolved.

timing_params.scl_high_cycles =
i2c_calc_scl_high_cycles(timing_params.rise_cycles, timing_params.fall_cycles,
Expand Down Expand Up @@ -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) {
Expand All @@ -200,36 +200,13 @@ 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) {

@ziuziakowska ziuziakowska Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably fine as-is, just with the name status.

@KinzaQamar KinzaQamar Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. status name will look inconsistent with the rest of the naming convention used within this function for registers.

return true; // Transaction succeeded
}
}
}
}

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
}
i2c_status i2c_status_reg = VOLATILE_READ(i2c->status);
if (i2c_status_reg & i2c_status_fmtempty) {
return true;
}
}
}

void i2c_enable_controller_mode(i2c_t i2c)
{
VOLATILE_WRITE(i2c->ctrl, i2c_ctrl_enablehost);
Expand Down
8 changes: 3 additions & 5 deletions sw/device/lib/hal/i2c.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion sw/device/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand Down
4 changes: 2 additions & 2 deletions sw/device/tests/i2c/smoketest.c → ...evice/tests/i2c/temperature_sensor_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ 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;
}

// Read current temperature
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;
}

Expand Down