+ Jean, Ludovic
On Thu, 1 Jul 2021 at 00:33, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> On Wed, Jun 30, 2021 at 5:28 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> > > /* Handle busy detection on DAT0 if the variant supports it. */
> > > if (busy_resp && host->variant->busy_detect)
> > > if (!host->ops->busy_complete(host, status, err_msk))
> > > return;
> > >
> > > These seemed to be especially problematic to me.
> >
> > Yes, exactly. The IRQ based busy detection code gets disabled with my
> > debug patch.
> >
> > It looks like there are some race conditions in the HW busy detection
> > path for mmci, which gets triggered by this eMMC card.
> (...)
> > Although, it's more optimal to receive an IRQ when busy on DAT0 is
> > de-asserted, rather than polling with ->card_busy(). Hence we also
> > have MMC_CAP_WAIT_WHILE_BUSY.
>
> Hmmmmm it kind of assumes that DAT0 will be de-asserted *before*
> we get a command response, never after. I think that is what the card
> is doing. If that is out-of-spec then we need to have a quirk like
> this but if it is legal behaviour, we rather need to fix the mmci driver.
That's correct and this could very well be the reason why polling
works better for this case.
On the other hand, I am still a bit puzzled why the mmci driver hangs,
waiting for the busy completion IRQ to be raised.
I did some more inspection of the code in ux500_busy_complete() and
found that there may be a potential race condition. I tried to fix it
up, but I don't know if it really makes any difference. Can you please
test the below patch and see if it helps.
---
From: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
Date: Thu, 1 Jul 2021 13:12:48 +0200
Subject: [PATCH] mmc: mmci: Fix busy detect completion
One of the pre-conditions to set the ->busy_detect_mask in the MMCIMASK0
register, is to first re-read the MMCISTATUS register to verify that the
->busy_detect_flag is set. The intent is to avoid enabling IRQ based busy
completion if the card does not signal busy.
Assuming the busy_detect_flag is set, we enter a small window before the
actual write of the ->busy_detect_mask hits the HW. In this window, the
->busy_detect_flag in the MMCISTATUS register may change to not indicate
busy any more. This could lead to that we end up waiting for a busy
completion IRQ forever.
Fix this, by writing the ->busy_detect_mask to the MMCIMASK0 first, but
clear it if it turns out that the card wasn't signaling busy.
Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
---
drivers/mmc/host/mmci.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 984d35055156..122de99759a5 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -671,14 +671,20 @@ static bool ux500_busy_complete(struct mmci_host
*host, u32 status, u32 err_msk)
* while, to allow it to be set, but tests indicates that it
* isn't needed.
*/
- if (!host->busy_status && !(status & err_msk) &&
- (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
- writel(readl(base + MMCIMASK0) |
- host->variant->busy_detect_mask,
+ if (!host->busy_status && !(status & err_msk)) {
+ u32 mask0 = readl(base + MMCIMASK0);
+
+ writel(mask0 | host->variant->busy_detect_mask,
base + MMCIMASK0);
+ wmb();
- host->busy_status = status & (MCI_CMDSENT | MCI_CMDRESPEND);
- return false;
+ if (readl(base + MMCISTATUS) &
+ host->variant->busy_detect_flag) {
+ host->busy_status = status &
+ (MCI_CMDSENT | MCI_CMDRESPEND);
+ return false;
+ }
+ writel(mask0, base + MMCIMASK0);
}
/*
--
Kind regards
Uffe
