[PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning()

[Ulf Hansson]

- Drastically decreased cc-list.

On 29 January 2015 at 01:55, Doug Anderson <dianders at chromium.org> wrote:
> Ulf,
>
> 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
>>> mmc_send_status().
>>
>> 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!?

>
> ...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.

Kind regards
Uffe