On Sun, 11 Jun 2023 at 21:41, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> This makes the ux500 ->busy_complete() callback re-read
> the status register 10 times while waiting for the busy
> signal to assert in the status register.
>
> If this does not happen, we bail out regarding the
> command completed already, i.e. before we managed to
> start to check the busy status.
>
> There is a comment in the code about this, let's just
> implement it to be certain that we can catch this glitch
> if it happens.
>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v2->v3:
> - Rebased.
> ChangeLog v1->v2:
> - Move over the initial saving of host->busy_status from
> an unrelated patch to this one: it is clear what we are
> doing: we don't want to miss any transient
> (MCI_CMDSENT | MCI_CMDRESPEND) in the status register.
> ---
> drivers/mmc/host/mmci.c | 40 ++++++++++++++++++++++++++++------------
> 1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index edc68fc7b642..2858d8d67129 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -664,6 +664,7 @@ static u32 ux500v2_get_dctrl_cfg(struct mmci_host *host)
> static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> {
> void __iomem *base = host->base;
> + int retries = 10;
>
> if (status & err_msk) {
> /* Stop any ongoing busy detection if an error occurs */
> @@ -684,21 +685,36 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> * Note that, the card may need a couple of clock cycles before
> * it starts signaling busy on DAT0, hence re-read the
> * MMCISTATUS register here, to allow the busy bit to be set.
> - * Potentially we may even need to poll the register for a
> - * while, to allow it to be set, but tests indicates that it
> - * isn't needed.
> */
> if (host->busy_state == MMCI_BUSY_DONE) {
> - status = readl(base + MMCISTATUS);
> - if (status & host->variant->busy_detect_flag) {
> - writel(readl(base + MMCIMASK0) |
> - host->variant->busy_detect_mask,
> - base + MMCIMASK0);
> -
> - host->busy_status = status & (MCI_CMDSENT | MCI_CMDRESPEND);
> - host->busy_state = MMCI_BUSY_WAITING_FOR_START_IRQ;
> - return false;
> + /*
> + * Save the first status register read to be sure to catch
> + * all bits that may be lost will retrying. If the command
> + * is still busy this will result in assigning 0 to
> + * host->busy_status, which is what it should be in IDLE.
> + */
> + host->busy_status = status & (MCI_CMDSENT | MCI_CMDRESPEND);
> + while (retries) {
> + status = readl(base + MMCISTATUS);
> + if (status & host->variant->busy_detect_flag) {
> + writel(readl(base + MMCIMASK0) |
> + host->variant->busy_detect_mask,
> + base + MMCIMASK0);
> +
> + /* Keep accumulating status bits */
> + host->busy_status |= status & (MCI_CMDSENT | MCI_CMDRESPEND);
Looks like we should accumulate the status, no matter whether the
host->variant->busy_detect_flag is set?
Perhaps move this just before the if-clause above?
> + host->busy_state = MMCI_BUSY_WAITING_FOR_START_IRQ;
> + return false;
> + }
> + retries--;
> }
> + dev_dbg(mmc_dev(host->mmc), "no busy signalling in time\n");
> + writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> + writel(readl(base + MMCIMASK0) &
> + ~host->variant->busy_detect_mask, base + MMCIMASK0);
> + host->busy_state = MMCI_BUSY_DONE;
> + host->busy_status = 0;
> + return true;
> }
>
Kind regards
Uffe
