[PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()
dianders at chromium.org
Thu Jan 29 16:13:34 PST 2015
On Thu, Jan 29, 2015 at 1:16 AM, Ulf Hansson <ulf.hansson at linaro.org> wrote:
> - Drastically decreased cc-list.
> On 29 January 2015 at 01:55, Doug Anderson <dianders at chromium.org> wrote:
>> On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson at linaro.org> wrote:
>>>> I asked Addy to post upstream against mmc_send_tuning(), but I guess
>>>> he didn't (he posted against Alex's NAKed patch instead).
>>>> ...when I talked to him about it, Addy was asserting that when tuning
>>>> fails it is important (at least on dw_mmc on rk3288) that we wait for
>>>> the card to stop being busy and that the way to detect was using
>>> So, could that be due to the internal logic of the error handling in
>>> dw_mmc driver? Or you think this is a generic issue?
>>> According to the specifications (eMMC and SD) both states that the
>>> tuning command has an R1 response. So, there shouldn't be any busy
>>> signalling involved - at least according to spec.
>> I did a bit of digging into this issue myself. What I found was that
>> a "response CRC" and "end of transfer". This was why I posted up
>> <https://patchwork.kernel.org/patch/5623071/>. From that patch:
>>> Specifically it looks like in certain error conditions (I saw this
>>> with Response CRC errors) that data keeps showing up in the FIFO even
>>> after the error is reported and the CD (command done) bit is set. If
>>> we don't wait for this data to finish transferring then it confuses
>>> the next transaction. In the specific failure case I ran into I found
>>> that I could monitor the data_state_mc_busy bit and wait for it to
>>> clear, but in other failure cases this bit was stuck at busy when we
>>> saw an error. Hence a generic big delay seems like the only option.
> I haven't queued that patch, I was waiting for an ack from Seungwon or Jaehoon.
> Do you think it could solve this issue, we could give it a try!?
My big fat delay does seem to solve the issue, but it has the side
effect of slowing down tuning quite a bit so I'd rather find a more
proper fix. We're talking several hundred extra milliseconds slower
per slot that is tuned...
I still don't exactly have a warm fuzzy about using the send_status()
command like this, but it seems to work (actually, I should verify
that myself--I've been taking Addy's word that his solution works). I
do wish someone could tell me "oh right, yeah, we do need a
send_status in that case". ;) Addy said that in the non-tuning case
that the core will always do a send_status so that this fix is really
only for tuning and doesn't need to be applied in general. I also
haven't validated that myself...
Overall it does sorta seem like this might just be a quirk with the
rk3288 dw_mmc. It feels like the controller is in a wonky state and
maybe this extra send_status helps it get out?
>> ...Addy instead fixed the problem using mmc_send_status() to try to
>> detect when the transfer was all done and it apparently worked, but it
>> seemed odd to me. My MMC "expertise" pretty much ends with looking
>> for simple logic errors in the MMC driver, so my hope was that one of
>> you guys would know this better...
>>>> That would mean that against upstream you'd need to change
>>>> mmc_send_tuning() to take in the card as well (or move the "host->card
>>>> = card" assignment to before UHS init, which seems less desirable?)
> I get your point now.
> Changing mmc_send_tuning() to take "card" will work due to $subject
> patch changed the ->execute_tuning() callbacks to take "card" as well.
>>>> What do you think about that? Is there a better solution?
>>> Why do we need to change mmc_send_tuning()? I thought the issue was
>>> that mmc_send_status(), which currently takes "card" as a parameter.
>> Well, if mmc_send_tuning() needed to call mmc_send_status() then
>> mmc_send_tuning() would need the card parameter, right?
> Correct, got it now. :-)
> I didn't understand that you wanted mmc_send_tuning() to invoke
> mmc_send_status() while it got some errors. From Addy's patch2,
> mmc_send_status() is invoked from the host driver.
> Anyway, I think we should follow your suggestion to change the
> behaviour of mmc_send_tuning(). Though, I think it should use
> bus_ops->alive() callback instead (and that callback then also need to
> change to take "card" as a parameter), since that would be generic and
> the cover the SDIO case as well.
That sounds reasonable to me.
Addy: you've been very quiet. What do you think?
More information about the Linux-rockchip