From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jaehoon Chung Subject: Re: [PATCH 1/2] mmc: core: use card pointer as the first parameter of execute_tuning() Date: Fri, 30 Jan 2015 11:15:04 +0900 Message-ID: <54CAE928.2000909@samsung.com> References: <1422271175-19445-1-git-send-email-addy.ke@rock-chips.com> <1422271175-19445-2-git-send-email-addy.ke@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: Received: from mailout2.samsung.com ([203.254.224.25]:11710 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756696AbbA3CPG (ORCPT ); Thu, 29 Jan 2015 21:15:06 -0500 Received: from epcpsbgr2.samsung.com (u142.gpu120.samsung.co.kr [203.254.230.142]) by mailout2.samsung.com (Oracle Communications Messaging Server 7u4-24.01 (7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0NIY003KCWX43HD0@mailout2.samsung.com> for linux-mmc@vger.kernel.org; Fri, 30 Jan 2015 11:15:04 +0900 (KST) In-reply-to: Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Doug Anderson , Ulf Hansson Cc: Addy Ke , "tgih.jun@samsung.com" , Chris Ball , linux-mmc , "open list:ARM/Rockchip SoC..." On 01/30/2015 09:13 AM, Doug Anderson wrote: > Ulf, > > On Thu, Jan 29, 2015 at 1:16 AM, Ulf Hansson wrote: >> - Drastically decreased cc-list. >> >> On 29 January 2015 at 01:55, Doug Anderson wrote: >>> Ulf, >>> >>> On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson 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 >>> . 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. Sorry, which patch? i missed it. >> >> 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? I think that dw-mmc controller seems to have the main problem for tuning sequence. In my knowledge, tuning sequence doesn't need to send the stop command. but the code of dw-mmc need to handle the stop command and it has implemented. Using Send_status() is good solution, but i think it's not main solution. I will work to clean up the code for dwmmc controller. And consider to handle the stop command.. Best Regards, Jaehoon Chung > > >>> ...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? > > -Doug >