[v4,10/10] mmc: mmci: Add busydetect timeout

Add a timeout for busydetect IRQs using a delayed work.
It might happen (and does happen) on Ux500 that the first
busy detect IRQ appears and not the second one. This will
make the host hang indefinitely waiting for the second
IRQ to appear.

Calculate the busy timeout unconditionally in
mmci_start_command() using the code developed for STM32
and use this as a timeout for the command.

This makes the eMMC work again on Skomer.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- Use the calculated command busy timeout from the core
  or the same calculated default as for STM32.
ChangeLog v2->v3:
- Rebased.
ChangeLog v1->v2:
- No changes
---
 drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++++---
 drivers/mmc/host/mmci.h |  1 +
 2 files changed, 28 insertions(+), 3 deletions(-)

On Tue, 13 Jun 2023 at 22:34, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Add a timeout for busydetect IRQs using a delayed work.
> It might happen (and does happen) on Ux500 that the first
> busy detect IRQ appears and not the second one. This will
> make the host hang indefinitely waiting for the second
> IRQ to appear.
>
> Calculate the busy timeout unconditionally in
> mmci_start_command() using the code developed for STM32
> and use this as a timeout for the command.
>
> This makes the eMMC work again on Skomer.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v3->v4:
> - Use the calculated command busy timeout from the core
>   or the same calculated default as for STM32.
> ChangeLog v2->v3:
> - Rebased.
> ChangeLog v1->v2:
> - No changes
> ---
>  drivers/mmc/host/mmci.c | 30 +++++++++++++++++++++++++++---
>  drivers/mmc/host/mmci.h |  1 +
>  2 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 478f71dc7f34..12df1c231177 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -37,6 +37,7 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/workqueue.h>
>
>  #include <asm/div64.h>
>  #include <asm/io.h>
> @@ -740,6 +741,8 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
>                         host->busy_status |= status & (MCI_CMDSENT | MCI_CMDRESPEND);
>                         writel(host->variant->busy_detect_mask, base + MMCICLEAR);
>                         host->busy_state = MMCI_BUSY_WAITING_FOR_END_IRQ;

Shouldn't we schedule the work at the point when we move to
MMCI_BUSY_WAITING_FOR_START_IRQ instead?

At least, it's from that point in time that we detect that the card
signals busy. Moreover, at least theoretically, we could end up
hanging/waiting for the busy start irq too, right?

> +                       schedule_delayed_work(&host->busy_timeout_work,
> +                                             msecs_to_jiffies(host->cmd->busy_timeout));
>                 } else {
>                         dev_dbg(mmc_dev(host->mmc),
>                                 "lost busy status when waiting for busy start IRQ\n");
> @@ -751,6 +754,7 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
>                 if (status & host->variant->busy_detect_flag) {
>                         host->busy_status |= status & (MCI_CMDSENT | MCI_CMDRESPEND);
>                         writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> +                       cancel_delayed_work_sync(&host->busy_timeout_work);
>                         ux500_busy_clear_mask_done(host);
>                 } else {
>                         dev_dbg(mmc_dev(host->mmc),
> @@ -1295,10 +1299,11 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
>         host->busy_status = 0;
>         host->busy_state = MMCI_BUSY_DONE;
>
> -       if (host->variant->busy_timeout && cmd->flags & MMC_RSP_BUSY) {
> -               if (!cmd->busy_timeout)
> -                       cmd->busy_timeout = 10 * MSEC_PER_SEC;
> +       /* Assign a default timeout if the core does not provide one */
> +       if (!cmd->busy_timeout)
> +               cmd->busy_timeout = 10 * MSEC_PER_SEC;
>
> +       if (host->variant->busy_timeout && cmd->flags & MMC_RSP_BUSY) {
>                 if (cmd->busy_timeout > host->mmc->max_busy_timeout)
>                         clks = (unsigned long long)host->mmc->max_busy_timeout * host->cclk;
>                 else
> @@ -1486,6 +1491,22 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>         }
>  }
>
> +/*
> + * This busy timeout worker is used to "kick" the command IRQ if a
> + * busy detect IRQ fails to appear in reasonable time. Only used on
> + * variants with busy detection IRQ delivery.
> + */
> +static void busy_timeout_work(struct work_struct *work)

In a way to try to be consistent with naming functions, perhaps add
the prefix "ux500_*?

> +{
> +       struct mmci_host *host =
> +               container_of(work, struct mmci_host, busy_timeout_work.work);
> +       u32 status;
> +
> +       dev_dbg(mmc_dev(host->mmc), "timeout waiting for busy IRQ\n");
> +       status = readl(host->base + MMCISTATUS);
> +       mmci_cmd_irq(host, host->cmd, status);
> +}
> +
>  static int mmci_get_rx_fifocnt(struct mmci_host *host, u32 status, int remain)
>  {
>         return remain - (readl(host->base + MMCIFIFOCNT) << 2);
> @@ -2299,6 +2320,9 @@ static int mmci_probe(struct amba_device *dev,
>                         goto clk_disable;
>         }
>
> +       if (host->variant->busy_detect && host->ops->busy_complete)

The ->busy_detect bool, mandates the ->busy_complete() callback. There
is no need to check for it too.

> +               INIT_DELAYED_WORK(&host->busy_timeout_work, busy_timeout_work);
> +
>         writel(MCI_IRQENABLE | variant->start_err, host->base + MMCIMASK0);
>
>         amba_set_drvdata(dev, mmc);
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index 12a7bbd3ce26..95d3d0a6049b 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -451,6 +451,7 @@ struct mmci_host {
>         void                    *dma_priv;
>
>         s32                     next_cookie;
> +       struct delayed_work     busy_timeout_work;
>  };
>
>  #define dma_inprogress(host)   ((host)->dma_in_progress)
>

Other than the above, I am still not convinced that we don't have a
locking issue, as we discussed for the previous version. However,
let's continue that discussion in the other thread, separately.

Kind regards
Uffe

On Wed, Jun 14, 2023 at 2:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:

> Shouldn't we schedule the work at the point when we move to
> MMCI_BUSY_WAITING_FOR_START_IRQ instead?

Yup

> > +/*
> > + * This busy timeout worker is used to "kick" the command IRQ if a
> > + * busy detect IRQ fails to appear in reasonable time. Only used on
> > + * variants with busy detection IRQ delivery.
> > + */
> > +static void busy_timeout_work(struct work_struct *work)
>
> In a way to try to be consistent with naming functions, perhaps add
> the prefix "ux500_*?

I thought to actually propose a patch for STM32 to use this too, as an
extra fallback timeout. But I don't know if it's useful to them, so I'll
rename it.

Yours,
Linus Walleij