Merge branch 'fix/i2c_pin_short_cut_v5.2' into 'release/v5.2'

fix(i2c_master): Modify the behavior from ISR WDT to return timeout when circut get shortcut(backport v5.2)

See merge request espressif/esp-idf!31569

components/driver/i2c/i2c.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -110,6 +110,18 @@ static const char *I2C_TAG = "i2c";
 #endif
 #define I2C_MEM_ALLOC_CAPS_DEFAULT      MALLOC_CAP_DEFAULT
 
+#if SOC_PERIPH_CLK_CTRL_SHARED
+#define I2C_CLOCK_SRC_ATOMIC() PERIPH_RCC_ATOMIC()
+#else
+#define I2C_CLOCK_SRC_ATOMIC()
+#endif
+
+#if !SOC_RCC_IS_INDEPENDENT
+#define I2C_RCC_ATOMIC() PERIPH_RCC_ATOMIC()
+#else
+#define I2C_RCC_ATOMIC()
+#endif
+
 /**
  * I2C bus are defined in the header files, let's check that the values are correct
  */
@@ -240,7 +252,9 @@ static void i2c_hw_disable(i2c_port_t i2c_num)
 {
     I2C_ENTER_CRITICAL(&(i2c_context[i2c_num].spinlock));
     if (i2c_context[i2c_num].hw_enabled != false) {
-        periph_module_disable(i2c_periph_signal[i2c_num].module);
+        I2C_RCC_ATOMIC() {
+            i2c_ll_enable_bus_clock(i2c_num, false);
+        }
         i2c_context[i2c_num].hw_enabled = false;
     }
     I2C_EXIT_CRITICAL(&(i2c_context[i2c_num].spinlock));
@@ -250,7 +264,10 @@ static void i2c_hw_enable(i2c_port_t i2c_num)
 {
     I2C_ENTER_CRITICAL(&(i2c_context[i2c_num].spinlock));
     if (i2c_context[i2c_num].hw_enabled != true) {
-        periph_module_enable(i2c_periph_signal[i2c_num].module);
+        I2C_RCC_ATOMIC() {
+            i2c_ll_enable_bus_clock(i2c_num, true);
+            i2c_ll_reset_register(i2c_num);
+        }
         i2c_context[i2c_num].hw_enabled = true;
     }
     I2C_EXIT_CRITICAL(&(i2c_context[i2c_num].spinlock));
@@ -375,7 +392,9 @@ esp_err_t i2c_driver_install(i2c_port_t i2c_num, i2c_mode_t mode, size_t slv_rx_
         return ESP_FAIL;
     }
     i2c_hw_enable(i2c_num);
-    i2c_hal_init(&i2c_context[i2c_num].hal, i2c_num);
+    I2C_CLOCK_SRC_ATOMIC() {
+        i2c_hal_init(&i2c_context[i2c_num].hal, i2c_num);
+    }
     //Disable I2C interrupt.
     i2c_ll_disable_intr_mask(i2c_context[i2c_num].hal.dev, I2C_LL_INTR_MASK);
     i2c_ll_clear_intr_mask(i2c_context[i2c_num].hal.dev, I2C_LL_INTR_MASK);
@@ -478,7 +497,9 @@ esp_err_t i2c_driver_delete(i2c_port_t i2c_num)
     }
 #endif
 
-    i2c_hal_deinit(&i2c_context[i2c_num].hal);
+    I2C_CLOCK_SRC_ATOMIC() {
+        i2c_hal_deinit(&i2c_context[i2c_num].hal);
+    }
     free(p_i2c_obj[i2c_num]);
     p_i2c_obj[i2c_num] = NULL;
 
@@ -746,15 +767,19 @@ esp_err_t i2c_param_config(i2c_port_t i2c_num, const i2c_config_t *i2c_conf)
         return ret;
     }
     i2c_hw_enable(i2c_num);
-    i2c_hal_init(&i2c_context[i2c_num].hal, i2c_num);
+    I2C_CLOCK_SRC_ATOMIC() {
+        i2c_hal_init(&i2c_context[i2c_num].hal, i2c_num);
+    }
     I2C_ENTER_CRITICAL(&(i2c_context[i2c_num].spinlock));
     i2c_ll_disable_intr_mask(i2c_context[i2c_num].hal.dev, I2C_LL_INTR_MASK);
     i2c_ll_clear_intr_mask(i2c_context[i2c_num].hal.dev, I2C_LL_INTR_MASK);
 #if SOC_I2C_SUPPORT_SLAVE
     if (i2c_conf->mode == I2C_MODE_SLAVE) {  //slave mode
         i2c_hal_slave_init(&(i2c_context[i2c_num].hal));
         i2c_ll_slave_tx_auto_start_en(i2c_context[i2c_num].hal.dev, true);
-        i2c_ll_set_source_clk(i2c_context[i2c_num].hal.dev, src_clk);
+        I2C_CLOCK_SRC_ATOMIC() {
+            i2c_ll_set_source_clk(i2c_context[i2c_num].hal.dev, src_clk);
+        }
         i2c_ll_set_slave_addr(i2c_context[i2c_num].hal.dev, i2c_conf->slave.slave_addr, i2c_conf->slave.addr_10bit_en);
         i2c_ll_set_rxfifo_full_thr(i2c_context[i2c_num].hal.dev, I2C_FIFO_FULL_THRESH_VAL);
         i2c_ll_set_txfifo_empty_thr(i2c_context[i2c_num].hal.dev, I2C_FIFO_EMPTY_THRESH_VAL);
@@ -768,7 +793,9 @@ esp_err_t i2c_param_config(i2c_port_t i2c_num, const i2c_config_t *i2c_conf)
         i2c_hal_master_init(&(i2c_context[i2c_num].hal));
         //Default, we enable hardware filter
         i2c_ll_master_set_filter(i2c_context[i2c_num].hal.dev, I2C_FILTER_CYC_NUM_DEF);
-        i2c_hal_set_bus_timing(&(i2c_context[i2c_num].hal), i2c_conf->master.clk_speed, src_clk, s_get_src_clk_freq(src_clk));
+        I2C_CLOCK_SRC_ATOMIC() {
+            i2c_hal_set_bus_timing(&(i2c_context[i2c_num].hal), i2c_conf->master.clk_speed, src_clk, s_get_src_clk_freq(src_clk));
+        }
     }
     i2c_ll_update(i2c_context[i2c_num].hal.dev);
     I2C_EXIT_CRITICAL(&(i2c_context[i2c_num].spinlock));

components/driver/i2c/i2c_common.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -56,9 +56,13 @@ static esp_err_t s_i2c_bus_handle_aquire(i2c_port_num_t port_num, i2c_bus_handle
             bus->bus_mode = mode;
 
             // Enable the I2C module
-            periph_module_enable(i2c_periph_signal[port_num].module);
-            periph_module_reset(i2c_periph_signal[port_num].module);
-            i2c_hal_init(&bus->hal, port_num);
+            I2C_RCC_ATOMIC() {
+                i2c_ll_enable_bus_clock(bus->port_num, true);
+                i2c_ll_reset_register(bus->port_num);
+            }
+            I2C_CLOCK_SRC_ATOMIC() {
+                i2c_hal_init(&bus->hal, port_num);
+            }
         }
     } else {
         ESP_LOGE(TAG, "I2C bus id(%d) has already been acquired", port_num);
@@ -131,7 +135,9 @@ esp_err_t i2c_release_bus_handle(i2c_bus_handle_t i2c_bus)
                 ESP_RETURN_ON_ERROR(esp_pm_lock_delete(i2c_bus->pm_lock), TAG, "delete pm_lock failed");
             }
             // Disable I2C module
-            periph_module_disable(i2c_periph_signal[port_num].module);
+            I2C_RCC_ATOMIC() {
+                i2c_ll_enable_bus_clock(port_num, false);
+            }
             free(i2c_bus);
         }
     }

components/driver/i2c/i2c_master.c

@@ -92,8 +92,9 @@ static esp_err_t s_i2c_hw_fsm_reset(i2c_master_bus_handle_t i2c_master)
 
     //to reset the I2C hw module, we need re-enable the hw
     s_i2c_master_clear_bus(i2c_master->base);
-    periph_module_disable(i2c_periph_signal[i2c_master->base->port_num].module);
-    periph_module_enable(i2c_periph_signal[i2c_master->base->port_num].module);
+    I2C_RCC_ATOMIC() {
+        i2c_ll_reset_register(i2c_master->base->port_num);
+    }
 
     i2c_hal_master_init(hal);
     i2c_ll_disable_intr_mask(hal->dev, I2C_LL_INTR_MASK);
@@ -107,6 +108,18 @@ static esp_err_t s_i2c_hw_fsm_reset(i2c_master_bus_handle_t i2c_master)
     return ESP_OK;
 }
 
+static void s_i2c_err_log_print(i2c_master_event_t event, bool bypass_nack_log)
+{
+    if (event == I2C_EVENT_TIMEOUT) {
+        ESP_LOGE(TAG, "I2C transaction timeout detected");
+    }
+    if (bypass_nack_log != true) {
+        if (event == I2C_EVENT_NACK) {
+            ESP_LOGE(TAG, "I2C transaction unexpected nack detected");
+        }
+    }
+}
+
 //////////////////////////////////////I2C operation functions////////////////////////////////////////////
 /**
  * @brief This function is used to send I2C write command, which is divided in two parts.
@@ -405,6 +418,9 @@ static void s_i2c_send_commands(i2c_master_bus_handle_t i2c_master, TickType_t t
             // Software timeout, clear the command link and finish this transaction.
             i2c_master->cmd_idx = 0;
             i2c_master->trans_idx = 0;
+            atomic_store(&i2c_master->status, I2C_STATUS_TIMEOUT);
+            ESP_LOGE(TAG, "I2C software timeout");
+            xSemaphoreGive(i2c_master->cmd_semphr);
             return;
         }
 
@@ -424,7 +440,9 @@ static void s_i2c_send_commands(i2c_master_bus_handle_t i2c_master, TickType_t t
             };
             i2c_ll_master_write_cmd_reg(hal->dev, hw_stop_cmd, 0);
             i2c_hal_master_trans_start(hal);
-            return;
+            // The master trans start would start a transaction.
+            // Queue wait for the event instead of return directly.
+            break;
         }
 
         i2c_operation_t *i2c_operation = &i2c_master->i2c_trans.ops[i2c_master->trans_idx];
@@ -446,6 +464,7 @@ static void s_i2c_send_commands(i2c_master_bus_handle_t i2c_master, TickType_t t
         if (event == I2C_EVENT_DONE) {
             atomic_store(&i2c_master->status, I2C_STATUS_DONE);
         }
+        s_i2c_err_log_print(event, i2c_master->bypass_nack_log);
     } else {
         i2c_master->cmd_idx = 0;
         i2c_master->trans_idx = 0;
@@ -530,7 +549,11 @@ static esp_err_t s_i2c_transaction_start(i2c_master_dev_handle_t i2c_dev, int xf
     i2c_master->read_len_static = 0;
 
     i2c_hal_master_set_scl_timeout_val(hal, i2c_dev->scl_wait_us, i2c_master->base->clk_src_freq_hz);
-    i2c_hal_set_bus_timing(hal, i2c_dev->scl_speed_hz, i2c_master->base->clk_src, i2c_master->base->clk_src_freq_hz);
+
+    I2C_CLOCK_SRC_ATOMIC() {
+        i2c_ll_set_source_clk(hal->dev, i2c_master->base->clk_src);
+        i2c_hal_set_bus_timing(hal, i2c_dev->scl_speed_hz, i2c_master->base->clk_src, i2c_master->base->clk_src_freq_hz);
+    }
     i2c_ll_master_set_fractional_divider(hal->dev, 0, 0);
     i2c_ll_update(hal->dev);
 
@@ -563,7 +586,6 @@ static esp_err_t s_i2c_transaction_start(i2c_master_dev_handle_t i2c_dev, int xf
 IRAM_ATTR static void i2c_isr_receive_handler(i2c_master_bus_t *i2c_master)
 {
     i2c_hal_context_t *hal = &i2c_master->base->hal;
-    while(i2c_ll_is_bus_busy(hal->dev)){}
     if (i2c_master->status == I2C_STATUS_READ) {
         i2c_operation_t *i2c_operation = &i2c_master->i2c_trans.ops[i2c_master->trans_idx];
         portENTER_CRITICAL_ISR(&i2c_master->base->spinlock);
@@ -624,7 +646,9 @@ static void IRAM_ATTR i2c_master_isr_handler_default(void *arg)
         xQueueSendFromISR(i2c_master->event_queue, (void *)&i2c_master->event, &HPTaskAwoken);
     }
     if (i2c_master->contains_read == true) {
-        i2c_isr_receive_handler(i2c_master);
+        if (int_mask & I2C_LL_INTR_MST_COMPLETE || int_mask & I2C_LL_INTR_END_DETECT) {
+            i2c_isr_receive_handler(i2c_master);
+        }
     }
 
     if (i2c_master->async_trans) {
@@ -1092,6 +1116,8 @@ esp_err_t i2c_master_probe(i2c_master_bus_handle_t bus_handle, uint16_t address,
     bus_handle->cmd_idx = 0;
     bus_handle->trans_idx = 0;
     bus_handle->trans_done = false;
+    bus_handle->status = I2C_STATUS_IDLE;
+    bus_handle->bypass_nack_log = true;
     i2c_hal_context_t *hal = &bus_handle->base->hal;
     i2c_operation_t i2c_ops[] = {
         {.hw_cmd = I2C_TRANS_START_COMMAND},
@@ -1106,7 +1132,10 @@ esp_err_t i2c_master_probe(i2c_master_bus_handle_t bus_handle, uint16_t address,
 
     // I2C probe does not have i2c device module. So set the clock parameter independently
     // This will not influence device transaction.
-    i2c_hal_set_bus_timing(hal, 100000, bus_handle->base->clk_src, bus_handle->base->clk_src_freq_hz);
+    I2C_CLOCK_SRC_ATOMIC() {
+        i2c_ll_set_source_clk(hal->dev, bus_handle->base->clk_src);
+        i2c_hal_set_bus_timing(hal, 100000, bus_handle->base->clk_src, bus_handle->base->clk_src_freq_hz);
+    }
     i2c_ll_master_set_fractional_divider(hal->dev, 0, 0);
     i2c_ll_enable_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR);
     i2c_ll_update(hal->dev);
@@ -1121,6 +1150,7 @@ esp_err_t i2c_master_probe(i2c_master_bus_handle_t bus_handle, uint16_t address,
 
     // Reset the status to done, in order not influence next time transaction.
     bus_handle->status = I2C_STATUS_DONE;
+    bus_handle->bypass_nack_log = false;
     i2c_ll_disable_intr_mask(hal->dev, I2C_LL_MASTER_EVENT_INTR);
     xSemaphoreGive(bus_handle->bus_lock_mux);
     return ret;

components/driver/i2c/i2c_private.h

@@ -17,12 +17,25 @@
 #include "freertos/task.h"
 #include "freertos/ringbuf.h"
 #include "driver/i2c_slave.h"
+#include "esp_private/periph_ctrl.h"
 #include "esp_pm.h"
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
+#if SOC_PERIPH_CLK_CTRL_SHARED
+#define I2C_CLOCK_SRC_ATOMIC() PERIPH_RCC_ATOMIC()
+#else
+#define I2C_CLOCK_SRC_ATOMIC()
+#endif
+
+#if !SOC_RCC_IS_INDEPENDENT
+#define I2C_RCC_ATOMIC() PERIPH_RCC_ATOMIC()
+#else
+#define I2C_RCC_ATOMIC()
+#endif
+
 #if CONFIG_I2C_ISR_IRAM_SAFE
 #define I2C_MEM_ALLOC_CAPS    (MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT)
 #else
@@ -126,7 +139,8 @@ struct i2c_master_bus_t {
     bool trans_over_buffer;                                          // Data length is more than hardware fifo length, needs interrupt.
     bool async_trans;                                                // asynchronous transaction, true after callback is installed.
     bool ack_check_disable;                                          // Disable ACK check
-    volatile bool trans_done;                                                 // transaction command finish
+    volatile bool trans_done;                                        // transaction command finish
+    bool bypass_nack_log;                                             // Bypass the error log. Sometimes the error is expected.
     SLIST_HEAD(i2c_master_device_list_head, i2c_master_device_list) device_list;      // I2C device (instance) list
     // asnyc trans members
     bool async_break;                                                // break transaction loop flag.

components/driver/i2c/i2c_slave.c

@@ -247,7 +247,9 @@ esp_err_t i2c_new_slave_device(const i2c_slave_config_t *slave_config, i2c_slave
 #endif
 
     //Default, we enable hardware filter
-    i2c_ll_set_source_clk(hal->dev, slave_config->clk_source);
+    I2C_CLOCK_SRC_ATOMIC() {
+        i2c_ll_set_source_clk(hal->dev, slave_config->clk_source);
+    }
     bool addr_10bit_en = slave_config->addr_bit_len != I2C_ADDR_BIT_LEN_7;
     i2c_ll_set_slave_addr(hal->dev, slave_config->slave_addr, addr_10bit_en);
 #if SOC_I2C_SLAVE_SUPPORT_BROADCAST

components/driver/test_apps/i2c_test_apps/main/test_i2c_common.c

@@ -167,6 +167,41 @@ TEST_CASE("I2C master probe device test", "[i2c]")
     TEST_ESP_OK(i2c_del_master_bus(bus_handle));
 }
 
+TEST_CASE("probe test after general call (0x00 0x06)", "[i2c]")
+{
+    uint8_t data_wr[1] = { 0x06 };
+
+    i2c_master_bus_config_t i2c_mst_config = {
+        .clk_source = I2C_CLK_SRC_DEFAULT,
+        .i2c_port = TEST_I2C_PORT,
+        .scl_io_num = I2C_MASTER_SCL_IO,
+        .sda_io_num = I2C_MASTER_SDA_IO,
+        .flags.enable_internal_pullup = true,
+    };
+    i2c_master_bus_handle_t bus_handle;
+
+    TEST_ESP_OK(i2c_new_master_bus(&i2c_mst_config, &bus_handle));
+
+    i2c_device_config_t dev_cfg1 = {
+        .dev_addr_length = I2C_ADDR_BIT_LEN_7,
+        .device_address = 0x00,
+        .scl_speed_hz = 100000,
+    };
+
+    i2c_master_dev_handle_t dev_handle1;
+    TEST_ESP_OK(i2c_master_bus_add_device(bus_handle, &dev_cfg1, &dev_handle1));
+
+    TEST_ESP_ERR(ESP_ERR_INVALID_STATE, i2c_master_transmit(dev_handle1, data_wr, 1, 200));
+
+    for (int i = 1; i < 0x7f; i++) {
+        TEST_ESP_ERR(ESP_ERR_NOT_FOUND, i2c_master_probe(bus_handle, i, 800));
+    }
+
+    TEST_ESP_OK(i2c_master_bus_rm_device(dev_handle1));
+
+    TEST_ESP_OK(i2c_del_master_bus(bus_handle));
+}
+
 #define LENGTH 48
 
 static IRAM_ATTR bool test_master_tx_done_callback(i2c_master_dev_handle_t i2c_dev, const i2c_master_event_data_t *evt_data, void *arg)