From c0e5f2b73a65acc87d24d5023f968f6b24617d61 Mon Sep 17 00:00:00 2001 From: Cao Sen Miao Date: Mon, 18 Mar 2024 20:21:40 +0800 Subject: [PATCH] fix(i2c_master): Fix issue that i2c clock got wrong after reset, Closes https://github.com/espressif/esp-idf/issues/13397 --- components/esp_driver_i2c/i2c_master.c | 2 + .../i2c_test_apps/main/test_i2c_multi.c | 96 +++++++++++++++++++ components/hal/esp32/include/hal/i2c_ll.h | 38 ++++++++ components/hal/esp32c2/include/hal/i2c_ll.h | 44 +++++++++ components/hal/esp32c3/include/hal/i2c_ll.h | 44 +++++++++ components/hal/esp32s2/include/hal/i2c_ll.h | 39 ++++++++ components/hal/esp32s3/include/hal/i2c_ll.h | 44 +++++++++ components/hal/i2c_hal.c | 10 +- components/hal/include/hal/i2c_hal.h | 16 +++- components/hal/include/hal/i2c_types.h | 3 +- 10 files changed, 333 insertions(+), 3 deletions(-) diff --git a/components/esp_driver_i2c/i2c_master.c b/components/esp_driver_i2c/i2c_master.c index 9a11338384..7b0a571101 100644 --- a/components/esp_driver_i2c/i2c_master.c +++ b/components/esp_driver_i2c/i2c_master.c @@ -829,6 +829,8 @@ static esp_err_t s_i2c_synchronous_transaction(i2c_master_dev_handle_t i2c_dev, i2c_dev->master_bus->queue_trans = false; i2c_dev->master_bus->ack_check_disable = i2c_dev->ack_check_disable; ESP_GOTO_ON_ERROR(s_i2c_transaction_start(i2c_dev, timeout_ms), err, TAG, "I2C transaction failed"); + xSemaphoreGive(i2c_dev->master_bus->bus_lock_mux); + return ret; err: // When error occurs, reset hardware fsm in case not influence following transactions. diff --git a/components/esp_driver_i2c/test_apps/i2c_test_apps/main/test_i2c_multi.c b/components/esp_driver_i2c/test_apps/i2c_test_apps/main/test_i2c_multi.c index 292b6612ce..ffd8a96a53 100644 --- a/components/esp_driver_i2c/test_apps/i2c_test_apps/main/test_i2c_multi.c +++ b/components/esp_driver_i2c/test_apps/i2c_test_apps/main/test_i2c_multi.c @@ -25,6 +25,13 @@ #include "esp_log.h" #include "test_utils.h" #include "test_board.h" +// For clock checking +#include "hal/uart_hal.h" +#include "soc/uart_periph.h" +#include "hal/clk_tree_hal.h" +#include "esp_private/gpio.h" +#include "hal/uart_ll.h" +#include "esp_clk_tree.h" void disp_buf(uint8_t *buf, int len) { @@ -689,3 +696,92 @@ static void i2c_slave_read_test_more_port(void) TEST_CASE_MULTIPLE_DEVICES("I2C master write slave test, more ports", "[i2c][test_env=generic_multi_device][timeout=150]", i2c_master_write_test_more_port, i2c_slave_read_test_more_port); #endif + +#if CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32S3 +// For now, we tested the chip which has such problem. +// This test can be extended to all chip when how uart baud rate +// works has been figured out. + +#if SOC_RCC_IS_INDEPENDENT +#define HP_UART_BUS_CLK_ATOMIC() +#else +#define HP_UART_BUS_CLK_ATOMIC() PERIPH_RCC_ATOMIC() +#endif + +//Init uart baud rate detection +static void uart_aut_baud_det_init(int rxd_io_num) +{ + gpio_func_sel(rxd_io_num, PIN_FUNC_GPIO); + gpio_set_direction(rxd_io_num, GPIO_MODE_INPUT); + gpio_pullup_en(rxd_io_num); + esp_rom_gpio_connect_in_signal(rxd_io_num, UART_PERIPH_SIGNAL(1, SOC_UART_RX_PIN_IDX), 0); + HP_UART_BUS_CLK_ATOMIC() { + uart_ll_enable_bus_clock(1, true); + uart_ll_reset_register(1); + } + /* Reset all the bits */ + uart_ll_disable_intr_mask(&UART1, ~0); + uart_ll_clr_intsts_mask(&UART1, ~0); + uart_ll_set_autobaud_en(&UART1, true); +} + +static void i2c_master_write_fsm_reset(void) +{ + uint8_t data_wr[3] = { 0 }; + + 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_cfg = { + .dev_addr_length = I2C_ADDR_BIT_LEN_7, + .device_address = 0x58, + .scl_speed_hz = 10000, // Not a typical value for I2C + }; + + i2c_master_dev_handle_t dev_handle; + TEST_ESP_OK(i2c_master_bus_add_device(bus_handle, &dev_cfg, &dev_handle)); + + // Nack will reset the bus + TEST_ESP_ERR(ESP_ERR_INVALID_STATE, i2c_master_transmit(dev_handle, data_wr, 3, -1)); + + unity_send_signal("i2c transmit fail--connect uart"); + unity_wait_for_signal("uart connected"); + + TEST_ESP_ERR(ESP_ERR_INVALID_STATE, i2c_master_transmit(dev_handle, data_wr, 3, -1)); + + unity_send_signal("i2c transmit after fsm reset"); + TEST_ESP_OK(i2c_master_bus_rm_device(dev_handle)); + + TEST_ESP_OK(i2c_del_master_bus(bus_handle)); +} + +static void uart_test_i2c_master_freq(void) +{ + unity_wait_for_signal("i2c transmit fail--connect uart"); + uart_aut_baud_det_init(I2C_MASTER_SCL_IO); + + unity_send_signal("uart connected"); + unity_wait_for_signal("i2c transmit after fsm reset"); + int pospulse_cnt = uart_ll_get_pos_pulse_cnt(&UART1); + int negpulse_cnt = uart_ll_get_neg_pulse_cnt(&UART1); + // Uart uses XTAL as default clock source + int freq_hz = (clk_hal_xtal_get_freq_mhz() * 1 * 1000 * 1000) / (pospulse_cnt + negpulse_cnt); + printf("The tested I2C SCL frequency is %d\n", freq_hz); + TEST_ASSERT_INT_WITHIN(500, 10000, freq_hz); + uart_ll_set_autobaud_en(&UART1, false); + HP_UART_BUS_CLK_ATOMIC() { + uart_ll_enable_bus_clock(1, false); + } +} + +TEST_CASE_MULTIPLE_DEVICES("I2C master clock frequency test", "[i2c][test_env=generic_multi_device][timeout=150]", uart_test_i2c_master_freq, i2c_master_write_fsm_reset); + +#endif // CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32S3 diff --git a/components/hal/esp32/include/hal/i2c_ll.h b/components/hal/esp32/include/hal/i2c_ll.h index 19884d4b89..640dea4c13 100644 --- a/components/hal/esp32/include/hal/i2c_ll.h +++ b/components/hal/esp32/include/hal/i2c_ll.h @@ -148,6 +148,44 @@ static inline void i2c_ll_master_set_fractional_divider(i2c_dev_t *hw, uint8_t d // Not supported on ESP32 } +/** + * @brief Set fractional divider + * + * @param hw Beginning address of the peripheral registers + * @param div_a The denominator of the frequency divider factor of the i2c function clock + * @param div_b The numerator of the frequency divider factor of the i2c function clock. + */ +static inline void i2c_ll_master_get_fractional_divider(i2c_dev_t *hw, uint32_t *div_a, uint32_t *div_b) +{ + // Not supported on ESP32 +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_save_clock_configurations(i2c_dev_t *hw, uint32_t *div_num, uint8_t *clk_sel, uint8_t *clk_active) +{ + // Not supported on ESP32 +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_restore_clock_configurations(i2c_dev_t *hw, uint32_t div_num, uint8_t clk_sel, uint8_t clk_active) +{ + // Not supported on ESP32 +} + /** * @brief Reset I2C txFIFO * diff --git a/components/hal/esp32c2/include/hal/i2c_ll.h b/components/hal/esp32c2/include/hal/i2c_ll.h index 1624d7d2c3..245f44fe87 100644 --- a/components/hal/esp32c2/include/hal/i2c_ll.h +++ b/components/hal/esp32c2/include/hal/i2c_ll.h @@ -190,6 +190,50 @@ static inline void i2c_ll_master_set_fractional_divider(i2c_dev_t *hw, uint8_t d HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_b, div_b); } +/** + * @brief Set fractional divider + * + * @param hw Beginning address of the peripheral registers + * @param div_a The denominator of the frequency divider factor of the i2c function clock + * @param div_b The numerator of the frequency divider factor of the i2c function clock. + */ +static inline void i2c_ll_master_get_fractional_divider(i2c_dev_t *hw, uint32_t *div_a, uint32_t *div_b) +{ + /* Set div_a and div_b to 0, as it's not necessary to use them */ + *div_a = hw->clk_conf.sclk_div_a; + *div_b = hw->clk_conf.sclk_div_b; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_save_clock_configurations(i2c_dev_t *hw, uint32_t *div_num, uint8_t *clk_sel, uint8_t *clk_active) +{ + *div_num = HAL_FORCE_READ_U32_REG_FIELD(hw->clk_conf, sclk_div_num); + *clk_sel = hw->clk_conf.sclk_sel; + *clk_active = hw->clk_conf.sclk_active; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_restore_clock_configurations(i2c_dev_t *hw, uint32_t div_num, uint8_t clk_sel, uint8_t clk_active) +{ + HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_num, div_num); + hw->clk_conf.sclk_sel = clk_sel; + hw->clk_conf.sclk_active = clk_active; +} + /** * @brief Reset I2C txFIFO * diff --git a/components/hal/esp32c3/include/hal/i2c_ll.h b/components/hal/esp32c3/include/hal/i2c_ll.h index 1423ca7978..97530cb9a1 100644 --- a/components/hal/esp32c3/include/hal/i2c_ll.h +++ b/components/hal/esp32c3/include/hal/i2c_ll.h @@ -204,6 +204,50 @@ static inline void i2c_ll_master_set_fractional_divider(i2c_dev_t *hw, uint8_t d HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_b, div_b); } +/** + * @brief Set fractional divider + * + * @param hw Beginning address of the peripheral registers + * @param div_a The denominator of the frequency divider factor of the i2c function clock + * @param div_b The numerator of the frequency divider factor of the i2c function clock. + */ +static inline void i2c_ll_master_get_fractional_divider(i2c_dev_t *hw, uint32_t *div_a, uint32_t *div_b) +{ + /* Set div_a and div_b to 0, as it's not necessary to use them */ + *div_a = hw->clk_conf.sclk_div_a; + *div_b = hw->clk_conf.sclk_div_b; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_save_clock_configurations(i2c_dev_t *hw, uint32_t *div_num, uint8_t *clk_sel, uint8_t *clk_active) +{ + *div_num = HAL_FORCE_READ_U32_REG_FIELD(hw->clk_conf, sclk_div_num); + *clk_sel = hw->clk_conf.sclk_sel; + *clk_active = hw->clk_conf.sclk_active; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_restore_clock_configurations(i2c_dev_t *hw, uint32_t div_num, uint8_t clk_sel, uint8_t clk_active) +{ + HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_num, div_num); + hw->clk_conf.sclk_sel = clk_sel; + hw->clk_conf.sclk_active = clk_active; +} + /** * @brief Reset I2C txFIFO * diff --git a/components/hal/esp32s2/include/hal/i2c_ll.h b/components/hal/esp32s2/include/hal/i2c_ll.h index a04d15db64..f30193dee1 100644 --- a/components/hal/esp32s2/include/hal/i2c_ll.h +++ b/components/hal/esp32s2/include/hal/i2c_ll.h @@ -135,6 +135,45 @@ static inline void i2c_ll_master_set_fractional_divider(i2c_dev_t *hw, uint8_t d // Not supported on ESP32S2 } +/** + * @brief Set fractional divider + * + * @param hw Beginning address of the peripheral registers + * @param div_a The denominator of the frequency divider factor of the i2c function clock + * @param div_b The numerator of the frequency divider factor of the i2c function clock. + */ +static inline void i2c_ll_master_get_fractional_divider(i2c_dev_t *hw, uint32_t *div_a, uint32_t *div_b) +{ + // Not supported on ESP32S2 +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_save_clock_configurations(i2c_dev_t *hw, uint32_t *div_num, uint8_t *clk_sel, uint8_t *clk_active) +{ + // Not supported on ESP32S2 +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_restore_clock_configurations(i2c_dev_t *hw, uint32_t div_num, uint8_t clk_sel, uint8_t clk_active) +{ + // Not supported on ESP32S2 +} + + /** * @brief Reset I2C txFIFO * diff --git a/components/hal/esp32s3/include/hal/i2c_ll.h b/components/hal/esp32s3/include/hal/i2c_ll.h index fcee10b115..d0bda86b50 100644 --- a/components/hal/esp32s3/include/hal/i2c_ll.h +++ b/components/hal/esp32s3/include/hal/i2c_ll.h @@ -212,6 +212,50 @@ static inline void i2c_ll_master_set_fractional_divider(i2c_dev_t *hw, uint8_t d HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_b, div_b); } +/** + * @brief Set fractional divider + * + * @param hw Beginning address of the peripheral registers + * @param div_a The denominator of the frequency divider factor of the i2c function clock + * @param div_b The numerator of the frequency divider factor of the i2c function clock. + */ +static inline void i2c_ll_master_get_fractional_divider(i2c_dev_t *hw, uint32_t *div_a, uint32_t *div_b) +{ + /* Set div_a and div_b to 0, as it's not necessary to use them */ + *div_a = hw->clk_conf.sclk_div_a; + *div_b = hw->clk_conf.sclk_div_b; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_save_clock_configurations(i2c_dev_t *hw, uint32_t *div_num, uint8_t *clk_sel, uint8_t *clk_active) +{ + *div_num = HAL_FORCE_READ_U32_REG_FIELD(hw->clk_conf, sclk_div_num); + *clk_sel = hw->clk_conf.sclk_sel; + *clk_active = hw->clk_conf.sclk_active; +} + +/** + * @brief Get clock configurations from registers + * + * @param hw Beginning address of the peripheral registers + * @param div_num div_num + * @param clk_sel clk_sel + * @param clk_active clk_active + */ +static inline void i2c_ll_master_restore_clock_configurations(i2c_dev_t *hw, uint32_t div_num, uint8_t clk_sel, uint8_t clk_active) +{ + HAL_FORCE_MODIFY_U32_REG_FIELD(hw->clk_conf, sclk_div_num, div_num); + hw->clk_conf.sclk_sel = clk_sel; + hw->clk_conf.sclk_active = clk_active; +} + /** * @brief Reset I2C txFIFO * diff --git a/components/hal/i2c_hal.c b/components/hal/i2c_hal.c index 92d64091c5..89bd76a908 100644 --- a/components/hal/i2c_hal.c +++ b/components/hal/i2c_hal.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 */ @@ -58,6 +58,8 @@ void _i2c_hal_deinit(i2c_hal_context_t *hal) hal->dev = NULL; } +#if !SOC_I2C_SUPPORT_HW_FSM_RST + void i2c_hal_get_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t *timing_config) { i2c_ll_get_scl_clk_timing(hal->dev, &timing_config->high_period, &timing_config->low_period, &timing_config->wait_high_period); @@ -65,6 +67,8 @@ void i2c_hal_get_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t * i2c_ll_get_stop_timing(hal->dev, &timing_config->stop_setup, &timing_config->stop_hold); i2c_ll_get_sda_timing(hal->dev, &timing_config->sda_sample, &timing_config->sda_hold); i2c_ll_get_tout(hal->dev, &timing_config->timeout); + i2c_ll_master_save_clock_configurations(hal->dev, &timing_config->clk_cfg.clk_div.integer, &timing_config->clk_cfg.clk_sel, &timing_config->clk_cfg.clk_active); + i2c_ll_master_get_fractional_divider(hal->dev, &timing_config->clk_cfg.clk_div.numerator, &timing_config->clk_cfg.clk_div.denominator); } void i2c_hal_set_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t *timing_config) @@ -74,8 +78,12 @@ void i2c_hal_set_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t * i2c_ll_master_set_stop_timing(hal->dev, timing_config->stop_setup, timing_config->stop_hold); i2c_ll_set_sda_timing(hal->dev, timing_config->sda_sample, timing_config->sda_hold); i2c_ll_set_tout(hal->dev, timing_config->timeout); + i2c_ll_master_restore_clock_configurations(hal->dev, timing_config->clk_cfg.clk_div.integer, timing_config->clk_cfg.clk_sel, timing_config->clk_cfg.clk_active); + i2c_ll_master_set_fractional_divider(hal->dev, timing_config->clk_cfg.clk_div.numerator, timing_config->clk_cfg.clk_div.denominator); } +#endif // !SOC_I2C_SUPPORT_HW_FSM_RST + void i2c_hal_master_trans_start(i2c_hal_context_t *hal) { i2c_ll_update(hal->dev); diff --git a/components/hal/include/hal/i2c_hal.h b/components/hal/include/hal/i2c_hal.h index ed8a73f748..7883be4821 100644 --- a/components/hal/include/hal/i2c_hal.h +++ b/components/hal/include/hal/i2c_hal.h @@ -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 */ @@ -33,6 +33,15 @@ typedef struct { i2c_dev_t *dev; } i2c_hal_context_t; +/** + * @brief I2C hal clock configurations + */ +typedef struct { + uint8_t clk_sel; // clock select + uint8_t clk_active; // clock active + hal_utils_clk_div_t clk_div; // clock dividers +} i2c_hal_sclk_info_t; + /** * @brief Timing configuration structure. Used for I2C reset internally. */ @@ -47,6 +56,7 @@ typedef struct { int sda_sample; /*!< high_period time */ int sda_hold; /*!< sda hold time */ int timeout; /*!< timeout value */ + i2c_hal_sclk_info_t clk_cfg; /*!< clock configuration */ } i2c_hal_timing_config_t; #if SOC_I2C_SUPPORT_SLAVE @@ -160,6 +170,8 @@ void _i2c_hal_deinit(i2c_hal_context_t *hal); */ void i2c_hal_master_trans_start(i2c_hal_context_t *hal); +#if !SOC_I2C_SUPPORT_HW_FSM_RST + /** * @brief Get timing configuration * @@ -176,6 +188,8 @@ void i2c_hal_get_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t * */ void i2c_hal_set_timing_config(i2c_hal_context_t *hal, i2c_hal_timing_config_t *timing_config); +#endif // !SOC_I2C_SUPPORT_HW_FSM_RST + #endif // #if SOC_I2C_SUPPORTED #ifdef __cplusplus diff --git a/components/hal/include/hal/i2c_types.h b/components/hal/include/hal/i2c_types.h index fba2ad0bad..e264b2aff2 100644 --- a/components/hal/include/hal/i2c_types.h +++ b/components/hal/include/hal/i2c_types.h @@ -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 */ @@ -14,6 +14,7 @@ extern "C" { #include #include "soc/soc_caps.h" #include "soc/clk_tree_defs.h" +#include "hal/hal_utils.h" /** * @brief I2C port number, can be I2C_NUM_0 ~ (I2C_NUM_MAX-1).