Messages in this thread Patch in this message | | | Subject | Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling | From | Ludovic BARRE <> | Date | Thu, 25 Apr 2019 11:22:37 +0200 |
| |
hi Ulf
On 4/23/19 3:39 PM, Ulf Hansson wrote: > On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote: >> >> From: Ludovic Barre <ludovic.barre@st.com> >> >> The busy status bit could occurred even if no busy response is >> expected (example cmd11). On sdmmc variant, the busy_detect_flag >> reflects inverted value of d0 state, it's sampled at the end of a >> CMD response and a second time 2 clk cycles after the CMD response. >> To avoid a fake busy, the busy status could be checked and polled >> only if the command has RSP_BUSY flag. > > I would appreciate a better explanation of what this patch really changes. > > The above is giving some background to the behavior of sdmmc variant, > but at this point that variant doesn't even have the > ->variant->busy_detect flag set. >
Yes, I will try to explain more and focus on common behavior.
>> >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >> --- >> drivers/mmc/host/mmci.c | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index 387ff14..4901b73 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >> unsigned int status) >> { >> void __iomem *base = host->base; >> - bool sbc; >> + bool sbc, busy_resp; >> >> if (!cmd) >> return; >> >> sbc = (cmd == host->mrq->sbc); >> + busy_resp = !!(cmd->flags & MMC_RSP_BUSY); >> >> /* >> * We need to be one of these interrupts to be considered worth >> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >> /* >> * ST Micro variant: handle busy detection. >> */ >> - if (host->variant->busy_detect) { >> - bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY); >> + if (busy_resp && host->variant->busy_detect) { >> >> /* We are busy with a command, return */ >> if (host->busy_status && >> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >> * that the special busy status bit is still set before >> * proceeding. >> */ >> - if (!host->busy_status && busy_resp && >> + if (!host->busy_status && >> !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) && >> (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) { > > All the changes above makes perfect sense to me, but looks more like a > cleanup of the code, rather than actually changing the behavior.
yes, few changing (this just avoid to enter in "if (host->variant->busy_detect)") at each time. I could move this part in cleanup patch (before this patch)
> >> >> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id) >> { >> struct mmci_host *host = dev_id; >> u32 status; >> + bool busy_resp; >> int ret = 0; >> >> spin_lock(&host->lock); >> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id) >> } >> >> /* >> - * Don't poll for busy completion in irq context. >> + * Don't poll for: >> + * -busy completion in irq context. >> + * -no busy response expected. >> */ >> - if (host->variant->busy_detect && host->busy_status) >> + busy_resp = host->cmd ? >> + !!(host->cmd->flags & MMC_RSP_BUSY) : false; > > This doesn't make sense to me, but I may be missing something. > > host->busy_status is being updated by mmci_cmd_irq() and only when > MMC_RSP_BUSY is set for the command in flight. In other words, > checking for MMC_RSP_BUSY here as well is redundant. No?
In mmci_irq the "do while" loops until the status is totally cleared.
Today (for variant with busy_detect option), the status busy_detect_flag is excluded only while busy_status period (command with MMC_RSP_BUSY and while busy line is low => "busy_status=1")
On SDMMC variant I noticed that busy_detect_flag status could be enabled even if the command is not MMC_RSP_BUSY, for example sdmmc variant stay in loop while cmd11 voltage switch.
So I wish extend host->variant->busy_detect_flag exclusion for all commands which is not a MMC_RSP_BUSY. I suppose that other variants could have the same behavior, and else there is no impact, normally.
> >> + >> + if (host->variant->busy_detect && >> + (!busy_resp || host->busy_status)) >> status &= ~host->variant->busy_detect_flag; >> >> ret = 1; >> -- >> 2.7.4 >> > > Kind regards > Uffe >
| |