On 2019/3/19 22:41, Ulf Hansson wrote:
On Tue, 19 Mar 2019 at 10:14, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote:
Move it from dw_mci_start_command() to dw_mci_request().
Then dw_mci_wait_while_busy() isn't called with host's
lock hold.
So, I decided to have a closer look at this.
Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
Tested-by: Ziyuan Xu <xzy.xu@xxxxxxxxxxxxxx>
---
Changes in v3: None
Changes in v2: None
drivers/mmc/host/dw_mmc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 80dc2fd..703dedf 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -426,7 +426,6 @@ static void dw_mci_start_command(struct dw_mci *host,
mci_writel(host, CMDARG, cmd->arg);
wmb(); /* drain writebuffer */
- dw_mci_wait_while_busy(host, cmd_flags);
mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START);
@@ -1419,6 +1418,10 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
return;
}
+ if ((mrq->cmd->opcode != MMC_SEND_STATUS && mrq->cmd->data) &&
+ !(mrq->cmd->opcode == SD_SWITCH_VOLTAGE))
+ dw_mci_wait_while_busy(host, SDMMC_CMD_PRV_DAT_WAIT);
+
This looks weird, because according to the change-log it sounds like
you are moving things around to just avoid having the "lock" held.
To me, there seems to be more changes because of the new checks for
the "opcode" above, no?
yes, that's what original dw_mci_wait_while_busy() did internally.
Moreover, dw_mci_wait_while_busy() is also called from
mci_send_cmd(), which means it may still be called with the "lock" is
held. That is really confusing and needs more explanation.
dwmmc need to send a *NULL* cmd with SDMMC_CMD_UPD_CLK set in its CMD
register, in order to update bus clock, which is quite differnet from
others hosts. So mci_send_cmd()->dw_mci_wait_while_busy() is used for
updating bus clock but not in the hot path. So the main concern is to
move dw_mci_wait_while_busy() out from normal data requst.
spin_lock_bh(&host->lock);
dw_mci_queue_request(host, slot, mrq);
--
1.9.1
Finally, I am not sure I understand why dw_mci_wait_while_busy() is
called before starting a command, at all. That seems wrong to me.
Instead, shouldn't we look at cmd->flags & MMC_RSP_BUSY, for the
request in question and don't call mmc_request_done() for it, before
the card have stop signaled busy. At least that the behavior the mmc
core expects by the driver.
yes, that was from commit 0bdbd0e88cf6b603("mmc: dw_mmc: Don't start
commands while busy"), and it says "We'll leverage the existing dw_mmc
knowledge about whether it should wait for the previous command to
finish to know whether we should check for busy before sending the
command". I think the word, "leverage", explains the whole point, seems
a little tricky though.
Kind regards
Uffe
