On Sun, 9 Apr 2023 at 00:00, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> This refactors the ->busy_complete() callback currently
> only used by Ux500 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.
Nitpick: The callback is being used by the mmci_stm32_sdmmc variant too.
>
> 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 four states:
> MMCI_BUSY_NOT_STARTED, MMCI_BUSY_WAITING_FOR_IRQS,
> MMCI_BUSY_START_IRQ and MMCI_BUSY_END_IRQ.
This makes perfect sense to me too. However, these don't really match
the names of enum types you eventually decided to use.
To be consistent, perhaps it's just easier to drop the actual enum
names from the commit message?
>
> The 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 dev_err() 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.
I agree, this whole thing has been fragile. Your observations seem
reasonable to me too.
>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v1->v2:
> - No changes
> ---
> drivers/mmc/host/mmci.c | 55 +++++++++++++++++++++++++++++++------
> drivers/mmc/host/mmci.h | 16 +++++++++++
> drivers/mmc/host/mmci_stm32_sdmmc.c | 6 +++-
> 3 files changed, 68 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 7d42625f2356..887b83e392a4 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_IDLE) {
> 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_IRQS;
> return false;
> }
> }
> @@ -710,11 +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_IRQS) {
> + 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_START_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 (host->busy_state == MMCI_BUSY_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_END_IRQ;
> + 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;
> + }
> }
>
> /*
> @@ -723,11 +754,18 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> * 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);
> + if (host->busy_state == MMCI_BUSY_END_IRQ) {
> + if (status & host->variant->busy_detect_flag) {
> + /* We should just get two IRQs for busy detect */
> + dev_err(mmc_dev(host->mmc), "spurious busy detect IRQ\n");
> + return false;
> + }
>
> + 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;
> }
>
> @@ -1258,6 +1296,7 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
> }
>
> if (cmd->flags & MMC_RSP_BUSY) {
> + host->busy_state = MMCI_BUSY_IDLE;
> host->busy_status = 0;
>
> if (host->variant->busy_timeout) {
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index e1a9b96a3396..82f3850325c8 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -261,6 +261,21 @@ struct clk;
> struct dma_chan;
> struct mmci_host;
>
> +/**
> + * enum mmci_busy_state - enumerate the busy detect wait states
> + *
> + * This is used for the state machine waiting for different busy detect
> + * interrupts on hardware that fire a single IRQ for start and end of
> + * the busy detect phase on DAT0.
> + */
> +enum mmci_busy_state {
> + MMCI_BUSY_IDLE,
This state name is a bit confusing to me. If I understand correctly,
this state means that the sent command has a corresponding busy
signaling that should be checked for, right? Can we perhaps reflect
that in the name somehow?
> + MMCI_BUSY_WAITING_FOR_IRQS,
> + MMCI_BUSY_START_IRQ,
> + MMCI_BUSY_END_IRQ,
> + MMCI_BUSY_DONE,
> +};
> +
> /**
> * struct variant_data - MMCI variant-specific quirks
> * @clkreg: default value for MCICLOCK register
> @@ -409,6 +424,7 @@ struct mmci_host {
> u32 clk_reg;
> u32 clk_reg_add;
> u32 datactrl_reg;
> + enum mmci_busy_state busy_state;
> u32 busy_status;
> u32 mask1_reg;
> u8 vqmmc_enabled:1;
> diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
> index 60bca78a72b1..24831a1092b2 100644
> --- a/drivers/mmc/host/mmci_stm32_sdmmc.c
> +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
> @@ -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_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);
>
Overall, I have to admit that I think the code is getting harder to
follow again, even if the end result from the series looks really
nice.
Besides making sure that each step/patch compiles, I am also worried
that we may be breaking things on the way. That said, I wonder if it's
not better to actually squash some of the patches in the series, to
avoid churns - can you please look into that?
Kind regards
Uffe
