Merge branch 'bugfix/sdio_write_unaligned_len' into 'master'

SDIO: allow read/write operations for lengths not divisible by 4

See merge request idf/esp-idf!2278

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 handle_event(sdmmc_command_t* cmd, sdmmc_req_state_t* state,
-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 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();
@@ -122,9 +124,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) {
+        // Length should be either <4 or >=4 and =0 (mod 4).
-            ESP_LOGD(TAG, "%s: invalid size: total=%d block=%d",
+        if (cmdinfo->datalen >= 4 && cmdinfo->datalen % 4 != 0) {
-                    __func__, cmdinfo->datalen, cmdinfo->blklen);
+            ESP_LOGD(TAG, "%s: invalid size: total=%d",
+                    __func__, cmdinfo->datalen);
             ret = ESP_ERR_INVALID_SIZE;
             goto out;
         }
@@ -156,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;
         }
@@ -212,7 +216,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;
@@ -245,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;
+    sdmmc_event_t event;
-    esp_err_t err = sdmmc_host_wait_for_event(cmd->timeout_ms / portTICK_PERIOD_MS, &evt);
+    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) {
@@ -256,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);
+    ESP_LOGV(TAG, "sdmmc_handle_event: event %08x %08x, unhandled %08x %08x",
-    process_events(evt, cmd, state);
+            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;
 }
 
@@ -363,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",
@@ -372,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) {
@@ -386,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 (mask_check_and_clear(&evt.sdmmc_status, SDMMC_INTMASK_CMD_DONE)) {
-                if (cmd->error != ESP_OK || cmd->data == NULL) {
+                    process_command_response(orig_evt.sdmmc_status, cmd);
-                    next_state = SDMMC_IDLE;
+                    if (cmd->error != ESP_OK) {
-                    break;
+                        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;
 
 
@@ -429,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;
 }
 

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,
+    /* host quirk: SDIO transfer with length not divisible by 4 bytes
-            SD_ARG_CMD53_READ | SD_ARG_CMD53_INCREMENT,
+     * has to be split into two transfers: one with aligned length,
-            dst, size);
+     * 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,
+    /* same host quirk as in sdmmc_io_read_bytes */
-            SD_ARG_CMD53_WRITE | SD_ARG_CMD53_INCREMENT,
+    const uint8_t *pc_src = (const uint8_t*) src;
-            (void*) src, size);
+
+    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);

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_bytes + 3) / 4; ++i) {
-    for (size_t i = 0; i < n_words; ++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));
+    TEST_ESP_OK(sdmmc_io_write_bytes(card, 1, scratch_area_reg, src, n_bytes));
-    ESP_LOG_BUFFER_HEX(TAG, src, len);
+    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);
+    ESP_LOG_BUFFER_HEX(TAG, dst, n_bytes);
-    TEST_ASSERT_EQUAL_UINT8_ARRAY(src, dst, len);
+    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));
+    TEST_ESP_OK(sdmmc_io_read_bytes(card, 1, scratch_area_reg, dst, n_bytes));
-    ESP_LOG_BUFFER_HEX(TAG, dst, len);
+    ESP_LOG_BUFFER_HEX(TAG, dst, n_bytes);
-    TEST_ASSERT_EQUAL_UINT8_ARRAY(src, dst, len);
+    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);