mmc: sdhci: don't limit discard timeout by data line timeout

Commit Message

Vladimir Zapolskiy Nov. 21, 2013, 3:07 p.m. UTC

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.

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.

Signed-off-by: Vladimir Zapolskiy <[e-mail]>
Reported-by: Ed Sutter <[e-mail]>
Cc: Chris Ball <[e-mail]>
Cc: Adrian Hunter <[e-mail]>
---
 drivers/mmc/host/sdhci.c  |    5 ++++-
 include/linux/mmc/sdhci.h |    1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Adrian Hunter Nov. 22, 2013, 11:38 a.m. UTC | #1

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
	b) it has a timeout which is less than the timeout specified by the
standard but the operation nevertheless completes

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

> 
> Signed-off-by: Vladimir Zapolskiy <[e-mail]>
> Reported-by: Ed Sutter <[e-mail]>
> Cc: Chris Ball <[e-mail]>
> Cc: Adrian Hunter <[e-mail]>
> ---
>  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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vladimir Zapolskiy Nov. 22, 2013, 12:24 p.m. UTC | #2

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.

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

>>
>> Signed-off-by: Vladimir Zapolskiy<[e-mail]>
>> Reported-by: Ed Sutter<[e-mail]>
>> Cc: Chris Ball<[e-mail]>
>> Cc: Adrian Hunter<[e-mail]>
>> ---
>>   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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Adrian Hunter Nov. 22, 2013, 1:04 p.m. UTC | #3

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?

>>>
>>> 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<[e-mail]>
>>> Reported-by: Ed Sutter<[e-mail]>
>>> Cc: Chris Ball<[e-mail]>
>>> Cc: Adrian Hunter<[e-mail]>
>>> ---
>>>   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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vladimir Zapolskiy Nov. 22, 2013, 1:50 p.m. UTC | #4

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<[e-mail]>
>>>> Reported-by: Ed Sutter<[e-mail]>
>>>> Cc: Chris Ball<[e-mail]>
>>>> Cc: Adrian Hunter<[e-mail]>
>>>> ---
>>>>    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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Adrian Hunter Nov. 22, 2013, 3:04 p.m. UTC | #5

On 22/11/13 15:50, Vladimir Zapolskiy wrote:
> 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?

I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It just
sets the timeout to maximum but max_discard_to is the maximum timeout.

As I understand it you don't want to limit the discard size, either because
your controller does not timeout, or because you are happy that the maximum
timeout is enough for your users and their use-cases.

If that is the case then the original patch just needs the quirk the other
way around. i.e.

    if (host->quirks2 & SDHCI_QUIRK2_NO_DISCARD_LIMIT)
        mmc->max_discard_to = 0;
    else
        mmc->max_discard_to = (1 << 27) / host->timeout_clk;

> 
>>>>>
>>>>> 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<[e-mail]>
>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>> Cc: Chris Ball<[e-mail]>
>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>> ---
>>>>>    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 [e-mail]
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vladimir Zapolskiy Nov. 22, 2013, 3:21 p.m. UTC | #6

On 22.11.2013 16:04, Adrian Hunter wrote:
> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>> 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?
>
> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It just
> sets the timeout to maximum but max_discard_to is the maximum timeout.

Here I meant to do something like:

	if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
		mmc->max_discard_to = 0;

Again I'm not sure that this applies well to all 
SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
controllers, therefore a new quirk might be better.

> As I understand it you don't want to limit the discard size, either because
> your controller does not timeout, or because you are happy that the maximum
> timeout is enough for your users and their use-cases.
>
> If that is the case then the original patch just needs the quirk the other
> way around. i.e.
>
>      if (host->quirks2&  SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>          mmc->max_discard_to = 0;
>      else
>          mmc->max_discard_to = (1<<  27) / host->timeout_clk;

This suits me fine, thanks for review, and I'll resend a change based on 
this.

Also I'd like to pay your attention to (1 << 27) / host->timeout_clk part of
calculation, following the spec it might be better to account the actual
value of Data Timeout Counter, otherwise a controller may get unintentional
Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.

>>
>>>>>>
>>>>>> 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<[e-mail]>
>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>> ---
>>>>>>     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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ed Sutter Nov. 25, 2013, 6:20 p.m. UTC | #7

For what its worth, I applied these two patches and the huge delay in 
mkfs.ext3
(when run on SABRESD-resident eMMC) is gone; however, based on the 
discussion,
it appears this isn't the *final* change, is that true?
Thanks,
Ed
> 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?
>
>>>> 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<[e-mail]>
>>>> Reported-by: Ed Sutter<[e-mail]>
>>>> Cc: Chris Ball<[e-mail]>
>>>> Cc: Adrian Hunter<[e-mail]>
>>>> ---
>>>>    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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vladimir Zapolskiy Nov. 25, 2013, 10:06 p.m. UTC | #8

Ed,

On 11/25/13 20:20, Ed Sutter wrote:
> For what its worth, I applied these two patches and the huge delay in
> mkfs.ext3
> (when run on SABRESD-resident eMMC) is gone; however, based on the
> discussion,
> it appears this isn't the *final* change, is that true?

that's correct. In spite of the change works well in your and my cases,
I don't like it much, even reversing a quirk probably won't be good
enough without performed deliberate testing with various eMMC/MMC.

I think more work is required to support proper timeout delay calculation
accounting changes in SDHC clock frequency for
SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK controllers and DAT counter value.

I plan to do a reimplementation of DAT delay calculation, and then
I'll publish an alternative change. However please feel free to use
this quick hackish change, if it serves your purpose.

With best wishes,
Vladimir

> Thanks,
> Ed
>> 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?
>>
>>>>> 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<[e-mail]>
>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>> Cc: Chris Ball<[e-mail]>
>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>> ---
>>>>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Adrian Hunter Nov. 26, 2013, 9:04 a.m. UTC | #9

On 22/11/13 17:21, Vladimir Zapolskiy wrote:
> On 22.11.2013 16:04, Adrian Hunter wrote:
>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>> 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?
>>
>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It just
>> sets the timeout to maximum but max_discard_to is the maximum timeout.
> 
> Here I meant to do something like:
> 
>     if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>         mmc->max_discard_to = 0;
> 
> Again I'm not sure that this applies well to all SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
> controllers, therefore a new quirk might be better.
> 
>> As I understand it you don't want to limit the discard size, either because
>> your controller does not timeout, or because you are happy that the maximum
>> timeout is enough for your users and their use-cases.
>>
>> If that is the case then the original patch just needs the quirk the other
>> way around. i.e.
>>
>>      if (host->quirks2&  SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>          mmc->max_discard_to = 0;
>>      else
>>          mmc->max_discard_to = (1<<  27) / host->timeout_clk;
> 
> This suits me fine, thanks for review, and I'll resend a change based on this.
> 
> Also I'd like to pay your attention to (1 << 27) / host->timeout_clk part of
> calculation, following the spec it might be better to account the actual
> value of Data Timeout Counter, otherwise a controller may get unintentional
> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.

Not sure what you mean.  max_discard_to is the maximum timeout (in
milliseconds) that the host controller supports.  The intent is to limit
erase operations to ones that have a timeout that is less than or equal to that.

Currently, the limit gets applied by the block layer before the mmc layer is
involved so there is no possibility to take the actual timeout into account.
 However if you have erase_group_def set, then it won't make any difference
i.e. the limit will be the same.

> 
>>>
>>>>>>>
>>>>>>> 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<[e-mail]>
>>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>>> ---
>>>>>>>     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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vladimir Zapolskiy Nov. 26, 2013, 4:33 p.m. UTC | #10

On 11/26/13 11:04, Adrian Hunter wrote:
> On 22/11/13 17:21, Vladimir Zapolskiy wrote:
>> On 22.11.2013 16:04, Adrian Hunter wrote:
>>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>>> 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?
>>>
>>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It just
>>> sets the timeout to maximum but max_discard_to is the maximum timeout.
>>
>> Here I meant to do something like:
>>
>>      if (host->quirks&  SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>>          mmc->max_discard_to = 0;
>>
>> Again I'm not sure that this applies well to all SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>> controllers, therefore a new quirk might be better.
>>
>>> As I understand it you don't want to limit the discard size, either because
>>> your controller does not timeout, or because you are happy that the maximum
>>> timeout is enough for your users and their use-cases.
>>>
>>> If that is the case then the original patch just needs the quirk the other
>>> way around. i.e.
>>>
>>>       if (host->quirks2&   SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>>           mmc->max_discard_to = 0;
>>>       else
>>>           mmc->max_discard_to = (1<<   27) / host->timeout_clk;
>>
>> This suits me fine, thanks for review, and I'll resend a change based on this.
>>
>> Also I'd like to pay your attention to (1<<  27) / host->timeout_clk part of
>> calculation, following the spec it might be better to account the actual
>> value of Data Timeout Counter, otherwise a controller may get unintentional
>> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.
>
> Not sure what you mean.  max_discard_to is the maximum timeout (in
> milliseconds) that the host controller supports.  The intent is to limit
> erase operations to ones that have a timeout that is less than or equal to that.

That's clear. But it's not obvious why do you prefer (1 << 27) numerator 
instead
of secure (1 << 13) or (1 << (13 + sdhci_readl(host, 
SDHCI_TIMEOUT_CONTROL))).

> Currently, the limit gets applied by the block layer before the mmc layer is
> involved so there is no possibility to take the actual timeout into account.
>   However if you have erase_group_def set, then it won't make any difference
> i.e. the limit will be the same.
>
>>
>>>>
>>>>>>>>
>>>>>>>> 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<[e-mail]>
>>>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>>>> ---
>>>>>>>>      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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Adrian Hunter Nov. 27, 2013, 8:21 a.m. UTC | #11

On 26/11/13 18:33, Vladimir Zapolskiy wrote:
> On 11/26/13 11:04, Adrian Hunter wrote:
>> On 22/11/13 17:21, Vladimir Zapolskiy wrote:
>>> On 22.11.2013 16:04, Adrian Hunter wrote:
>>>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>>>> 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?
>>>>
>>>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It just
>>>> sets the timeout to maximum but max_discard_to is the maximum timeout.
>>>
>>> Here I meant to do something like:
>>>
>>>      if (host->quirks&  SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>>>          mmc->max_discard_to = 0;
>>>
>>> Again I'm not sure that this applies well to all
>>> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>>> controllers, therefore a new quirk might be better.
>>>
>>>> As I understand it you don't want to limit the discard size, either because
>>>> your controller does not timeout, or because you are happy that the maximum
>>>> timeout is enough for your users and their use-cases.
>>>>
>>>> If that is the case then the original patch just needs the quirk the other
>>>> way around. i.e.
>>>>
>>>>       if (host->quirks2&   SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>>>           mmc->max_discard_to = 0;
>>>>       else
>>>>           mmc->max_discard_to = (1<<   27) / host->timeout_clk;
>>>
>>> This suits me fine, thanks for review, and I'll resend a change based on
>>> this.
>>>
>>> Also I'd like to pay your attention to (1<<  27) / host->timeout_clk part of
>>> calculation, following the spec it might be better to account the actual
>>> value of Data Timeout Counter, otherwise a controller may get unintentional
>>> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.
>>
>> Not sure what you mean.  max_discard_to is the maximum timeout (in
>> milliseconds) that the host controller supports.  The intent is to limit
>> erase operations to ones that have a timeout that is less than or equal to
>> that.
> 
> That's clear. But it's not obvious why do you prefer (1 << 27) numerator
> instead
> of secure (1 << 13) or (1 << (13 + sdhci_readl(host, SDHCI_TIMEOUT_CONTROL))).

The maximum value of "Data Timeout Counter Value" in "Timeout Control
Register" is 14 and the maximum timeout is therefore (1 << 27).

> 
>> Currently, the limit gets applied by the block layer before the mmc layer is
>> involved so there is no possibility to take the actual timeout into account.
>>   However if you have erase_group_def set, then it won't make any difference
>> i.e. the limit will be the same.
>>
>>>
>>>>>
>>>>>>>>>
>>>>>>>>> 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<[e-mail]>
>>>>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>>>>> ---
>>>>>>>>>      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 [e-mail]
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vladimir Zapolskiy Nov. 27, 2013, 2:57 p.m. UTC | #12

On 11/27/13 10:21, Adrian Hunter wrote:
> On 26/11/13 18:33, Vladimir Zapolskiy wrote:
>> On 11/26/13 11:04, Adrian Hunter wrote:
>>> On 22/11/13 17:21, Vladimir Zapolskiy wrote:
>>>> On 22.11.2013 16:04, Adrian Hunter wrote:
>>>>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>>>>> 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?
>>>>>
>>>>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It just
>>>>> sets the timeout to maximum but max_discard_to is the maximum timeout.
>>>>
>>>> Here I meant to do something like:
>>>>
>>>>       if (host->quirks&   SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>>>>           mmc->max_discard_to = 0;
>>>>
>>>> Again I'm not sure that this applies well to all
>>>> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>>>> controllers, therefore a new quirk might be better.
>>>>
>>>>> As I understand it you don't want to limit the discard size, either because
>>>>> your controller does not timeout, or because you are happy that the maximum
>>>>> timeout is enough for your users and their use-cases.
>>>>>
>>>>> If that is the case then the original patch just needs the quirk the other
>>>>> way around. i.e.
>>>>>
>>>>>        if (host->quirks2&    SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>>>>            mmc->max_discard_to = 0;
>>>>>        else
>>>>>            mmc->max_discard_to = (1<<    27) / host->timeout_clk;
>>>>
>>>> This suits me fine, thanks for review, and I'll resend a change based on
>>>> this.
>>>>
>>>> Also I'd like to pay your attention to (1<<   27) / host->timeout_clk part of
>>>> calculation, following the spec it might be better to account the actual
>>>> value of Data Timeout Counter, otherwise a controller may get unintentional
>>>> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.
>>>
>>> Not sure what you mean.  max_discard_to is the maximum timeout (in
>>> milliseconds) that the host controller supports.  The intent is to limit
>>> erase operations to ones that have a timeout that is less than or equal to
>>> that.
>>
>> That's clear. But it's not obvious why do you prefer (1<<  27) numerator
>> instead
>> of secure (1<<  13) or (1<<  (13 + sdhci_readl(host, SDHCI_TIMEOUT_CONTROL))).
>
> The maximum value of "Data Timeout Counter Value" in "Timeout Control
> Register" is 14 and the maximum timeout is therefore (1<<  27).

So, from this perspective I assume this is a potential theoretical maximum
timeout for a controller, which may be 16384 times more than the maximum
guaranteed timeout before getting a DAT timeout. Why is the theoretical 
maximum
supposed to be used in calculations of a guaranteed discard operation 
timeout
instead of promised DAT timeout by a controller?

>>
>>> Currently, the limit gets applied by the block layer before the mmc layer is
>>> involved so there is no possibility to take the actual timeout into account.
>>>    However if you have erase_group_def set, then it won't make any difference
>>> i.e. the limit will be the same.
>>>
>>>>
>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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<[e-mail]>
>>>>>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>       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 [e-mail]
>>> 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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Philip Rakity Nov. 27, 2013, 3:48 p.m. UTC | #13

On Nov 27, 2013, at 2:57 PM, Vladimir Zapolskiy <[e-mail]> wrote:

> On 11/27/13 10:21, Adrian Hunter wrote:
>> On 26/11/13 18:33, Vladimir Zapolskiy wrote:
>>> On 11/26/13 11:04, Adrian Hunter wrote:
>>>> On 22/11/13 17:21, Vladimir Zapolskiy wrote:
>>>>> On 22.11.2013 16:04, Adrian Hunter wrote:
>>>>>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>>>>>> 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?
>>>>>> 
>>>>>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It just
>>>>>> sets the timeout to maximum but max_discard_to is the maximum timeout.
>>>>> 
>>>>> Here I meant to do something like:
>>>>> 
>>>>>      if (host->quirks&   SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>>>>>          mmc->max_discard_to = 0;
>>>>> 
>>>>> Again I'm not sure that this applies well to all
>>>>> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>>>>> controllers, therefore a new quirk might be better.
>>>>> 
>>>>>> As I understand it you don't want to limit the discard size, either because
>>>>>> your controller does not timeout, or because you are happy that the maximum
>>>>>> timeout is enough for your users and their use-cases.
>>>>>> 
>>>>>> If that is the case then the original patch just needs the quirk the other
>>>>>> way around. i.e.
>>>>>> 
>>>>>>       if (host->quirks2&    SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>>>>>           mmc->max_discard_to = 0;
>>>>>>       else
>>>>>>           mmc->max_discard_to = (1<<    27) / host->timeout_clk;
>>>>> 
>>>>> This suits me fine, thanks for review, and I'll resend a change based on
>>>>> this.
>>>>> 
>>>>> Also I'd like to pay your attention to (1<<   27) / host->timeout_clk part of
>>>>> calculation, following the spec it might be better to account the actual
>>>>> value of Data Timeout Counter, otherwise a controller may get unintentional
>>>>> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.
>>>> 
>>>> Not sure what you mean.  max_discard_to is the maximum timeout (in
>>>> milliseconds) that the host controller supports.  The intent is to limit
>>>> erase operations to ones that have a timeout that is less than or equal to
>>>> that.
>>> 
>>> That's clear. But it's not obvious why do you prefer (1<<  27) numerator
>>> instead
>>> of secure (1<<  13) or (1<<  (13 + sdhci_readl(host, SDHCI_TIMEOUT_CONTROL))).
>> 
>> The maximum value of "Data Timeout Counter Value" in "Timeout Control
>> Register" is 14 and the maximum timeout is therefore (1<<  27).
> 
> So, from this perspective I assume this is a potential theoretical maximum
> timeout for a controller, which may be 16384 times more than the maximum
> guaranteed timeout before getting a DAT timeout. Why is the theoretical
> maximum
> supposed to be used in calculations of a guaranteed discard operation
> timeout
> instead of promised DAT timeout by a controller?

cards data is not always truthful is what I have found.  The timeout is rare so setting a
timeout little longer is better then timing out a transaction because the card data is
not right.

> 
>>> 
>>>> Currently, the limit gets applied by the block layer before the mmc layer is
>>>> involved so there is no possibility to take the actual timeout into account.
>>>>   However if you have erase_group_def set, then it won't make any difference
>>>> i.e. the limit will be the same.
>>>> 
>>>>> 
>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> 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<[e-mail]>
>>>>>>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>>>>>>> ---
>>>>>>>>>>>      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 [e-mail]
>>>> 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 [e-mail]
>> 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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vladimir Zapolskiy Nov. 27, 2013, 4:11 p.m. UTC | #14

On 11/27/13 17:48, Philip Rakity wrote:
> On Nov 27, 2013, at 2:57 PM, Vladimir Zapolskiy<[e-mail]>  wrote:
>> On 11/27/13 10:21, Adrian Hunter wrote:
>>> On 26/11/13 18:33, Vladimir Zapolskiy wrote:
>>>> On 11/26/13 11:04, Adrian Hunter wrote:
>>>>> On 22/11/13 17:21, Vladimir Zapolskiy wrote:
>>>>>> On 22.11.2013 16:04, Adrian Hunter wrote:
>>>>>>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>>>>>>> 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?
>>>>>>>
>>>>>>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It just
>>>>>>> sets the timeout to maximum but max_discard_to is the maximum timeout.
>>>>>>
>>>>>> Here I meant to do something like:
>>>>>>
>>>>>>       if (host->quirks&    SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>>>>>>           mmc->max_discard_to = 0;
>>>>>>
>>>>>> Again I'm not sure that this applies well to all
>>>>>> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>>>>>> controllers, therefore a new quirk might be better.
>>>>>>
>>>>>>> As I understand it you don't want to limit the discard size, either because
>>>>>>> your controller does not timeout, or because you are happy that the maximum
>>>>>>> timeout is enough for your users and their use-cases.
>>>>>>>
>>>>>>> If that is the case then the original patch just needs the quirk the other
>>>>>>> way around. i.e.
>>>>>>>
>>>>>>>        if (host->quirks2&     SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>>>>>>            mmc->max_discard_to = 0;
>>>>>>>        else
>>>>>>>            mmc->max_discard_to = (1<<     27) / host->timeout_clk;
>>>>>>
>>>>>> This suits me fine, thanks for review, and I'll resend a change based on
>>>>>> this.
>>>>>>
>>>>>> Also I'd like to pay your attention to (1<<    27) / host->timeout_clk part of
>>>>>> calculation, following the spec it might be better to account the actual
>>>>>> value of Data Timeout Counter, otherwise a controller may get unintentional
>>>>>> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.
>>>>>
>>>>> Not sure what you mean.  max_discard_to is the maximum timeout (in
>>>>> milliseconds) that the host controller supports.  The intent is to limit
>>>>> erase operations to ones that have a timeout that is less than or equal to
>>>>> that.
>>>>
>>>> That's clear. But it's not obvious why do you prefer (1<<   27) numerator
>>>> instead
>>>> of secure (1<<   13) or (1<<   (13 + sdhci_readl(host, SDHCI_TIMEOUT_CONTROL))).
>>>
>>> The maximum value of "Data Timeout Counter Value" in "Timeout Control
>>> Register" is 14 and the maximum timeout is therefore (1<<   27).
>>
>> So, from this perspective I assume this is a potential theoretical maximum
>> timeout for a controller, which may be 16384 times more than the maximum
>> guaranteed timeout before getting a DAT timeout. Why is the theoretical
>> maximum
>> supposed to be used in calculations of a guaranteed discard operation
>> timeout
>> instead of promised DAT timeout by a controller?
>
> cards data is not always truthful is what I have found.  The timeout is rare so setting a
> timeout little longer is better then timing out a transaction because the card data is
> not right.

I don't find this explanation as acceptable one for the following reasons:
a) DAT0 busy state issued by a card, but data timeout is detected on 
controller's
    side based on controller's setting and should be independent on a card,
b) drivers/mmc/host/sdhci.c should follow the spec, for "bad" controllers
    there are quirks, also see Adrian's comment in this thread about it,
c) it makes selected discard timeout limitation awkward, if we silently 
agree
    that 16384 times longer (eh, not so little, right?) timeout value is OK,
    why MAX_UINT timeout is bad then?

>>
>>>>
>>>>> Currently, the limit gets applied by the block layer before the mmc layer is
>>>>> involved so there is no possibility to take the actual timeout into account.
>>>>>    However if you have erase_group_def set, then it won't make any difference
>>>>> i.e. the limit will be the same.
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> 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<[e-mail]>
>>>>>>>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>>>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>>>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>>>>>>>> ---
>>>>>>>>>>>>       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 [e-mail]
>>>>> 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 [e-mail]
>>> 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 [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may contain
> confidential information.  Any unauthorized review, use, disclosure or distribution
> is prohibited.  If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Adrian Hunter Nov. 28, 2013, 7:12 a.m. UTC | #15

On 27/11/13 16:57, Vladimir Zapolskiy wrote:
> On 11/27/13 10:21, Adrian Hunter wrote:
>> On 26/11/13 18:33, Vladimir Zapolskiy wrote:
>>> On 11/26/13 11:04, Adrian Hunter wrote:
>>>> On 22/11/13 17:21, Vladimir Zapolskiy wrote:
>>>>> On 22.11.2013 16:04, Adrian Hunter wrote:
>>>>>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>>>>>> 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?
>>>>>>
>>>>>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It
>>>>>> just
>>>>>> sets the timeout to maximum but max_discard_to is the maximum timeout.
>>>>>
>>>>> Here I meant to do something like:
>>>>>
>>>>>       if (host->quirks&   SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>>>>>           mmc->max_discard_to = 0;
>>>>>
>>>>> Again I'm not sure that this applies well to all
>>>>> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>>>>> controllers, therefore a new quirk might be better.
>>>>>
>>>>>> As I understand it you don't want to limit the discard size, either
>>>>>> because
>>>>>> your controller does not timeout, or because you are happy that the
>>>>>> maximum
>>>>>> timeout is enough for your users and their use-cases.
>>>>>>
>>>>>> If that is the case then the original patch just needs the quirk the
>>>>>> other
>>>>>> way around. i.e.
>>>>>>
>>>>>>        if (host->quirks2&    SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>>>>>            mmc->max_discard_to = 0;
>>>>>>        else
>>>>>>            mmc->max_discard_to = (1<<    27) / host->timeout_clk;
>>>>>
>>>>> This suits me fine, thanks for review, and I'll resend a change based on
>>>>> this.
>>>>>
>>>>> Also I'd like to pay your attention to (1<<   27) / host->timeout_clk
>>>>> part of
>>>>> calculation, following the spec it might be better to account the actual
>>>>> value of Data Timeout Counter, otherwise a controller may get
>>>>> unintentional
>>>>> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.
>>>>
>>>> Not sure what you mean.  max_discard_to is the maximum timeout (in
>>>> milliseconds) that the host controller supports.  The intent is to limit
>>>> erase operations to ones that have a timeout that is less than or equal to
>>>> that.
>>>
>>> That's clear. But it's not obvious why do you prefer (1<<  27) numerator
>>> instead
>>> of secure (1<<  13) or (1<<  (13 + sdhci_readl(host,
>>> SDHCI_TIMEOUT_CONTROL))).
>>
>> The maximum value of "Data Timeout Counter Value" in "Timeout Control
>> Register" is 14 and the maximum timeout is therefore (1<<  27).
> 
> So, from this perspective I assume this is a potential theoretical maximum
> timeout for a controller, which may be 16384 times more than the maximum
> guaranteed timeout before getting a DAT timeout. Why is the theoretical maximum

Where do you get the notion of "maximum guaranteed timeout"?  The timeout is
what is programmed in "Data Timeout Counter Value".

> supposed to be used in calculations of a guaranteed discard operation timeout
> instead of promised DAT timeout by a controller?

What is "promised DAT timeout"?

> 
>>>
>>>> Currently, the limit gets applied by the block layer before the mmc
>>>> layer is
>>>> involved so there is no possibility to take the actual timeout into
>>>> account.
>>>>    However if you have erase_group_def set, then it won't make any
>>>> difference
>>>> i.e. the limit will be the same.
>>>>
>>>>>
>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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<[e-mail]>
>>>>>>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>>>>>>> ---
>>>>>>>>>>>       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 [e-mail]
>>>> 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 [e-mail]
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vladimir Zapolskiy Nov. 28, 2013, 11:48 a.m. UTC | #16

On 11/28/13 09:12, Adrian Hunter wrote:
> On 27/11/13 16:57, Vladimir Zapolskiy wrote:
>> On 11/27/13 10:21, Adrian Hunter wrote:
>>> On 26/11/13 18:33, Vladimir Zapolskiy wrote:
>>>> On 11/26/13 11:04, Adrian Hunter wrote:
>>>>> On 22/11/13 17:21, Vladimir Zapolskiy wrote:
>>>>>> On 22.11.2013 16:04, Adrian Hunter wrote:
>>>>>>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>>>>>>> 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?
>>>>>>>
>>>>>>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It
>>>>>>> just
>>>>>>> sets the timeout to maximum but max_discard_to is the maximum timeout.
>>>>>>
>>>>>> Here I meant to do something like:
>>>>>>
>>>>>>        if (host->quirks&    SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>>>>>>            mmc->max_discard_to = 0;
>>>>>>
>>>>>> Again I'm not sure that this applies well to all
>>>>>> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>>>>>> controllers, therefore a new quirk might be better.
>>>>>>
>>>>>>> As I understand it you don't want to limit the discard size, either
>>>>>>> because
>>>>>>> your controller does not timeout, or because you are happy that the
>>>>>>> maximum
>>>>>>> timeout is enough for your users and their use-cases.
>>>>>>>
>>>>>>> If that is the case then the original patch just needs the quirk the
>>>>>>> other
>>>>>>> way around. i.e.
>>>>>>>
>>>>>>>         if (host->quirks2&     SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>>>>>>             mmc->max_discard_to = 0;
>>>>>>>         else
>>>>>>>             mmc->max_discard_to = (1<<     27) / host->timeout_clk;
>>>>>>
>>>>>> This suits me fine, thanks for review, and I'll resend a change based on
>>>>>> this.
>>>>>>
>>>>>> Also I'd like to pay your attention to (1<<    27) / host->timeout_clk
>>>>>> part of
>>>>>> calculation, following the spec it might be better to account the actual
>>>>>> value of Data Timeout Counter, otherwise a controller may get
>>>>>> unintentional
>>>>>> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.
>>>>>
>>>>> Not sure what you mean.  max_discard_to is the maximum timeout (in
>>>>> milliseconds) that the host controller supports.  The intent is to limit
>>>>> erase operations to ones that have a timeout that is less than or equal to
>>>>> that.
>>>>
>>>> That's clear. But it's not obvious why do you prefer (1<<   27) numerator
>>>> instead
>>>> of secure (1<<   13) or (1<<   (13 + sdhci_readl(host,
>>>> SDHCI_TIMEOUT_CONTROL))).
>>>
>>> The maximum value of "Data Timeout Counter Value" in "Timeout Control
>>> Register" is 14 and the maximum timeout is therefore (1<<   27).
>>
>> So, from this perspective I assume this is a potential theoretical maximum
>> timeout for a controller, which may be 16384 times more than the maximum
>> guaranteed timeout before getting a DAT timeout. Why is the theoretical maximum
>
> Where do you get the notion of "maximum guaranteed timeout"?  The timeout is
> what is programmed in "Data Timeout Counter Value".

And exactly this "Data Timeout Counter Value" is not used in your code to
predict controller's data line timeout.

>> supposed to be used in calculations of a guaranteed discard operation timeout
>> instead of promised DAT timeout by a controller?
>
> What is "promised DAT timeout"?

This is a timeout with respect to "Data Timeout Counter Value".

According to your words max_discard_to is the maximum timeout that the host
controller supports, but such a parameter is useless, because nobody sets
the host controller SDHCI_TIMEOUT_CONTROL register to maximum supported 
value,
so there is a probability that you greatly overestimate Data Timeout value,
and therefore block layer or other subsystem can't rely on it. Please 
correct
me here.

>>
>>>>
>>>>> Currently, the limit gets applied by the block layer before the mmc
>>>>> layer is
>>>>> involved so there is no possibility to take the actual timeout into
>>>>> account.
>>>>>     However if you have erase_group_def set, then it won't make any
>>>>> difference
>>>>> i.e. the limit will be the same.
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> 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<[e-mail]>
>>>>>>>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>>>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>>>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>>>>>>>> ---
>>>>>>>>>>>>        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 [e-mail]
>>>>> 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 [e-mail]
>>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Adrian Hunter Nov. 28, 2013, 1:06 p.m. UTC | #17

On 28/11/13 13:48, Vladimir Zapolskiy wrote:
> On 11/28/13 09:12, Adrian Hunter wrote:
>> On 27/11/13 16:57, Vladimir Zapolskiy wrote:
>>> On 11/27/13 10:21, Adrian Hunter wrote:
>>>> On 26/11/13 18:33, Vladimir Zapolskiy wrote:
>>>>> On 11/26/13 11:04, Adrian Hunter wrote:
>>>>>> On 22/11/13 17:21, Vladimir Zapolskiy wrote:
>>>>>>> On 22.11.2013 16:04, Adrian Hunter wrote:
>>>>>>>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>>>>>>>> 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?
>>>>>>>>
>>>>>>>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It
>>>>>>>> just
>>>>>>>> sets the timeout to maximum but max_discard_to is the maximum timeout.
>>>>>>>
>>>>>>> Here I meant to do something like:
>>>>>>>
>>>>>>>        if (host->quirks&    SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>>>>>>>            mmc->max_discard_to = 0;
>>>>>>>
>>>>>>> Again I'm not sure that this applies well to all
>>>>>>> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>>>>>>> controllers, therefore a new quirk might be better.
>>>>>>>
>>>>>>>> As I understand it you don't want to limit the discard size, either
>>>>>>>> because
>>>>>>>> your controller does not timeout, or because you are happy that the
>>>>>>>> maximum
>>>>>>>> timeout is enough for your users and their use-cases.
>>>>>>>>
>>>>>>>> If that is the case then the original patch just needs the quirk the
>>>>>>>> other
>>>>>>>> way around. i.e.
>>>>>>>>
>>>>>>>>         if (host->quirks2&     SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>>>>>>>             mmc->max_discard_to = 0;
>>>>>>>>         else
>>>>>>>>             mmc->max_discard_to = (1<<     27) / host->timeout_clk;
>>>>>>>
>>>>>>> This suits me fine, thanks for review, and I'll resend a change based on
>>>>>>> this.
>>>>>>>
>>>>>>> Also I'd like to pay your attention to (1<<    27) / host->timeout_clk
>>>>>>> part of
>>>>>>> calculation, following the spec it might be better to account the actual
>>>>>>> value of Data Timeout Counter, otherwise a controller may get
>>>>>>> unintentional
>>>>>>> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.
>>>>>>
>>>>>> Not sure what you mean.  max_discard_to is the maximum timeout (in
>>>>>> milliseconds) that the host controller supports.  The intent is to limit
>>>>>> erase operations to ones that have a timeout that is less than or
>>>>>> equal to
>>>>>> that.
>>>>>
>>>>> That's clear. But it's not obvious why do you prefer (1<<   27) numerator
>>>>> instead
>>>>> of secure (1<<   13) or (1<<   (13 + sdhci_readl(host,
>>>>> SDHCI_TIMEOUT_CONTROL))).
>>>>
>>>> The maximum value of "Data Timeout Counter Value" in "Timeout Control
>>>> Register" is 14 and the maximum timeout is therefore (1<<   27).
>>>
>>> So, from this perspective I assume this is a potential theoretical maximum
>>> timeout for a controller, which may be 16384 times more than the maximum
>>> guaranteed timeout before getting a DAT timeout. Why is the theoretical
>>> maximum
>>
>> Where do you get the notion of "maximum guaranteed timeout"?  The timeout is
>> what is programmed in "Data Timeout Counter Value".
> 
> And exactly this "Data Timeout Counter Value" is not used in your code to
> predict controller's data line timeout.
> 
>>> supposed to be used in calculations of a guaranteed discard operation
>>> timeout
>>> instead of promised DAT timeout by a controller?
>>
>> What is "promised DAT timeout"?
> 
> This is a timeout with respect to "Data Timeout Counter Value".
> 
> According to your words max_discard_to is the maximum timeout that the host
> controller supports, but such a parameter is useless, because nobody sets
> the host controller SDHCI_TIMEOUT_CONTROL register to maximum supported value,

sdhci_prepare_data() -> sdhci_calc_timeout() sets the timeout based on what
the upper layers specify, up to and including the maximum value.

So what max_discard_to does is to limit the erase size so that when
sdhci_calc_timeout() is called it won't exceed the maximum.

> so there is a probability that you greatly overestimate Data Timeout value,
> and therefore block layer or other subsystem can't rely on it. Please correct
> me here.
> 
>>>
>>>>>
>>>>>> Currently, the limit gets applied by the block layer before the mmc
>>>>>> layer is
>>>>>> involved so there is no possibility to take the actual timeout into
>>>>>> account.
>>>>>>     However if you have erase_group_def set, then it won't make any
>>>>>> difference
>>>>>> i.e. the limit will be the same.
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 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<[e-mail]>
>>>>>>>>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>>>>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>>>>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>        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 [e-mail]
>>>>>> 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 [e-mail]
>>>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vladimir Zapolskiy Nov. 29, 2013, 7:33 a.m. UTC | #18

On 11/28/13 15:06, Adrian Hunter wrote:
> On 28/11/13 13:48, Vladimir Zapolskiy wrote:
>> On 11/28/13 09:12, Adrian Hunter wrote:
>>> On 27/11/13 16:57, Vladimir Zapolskiy wrote:
>>>> On 11/27/13 10:21, Adrian Hunter wrote:
>>>>> On 26/11/13 18:33, Vladimir Zapolskiy wrote:
>>>>>> On 11/26/13 11:04, Adrian Hunter wrote:
>>>>>>> On 22/11/13 17:21, Vladimir Zapolskiy wrote:
>>>>>>>> On 22.11.2013 16:04, Adrian Hunter wrote:
>>>>>>>>> On 22/11/13 15:50, Vladimir Zapolskiy wrote:
>>>>>>>>>> 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?
>>>>>>>>>
>>>>>>>>> I don't understand how SDHCI_QUIRK_BROKEN_TIMEOUT_VAL would help.  It
>>>>>>>>> just
>>>>>>>>> sets the timeout to maximum but max_discard_to is the maximum timeout.
>>>>>>>>
>>>>>>>> Here I meant to do something like:
>>>>>>>>
>>>>>>>>         if (host->quirks&     SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>>>>>>>>             mmc->max_discard_to = 0;
>>>>>>>>
>>>>>>>> Again I'm not sure that this applies well to all
>>>>>>>> SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
>>>>>>>> controllers, therefore a new quirk might be better.
>>>>>>>>
>>>>>>>>> As I understand it you don't want to limit the discard size, either
>>>>>>>>> because
>>>>>>>>> your controller does not timeout, or because you are happy that the
>>>>>>>>> maximum
>>>>>>>>> timeout is enough for your users and their use-cases.
>>>>>>>>>
>>>>>>>>> If that is the case then the original patch just needs the quirk the
>>>>>>>>> other
>>>>>>>>> way around. i.e.
>>>>>>>>>
>>>>>>>>>          if (host->quirks2&      SDHCI_QUIRK2_NO_DISCARD_LIMIT)
>>>>>>>>>              mmc->max_discard_to = 0;
>>>>>>>>>          else
>>>>>>>>>              mmc->max_discard_to = (1<<      27) / host->timeout_clk;
>>>>>>>>
>>>>>>>> This suits me fine, thanks for review, and I'll resend a change based on
>>>>>>>> this.
>>>>>>>>
>>>>>>>> Also I'd like to pay your attention to (1<<     27) / host->timeout_clk
>>>>>>>> part of
>>>>>>>> calculation, following the spec it might be better to account the actual
>>>>>>>> value of Data Timeout Counter, otherwise a controller may get
>>>>>>>> unintentional
>>>>>>>> Data Timeout Error pretty soon. Please correct me, if I'm mistaken here.
>>>>>>>
>>>>>>> Not sure what you mean.  max_discard_to is the maximum timeout (in
>>>>>>> milliseconds) that the host controller supports.  The intent is to limit
>>>>>>> erase operations to ones that have a timeout that is less than or
>>>>>>> equal to
>>>>>>> that.
>>>>>>
>>>>>> That's clear. But it's not obvious why do you prefer (1<<    27) numerator
>>>>>> instead
>>>>>> of secure (1<<    13) or (1<<    (13 + sdhci_readl(host,
>>>>>> SDHCI_TIMEOUT_CONTROL))).
>>>>>
>>>>> The maximum value of "Data Timeout Counter Value" in "Timeout Control
>>>>> Register" is 14 and the maximum timeout is therefore (1<<    27).
>>>>
>>>> So, from this perspective I assume this is a potential theoretical maximum
>>>> timeout for a controller, which may be 16384 times more than the maximum
>>>> guaranteed timeout before getting a DAT timeout. Why is the theoretical
>>>> maximum
>>>
>>> Where do you get the notion of "maximum guaranteed timeout"?  The timeout is
>>> what is programmed in "Data Timeout Counter Value".
>>
>> And exactly this "Data Timeout Counter Value" is not used in your code to
>> predict controller's data line timeout.
>>
>>>> supposed to be used in calculations of a guaranteed discard operation
>>>> timeout
>>>> instead of promised DAT timeout by a controller?
>>>
>>> What is "promised DAT timeout"?
>>
>> This is a timeout with respect to "Data Timeout Counter Value".
>>
>> According to your words max_discard_to is the maximum timeout that the host
>> controller supports, but such a parameter is useless, because nobody sets
>> the host controller SDHCI_TIMEOUT_CONTROL register to maximum supported value,
>
> sdhci_prepare_data() ->  sdhci_calc_timeout() sets the timeout based on what
> the upper layers specify, up to and including the maximum value.
>
> So what max_discard_to does is to limit the erase size so that when
> sdhci_calc_timeout() is called it won't exceed the maximum.

Now the idea is clear, thank you.

>> so there is a probability that you greatly overestimate Data Timeout value,
>> and therefore block layer or other subsystem can't rely on it. Please correct
>> me here.
>>
>>>>
>>>>>>
>>>>>>> Currently, the limit gets applied by the block layer before the mmc
>>>>>>> layer is
>>>>>>> involved so there is no possibility to take the actual timeout into
>>>>>>> account.
>>>>>>>      However if you have erase_group_def set, then it won't make any
>>>>>>> difference
>>>>>>> i.e. the limit will be the same.
>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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<[e-mail]>
>>>>>>>>>>>>>> Reported-by: Ed Sutter<[e-mail]>
>>>>>>>>>>>>>> Cc: Chris Ball<[e-mail]>
>>>>>>>>>>>>>> Cc: Adrian Hunter<[e-mail]>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>         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 [e-mail]
>>>>>>> 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 [e-mail]
>>>>> 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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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