eMMC/MMC support for ESP32 #1941
Conversation
|
|
| @@ -120,6 +120,9 @@ typedef struct { | |||
| #define SDMMC_HOST_FLAG_4BIT BIT(1) /*!< host supports 4-line SD and MMC protocol */ | |||
| #define SDMMC_HOST_FLAG_8BIT BIT(2) /*!< host supports 8-line MMC protocol */ | |||
| #define SDMMC_HOST_FLAG_SPI BIT(3) /*!< host supports SPI protocol */ | |||
| #define SDMMC_HOST_MMC_CARD BIT(8) /*!< card in MMC mode (SD otherwise) */ | |||
| #define SDMMC_HOST_IO_CARD BIT(9) /*!< card in IO mode (SD moe only) */ | |||
| #define SDMMC_HOST_MEM_CARD BIT(10) /*!< card in memory mode (SD or MMC) */ | |||
There was a problem hiding this comment.
SDMMC_HOST_ flags represent host capabilities, not card properties. Please add the bit you need into sdmmc_card_t, next to is_mem and is_sdio.
components/sdmmc/sdmmc_cmd.c
Outdated
| @@ -25,6 +25,7 @@ | |||
| #include "sdmmc_cmd.h" | |||
| #include "sys/param.h" | |||
| #include "soc/soc_memory_layout.h" | |||
| #include "soc/sdmmc_struct.h" | |||
There was a problem hiding this comment.
Ideally the protocol layer should not know about host registers. Can this be removed?
components/sdmmc/sdmmc_cmd.c
Outdated
| @@ -191,6 +196,17 @@ esp_err_t sdmmc_card_init(const sdmmc_host_t* config, sdmmc_card_t* card) | |||
|
|
|||
| /* Send SEND_OP_COND (ACMD41) command to the card until it becomes ready. */ | |||
| err = sdmmc_send_cmd_send_op_cond(card, host_ocr, &card->ocr); | |||
|
|
|||
| //if time-out try switching from SD to MMC and vice-versa | |||
There was a problem hiding this comment.
Please convert indentation to be the same as surrounding code
components/sdmmc/sdmmc_cmd.c
Outdated
| /* sdmmc_mem_mmc_init */ | ||
| int width, value; | ||
| int card_type; | ||
| int speed = 20000; |
There was a problem hiding this comment.
This default value should preferably be a named constant (SDMMC_FREQ_DEFAULT)
components/sdmmc/sdmmc_cmd.c
Outdated
| //malloc(size) on esp32 is same as heap_caps_malloc(size, MALLOC_CAP_8BIT) | ||
| //!!!remember to free(ext_csd) before all return-s in this block !!! | ||
| #define EXT_CSD_SIZE 512 | ||
| uint8_t* ext_csd = malloc(EXT_CSD_SIZE); |
There was a problem hiding this comment.
If you are passing this buffer to the host driver, it might need to be in DMA-capable memory, so heap_caps_malloc with MALLOC_CAP_DMA would be better.
components/sdmmc/sdmmc_cmd.c
Outdated
| sectors = (ext_csd[EXT_CSD_SEC_COUNT + 0] << 0) | ||
| | (ext_csd[EXT_CSD_SEC_COUNT + 1] << 8) | ||
| | (ext_csd[EXT_CSD_SEC_COUNT + 2] << 16) | ||
| | (ext_csd[EXT_CSD_SEC_COUNT + 3] << 24); |
There was a problem hiding this comment.
It seems that decoding of ext_csd is scattered across this MMC handling code block. How about introducing a helper function for decoding ext_csd, calling this function once ext_csd is read, and freeing the ext_csd? That would help reduce complexity related to freeing ext_csd. You can store decoded information in the card structure (see how this is done with CSD et al).
There was a problem hiding this comment.
We can do it this way , although that would be a major refactor and would need a lot of testing to cover all cases.
I would suggest taking this entire mmc initialization code block in a separate function since sdmmc_card_init has grown too big.
I noticed 'goto out' is used in situations where memory needs to be freed eventually. I think a single goto per function is OK and in fact is used extensively in this repository:
https://github.com/espressif/esp-idf/search?utf8=%E2%9C%93&q=goto&type=
components/sdmmc/sdmmc_cmd.c
Outdated
| free(ext_csd); //done with ext_csd | ||
| } | ||
|
|
||
| }else{ //SD CARD |
There was a problem hiding this comment.
Please add spaces between keywords and braces
components/sdmmc/sdmmc_cmd.c
Outdated
| @@ -539,6 +715,8 @@ static esp_err_t sdmmc_send_cmd_send_if_cond(sdmmc_card_t* card, uint32_t ocr) | |||
|
|
|||
| static esp_err_t sdmmc_send_cmd_send_op_cond(sdmmc_card_t* card, uint32_t ocr, uint32_t *ocrp) | |||
| { | |||
| esp_err_t err; | |||
components/sdmmc/sdmmc_cmd.c
Outdated
| if (ptr == NULL) { | ||
| error = ESP_ERR_NO_MEM; | ||
| } else { | ||
| memset(&cmd, 0, sizeof(cmd)); |
components/sdmmc/sdmmc_cmd.c
Outdated
| sdmmc_command_t cmd = { | ||
| .opcode = SD_APP_SET_BUS_WIDTH, | ||
| .flags = SCF_RSP_R1 | SCF_CMD_AC, | ||
| .arg = (width == 4) ? SD_ARG_BUS_WIDTH_4 : SD_ARG_BUS_WIDTH_1 | ||
| .arg = (width == 4) ? SD_ARG_BUS_WIDTH_4 : SD_ARG_BUS_WIDTH_1, | ||
| .data = ignored, |
There was a problem hiding this comment.
I couldn't find a reference that SET_BUS_WIDTH is a data transfer command, could you explain why this is needed?
There was a problem hiding this comment.
Do you mean SD_APP_SET_BUS_WIDTH ? Looking at Jeded documentation it looks like bus width is set with the SWITCH command , indeed SD_APP_SET_BUS_WIDTH = SD_SEND_SWITCH_FUNC = CMD6
Merges espressif/esp-idf#1941 Previous work in espressif/esp-idf#590
To test this you can use a eMMC to SD adapter and thus connect to a standard SD card slot.
( No need to wait for integrated esp32 eMMC boards )
I tested with following eMMC to SD adapter boards
Micron Chip on mmc+ adapter board
https://www.m-pression.com/solutions/boards/mmcplus
https://www.mouser.com/ProductDetail/Mpression/MFSUSANOEMMC?qs=%2fha2pyFadugEKeD5BFVzYhxAOG8O98MMn%252bx6aKXPpVs%3d
DIY solution with Sandisk chip:
https://intelligenttoasters.blog/2017/01/29/retro-cpc-dongle-part-18/
Kingston ( EMMC04G-M627-ADP, EMMC08G-M325-ADP)
Available through sales representatives only:
https://www.kingston.com/us/embedded/emmc