From 92d6c4a5023563b1cea435e6acbfc3a53012f4c6 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 77a22ae662..842bb2a8e2 100644 --- a/components/esp_hw_support/regi2c_ctrl.c +++ b/components/esp_hw_support/regi2c_ctrl.c @@ -21,30 +21,30 @@ 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 75c720bcd333600899cf27bd68b66866fc89974c 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/esp32s2/regi2c_ctrl.h | 4 ++ .../esp_hw_support/port/esp32s3/regi2c_ctrl.h | 4 ++ components/esp_hw_support/regi2c_ctrl.c | 10 ++++ components/esp_wifi/CMakeLists.txt | 56 ++++++++----------- components/esp_wifi/src/phy_override.c | 52 +++++++++++++++++ components/esp_wifi/src/wifi_init.c | 20 ------- 8 files changed, 100 insertions(+), 54 deletions(-) create mode 100644 components/esp_wifi/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 69f2919207..e4a81c1910 100644 --- a/components/esp_hw_support/port/esp32/regi2c_ctrl.h +++ b/components/esp_hw_support/port/esp32/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/esp32c3/regi2c_ctrl.h b/components/esp_hw_support/port/esp32c3/regi2c_ctrl.h index 95f2626656..802434b11d 100644 --- a/components/esp_hw_support/port/esp32c3/regi2c_ctrl.h +++ b/components/esp_hw_support/port/esp32c3/regi2c_ctrl.h @@ -87,6 +87,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 51a0a22dcb..34c26c6949 100644 --- a/components/esp_hw_support/port/esp32s2/regi2c_ctrl.h +++ b/components/esp_hw_support/port/esp32s2/regi2c_ctrl.h @@ -68,6 +68,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 b8507917a7..f38fa7afba 100644 --- a/components/esp_hw_support/port/esp32s3/regi2c_ctrl.h +++ b/components/esp_hw_support/port/esp32s3/regi2c_ctrl.h @@ -61,6 +61,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 842bb2a8e2..d902dcd3ce 100644 --- a/components/esp_hw_support/regi2c_ctrl.c +++ b/components/esp_hw_support/regi2c_ctrl.c @@ -48,3 +48,13 @@ void IRAM_ATTR regi2c_ctrl_write_reg_mask(uint8_t block, uint8_t host_id, uint8_ i2c_write_reg_mask_raw(block, host_id, reg_add, msb, lsb, data); 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); +} diff --git a/components/esp_wifi/CMakeLists.txt b/components/esp_wifi/CMakeLists.txt index 5ae2b0b999..791d906cc8 100644 --- a/components/esp_wifi/CMakeLists.txt +++ b/components/esp_wifi/CMakeLists.txt @@ -24,45 +24,33 @@ if(CONFIG_ESP32_SUPPORT_MULTIPLE_PHY_INIT_DATA_BIN) endif() if(CONFIG_ESP32_MULTIPLE_PHY_DATA_BIN_EMBEDDED) - idf_component_register(SRCS "src/coexist.c" - "src/lib_printf.c" - "src/mesh_event.c" - "src/phy_init.c" - "src/smartconfig.c" - "src/smartconfig_ack.c" - "src/wifi_init.c" - "src/wifi_default.c" - "src/wifi_netif.c" - "${idf_target}/esp_adapter.c" - INCLUDE_DIRS "include" "${idf_target}/include" - PRIV_REQUIRES wpa_supplicant nvs_flash esp_netif driver ${extra_priv_requires} - REQUIRES esp_event - PRIV_REQUIRES esp_timer esp_pm wpa_supplicant nvs_flash esp_netif ${extra_priv_requires} - LDFRAGMENTS "${ldfragments}" - EMBED_FILES "${build_dir}/phy_multiple_init_data.bin" - ) -else() - idf_component_register(SRCS "src/coexist.c" - "src/lib_printf.c" - "src/mesh_event.c" - "src/phy_init.c" - "src/smartconfig.c" - "src/smartconfig_ack.c" - "src/wifi_init.c" - "src/wifi_default.c" - "src/wifi_netif.c" - "${idf_target}/esp_adapter.c" - INCLUDE_DIRS "include" "${idf_target}/include" - PRIV_REQUIRES wpa_supplicant nvs_flash esp_netif driver ${extra_priv_requires} - REQUIRES esp_event - PRIV_REQUIRES esp_timer esp_pm wpa_supplicant nvs_flash esp_netif ${extra_priv_requires} - LDFRAGMENTS "${ldfragments}" - ) + set(embed_files "${build_dir}/phy_multiple_init_data.bin") endif() +idf_component_register(SRCS "src/coexist.c" + "src/lib_printf.c" + "src/mesh_event.c" + "src/phy_init.c" + "src/smartconfig.c" + "src/smartconfig_ack.c" + "src/wifi_init.c" + "src/wifi_default.c" + "src/wifi_netif.c" + "${idf_target}/esp_adapter.c" + "src/phy_override.c" + INCLUDE_DIRS "include" "${idf_target}/include" + PRIV_REQUIRES wpa_supplicant nvs_flash esp_netif driver esp_timer esp_pm ${extra_priv_requires} + REQUIRES esp_event + 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) set(phy phy) set(blobs coexist core espnow mesh net80211 pp smartconfig wapi ${phy}) diff --git a/components/esp_wifi/src/phy_override.c b/components/esp_wifi/src/phy_override.c new file mode 100644 index 0000000000..9a2692737f --- /dev/null +++ b/components/esp_wifi/src/phy_override.c @@ -0,0 +1,52 @@ +/* + * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include +#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 +extern void regi2c_enter_critical(void); +extern void regi2c_exit_critical(void); + +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 8319632370..0e4a6aed2d 100644 --- a/components/esp_wifi/src/wifi_init.c +++ b/components/esp_wifi/src/wifi_init.c @@ -23,7 +23,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" @@ -64,7 +63,6 @@ uint64_t g_wifi_feature_caps = #endif 0; -static bool s_wifi_adc_xpd_flag; static const char* TAG = "wifi_init"; @@ -290,24 +288,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) {