drivers/sdcard_spi: remove auto-init

[DanielLockau-MLPA]

Contribution description

This contribution removes the auto-init functionality from the sdcard_spi module for the following reasons:

• Having auto-init for sdcard_spi can be considered unnecessary because use of the sd card through the mtd layer will initialize the required device anyway.
• The current auto-init implementation mixes global variable declaration of variables which are required for using the module with the auto-init functionality. Due to that the sdcard_spi auto-init is enabled by default and cannot be disabled in applications. This behavior is undesirable whenever the sdcard device is powered down during boot or the sd card is simply not mounted in the device.

Testing procedure

The changes have been tested with the following tests and examples:

• examples/filesystem: works, tested with sdcard and fatfs / littlefs
• tests/drivers_sdcard_spi: works
• tests/pkg_fatfs: works
• tests/pkg_fatfs_vfs: generally works, some tests unrelated to this PR fail

  2020-07-09 10:20:53,452 # Help: Press s to start test, r to print it is ready
  s
  2020-07-09 10:20:56,470 # START
  2020-07-09 10:20:56,478 # main(): This is RIOT! (Version: 2020.07-devel-1879-g0d3dc1-fix/20200706__sdcard_spi_auto_init)
  2020-07-09 10:20:56,729 # Tests for FatFs over VFS - test results will be printed in the format test_name:result
  2020-07-09 10:20:56,744 # test_mount__mount:[OK]
  2020-07-09 10:20:56,745 # test_mount__umount:[OK]
  2020-07-09 10:20:56,745 # test_open__mount:[OK]
  2020-07-09 10:20:56,748 # test_open__open:[OK]
  2020-07-09 10:20:56,750 # test_open__open_ro:[OK]
  2020-07-09 10:20:56,753 # test_open__close_ro:[OK]
  2020-07-09 10:20:56,755 # test_open__open_wo:[OK]
  2020-07-09 10:20:56,757 # test_open__close_wo:[OK]
  2020-07-09 10:20:56,760 # test_open__open_rw:[OK]
  2020-07-09 10:20:56,762 # test_open__close_rw:[OK]
  2020-07-09 10:20:56,764 # test_open__umount:[OK]
  2020-07-09 10:20:56,770 # test_rw__mount:[OK]
  2020-07-09 10:20:56,775 # test_rw__open_ro:[OK]
  2020-07-09 10:20:56,782 # test_rw__read_ro:[OK]
  2020-07-09 10:20:56,784 # test_rw__write_ro:[OK]
  2020-07-09 10:20:56,785 # test_rw__close_ro:[OK]
  2020-07-09 10:20:56,791 # test_rw__open_wo:[OK]
  2020-07-09 10:20:56,793 # test_rw__read_wo:[OK]
  2020-07-09 10:20:56,795 # test_rw__close_wo:[OK]
  2020-07-09 10:20:56,797 # test_rw__open_rw:[OK]
  2020-07-09 10:20:56,803 # test_rw__read_rw:[OK]
  2020-07-09 10:20:56,806 # test_rw__write_rw:[OK]
  2020-07-09 10:20:56,807 # test_rw__lseek:[OK]
  2020-07-09 10:20:56,809 # test_rw__read_rw:[OK]
  2020-07-09 10:20:56,928 # test_rw__close_rw:[OK]
  2020-07-09 10:20:56,935 # test_rw__open_rwc:[OK]
  2020-07-09 10:20:56,965 # test_rw__write_rwc:[OK]
  2020-07-09 10:20:56,967 # test_rw__lseek_rwc:[OK]
  2020-07-09 10:20:56,969 # test_rw__read_rwc:[OK]
  2020-07-09 10:20:56,996 # test_rw__close_rwc:[OK]
  2020-07-09 10:20:56,998 # test_rw__umount:[OK]
  2020-07-09 10:20:57,012 # test_dir__mount:[OK]
  2020-07-09 10:20:57,012 # test_dir__opendir:[OK]
  2020-07-09 10:20:57,013 # test_dir__readdir1:[OK]
  2020-07-09 10:20:57,015 # test_dir__readdir2:[OK]
  2020-07-09 10:20:57,018 # test_dir__readdir_name:[FAILED]
  2020-07-09 10:20:57,020 # test_dir__readdir3:[FAILED]
  2020-07-09 10:20:57,023 # test_dir__closedir:[OK]
  2020-07-09 10:20:57,024 # test_dir__umount:[OK]
  2020-07-09 10:20:57,030 # test_rename__mount:[OK]
  2020-07-09 10:20:57,047 # test_rename__rename:[OK]
  2020-07-09 10:20:57,050 # test_rename__opendir:[OK]
  2020-07-09 10:20:57,053 # test_rename__readdir1:[OK]
  2020-07-09 10:20:57,055 # test_rename__readdir2:[OK]
  2020-07-09 10:20:57,059 # test_rename__check_name:[FAILED]
  2020-07-09 10:20:57,062 # test_rename__readdir3:[FAILED]
  2020-07-09 10:20:57,067 # test_rename__closedir:[OK]
  2020-07-09 10:20:57,069 # test_rename__umount:[OK]
  2020-07-09 10:20:57,071 # test_unlink__mount:[OK]
  2020-07-09 10:20:57,106 # test_unlink__unlink1:[OK]
  2020-07-09 10:20:57,142 # test_unlink__unlink2:[OK]
  2020-07-09 10:20:57,144 # test_unlink__opendir:[OK]
  2020-07-09 10:20:57,151 # test_unlink__readdir:[FAILED]
  2020-07-09 10:20:57,153 # test_unlink__closedir:[OK]
  2020-07-09 10:20:57,156 # test_unlink__umount:[OK]
  2020-07-09 10:20:57,162 # test_mkrmdir__mount:[OK]
  2020-07-09 10:20:57,235 # test_mkrmdir__mkdir:[OK]
  2020-07-09 10:20:57,241 # test_mkrmdir__opendir1:[OK]
  2020-07-09 10:20:57,242 # test_mkrmdir__closedir:[OK]
  2020-07-09 10:20:57,277 # test_mkrmdir__rmdir:[OK]
  2020-07-09 10:20:57,284 # test_mkrmdir__opendir2:[OK]
  2020-07-09 10:20:57,286 # test_mkrmdir__umount:[OK]
  2020-07-09 10:20:57,292 # test_create__mount:[OK]
  2020-07-09 10:20:57,303 # test_create__open_wo:[OK]
  2020-07-09 10:20:57,331 # test_create__write_wo:[OK]
  2020-07-09 10:20:57,357 # test_create__close_wo:[OK]
  2020-07-09 10:20:57,360 # test_create__open_wo2:[OK]
  2020-07-09 10:20:57,367 # test_create__write_wo2:[OK]
  2020-07-09 10:20:57,398 # test_create__close_wo2:[OK]
  2020-07-09 10:20:57,399 # test_create__umount:[OK]
  2020-07-09 10:20:57,404 # test_stat__mount:[OK]
  2020-07-09 10:20:57,445 # test_stat__open:[OK]
  2020-07-09 10:20:57,470 # test_stat__write:[OK]
  2020-07-09 10:20:57,490 # test_stat__close:[OK]
  2020-07-09 10:20:57,491 # test_stat__open:[OK]
  2020-07-09 10:20:57,494 # test_stat__stat:[OK]
  2020-07-09 10:20:57,495 # test_stat__close:[OK]
  2020-07-09 10:20:57,500 # test_stat__size:[OK]
  2020-07-09 10:20:57,501 # test_stat__umount:[OK]
  2020-07-09 10:20:57,506 # test_newlib__mount:[OK]
  2020-07-09 10:20:57,512 # test_newlib__fopen:[OK]
  2020-07-09 10:20:57,520 # test_newlib__fopen_w:[OK]
  2020-07-09 10:20:57,523 # test_newlib__fputs_w:[OK]
  2020-07-09 10:20:57,551 # test_newlib__fread_w:[OK]
  2020-07-09 10:20:57,554 # test_newlib__strcmp_w:[OK]
  2020-07-09 10:20:57,582 # test_newlib__fclose_w:[OK]
  2020-07-09 10:20:57,586 # test_newlib__fopen_r:[OK]
  2020-07-09 10:20:57,589 # test_newlib__fclose_r:[OK]
  2020-07-09 10:20:57,614 # test_newlib__remove:[OK]
  2020-07-09 10:20:57,626 # test_newlib__fopen_a:[OK]
  2020-07-09 10:20:57,628 # test_newlib__fputs_a:[OK]
  2020-07-09 10:20:57,680 # test_newlib__fclose_a:[OK]
  2020-07-09 10:20:57,681 # test_newlib__fopen_a2:[OK]
  2020-07-09 10:20:57,684 # test_newlib__fputs_a2:[OK]
  2020-07-09 10:20:57,691 # test_newlib__fread_a2:[OK]
  2020-07-09 10:20:57,693 # test_newlib__strcmp_a2:[OK]
  2020-07-09 10:20:57,696 # test_newlib__strcmp_a2:[OK]
  2020-07-09 10:20:57,723 # test_newlib__fclose_a2:[OK]
  2020-07-09 10:20:57,751 # test_newlib__remove:[OK]
  2020-07-09 10:20:57,752 # test_newlib__umount:[OK]
  2020-07-09 10:20:57,753 # Test end.

Issues/PRs references

None.

[benpicco]

Looks good to me.
The SD card will get initialized by mtd_sdcard_init() already, there is no reason for SD card to be more special than mtd_spi_nor or at24cxxx, at25xxx

[fabian18]

The SD card will get initialized by mtd_sdcard_init() already, there is no reason for SD card to be more special than mtd_spi_nor or at24cxxx, at25xxx

I was going to implement an auto_init_storage in the manner of #11871 where you would do USEMODULE += auto_init_storage_sdcard_spi to have sdcards auto-initialized, along with the other drivers you mentioned above.
Now that I came across this, I am confused because there already has been a module auto_init_storage. Is it not desirable to have this?

[benpicco]

mtd_init() will usually take care of initializing the low-level driver.
So unless you are not using the mtd layer (which IMHO is a bad idea as by moving some logic there we can keep the low-level drivers simpler) there is no need to initialize them individually.

[fabian18]

Ok, we have a board that needs to read something from an at24cxxx EEPROM to boot up properly.
Doing that in board_init() is not a clean solution for that, because the read is going to lock a mutex and if the EEPROM is not responsive (because it may be broken or may not be connected), then the board crashes because ztimer is not initialized yet but a ztimer_sleep is triggered but the wake up does not work because the main thread is not running yet.
Putting the ztimer_init() before the read in board_init() triggers an expected assertion failure in core/mutex.c:54.

So what would help us IMO is an auto_init_storage that initializes storage devices and can take a hook from the board to read something from an MTD, whatever it expects to be there.
There is a board mulle which does something similar.

Second, I think it would be nice if the MTD e.g. mtd_sdcard_t devices[] and the underlying storage device, in that case sdcard_spi_t devices[], would be set up in one place.
And if I have an extra sdcard (or EEPROM) connected to the board, it could be set up there by auto_init_storage_<device> as well.

How do you think about this?

[DanielLockau-MLPA]

I understand your intention. However I think it is unrelated to this PR and also not in conflict.

This contribution was made to remove auto-init for SD cards which are removable storage devices and IMHO a totally different case than a soldered EEPROM (which probably is even used without file system). When writing this I saw it as undesirable that the removal of a storage device would keep the application from booting into the user code. I still see this case as the desired solution. At the time of writing, the auto-init was tangled with the module and could not be used without. As the SD card was the single use case of the storage auto-init, I decided to completely remove it. I would generally agree with Ben that any storage device with a file system should be handled through the MTD layer.

Maybe this is not the right place for discussing this matter but rather #11871 and related as mentioned above.