| Message ID | 20241008003503.1914718-1-jonas@kwiboo.se |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | mmc: dw_mmc: Fix FIFO data transfer timeout | expand |
> Subject: [PATCH] mmc: dw_mmc: Fix FIFO data transfer timeout > > The commit 0252924ac6d4 ("mmc: dw_mmc: Extract FIFO data > transfer into a separate routine") unintentionally changed behavior of > the FIFO data transfer routine. > > When data is read and size reaches 0 the original loop would wait on > DWMCI_INTMSK_DTO or timeout. The remaining size to read is no > longer tracked across dwmci_data_transfer_fifo() calls and because of > this an extra call to fifo() and dwmci_fifo_ready() may now trigger a > FIFO underflow timeout. > > Buswidth = 4, clock: 50000000 > Sending CMD16 > Sending CMD17 > dwmci_fifo_ready: FIFO underflow timeout > Sending CMD16 > Sending CMD18 > dwmci_fifo_ready: FIFO underflow timeout > Sending CMD12 > ## Checking hash(es) for config config-1 ... OK > > Restore old behavior and track remaining size to read across calls to fix > this. > > Fixes: 0252924ac6d4 ("mmc: dw_mmc: Extract FIFO data transfer into > a separate routine") > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> LGTM: Reviewed-by: Peng Fan <peng.fan@nxp.com>
Hi, On 2024-10-08 03:19, Peng Fan wrote: >> Subject: [PATCH] mmc: dw_mmc: Fix FIFO data transfer timeout >> >> The commit 0252924ac6d4 ("mmc: dw_mmc: Extract FIFO data >> transfer into a separate routine") unintentionally changed behavior of >> the FIFO data transfer routine. >> >> When data is read and size reaches 0 the original loop would wait on >> DWMCI_INTMSK_DTO or timeout. The remaining size to read is no >> longer tracked across dwmci_data_transfer_fifo() calls and because of >> this an extra call to fifo() and dwmci_fifo_ready() may now trigger a >> FIFO underflow timeout. >> >> Buswidth = 4, clock: 50000000 >> Sending CMD16 >> Sending CMD17 >> dwmci_fifo_ready: FIFO underflow timeout >> Sending CMD16 >> Sending CMD18 >> dwmci_fifo_ready: FIFO underflow timeout >> Sending CMD12 >> ## Checking hash(es) for config config-1 ... OK >> >> Restore old behavior and track remaining size to read across calls to fix >> this. >> >> Fixes: 0252924ac6d4 ("mmc: dw_mmc: Extract FIFO data transfer into >> a separate routine") >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > > LGTM: Reviewed-by: Peng Fan <peng.fan@nxp.com> Thanks, however after looking at this with fresh eyes a full revert of the offending commit may instead be a better option. The target/source buf is also not carried over and is reset for each call to fifo(), not sure if resuming a read or write is even needed/useful/wanted/possible. I will send a v2 with a revert of the offending commit later after more testing. Someone can then take a new look at refactoring FIFO handling. Regards, Jonas
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8551eac70185..98b85fd1795a 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -214,16 +214,15 @@ static unsigned int dwmci_get_timeout(struct mmc *mmc, const unsigned int size) return timeout; } -static int dwmci_data_transfer_fifo(struct dwmci_host *host, - struct mmc_data *data, u32 mask) +static u32 dwmci_data_transfer_fifo(struct dwmci_host *host, + struct mmc_data *data, u32 size, u32 mask) { const u32 int_rx = mask & (DWMCI_INTMSK_RXDR | DWMCI_INTMSK_DTO); const u32 int_tx = mask & DWMCI_INTMSK_TXDR; int ret = 0; - u32 len = 0, size, i; + u32 len = 0, i; u32 *buf; - size = (data->blocksize * data->blocks) / 4; if (!host->fifo_mode || !size) return 0; @@ -261,7 +260,7 @@ static int dwmci_data_transfer_fifo(struct dwmci_host *host, dwmci_writel(host, DWMCI_RINTSTS, DWMCI_INTMSK_TXDR); } - return ret; + return size; } static int dwmci_data_transfer(struct dwmci_host *host, struct mmc_data *data) @@ -273,6 +272,7 @@ static int dwmci_data_transfer(struct dwmci_host *host, struct mmc_data *data) size = data->blocksize * data->blocks; timeout = dwmci_get_timeout(mmc, size); + size /= 4; for (;;) { mask = dwmci_readl(host, DWMCI_RINTSTS); @@ -283,7 +283,7 @@ static int dwmci_data_transfer(struct dwmci_host *host, struct mmc_data *data) break; } - ret = dwmci_data_transfer_fifo(host, data, mask); + size = dwmci_data_transfer_fifo(host, data, size, mask); /* Data arrived correctly */ if (mask & DWMCI_INTMSK_DTO) {
The commit 0252924ac6d4 ("mmc: dw_mmc: Extract FIFO data transfer into a separate routine") unintentionally changed behavior of the FIFO data transfer routine. When data is read and size reaches 0 the original loop would wait on DWMCI_INTMSK_DTO or timeout. The remaining size to read is no longer tracked across dwmci_data_transfer_fifo() calls and because of this an extra call to fifo() and dwmci_fifo_ready() may now trigger a FIFO underflow timeout. Buswidth = 4, clock: 50000000 Sending CMD16 Sending CMD17 dwmci_fifo_ready: FIFO underflow timeout Sending CMD16 Sending CMD18 dwmci_fifo_ready: FIFO underflow timeout Sending CMD12 ## Checking hash(es) for config config-1 ... OK Restore old behavior and track remaining size to read across calls to fix this. Fixes: 0252924ac6d4 ("mmc: dw_mmc: Extract FIFO data transfer into a separate routine") Signed-off-by: Jonas Karlman <jonas@kwiboo.se> --- Reading FIT from SD-card take ~20-30 seconds on a Rockchip RK3328 board without this fixed, and goes back to sub second once fixed. --- drivers/mmc/dw_mmc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)