-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
esp32s3: Implement sdioio #9641
base: main
Are you sure you want to change the base?
Conversation
I was just scrolling through the changes and was wondering if I should enclose all the ESP_LOGx statements withing if DEBUG compiler conditionals? |
Oh this looks interesting!!! My new storage shield has 4bit wide eMMC data on non-default IO, and folks have only been able to use it in Arduino and IDF but with this folks should be able to use CP as well! Wicked! |
Looks good but shouldn't it be enabled by default for all S3 boards? |
ESP_LOG does this automatically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this! I'm excited to try it out. I've added a number of suggestions to look into.
// ESP32-S3 and P4 can use any pin for any SDMMC func in either slot | ||
// Default to SLOT_1 for SD cards | ||
ESP_LOGI(TAG, "Using chip with CONFIG_SOC_SDMMC_USE_GPIO_MATRIX"); | ||
return SDMMC_HOST_SLOT_1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to track both slots so you can use two cards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I'll start working on an update...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code, it looks to me like the slot is determined by the command and clock pins. there also appears to be data pin requirements depending on which slot is being used. I haven't dug through the Espressif documents to verify but if you think that's more restrictive than the peripheral requires (which the comment does suggest) I can try and figure that out.
I believe, currently both slots could be used by selecting the associated gpio pins for each slot?
EDIT: Nevermind, the CONFIG_SOC_SDMMC_USE_GPIO_MATRIX parameter must be set and the code is simply defaulting to slot number 1 😁 I'll look into tracking the slots 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the code to track the slot usage but ended up defaulting to slot 1 because using slot 0 isn't working. I've spent a lot of time staring at the code and tweaking little things but so far I'm stumped. The code still seems to mount an SD card fine using slot 1.
Thanks, sounds right to me, I didn't look too long at the mpconfigport settings. The PR currently sets CIRCUITPY_SDIOIO to 0 (using ?=) for all Espressif chips and then hard disables it for the following chips: esp32c2, esp32c6, esp32h2, esp32p4, esp32s2 It's currently left overridable on esp32, esp32c3, esp32s3. It looks to me like the SDMMC_HOST peripheral is supported on the esp32, esp32s3 and esp32p4, so I'll go ahead and update mpconfigport.mk to default (overridable) enabled for those boards and disabled for the rest. EDIT The esp32 SDMMC_HOST peripheral must have a different API so I'm disabling for that chip as well |
This closes #8498
@jacobcrigby created this update (Thanks so much!!!) to support sdioio for ESP32S3 and I think they planned on submitting it as a PR, however with the recent updates to the IDF I wanted to get this code integrated before the refactoring got more difficult.
Dan had mentioned that updates from #9418 would probably be needed which I've gone ahead and done. The symptoms from #9417 appear to have been resolved with those updates.
I noticed some messages in circuitpython.pot that were very similar and I thought could be factored out so I did a little message factoring as well. I'm not sure if that will cause any side effects and if so, I can remove those changes.
I discussed some testing of the original code from @jacobcrigby's github repository on both the Feather esp32s3 (1-wire and 4-wire) and the Makerfabs MaTouch 7" boards (1-wire) in #8498 but have only done basic testing using the Makerfabs board with the final code from this PR.