Re: [PATCH 1/1] mmc: meson-mx-sdio: Set MMC_CAP_WAIT_WHILE_BUSY

[Ulf Hansson]

On Wed, 15 Apr 2020 at 23:24, Martin Blumenstingl
<martin.blumenstingl@xxxxxxxxxxxxxx> wrote:
>
> Hi Ulf,
>
> thank you very much for taking the time to look into this!
>
> On Wed, Apr 15, 2020 at 2:57 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> [...]
> > Thanks for sending this! I assume it's a regression and caused by one
> > of my patches that went in for 5.7. Probably this one:
> > 0d84c3e6a5b2 mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard
> indeed, I only observed this with 5.7-rc1-ish, before everything was
> working fine
>
> > Now, even if enabling MMC_CAP_WAIT_WHILE_BUSY seems like the correct
> > thing to do, I suggest we really try understand why it works, so we
> > don't overlook some other issue that needs to be fixed.
> great, that's why I'm seeking for help!
>
> > Would you be willing to try a few debug patches, according to the below?
> sure
> while reading your suggestions I went back to the vendor driver and
> observed that they don't implement card_busy for this controller
> Thus I added the following line to meson_mx_mmc_card_busy for all of
> your tests to see what the controller sees in terms of our card busy
> implementation:
>   dev_info(mmc_dev(host->mmc), "%s read IRQC = 0x%08x\n",
>                  __func__, irqc);
>
> > First, can you double check so the original polling with CMD13 is
> > still okay, by trying the below minor change. The intent is to force
> > polling with CMD13 for the erase/discard operation.
> I have tried this one and it seems to work around the problem (before
> I reverted my change and dropped MMC_CAP_WAIT_WHILE_BUSY from
> mmc->caps)
> also I did not see meson_mx_mmc_card_busy being invoked (not even
> once, but I don't know if that's expected)

For eMMC it should be used quite frequently, as CMD6 is sent quite
often, during initialization for example (see mmc_switch() and
__mmc_switch()).

For SD cards, it's being used for erase/trim/discard and while
changing to UHS-I speed modes (1.8V I/O voltage, see
mmc_set_uhs_voltage(). The latter also requires your host driver to
implement the ->start_signal_voltage_switch() host ops, which isn't
the case (yet!?)

For SDIO cards it's being used in-between requests to make sure the
SDIO card is ready for the next command (see __mmc_start_request())

>
> [...]
> > Second, if the above works, it looks like the polling with
> > ->card_busy() isn't really working for meson-mx-sdio.c, together with
> > erase/discard. To narrow down that problem, I suggest to try with a
> > longer erase/discard timeout in a retry fashion, while using
> > ->card_busy(). Along the lines of the below:
> I have tried this one as well (before I reverted the earlier CMD13
> patch) and with MMC_CAP_WAIT_WHILE_BUSY unset in mmc->caps
> This doesn't seem to work around the issue - kernel log extract attached.
> Also I'm seeing that the the current meson_mx_mmc_card_busy
> implementation returns that the card is busy.
> example: 0x1f001f10 & 0x3c00 = 0x1c00. the busy logic in the driver
> is: !!0x1c00 = 1
>
> My conclusion is:
> - meson_mx_mmc_card_busy is not working and should be removed (because
> I don't know how to make it work). it probably never worked but we
> didn't notice until a recent change

I see.

Depending on what your driver plans to support for the future, see
above, you may need to come back to this in future.

> - set MMC_CAP_WAIT_WHILE_BUSY as per my initial patch
> - use Fixes: ed80a13bb4c4c9 ("mmc: meson-mx-sdio: Add a driver for the
> Amlogic Meson8 and Meson8b SoCs")
>
> Does this make sense?

Yes, I think so.

> Also please let me know if you want me to try something else

I would also suggest adding a patch that removes the ->card_busy() ops
from the meson driver - and that should probably also carry the same
fixes tag as above. Just to make sure the callback doesn't get used in
some other circumstances, when going forward.

Kind regards
Uffe