cpu/sam0_common/periph: add low-level SDMMC peripheral driver for SDHC

[gschorcht]

Contribution description

This PR implements the low-level SDIO/SDMMC peripheral driver for SAM0 SDHC according to the definition in #19539.

Testing procedure

BOARD=same54-xpro make -C tests/drivers/sdmmc

BOARD=same54-xpro make -C tests/sys/vfs_default

Issues/PRs references

Depends on PR #19539
Depends on PR #19899

[riot-ci]

Murdock results

✔️ PASSED

ca44651 drivers/mtd: fix missing dependency in Kconfig for periph_sdmmc

Success	Failures	Total	Runtime
7937	0	7937	16m:08s

Artifacts

• Documentation preview

[benpicco]

Looks good to me and works as expected.
Not sure where the extra 2k ROM compared to sam0_sdhc come from, but I'm happy about shared code - please squash!

[gschorcht]

Looks good to me and works as expected.

It would be interesting whether it works as stable as the busy waiting implementation of sam0_sdhc. Perhaps we should allow the use of both implementations for a while. In the PR, enabling the sam0_sdhc module will actually cause the sdmmc_sdhc module to be used.

Not sure where the extra 2k ROM compared to sam0_sdhc come from,

The implementation differs completely. The increase is probably caused by both sdmmc and sdmmc_sdhc.

At this point, the question arises whether a copyright notice of "Copyright (C) 2018 Alkgrove" in sdmmc_sdhc is still necessary. diff says that 954 lines of the 1181 of sam0_sdhc.c have been changed. A lot lines in sam0_sdhc.c were already modifications.

[benpicco]

Perhaps we should allow the use of both implementations for a while.

Yea I think we can start by deprecating the sam0_sdhc after the next release, we will only find issues by using the new module for a while.

Existing (externa) boards will keep using the old module unless they opt into the new implementation for the time being.

At this point, the question arises whether a copyright notice of "Copyright (C) 2018 Alkgrove" in sdmmc_sdhc is still necessary

If you think it's a rewrite at this point, feel free to drop it. You could also write that it's based on the driver from Alkgrove or that it took inspiration from it if that's the case.

[gschorcht]

At this point, the question arises whether a copyright notice of "Copyright (C) 2018 Alkgrove" in sdmmc_sdhc is still necessary

If you think it's a rewrite at this point, feel free to drop it. You could also write that it's based on the driver from Alkgrove or that it took inspiration from it if that's the case.

I have checked again. To be honest, the new code looks rather like a rewrite than a modification. Neither the structure nor the code seems like a simple modification of https://github.com/alkgrove/initmaker/blob/master/samd5x/src/sd.c. The reason is probably that you already changed the code a lot and I took some code from your implementation and modified it again a lot to implement the API functions. Of course there are some small code snippets that are identical, e.g.

    while (!sdhc->CCR.bit.INTCLKS) {}    /* wait for clock to be stable */

but there is no usual way to implement it and is trivial.

[benpicco]

Please squash!

[benpicco]

bors merge

[bors]

Build failed (retrying...):

• Murdock

[bors]

Build failed:

• Murdock

[benpicco]

It's always Kconfig 😕

[gschorcht]

It's always Kconfig 😕

Commits 5393c75 and ca44651 should fix it.

1. Implying sdmmc if module mtd and the board has the periph_sdmmc feature was missing (ca44651).
2. Enabling ztimer_msec by sdmmc in case ztimer_usec is not enabled will not work if the dependencies for sdmmc are resolved before the dependencies of another module that enables ztimer_usec. Therefore, ztimer_msec has to be enabled by sdmmc.

[benpicco]

bors merge

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• Murdock
• static-tests
• tools-build-success

[benpicco]

Unfortunately I really do see it getting stuck here when I run tests/sys/vfs_default with #20053 and

USEMODULE += lwext4_vfs
CFLAGS += -DCONFIG_MTD_SDMMC_ERASE=1 # not sure if this is really needed
CFLAGS += -DTHREAD_STACKSIZE_MAIN=THREAD_STACKSIZE_LARGE

The test will start by formatting the SD card (which seems to be done rather inefficiently, there are many duplicate writes it seems) but this onslaught of SD commands does a good job of triggering the bug.

I tried adding a naive timeout:

--- a/drivers/sdmmc/sdmmc_sdhc.c
+++ b/drivers/sdmmc/sdmmc_sdhc.c
@@ -805,7 +805,14 @@ static int _disable_sd_clk(sdhc_dev_t *sdhc_dev)
     if (sdhc->CCR.bit.SDCLKEN) {
         DEBUG("[sdmmc] disable SDCLK\n");
         /* wait for command/data to go inactive */
-        while (sdhc->PSR.reg & (SDHC_PSR_CMDINHC | SDHC_PSR_CMDINHD)) {}
+        uint32_t timeout = 0xfffff;
+        while (sdhc->PSR.reg & (SDHC_PSR_CMDINHC | SDHC_PSR_CMDINHD)) {
+            if (--timeout == 0) {
+                break;
+            }
+        }
+        printf("cmd took %lu loops\n", 0xfffff - timeout);
+
         /* disable the clock to card */
         sdhc->CCR.reg &= ~SDHC_CCR_SDCLKEN;
     }

but the controller does not recover from this condition:

2023-11-07 19:02:20,637 - INFO # mtd_sdmmc_write_page: page:442384 offset:0 size:1024
2023-11-07 19:02:20,643 - INFO # cmd took 0 loops
2023-11-07 19:02:20,645 - INFO # write bgroup 36 / 948
2023-11-07 19:02:20,649 - INFO # lwext4: erase 2 sectors starting with lu
2023-11-07 19:02:20,652 - INFO # cmd took 0 loops
2023-11-07 19:02:20,655 - INFO # lwext4: write 1024 bytes to page 458766
2023-11-07 19:02:20,660 - INFO # mtd_sdmmc_write_page: page:458766 offset:0 size:1024
2023-11-07 19:02:20,666 - INFO # cmd took 0 loops
2023-11-07 19:02:20,667 - INFO # write bgroup 37 / 948
2023-11-07 19:02:20,671 - INFO # lwext4: erase 2 sectors starting with lu
2023-11-07 19:02:22,980 - INFO # cmd took 1048575 loops
2023-11-07 19:02:22,984 - INFO # lwext4: write 1024 bytes to page 475150
2023-11-07 19:02:22,988 - INFO # mtd_sdmmc_write_page: page:475150 offset:0 size:1024
2023-11-07 19:02:23,490 - INFO # [sdmmc] Card not present
2023-11-07 19:02:23,493 - INFO # mtd_sdmmc_write_page: error -19