On 27 January 2014 11:40, Adrian Hunter <adrian.hun...@intel.com> wrote:
> On 23/01/14 16:11, Ulf Hansson wrote:
>> On 23 January 2014 11:10, Adrian Hunter <adrian.hun...@intel.com> wrote:
>>> On 22/01/14 17:00, Ulf Hansson wrote:
>>>> If the host controller supports busy detection in HW, we expect the
>>>> MMC_CAP_WAIT_WHILE_BUSY to be set. Likewise the corresponding
>>>> host->max_busy_timeout shall reflect the maximum busy detection timeout
>>>> supported by the host. A timeout set to zero, is interpreted as the
>>>> host supports whatever timeout the mmc core provides it with.
>>>>
>>>> Previously we expected a host that supported MMC_CAP_WAIT_WHILE_BUSY to
>>>> cope with any timeout, which just isn't feasible due to HW limitations.
>>>>
>>>> For most switch operations, R1B responses are expected and thus we need
>>>> to check for busy detection completion. To cope with cases where the
>>>> requested busy detection timeout is greater than what the host are able
>>>> to support, we fallback to use a R1 response instead. This will prevent
>>>> the host from doing HW busy detection.
>>>>
>>>> In those cases busy detection completion is handled by polling the for
>>>> the card's status using CMD13, which is the same mechanism used when
>>>> the host doesn't support MMC_CAP_WAIT_WHILE_BUSY.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hans...@linaro.org>
>>>> ---
>>>> drivers/mmc/core/mmc_ops.c | 53
>>>> ++++++++++++++++++++++++++++++--------------
>>>> 1 file changed, 36 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>>> index 5e1a2cb..2e0cccb 100644
>>>> --- a/drivers/mmc/core/mmc_ops.c
>>>> +++ b/drivers/mmc/core/mmc_ops.c
>>>> @@ -413,13 +413,31 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8
>>>> index, u8 value,
>>>> unsigned int timeout_ms, bool use_busy_signal, bool
>>>> send_status,
>>>> bool ignore_crc)
>>>> {
>>>> + struct mmc_host *host;
>>>
>>> It would be nicer if the addition of 'host' was a separate patch. You
>>> should remove the unnecessary BUG_ONs (it will oops anyway) at the same
>>> time and then just do:
>>>
>>> struct mmc_host *host = card->host;
>>
>> Sure, make sense!
>>
>>>
>>>> int err;
>>>> struct mmc_command cmd = {0};
>>>> unsigned long timeout;
>>>> + unsigned int max_busy_timeout;
>>>> u32 status = 0;
>>>> + bool use_r1b_resp = true;
>>>
>>> This is a little confusing. Why not:
>>>
>>> bool use_r1b_resp = use_busy_signal;
>>>
>>> Although 'use_busy_signal' actually means 'wait_while_busy'.
>>
>> Right, that should simplify code a bit. I will update in a v2.
>>
>>>
>>>>
>>>> BUG_ON(!card);
>>>> BUG_ON(!card->host);
>>>> + host = card->host;
>>>> +
>>>> + /* Once all callers provides a timeout, remove this fallback. */
>>>> + if (!timeout_ms)
>>>> + timeout_ms = MMC_OPS_TIMEOUT_MS;
>>>
>>> A timeout of zero does not mean a very long timeout. It means an unknown
>>> timeout.
>>
>> I guess this is a matter of definition.
>
> JEDEC did not define GENERIC_CMD6_TIME until v4.5 so before that the timeout
> is unknown. It is reasonable for the host controller drivers to select a
> value that suits them rather than constrain them to some arbitrarily large
> timeout.
You are right, did not think of this!
I suppose the MMC_OPS_TIMEOUT_MS, also should be decreased, it just
seems silly waiting for 10 minutes. :-) I guess somewhere around 10 -
20 s should be enough for those cases were we need to guess.
>
>>
>> For those hosts that don't have a hw timeout, but maybe implements a
>> software timeout, I thought this was more convenient. We likely then
>> also need to define a "MAX_BUSY_TIMEOUT", which host drivers could
>> use.
>>
>> Additionally, since as of today only sdhci specifies the
>> max_discard_to (renamed to max_busy_timeout), I thought it make sense
>> to not force other hosts to specify the timeout to keep the existing
>> behaviour.
>
> Yes max_busy_timeout of zero again means unknown.
Got, it. Thanks for your input!
>
>>
>>>
>>>> +
>>>> + /* 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 (use_busy_signal && (host->caps & MMC_CAP_WAIT_WHILE_BUSY) &&
>>>> + (timeout_ms > max_busy_timeout))
>>>> + use_r1b_resp = false;
>>>> + else if (!use_busy_signal)
>>>> + use_r1b_resp = false;
>>>
>>> Why not just check what you know:
>>>
>>> if (timeout_ms && host->max_busy_timeout && timeout_ms >
>>> host->max_busy_timeout)
>>> use_r1b_resp = false;
>>>
>>
>> I wanted to maintain the R1B response for hosts that don't support
>> MMC_CAP_WAIT_WHILE_BUSY. With your proposal this will not be done.
>>
>> Given this a second thought. I think it would make sense to adapt to
>> your proposal. I will update in v2.
>>
>>>>
>>>> cmd.opcode = MMC_SWITCH;
>>>> cmd.arg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
>>>> @@ -427,17 +445,25 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8
>>>> index, u8 value,
>>>> (value << 8) |
>>>> set;
>>>> cmd.flags = MMC_CMD_AC;
>>>> - if (use_busy_signal)
>>>> + if (use_r1b_resp)
>>>> cmd.flags |= MMC_RSP_SPI_R1B | MMC_RSP_R1B;
>>>> else
>>>> cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
>>>>
>>>> + if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) {
>>>> + /* Tell the host what busy detection timeout to use. */
>>>> + cmd.busy_timeout = timeout_ms;
>>>> + /*
>>>> + * CRC errors shall only be ignored in cases were CMD13 is
>>>> used
>>>> + * to poll to detect busy completion.
>>>> + */
>>>> + ignore_crc = false;
>>>> + }
>>>>
>>>> - cmd.busy_timeout = timeout_ms;
>>>
>>> The busy_timeout should be provided for R1B i.e. this should be:
>>>
>>> if (use_r1b_resp)
>>> cmd.busy_timeout = timeout_ms;
>>>
>>
>> Will fix in v2, given you still think this is good approach according
>> to my comment just above.
>>
>>>> if (index == EXT_CSD_SANITIZE_START)
>>>> cmd.sanitize_busy = true;
>>>>
>>>> - err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
>>>> + err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
>>>> if (err)
>>>> return err;
>>>>
>>>> @@ -445,24 +471,17 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8
>>>> index, u8 value,
>>>> if (!use_busy_signal)
>>>> return 0;
>>>>
>>>> - /*
>>>> - * CRC errors shall only be ignored in cases were CMD13 is used to
>>>> poll
>>>> - * to detect busy completion.
>>>> - */
>>>> - if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
>>>> - ignore_crc = false;
>>>> -
>>>> /* Must check status to be sure of no errors. */
>>>> - timeout = jiffies + msecs_to_jiffies(MMC_OPS_TIMEOUT_MS);
>>>
>>> This is the place to set the default timeout for the loop.
>>>
>>> if (!timeout_ms)
>>> timeout_ms = MMC_OPS_TIMEOUT_MS
>>>
>>>> + timeout = jiffies + msecs_to_jiffies(timeout_ms);
>>>> do {
>>>> if (send_status) {
>>>> err = __mmc_send_status(card, &status, ignore_crc);
>>>> if (err)
>>>> return err;
>>>> }
>>>> - if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
>>>> + if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
>>>> break;
>>>> - if (mmc_host_is_spi(card->host))
>>>> + if (mmc_host_is_spi(host))
>>>> break;
>>>>
>>>> /*
>>>> @@ -478,18 +497,18 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8
>>>> index, u8 value,
>>>> /* Timeout if the device never leaves the program state. */
>>>> if (time_after(jiffies, timeout)) {
>>>> pr_err("%s: Card stuck in programming state! %s\n",
>>>> - mmc_hostname(card->host), __func__);
>>>> + mmc_hostname(host), __func__);
>>>> return -ETIMEDOUT;
>>>> }
>>>> } while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
>>>>
>>>> - if (mmc_host_is_spi(card->host)) {
>>>> + if (mmc_host_is_spi(host)) {
>>>> if (status & R1_SPI_ILLEGAL_COMMAND)
>>>> return -EBADMSG;
>>>> } else {
>>>> if (status & 0xFDFFA000)
>>>> - pr_warning("%s: unexpected status %#x after "
>>>> - "switch", mmc_hostname(card->host), status);
>>>> + pr_warn("%s: unexpected status %#x after switch\n",
>>>> + mmc_hostname(host), status);
>>>> if (status & R1_SWITCH_ERROR)
>>>> return -EBADMSG;
>>>> }
>>>>
>>>
>>
>> Adrian, thanks for reviewing!
>>
>> Kind regards
>> Uffe
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
