On Wed, 5 Apr 2023 at 08:50, Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
