Skip to content

boards/esp32s3-usb-otg: enable SDMMC peripheral #21471

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

Merged

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented May 7, 2025

Contribution description

This PR enables the periph_sdmmc feature for the esp32s3-usb-otg board to use the SDMMC Host to access the SD Card instead of using the SD Card in SPI mode.

Testing procedure

BOARD=esp32s3-usb-otg make -j4 -C tests/sys/vfs_default/ flash term

should still work.

Issues/PRs references

Depends on PR #21478

@github-actions github-actions bot added Area: doc Area: Documentation Area: boards Area: Board ports labels May 7, 2025
@gschorcht gschorcht added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 7, 2025
@riot-ci
Copy link

riot-ci commented May 7, 2025

Murdock results

✔️ PASSED

1842b2f boards/esp32s3-usb-otg: enable SDMMC peripheral

Success Failures Total Runtime
472 0 472 03m:53s

Artifacts

@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 7, 2025
Comment on lines 111 to 116
@note
SPI_DEV(1) is only configured if the `sdcard_spi` module is explicitly used to
access the SD card in SPI mode. Otherwise, the SDMMC host is configured and
used by the `periph_sdmmc` module to access the SD card (default). That is,
add module `sdcard_spi` to the make variable `USEMODULE` to use the SD Card
in SPI mode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@note
SPI_DEV(1) is only configured if the `sdcard_spi` module is explicitly used to
access the SD card in SPI mode. Otherwise, the SDMMC host is configured and
used by the `periph_sdmmc` module to access the SD card (default). That is,
add module `sdcard_spi` to the make variable `USEMODULE` to use the SD Card
in SPI mode.
@note
SPI_DEV(1) is only configured if the `sdcard_spi` module is explicitly added to the
`USEMODULE` in the application's Makefile to access the SD card in SPI mode.
By default, the SDMMC host is configured and used by the `periph_sdmmc`
module to access the SD card.

The last sentence is a bit redundant IMO.

ifeq (,$(filter sdcard_spi,$(USEMODULE)))
# use mtd_sdmmc_default if sdcard_spi isn't explicitly enabled
USEMODULE += mtd_sdmmc_default
endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this need an "else" case to include mtd_sdcard_default?

Copy link
Contributor Author

@gschorcht gschorcht May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the else branch in the initial commit d1774e6, but I had to remove it with commit 9960a62 because the compilation of tests/pkg/fatfs failed because both mtd_sdmmc_default and mtd_sdcard_default were enabled for this application.

BOARD=esp32s3-usb-otg make -j4 -C tests/pkg/fatfs info-modules
...
mtd
mtd_sdcard
mtd_sdcard_default
mtd_sdmmc
mtd_sdmmc_default

The reason seems to be that the Makefile of this application enables mtd_sdcard unconditionally. We should probably change this Makefile in near future. But for this PR, it works in that way and it is defined in same way for the esp32-wrover-kit board.

Copy link
Contributor Author

@gschorcht gschorcht May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same problem is given for tests/pkg/fatfs_vfs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see. I'd feel comfortable merging this intermediate solution if you opened a second PR or issue, so that we don't lose track of it and possibly introduce unforeseen behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crasbe I just provided PR #21478 which should fix the dependency problem for default MTDs in these test applications. Once PR #21478 is merged, rebasing this PR should fix the dependency problems.

Copy link
Contributor

@crasbe crasbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I have no hardware to test this, but the changes look reasonable as well.

I'm sorry that this took such a tangent with the other PR, which was more work than anticipated 😅
But nevertheless, thank you for your persistence.

You can squash the fixups and hit merge once everything is ready.

@gschorcht
Copy link
Contributor Author

Again, I have no hardware to test this, but the changes look reasonable as well.

I have the hardware and have tested it extensively 😉

@gschorcht gschorcht force-pushed the boards/esp32-usb-otg_enable_sdmmc branch from e770299 to 1842b2f Compare May 21, 2025 11:05
@gschorcht gschorcht added this pull request to the merge queue May 21, 2025
Merged via the queue into RIOT-OS:master with commit 03ca34b May 21, 2025
25 checks passed
@Teufelchen1 Teufelchen1 added this to the Release 2025.07 milestone Jul 14, 2025
@gschorcht gschorcht deleted the boards/esp32-usb-otg_enable_sdmmc branch July 30, 2025 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants