On Tue, 13 Jun 2023 at 22:34, Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
