apache
drivers/mmcsd/mmcsd_sdio.c: use dma to receive setup if dma is enabled
Summary
SD card may suffers from reading problems on some devices including esp32s3. The root cause is, FIFO is not supported on it so far, so that SDIO_RECVSETUP does not work properly. So the solution is to call SDIO_DMARECVSETUP instead of SDIO_RECVSETUP if DMA is enabled. Which is also aligning with all the other callings of "recvsetup" in mmcsd_sdio.c.
Impact
drivers/mmcsd/mmcsd_sdio.c: use SDIO_DMARECVSETUP instead of SDIO_RECVSETUP if DMA is enabled in mmcsd_get_scr.
Testing
https://gist.github.com/Windrow14/19241fc1381013d8255d0df5d428c4e0
fatal: unable to access 'https://github.com/espressif/esp-hal-3rdparty.git/': Failed to connect to github.com port 443 after 135165 ms: Connection timed out
Build failed due to network problem, please re-trigger, thanks.
@Windrow14 have you tested this on stm32[f|h][7] arches?
CONFIG_MMCSD=y CONFIG_MMCSD_SDIO=y CONFIG_MMCSD_SDIOWAIT_WRCOMPLETE=y
for both SDMMC1 and 2?
SDMMC1 has more DMA restrictions then SDMMC2
@Windrow14 have you tested this on stm32[f|h][7] arches?
CONFIG_MMCSD=y CONFIG_MMCSD_SDIO=y CONFIG_MMCSD_SDIOWAIT_WRCOMPLETE=y
for both SDMMC1 and 2?
SDMMC1 has more DMA restrictions then SDMMC2
@davids5 We don't have hardware of these environments, so we are not able to do the tests.
With the newest update, all failed cases, including pre-flight failure and receiving setup through DMA failure, will drop to receiving setup without DMA, just like before this submit. For stm32 platform, pre-flight will not fail if other functions using it are working properly. DMARECVSETUP will fail because the buffer length to get SCR is too short to use DMA, so it will goes back to use RECVSETUP. Thus I think this change is safe on stm32 according to code logic.
@Windrow14
I was thinking about what you have run into on the ESPs. When I was working on the stm32h7 sdmmc driver, 5 years ago. I encountered as similar issue. This is how I described it and fixed it.
It basically does same thing as this PR, but because of a HW issue, I place it in the h7 driver. I think if you have the same IP block it may be the same issues and may be fixable in the arch driver and this driver does not need the change.
Thoughts?
@Windrow14 @davids5 should we merge this patch?
In my colleagues' and my opinion, we prefer https://github.com/apache/nuttx/commit/5e64f007c4050d001a4db60380186df28b710114 the most. It aligns the implementation of mmcsd_get_scr with all the other functions using SDIO_RECVSETUP such as mmcsd_readsingle and mmcsd_read_csd in the same file. Maybe we can ask the origin author of these functions to make a judgement if possible.
commit 7ef8423ad0bc34850129e9ecb8ed439fe9307c5e
Author: patacongo <patacongo@42af7a65-404d-4744-a932-0658087f49c3>
Date: Tue Nov 17 16:24:44 2009 +0000
Add basic data transfer logic
git-svn-id: svn://svn.code.sf.net/p/nuttx/code/trunk@2264 42af7a65-404d-4744-a932-0658087f49c3
In our consideration, the additional fallback and workaround suggested by David might be used in hardware drivers for different devices separately.
so, we can close this pr without merge?
Hi @xiaoxiang781216, sorry for late reply, I was on a long vacation. Just to clarify, the commit https://github.com/apache/nuttx/commit/5e64f007c4050d001a4db60380186df28b710114 I mentioned in earlier comment is a history version of this PR, not in another PR. In this version, pre-flight check is added as suggested, and fall-back to non-DMA interface mechanism is not added. Could do please reopen this PR? We still think this is a useful one.
Hi @davids5, in your suggested example in stm32h7, https://github.com/apache/nuttx/blob/master/arch/arm/src/stm32h7/stm32_sdmmc.c#L2342 IDMA register is used within "#ifndef CONFIG_STM32H7_SDMMC_IDMA". I doubt that all the other devices can use DMA registers without enable DMA, so I think we'd better not add that "fall-back" in common driver. I think if we use DMA and non-DMA interfaces strictly according to whether DMA is enabled or not in common driver code, then additional mechanisms in device specified code will not be affected.
@JackyCaoYJ
@Windrow14 please fix the spelling:
/home/runner/work/nuttx/nuttx/nuttx/drivers/mmcsd/mmcsd_sdio.c:2658: muti ==> multi
@Windrow14 please fix the spelling:
/home/runner/work/nuttx/nuttx/nuttx/drivers/mmcsd/mmcsd_sdio.c:2658: muti ==> multi
Fixed.