From cf81db40a2a5afa8df4e2b14a47f25a01f14621c Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 20 Apr 2018 17:42:13 +0800 Subject: [PATCH 1/2] sdio: allow reads/writes with lengths not divisible by 4 CMD53 in byte mode supports transfers of any number of bytes between 1 and 512. This change removes limitation that the number of bytes must be divisible by 4. Host quirk, that such transfers must be split into two commands (one for the aligned part and the other one for unaligned) is taken into account. --- components/driver/sdmmc_transaction.c | 10 +++--- components/sdmmc/sdmmc_cmd.c | 50 +++++++++++++++++++++++---- components/sdmmc/test/test_sdio.c | 35 +++++++++++-------- 3 files changed, 70 insertions(+), 25 deletions(-) diff --git a/components/driver/sdmmc_transaction.c b/components/driver/sdmmc_transaction.c index 52dec2991..c53b4b11c 100644 --- a/components/driver/sdmmc_transaction.c +++ b/components/driver/sdmmc_transaction.c @@ -122,9 +122,10 @@ esp_err_t sdmmc_host_do_transaction(int slot, sdmmc_command_t* cmdinfo) // convert cmdinfo to hardware register value sdmmc_hw_cmd_t hw_cmd = make_hw_cmd(cmdinfo); if (cmdinfo->data) { - if (cmdinfo->datalen < 4 || cmdinfo->blklen % 4 != 0) { - ESP_LOGD(TAG, "%s: invalid size: total=%d block=%d", - __func__, cmdinfo->datalen, cmdinfo->blklen); + // Length should be either <4 or >=4 and =0 (mod 4). + if (cmdinfo->datalen >= 4 && cmdinfo->datalen % 4 != 0) { + ESP_LOGD(TAG, "%s: invalid size: total=%d", + __func__, cmdinfo->datalen); ret = ESP_ERR_INVALID_SIZE; goto out; } @@ -212,7 +213,8 @@ static void fill_dma_descriptors(size_t num_desc) desc->owned_by_idmac = 1; desc->buffer1_ptr = s_cur_transfer.ptr; desc->next_desc_ptr = (last) ? NULL : &s_dma_desc[(next + 1) % SDMMC_DMA_DESC_CNT]; - desc->buffer1_size = size_to_fill; + assert(size_to_fill < 4 || size_to_fill % 4 == 0); + desc->buffer1_size = (size_to_fill + 3) & (~3); s_cur_transfer.size_remaining -= size_to_fill; s_cur_transfer.ptr += size_to_fill; diff --git a/components/sdmmc/sdmmc_cmd.c b/components/sdmmc/sdmmc_cmd.c index 7279dec44..ab88b7780 100644 --- a/components/sdmmc/sdmmc_cmd.c +++ b/components/sdmmc/sdmmc_cmd.c @@ -1269,22 +1269,57 @@ static esp_err_t sdmmc_io_rw_extended(sdmmc_card_t* card, int func, esp_err_t sdmmc_io_read_bytes(sdmmc_card_t* card, uint32_t function, uint32_t addr, void* dst, size_t size) { - return sdmmc_io_rw_extended(card, function, addr, - SD_ARG_CMD53_READ | SD_ARG_CMD53_INCREMENT, - dst, size); + /* host quirk: SDIO transfer with length not divisible by 4 bytes + * has to be split into two transfers: one with aligned length, + * the other one for the remaining 1-3 bytes. + */ + uint8_t *pc_dst = dst; + while (size > 0) { + size_t size_aligned = size & (~3); + size_t will_transfer = size_aligned > 0 ? size_aligned : size; + + esp_err_t err = sdmmc_io_rw_extended(card, function, addr, + SD_ARG_CMD53_READ | SD_ARG_CMD53_INCREMENT, + pc_dst, will_transfer); + if (err != ESP_OK) { + return err; + } + pc_dst += will_transfer; + size -= will_transfer; + addr += will_transfer; + } + return ESP_OK; } esp_err_t sdmmc_io_write_bytes(sdmmc_card_t* card, uint32_t function, uint32_t addr, const void* src, size_t size) { - return sdmmc_io_rw_extended(card, function, addr, - SD_ARG_CMD53_WRITE | SD_ARG_CMD53_INCREMENT, - (void*) src, size); + /* same host quirk as in sdmmc_io_read_bytes */ + const uint8_t *pc_src = (const uint8_t*) src; + + while (size > 0) { + size_t size_aligned = size & (~3); + size_t will_transfer = size_aligned > 0 ? size_aligned : size; + + esp_err_t err = sdmmc_io_rw_extended(card, function, addr, + SD_ARG_CMD53_WRITE | SD_ARG_CMD53_INCREMENT, + (void*) pc_src, will_transfer); + if (err != ESP_OK) { + return err; + } + pc_src += will_transfer; + size -= will_transfer; + addr += will_transfer; + } + return ESP_OK; } esp_err_t sdmmc_io_read_blocks(sdmmc_card_t* card, uint32_t function, uint32_t addr, void* dst, size_t size) { + if (size % 4 != 0) { + return ESP_ERR_INVALID_SIZE; + } return sdmmc_io_rw_extended(card, function, addr, SD_ARG_CMD53_READ | SD_ARG_CMD53_INCREMENT | SD_ARG_CMD53_BLOCK_MODE, dst, size); @@ -1293,6 +1328,9 @@ esp_err_t sdmmc_io_read_blocks(sdmmc_card_t* card, uint32_t function, esp_err_t sdmmc_io_write_blocks(sdmmc_card_t* card, uint32_t function, uint32_t addr, const void* src, size_t size) { + if (size % 4 != 0) { + return ESP_ERR_INVALID_SIZE; + } return sdmmc_io_rw_extended(card, function, addr, SD_ARG_CMD53_WRITE | SD_ARG_CMD53_INCREMENT | SD_ARG_CMD53_BLOCK_MODE, (void*) src, size); diff --git a/components/sdmmc/test/test_sdio.c b/components/sdmmc/test/test_sdio.c index 8b885baf5..f4eae0598 100644 --- a/components/sdmmc/test/test_sdio.c +++ b/components/sdmmc/test/test_sdio.c @@ -309,35 +309,33 @@ static void test_cmd52_read_write_single_byte(sdmmc_card_t* card) TEST_ASSERT_EQUAL_UINT8(test_byte_2, val); } -static void test_cmd53_read_write_multiple_bytes(sdmmc_card_t* card) +static void test_cmd53_read_write_multiple_bytes(sdmmc_card_t* card, size_t n_bytes) { printf("Write multiple bytes using CMD53\n"); const size_t scratch_area_reg = SLCHOST_CONF_W0 - DR_REG_SLCHOST_BASE; uint8_t* src = heap_caps_malloc(512, MALLOC_CAP_DMA); uint32_t* src_32 = (uint32_t*) src; - const size_t n_words = 6; - srand(0); - for (size_t i = 0; i < n_words; ++i) { + + for (size_t i = 0; i < (n_bytes + 3) / 4; ++i) { src_32[i] = rand(); } - size_t len = n_words * sizeof(uint32_t); - TEST_ESP_OK(sdmmc_io_write_bytes(card, 1, scratch_area_reg, src, len)); - ESP_LOG_BUFFER_HEX(TAG, src, len); + TEST_ESP_OK(sdmmc_io_write_bytes(card, 1, scratch_area_reg, src, n_bytes)); + ESP_LOG_BUFFER_HEX(TAG, src, n_bytes); printf("Read back using CMD52\n"); uint8_t* dst = heap_caps_malloc(512, MALLOC_CAP_DMA); - for (size_t i = 0; i < len; ++i) { + for (size_t i = 0; i < n_bytes; ++i) { TEST_ESP_OK(sdmmc_io_read_byte(card, 1, scratch_area_reg + i, &dst[i])); } - ESP_LOG_BUFFER_HEX(TAG, dst, len); - TEST_ASSERT_EQUAL_UINT8_ARRAY(src, dst, len); + ESP_LOG_BUFFER_HEX(TAG, dst, n_bytes); + TEST_ASSERT_EQUAL_UINT8_ARRAY(src, dst, n_bytes); printf("Read back using CMD53\n"); - TEST_ESP_OK(sdmmc_io_read_bytes(card, 1, scratch_area_reg, dst, len)); - ESP_LOG_BUFFER_HEX(TAG, dst, len); - TEST_ASSERT_EQUAL_UINT8_ARRAY(src, dst, len); + TEST_ESP_OK(sdmmc_io_read_bytes(card, 1, scratch_area_reg, dst, n_bytes)); + ESP_LOG_BUFFER_HEX(TAG, dst, n_bytes); + TEST_ASSERT_EQUAL_UINT8_ARRAY(src, dst, n_bytes); free(src); free(dst); @@ -364,9 +362,16 @@ TEST_CASE("can probe and talk to ESP32 SDIO slave", "[sdio][ignore]") /* Set up standard SDIO registers */ sdio_slave_common_init(card); - for (int repeat = 0; repeat < 10; ++repeat) { + srand(0); + for (int repeat = 0; repeat < 4; ++repeat) { test_cmd52_read_write_single_byte(card); - test_cmd53_read_write_multiple_bytes(card); + test_cmd53_read_write_multiple_bytes(card, 1); + test_cmd53_read_write_multiple_bytes(card, 2); + test_cmd53_read_write_multiple_bytes(card, 3); + test_cmd53_read_write_multiple_bytes(card, 4); + test_cmd53_read_write_multiple_bytes(card, 5); + test_cmd53_read_write_multiple_bytes(card, 23); + test_cmd53_read_write_multiple_bytes(card, 24); } sdio_slave_set_blocksize(card, 0, 512); From 9379d7b9f906df732a466c610e856139ba668f46 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 20 Apr 2018 17:48:34 +0800 Subject: [PATCH 2/2] sdmmc: wait for command done event even if data transfer is over This fixes errors logged on the console: sdmmc_req: handle_idle_state_events unhandled: 00000004 00000000 The issue happens if "data done" event occurs before "command done". State machine code did not check *which* event occurred in SENDING_CMD state, and went to IDLE or SENDING_DATA state on any non-error event. In this case, we can't process "data done" event until command has completed. This change introduces "unhandled event" mask, which is carried over from one run of process_events to the other. This allows waiting for the "command done" event to complete, and then process "data done" event. Ref TW17126. --- components/driver/sdmmc_transaction.c | 49 +++++++++++++++++++-------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/components/driver/sdmmc_transaction.c b/components/driver/sdmmc_transaction.c index c53b4b11c..8abf9bb92 100644 --- a/components/driver/sdmmc_transaction.c +++ b/components/driver/sdmmc_transaction.c @@ -74,8 +74,10 @@ static esp_pm_lock_handle_t s_pm_lock; static esp_err_t handle_idle_state_events(); static sdmmc_hw_cmd_t make_hw_cmd(sdmmc_command_t* cmd); -static esp_err_t handle_event(sdmmc_command_t* cmd, sdmmc_req_state_t* pstate); -static esp_err_t process_events(sdmmc_event_t evt, sdmmc_command_t* cmd, sdmmc_req_state_t* pstate); +static esp_err_t handle_event(sdmmc_command_t* cmd, sdmmc_req_state_t* state, + sdmmc_event_t* unhandled_events); +static esp_err_t process_events(sdmmc_event_t evt, sdmmc_command_t* cmd, + sdmmc_req_state_t* pstate, sdmmc_event_t* unhandled_events); static void process_command_response(uint32_t status, sdmmc_command_t* cmd); static void fill_dma_descriptors(size_t num_desc); static size_t get_free_descriptors_count(); @@ -157,8 +159,9 @@ esp_err_t sdmmc_host_do_transaction(int slot, sdmmc_command_t* cmdinfo) // process events until transfer is complete cmdinfo->error = ESP_OK; sdmmc_req_state_t state = SDMMC_SENDING_CMD; + sdmmc_event_t unhandled_events = { 0 }; while (state != SDMMC_IDLE) { - ret = handle_event(cmdinfo, &state); + ret = handle_event(cmdinfo, &state, &unhandled_events); if (ret != ESP_OK) { break; } @@ -247,10 +250,11 @@ static esp_err_t handle_idle_state_events() } -static esp_err_t handle_event(sdmmc_command_t* cmd, sdmmc_req_state_t* state) +static esp_err_t handle_event(sdmmc_command_t* cmd, sdmmc_req_state_t* state, + sdmmc_event_t* unhandled_events) { - sdmmc_event_t evt; - esp_err_t err = sdmmc_host_wait_for_event(cmd->timeout_ms / portTICK_PERIOD_MS, &evt); + sdmmc_event_t event; + esp_err_t err = sdmmc_host_wait_for_event(cmd->timeout_ms / portTICK_PERIOD_MS, &event); if (err != ESP_OK) { ESP_LOGE(TAG, "sdmmc_host_wait_for_event returned 0x%x", err); if (err == ESP_ERR_TIMEOUT) { @@ -258,8 +262,13 @@ static esp_err_t handle_event(sdmmc_command_t* cmd, sdmmc_req_state_t* state) } return err; } - ESP_LOGV(TAG, "sdmmc_handle_event: evt %08x %08x", evt.sdmmc_status, evt.dma_status); - process_events(evt, cmd, state); + ESP_LOGV(TAG, "sdmmc_handle_event: event %08x %08x, unhandled %08x %08x", + event.sdmmc_status, event.dma_status, + unhandled_events->sdmmc_status, unhandled_events->dma_status); + event.sdmmc_status |= unhandled_events->sdmmc_status; + event.dma_status |= unhandled_events->dma_status; + process_events(event, cmd, state, unhandled_events); + ESP_LOGV(TAG, "sdmmc_handle_event: events unhandled: %08x %08x", unhandled_events->sdmmc_status, unhandled_events->dma_status); return ESP_OK; } @@ -365,7 +374,8 @@ static inline bool mask_check_and_clear(uint32_t* state, uint32_t mask) { return ret; } -static esp_err_t process_events(sdmmc_event_t evt, sdmmc_command_t* cmd, sdmmc_req_state_t* pstate) +static esp_err_t process_events(sdmmc_event_t evt, sdmmc_command_t* cmd, + sdmmc_req_state_t* pstate, sdmmc_event_t* unhandled_events) { const char* const s_state_names[] __attribute__((unused)) = { "IDLE", @@ -374,7 +384,8 @@ static esp_err_t process_events(sdmmc_event_t evt, sdmmc_command_t* cmd, sdmmc_r "BUSY" }; sdmmc_event_t orig_evt = evt; - ESP_LOGV(TAG, "%s: state=%s", __func__, s_state_names[*pstate]); + ESP_LOGV(TAG, "%s: state=%s evt=%x dma=%x", __func__, s_state_names[*pstate], + evt.sdmmc_status, evt.dma_status); sdmmc_req_state_t next_state = *pstate; sdmmc_req_state_t state = (sdmmc_req_state_t) -1; while (next_state != state) { @@ -388,12 +399,19 @@ static esp_err_t process_events(sdmmc_event_t evt, sdmmc_command_t* cmd, sdmmc_r process_command_response(orig_evt.sdmmc_status, cmd); break; // Need to wait for the CMD_DONE interrupt } - process_command_response(orig_evt.sdmmc_status, cmd); - if (cmd->error != ESP_OK || cmd->data == NULL) { - next_state = SDMMC_IDLE; - break; + if (mask_check_and_clear(&evt.sdmmc_status, SDMMC_INTMASK_CMD_DONE)) { + process_command_response(orig_evt.sdmmc_status, cmd); + if (cmd->error != ESP_OK) { + next_state = SDMMC_IDLE; + break; + } + + if (cmd->data == NULL) { + next_state = SDMMC_IDLE; + } else { + next_state = SDMMC_SENDING_DATA; + } } - next_state = SDMMC_SENDING_DATA; break; @@ -431,6 +449,7 @@ static esp_err_t process_events(sdmmc_event_t evt, sdmmc_command_t* cmd, sdmmc_r ESP_LOGV(TAG, "%s state=%s next_state=%s", __func__, s_state_names[state], s_state_names[next_state]); } *pstate = state; + *unhandled_events = evt; return ESP_OK; }