[...] >>>> 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? >> >> 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? > >> >>> >>> 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? Kind regards Uffe -- 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
