Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200410213043.1091851-1-martin.blumenstingl@googlemail.com>
 <CAPDyKFpsTTUaTLy--DvOr0oAB71cTBdH1pfTV63LtcKOKOsnDw@mail.gmail.com>
 <CAFBinCAhfgEdT383x61mh9E0y3-3MLA-fUmZj-4C-_VUdGWtGQ@mail.gmail.com> <CAPDyKFrj6jdHe8OSvtuT7=QdreZ1YzMt=791ABSZfos8yux71A@mail.gmail.com>
In-Reply-To: <CAPDyKFrj6jdHe8OSvtuT7=QdreZ1YzMt=791ABSZfos8yux71A@mail.gmail.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Thu, 16 Apr 2020 19:57:10 +0200
Message-ID: <CAFBinCDT08bts6xfNkTV-aqgOq=jDo+BQHGOVOQyFjRSQJpBvA@mail.gmail.com>
Subject: Re: [PATCH 1/1] mmc: meson-mx-sdio: Set MMC_CAP_WAIT_WHILE_BUSY
To:     Ulf Hansson <ulf.hansson@linaro.org>
Cc:     "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        "open list:ARM/Amlogic Meson..." <linux-amlogic@lists.infradead.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Ulf,

On Thu, Apr 16, 2020 at 1:26 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
[...]
> > > 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()).
I only tested the meson-mx-sdio driver with eMMC once (a long time
ago) and it did not work.
...maybe this is the reason ;)

> 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!?)
SD cards and SDIO cards are the main use-case for this driver.
I don't know of any board which connects this controller to a card
with 1.8V (or 1.8V/3.3V configurable) I/O voltage. This is why I
didn't care about ->start_signal_voltage_switch() so far

[...]
> > > 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.
I'll let future Martin deal with that - he can add it back as needed ;-)
current Martin has his doubts that it'll be needed (see above)

> > - 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.
thank you for double-checking!

> > 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.
agreed, I will send a v2 later which adds the Fixes tag, a bit more
description and an additional patch to remove ->card_busy()


Thank you again very much for the insights!
Have a great day,
Martin