On Fri, 16 Jun 2023 at 10:30, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> This refactors the ->busy_complete() callback currently
> only used by Ux500 and STM32 to handle busy detection on
> hardware where one and the same IRQ is fired whether we get
> a start or an end signal on busy detect.
>
> The code is currently using the cached status from the
> command IRQ in ->busy_status as a state to select what to
> do next: if this state is non-zero we are waiting for
> IRQs and if it is zero we treat the state as the starting
> point for a busy detect wait cycle.
>
> Make this explicit by creating a state machine where the
> ->busy_complete callback moves between three states.
>
> The Ux500 busy detect code currently assumes this order:
> we enable the busy detect IRQ, get a busy start IRQ, then a
> busy end IRQ, and then we clear and mask this IRQ and
> proceed.
>
> We insert debug prints for unexpected states.
>
> Augment the STM32 driver with similar states for
> completeness.
>
> This works as before on most cards, however on a
> problematic card that is not working with busy detect, and
> which I have been debugging, this happens:
>
> [127220.662719] mmci-pl18x 80005000.mmc: lost busy status
> when waiting for busy end IRQ
>
> This probably means that the busy detect start IRQ has
> already occurred when we start executing the
> ->busy_complete() callbacks, and the busy detect end IRQ
> is counted as the start IRQ, and this is what is causing
> the card to not be detected properly.
>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v4->v6:
> - When waiting for the busy end IRQ, what we expect is that
> the IRQ should trigger right after the busy signal ends, and
> thus thes busy indication should be de-asserted (low). We
> invert the logic such that this is what we expect, and
> the error case is if the busy detect status is still high
> when we get the busy end IRQ.
> ChangeLog v3->v4:
> - Assign state MMCI_BUSY_DONE outside the if()-clause for
> the busy detect initialization.
> ChangeLog v2->v3:
> - Drop surplus states and merge IDLE and DONE states into one,
> we start out DONE. Name states *_WAITING_FOR_* so it is clear
> what is going on.
> - Rebase on other changes.
> - Reword commit message.
> ChangeLog v1->v2:
> - No changes
> ---
> drivers/mmc/host/mmci.c | 55 +++++++++++++++++++++++++------------
> drivers/mmc/host/mmci.h | 14 ++++++++++
> drivers/mmc/host/mmci_stm32_sdmmc.c | 6 +++-
> 3 files changed, 56 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index d632658d9d20..ccfca055f9a1 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -670,6 +670,7 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> 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;
> }
> @@ -687,7 +688,7 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> * while, to allow it to be set, but tests indicates that it
> * isn't needed.
> */
> - if (!host->busy_status) {
> + if (host->busy_state == MMCI_BUSY_DONE) {
> status = readl(base + MMCISTATUS);
> if (status & host->variant->busy_detect_flag) {
> writel(readl(base + MMCIMASK0) |
> @@ -695,6 +696,7 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> base + MMCIMASK0);
>
> host->busy_status = status & (MCI_CMDSENT | MCI_CMDRESPEND);
> + host->busy_state = MMCI_BUSY_WAITING_FOR_START_IRQ;
> return false;
> }
> }
> @@ -710,25 +712,40 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> * both the start and the end interrupts needs to be cleared,
> * one after the other. So, clear the busy start IRQ here.
> */
> - if (host->busy_status &&
> - (status & host->variant->busy_detect_flag)) {
> - host->busy_status |= status & (MCI_CMDSENT | MCI_CMDRESPEND);
> - writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> - return false;
> + if (host->busy_state == MMCI_BUSY_WAITING_FOR_START_IRQ) {
> + if (status & host->variant->busy_detect_flag) {
> + 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;
> + return false;
> + } else {
> + dev_dbg(mmc_dev(host->mmc),
> + "lost busy status when waiting for busy start IRQ\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;
> + }
> }
>
> - /*
> - * If there is a command in-progress that has been successfully
> - * sent and the busy bit isn't set, it means we have received
> - * the busy end IRQ. Clear and mask the IRQ, then continue to
> - * process the command.
> - */
> - if (host->busy_status) {
> - writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> -
> - writel(readl(base + MMCIMASK0) &
> - ~host->variant->busy_detect_mask, base + MMCIMASK0);
> - host->busy_status = 0;
> + if (host->busy_state == MMCI_BUSY_WAITING_FOR_END_IRQ) {
> + if (!(status & host->variant->busy_detect_flag)) {
> + host->busy_status |= status & (MCI_CMDSENT | MCI_CMDRESPEND);
> + host->busy_state = MMCI_BUSY_DONE;
> + return false;
> + } else {
> + dev_dbg(mmc_dev(host->mmc),
> + "busy status still asserted when handling busy end IRQ\n");
> + /* Force clear the spurious IRQ */
It may certainly be a spurious IRQ, but I am not sure that the below
is the correct thing to do in this case. I think it would be better to
continue to wait for another IRQ that could be the busy end IRQ.
Note that, the ->busy_detect_flag-bit in the status register can not
be trusted to be the root cause of the spurious IRQ. In fact, that bit
may not be linked to an IRQ at all, as it's dynamically updated and
can be polled too. See mmci_card_busy().
> + 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;
> + }
> }
>
[...]
> diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
> index 60bca78a72b1..a805647b6664 100644
> --- a/drivers/mmc/host/mmci_stm32_sdmmc.c
> +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
After a second thought, I think we should leave mmci_stm32_sdmmc as
is. There is no point in assigning the ->busy_state, unless we really
make use of it.
Moreover, I think sdmmc_busy_complete() is already quite simple to follow.
> @@ -393,8 +393,10 @@ static bool sdmmc_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> busy_d0 = sdmmc_status & MCI_STM32_BUSYD0;
>
> /* complete if there is an error or busy_d0end */
> - if ((status & err_msk) || busy_d0end)
> + if ((status & err_msk) || busy_d0end) {
> + host->busy_state = MMCI_BUSY_DONE;
> goto complete;
> + }
>
> /*
> * On response the busy signaling is reflected in the BUSYD0 flag.
> @@ -408,6 +410,7 @@ static bool sdmmc_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> host->busy_status = status &
> (MCI_CMDSENT | MCI_CMDRESPEND);
> }
> + host->busy_state = MMCI_BUSY_WAITING_FOR_END_IRQ;
> return false;
> }
>
> @@ -416,6 +419,7 @@ static bool sdmmc_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> writel_relaxed(mask & ~host->variant->busy_detect_mask,
> base + MMCIMASK0);
> host->busy_status = 0;
> + host->busy_state = MMCI_BUSY_DONE;
> }
>
> writel_relaxed(host->variant->busy_detect_mask, base + MMCICLEAR);
Kind regards
Uffe
