drivers/sdcard_spi: make usec ztimer optional #17233

fjmolinas · 2021-11-18T17:26:58Z

Contribution description

This PR does a couple of things:

migrates sdcard_spi to ztimer ( this was done simply running the coccinelle scipt)
currently, soft-spi is used during the init sequence to be able to set a pull-up on miso. This is only needed if the CPU is not able to do so, some do support it, in these cases avoid the custom soft-spi
makes ztimer_usec optional

Testing procedure

I tested on a samr21-xpro with the periph_spi_gpio_mode feature enabled and disabled, reading and writing to the spi-card worked in all cases.

bergzand · 2021-12-08T09:07:44Z

I tested on a samr21-xpro with the periph_spi_gpio_mode feature enabled and disabled, reading and writing to the spi-card worked in all cases.

Can you provide some test output here?

bergzand · 2021-12-08T09:09:09Z

drivers/include/periph/spi.h

@@ -329,7 +329,7 @@ typedef struct {
 * @retval  0           success
 * @retval  <0          error
 */
-int spi_init_with_gpio_mode(spi_t bus, spi_gpio_mode_t mode);
+int spi_init_with_gpio_mode(spi_t bus, const spi_gpio_mode_t* mode);


Are you sneaking in an API change here?

Ehh true that should be split

bergzand · 2021-12-10T14:15:04Z

@fjmolinas This one needs a rebase

fjmolinas · 2022-02-10T11:39:17Z

@bergzand I have split out and merged all the dependencies, this is now only about making ztimer_usec optional.

fjmolinas · 2022-02-14T09:01:37Z

Maybe @benpicco can take a look at this one as well?

benpicco

Still works on same54-xpro which provides periph_spi_gpio_mode but no ZTIMER_MSEC

benpicco · 2022-02-14T14:37:39Z

drivers/sdcard_spi/sdcard_spi.c

            (gpio_init(card->params.miso, GPIO_IN_PU) == 0) &&
            ((!gpio_is_valid(card->params.power)) ||
             (gpio_init(card->params.power, GPIO_OUT) == 0))) {

            DEBUG("gpio_init(): [OK]\n");
+            /* use soft-spi to perform init command to allow use of internal pull-ups on miso */
+            _dyn_spi_rxtx_byte = &_sw_spi_rxtx_byte;


Suggested change

_dyn_spi_rxtx_byte = &_sw_spi_rxtx_byte;

_dyn_spi_rxtx_byte = _sw_spi_rxtx_byte;

will work as well

benpicco · 2022-02-14T14:39:23Z

drivers/sdcard_spi/sdcard_spi.c

+             (gpio_init(card->params.power, GPIO_OUT) == 0))) {
+            DEBUG("gpio_init(): [OK]\n");
+            /* always use hw-spi */
+            _dyn_spi_rxtx_byte = &_hw_spi_rxtx_byte;


might as well

#define _dyn_spi_rxtx_byte _hw_spi_rxtx_byte

in that case then?

benpicco · 2022-02-14T14:44:32Z

drivers/sdcard_spi/sdcard_spi.c

        for (int i = 0; i < SD_POWERSEQUENCE_CLOCK_COUNT; i += 1) {
-            gpio_set(card->params.clk);
-            ztimer_sleep(ZTIMER_USEC, SD_CARD_PREINIT_CLOCK_PERIOD_US / 2);
-            gpio_clear(card->params.clk);
-            ztimer_sleep(ZTIMER_USEC, SD_CARD_PREINIT_CLOCK_PERIOD_US / 2);
+            _dyn_spi_rxtx_byte(card, SD_CARD_DUMMY_BYTE, &dummy);
        }


this used to send SD_POWERSEQUENCE_CLOCK_COUNT bits, now it sends SD_POWERSEQUENCE_CLOCK_COUNT bytes.

Ah true... will devide by 8

benpicco · 2022-02-14T15:20:17Z

I'm currently trying the following:

init with SD card not inserted: init fails, as expected.

2022-02-14 16:17:12,562 # SD_INIT_START
2022-02-14 16:17:12,564 # gpio_init(): [OK]
2022-02-14 16:17:12,566 # SD_INIT_SPI_POWER_SEQ
2022-02-14 16:17:12,568 # SD_INIT_SEND_CMD0
2022-02-14 16:17:12,574 # sdcard_spi_send_cmd: CMD00 (0x00000000) (remaining retry time 94 usec)
2022-02-14 16:17:12,577 # _wait_for_not_busy: [BUSY]
…
2022-02-14 16:17:12,826 # _wait_for_not_busy: [BUSY]
2022-02-14 16:17:12,828 # _wait_for_not_busy: [FAILED]
2022-02-14 16:17:12,834 # sdcard_spi_send_cmd: timeout while waiting for bus to be not busy!
2022-02-14 16:17:12,836 # SD_INIT_CARD_UNKNOWN

insert SD card, call init again: something is detected, but init fails again (works when the board is restarted and the first init succeeds)

2022-02-14 16:17:23,793 # SD_INIT_START
2022-02-14 16:17:23,794 # deinit
2022-02-14 16:17:23,795 # gpio_init(): [OK]
2022-02-14 16:17:23,797 # SD_INIT_SPI_POWER_SEQ
2022-02-14 16:17:23,800 # SD_INIT_SEND_CMD0
2022-02-14 16:17:23,806 # sdcard_spi_send_cmd: CMD00 (0x00000000) (remaining retry time 94 usec)
2022-02-14 16:17:23,809 # _wait_for_not_busy: [BUSY]
…
2022-02-14 16:17:23,982 # _wait_for_not_busy: [BUSY]
2022-02-14 16:17:23,984 # _wait_for_not_busy: [OK]
2022-02-14 16:17:23,988 # CMD00 echo: 0x00 0x00 0x00 0x00 0x00 0x00 
2022-02-14 16:17:23,990 # _wait_for_r1: r1=0x3f
2022-02-14 16:17:23,992 # _wait_for_r1: R1_VALID
2022-02-14 16:17:23,994 # SD_INIT_CARD_UNKNOWN

fjmolinas · 2022-02-14T15:35:57Z

I'm currently trying the following:

Thanks for testing, are you running the tests/driver_sdcard_spi or through vfs or other layers?

benpicco · 2022-02-14T15:55:57Z

I'm using a variant of this with an additional mount shell command that will try to mount a mount point manually.

But tests/driver_sdcard_spi also fails to init if the SD card was not inserted on the first init.

fjmolinas · 2022-02-15T08:36:36Z

Indeed seems like something has gone wrong here.

fjmolinas · 2022-02-15T10:34:55Z

Indeed seems like something has gone wrong here.

I found some issues, but it also seems like sending 0xFF does not works on all hardware for the init sequence...

Most delays are in ms units, only a couple are < 1ms an in those cases its for timeout in cases of retries erros, paying the price of waiting x10 more before failing can be worth the price of avoiding the use of ztimer_usec.

benpicco · 2022-06-09T14:56:59Z

This needs a rebase.

fjmolinas requested review from aabadie, benpicco, cgundogan, DipSwitch, dylad, jia200x, keestux, leandrolanzieri, MrKevinWeiss and vincent-d as code owners November 18, 2021 17:26

github-actions bot added Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: LoRa Area: LoRa radio support Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Nov 18, 2021

bergzand reviewed Dec 8, 2021

View reviewed changes

fjmolinas mentioned this pull request Dec 9, 2021

drivers/sdcard_spi: convert to ztimer_usec #17361

Merged

aabadie changed the title ~~drivers/sdcard_spi: make usec ttimer optional~~ drivers/sdcard_spi: make usec ztimer optional Jan 5, 2022

fjmolinas mentioned this pull request Jan 18, 2022

drivers/periph_spi: spi_init_with_gpio_mode mode by reference #17531

Merged

This was referenced Feb 4, 2022

cpu/sam0_common: add periph_spi_init_gpio #17616

Merged

cpu/nrf52-9160: add periph_spi_init_gpio #17617

Merged

fjmolinas force-pushed the pr_sdcard_spi_conditional_softspi branch from 2ddacec to 8436d78 Compare February 10, 2022 11:38

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 10, 2022

github-actions bot removed Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports Area: LoRa Area: LoRa radio support labels Feb 10, 2022

benpicco reviewed Feb 14, 2022

View reviewed changes

github-actions bot added the Area: tools Area: Supplementary tools label Feb 14, 2022

fjmolinas mentioned this pull request Apr 20, 2022

drivers/scard_spi: _dyn_spi_rxtx_byte return void #17968

Merged

fjmolinas added 2 commits April 20, 2022 15:18

drivers/sdcard_spi: add version with no soft spi

4bc4266

drivers/sdcard_spi: make ztimer_usec optional

7d63208

Most delays are in ms units, only a couple are < 1ms an in those cases its for timeout in cases of retries erros, paying the price of waiting x10 more before failing can be worth the price of avoiding the use of ztimer_usec.

fjmolinas force-pushed the pr_sdcard_spi_conditional_softspi branch from 333a42d to 7d63208 Compare June 9, 2022 09:06

fjmolinas added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/sdcard_spi: make usec ztimer optional #17233

drivers/sdcard_spi: make usec ztimer optional #17233

fjmolinas commented Nov 18, 2021

bergzand commented Dec 8, 2021

bergzand Dec 8, 2021

fjmolinas Dec 8, 2021

bergzand commented Dec 10, 2021

fjmolinas commented Feb 10, 2022

fjmolinas commented Feb 14, 2022

benpicco left a comment

benpicco Feb 14, 2022

benpicco Feb 14, 2022

benpicco Feb 14, 2022

fjmolinas Feb 14, 2022

benpicco commented Feb 14, 2022

fjmolinas commented Feb 14, 2022

benpicco commented Feb 14, 2022

fjmolinas commented Feb 15, 2022

fjmolinas commented Feb 15, 2022

benpicco commented Jun 9, 2022

	_dyn_spi_rxtx_byte = &_sw_spi_rxtx_byte;
	_dyn_spi_rxtx_byte = _sw_spi_rxtx_byte;

drivers/sdcard_spi: make usec ztimer optional #17233

Are you sure you want to change the base?

drivers/sdcard_spi: make usec ztimer optional #17233

Conversation

fjmolinas commented Nov 18, 2021

Contribution description

Testing procedure

bergzand commented Dec 8, 2021

bergzand Dec 8, 2021

Choose a reason for hiding this comment

fjmolinas Dec 8, 2021

Choose a reason for hiding this comment

bergzand commented Dec 10, 2021

fjmolinas commented Feb 10, 2022

fjmolinas commented Feb 14, 2022

benpicco left a comment

Choose a reason for hiding this comment

benpicco Feb 14, 2022

Choose a reason for hiding this comment

benpicco Feb 14, 2022

Choose a reason for hiding this comment

benpicco Feb 14, 2022

Choose a reason for hiding this comment

fjmolinas Feb 14, 2022

Choose a reason for hiding this comment

benpicco commented Feb 14, 2022

fjmolinas commented Feb 14, 2022

benpicco commented Feb 14, 2022

fjmolinas commented Feb 15, 2022

fjmolinas commented Feb 15, 2022

benpicco commented Jun 9, 2022