From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ulf Hansson Subject: Re: [PATCH V6 08/15] mmc: mmc: Hold re-tuning if the card is put to sleep Date: Wed, 6 May 2015 15:21:53 +0200 Message-ID: References: <1429531796-21987-1-git-send-email-adrian.hunter@intel.com> <1429531796-21987-9-git-send-email-adrian.hunter@intel.com> <5549D336.4040206@intel.com> <5549ECBD.3010608@intel.com> <554A0C1A.60000@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Received: from mail-qc0-f181.google.com ([209.85.216.181]:34021 "EHLO mail-qc0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751174AbbEFNVx (ORCPT ); Wed, 6 May 2015 09:21:53 -0400 Received: by qcyk17 with SMTP id k17so4253948qcy.1 for ; Wed, 06 May 2015 06:21:53 -0700 (PDT) In-Reply-To: <554A0C1A.60000@intel.com> Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Adrian Hunter Cc: linux-mmc , Aaron Lu , Philip Rakity , Al Cooper , Arend van Spriel On 6 May 2015 at 14:42, Adrian Hunter wrote: > On 06/05/15 14:36, Ulf Hansson wrote: >> [...] >> >>>>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would >>>>>> like you to move that handling into mmc_sleep(). The code should be >>>>>> easier and it becomes more clear that it's because of a command >>>>>> sequence. >>>>>> >>>>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd() >>>>>> and then mmc_retune_release() just after, in mmc_sleep(). That should >>>>>> work, right!? >>>>> >>>>> That would be the same as holding re-tuning for that request, which is >>>>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd() >>>>> is redundant. >>>> >>>> I don't understand your point, sorry. >>> >>> mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req() >>> which calls mmc_start_request() which calls mmc_retune_hold() >>> >>> Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls >>> mmc_retune_release(). >>> >>> So >>> mmc_wait_for_cmd() (with no retries) >>> has the same effect as >>> mmc_retune_hold() >>> mmc_wait_for_cmd() >>> mmc_retune_release() >>> >> >> Huh, you are right - again. >> >> There have been a couple of iterations of this patchset, I don't >> recall why we need to hold retune for all requests? It seems awkward. >> Shouldn't we just hold retune for those requests that needs it? > > For data requests (which also call __mmc_start_req()) there is the > possibility that a 'write' is not finished and is polled with CMD13. > So re-tuning is held to avoid conflicting with the busy state. > It also aids controlling when re-tuning happens in the recovery path > i.e. we have a go at getting the status first and if that doesn't > work first time, then re-tune if needed. > > Also mmc_retune_hold() does not only hold retuning, it also causes > re-tuning to happen if the hold_count was zero, so it does > "make-retuning-happen-if-needed-and-not-already-held-and-then-hold-retuning" Hmm, is there anyway we can make this easier to understand in the code path and maybe clarify via the name of functions/APIs you add? Could we have a state variable instead of bunch of int variables? Since I apparently have a bit hard time to understand how this actually works, I am a bit concerned about the maintenance of it. :-) Anyway, if you can't find any better option - I will accept it as is. > >> >>>> >>>> Anyway, my proposal didn't quite work, which is due to that >>>> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If >>>> there had been only one try, I thought it could be okay to have that >>>> command to be preceded by a re-tune. >>>> >>>> Anyway, I would like you to move the mmc_retune_hold|release() calls >>>> into the mmc_sleep() function. >>> >>> That would have no effect as explained above. >> >> Then why did you add it to the _mmc_suspend() function? What am I missing here? > > It was added in response to our discussions. It was not in my original > patches. I can take it out. > >> >>> >>>> >>>>> >>>>> The options for the caller are: >>>>> >>>>> 1) >>>>> hold re-tuning >>>>> put emmc to sleep >>>>> later wake up emmc >>>>> release re-tuning >>>>> >>>>> 2) >>>>> put emmc to sleep >>>>> later increment hold_count >>>>> wake up emmc ignoring CRC errors >>>>> release re-tuning >>>>> >>>>> But there is no wake-up function and the suspend path is using an unbalanced >>>>> mmc_sleep i.e. no corresponding wake up. >>>>> >>>>> So that leaves what is happening now i.e. a comment plus explicit >>>>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend() >>>>> know to take mmc_sleep re-tuning requirements into account. >>>> >>>> Why all this complexity? >>>> >>>> mmc_power_off() is called in _mmc_suspend(), that will eventually >>>> disable re-tune. Thus re-tuning will be prevented for >>>> commands/requests during the system PM resume sequence, until the card >>>> has been fully re-initialized (and a tuning sequence done). Isn't that >>>> sufficient? >>> >>> Yes my original patch did not have any of that complexity. I added it in >>> response to our discussions. >>> >>> As you wrote, _mmc_suspend() does not need to do anything with retuning >>> because mmc_sleep() is followed by mmc_power_off(). >>> >>> The original patch added a comment to mmc_sleep() and that was all. That >>> would still be the best approach. >>> >> >> What I had in mind was that the re-tune timer could time out in the >> middle of the _mmc_suspend() sequence. >> >> If that happens in-between mmc_deselect_cards() and when the CMD5 is >> to be sent, in mmc_sleep() - we must not allow a re-tune sequence. >> Unless holding re-tune here, how is that prevented? > > Oh yes, I have overlooked that re-tuning can't be done on a de-selected > card. So I will add mmc_retune_hold()/mmc_retune_release(). I will have to > think about the error handling. It looks broken now anyway since it doesn't > reselect the card in the error path. > I suggest you don't bother about the error handling for now, we can take that separately. Kind regards Uffe