Merge branch 'bugfix/sdmmc_unaligned_write' into 'master'

sdmmc: fix reads/writes to/from unaligned buffers

See merge request !1071

components/driver/include/driver/sdmmc_host.h

@@ -142,6 +142,8 @@ esp_err_t sdmmc_host_set_card_clk(int slot, uint32_t freq_khz);
  *       can call sdmmc_host_do_transaction as long as other sdmmc_host_*
  *       functions are not called.
  *
+ * @attention Data buffer passed in cmdinfo->data must be in DMA capable memory
+ *
  * @param slot  slot number (SDMMC_HOST_SLOT_0 or SDMMC_HOST_SLOT_1)
  * @param cmdinfo   pointer to structure describing command and data to transfer
  * @return
@@ -149,6 +151,8 @@ esp_err_t sdmmc_host_set_card_clk(int slot, uint32_t freq_khz);
  *      - ESP_ERR_TIMEOUT if response or data transfer has timed out
  *      - ESP_ERR_INVALID_CRC if response or data transfer CRC check has failed
  *      - ESP_ERR_INVALID_RESPONSE if the card has sent an invalid response
+ *      - ESP_ERR_INVALID_SIZE if the size of data transfer is not valid in SD protocol
+ *      - ESP_ERR_INVALID_ARG if the data buffer is not in DMA capable memory
  */
 esp_err_t sdmmc_host_do_transaction(int slot, sdmmc_command_t* cmdinfo);
 

components/driver/sdmmc_transaction.c

@@ -20,6 +20,7 @@
 #include "freertos/semphr.h"
 #include "soc/sdmmc_reg.h"
 #include "soc/sdmmc_struct.h"
+#include "soc/soc_memory_layout.h"
 #include "driver/sdmmc_types.h"
 #include "driver/sdmmc_defs.h"
 #include "driver/sdmmc_host.h"
@@ -105,9 +106,16 @@ 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) {
-        // these constraints should be handled by upper layer
+        if (cmdinfo->datalen < 4 || cmdinfo->blklen % 4 != 0) {
-        assert(cmdinfo->datalen >= 4);
+            ESP_LOGD(TAG, "%s: invalid size: total=%d block=%d",
-        assert(cmdinfo->blklen % 4 == 0);
+                    __func__, cmdinfo->datalen, cmdinfo->blklen);
+            return ESP_ERR_INVALID_SIZE;
+        }
+        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;
+        }
         // this clears "owned by IDMAC" bits
         memset(s_dma_desc, 0, sizeof(s_dma_desc));
         // initialize first descriptor

components/sdmmc/sdmmc_cmd.c

@@ -24,6 +24,7 @@
 #include "driver/sdmmc_types.h"
 #include "sdmmc_cmd.h"
 #include "sys/param.h"
+#include "soc/soc_memory_layout.h"
 
 #define SDMMC_GO_IDLE_DELAY_MS      20
 
@@ -51,6 +52,10 @@ static esp_err_t sdmmc_send_cmd_send_status(sdmmc_card_t* card, uint32_t* out_st
 static esp_err_t sdmmc_send_cmd_crc_on_off(sdmmc_card_t* card, bool crc_enable);
 static uint32_t  get_host_ocr(float voltage);
 static void response_ntoh(sdmmc_response_t response);
+static esp_err_t sdmmc_write_sectors_dma(sdmmc_card_t* card, const void* src,
+        size_t start_block, size_t block_count);
+static esp_err_t sdmmc_read_sectors_dma(sdmmc_card_t* card, void* dst,
+        size_t start_block, size_t block_count);
 
 
 static bool host_is_spi(const sdmmc_card_t* card)
@@ -618,6 +623,37 @@ static esp_err_t sdmmc_send_cmd_send_status(sdmmc_card_t* card, uint32_t* out_st
 
 esp_err_t sdmmc_write_sectors(sdmmc_card_t* card, const void* src,
         size_t start_block, size_t block_count)
+{
+    esp_err_t err = ESP_OK;
+    size_t block_size = card->csd.sector_size;
+    if (esp_ptr_dma_capable(src) && (intptr_t)src % 4 == 0) {
+        err = sdmmc_write_sectors_dma(card, src, start_block, block_count);
+    } else {
+        // SDMMC peripheral needs DMA-capable buffers. Split the write into
+        // separate single block writes, if needed, and allocate a temporary
+        // DMA-capable buffer.
+        void* tmp_buf = heap_caps_malloc(block_size, MALLOC_CAP_DMA);
+        if (tmp_buf == NULL) {
+            return ESP_ERR_NO_MEM;
+        }
+        const uint8_t* cur_src = (const uint8_t*) src;
+        for (size_t i = 0; i < block_count; ++i) {
+            memcpy(tmp_buf, cur_src, block_size);
+            cur_src += block_size;
+            err = sdmmc_write_sectors_dma(card, tmp_buf, start_block + i, 1);
+            if (err != ESP_OK) {
+                ESP_LOGD(TAG, "%s: error 0x%x writing block %d+%d",
+                        __func__, err, start_block, i);
+                break;
+            }
+        }
+        free(tmp_buf);
+    }
+    return err;
+}
+
+static esp_err_t sdmmc_write_sectors_dma(sdmmc_card_t* card, const void* src,
+        size_t start_block, size_t block_count)
 {
     if (start_block + block_count > card->csd.capacity) {
         return ESP_ERR_INVALID_SIZE;
@@ -661,6 +697,37 @@ esp_err_t sdmmc_write_sectors(sdmmc_card_t* card, const void* src,
 
 esp_err_t sdmmc_read_sectors(sdmmc_card_t* card, void* dst,
         size_t start_block, size_t block_count)
+{
+    esp_err_t err = ESP_OK;
+    size_t block_size = card->csd.sector_size;
+    if (esp_ptr_dma_capable(dst) && (intptr_t)dst % 4 == 0) {
+        err = sdmmc_read_sectors_dma(card, dst, start_block, block_count);
+    } else {
+        // SDMMC peripheral needs DMA-capable buffers. Split the read into
+        // separate single block reads, if needed, and allocate a temporary
+        // DMA-capable buffer.
+        void* tmp_buf = heap_caps_malloc(block_size, MALLOC_CAP_DMA);
+        if (tmp_buf == NULL) {
+            return ESP_ERR_NO_MEM;
+        }
+        uint8_t* cur_dst = (uint8_t*) dst;
+        for (size_t i = 0; i < block_count; ++i) {
+            err = sdmmc_read_sectors_dma(card, tmp_buf, start_block + i, 1);
+            if (err != ESP_OK) {
+                ESP_LOGD(TAG, "%s: error 0x%x writing block %d+%d",
+                        __func__, err, start_block, i);
+                break;
+            }
+            memcpy(cur_dst, tmp_buf, block_size);
+            cur_dst += block_size;
+        }
+        free(tmp_buf);
+    }
+    return err;
+}
+
+static esp_err_t sdmmc_read_sectors_dma(sdmmc_card_t* card, void* dst,
+        size_t start_block, size_t block_count)
 {
     if (start_block + block_count > card->csd.capacity) {
         return ESP_ERR_INVALID_SIZE;

components/sdmmc/test/test_sd.c

@@ -22,7 +22,7 @@
 #include "driver/sdmmc_defs.h"
 #include "sdmmc_cmd.h"
 #include "esp_log.h"
-#include "esp_heap_alloc_caps.h"
+#include "esp_heap_caps.h"
 #include <time.h>
 #include <sys/time.h>
 
@@ -55,61 +55,85 @@ TEST_CASE("can probe SD (using SPI)", "[sdspi][ignore]")
     free(card);
 }
 
+// Fill buffer pointed to by 'dst' with 'count' 32-bit ints generated
+// from 'rand' with the starting value of 'seed'
+static void fill_buffer(uint32_t seed, uint8_t* dst, size_t count) {
+    srand(seed);
+    for (size_t i = 0; i < count; ++i) {
+        uint32_t val = rand();
+        memcpy(dst + i * sizeof(uint32_t), &val, sizeof(val));
+    }
+}
+
+// Check if the buffer pointed to by 'dst' contains 'count' 32-bit
+// ints generated from 'rand' with the starting value of 'seed'
+static void check_buffer(uint32_t seed, const uint8_t* src, size_t count) {
+    srand(seed);
+    for (size_t i = 0; i < count; ++i) {
+        uint32_t val;
+        memcpy(&val, src + i * sizeof(uint32_t), sizeof(val));
+        TEST_ASSERT_EQUAL_HEX32(rand(), val);
+    }
+}
+
 static void do_single_write_read_test(sdmmc_card_t* card,
-        size_t start_block, size_t block_count)
+        size_t start_block, size_t block_count, size_t alignment)
 {
     size_t block_size = card->csd.sector_size;
     size_t total_size = block_size * block_count;
-    printf(" %8d |  %3d  |   %4.1f  ", start_block, block_count, total_size / 1024.0f);
+    printf(" %8d |  %3d  |   %d   |    %4.1f  ", start_block, block_count, alignment, total_size / 1024.0f);
-    uint32_t* buffer = pvPortMallocCaps(total_size, MALLOC_CAP_DMA);
+
-    srand(start_block);
+    uint32_t* buffer = heap_caps_malloc(total_size + 4, MALLOC_CAP_DMA);
-    for (size_t i = 0; i < total_size / sizeof(buffer[0]); ++i) {
+    size_t offset = alignment % 4;
-        buffer[i] = rand();
+    uint8_t* c_buffer = (uint8_t*) buffer + offset;
-    }
+    fill_buffer(start_block, c_buffer, total_size / sizeof(buffer[0]));
+
     struct timeval t_start_wr;
     gettimeofday(&t_start_wr, NULL);
-    TEST_ESP_OK(sdmmc_write_sectors(card, buffer, start_block, block_count));
+    TEST_ESP_OK(sdmmc_write_sectors(card, c_buffer, start_block, block_count));
     struct timeval t_stop_wr;
     gettimeofday(&t_stop_wr, NULL);
     float time_wr = 1e3f * (t_stop_wr.tv_sec - t_start_wr.tv_sec) + 1e-3f * (t_stop_wr.tv_usec - t_start_wr.tv_usec);
-    memset(buffer, 0xbb, total_size);
+
+    memset(buffer, 0xbb, total_size + 4);
+
     struct timeval t_start_rd;
     gettimeofday(&t_start_rd, NULL);
-    TEST_ESP_OK(sdmmc_read_sectors(card, buffer, start_block, block_count));
+    TEST_ESP_OK(sdmmc_read_sectors(card, c_buffer, start_block, block_count));
     struct timeval t_stop_rd;
     gettimeofday(&t_stop_rd, NULL);
     float time_rd = 1e3f * (t_stop_rd.tv_sec - t_start_rd.tv_sec) + 1e-3f * (t_stop_rd.tv_usec - t_start_rd.tv_usec);
 
-    printf(" |   %6.2f    |      %.2f      |    %.2f     |     %.2f\n",
+    printf(" |   %6.2f    |      %5.2f      |    %6.2f     |     %5.2f\n",
             time_wr, total_size / (time_wr / 1000) / (1024 * 1024),
             time_rd, total_size / (time_rd / 1000) / (1024 * 1024));
-    srand(start_block);
+    check_buffer(start_block, c_buffer, total_size / sizeof(buffer[0]));
-    for (size_t i = 0; i < total_size / sizeof(buffer[0]); ++i) {
-        TEST_ASSERT_EQUAL_HEX32(rand(), buffer[i]);
-    }
     free(buffer);
 }
 
 static void read_write_test(sdmmc_card_t* card)
 {
     sdmmc_card_print_info(stdout, card);
-    printf("  sector  | count | size(kB) | wr_time(ms) | wr_speed(MB/s) | rd_time(ms) | rd_speed(MB/s)\n");
+    printf("  sector  | count | align | size(kB)  | wr_time(ms) | wr_speed(MB/s)  |  rd_time(ms)  | rd_speed(MB/s)\n");
-    do_single_write_read_test(card, 0, 1);
+    do_single_write_read_test(card, 0, 1, 4);
-    do_single_write_read_test(card, 0, 4);
+    do_single_write_read_test(card, 0, 4, 4);
-    do_single_write_read_test(card, 1, 16);
+    do_single_write_read_test(card, 1, 16, 4);
-    do_single_write_read_test(card, 16, 32);
+    do_single_write_read_test(card, 16, 32, 4);
-    do_single_write_read_test(card, 48, 64);
+    do_single_write_read_test(card, 48, 64, 4);
-    do_single_write_read_test(card, 128, 128);
+    do_single_write_read_test(card, 128, 128, 4);
-    do_single_write_read_test(card, card->csd.capacity - 64, 32);
+    do_single_write_read_test(card, card->csd.capacity - 64, 32, 4);
-    do_single_write_read_test(card, card->csd.capacity - 64, 64);
+    do_single_write_read_test(card, card->csd.capacity - 64, 64, 4);
-    do_single_write_read_test(card, card->csd.capacity - 8, 1);
+    do_single_write_read_test(card, card->csd.capacity - 8, 1, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 1);
+    do_single_write_read_test(card, card->csd.capacity/2, 1, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 4);
+    do_single_write_read_test(card, card->csd.capacity/2, 4, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 8);
+    do_single_write_read_test(card, card->csd.capacity/2, 8, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 16);
+    do_single_write_read_test(card, card->csd.capacity/2, 16, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 32);
+    do_single_write_read_test(card, card->csd.capacity/2, 32, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 64);
+    do_single_write_read_test(card, card->csd.capacity/2, 64, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 128);
+    do_single_write_read_test(card, card->csd.capacity/2, 128, 4);
+    do_single_write_read_test(card, card->csd.capacity/2, 1, 1);
+    do_single_write_read_test(card, card->csd.capacity/2, 8, 1);
+    do_single_write_read_test(card, card->csd.capacity/2, 128, 1);
 }
 
 TEST_CASE("can write and read back blocks", "[sd][ignore]")
@@ -142,3 +166,37 @@ TEST_CASE("can write and read back blocks (using SPI)", "[sdspi][ignore]")
     TEST_ESP_OK(sdspi_host_deinit());
 }
 
+TEST_CASE("reads and writes with an unaligned buffer", "[sd]")
+{
+    sdmmc_host_t config = SDMMC_HOST_DEFAULT();
+    TEST_ESP_OK(sdmmc_host_init());
+    sdmmc_slot_config_t slot_config = SDMMC_SLOT_CONFIG_DEFAULT();
+    TEST_ESP_OK(sdmmc_host_init_slot(SDMMC_HOST_SLOT_1, &slot_config));
+    sdmmc_card_t* card = malloc(sizeof(sdmmc_card_t));
+    TEST_ASSERT_NOT_NULL(card);
+    TEST_ESP_OK(sdmmc_card_init(&config, card));
+
+    const size_t buffer_size = 4096;
+    const size_t block_count = buffer_size / 512;
+    const size_t extra = 4;
+    uint8_t* buffer = heap_caps_malloc(buffer_size + extra, MALLOC_CAP_DMA);
+
+    // Check read behavior: do aligned write, then unaligned read
+    const uint32_t seed = 0x89abcdef;
+    fill_buffer(seed, buffer, buffer_size / sizeof(uint32_t));
+    TEST_ESP_OK(sdmmc_write_sectors(card, buffer, 0, block_count));
+    memset(buffer, 0xcc, buffer_size + extra);
+    TEST_ESP_OK(sdmmc_read_sectors(card, buffer + 1, 0, block_count));
+    check_buffer(seed, buffer + 1, buffer_size / sizeof(uint32_t));
+
+    // Check write behavior: do unaligned write, then aligned read
+    fill_buffer(seed, buffer + 1, buffer_size / sizeof(uint32_t));
+    TEST_ESP_OK(sdmmc_write_sectors(card, buffer + 1, 8, block_count));
+    memset(buffer, 0xcc, buffer_size + extra);
+    TEST_ESP_OK(sdmmc_read_sectors(card, buffer, 8, block_count));
+    check_buffer(seed, buffer, buffer_size / sizeof(uint32_t));
+
+    free(buffer);
+    free(card);
+    TEST_ESP_OK(sdmmc_host_deinit());
+}