Skip to content
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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

RetiredWizard
Copy link

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.

@RetiredWizard
Copy link
Author

I was just scrolling through the changes and was wondering if I should enclose all the ESP_LOGx statements withing if DEBUG compiler conditionals?

@UnexpectedMaker
Copy link

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!

@bill88t
Copy link

bill88t commented Sep 19, 2024

Looks good but shouldn't it be enabled by default for all S3 boards?

@tannewt
Copy link
Member

tannewt commented Sep 19, 2024

I was just scrolling through the changes and was wondering if I should enclose all the ESP_LOGx statements withing if DEBUG compiler conditionals?

ESP_LOG does this automatically.

Copy link
Member

@tannewt tannewt left a 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.

ports/espressif/common-hal/sdioio/SDCard.c Outdated Show resolved Hide resolved
// 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;
Copy link
Member

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?

Copy link
Author

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...

Copy link
Author

@RetiredWizard RetiredWizard Sep 19, 2024

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 😁

Copy link
Author

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.

ports/espressif/boards/makerfabs_tft7/mpconfigboard.mk Outdated Show resolved Hide resolved
ports/espressif/common-hal/sdioio/SDCard.c Outdated Show resolved Hide resolved
ports/espressif/common-hal/sdioio/SDCard.c Outdated Show resolved Hide resolved
ports/espressif/common-hal/sdioio/SDCard.c Outdated Show resolved Hide resolved
ports/espressif/mpconfigport.mk Outdated Show resolved Hide resolved
@RetiredWizard
Copy link
Author

RetiredWizard commented Sep 19, 2024

Looks good but shouldn't it be enabled by default for all S3 boards?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

esp32s3: Implement sdioio
4 participants