From 8522bb11783210cb595fe25bf2114a8e47069979 Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Thu, 3 Mar 2022 16:55:08 +0800 Subject: [PATCH 1/2] regi2c: use safe version of spinlock, instead of ISR ver --- components/esp_hw_support/regi2c_ctrl.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/components/esp_hw_support/regi2c_ctrl.c b/components/esp_hw_support/regi2c_ctrl.c index 5b263f7c33..03500e9785 100644 --- a/components/esp_hw_support/regi2c_ctrl.c +++ b/components/esp_hw_support/regi2c_ctrl.c @@ -14,32 +14,32 @@ static portMUX_TYPE mux = portMUX_INITIALIZER_UNLOCKED; uint8_t IRAM_ATTR regi2c_ctrl_read_reg(uint8_t block, uint8_t host_id, uint8_t reg_add) { - portENTER_CRITICAL_ISR(&mux); + portENTER_CRITICAL_SAFE(&mux); uint8_t value = i2c_read_reg_raw(block, host_id, reg_add); - portEXIT_CRITICAL_ISR(&mux); + portEXIT_CRITICAL_SAFE(&mux); return value; } uint8_t IRAM_ATTR regi2c_ctrl_read_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t msb, uint8_t lsb) { - portENTER_CRITICAL_ISR(&mux); + portENTER_CRITICAL_SAFE(&mux); uint8_t value = i2c_read_reg_mask_raw(block, host_id, reg_add, msb, lsb); - portEXIT_CRITICAL_ISR(&mux); + portEXIT_CRITICAL_SAFE(&mux); return value; } void IRAM_ATTR regi2c_ctrl_write_reg(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t data) { - portENTER_CRITICAL_ISR(&mux); + portENTER_CRITICAL_SAFE(&mux); i2c_write_reg_raw(block, host_id, reg_add, data); - portEXIT_CRITICAL_ISR(&mux); + portEXIT_CRITICAL_SAFE(&mux); } void IRAM_ATTR regi2c_ctrl_write_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t msb, uint8_t lsb, uint8_t data) { - portENTER_CRITICAL_ISR(&mux); + portENTER_CRITICAL_SAFE(&mux); i2c_write_reg_mask_raw(block, host_id, reg_add, msb, lsb, data); - portEXIT_CRITICAL_ISR(&mux); + portEXIT_CRITICAL_SAFE(&mux); } /** From d378ca2b783102582ed90ee8b473f5e808feab43 Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Tue, 1 Mar 2022 16:12:45 +0800 Subject: [PATCH 2/2] esp_phy: use spinlock to avoid regi2c access conflicts --- .../esp_hw_support/port/esp32/regi2c_ctrl.h | 4 ++ .../esp_hw_support/port/esp32c3/regi2c_ctrl.h | 4 ++ .../esp_hw_support/port/esp32h2/regi2c_ctrl.h | 4 ++ .../esp_hw_support/port/esp32s2/regi2c_ctrl.h | 4 ++ .../esp_hw_support/port/esp32s3/regi2c_ctrl.h | 4 ++ components/esp_hw_support/regi2c_ctrl.c | 10 ++++ components/esp_phy/CMakeLists.txt | 31 ++++++------ components/esp_phy/src/phy_override.c | 50 +++++++++++++++++++ components/esp_wifi/src/wifi_init.c | 20 -------- 9 files changed, 96 insertions(+), 35 deletions(-) create mode 100644 components/esp_phy/src/phy_override.c diff --git a/components/esp_hw_support/port/esp32/regi2c_ctrl.h b/components/esp_hw_support/port/esp32/regi2c_ctrl.h index ea2870e99b..236e49b69e 100644 --- a/components/esp_hw_support/port/esp32/regi2c_ctrl.h +++ b/components/esp_hw_support/port/esp32/regi2c_ctrl.h @@ -52,6 +52,10 @@ uint8_t regi2c_ctrl_read_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_ad void regi2c_ctrl_write_reg(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t data); void regi2c_ctrl_write_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t msb, uint8_t lsb, uint8_t data); +/* enter the critical section that protects internal registers. Don't use it in SDK. Use the functions above. */ +void regi2c_enter_critical(void); +void regi2c_exit_critical(void); + #endif // BOOTLOADER_BUILD /* Convenience macros for the above functions, these use register definitions diff --git a/components/esp_hw_support/port/esp32c3/regi2c_ctrl.h b/components/esp_hw_support/port/esp32c3/regi2c_ctrl.h index 7e5060288c..ebef50d125 100644 --- a/components/esp_hw_support/port/esp32c3/regi2c_ctrl.h +++ b/components/esp_hw_support/port/esp32c3/regi2c_ctrl.h @@ -65,6 +65,10 @@ uint8_t regi2c_ctrl_read_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_ad void regi2c_ctrl_write_reg(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t data); void regi2c_ctrl_write_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t msb, uint8_t lsb, uint8_t data); +/* enter the critical section that protects internal registers. Don't use it in SDK. Use the functions above. */ +void regi2c_enter_critical(void); +void regi2c_exit_critical(void); + #endif // BOOTLOADER_BUILD /* Convenience macros for the above functions, these use register definitions diff --git a/components/esp_hw_support/port/esp32h2/regi2c_ctrl.h b/components/esp_hw_support/port/esp32h2/regi2c_ctrl.h index 28436f5790..1b0be51fca 100644 --- a/components/esp_hw_support/port/esp32h2/regi2c_ctrl.h +++ b/components/esp_hw_support/port/esp32h2/regi2c_ctrl.h @@ -79,6 +79,10 @@ uint8_t regi2c_ctrl_read_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_ad void regi2c_ctrl_write_reg(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t data); void regi2c_ctrl_write_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t msb, uint8_t lsb, uint8_t data); +/* enter the critical section that protects internal registers. Don't use it in SDK. Use the functions above. */ +void regi2c_enter_critical(void); +void regi2c_exit_critical(void); + #endif // BOOTLOADER_BUILD /* Convenience macros for the above functions, these use register definitions diff --git a/components/esp_hw_support/port/esp32s2/regi2c_ctrl.h b/components/esp_hw_support/port/esp32s2/regi2c_ctrl.h index 5058cbb57e..49c247499a 100644 --- a/components/esp_hw_support/port/esp32s2/regi2c_ctrl.h +++ b/components/esp_hw_support/port/esp32s2/regi2c_ctrl.h @@ -60,6 +60,10 @@ uint8_t regi2c_ctrl_read_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_ad void regi2c_ctrl_write_reg(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t data); void regi2c_ctrl_write_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t msb, uint8_t lsb, uint8_t data); +/* enter the critical section that protects internal registers. Don't use it in SDK. Use the functions above. */ +void regi2c_enter_critical(void); +void regi2c_exit_critical(void); + #endif // BOOTLOADER_BUILD /* Convenience macros for the above functions, these use register definitions diff --git a/components/esp_hw_support/port/esp32s3/regi2c_ctrl.h b/components/esp_hw_support/port/esp32s3/regi2c_ctrl.h index 385444d5fe..a4a3cbce20 100644 --- a/components/esp_hw_support/port/esp32s3/regi2c_ctrl.h +++ b/components/esp_hw_support/port/esp32s3/regi2c_ctrl.h @@ -63,6 +63,10 @@ uint8_t regi2c_ctrl_read_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_ad void regi2c_ctrl_write_reg(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t data); void regi2c_ctrl_write_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t msb, uint8_t lsb, uint8_t data); +/* enter the critical section that protects internal registers. Don't use it in SDK. Use the functions above. */ +void regi2c_enter_critical(void); +void regi2c_exit_critical(void); + #endif // BOOTLOADER_BUILD /* Convenience macros for the above functions, these use register definitions diff --git a/components/esp_hw_support/regi2c_ctrl.c b/components/esp_hw_support/regi2c_ctrl.c index 03500e9785..6ce93cc5c1 100644 --- a/components/esp_hw_support/regi2c_ctrl.c +++ b/components/esp_hw_support/regi2c_ctrl.c @@ -42,6 +42,16 @@ void IRAM_ATTR regi2c_ctrl_write_reg_mask(uint8_t block, uint8_t host_id, uint8_ portEXIT_CRITICAL_SAFE(&mux); } +void IRAM_ATTR regi2c_enter_critical(void) +{ + portENTER_CRITICAL_SAFE(&mux); +} + +void IRAM_ATTR regi2c_exit_critical(void) +{ + portEXIT_CRITICAL_SAFE(&mux); +} + /** * Restore regi2c analog calibration related configuration registers. * This is a workaround, and is fixed on later chips diff --git a/components/esp_phy/CMakeLists.txt b/components/esp_phy/CMakeLists.txt index 5943808bbe..b16bbd155f 100644 --- a/components/esp_phy/CMakeLists.txt +++ b/components/esp_phy/CMakeLists.txt @@ -1,5 +1,7 @@ idf_build_get_property(idf_target IDF_TARGET) +set(srcs "src/phy_override.c" "src/lib_printf.c") + if(CONFIG_ESP32_NO_BLOBS OR CONFIG_ESP32S2_NO_BLOBS) set(link_binary_libs 0) set(ldfragments) @@ -9,11 +11,10 @@ else() endif() if(IDF_TARGET STREQUAL "esp32h2") - set(srcs "src/phy_init_esp32hxx.c") + list(APPEND srcs "src/phy_init_esp32hxx.c") else() - set(srcs "src/phy_init.c") + list(APPEND srcs "src/phy_init.c") endif() - list(APPEND srcs "src/lib_printf.c") idf_build_get_property(build_dir BUILD_DIR) @@ -24,23 +25,23 @@ if(CONFIG_ESP_PHY_MULTIPLE_INIT_DATA_BIN) endif() if(CONFIG_ESP_PHY_MULTIPLE_INIT_DATA_BIN_EMBED) - idf_component_register(SRCS "${srcs}" - INCLUDE_DIRS "include" "${idf_target}/include" - PRIV_REQUIRES nvs_flash - LDFRAGMENTS "${ldfragments}" - EMBED_FILES "${build_dir}/phy_multiple_init_data.bin" - ) -else() - idf_component_register(SRCS "${srcs}" - INCLUDE_DIRS "include" "${idf_target}/include" - PRIV_REQUIRES nvs_flash - LDFRAGMENTS "${ldfragments}" - ) + set(embed_files "${build_dir}/phy_multiple_init_data.bin") endif() +# [refactor-todo]: requires "driver" component for adc_power_acquire/release() +idf_component_register(SRCS "${srcs}" + INCLUDE_DIRS "include" "${idf_target}/include" + PRIV_REQUIRES nvs_flash driver + LDFRAGMENTS "${ldfragments}" + EMBED_FILES ${embed_files} + ) + set(target_name "${idf_target}") target_link_libraries(${COMPONENT_LIB} PUBLIC "-L \"${CMAKE_CURRENT_SOURCE_DIR}/lib/${target_name}\"") +# Override functions in PHY lib with the functions in 'phy_override.c' +target_link_libraries(${COMPONENT_LIB} INTERFACE "-u include_esp_phy_override") + if(link_binary_libs) target_link_libraries(${COMPONENT_LIB} PUBLIC phy) diff --git a/components/esp_phy/src/phy_override.c b/components/esp_phy/src/phy_override.c new file mode 100644 index 0000000000..add05e5ac8 --- /dev/null +++ b/components/esp_phy/src/phy_override.c @@ -0,0 +1,50 @@ +/* + * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include +#include "regi2c_ctrl.h" +#include "driver/adc.h" + +/* + * This file is used to override the hooks provided by the PHY lib for some system features. + * Call phy_override() so that this file will be linked. + */ + +static bool s_wifi_adc_xpd_flag; + +void include_esp_phy_override(void) +{ + /* When this empty function is called, all functions below will be linked. */ +} + +/* Coordinate ADC power with other modules. */ +// It seems that it is only required on ESP32, but we still compile it for all chips, in case it is +// called by PHY unexpectedly. +void set_xpd_sar(bool en) +{ + if (s_wifi_adc_xpd_flag == en) { + /* ignore repeated calls to set_xpd_sar when the state is already correct */ + return; + } + + s_wifi_adc_xpd_flag = en; + if (en) { + adc_power_acquire(); + } else { + adc_power_release(); + } +} + +//add spinlock protection +void phy_i2c_enter_critical(void) +{ + regi2c_enter_critical(); +} + +void phy_i2c_exit_critical(void) +{ + regi2c_exit_critical(); +} diff --git a/components/esp_wifi/src/wifi_init.c b/components/esp_wifi/src/wifi_init.c index 6473272cd3..28f673d13b 100644 --- a/components/esp_wifi/src/wifi_init.c +++ b/components/esp_wifi/src/wifi_init.c @@ -15,7 +15,6 @@ #include "esp_wpa.h" #include "esp_netif.h" #include "tcpip_adapter_compatible/tcpip_adapter_compat.h" -#include "driver/adc.h" #include "driver/adc2_wifi_private.h" #include "esp_coexist_internal.h" #include "esp_phy_init.h" @@ -57,7 +56,6 @@ uint64_t g_wifi_feature_caps = #endif 0; -static bool s_wifi_adc_xpd_flag; static const char* TAG = "wifi_init"; @@ -288,24 +286,6 @@ void wifi_apb80m_release(void) } #endif //CONFIG_PM_ENABLE -/* Coordinate ADC power with other modules. This overrides the function from PHY lib. */ -// It seems that it is only required on ESP32, but we still compile it for all chips, in case it is -// called by PHY unexpectedly. -void set_xpd_sar(bool en) -{ - if (s_wifi_adc_xpd_flag == en) { - /* ignore repeated calls to set_xpd_sar when the state is already correct */ - return; - } - - s_wifi_adc_xpd_flag = en; - if (en) { - adc_power_acquire(); - } else { - adc_power_release(); - } -} - #ifndef CONFIG_ESP_WIFI_FTM_ENABLE void ieee80211_ftm_attach(void) {