From d477d3c5bd07d3627e7ef84786b47336c3aaac84 Mon Sep 17 00:00:00 2001 From: Flavio Bayer Date: Fri, 13 Apr 2018 16:34:23 -0300 Subject: [PATCH 1/7] bugfix/sdspi_host.c: wrong CD/WP pin configuration Looks like the configuration of CP and WP pins are wrong, since the check for `gpio_cd` and `gpio_wp` in `slot_config` seems to be swapped. --- components/driver/sdspi_host.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/driver/sdspi_host.c b/components/driver/sdspi_host.c index 5c9db7e46a..f47b590031 100644 --- a/components/driver/sdspi_host.c +++ b/components/driver/sdspi_host.c @@ -316,16 +316,16 @@ esp_err_t sdspi_host_init_slot(int slot, const sdspi_slot_config_t* slot_config) }; if (slot_config->gpio_cd != SDSPI_SLOT_NO_CD) { io_conf.pin_bit_mask |= (1 << slot_config->gpio_cd); - s_slots[slot].gpio_wp = slot_config->gpio_wp; + s_slots[slot].gpio_cd = slot_config->gpio_cd; } else { - s_slots[slot].gpio_wp = GPIO_UNUSED; + s_slots[slot].gpio_cd = GPIO_UNUSED; } if (slot_config->gpio_wp != SDSPI_SLOT_NO_WP) { io_conf.pin_bit_mask |= (1 << slot_config->gpio_wp); - s_slots[slot].gpio_cd = slot_config->gpio_cd; + s_slots[slot].gpio_wp = slot_config->gpio_wp; } else { - s_slots[slot].gpio_cd = GPIO_UNUSED; + s_slots[slot].gpio_wp = GPIO_UNUSED; } if (io_conf.pin_bit_mask != 0) { From 2a7d85cfa4705cbf2a95c61f31671d3453dac6bc Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 23 Apr 2018 14:35:13 +0800 Subject: [PATCH 2/7] sdmmc, sdspi: fix initializers to be C++-compatible, add test Closes https://github.com/espressif/esp-idf/issues/1861 Closes https://github.com/espressif/arduino-esp32/issues/1312 --- components/driver/include/driver/sdmmc_host.h | 2 +- components/driver/include/driver/sdspi_host.h | 5 +++-- .../driver/test/test_sdmmc_sdspi_init.cpp | 20 +++++++++++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 components/driver/test/test_sdmmc_sdspi_init.cpp diff --git a/components/driver/include/driver/sdmmc_host.h b/components/driver/include/driver/sdmmc_host.h index aa3357a736..2c610ad365 100644 --- a/components/driver/include/driver/sdmmc_host.h +++ b/components/driver/include/driver/sdmmc_host.h @@ -43,9 +43,9 @@ extern "C" { .set_card_clk = &sdmmc_host_set_card_clk, \ .do_transaction = &sdmmc_host_do_transaction, \ .deinit = &sdmmc_host_deinit, \ - .command_timeout_ms = 0, \ .io_int_enable = sdmmc_host_io_int_enable, \ .io_int_wait = sdmmc_host_io_int_wait, \ + .command_timeout_ms = 0, \ } /** diff --git a/components/driver/include/driver/sdspi_host.h b/components/driver/include/driver/sdspi_host.h index 9dad6cf7a7..9f72cb0dc4 100644 --- a/components/driver/include/driver/sdspi_host.h +++ b/components/driver/include/driver/sdspi_host.h @@ -40,6 +40,7 @@ extern "C" { .io_voltage = 3.3f, \ .init = &sdspi_host_init, \ .set_bus_width = NULL, \ + .get_bus_width = NULL, \ .set_card_clk = &sdspi_host_set_card_clk, \ .do_transaction = &sdspi_host_do_transaction, \ .deinit = &sdspi_host_deinit, \ @@ -72,8 +73,8 @@ typedef struct { .gpio_mosi = GPIO_NUM_15, \ .gpio_sck = GPIO_NUM_14, \ .gpio_cs = GPIO_NUM_13, \ - .gpio_cd = SDMMC_SLOT_NO_CD, \ - .gpio_wp = SDMMC_SLOT_NO_WP, \ + .gpio_cd = SDSPI_SLOT_NO_CD, \ + .gpio_wp = SDSPI_SLOT_NO_WP, \ .dma_channel = 1 \ } diff --git a/components/driver/test/test_sdmmc_sdspi_init.cpp b/components/driver/test/test_sdmmc_sdspi_init.cpp new file mode 100644 index 0000000000..d2406f582c --- /dev/null +++ b/components/driver/test/test_sdmmc_sdspi_init.cpp @@ -0,0 +1,20 @@ +#include "driver/sdmmc_host.h" +#include "driver/sdspi_host.h" + + +/** + * Check that C-style designated initializers are valid in C++ file. + */ +static void test_initializers() __attribute__((unused)); + +static void test_initializers() +{ + sdmmc_host_t sdmmc_host = SDMMC_HOST_DEFAULT(); + (void) sdmmc_host; + sdmmc_slot_config_t sdmmc_slot = SDMMC_SLOT_CONFIG_DEFAULT(); + (void) sdmmc_slot; + sdmmc_host_t sdspi_host = SDSPI_HOST_DEFAULT(); + (void) sdspi_host; + sdspi_slot_config_t sdspi_slot = SDSPI_SLOT_CONFIG_DEFAULT(); + (void) sdspi_slot; +} From 92a16ac6e68d87453d5ba40c6bad82b078310280 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 23 Apr 2018 14:35:40 +0800 Subject: [PATCH 3/7] sdspi: fix CD and WP incorrectly configured as outputs --- components/driver/sdspi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/driver/sdspi_host.c b/components/driver/sdspi_host.c index f47b590031..e0faa73a68 100644 --- a/components/driver/sdspi_host.c +++ b/components/driver/sdspi_host.c @@ -310,7 +310,7 @@ esp_err_t sdspi_host_init_slot(int slot, const sdspi_slot_config_t* slot_config) // Configure CD and WP pins io_conf = (gpio_config_t) { .intr_type = GPIO_PIN_INTR_DISABLE, - .mode = GPIO_MODE_OUTPUT, + .mode = GPIO_MODE_INPUT, .pin_bit_mask = 0, .pull_up_en = true }; From 829c6cef395e9eeb05ab5c0780abcb59286c888d Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 23 Apr 2018 14:38:00 +0800 Subject: [PATCH 4/7] sdmmc host: when parameter validation fails, exit cleanly This fixes lock-up which happened when sending a command, if the previous command has failed. --- components/driver/sdmmc_transaction.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/components/driver/sdmmc_transaction.c b/components/driver/sdmmc_transaction.c index 8d87847282..52dec2991c 100644 --- a/components/driver/sdmmc_transaction.c +++ b/components/driver/sdmmc_transaction.c @@ -112,6 +112,7 @@ void sdmmc_host_transaction_handler_deinit() esp_err_t sdmmc_host_do_transaction(int slot, sdmmc_command_t* cmdinfo) { + esp_err_t ret; xSemaphoreTake(s_request_mutex, portMAX_DELAY); #ifdef CONFIG_PM_ENABLE esp_pm_lock_acquire(s_pm_lock); @@ -124,12 +125,14 @@ esp_err_t sdmmc_host_do_transaction(int slot, sdmmc_command_t* cmdinfo) if (cmdinfo->datalen < 4 || cmdinfo->blklen % 4 != 0) { ESP_LOGD(TAG, "%s: invalid size: total=%d block=%d", __func__, cmdinfo->datalen, cmdinfo->blklen); - return ESP_ERR_INVALID_SIZE; + ret = ESP_ERR_INVALID_SIZE; + goto out; } if ((intptr_t) cmdinfo->data % 4 != 0 || !esp_ptr_dma_capable(cmdinfo->data)) { ESP_LOGD(TAG, "%s: buffer %p can not be used for DMA", __func__, cmdinfo->data); - return ESP_ERR_INVALID_ARG; + ret = ESP_ERR_INVALID_ARG; + goto out; } // this clears "owned by IDMAC" bits memset(s_dma_desc, 0, sizeof(s_dma_desc)); @@ -146,10 +149,9 @@ esp_err_t sdmmc_host_do_transaction(int slot, sdmmc_command_t* cmdinfo) sdmmc_host_dma_prepare(&s_dma_desc[0], cmdinfo->blklen, cmdinfo->datalen); } // write command into hardware, this also sends the command to the card - esp_err_t ret = sdmmc_host_start_command(slot, hw_cmd, cmdinfo->arg); + ret = sdmmc_host_start_command(slot, hw_cmd, cmdinfo->arg); if (ret != ESP_OK) { - xSemaphoreGive(s_request_mutex); - return ret; + goto out; } // process events until transfer is complete cmdinfo->error = ESP_OK; @@ -161,6 +163,8 @@ esp_err_t sdmmc_host_do_transaction(int slot, sdmmc_command_t* cmdinfo) } } s_is_app_cmd = (ret == ESP_OK && cmdinfo->opcode == MMC_APP_CMD); + +out: #ifdef CONFIG_PM_ENABLE esp_pm_lock_release(s_pm_lock); #endif From 85ab4fc83e05f04f33324a7a1a51263eaaeeb926 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 23 Apr 2018 14:44:46 +0800 Subject: [PATCH 5/7] sdmmc host: add handling of CD and WP pins Previous version of the code only connected CD and WP to the peripheral, in fact the hardware does not use the values of these signals automatically. This adds code to read CD and WP values when command is executed and return errors if card is not present, or write command is executed when WP signal is active. --- components/driver/sdmmc_host.c | 44 ++++++++++++++----- .../soc/esp32/include/soc/sdmmc_struct.h | 4 +- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/components/driver/sdmmc_host.c b/components/driver/sdmmc_host.c index f09d94d585..95cb81fdcf 100644 --- a/components/driver/sdmmc_host.c +++ b/components/driver/sdmmc_host.c @@ -255,6 +255,12 @@ esp_err_t sdmmc_host_start_command(int slot, sdmmc_hw_cmd_t cmd, uint32_t arg) { if (!(slot == 0 || slot == 1)) { return ESP_ERR_INVALID_ARG; } + if ((SDMMC.cdetect.cards & BIT(slot)) != 0) { + return ESP_ERR_NOT_FOUND; + } + if (cmd.data_expected && cmd.rw && (SDMMC.wrtprt.cards & BIT(slot)) != 0) { + return ESP_ERR_INVALID_STATE; + } while (SDMMC.cmd.start_command == 1) { ; } @@ -397,18 +403,36 @@ esp_err_t sdmmc_host_init_slot(int slot, const sdmmc_slot_config_t* slot_config) // SDIO slave interrupt is edge sensitive to ~(int_n | card_int | card_detect) // set this and card_detect to high to enable sdio interrupt - gpio_matrix_in(GPIO_FUNC_IN_HIGH, pslot->card_int, 0); - if (gpio_cd != -1) { - gpio_set_direction(gpio_cd, GPIO_MODE_INPUT); - gpio_matrix_in(gpio_cd, pslot->card_detect, 0); - } else { - gpio_matrix_in(GPIO_FUNC_IN_HIGH, pslot->card_detect, 0); - } + gpio_matrix_in(GPIO_FUNC_IN_HIGH, pslot->card_int, false); - if (gpio_wp != -1) { - gpio_set_direction(gpio_wp, GPIO_MODE_INPUT); - gpio_matrix_in(gpio_wp, pslot->write_protect, 0); + // Set up Card Detect input + int matrix_in_cd; + if (gpio_cd != SDMMC_SLOT_NO_CD) { + ESP_LOGD(TAG, "using GPIO%d as CD pin", gpio_cd); + gpio_pad_select_gpio(gpio_cd); + gpio_set_direction(gpio_cd, GPIO_MODE_INPUT); + matrix_in_cd = gpio_cd; + } else { + // if not set, default to CD low (card present) + matrix_in_cd = GPIO_FUNC_IN_LOW; } + gpio_matrix_in(matrix_in_cd, pslot->card_detect, false); + + // Set up Write Protect input + int matrix_in_wp; + if (gpio_wp != SDMMC_SLOT_NO_WP) { + ESP_LOGD(TAG, "using GPIO%d as WP pin", gpio_wp); + gpio_pad_select_gpio(gpio_wp); + gpio_set_direction(gpio_wp, GPIO_MODE_INPUT); + matrix_in_wp = gpio_wp; + } else { + // if not set, default to WP high (not write protected) + matrix_in_wp = GPIO_FUNC_IN_HIGH; + } + // WP signal is normally active low, but hardware expects + // an active-high signal, so invert it in GPIO matrix + gpio_matrix_in(matrix_in_wp, pslot->write_protect, true); + // 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) { diff --git a/components/soc/esp32/include/soc/sdmmc_struct.h b/components/soc/esp32/include/soc/sdmmc_struct.h index 9f3625a3d9..7e3c6912eb 100644 --- a/components/soc/esp32/include/soc/sdmmc_struct.h +++ b/components/soc/esp32/include/soc/sdmmc_struct.h @@ -255,7 +255,7 @@ typedef volatile struct { union { struct { - uint32_t cards: 2; ///< bit N reads 1 if card N is present + uint32_t cards: 2; ///< bit N reads 0 if card N is present uint32_t reserved: 30; }; uint32_t val; @@ -263,7 +263,7 @@ typedef volatile struct { union { struct { - uint32_t card0: 2; ///< bit N reads 1 if card N is write protected + uint32_t cards: 2; ///< bit N reads 1 if card N is write protected uint32_t reserved: 30; }; uint32_t val; From e19901c898e65f2c1866e1824fe14c0dee17ad6c Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 23 Apr 2018 14:45:59 +0800 Subject: [PATCH 6/7] =?UTF-8?q?sdmmc:=20don=E2=80=99t=20print=20sdio=5Fres?= =?UTF-8?q?et=20warning=20when=20CD=20is=20idle?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- components/sdmmc/sdmmc_cmd.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/components/sdmmc/sdmmc_cmd.c b/components/sdmmc/sdmmc_cmd.c index 9b09ab8ce2..7279dec44c 100644 --- a/components/sdmmc/sdmmc_cmd.c +++ b/components/sdmmc/sdmmc_cmd.c @@ -102,14 +102,17 @@ esp_err_t sdmmc_card_init(const sdmmc_host_t* config, sdmmc_card_t* card) /* ----------- standard initialization process starts here ---------- */ - /* Reset SDIO (CMD52, RES) before re-initializing IO (CMD5). - * Non-IO cards are allowed to time out (in SD mode) or - * return "invalid command" error (in SPI mode). - */ + /* Reset SDIO (CMD52, RES) before re-initializing IO (CMD5). */ uint8_t sdio_reset = CCCR_CTL_RES; err = sdmmc_io_rw_direct(card, 0, SD_IO_CCCR_CTL, SD_ARG_CMD52_WRITE, &sdio_reset); - if (err != ESP_OK && err != ESP_ERR_TIMEOUT - && !(is_spi && err == ESP_ERR_NOT_SUPPORTED)) { + if (err == ESP_ERR_TIMEOUT || (is_spi && err == ESP_ERR_NOT_SUPPORTED)) { + /* Non-IO cards are allowed to time out (in SD mode) or + * return "invalid command" error (in SPI mode). + */ + } else if (err == ESP_ERR_NOT_FOUND) { + ESP_LOGD(TAG, "%s: card not present", __func__); + return err; + } else if (err != ESP_OK) { ESP_LOGE(TAG, "%s: sdio_reset: unexpected return: 0x%x", __func__, err ); return err; } From e0f1524c86c1a130f2121e192744a8fa1296e3cf Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Mon, 23 Apr 2018 14:47:00 +0800 Subject: [PATCH 7/7] sdmmc: add tests for CD and WP pins for SD and SPI mode --- components/sdmmc/test/test_sd.c | 115 ++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/components/sdmmc/test/test_sd.c b/components/sdmmc/test/test_sd.c index 0fbe6c5aee..33b3890ccd 100644 --- a/components/sdmmc/test/test_sd.c +++ b/components/sdmmc/test/test_sd.c @@ -20,11 +20,13 @@ #include "driver/sdmmc_host.h" #include "driver/sdspi_host.h" #include "driver/sdmmc_defs.h" +#include "soc/gpio_reg.h" #include "sdmmc_cmd.h" #include "esp_log.h" #include "esp_heap_caps.h" #include #include +#include TEST_CASE("MMC_RSP_BITS", "[sd]") { @@ -239,3 +241,116 @@ TEST_CASE("reads and writes with an unaligned buffer", "[sd][test_env=UT_T1_SDMO free(card); TEST_ESP_OK(sdmmc_host_deinit()); } + +static void test_cd_input(int gpio_cd_num, const sdmmc_host_t* config) +{ + sdmmc_card_t* card = malloc(sizeof(sdmmc_card_t)); + TEST_ASSERT_NOT_NULL(card); + + // SDMMC host should have configured CD as input. + // Enable output as well (not using the driver, to avoid touching input + // enable bits). + gpio_matrix_out(gpio_cd_num, SIG_GPIO_OUT_IDX, false, false); + REG_WRITE(GPIO_ENABLE_W1TS_REG, BIT(gpio_cd_num)); + + // Check that card initialization fails if CD is high + REG_WRITE(GPIO_OUT_W1TS_REG, BIT(gpio_cd_num)); + usleep(100); + TEST_ESP_ERR(ESP_ERR_NOT_FOUND, sdmmc_card_init(config, card)); + + // Check that card initialization succeeds if CD is low + REG_WRITE(GPIO_OUT_W1TC_REG, BIT(gpio_cd_num)); + usleep(100); + TEST_ESP_OK(sdmmc_card_init(config, card)); + + free(card); +} + +TEST_CASE("CD input works in SD mode", "[sd][test_env=UT_T1_SDMODE]") +{ + sdmmc_host_t config = SDMMC_HOST_DEFAULT(); + sdmmc_slot_config_t slot_config = SDMMC_SLOT_CONFIG_DEFAULT(); + const int gpio_cd_num = 5; + slot_config.gpio_cd = gpio_cd_num; + TEST_ESP_OK(sdmmc_host_init()); + TEST_ESP_OK(sdmmc_host_init_slot(SDMMC_HOST_SLOT_1, &slot_config)); + + test_cd_input(gpio_cd_num, &config); + + TEST_ESP_OK(sdmmc_host_deinit()); +} + +TEST_CASE("CD input works in SPI mode", "[sd][test_env=UT_T1_SPIMODE]") +{ + sdmmc_host_t config = SDSPI_HOST_DEFAULT(); + sdspi_slot_config_t slot_config = SDSPI_SLOT_CONFIG_DEFAULT(); + const int gpio_cd_num = 5; + slot_config.gpio_cd = gpio_cd_num; + TEST_ESP_OK(sdspi_host_init()); + TEST_ESP_OK(sdspi_host_init_slot(config.slot, &slot_config)); + + test_cd_input(gpio_cd_num, &config); + + TEST_ESP_OK(sdspi_host_deinit()); +} + +static void test_wp_input(int gpio_wp_num, const sdmmc_host_t* config) +{ + sdmmc_card_t* card = malloc(sizeof(sdmmc_card_t)); + TEST_ASSERT_NOT_NULL(card); + + // SDMMC host should have configured WP as input. + // Enable output as well (not using the driver, to avoid touching input + // enable bits). + gpio_matrix_out(gpio_wp_num, SIG_GPIO_OUT_IDX, false, false); + REG_WRITE(GPIO_ENABLE_W1TS_REG, BIT(gpio_wp_num)); + + // Check that the card can be initialized with WP low + REG_WRITE(GPIO_OUT_W1TC_REG, BIT(gpio_wp_num)); + TEST_ESP_OK(sdmmc_card_init(config, card)); + + uint32_t* data = heap_caps_calloc(1, 512, MALLOC_CAP_DMA); + + // Check that card write succeeds if WP is high + REG_WRITE(GPIO_OUT_W1TS_REG, BIT(gpio_wp_num)); + usleep(100); + TEST_ESP_OK(sdmmc_write_sectors(card, &data, 0, 1)); + + // Check that write fails if WP is low + REG_WRITE(GPIO_OUT_W1TC_REG, BIT(gpio_wp_num)); + usleep(100); + TEST_ESP_ERR(ESP_ERR_INVALID_STATE, sdmmc_write_sectors(card, &data, 0, 1)); + // ...but reads still work + TEST_ESP_OK(sdmmc_read_sectors(card, &data, 0, 1)); + + free(data); + free(card); +} + +TEST_CASE("WP input works in SD mode", "[sd][test_env=UT_T1_SDMODE]") +{ + sdmmc_host_t config = SDMMC_HOST_DEFAULT(); + sdmmc_slot_config_t slot_config = SDMMC_SLOT_CONFIG_DEFAULT(); + const int gpio_wp_num = 5; + slot_config.gpio_wp = gpio_wp_num; + TEST_ESP_OK(sdmmc_host_init()); + TEST_ESP_OK(sdmmc_host_init_slot(SDMMC_HOST_SLOT_1, &slot_config)); + + test_wp_input(gpio_wp_num, &config); + + TEST_ESP_OK(sdmmc_host_deinit()); +} + +TEST_CASE("WP input works in SPI mode", "[sd][test_env=UT_T1_SPIMODE]") +{ + sdmmc_host_t config = SDSPI_HOST_DEFAULT(); + sdspi_slot_config_t slot_config = SDSPI_SLOT_CONFIG_DEFAULT(); + const int gpio_wp_num = 5; + slot_config.gpio_wp = gpio_wp_num; + TEST_ESP_OK(sdspi_host_init()); + TEST_ESP_OK(sdspi_host_init_slot(config.slot, &slot_config)); + + test_wp_input(gpio_wp_num, &config); + + TEST_ESP_OK(sdspi_host_deinit()); +}