Re: [PATCH 06/10] mmc: core: Respect host's max_busy_timeout when sending sleep cmd

[Adrian Hunter <adrian.hunter@intel.com>]

On 23/01/14 16:26, Ulf Hansson wrote:
> On 23 January 2014 11:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 22/01/14 17:00, Ulf Hansson wrote:
>>> When sending the sleep command for host drivers supporting
>>> MMC_CAP_WAIT_WHILE_BUSY, we need to confirm that max_busy_timeout is
>>> big enough comparing to the sleep timeout specified from card's
>>> EXT_CSD. If this isn't case, we use a R1 response instead of R1B and
>>> fallback to use a delay instead.
>>>
>>> Do note that a max_busy_timeout set to zero by the host, is interpreted
>>> as it can cope with whatever timeout the mmc core provides it with.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/mmc/core/mmc.c |   19 ++++++++++++++++---
>>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 897fdd1..32e1546 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1359,6 +1359,8 @@ static int mmc_sleep(struct mmc_host *host)
>>>  {
>>>       struct mmc_command cmd = {0};
>>>       struct mmc_card *card = host->card;
>>> +     unsigned int timeout_ms = DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000);
>>> +     unsigned int max_busy_timeout;
>>>       int err;
>>>
>>>       if (host->caps2 & MMC_CAP2_NO_SLEEP_CMD)
>>> @@ -1372,7 +1374,18 @@ static int mmc_sleep(struct mmc_host *host)
>>>       cmd.arg = card->rca << 16;
>>>       cmd.arg |= 1 << 15;
>>>
>>> -     cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>>> +     /* We interpret unspecified timeouts as the host can cope with all. */
>>> +     max_busy_timeout = host->max_busy_timeout ?
>>> +                     host->max_busy_timeout : timeout_ms;
>>> +
>>> +     if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>>> +             (timeout_ms <= max_busy_timeout)) {
>>> +             cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>>> +             cmd.busy_timeout = timeout_ms;
>>> +     } else {
>>> +             cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>>> +     }
>>
>> I do not see why this is related to MMC_CAP_WAIT_WHILE_BUSY.
>> Why not just:
> 
> Before I do any update, we need to decide what host->max_busy_timeout
> of zero means. Please see the response in the other patch in this
> patchset.

Unless you want to change all the host controller drivers, zero means
don't know.

> 
> I see that my patch for the mmc_switch function, maintain the R1B for
> host not supporting MMC_CAP_WAIT_WHILE_BUSY, but this one for sleep
> doesn't. :-) We should align the behaviour.
> 
> 
>>
>>         if (host->max_busy_timeout && timeout_ms > host->max_busy_timeout) {
>>                 cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>>         } else {
>>                 cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
>>                 cmd.busy_timeout = timeout_ms;
>>         }
> 
> So here your suggestion will mean you would like to keep R1B for hosts
> not supporting MMC_CAP_WAIT_WHILE_BUSY. This opposite of what you
> proposed for the mmc_switch. :-)

I suggested:

	if (timeout_ms && host->max_busy_timeout && timeout_ms > host->max_busy_timeout)
                use_r1b_resp = false;

(without modifying timeout_ms) which wasn't related to MMC_CAP_WAIT_WHILE_BUSY
i.e. keeps R1B for hosts not supporting MMC_CAP_WAIT_WHILE_BUSY

> 
> I suggest that we only use R1B when the host are able to handle busy
> detection in hw. If you think that is bad idea, please let me know.
> 
>>
>>> +
>>>       err = mmc_wait_for_cmd(host, &cmd, 0);
>>>       if (err)
>>>               return err;
>>> @@ -1383,8 +1396,8 @@ static int mmc_sleep(struct mmc_host *host)
>>>        * SEND_STATUS command to poll the status because that command (and most
>>>        * others) is invalid while the card sleeps.
>>>        */
>>> -     if (!(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
>>> -             mmc_delay(DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000));
>>> +     if (!cmd.busy_timeout)
>>> +             mmc_delay(timeout_ms);
>>
>> And this becomes:
>>
>>         if (!cmd.busy_timeout || !(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
>>                 mmc_delay(timeout_ms);
>>
>>>
>>>       return err;
>>>  }
>>>
>>
> 
> Kind regards
> Uffe
> 
>