Skip to content

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