| Message ID | 20230405-pl180-busydetect-fix-v1-3-28ac19a74e5e@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix busydetect on Ux500 PL180/MMCI | expand |
On Wed, 5 Apr 2023 at 08:50, Linus Walleij <linus.walleij@linaro.org> wrote: > > This does two things: firsr replace the hard-to-read long > if-expression: > > if (!host->busy_status && !(status & err_msk) && > (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) { > > With the more readable: > > if (!host->busy_status && !(status & err_msk)) { > status = readl(base + MMCISTATUS); > if (status & host->variant->busy_detect_flag) { If I am reading the code below, this isn't what is being done. See more below. > > Second notice that the re-read MMCISTATUS register is now > stored into the status variable, using logic OR because what > if something else changed too? > > While we are at it, explain what the function is doing. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/mmc/host/mmci.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index 3e08b2e95550..3c1e11266fa9 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -654,6 +654,13 @@ static u32 ux500v2_get_dctrl_cfg(struct mmci_host *host) > return MCI_DPSM_ENABLE | (host->data->blksz << 16); > } > > +/* > + * ux500_busy_complete() - this will wait until the busy status > + * goes off, saving any status that occur in the meantime into > + * host->busy_status until we know the card is not busy any more. > + * The function returns true when the busy detection is ended > + * and we should continue processing the command. > + */ > static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk) > { > void __iomem *base = host->base; > @@ -671,14 +678,17 @@ 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 && !(status & err_msk) && > - (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) { > - writel(readl(base + MMCIMASK0) | > - host->variant->busy_detect_mask, > - base + MMCIMASK0); > - > + if (!host->busy_status && !(status & err_msk)) { > host->busy_status = status & (MCI_CMDSENT | MCI_CMDRESPEND); I wonder if this change is intentional. According to the commit message, I assume not. So, this may lead to that we end up giving host->busy_status a value, even if the busy_detect_flag bit isn't set in the status register. In other words, we could end up waiting for busy completion, while we shouldn't. > - return false; > + status = readl(base + MMCISTATUS); > + if (status & host->variant->busy_detect_flag) { > + writel(readl(base + MMCIMASK0) | > + host->variant->busy_detect_mask, > + base + MMCIMASK0); > + > + host->busy_status |= status & (MCI_CMDSENT | MCI_CMDRESPEND); > + return false; > + } > } > Kind regards Uffe
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index 3e08b2e95550..3c1e11266fa9 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -654,6 +654,13 @@ static u32 ux500v2_get_dctrl_cfg(struct mmci_host *host) return MCI_DPSM_ENABLE | (host->data->blksz << 16); } +/* + * ux500_busy_complete() - this will wait until the busy status + * goes off, saving any status that occur in the meantime into + * host->busy_status until we know the card is not busy any more. + * The function returns true when the busy detection is ended + * and we should continue processing the command. + */ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk) { void __iomem *base = host->base; @@ -671,14 +678,17 @@ 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 && !(status & err_msk) && - (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) { - writel(readl(base + MMCIMASK0) | - host->variant->busy_detect_mask, - base + MMCIMASK0); - + if (!host->busy_status && !(status & err_msk)) { host->busy_status = status & (MCI_CMDSENT | MCI_CMDRESPEND); - return false; + status = readl(base + MMCISTATUS); + if (status & host->variant->busy_detect_flag) { + writel(readl(base + MMCIMASK0) | + host->variant->busy_detect_mask, + base + MMCIMASK0); + + host->busy_status |= status & (MCI_CMDSENT | MCI_CMDRESPEND); + return false; + } } /*
This does two things: firsr replace the hard-to-read long if-expression: if (!host->busy_status && !(status & err_msk) && (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) { With the more readable: if (!host->busy_status && !(status & err_msk)) { status = readl(base + MMCISTATUS); if (status & host->variant->busy_detect_flag) { Second notice that the re-read MMCISTATUS register is now stored into the status variable, using logic OR because what if something else changed too? While we are at it, explain what the function is doing. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/mmc/host/mmci.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)