From fdc5fa2931448c862b5a69a727ecdd261d3db70a Mon Sep 17 00:00:00 2001 From: "C.S.M" Date: Wed, 28 Aug 2024 18:21:28 +0800 Subject: [PATCH] fix(i2c): Fix possible error state in clear the bus, Closes https://github.com/espressif/esp-idf/issues/13647 --- components/driver/i2c/i2c.c | 5 +++- components/driver/i2c/i2c_master.c | 28 ++++++++++++++++----- components/hal/esp32/include/hal/i2c_ll.h | 24 +++++++++++++----- components/hal/esp32c2/include/hal/i2c_ll.h | 21 ++++++++++++---- components/hal/esp32c3/include/hal/i2c_ll.h | 21 ++++++++++++---- components/hal/esp32c6/include/hal/i2c_ll.h | 23 ++++++++++++----- components/hal/esp32h2/include/hal/i2c_ll.h | 21 ++++++++++++---- components/hal/esp32p4/include/hal/i2c_ll.h | 21 ++++++++++++---- components/hal/esp32s2/include/hal/i2c_ll.h | 23 +++++++++++++---- components/hal/esp32s3/include/hal/i2c_ll.h | 15 ++++++++++- 10 files changed, 157 insertions(+), 45 deletions(-) diff --git a/components/driver/i2c/i2c.c b/components/driver/i2c/i2c.c index 658abff1e9..dee0445852 100644 --- a/components/driver/i2c/i2c.c +++ b/components/driver/i2c/i2c.c @@ -653,7 +653,10 @@ static esp_err_t i2c_master_clear_bus(i2c_port_t i2c_num) gpio_set_level(sda_io, 1); // STOP, SDA low -> high while SCL is HIGH i2c_set_pin(i2c_num, sda_io, scl_io, 1, 1, I2C_MODE_MASTER); #else - i2c_ll_master_clr_bus(i2c_context[i2c_num].hal.dev, I2C_CLR_BUS_SCL_NUM); + i2c_ll_master_clr_bus(i2c_context[i2c_num].hal.dev, I2C_CLR_BUS_SCL_NUM, true); + while (i2c_ll_master_is_bus_clear_done(i2c_context[i2c_num].hal.dev)) { + } + i2c_ll_update(i2c_context[i2c_num].hal.dev); #endif return ESP_OK; } diff --git a/components/driver/i2c/i2c_master.c b/components/driver/i2c/i2c_master.c index afbc993359..c327d703b0 100644 --- a/components/driver/i2c/i2c_master.c +++ b/components/driver/i2c/i2c_master.c @@ -44,6 +44,7 @@ static const char *TAG = "i2c.master"; static esp_err_t s_i2c_master_clear_bus(i2c_bus_handle_t handle) { + esp_err_t ret = ESP_OK; #if !SOC_I2C_SUPPORT_HW_CLR_BUS const int scl_half_period = 5; // use standard 100kHz data rate int i = 0; @@ -70,9 +71,23 @@ static esp_err_t s_i2c_master_clear_bus(i2c_bus_handle_t handle) i2c_common_set_pins(handle); #else i2c_hal_context_t *hal = &handle->hal; - i2c_ll_master_clr_bus(hal->dev, I2C_LL_RESET_SLV_SCL_PULSE_NUM_DEFAULT); + i2c_ll_master_clr_bus(hal->dev, I2C_LL_RESET_SLV_SCL_PULSE_NUM_DEFAULT, true); + // If the i2c master clear bus state machine got disturbed when its work, it would go into error state. + // The solution here is to use freertos tick counter to set time threshold. If its not return on time, + // return invalid state and turn off the state machine for avoiding its always wrong. + TickType_t start_tick = xTaskGetTickCount(); + const TickType_t timeout_ticks = pdMS_TO_TICKS(I2C_CLR_BUS_TIMEOUT_MS); + while (i2c_ll_master_is_bus_clear_done(hal->dev)) { + if ((xTaskGetTickCount() - start_tick) > timeout_ticks) { + ESP_LOGE(TAG, "clear bus failed."); + i2c_ll_master_clr_bus(hal->dev, 0, false); + ret = ESP_ERR_INVALID_STATE; + break; + } + } + i2c_ll_update(hal->dev); #endif - return ESP_OK; + return ret; } /** @@ -84,6 +99,7 @@ static esp_err_t s_i2c_master_clear_bus(i2c_bus_handle_t handle) */ static esp_err_t s_i2c_hw_fsm_reset(i2c_master_bus_handle_t i2c_master) { + esp_err_t ret = ESP_OK; i2c_hal_context_t *hal = &i2c_master->base->hal; #if !SOC_I2C_SUPPORT_HW_FSM_RST i2c_hal_timing_config_t timing_config; @@ -93,7 +109,7 @@ static esp_err_t s_i2c_hw_fsm_reset(i2c_master_bus_handle_t i2c_master) i2c_ll_master_get_filter(hal->dev, &filter_cfg); //to reset the I2C hw module, we need re-enable the hw - s_i2c_master_clear_bus(i2c_master->base); + ret = s_i2c_master_clear_bus(i2c_master->base); I2C_RCC_ATOMIC() { i2c_ll_reset_register(i2c_master->base->port_num); } @@ -105,9 +121,9 @@ static esp_err_t s_i2c_hw_fsm_reset(i2c_master_bus_handle_t i2c_master) i2c_ll_master_set_filter(hal->dev, filter_cfg); #else i2c_ll_master_fsm_rst(hal->dev); - s_i2c_master_clear_bus(i2c_master->base); + ret = s_i2c_master_clear_bus(i2c_master->base); #endif - return ESP_OK; + return ret; } static void s_i2c_err_log_print(i2c_master_event_t event, bool bypass_nack_log) @@ -536,7 +552,7 @@ static esp_err_t s_i2c_transaction_start(i2c_master_dev_handle_t i2c_dev, int xf // Sometimes when the FSM get stuck, the ACK_ERR interrupt will occur endlessly until we reset the FSM and clear bus. esp_err_t ret = ESP_OK; if (i2c_master->status == I2C_STATUS_TIMEOUT || i2c_ll_is_bus_busy(hal->dev)) { - s_i2c_hw_fsm_reset(i2c_master); + ESP_RETURN_ON_ERROR(s_i2c_hw_fsm_reset(i2c_master), TAG, "reset hardware failed"); } if (i2c_master->base->pm_lock) { diff --git a/components/hal/esp32/include/hal/i2c_ll.h b/components/hal/esp32/include/hal/i2c_ll.h index 243f732756..b5e2e7a1cb 100644 --- a/components/hal/esp32/include/hal/i2c_ll.h +++ b/components/hal/esp32/include/hal/i2c_ll.h @@ -106,7 +106,7 @@ static inline void i2c_ll_master_set_bus_timing(i2c_dev_t *hw, i2c_hal_clk_confi /* SCL period. According to the TRM, we should always subtract 1 to SCL low period */ HAL_ASSERT(bus_cfg->scl_low > 0); hw->scl_low_period.period = bus_cfg->scl_low - 1; - /* Still according to the TRM, if filter is not enbled, we have to subtract 7, + /* Still according to the TRM, if filter is not enabled, we have to subtract 7, * if SCL filter is enabled, we have to subtract: * 8 if SCL filter is between 0 and 2 (included) * 6 + SCL threshold if SCL filter is between 3 and 7 (included) @@ -547,7 +547,7 @@ static inline void i2c_ll_get_stop_timing(i2c_dev_t *hw, int *setup_time, int *h * * @param hw Beginning address of the peripheral registers * @param ptr Pointer to data buffer - * @param len Amount of data needs to be writen + * @param len Amount of data needs to be written * * @return None. */ @@ -612,7 +612,7 @@ static inline void i2c_ll_master_get_filter(i2c_dev_t *hw, uint8_t *filter_conf) } /** - * @brief Reste I2C master FSM. When the master FSM is stuck, call this function to reset the FSM + * @brief Reset I2C master FSM. When the master FSM is stuck, call this function to reset the FSM * * @param hw Beginning address of the peripheral registers * @@ -633,11 +633,23 @@ static inline void i2c_ll_master_fsm_rst(i2c_dev_t *hw) * * @return None */ -static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses) +static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses, bool enable) { ;//ESP32 do not support } +/** + * @brief Get the clear bus state + * + * @param hw Beginning address of the peripheral registers + * + * @return true: the clear bus not finish, otherwise, false. + */ +static inline bool i2c_ll_master_is_bus_clear_done(i2c_dev_t *hw) +{ + return true; +} + /** * @brief Set I2C source clock * @@ -861,7 +873,7 @@ static inline void i2c_ll_get_scl_clk_timing(i2c_dev_t *hw, int *high_period, in * @brief Configure I2C SCL timing * * @param hw Beginning address of the peripheral registers - * @param high_period The I2C SCL hight period (in core clock cycle, hight_period > 2) + * @param high_period The I2C SCL high period (in core clock cycle, hight_period > 2) * @param low_period The I2C SCL low period (in core clock cycle, low_period > 1) * @param wait_high_period The I2C SCL wait rising edge period. * @@ -1044,7 +1056,7 @@ static inline uint32_t i2c_ll_get_hw_version(i2c_dev_t *hw) * @brief Configure I2C SCL timing * * @param hw Beginning address of the peripheral registers - * @param hight_period The I2C SCL hight period (in APB cycle) + * @param hight_period The I2C SCL high period (in APB cycle) * @param low_period The I2C SCL low period (in APB cycle) * * @return None. diff --git a/components/hal/esp32c2/include/hal/i2c_ll.h b/components/hal/esp32c2/include/hal/i2c_ll.h index 1fe3d4b46a..170b8a2d45 100644 --- a/components/hal/esp32c2/include/hal/i2c_ll.h +++ b/components/hal/esp32c2/include/hal/i2c_ll.h @@ -667,18 +667,29 @@ static inline void i2c_ll_master_fsm_rst(i2c_dev_t *hw) * * @param hw Beginning address of the peripheral registers * @param slave_pulses When I2C master is IDLE, the number of pulses will be sent out. + * @param enable True to start the state machine, otherwise, false * * @return None */ -static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses) +static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses, bool enable) { hw->scl_sp_conf.scl_rst_slv_num = slave_pulses; - hw->scl_sp_conf.scl_rst_slv_en = 1; + hw->scl_sp_conf.scl_rst_slv_en = enable; hw->ctr.conf_upgate = 1; // hardware will clear scl_rst_slv_en after sending SCL pulses, - // and we should set conf_upgate bit to synchronize register value. - while (hw->scl_sp_conf.scl_rst_slv_en); - hw->ctr.conf_upgate = 1; + // and we should set conf_upgate bit to synchronize register value after this function. +} + +/** + * @brief Get the clear bus state + * + * @param hw Beginning address of the peripheral registers + * + * @return true: the clear bus not finish, otherwise, false. + */ +static inline bool i2c_ll_master_is_bus_clear_done(i2c_dev_t *hw) +{ + return hw->scl_sp_conf.scl_rst_slv_en; } /** diff --git a/components/hal/esp32c3/include/hal/i2c_ll.h b/components/hal/esp32c3/include/hal/i2c_ll.h index 5d06f2afb5..ea216a2a43 100644 --- a/components/hal/esp32c3/include/hal/i2c_ll.h +++ b/components/hal/esp32c3/include/hal/i2c_ll.h @@ -790,18 +790,29 @@ static inline void i2c_ll_master_fsm_rst(i2c_dev_t *hw) * * @param hw Beginning address of the peripheral registers * @param slave_pulses When I2C master is IDLE, the number of pulses will be sent out. + * @param enable True to start the state machine, otherwise, false * * @return None */ -static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses) +static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses, bool enable) { hw->scl_sp_conf.scl_rst_slv_num = slave_pulses; - hw->scl_sp_conf.scl_rst_slv_en = 1; + hw->scl_sp_conf.scl_rst_slv_en = enable; hw->ctr.conf_upgate = 1; // hardware will clear scl_rst_slv_en after sending SCL pulses, - // and we should set conf_upgate bit to synchronize register value. - while (hw->scl_sp_conf.scl_rst_slv_en); - hw->ctr.conf_upgate = 1; + // and we should set conf_upgate bit to synchronize register value after this function. +} + +/** + * @brief Get the clear bus state + * + * @param hw Beginning address of the peripheral registers + * + * @return true: the clear bus not finish, otherwise, false. + */ +static inline bool i2c_ll_master_is_bus_clear_done(i2c_dev_t *hw) +{ + return hw->scl_sp_conf.scl_rst_slv_en; } /** diff --git a/components/hal/esp32c6/include/hal/i2c_ll.h b/components/hal/esp32c6/include/hal/i2c_ll.h index 55de5ccdd4..1fc496cbd1 100644 --- a/components/hal/esp32c6/include/hal/i2c_ll.h +++ b/components/hal/esp32c6/include/hal/i2c_ll.h @@ -743,18 +743,29 @@ static inline void i2c_ll_master_fsm_rst(i2c_dev_t *hw) * * @param hw Beginning address of the peripheral registers * @param slave_pulses When I2C master is IDLE, the number of pulses will be sent out. + * @param enable True to start the state machine, otherwise, false * * @return None */ -static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses) +static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses, bool enable) { hw->scl_sp_conf.scl_rst_slv_num = slave_pulses; - hw->scl_sp_conf.scl_rst_slv_en = 1; - hw->ctr.conf_upgate = 1; - // hardward will clear scl_rst_slv_en after sending SCL pulses, - // and we should set conf_upgate bit to synchronize register value. - while (hw->scl_sp_conf.scl_rst_slv_en); + hw->scl_sp_conf.scl_rst_slv_en = enable; hw->ctr.conf_upgate = 1; + // hardware will clear scl_rst_slv_en after sending SCL pulses, + // and we should set conf_upgate bit to synchronize register value after this function. +} + +/** + * @brief Get the clear bus state + * + * @param hw Beginning address of the peripheral registers + * + * @return true: the clear bus not finish, otherwise, false. + */ +static inline bool i2c_ll_master_is_bus_clear_done(i2c_dev_t *hw) +{ + return hw->scl_sp_conf.scl_rst_slv_en; } /** diff --git a/components/hal/esp32h2/include/hal/i2c_ll.h b/components/hal/esp32h2/include/hal/i2c_ll.h index 7e990d6c0c..8d71d89510 100644 --- a/components/hal/esp32h2/include/hal/i2c_ll.h +++ b/components/hal/esp32h2/include/hal/i2c_ll.h @@ -737,18 +737,29 @@ static inline void i2c_ll_master_fsm_rst(i2c_dev_t *hw) * * @param hw Beginning address of the peripheral registers * @param slave_pulses When I2C master is IDLE, the number of pulses will be sent out. + * @param enable True to start the state machine, otherwise, false * * @return None */ -static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses) +static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses, bool enable) { hw->scl_sp_conf.scl_rst_slv_num = slave_pulses; - hw->scl_sp_conf.scl_rst_slv_en = 1; + hw->scl_sp_conf.scl_rst_slv_en = enable; hw->ctr.conf_upgate = 1; // hardware will clear scl_rst_slv_en after sending SCL pulses, - // and we should set conf_upgate bit to synchronize register value. - while (hw->scl_sp_conf.scl_rst_slv_en); - hw->ctr.conf_upgate = 1; + // and we should set conf_upgate bit to synchronize register value after this function. +} + +/** + * @brief Get the clear bus state + * + * @param hw Beginning address of the peripheral registers + * + * @return true: the clear bus not finish, otherwise, false. + */ +static inline bool i2c_ll_master_is_bus_clear_done(i2c_dev_t *hw) +{ + return hw->scl_sp_conf.scl_rst_slv_en; } /** diff --git a/components/hal/esp32p4/include/hal/i2c_ll.h b/components/hal/esp32p4/include/hal/i2c_ll.h index a87188b472..c97fcc6cb8 100644 --- a/components/hal/esp32p4/include/hal/i2c_ll.h +++ b/components/hal/esp32p4/include/hal/i2c_ll.h @@ -741,18 +741,29 @@ static inline void i2c_ll_master_fsm_rst(i2c_dev_t *hw) * * @param hw Beginning address of the peripheral registers * @param slave_pulses When I2C master is IDLE, the number of pulses will be sent out. + * @param enable True to start the state machine, otherwise, false * * @return None */ -static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses) +static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses, bool enable) { hw->scl_sp_conf.scl_rst_slv_num = slave_pulses; - hw->scl_sp_conf.scl_rst_slv_en = 1; + hw->scl_sp_conf.scl_rst_slv_en = enable; hw->ctr.conf_upgate = 1; // hardware will clear scl_rst_slv_en after sending SCL pulses, - // and we should set conf_upgate bit to synchronize register value. - while (hw->scl_sp_conf.scl_rst_slv_en); - hw->ctr.conf_upgate = 1; + // and we should set conf_upgate bit to synchronize register value after this function. +} + +/** + * @brief Get the clear bus state + * + * @param hw Beginning address of the peripheral registers + * + * @return true: the clear bus not finish, otherwise, false. + */ +static inline bool i2c_ll_master_is_bus_clear_done(i2c_dev_t *hw) +{ + return hw->scl_sp_conf.scl_rst_slv_en; } /** diff --git a/components/hal/esp32s2/include/hal/i2c_ll.h b/components/hal/esp32s2/include/hal/i2c_ll.h index f22accc184..1befbe3a1e 100644 --- a/components/hal/esp32s2/include/hal/i2c_ll.h +++ b/components/hal/esp32s2/include/hal/i2c_ll.h @@ -573,7 +573,7 @@ static inline void i2c_ll_get_stop_timing(i2c_dev_t *hw, int *setup_time, int *h * * @param hw Beginning address of the peripheral registers * @param ptr Pointer to data buffer - * @param len Amount of data needs to be writen + * @param len Amount of data needs to be written * * @return None. */ @@ -658,16 +658,29 @@ static inline void i2c_ll_master_fsm_rst(i2c_dev_t *hw) * * @param hw Beginning address of the peripheral registers * @param slave_pulses When I2C master is IDLE, the number of pulses will be sent out. + * @param enable True to start the state machine, otherwise, false * * @return None */ -static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses) +static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses, bool enable) { hw->scl_sp_conf.scl_rst_slv_num = slave_pulses; hw->scl_sp_conf.scl_rst_slv_en = 0; hw->scl_sp_conf.scl_rst_slv_en = 1; } +/** + * @brief Get the clear bus state + * + * @param hw Beginning address of the peripheral registers + * + * @return true: the clear bus not finish, otherwise, false. + */ +static inline bool i2c_ll_master_is_bus_clear_done(i2c_dev_t *hw) +{ + return true; // not supported on esp32s2 +} + /** * @brief Set I2C source clock * @@ -717,7 +730,7 @@ static inline void i2c_ll_master_init(i2c_dev_t *hw) * Otherwise it is not needed. * * @param hw Beginning address of the peripheral registers - * @param internal_od_ena Set true to enble internal open-drain, otherwise, set it false. + * @param internal_od_ena Set true to enable internal open-drain, otherwise, set it false. * * @return None */ @@ -894,7 +907,7 @@ typedef enum { * @brief Configure I2C SCL timing * * @param hw Beginning address of the peripheral registers - * @param high_period The I2C SCL hight period (in core clock cycle, hight_period > 2) + * @param high_period The I2C SCL high period (in core clock cycle, hight_period > 2) * @param low_period The I2C SCL low period (in core clock cycle, low_period > 1) * @param wait_high_period The I2C SCL wait rising edge period. * @@ -1082,7 +1095,7 @@ static inline void i2c_ll_slave_disable_rx_it(i2c_dev_t *hw) * @brief Configure I2C SCL timing * * @param hw Beginning address of the peripheral registers - * @param hight_period The I2C SCL hight period (in APB cycle, hight_period > 2) + * @param hight_period The I2C SCL high period (in APB cycle, hight_period > 2) * @param low_period The I2C SCL low period (in APB cycle, low_period > 1) * * @return None. diff --git a/components/hal/esp32s3/include/hal/i2c_ll.h b/components/hal/esp32s3/include/hal/i2c_ll.h index 5e3006b497..1ee702f071 100644 --- a/components/hal/esp32s3/include/hal/i2c_ll.h +++ b/components/hal/esp32s3/include/hal/i2c_ll.h @@ -797,16 +797,29 @@ static inline void i2c_ll_master_fsm_rst(i2c_dev_t *hw) * * @param hw Beginning address of the peripheral registers * @param slave_pulses When I2C master is IDLE, the number of pulses will be sent out. + * @param enable True to start the state machine, otherwise, false * * @return None */ -static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses) +static inline void i2c_ll_master_clr_bus(i2c_dev_t *hw, uint32_t slave_pulses, bool enable) { hw->scl_sp_conf.scl_rst_slv_num = slave_pulses; hw->scl_sp_conf.scl_rst_slv_en = 1; hw->ctr.conf_upgate = 1; } +/** + * @brief Get the clear bus state + * + * @param hw Beginning address of the peripheral registers + * + * @return true: the clear bus not finish, otherwise, false. + */ +static inline bool i2c_ll_master_is_bus_clear_done(i2c_dev_t *hw) +{ + return true; // not supported on esp32s3 +} + /** * @brief Set I2C source clock *