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

Message ID 1390402824-9850-7-git-send-email-ulf.hansson@linaro.org
State New
Headers show

Commit Message

Ulf Hansson Jan. 22, 2014, 3 p.m.
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(-)

Comments

Ulf Hansson Jan. 23, 2014, 2:26 p.m. | #1
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.

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 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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Jan. 28, 2014, 12:43 p.m. | #2
On 27 January 2014 11:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 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.
>

Agree!

>>
>> 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

Will fix in v2, thanks!


>
>>
>> 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
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;
+	}
+
 	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);
 
 	return err;
 }