Awful timeout implementation in SDMMC

[Mikk Leini]

SDMMC HAL driver implementation is full of timeouts like this:

uint32_t count = SDMMC_CMDTIMEOUT * (SystemCoreClock / 8U / 1000U);
do
{
 if (count-- == 0U)
 {
 return SDMMC_ERROR_TIMEOUT;
 }
 sta_reg = SDMMCx->STA;
} while (SOME_FLAG_NOT_SET);

It may work in a single-threaded application on some basic CPU which runs fixed clocks per cycle, but it's not accurate on CPUs with dual-issue pipeline. All preempting interrupts and in case RTOS is used, all higher priority tasks, prolong this timeout. And vice versa - the longer this timeout gets the more it blocks lower priority tasks and interrupts. And the timeout isn't even in microseconds to justify it. SDMMC_CMDTIMEOUT value is 5000 milliseconds (by the comment). It's bad implementation by any standard.

It's been so for a long time in many STM32 series (F4, H7, H7RS, N6, etc.). It's reported at least 6 years ago: I may have encountered a bug in SDMMC_GetCmdResp1 ... - STMicroelectronics Community

I didn't check all the posts in forum, but I bet this implementation has caused problem for a lot of engineers.

How about fixing it? With HAL_GetTick() for start. Those while loops all over HAL checking for HAL_GetTick() aren't good for RTOS use either, but at least the timing would be correct.

Created second post about the general issue: HAL blocking lower priority tasks and interrupts - STMicroelectronics Community

[LCE]

Almost the worst possible solution for checking timeouts! :D

So many reasons for not using HAL - in most peripherals, there are only a few which are really "fast and clean".

[Ozone]

> I didn't check all the posts in forum, but I bet this implementation has caused problem for a lot of engineers.

You can bet.

Seeing this "feature" implementation was one of the reasons for me to forgo Cube/HAL at all.
As expected, this was not really a one-off.
I either use the old SPL, or bare metal code.

[Seb]

While being occasional coder as focusing more on hardware boards, one question: If the delays and timeouts were using the core debug cycle counter, would this be an improvement? (no pipeline, interrupt, compile optimisation/maker time "stretch"). This is what I do on I2C by IO bit bang with auto calibration, a code developped for board bringup with an intern here : Solved: NUCLEO-C5A3ZG STM32C5 QSPI Display Bring up Projec... - STMicroelectronics Community  Thanks!

[LCE]

I'd say that almost every other solution is better than while(count--).
The only reason to use this is ... lazyness! :D 

Only using hardware timers or the cycle counter can give accurate timings.

Especially for delays / timeouts much bigger than the SysTick it's kinda embarrassing to use while(count--).

[Mikk Leini]

In the past projects I have "worked it around" by placing all FreeRTOS tasks on the same priority so RTOS does time-slicing and nothing starves for too long. But in current project we need priorities and need to react reliably and fast to some events. I gave it a try and modified SDMMC driver to use HAL_GetTick() and implemented yielding function from my another post (HAL cooperative mode feature request - STMicroelectronics Community). It works. When I inject errors into SD bus, other same and lower priority tasks won't starve anymore and there is no snowball effect of failures like it was before.

TLDR this is the code:

do
{
 if ((HAL_GetTick() - tickstart) > SDMMC_CMDTIMEOUT)
 {
 return SDMMC_ERROR_TIMEOUT;
 }
 HAL_Yield(); 
} while (!CONDITION);

/**
 * @brief Provides other tasks time to execute shortly (single RTOS tick)
 * while waiting and polling HAL events.
 */
void HAL_Yield(void)
{
 /* Kernel running and call not from ISR ? */
 if ((osKernelGetState() == osKernelRunning) && (__get_IPSR() == 0U))
 {
 osDelay(1);
 }
}

 

[Mikk Leini]

Hello ST, will you fix SDMMC HAL timeouts?

[Danish1]

I also dislike such code.

The variability in time-out time doesn’t worry me too much. It’s a vary rare failure condition so who cares if the failure takes a little longer to detect some times more than others.

What annoys me is that all the time the processor is looping round waiting for the SDMMC to complete an operation, it isn’t available to process any lower-priority threads.

Another criticism I have of HAL is the many layers of call hierarchy. I suspect other necessary delays between peripheral-writes are hidden in the many layers of function-calls within HAL, and that a sufficiently smart optimiser will fall foul of them even if it performs precisely the same writes in exactly the same order

[Mikk Leini]

How about finally fixing this?

Preferably with this feature request: HAL cooperative mode feature request | Community

[David Littell]

Good luck with that.  It’s probably scheduled to be “looked at” in HAL5 or so…  😉

[Mikk Leini]

I think I know why ST haven’t fixed it. The HAL part of SDMMC driver uses HAL_GetTick() but LL driver doesn’t. They might have thought that LL is not supposed to use HAL by design (because LL is lightweight) so they didn’t use HAL_GetTick() there and left CPU only timeout. However LL files include stm32xxx_hal.h file where registers and structures and HAL_GetTick() is defined so LL isn’t that independent anyway.

If they want to break the dependency, I think they should eliminate blocking code (while loops) from SDMMC LL completely so there is no need for GetTick() or CPU cycle counter at all. Rather than having function called SDMMC_GetCmdResp1, implement function called SDMMC_PollCmdResp1 which simply checks the condition. Timeout loop shall be part of parent code - HAL or user code.

 

[Mikk Leini]

For those who want to fix it themselves, here is a patch file content. Sorry, patch files aren’t allowed to upload.

--- a/Drivers/STM32N6xx_HAL_Driver/Src/stm32n6xx_ll_sdmmc.c
+++ b/Drivers/STM32N6xx_HAL_Driver/Src/stm32n6xx_ll_sdmmc.c
@@ -1356,14 +1356,11 @@ uint32_t SDMMC_GetCmdResp1(SDMMC_TypeDef *SDMMCx, uint8_t SD_CMD, uint32_t Timeo
 {
   uint32_t response_r1;
   uint32_t sta_reg;
-
-  /* 8 is the number of required instructions cycles for the below loop statement.
-  The Timeout is expressed in ms */
-  uint32_t count = Timeout * (SystemCoreClock / 8U / 1000U);
+  uint32_t tick_start = HAL_GetTick();

   do
   {
-    if (count-- == 0U)
+    if ((HAL_GetTick() - tick_start) > Timeout)
     {
       return SDMMC_ERROR_TIMEOUT;
     }
@@ -1490,13 +1487,11 @@ uint32_t SDMMC_GetCmdResp1(SDMMC_TypeDef *SDMMCx, uint8_t SD_CMD, uint32_t Timeo
 uint32_t SDMMC_GetCmdResp2(SDMMC_TypeDef *SDMMCx)
 {
   uint32_t sta_reg;
-  /* 8 is the number of required instructions cycles for the below loop statement.
-  The SDMMC_CMDTIMEOUT is expressed in ms */
-  uint32_t count = SDMMC_CMDTIMEOUT * (SystemCoreClock / 8U / 1000U);
+  uint32_t tick_start = HAL_GetTick();

   do
   {
-    if (count-- == 0U)
+    if ((HAL_GetTick() - tick_start) > SDMMC_CMDTIMEOUT)
     {
       return SDMMC_ERROR_TIMEOUT;
     }
@@ -1534,13 +1529,11 @@ uint32_t SDMMC_GetCmdResp2(SDMMC_TypeDef *SDMMCx)
 uint32_t SDMMC_GetCmdResp3(SDMMC_TypeDef *SDMMCx)
 {
   uint32_t sta_reg;
-  /* 8 is the number of required instructions cycles for the below loop statement.
-  The SDMMC_CMDTIMEOUT is expressed in ms */
-  uint32_t count = SDMMC_CMDTIMEOUT * (SystemCoreClock / 8U / 1000U);
+  uint32_t tick_start = HAL_GetTick();

   do
   {
-    if (count-- == 0U)
+    if ((HAL_GetTick() - tick_start) > SDMMC_CMDTIMEOUT)
     {
       return SDMMC_ERROR_TIMEOUT;
     }
@@ -1572,14 +1565,11 @@ uint32_t SDMMC_GetCmdResp3(SDMMC_TypeDef *SDMMCx)
 uint32_t SDMMC_GetCmdResp4(SDMMC_TypeDef *SDMMCx, uint32_t *pResp)
 {
   uint32_t sta_reg;
-
-  /* 8 is the number of required instructions cycles for the below loop statement.
-  The SDMMC_CMDTIMEOUT is expressed in ms */
-  uint32_t count = SDMMC_CMDTIMEOUT * (SystemCoreClock / 8U / 1000U);
+  uint32_t tick_start = HAL_GetTick();

   do
   {
-    if (count-- == 0U)
+    if ((HAL_GetTick() - tick_start) > SDMMC_CMDTIMEOUT)
     {
       return SDMMC_ERROR_TIMEOUT;
     }
@@ -1619,14 +1609,11 @@ uint32_t SDMMC_GetCmdResp5(SDMMC_TypeDef *SDMMCx, uint8_t SDIO_CMD, uint8_t *pDa
 {
   uint32_t response_r5;
   uint32_t sta_reg;
-
-  /* 8 is the number of required instructions cycles for the below loop statement.
-  The SDMMC_CMDTIMEOUT is expressed in ms */
-  uint32_t count = SDMMC_CMDTIMEOUT * (SystemCoreClock / 8U / 1000U);
+  uint32_t tick_start = HAL_GetTick();

   do
   {
-    if (count-- == 0U)
+    if ((HAL_GetTick() - tick_start) > SDMMC_CMDTIMEOUT)
     {
       return SDMMC_ERROR_TIMEOUT;
     }
@@ -1707,14 +1694,11 @@ uint32_t SDMMC_GetCmdResp6(SDMMC_TypeDef *SDMMCx, uint8_t SD_CMD, uint16_t *pRCA
 {
   uint32_t response_r1;
   uint32_t sta_reg;
-
-  /* 8 is the number of required instructions cycles for the below loop statement.
-  The SDMMC_CMDTIMEOUT is expressed in ms */
-  uint32_t count = SDMMC_CMDTIMEOUT * (SystemCoreClock / 8U / 1000U);
+  uint32_t tick_start = HAL_GetTick();

   do
   {
-    if (count-- == 0U)
+    if ((HAL_GetTick() - tick_start) > SDMMC_CMDTIMEOUT)
     {
       return SDMMC_ERROR_TIMEOUT;
     }
@@ -1780,13 +1764,11 @@ uint32_t SDMMC_GetCmdResp6(SDMMC_TypeDef *SDMMCx, uint8_t SD_CMD, uint16_t *pRCA
 uint32_t SDMMC_GetCmdResp7(SDMMC_TypeDef *SDMMCx)
 {
   uint32_t sta_reg;
-  /* 8 is the number of required instructions cycles for the below loop statement.
-  The SDMMC_CMDTIMEOUT is expressed in ms */
-  uint32_t count = SDMMC_CMDTIMEOUT * (SystemCoreClock / 8U / 1000U);
+  uint32_t tick_start = HAL_GetTick();

   do
   {
-    if (count-- == 0U)
+    if ((HAL_GetTick() - tick_start) > SDMMC_CMDTIMEOUT)
     {
       return SDMMC_ERROR_TIMEOUT;
     }
@@ -2077,13 +2059,11 @@ uint32_t SDMMC_DMALinkedList_DisableCircularMode(SDMMC_DMALinkedListTypeDef *pLi
   */
 static uint32_t SDMMC_GetCmdError(SDMMC_TypeDef *SDMMCx)
 {
-  /* 8 is the number of required instructions cycles for the below loop statement.
-  The SDMMC_CMDTIMEOUT is expressed in ms */
-  uint32_t count = SDMMC_CMDTIMEOUT * (SystemCoreClock / 8U / 1000U);
+  uint32_t tick_start = HAL_GetTick();

   do
   {
-    if (count-- == 0U)
+    if ((HAL_GetTick() - tick_start) > SDMMC_CMDTIMEOUT)
     {
       return SDMMC_ERROR_TIMEOUT;
     }

To apply, call this under project folder (where you see Drivers subfolder):

git apply sdmmc_ll_timeout.patch

This call can be put into CubeMX post-generation batch / bash file to apply automatically.