On 2015/1/30 09:08, addy ke wrote: > hi, Doug > > On 2015/1/30 08:13, Doug Anderson wrote: >> Ulf, >> >> On Thu, Jan 29, 2015 at 1:16 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >>> - Drastically decreased cc-list. >>> >>> On 29 January 2015 at 01:55, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: >>>> Ulf, >>>> >>>> On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> 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!? >> >> 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? > Sorry for reply late. > I am busy with some other important things, and can't confirm it by pink2 board. > > about bus_ops->alive, I think it can't use in tuning state. > Because: > bus_ops->alive() --> mmc_sd_alive(host) /* sd card */ -->mmc_send_status(host->card, NULL); > But host->card == NULL in tuning state(mmc_sd_init_card->mmc_sd_init_uhs_card). > > Only if sd is initialized successfully, we can get card pointer by host->card. > see: mmc_sd_init_card(drivers/mmc/core/sd.c), the end of this function: host->card = card And bus_ops->alive only check whether mmc is alive or not, the second parameter(*status) is NULL, We can not get the card status. But in tuning state, we need wait until card is idle, if the previous tuning is failed. >> >> -Doug >> >> >> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
