Re: [PATCH] mmc: sdhci: don't limit discard timeout by data line timeout

[Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>]

On 22.11.2013 14:04, Adrian Hunter wrote:
> On 22/11/13 14:24, Vladimir Zapolskiy wrote:
>> On 22.11.2013 12:38, Adrian Hunter wrote:
>>> On 21/11/13 17:07, Vladimir Zapolskiy wrote:
>>>> JEDEC specification defines quite high erase timeout value for 300ms
>>>> multiplied by erase group number, and SD Host Controller specification
>>>> data line timeout may be much less, e.g. 2^13 / 52MHz ~ 160us.
>>>>
>>>>>  From block layer and MMC perfromance perspective it is desirable that
>>>> millions of erase groups are discarded at once, so there is no much
>>>> sense to limit maximum erase timeout by data line timeout, if a
>>>> controller handles correctly erase operation without indication of
>>>> data line timeout.
>>>
>>> Would you explain that some more.  Do you mean that:
>>>      a) it does not have a timeout
>>
>> JEDEC defines a timeout on erase/trim operations, also in
>> drivers/mmc/core/core.c
>> there is a reasonable enough 10 minutes limitation for discard operations.
>>
>>>      b) it has a timeout which is less than the timeout specified by the
>>> standard but the operation nevertheless completes
>>
>> SDHC data line timeout is enormously less than erase group timeout, and
>> trivial testing shows that those two timeouts are independent, probably
>> except some particular cases of controllers not known before commits
>> 58d1246db3 and e056a1b5b.
>>
>> According to the currently implemented logic, mmc_do_erase() commonly is
>> instructed to discard 1-2 erase groups at maximum, however it tends to be
>> capable to successfully discard millions of erase groups at once ignoring
>> that SDHC data line timeout limitation.
>>
>
> You seem to be trying to say that the SDHCI spec. says that the host
> controller does not timeout erase operations or uses a different timeout
> than the one programmed in the "Timeout Control Register".  Where is that is
> the SDHCI spec?

According to the spec a host controller timeouts erase operations like any
other R1B command.

So in your opinion, should there be SDHCI_QUIRK_BROKEN_TIMEOUT_VAL instead
of the new quirk?

>>>>
>>>> Potentially the change may break some of the SDHCs on discard of mmc,
>>>> and for backward compatibility a new quirk is introduced, which is NOT
>>>> set by default.
>>>
>>> It sounds to me that what you want to do is not standard so the quirk should
>>> be the other way around.
>>
>> Please take a look at commits 58d1246db3 and e056a1b5b, I'd be glad, if you
>> could elaborate to which "some host controllers" the quirk in my definition
>> applies, I believe all other host controllers present at that time in
>> drivers/mmc/host/* are capable to discard without introduced limitation.
>
> "some host controllers" == SDHCI i.e. to all of the ones you are applying
> the change.
>
>>
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy<vladimir_zapolskiy@mentor.com>
>>>> Reported-by: Ed Sutter<ed.sutter@alcatel-lucent.com>
>>>> Cc: Chris Ball<cjb@laptop.org>
>>>> Cc: Adrian Hunter<adrian.hunter@intel.com>
>>>> ---
>>>>    drivers/mmc/host/sdhci.c  |    5 ++++-
>>>>    include/linux/mmc/sdhci.h |    1 +
>>>>    2 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index bd8a098..b1fdddb 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -2930,7 +2930,10 @@ int sdhci_add_host(struct sdhci_host *host)
>>>>        if (host->quirks&   SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
>>>>            host->timeout_clk = mmc->f_max / 1000;
>>>>
>>>> -    mmc->max_discard_to = (1<<   27) / host->timeout_clk;
>>>> +    if (host->quirks2&   SDHCI_QUIRK2_DATA_TIMEOUT_ON_DISCARD)
>>>> +        mmc->max_discard_to = (1<<   27) / host->timeout_clk;
>>>> +    else
>>>> +        mmc->max_discard_to = 0;
>>>>
>>>>        mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23;
>>>>
>>>> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
>>>> index 3e781b8..e7f6bd2 100644
>>>> --- a/include/linux/mmc/sdhci.h
>>>> +++ b/include/linux/mmc/sdhci.h
>>>> @@ -98,6 +98,7 @@ struct sdhci_host {
>>>>    #define SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON        (1<<4)
>>>>    /* Controller has a non-standard host control register */
>>>>    #define SDHCI_QUIRK2_BROKEN_HOST_CONTROL        (1<<5)
>>>> +#define SDHCI_QUIRK2_DATA_TIMEOUT_ON_DISCARD        (1<<6)
>>>>
>>>>        int irq;        /* Device IRQ */
>>>>        void __iomem *ioaddr;    /* Mapped address */
>>>>
>>>
>>
>>
>
> --
> 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