On Sun, 11 Jun 2023 at 21:41, 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.
>
> Fire a delayed work after 10ms and re-engage the command
> IRQ so the transaction finishes: we are certainly done
> at this point, or we will catch an error in the status
> register.
A fixed value of 10ms doesn't work. We have lots of commands that are
associated with way longer timeouts than this.
Typically, the cmd->busy_timeout contains the current value of the
timeout that should be used for the commands that have the flags
MMC_RSP_BUSY set for it.
The stm variant already uses cmd->busy_timeout, but also assigns a
default timeout, just to make sure if the core has failed to set
cmd->busy_timeout (it shouldn't but who knows).
A few more more comments to code, see below.
>
> This makes the eMMC work again on Skomer.
>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v2->v3:
> - Rebased.
> ChangeLog v1->v2:
> - No changes
> ---
> drivers/mmc/host/mmci.c | 23 +++++++++++++++++++++++
> drivers/mmc/host/mmci.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 05b8fad26c10..7e40b8f2dbf3 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>
> @@ -741,6 +742,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;
> + schedule_delayed_work(&host->busy_timeout_work,
> + msecs_to_jiffies(10));
> } else {
> dev_dbg(mmc_dev(host->mmc),
> "lost busy status when waiting for busy start IRQ\n");
> @@ -752,6 +755,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),
> @@ -1487,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)
> +{
> + 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);
There's no locking here. I assume that's because we call
cancel_delayed_work_sync() from an atomic context and we don't want
that to hang.
However, can't the call to mmci_cmd_irq() race with a proper irq being
managed in parallel?
> +}
> +
> static int mmci_get_rx_fifocnt(struct mmci_host *host, u32 status, int remain)
> {
> return remain - (readl(host->base + MMCIFIFOCNT) << 2);
> @@ -2300,6 +2320,9 @@ static int mmci_probe(struct amba_device *dev,
> goto clk_disable;
> }
>
> + if (host->variant->busy_detect && host->ops->busy_complete)
> + 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)
>
Kind regards
Uffe
