On Tue, 13 Jun 2023 at 22:34, 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 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..3b1a23e4ec1c 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) {
I realize that I have already queued up $subject patch, but I haven't
been able to test this myself yet.
However, I wonder if the above is really correct? More precisely, did
you confirm that the variant->busy_detect_flag-bit becomes set for the
ux500 variant, when the card stops signaling busy? If not, we will
always end up in the else clause below?
> + host->busy_status |= status & (MCI_CMDSENT | MCI_CMDRESPEND);
> + writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> + host->busy_state = MMCI_BUSY_DONE;
> + return false;
> + } else {
> + dev_dbg(mmc_dev(host->mmc),
> + "lost busy status when waiting for busy end 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;
> + }
> }
>
[...]
Kind regards
Uffe
