Merge branch 'bugfix/phy_regi2c_critical_section' into 'master'

esp_phy: use spinlock to avoid regi2c access conflicts

See merge request espressif/esp-idf!17298

components/esp_hw_support/include/esp_private/regi2c_ctrl.h

@@ -34,11 +34,16 @@ extern "C" {
 
 #else
 
+/* Access internal registers, don't use in application */
 uint8_t regi2c_ctrl_read_reg(uint8_t block, uint8_t host_id, uint8_t reg_add);
 uint8_t regi2c_ctrl_read_reg_mask(uint8_t block, uint8_t host_id, uint8_t reg_add, uint8_t msb, uint8_t lsb);
 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

components/esp_hw_support/regi2c_ctrl.c

@@ -15,32 +15,42 @@ 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 = regi2c_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 = regi2c_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);
     regi2c_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);
     regi2c_write_reg_mask_raw(block, host_id, reg_add, msb, lsb, data);
-    portEXIT_CRITICAL_ISR(&mux);
+    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);
 }
 
 /**

components/esp_phy/CMakeLists.txt

@@ -5,6 +5,8 @@ if(IDF_TARGET STREQUAL "esp32c2")
     return()
 endif()
 
+set(srcs "src/phy_override.c" "src/lib_printf.c")
+
 if(CONFIG_APP_NO_BLOBS)
     set(link_binary_libs 0)
     set(ldfragments)
@@ -14,11 +16,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)
 
@@ -29,23 +30,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 efuse
-                        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 efuse
-                        LDFRAGMENTS "${ldfragments}"
-                        )
+    set(embed_files "${build_dir}/phy_multiple_init_data.bin")
 endif()
 
+# [refactor-todo]: requires "driver" component for periph_ctrl header file
+idf_component_register(SRCS "${srcs}"
+                    INCLUDE_DIRS "include" "${idf_target}/include"
+                    PRIV_REQUIRES nvs_flash driver efuse
+                    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)
 

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 <stdbool.h>
+#include "esp_private/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();
+}

components/esp_wifi/src/wifi_init.c

@@ -15,7 +15,6 @@
 #include "soc/rtc.h"
 #include "esp_wpa.h"
 #include "esp_netif.h"
-#include "driver/adc.h"
 #include "esp_coexist_internal.h"
 #include "esp_phy_init.h"
 #include "phy.h"
@@ -56,7 +55,6 @@ uint64_t g_wifi_feature_caps =
 #endif
 0;
 
-static bool s_wifi_adc_xpd_flag;
 
 static const char* TAG = "wifi_init";
 
@@ -278,24 +276,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)
 {