[Linux-stm32] [PATCH v3 06/10] mmc: mmci: Retry the busy start condition

[Ulf Hansson]

On Sun, 11 Jun 2023 at 21:41, Linus Walleij <linus.walleij at linaro.org> 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 at linaro.org>
> ---
> 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