feat(sd_mmc) Make UHS-I SDR the default for ESP-P4 SD_MMC

[lbernstone]

Description of Change

SD_MMC driver had been updated to support UHS-I on ESP-P4, but didn't have the flags quite right. This updates the driver to use UHS SDR by default.

Test Scenarios

ESP32-P4-EYE reading a file off SD card. Speed is improved about 300%.

Related links

Should close #12027

[github-actions]

Warnings

⚠️

Some issues found for the commit messages in this PR: the commit message "Allow setting DDR, and only enable UHS for higher frequencies": summary looks empty type/action looks empty the commit message "Fix indentation": summary looks empty type/action looks empty Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test sufficiently descriptive message summary should be between 10 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

👋 Hello lbernstone, we appreciate your contribution to this project!

---

📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

---

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

---

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 4716fc0

[github-actions]

Memory usage test (comparing PR against master branch)

The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.

Memory	FLASH [bytes]		FLASH [%]		RAM [bytes]		RAM [%]
Target	DEC	INC	DEC	INC	DEC	INC	DEC	INC
ESP32P4	0	⚠️ +706	0.00	⚠️ +0.17	0	0	0.00	0.00
ESP32S3	0	⚠️ +476	0.00	⚠️ +0.13	0	0	0.00	0.00
ESP32	0	⚠️ +712	0.00	⚠️ +0.19	0	⚠️ +108	0.00	⚠️ +0.49

Click to expand the detailed deltas report [usage change in BYTES]

Target	ESP32P4		ESP32S3		ESP32
Example	FLASH	RAM	FLASH	RAM	FLASH	RAM
libraries/ESP_I2S/examples/Record_to_WAV	⚠️ +16	0	0	0	0	0
libraries/SD_MMC/examples/SD2USBMSC	⚠️ +16	0	0	0	-	-
libraries/SD_MMC/examples/SDMMC_Test	⚠️ +706	0	⚠️ +476	0	⚠️ +712	⚠️ +108
libraries/SD_MMC/examples/SDMMC_time	⚠️ +16	0	0	0	0	0

[github-actions]

Test Results

 83 files   83 suites   26m 19s ⏱️
 55 tests  54 ✅ 0 💤 1 ❌
598 runs  597 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 4716fc0.

♻️ This comment has been updated with latest results.

[lucasssvaz]

Please add a check for IDF versions greater than 5.3:

https://github.com/espressif/arduino-esp32/actions/runs/19442524813/job/55629051409?pr=12038

[P-R-O-C-H-Y]

On P4 EV Board card is no longer mounting with the changes in PR.

[P-R-O-C-H-Y]

@lbernstone If I comment out the UHS-1 flag, the sdmmc starts working again and the speed is improved with the DDR flag only.

[TD-er]

@P-R-O-C-H-Y What kind of SD card do you have?
Does it have UHS markings on the card or is it just a SDHC/SDXC card? (or even just SD, which is really a collectors item these days)

[P-R-O-C-H-Y]

@P-R-O-C-H-Y What kind of SD card do you have? Does it have UHS markings on the card or is it just a SDHC/SDXC card? (or even just SD, which is really a collectors item these days)

Its a Kingston 16GB with USH-1 marking on it.

[P-R-O-C-H-Y]

Weird is, that the SD card is not working on the P4-EYE board I got. Even with/without the changes in this PR.

[TD-er]

Can you try to format it using some SD-format tool?
Or format it using some non-PC device like a camera.
It might be the partitioning is unconventional and/or the file system already present is not supported by the libraries we use.
Or maybe the "P4 EYE" doesn't use all 4 pins? For example DAT3 is wired as card detection?

[P-R-O-C-H-Y]

The P4-eye have identical SD card schema as the P4-EV board. Will try with formatted card and also with different one.

[TD-er]

Maybe also try some SDHC/SDXC card?
Those UHS cards may want to negotiate a lower signal level of 1V8. Not sure if this is expected or required. I know it is a feature of the standard.

[me-no-dev]

in all cases, we want to support more cards, not less. If these options make cards not work, we need to make them optional

[TD-er]

I'm not sure if SDHC uses DDR signalling.
If I'm not mistaken, this was initially only a protocol update to support cards > 2 GB.
There is a "high speed" variant for SDHC/XC/UC which doubles the bus speed, so I guess that does look like DDR.

Looking here: https://www.sdcard.org/developers/sd-standard-overview/bus-speed-default-speed-high-speed-uhs-sd-express/
I see there is a 50 MHz and 104 MHz signalling for UHS-I, where there doesn't seem to be a DDR104.
I have no idea what frequency is being used here?

[lbernstone]

I'll see if I can find some old cards around here and test an array of them. Not sure if I have really small & slow ones around any more.

[lbernstone]

Weird is, that the SD card is not working on the P4-EYE board I got. Even with/without the changes in this PR.

P4-eye (waveshare) has BOARD_SDMMC_POWER_PIN on 46

[TD-er]

Hmm I took another look at the source code of this PR and

host.flags &= ~SDMMC_HOST_FLAG_DDR;

looks like it is disabling DDR?
If so then I am quite curious to why it is improving the speed.

If it is indeed increasing the transfer speed, then it sounds like a timing issue, or maybe some signal is inverted?

[TD-er]

Hmm these defines for sure would be a lot more clear with quite a bit more comments...
https://github.com/espressif/esp-idf/blob/8f1e7bc4e08878f5a2aab55fdf302789d3a3eac4/components/sdmmc/include/sd_protocol_types.h#L198-L205

#define SDMMC_FREQ_DEFAULT      20000       /*!< SD/MMC Default speed (limited by clock divider) */
#define SDMMC_FREQ_HIGHSPEED    40000       /*!< SD High speed (limited by clock divider) */
#define SDMMC_FREQ_PROBING      400         /*!< SD/MMC probing speed */
#define SDMMC_FREQ_52M          52000       /*!< MMC 52MHz speed */
#define SDMMC_FREQ_26M          26000       /*!< MMC 26MHz speed */
#define SDMMC_FREQ_DDR50        50000       /*!< MMC 50MHz speed */
#define SDMMC_FREQ_SDR50        100000      /*!< MMC 100MHz speed */
#define SDMMC_FREQ_SDR104       200000      /*!< MMC 200MHz speed */

The names of those defines don't appear to match the frequency numbers.
I assume the "DDR50", "SDR50" and "SDR104" refer to the bus speeds on this page
If so, then both DDR50 and SDR50 refer to 50 MByte/sec.
And since there are 4 bits per transfer on UHS-I cards (not sure about the others as they do have 2 rows of pads, always thought those were for differential signalling, but not 100% sure), you need 2 transfers per byte.
So for SDR50 -> 100 MHz and DDR50 -> 50MHz

Then the most important change of this PR is setting the clock?
And then not disabling DDR and setting the max. freq to SDMMC_FREQ_DDR50 would result in the same speed improvement, right?
Likely with less RF noise and better timings.

[lbernstone]

Hmm these defines for sure would be a lot more clear with quite a bit more comments... espressif/esp-idf@8f1e7bc/components/sdmmc/include/sd_protocol_types.h#L198-L205

#define SDMMC_FREQ_DEFAULT      20000       /*!< SD/MMC Default speed (limited by clock divider) */
#define SDMMC_FREQ_HIGHSPEED    40000       /*!< SD High speed (limited by clock divider) */
#define SDMMC_FREQ_PROBING      400         /*!< SD/MMC probing speed */
#define SDMMC_FREQ_52M          52000       /*!< MMC 52MHz speed */
#define SDMMC_FREQ_26M          26000       /*!< MMC 26MHz speed */
#define SDMMC_FREQ_DDR50        50000       /*!< MMC 50MHz speed */
#define SDMMC_FREQ_SDR50        100000      /*!< MMC 100MHz speed */
#define SDMMC_FREQ_SDR104       200000      /*!< MMC 200MHz speed */

The names of those defines don't appear to match the frequency numbers. I assume the "DDR50", "SDR50" and "SDR104" refer to the bus speeds on this page If so, then both DDR50 and SDR50 refer to 50 MByte/sec. And since there are 4 bits per transfer on UHS-I cards (not sure about the others as they do have 2 rows of pads, always thought those were for differential signalling, but not 100% sure), you need 2 transfers per byte. So for SDR50 -> 100 MHz and DDR50 -> 50MHz

Then the most important change of this PR is setting the clock? And then not disabling DDR and setting the max. freq to SDMMC_FREQ_DDR50 would result in the same speed improvement, right? Likely with less RF noise and better timings.

Yes, you should be able to force to lower rates by setting a lower frequency, and the PR as written sets SDR (by turning DDR off). But, I do think the process should be backwards compatible, such that if the fastest speed fails, it drops down to try to init at a rate that the card understands, like happens transparently on your PC. I just don't have the hardware available to code & test that.

[lbernstone]

Ok, the dev board I got doesn't break out the SDIO pins, so I still have no way to test the old (standard SD) cards I have. Does anybody have old microsd cards and a working P4 that can test if the code as written fails back to lower rates automatically? Ideally, it needs to be tested on a non-UHS SDHC (eg 4GB), and then an old 64MB capacity card.

[TD-er]

and then an old 64MB capacity card.

That would be extremely hard to find, as I have never seen non-SDHC cards in the micro-SD ("TF") format.

[lbernstone]

The thing that really needs to be tested is if this works on a device that is not capable of 1V8 operation.