| Message ID | 20230405-pl180-busydetect-fix-v2-4-eeb10323b546@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix busydetect on Ux500 PL180/MMCI | expand |
On Sun, 9 Apr 2023 at 00:00, Linus Walleij <linus.walleij@linaro.org> wrote: > > The busy detect callback for Ux500 checks for an error > in the status in the first if() clause. The only practical > reason is that if an error occurs, the if()-clause is not > executed, and the code falls through to the last > if()-clause if (host->busy_status) which will clear and > disable the irq. Make this explicit instead: it is easier > to read. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - No changes > --- > drivers/mmc/host/mmci.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index e742dedaca1a..7d42625f2356 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -665,6 +665,15 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk) > { > void __iomem *base = host->base; > > + if (status & err_msk) { > + /* Stop any ongoing busy detection if an error occurs */ > + writel(host->variant->busy_detect_mask, base + MMCICLEAR); > + writel(readl(base + MMCIMASK0) & > + ~host->variant->busy_detect_mask, base + MMCIMASK0); > + host->busy_status = 0; > + return true; I agree that this makes the code more explicit, but unfortunately it also means duplicating the code from the bottom of this function. Can you instead add a helper function and call it from here and from the part at the bottom? > + } > + > /* > * Before unmasking for the busy end IRQ, confirm that the > * command was sent successfully. To keep track of having a > @@ -678,7 +687,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 && !(status & err_msk)) { > + if (!host->busy_status) { > status = readl(base + MMCISTATUS); > if (status & host->variant->busy_detect_flag) { > writel(readl(base + MMCIMASK0) | > Kind regards Uffe
On Mon, Apr 17, 2023 at 3:24 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Sun, 9 Apr 2023 at 00:00, Linus Walleij <linus.walleij@linaro.org> wrote: > > > > The busy detect callback for Ux500 checks for an error > > in the status in the first if() clause. The only practical > > reason is that if an error occurs, the if()-clause is not > > executed, and the code falls through to the last > > if()-clause if (host->busy_status) which will clear and > > disable the irq. Make this explicit instead: it is easier > > to read. > > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > --- > > ChangeLog v1->v2: > > - No changes > > --- > > drivers/mmc/host/mmci.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > > index e742dedaca1a..7d42625f2356 100644 > > --- a/drivers/mmc/host/mmci.c > > +++ b/drivers/mmc/host/mmci.c > > @@ -665,6 +665,15 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk) > > { > > void __iomem *base = host->base; > > > > + if (status & err_msk) { > > + /* Stop any ongoing busy detection if an error occurs */ > > + writel(host->variant->busy_detect_mask, base + MMCICLEAR); > > + writel(readl(base + MMCIMASK0) & > > + ~host->variant->busy_detect_mask, base + MMCIMASK0); > > + host->busy_status = 0; > > + return true; > > I agree that this makes the code more explicit, but unfortunately it > also means duplicating the code from the bottom of this function. > > Can you instead add a helper function and call it from here and from > the part at the bottom? I break that out as a separate function in patch 9, can you check if that solves the problem? The end result after all the patches should be fine. Yours, Linus Walleij
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index e742dedaca1a..7d42625f2356 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -665,6 +665,15 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk) { void __iomem *base = host->base; + if (status & err_msk) { + /* Stop any ongoing busy detection if an error occurs */ + writel(host->variant->busy_detect_mask, base + MMCICLEAR); + writel(readl(base + MMCIMASK0) & + ~host->variant->busy_detect_mask, base + MMCIMASK0); + host->busy_status = 0; + return true; + } + /* * Before unmasking for the busy end IRQ, confirm that the * command was sent successfully. To keep track of having a @@ -678,7 +687,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 && !(status & err_msk)) { + if (!host->busy_status) { status = readl(base + MMCISTATUS); if (status & host->variant->busy_detect_flag) { writel(readl(base + MMCIMASK0) |
The busy detect callback for Ux500 checks for an error in the status in the first if() clause. The only practical reason is that if an error occurs, the if()-clause is not executed, and the code falls through to the last if()-clause if (host->busy_status) which will clear and disable the irq. Make this explicit instead: it is easier to read. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v1->v2: - No changes --- drivers/mmc/host/mmci.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)