From 6ff1059da769443012e41d104b995aa83dca1b4b Mon Sep 17 00:00:00 2001 From: Chip Weinberger Date: Thu, 12 Jan 2023 03:04:42 -0800 Subject: [PATCH 1/4] [SDMMC Mount] fix infinite loop when SD card is not responsive Closes: https://github.com/espressif/esp-idf/pull/10532 --- components/driver/sdmmc/sdmmc_host.c | 61 +++++++++++++++++++++---- components/driver/sdmmc/sdmmc_private.h | 2 +- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/components/driver/sdmmc/sdmmc_host.c b/components/driver/sdmmc/sdmmc_host.c index 97b015c7cc..500e55ad73 100644 --- a/components/driver/sdmmc/sdmmc_host.c +++ b/components/driver/sdmmc/sdmmc_host.c @@ -25,6 +25,8 @@ #define SDMMC_EVENT_QUEUE_LENGTH 32 +#define SDMMC_TIMEOUT_MS 1000 + static void sdmmc_isr(void* arg); static void sdmmc_host_dma_init(void); @@ -68,16 +70,22 @@ static void configure_pin_iomux(uint8_t gpio_num); static esp_err_t sdmmc_host_pullup_en_internal(int slot, int width); -void sdmmc_host_reset(void) +esp_err_t sdmmc_host_reset(void) { // Set reset bits SDMMC.ctrl.controller_reset = 1; SDMMC.ctrl.dma_reset = 1; SDMMC.ctrl.fifo_reset = 1; + // Wait for the reset bits to be cleared by hardware + int t0 = esp_timer_get_time(); while (SDMMC.ctrl.controller_reset || SDMMC.ctrl.fifo_reset || SDMMC.ctrl.dma_reset) { - ; + if (esp_timer_get_time() - t0 > SDMMC_TIMEOUT_MS) { + return ESP_ERR_TIMEOUT; + } } + + return ESP_OK; } /* We have two clock divider stages: @@ -172,7 +180,7 @@ static void sdmmc_host_input_clk_disable(void) SDMMC.clock.val = 0; } -static void sdmmc_host_clock_update_command(int slot) +static esp_err_t sdmmc_host_clock_update_command(int slot) { // Clock update command (not a real command; just updates CIU registers) sdmmc_hw_cmd_t cmd_val = { @@ -182,8 +190,20 @@ static void sdmmc_host_clock_update_command(int slot) }; bool repeat = true; while(repeat) { - sdmmc_host_start_command(slot, cmd_val, 0); + + esp_err_t err = sdmmc_host_start_command(slot, cmd_val, 0); + if (err != ESP_OK) { + ESP_LOGE(TAG, "sdmmc_host_start_command() failed"); + return err; + } + + int t0 = esp_timer_get_time(); while (true) { + + if (esp_timer_get_time() - t0 > SDMMC_TIMEOUT_MS) { + return ESP_ERR_TIMEOUT; + } + // Sending clock update command to the CIU can generate HLE error. // According to the manual, this is okay and we must retry the command. if (SDMMC.rintsts.hle) { @@ -199,6 +219,8 @@ static void sdmmc_host_clock_update_command(int slot) } } } + + return ESP_OK; } void sdmmc_host_get_clk_dividers(const uint32_t freq_khz, int *host_div, int *card_div) @@ -245,7 +267,11 @@ esp_err_t sdmmc_host_set_card_clk(int slot, uint32_t freq_khz) // Disable clock first SDMMC.clkena.cclk_enable &= ~BIT(slot); - sdmmc_host_clock_update_command(slot); + esp_err_t err = sdmmc_host_clock_update_command(slot); + if (err != ESP_OK) { + ESP_LOGE(TAG, "disable clk failed"); + return err; + } int host_div = 0; /* clock divider of the host (SDMMC.clock) */ int card_div = 0; /* 1/2 of card clock divider (SDMMC.clkdiv) */ @@ -266,12 +292,20 @@ esp_err_t sdmmc_host_set_card_clk(int slot, uint32_t freq_khz) break; } sdmmc_host_set_clk_div(host_div); - sdmmc_host_clock_update_command(slot); + err = sdmmc_host_clock_update_command(slot); + if (err != ESP_OK) { + ESP_LOGE(TAG, "set clk div failed"); + return err; + } // Re-enable clocks SDMMC.clkena.cclk_enable |= BIT(slot); SDMMC.clkena.cclk_low_power |= BIT(slot); - sdmmc_host_clock_update_command(slot); + err = sdmmc_host_clock_update_command(slot); + if (err != ESP_OK) { + ESP_LOGE(TAG, "re-enable clk failed"); + return err; + } // set data timeout const uint32_t data_timeout_ms = 100; @@ -315,8 +349,11 @@ esp_err_t sdmmc_host_start_command(int slot, sdmmc_hw_cmd_t cmd, uint32_t arg) { /* Outputs should be synchronized to cclk_out */ cmd.use_hold_reg = 1; + int t0 = esp_timer_get_time(); while (SDMMC.cmd.start_command == 1) { - ; + if (esp_timer_get_time() - t0 > SDMMC_TIMEOUT_MS) { + return ESP_ERR_TIMEOUT; + } } SDMMC.cmdarg = arg; cmd.card_num = slot; @@ -338,7 +375,12 @@ esp_err_t sdmmc_host_init(void) sdmmc_host_set_clk_div(2); // Reset - sdmmc_host_reset(); + esp_err_t err = sdmmc_host_reset(); + if (err != ESP_OK) { + ESP_LOGE(TAG, "sdmmc_host_reset() failed"); + return err; + } + ESP_LOGD(TAG, "peripheral version %"PRIx32", hardware config %08"PRIx32, SDMMC.verid, SDMMC.hcon); // Clear interrupt status and set interrupt mask to known state @@ -545,6 +587,7 @@ esp_err_t sdmmc_host_init_slot(int slot, const sdmmc_slot_config_t* slot_config) // By default, set probing frequency (400kHz) and 1-bit bus esp_err_t ret = sdmmc_host_set_card_clk(slot, 400); if (ret != ESP_OK) { + ESP_LOGE(TAG, "sdmmc_host_set_card_clk() 400kHz failed"); return ret; } ret = sdmmc_host_set_bus_width(slot, 1); diff --git a/components/driver/sdmmc/sdmmc_private.h b/components/driver/sdmmc/sdmmc_private.h index e8442c5f2e..b065847579 100644 --- a/components/driver/sdmmc/sdmmc_private.h +++ b/components/driver/sdmmc/sdmmc_private.h @@ -18,7 +18,7 @@ typedef struct { uint32_t dma_status; ///< masked DMA interrupt status } sdmmc_event_t; -void sdmmc_host_reset(void); +esp_err_t sdmmc_host_reset(void); esp_err_t sdmmc_host_start_command(int slot, sdmmc_hw_cmd_t cmd, uint32_t arg); From c7ca30e62f510fbb8176f6fb941e7615d342eaae Mon Sep 17 00:00:00 2001 From: Chip Weinberger Date: Wed, 18 Jan 2023 15:22:07 -0800 Subject: [PATCH 2/4] [SDMMC] add reasonable timeouts to all while loops Closes: https://github.com/espressif/esp-idf/pull/10532 --- components/driver/sdmmc/sdmmc_host.c | 16 +++++++--------- components/driver/sdmmc/sdmmc_private.h | 4 ++++ components/driver/sdmmc/sdmmc_transaction.c | 1 + components/fatfs/vfs/vfs_fat_sdmmc.c | 1 - components/sdmmc/CMakeLists.txt | 2 +- components/sdmmc/sdmmc_cmd.c | 17 +++++++++++++++-- components/sdmmc/sdmmc_common.h | 3 +++ components/sdmmc/sdmmc_io.c | 4 ++++ components/sdmmc/sdmmc_sd.c | 7 ++++++- 9 files changed, 41 insertions(+), 14 deletions(-) diff --git a/components/driver/sdmmc/sdmmc_host.c b/components/driver/sdmmc/sdmmc_host.c index 500e55ad73..158e4eded8 100644 --- a/components/driver/sdmmc/sdmmc_host.c +++ b/components/driver/sdmmc/sdmmc_host.c @@ -9,6 +9,7 @@ #include #include "esp_log.h" #include "esp_intr_alloc.h" +#include "esp_timer.h" #include "soc/soc_caps.h" #include "soc/soc_pins.h" #include "soc/gpio_periph.h" @@ -25,9 +26,6 @@ #define SDMMC_EVENT_QUEUE_LENGTH 32 -#define SDMMC_TIMEOUT_MS 1000 - - static void sdmmc_isr(void* arg); static void sdmmc_host_dma_init(void); @@ -78,9 +76,9 @@ esp_err_t sdmmc_host_reset(void) SDMMC.ctrl.fifo_reset = 1; // Wait for the reset bits to be cleared by hardware - int t0 = esp_timer_get_time(); + int64_t t0 = esp_timer_get_time(); while (SDMMC.ctrl.controller_reset || SDMMC.ctrl.fifo_reset || SDMMC.ctrl.dma_reset) { - if (esp_timer_get_time() - t0 > SDMMC_TIMEOUT_MS) { + if (esp_timer_get_time() - t0 > SDMMC_HOST_RESET_TIMEOUT_US) { return ESP_ERR_TIMEOUT; } } @@ -197,10 +195,10 @@ static esp_err_t sdmmc_host_clock_update_command(int slot) return err; } - int t0 = esp_timer_get_time(); + int64_t t0 = esp_timer_get_time(); while (true) { - if (esp_timer_get_time() - t0 > SDMMC_TIMEOUT_MS) { + if (esp_timer_get_time() - t0 > SDMMC_HOST_CLOCK_UPDATE_CMD_TIMEOUT_US) { return ESP_ERR_TIMEOUT; } @@ -349,9 +347,9 @@ esp_err_t sdmmc_host_start_command(int slot, sdmmc_hw_cmd_t cmd, uint32_t arg) { /* Outputs should be synchronized to cclk_out */ cmd.use_hold_reg = 1; - int t0 = esp_timer_get_time(); + int64_t t0 = esp_timer_get_time(); while (SDMMC.cmd.start_command == 1) { - if (esp_timer_get_time() - t0 > SDMMC_TIMEOUT_MS) { + if (esp_timer_get_time() - t0 > SDMMC_HOST_START_CMD_TIMEOUT_US) { return ESP_ERR_TIMEOUT; } } diff --git a/components/driver/sdmmc/sdmmc_private.h b/components/driver/sdmmc/sdmmc_private.h index b065847579..529fa6b823 100644 --- a/components/driver/sdmmc/sdmmc_private.h +++ b/components/driver/sdmmc/sdmmc_private.h @@ -18,6 +18,10 @@ typedef struct { uint32_t dma_status; ///< masked DMA interrupt status } sdmmc_event_t; +#define SDMMC_HOST_CLOCK_UPDATE_CMD_TIMEOUT_US 1000 * 1000 +#define SDMMC_HOST_START_CMD_TIMEOUT_US 1000 * 1000 +#define SDMMC_HOST_RESET_TIMEOUT_US 5000 * 1000 + esp_err_t sdmmc_host_reset(void); esp_err_t sdmmc_host_start_command(int slot, sdmmc_hw_cmd_t cmd, uint32_t arg); diff --git a/components/driver/sdmmc/sdmmc_transaction.c b/components/driver/sdmmc/sdmmc_transaction.c index f79dd7feac..5a71479494 100644 --- a/components/driver/sdmmc/sdmmc_transaction.c +++ b/components/driver/sdmmc/sdmmc_transaction.c @@ -17,6 +17,7 @@ #include "driver/sdmmc_types.h" #include "driver/sdmmc_defs.h" #include "driver/sdmmc_host.h" +#include "esp_timer.h" #include "sdmmc_private.h" diff --git a/components/fatfs/vfs/vfs_fat_sdmmc.c b/components/fatfs/vfs/vfs_fat_sdmmc.c index 674ef3e90e..f3625a2f0e 100644 --- a/components/fatfs/vfs/vfs_fat_sdmmc.c +++ b/components/fatfs/vfs/vfs_fat_sdmmc.c @@ -88,7 +88,6 @@ static esp_err_t mount_prepare_mem(const char *base_path, if (ff_diskio_get_drive(&pdrv) != ESP_OK || pdrv == FF_DRV_NOT_USED) { ESP_LOGD(TAG, "the maximum count of volumes is already mounted"); return ESP_ERR_NO_MEM; - } // not using ff_memalloc here, as allocation in internal RAM is preferred diff --git a/components/sdmmc/CMakeLists.txt b/components/sdmmc/CMakeLists.txt index b6b1ad4f18..5de0232b67 100644 --- a/components/sdmmc/CMakeLists.txt +++ b/components/sdmmc/CMakeLists.txt @@ -6,6 +6,6 @@ idf_component_register(SRCS "sdmmc_cmd.c" "sdmmc_sd.c" INCLUDE_DIRS include REQUIRES driver - PRIV_REQUIRES soc) + PRIV_REQUIRES soc esp_timer) target_compile_options(${COMPONENT_LIB} PRIVATE "-Wno-format") diff --git a/components/sdmmc/sdmmc_cmd.c b/components/sdmmc/sdmmc_cmd.c index 450c6e6c8a..9279532104 100644 --- a/components/sdmmc/sdmmc_cmd.c +++ b/components/sdmmc/sdmmc_cmd.c @@ -3,6 +3,7 @@ * * SPDX-License-Identifier: Apache-2.0 */ +#include "esp_timer.h" #include "sdmmc_common.h" @@ -451,11 +452,16 @@ esp_err_t sdmmc_write_sectors_dma(sdmmc_card_t* card, const void* src, } uint32_t status = 0; size_t count = 0; + int64_t t0 = esp_timer_get_time(); /* SD mode: wait for the card to become idle based on R1 status */ while (!host_is_spi(card) && !(status & MMC_R1_READY_FOR_DATA)) { - // TODO: add some timeout here + if (esp_timer_get_time() - t0 > SDMMC_READY_FOR_DATA_TIMEOUT_US) { + ESP_LOGE(TAG, "write sectors dma - timeout"); + return ESP_ERR_TIMEOUT; + } err = sdmmc_send_cmd_send_status(card, &status); if (err != ESP_OK) { + ESP_LOGE(TAG, "%s: sdmmc_send_cmd_send_status returned 0x%x", __func__, err); return err; } if (++count % 10 == 0) { @@ -470,6 +476,7 @@ esp_err_t sdmmc_write_sectors_dma(sdmmc_card_t* card, const void* src, if (host_is_spi(card)) { err = sdmmc_send_cmd_send_status(card, &status); if (err != ESP_OK) { + ESP_LOGE(TAG, "%s: sdmmc_send_cmd_send_status returned 0x%x", __func__, err); return err; } if (status & SD_SPI_R2_CARD_LOCKED) { @@ -551,10 +558,15 @@ esp_err_t sdmmc_read_sectors_dma(sdmmc_card_t* card, void* dst, } uint32_t status = 0; size_t count = 0; + int64_t t0 = esp_timer_get_time(); while (!host_is_spi(card) && !(status & MMC_R1_READY_FOR_DATA)) { - // TODO: add some timeout here + if (esp_timer_get_time() - t0 > SDMMC_READY_FOR_DATA_TIMEOUT_US) { + ESP_LOGE(TAG, "read sectors dma - timeout"); + return ESP_ERR_TIMEOUT; + } err = sdmmc_send_cmd_send_status(card, &status); if (err != ESP_OK) { + ESP_LOGE(TAG, "%s: sdmmc_send_cmd_send_status returned 0x%x", __func__, err); return err; } if (++count % 10 == 0) { @@ -644,6 +656,7 @@ esp_err_t sdmmc_erase_sectors(sdmmc_card_t* card, size_t start_sector, uint32_t status; err = sdmmc_send_cmd_send_status(card, &status); if (err != ESP_OK) { + ESP_LOGE(TAG, "%s: sdmmc_send_cmd_send_status returned 0x%x", __func__, err); return err; } if (status != 0) { diff --git a/components/sdmmc/sdmmc_common.h b/components/sdmmc/sdmmc_common.h index 759151e14c..ee8a59cfb0 100644 --- a/components/sdmmc/sdmmc_common.h +++ b/components/sdmmc/sdmmc_common.h @@ -30,6 +30,9 @@ #define SDMMC_GO_IDLE_DELAY_MS 20 #define SDMMC_IO_SEND_OP_COND_DELAY_MS 10 +#define SDMMC_INIT_WAIT_DATA_READY_TIMEOUT_US 5000 * 1000 +#define SDMMC_READY_FOR_DATA_TIMEOUT_US 5000 * 1000 + /* These delay values are mostly useful for cases when CD pin is not used, and * the card is removed. In this case, SDMMC peripheral may not always return * CMD_DONE / DATA_DONE interrupts after signaling the error. These timeouts work diff --git a/components/sdmmc/sdmmc_io.c b/components/sdmmc/sdmmc_io.c index f36528faa0..1e2e069f4d 100644 --- a/components/sdmmc/sdmmc_io.c +++ b/components/sdmmc/sdmmc_io.c @@ -324,6 +324,8 @@ esp_err_t sdmmc_io_read_bytes(sdmmc_card_t* card, uint32_t function, size_t size_aligned = size & (~3); size_t will_transfer = size_aligned > 0 ? size_aligned : size; + // Note: sdmmc_io_rw_extended has an internal timeout, + // typically SDMMC_DEFAULT_CMD_TIMEOUT_MS esp_err_t err = sdmmc_io_rw_extended(card, function, addr, SD_ARG_CMD53_READ | SD_ARG_CMD53_INCREMENT, pc_dst, will_transfer); @@ -347,6 +349,8 @@ esp_err_t sdmmc_io_write_bytes(sdmmc_card_t* card, uint32_t function, size_t size_aligned = size & (~3); size_t will_transfer = size_aligned > 0 ? size_aligned : size; + // Note: sdmmc_io_rw_extended has an internal timeout, + // typically SDMMC_DEFAULT_CMD_TIMEOUT_MS esp_err_t err = sdmmc_io_rw_extended(card, function, addr, SD_ARG_CMD53_WRITE | SD_ARG_CMD53_INCREMENT, (void*) pc_src, will_transfer); diff --git a/components/sdmmc/sdmmc_sd.c b/components/sdmmc/sdmmc_sd.c index 5b33cb7e45..63d6bf2ca5 100644 --- a/components/sdmmc/sdmmc_sd.c +++ b/components/sdmmc/sdmmc_sd.c @@ -14,6 +14,7 @@ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ +#include "esp_timer.h" #include "sdmmc_common.h" @@ -138,8 +139,12 @@ esp_err_t sdmmc_init_sd_wait_data_ready(sdmmc_card_t* card) /* Wait for the card to be ready for data transfers */ uint32_t status = 0; uint32_t count = 0; + int64_t t0 = esp_timer_get_time(); while (!host_is_spi(card) && !(status & MMC_R1_READY_FOR_DATA)) { - // TODO: add some timeout here + if (esp_timer_get_time() - t0 > SDMMC_INIT_WAIT_DATA_READY_TIMEOUT_US) { + ESP_LOGE(TAG, "init wait data ready - timeout"); + return ESP_ERR_TIMEOUT; + } esp_err_t err = sdmmc_send_cmd_send_status(card, &status); if (err != ESP_OK) { return err; From 62a5ad5fc168a976becfe885097bc078255b8faf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20M=C3=BAdry?= Date: Wed, 8 Mar 2023 13:29:27 +0100 Subject: [PATCH 3/4] sdmmc: add vTaskDelay to loops to prevent potential WDT trigger Also change timeout to 120 seconds in fatfs sdcard pytest to prevent failing during formatting. --- components/driver/sdmmc/sdmmc_host.c | 25 +++++++++++-------- .../test_apps/sdcard/pytest_fatfs_sdcard.py | 10 ++++---- components/sdmmc/sdmmc_cmd.c | 4 ++- components/sdmmc/sdmmc_sd.c | 3 ++- 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/components/driver/sdmmc/sdmmc_host.c b/components/driver/sdmmc/sdmmc_host.c index 158e4eded8..16e33cbf92 100644 --- a/components/driver/sdmmc/sdmmc_host.c +++ b/components/driver/sdmmc/sdmmc_host.c @@ -10,6 +10,7 @@ #include "esp_log.h" #include "esp_intr_alloc.h" #include "esp_timer.h" +#include "esp_check.h" #include "soc/soc_caps.h" #include "soc/soc_pins.h" #include "soc/gpio_periph.h" @@ -81,6 +82,7 @@ esp_err_t sdmmc_host_reset(void) if (esp_timer_get_time() - t0 > SDMMC_HOST_RESET_TIMEOUT_US) { return ESP_ERR_TIMEOUT; } + vTaskDelay(1); } return ESP_OK; @@ -189,11 +191,7 @@ static esp_err_t sdmmc_host_clock_update_command(int slot) bool repeat = true; while(repeat) { - esp_err_t err = sdmmc_host_start_command(slot, cmd_val, 0); - if (err != ESP_OK) { - ESP_LOGE(TAG, "sdmmc_host_start_command() failed"); - return err; - } + ESP_RETURN_ON_ERROR(sdmmc_host_start_command(slot, cmd_val, 0), TAG, "sdmmc_host_start_command returned 0x%x", err_rc_); int64_t t0 = esp_timer_get_time(); while (true) { @@ -215,6 +213,8 @@ static esp_err_t sdmmc_host_clock_update_command(int slot) repeat = false; break; } + + vTaskDelay(1); } } @@ -267,7 +267,8 @@ esp_err_t sdmmc_host_set_card_clk(int slot, uint32_t freq_khz) SDMMC.clkena.cclk_enable &= ~BIT(slot); esp_err_t err = sdmmc_host_clock_update_command(slot); if (err != ESP_OK) { - ESP_LOGE(TAG, "disable clk failed"); + ESP_LOGE(TAG, "disabling clk failed"); + ESP_LOGE(TAG, "%s: sdmmc_host_clock_update_command returned 0x%x", __func__, err); return err; } @@ -292,7 +293,8 @@ esp_err_t sdmmc_host_set_card_clk(int slot, uint32_t freq_khz) sdmmc_host_set_clk_div(host_div); err = sdmmc_host_clock_update_command(slot); if (err != ESP_OK) { - ESP_LOGE(TAG, "set clk div failed"); + ESP_LOGE(TAG, "setting clk div failed"); + ESP_LOGE(TAG, "%s: sdmmc_host_clock_update_command returned 0x%x", __func__, err); return err; } @@ -301,7 +303,8 @@ esp_err_t sdmmc_host_set_card_clk(int slot, uint32_t freq_khz) SDMMC.clkena.cclk_low_power |= BIT(slot); err = sdmmc_host_clock_update_command(slot); if (err != ESP_OK) { - ESP_LOGE(TAG, "re-enable clk failed"); + ESP_LOGE(TAG, "re-enabling clk failed"); + ESP_LOGE(TAG, "%s: sdmmc_host_clock_update_command returned 0x%x", __func__, err); return err; } @@ -352,6 +355,7 @@ esp_err_t sdmmc_host_start_command(int slot, sdmmc_hw_cmd_t cmd, uint32_t arg) { if (esp_timer_get_time() - t0 > SDMMC_HOST_START_CMD_TIMEOUT_US) { return ESP_ERR_TIMEOUT; } + vTaskDelay(1); } SDMMC.cmdarg = arg; cmd.card_num = slot; @@ -375,7 +379,7 @@ esp_err_t sdmmc_host_init(void) // Reset esp_err_t err = sdmmc_host_reset(); if (err != ESP_OK) { - ESP_LOGE(TAG, "sdmmc_host_reset() failed"); + ESP_LOGE(TAG, "%s: sdmmc_host_reset returned 0x%x", __func__, err); return err; } @@ -585,7 +589,8 @@ esp_err_t sdmmc_host_init_slot(int slot, const sdmmc_slot_config_t* slot_config) // By default, set probing frequency (400kHz) and 1-bit bus esp_err_t ret = sdmmc_host_set_card_clk(slot, 400); if (ret != ESP_OK) { - ESP_LOGE(TAG, "sdmmc_host_set_card_clk() 400kHz failed"); + ESP_LOGE(TAG, "setting probing freq and 1-bit bus failed"); + ESP_LOGE(TAG, "%s: sdmmc_host_set_card_clk returned 0x%x", __func__, ret); return ret; } ret = sdmmc_host_set_bus_width(slot, 1); diff --git a/components/fatfs/test_apps/sdcard/pytest_fatfs_sdcard.py b/components/fatfs/test_apps/sdcard/pytest_fatfs_sdcard.py index 4ec667dd14..61b97c4cf8 100644 --- a/components/fatfs/test_apps/sdcard/pytest_fatfs_sdcard.py +++ b/components/fatfs/test_apps/sdcard/pytest_fatfs_sdcard.py @@ -1,4 +1,4 @@ -# SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD +# SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD # SPDX-License-Identifier: CC0-1.0 import pytest @@ -19,7 +19,7 @@ def test_fatfs_sdcard_generic_sdmmc(dut: Dut) -> None: dut.write('') dut.expect_exact('Enter test for running.') dut.write('[sdmmc]') - dut.expect_unity_test_output() + dut.expect_unity_test_output(timeout=120) @pytest.mark.esp32 @@ -38,7 +38,7 @@ def test_fatfs_sdcard_generic_sdspi(dut: Dut) -> None: dut.write('') dut.expect_exact('Enter test for running.') dut.write('[sdspi]') - dut.expect_unity_test_output() + dut.expect_unity_test_output(timeout=120) @pytest.mark.esp32 @@ -55,7 +55,7 @@ def test_fatfs_sdcard_psram_sdmmc(dut: Dut) -> None: dut.write('') dut.expect_exact('Enter test for running.') dut.write('[sdmmc]') - dut.expect_unity_test_output() + dut.expect_unity_test_output(timeout=120) @pytest.mark.esp32 @@ -72,4 +72,4 @@ def test_fatfs_sdcard_psram_sdspi(dut: Dut) -> None: dut.write('') dut.expect_exact('Enter test for running.') dut.write('[sdspi]') - dut.expect_unity_test_output() + dut.expect_unity_test_output(timeout=120) diff --git a/components/sdmmc/sdmmc_cmd.c b/components/sdmmc/sdmmc_cmd.c index 9279532104..1851a8ca94 100644 --- a/components/sdmmc/sdmmc_cmd.c +++ b/components/sdmmc/sdmmc_cmd.c @@ -3,8 +3,8 @@ * * SPDX-License-Identifier: Apache-2.0 */ -#include "esp_timer.h" +#include "esp_timer.h" #include "sdmmc_common.h" static const char* TAG = "sdmmc_cmd"; @@ -467,6 +467,7 @@ esp_err_t sdmmc_write_sectors_dma(sdmmc_card_t* card, const void* src, if (++count % 10 == 0) { ESP_LOGV(TAG, "waiting for card to become ready (%d)", count); } + vTaskDelay(1); } /* SPI mode: although card busy indication is based on the busy token, * SD spec recommends that the host checks the results of programming by sending @@ -572,6 +573,7 @@ esp_err_t sdmmc_read_sectors_dma(sdmmc_card_t* card, void* dst, if (++count % 10 == 0) { ESP_LOGV(TAG, "waiting for card to become ready (%d)", count); } + vTaskDelay(1); } return ESP_OK; } diff --git a/components/sdmmc/sdmmc_sd.c b/components/sdmmc/sdmmc_sd.c index 63d6bf2ca5..50147fef6d 100644 --- a/components/sdmmc/sdmmc_sd.c +++ b/components/sdmmc/sdmmc_sd.c @@ -14,8 +14,8 @@ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include "esp_timer.h" +#include "esp_timer.h" #include "sdmmc_common.h" static const char* TAG = "sdmmc_sd"; @@ -152,6 +152,7 @@ esp_err_t sdmmc_init_sd_wait_data_ready(sdmmc_card_t* card) if (++count % 16 == 0) { ESP_LOGV(TAG, "waiting for card to become ready (%d)", count); } + vTaskDelay(1); } return ESP_OK; } From 6477500cf1007a94c7423f2caf529bf45fa55fad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20M=C3=BAdry?= Date: Tue, 16 May 2023 00:16:52 +0200 Subject: [PATCH 4/4] sdmmc: incrementally increase delay of vTaskDelay Prevents unnecessary slowdown in polling functions --- components/driver/sdmmc/sdmmc_host.c | 37 +++++++++++++++++++--------- components/sdmmc/sdmmc_cmd.c | 25 ++++++++++++++----- components/sdmmc/sdmmc_common.h | 4 +-- components/sdmmc/sdmmc_sd.c | 10 ++++++-- 4 files changed, 55 insertions(+), 21 deletions(-) diff --git a/components/driver/sdmmc/sdmmc_host.c b/components/driver/sdmmc/sdmmc_host.c index 16e33cbf92..1dbee8c3b0 100644 --- a/components/driver/sdmmc/sdmmc_host.c +++ b/components/driver/sdmmc/sdmmc_host.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -77,12 +77,18 @@ esp_err_t sdmmc_host_reset(void) SDMMC.ctrl.fifo_reset = 1; // Wait for the reset bits to be cleared by hardware + int64_t yield_delay_us = 100 * 1000; // initially 100ms int64_t t0 = esp_timer_get_time(); + int64_t t1 = 0; while (SDMMC.ctrl.controller_reset || SDMMC.ctrl.fifo_reset || SDMMC.ctrl.dma_reset) { - if (esp_timer_get_time() - t0 > SDMMC_HOST_RESET_TIMEOUT_US) { + t1 = esp_timer_get_time(); + if (t1 - t0 > SDMMC_HOST_RESET_TIMEOUT_US) { return ESP_ERR_TIMEOUT; } - vTaskDelay(1); + if (t1 - t0 > yield_delay_us) { + yield_delay_us *= 2; + vTaskDelay(1); + } } return ESP_OK; @@ -193,13 +199,14 @@ static esp_err_t sdmmc_host_clock_update_command(int slot) ESP_RETURN_ON_ERROR(sdmmc_host_start_command(slot, cmd_val, 0), TAG, "sdmmc_host_start_command returned 0x%x", err_rc_); + int64_t yield_delay_us = 100 * 1000; // initially 100ms int64_t t0 = esp_timer_get_time(); + int64_t t1 = 0; while (true) { - - if (esp_timer_get_time() - t0 > SDMMC_HOST_CLOCK_UPDATE_CMD_TIMEOUT_US) { + t1 = esp_timer_get_time(); + if (t1 - t0 > SDMMC_HOST_CLOCK_UPDATE_CMD_TIMEOUT_US) { return ESP_ERR_TIMEOUT; } - // Sending clock update command to the CIU can generate HLE error. // According to the manual, this is okay and we must retry the command. if (SDMMC.rintsts.hle) { @@ -213,8 +220,10 @@ static esp_err_t sdmmc_host_clock_update_command(int slot) repeat = false; break; } - - vTaskDelay(1); + if (t1 - t0 > yield_delay_us) { + yield_delay_us *= 2; + vTaskDelay(1); + } } } @@ -350,12 +359,18 @@ esp_err_t sdmmc_host_start_command(int slot, sdmmc_hw_cmd_t cmd, uint32_t arg) { /* Outputs should be synchronized to cclk_out */ cmd.use_hold_reg = 1; + int64_t yield_delay_us = 100 * 1000; // initially 100ms int64_t t0 = esp_timer_get_time(); + int64_t t1 = 0; while (SDMMC.cmd.start_command == 1) { - if (esp_timer_get_time() - t0 > SDMMC_HOST_START_CMD_TIMEOUT_US) { + t1 = esp_timer_get_time(); + if (t1 - t0 > SDMMC_HOST_START_CMD_TIMEOUT_US) { return ESP_ERR_TIMEOUT; } - vTaskDelay(1); + if (t1 - t0 > yield_delay_us) { + yield_delay_us *= 2; + vTaskDelay(1); + } } SDMMC.cmdarg = arg; cmd.card_num = slot; @@ -815,7 +830,7 @@ static void sdmmc_isr(void* arg) { uint32_t sdio_pending = SDMMC.mintsts.sdio; if (sdio_pending) { - // disable the interrupt (no need to clear here, this is done in sdmmc_host_io_wait_int) + // disable the interrupt (no need to clear here, this is done in sdmmc_host_io_int_wait) SDMMC.intmask.sdio &= ~sdio_pending; xSemaphoreGiveFromISR(s_io_intr_event, &higher_priority_task_awoken); } diff --git a/components/sdmmc/sdmmc_cmd.c b/components/sdmmc/sdmmc_cmd.c index 1851a8ca94..71302cccae 100644 --- a/components/sdmmc/sdmmc_cmd.c +++ b/components/sdmmc/sdmmc_cmd.c @@ -452,22 +452,28 @@ esp_err_t sdmmc_write_sectors_dma(sdmmc_card_t* card, const void* src, } uint32_t status = 0; size_t count = 0; + int64_t yield_delay_us = 100 * 1000; // initially 100ms int64_t t0 = esp_timer_get_time(); + int64_t t1 = 0; /* SD mode: wait for the card to become idle based on R1 status */ while (!host_is_spi(card) && !(status & MMC_R1_READY_FOR_DATA)) { - if (esp_timer_get_time() - t0 > SDMMC_READY_FOR_DATA_TIMEOUT_US) { + t1 = esp_timer_get_time(); + if (t1 - t0 > SDMMC_READY_FOR_DATA_TIMEOUT_US) { ESP_LOGE(TAG, "write sectors dma - timeout"); return ESP_ERR_TIMEOUT; } + if (t1 - t0 > yield_delay_us) { + yield_delay_us *= 2; + vTaskDelay(1); + } err = sdmmc_send_cmd_send_status(card, &status); if (err != ESP_OK) { ESP_LOGE(TAG, "%s: sdmmc_send_cmd_send_status returned 0x%x", __func__, err); return err; } - if (++count % 10 == 0) { + if (++count % 16 == 0) { ESP_LOGV(TAG, "waiting for card to become ready (%d)", count); } - vTaskDelay(1); } /* SPI mode: although card busy indication is based on the busy token, * SD spec recommends that the host checks the results of programming by sending @@ -559,21 +565,28 @@ esp_err_t sdmmc_read_sectors_dma(sdmmc_card_t* card, void* dst, } uint32_t status = 0; size_t count = 0; + int64_t yield_delay_us = 100 * 1000; // initially 100ms int64_t t0 = esp_timer_get_time(); + int64_t t1 = 0; + /* SD mode: wait for the card to become idle based on R1 status */ while (!host_is_spi(card) && !(status & MMC_R1_READY_FOR_DATA)) { - if (esp_timer_get_time() - t0 > SDMMC_READY_FOR_DATA_TIMEOUT_US) { + t1 = esp_timer_get_time(); + if (t1 - t0 > SDMMC_READY_FOR_DATA_TIMEOUT_US) { ESP_LOGE(TAG, "read sectors dma - timeout"); return ESP_ERR_TIMEOUT; } + if (t1 - t0 > yield_delay_us) { + yield_delay_us *= 2; + vTaskDelay(1); + } err = sdmmc_send_cmd_send_status(card, &status); if (err != ESP_OK) { ESP_LOGE(TAG, "%s: sdmmc_send_cmd_send_status returned 0x%x", __func__, err); return err; } - if (++count % 10 == 0) { + if (++count % 16 == 0) { ESP_LOGV(TAG, "waiting for card to become ready (%d)", count); } - vTaskDelay(1); } return ESP_OK; } diff --git a/components/sdmmc/sdmmc_common.h b/components/sdmmc/sdmmc_common.h index ee8a59cfb0..e28ef3d0ec 100644 --- a/components/sdmmc/sdmmc_common.h +++ b/components/sdmmc/sdmmc_common.h @@ -30,8 +30,8 @@ #define SDMMC_GO_IDLE_DELAY_MS 20 #define SDMMC_IO_SEND_OP_COND_DELAY_MS 10 -#define SDMMC_INIT_WAIT_DATA_READY_TIMEOUT_US 5000 * 1000 -#define SDMMC_READY_FOR_DATA_TIMEOUT_US 5000 * 1000 +#define SDMMC_INIT_WAIT_DATA_READY_TIMEOUT_US (5000 * 1000) +#define SDMMC_READY_FOR_DATA_TIMEOUT_US (5000 * 1000) /* These delay values are mostly useful for cases when CD pin is not used, and * the card is removed. In this case, SDMMC peripheral may not always return diff --git a/components/sdmmc/sdmmc_sd.c b/components/sdmmc/sdmmc_sd.c index 50147fef6d..6c71f41e4d 100644 --- a/components/sdmmc/sdmmc_sd.c +++ b/components/sdmmc/sdmmc_sd.c @@ -139,12 +139,19 @@ esp_err_t sdmmc_init_sd_wait_data_ready(sdmmc_card_t* card) /* Wait for the card to be ready for data transfers */ uint32_t status = 0; uint32_t count = 0; + int64_t yield_delay_us = 100 * 1000; // initially 100ms int64_t t0 = esp_timer_get_time(); + int64_t t1 = 0; while (!host_is_spi(card) && !(status & MMC_R1_READY_FOR_DATA)) { - if (esp_timer_get_time() - t0 > SDMMC_INIT_WAIT_DATA_READY_TIMEOUT_US) { + t1 = esp_timer_get_time(); + if (t1 - t0 > SDMMC_INIT_WAIT_DATA_READY_TIMEOUT_US) { ESP_LOGE(TAG, "init wait data ready - timeout"); return ESP_ERR_TIMEOUT; } + if (t1 - t0 > yield_delay_us) { + yield_delay_us *= 2; + vTaskDelay(1); + } esp_err_t err = sdmmc_send_cmd_send_status(card, &status); if (err != ESP_OK) { return err; @@ -152,7 +159,6 @@ esp_err_t sdmmc_init_sd_wait_data_ready(sdmmc_card_t* card) if (++count % 16 == 0) { ESP_LOGV(TAG, "waiting for card to become ready (%d)", count); } - vTaskDelay(1); } return ESP_OK; }