* [PATCH v2] mmc: core: not need to check timeout for SDHC
@ 2017-08-04 2:08 Shawn Lin
2017-08-04 6:38 ` Adrian Hunter
0 siblings, 1 reply; 6+ messages in thread
From: Shawn Lin @ 2017-08-04 2:08 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
Per the SD physical layer simplified specification V4.10,
section 4.6.2, CSD version 1.0 SD card should use taac, nsac
and r2w_factor for calculating the data access time. But the
taac and nsac for SDHC(CSD version 2.0) are always fixed and
the software should use the recommended value for timeout. When
parsing the CSD, we sanely set them to zero for SDHC(CSD version
2.0), all the calculation for timeout_ns and timeout_clk is zero
as well. So what we actually want to limit here is either SDHC
case or unreasonable timeout reported by the cards. In principle
we should at least be able to remove the bogus check for the
mmc_card_blockaddr.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
Changes in v2:
- rephrase the changelog and only remove mmc_card_blockaddr.
drivers/mmc/core/core.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6177eb0..edc2e75 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -761,15 +761,11 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
limit_us = 100000;
/*
- * SDHC cards always use these fixed values.
+ * Assign limit value if invalid. Note that for the SDHC case,
+ * we set taac and nasc to zero when parsing CSD, so it's safe
+ * to fall through here.
*/
- if (timeout_us > limit_us || mmc_card_blockaddr(card)) {
- data->timeout_ns = limit_us * 1000;
- data->timeout_clks = 0;
- }
-
- /* assign limit value if invalid */
- if (timeout_us == 0)
+ if (timeout_us == 0 || timeout_us > limit_us)
data->timeout_ns = limit_us * 1000;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mmc: core: not need to check timeout for SDHC
2017-08-04 2:08 [PATCH v2] mmc: core: not need to check timeout for SDHC Shawn Lin
@ 2017-08-04 6:38 ` Adrian Hunter
2017-08-04 7:22 ` Shawn Lin
0 siblings, 1 reply; 6+ messages in thread
From: Adrian Hunter @ 2017-08-04 6:38 UTC (permalink / raw)
To: Shawn Lin, Ulf Hansson; +Cc: linux-mmc
On 04/08/17 05:08, Shawn Lin wrote:
> Per the SD physical layer simplified specification V4.10,
> section 4.6.2, CSD version 1.0 SD card should use taac, nsac
> and r2w_factor for calculating the data access time. But the
> taac and nsac for SDHC(CSD version 2.0) are always fixed and
> the software should use the recommended value for timeout. When
> parsing the CSD, we sanely set them to zero for SDHC(CSD version
> 2.0), all the calculation for timeout_ns and timeout_clk is zero
> as well. So what we actually want to limit here is either SDHC
> case or unreasonable timeout reported by the cards. In principle
> we should at least be able to remove the bogus check for the
> mmc_card_blockaddr.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> ---
>
> Changes in v2:
> - rephrase the changelog and only remove mmc_card_blockaddr.
>
> drivers/mmc/core/core.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 6177eb0..edc2e75 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -761,15 +761,11 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
> limit_us = 100000;
>
> /*
> - * SDHC cards always use these fixed values.
> + * Assign limit value if invalid. Note that for the SDHC case,
> + * we set taac and nasc to zero when parsing CSD, so it's safe
> + * to fall through here.
> */
> - if (timeout_us > limit_us || mmc_card_blockaddr(card)) {
> - data->timeout_ns = limit_us * 1000;
> - data->timeout_clks = 0;
> - }
> -
> - /* assign limit value if invalid */
> - if (timeout_us == 0)
> + if (timeout_us == 0 || timeout_us > limit_us)
> data->timeout_ns = limit_us * 1000;
Shouldn't we still be ensuring 'data->timeout_clks = 0' for the 'timeout_us
> limit_us' case
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mmc: core: not need to check timeout for SDHC
2017-08-04 6:38 ` Adrian Hunter
@ 2017-08-04 7:22 ` Shawn Lin
2017-08-04 7:24 ` Shawn Lin
0 siblings, 1 reply; 6+ messages in thread
From: Shawn Lin @ 2017-08-04 7:22 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson; +Cc: shawn.lin, linux-mmc
On 2017/8/4 14:38, Adrian Hunter wrote:
> On 04/08/17 05:08, Shawn Lin wrote:
>> Per the SD physical layer simplified specification V4.10,
>> section 4.6.2, CSD version 1.0 SD card should use taac, nsac
>> and r2w_factor for calculating the data access time. But the
>> taac and nsac for SDHC(CSD version 2.0) are always fixed and
>> the software should use the recommended value for timeout. When
>> parsing the CSD, we sanely set them to zero for SDHC(CSD version
>> 2.0), all the calculation for timeout_ns and timeout_clk is zero
>> as well. So what we actually want to limit here is either SDHC
>> case or unreasonable timeout reported by the cards. In principle
>> we should at least be able to remove the bogus check for the
>> mmc_card_blockaddr.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - rephrase the changelog and only remove mmc_card_blockaddr.
>>
>> drivers/mmc/core/core.c | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 6177eb0..edc2e75 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -761,15 +761,11 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
>> limit_us = 100000;
>>
>> /*
>> - * SDHC cards always use these fixed values.
>> + * Assign limit value if invalid. Note that for the SDHC case,
>> + * we set taac and nasc to zero when parsing CSD, so it's safe
>> + * to fall through here.
>> */
>> - if (timeout_us > limit_us || mmc_card_blockaddr(card)) {
>> - data->timeout_ns = limit_us * 1000;
>> - data->timeout_clks = 0;
>> - }
>> -
>> - /* assign limit value if invalid */
>> - if (timeout_us == 0)
>> + if (timeout_us == 0 || timeout_us > limit_us)
>> data->timeout_ns = limit_us * 1000;
>
> Shouldn't we still be ensuring 'data->timeout_clks = 0' for the 'timeout_us
>> limit_us' case
I have a question that why we expose data->timeout_clks for host
drivers?
I quick look at how the host drivers use it and find it almost does
the same thing as the core layer if card->host->ios.clock is present.
So shouldn't we *always* set data->timeout_clks to zero and fold the
extra cycle(actually it's NSAC) into data->timeout_ns so that the host
drivers only need to care about data->timeout_clks?
>
>> }
>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mmc: core: not need to check timeout for SDHC
2017-08-04 7:22 ` Shawn Lin
@ 2017-08-04 7:24 ` Shawn Lin
2017-08-04 7:47 ` Adrian Hunter
0 siblings, 1 reply; 6+ messages in thread
From: Shawn Lin @ 2017-08-04 7:24 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson; +Cc: shawn.lin, linux-mmc
On 2017/8/4 15:22, Shawn Lin wrote:
> On 2017/8/4 14:38, Adrian Hunter wrote:
>> On 04/08/17 05:08, Shawn Lin wrote:
>>> Per the SD physical layer simplified specification V4.10,
>>> section 4.6.2, CSD version 1.0 SD card should use taac, nsac
>>> and r2w_factor for calculating the data access time. But the
>>> taac and nsac for SDHC(CSD version 2.0) are always fixed and
>>> the software should use the recommended value for timeout. When
>>> parsing the CSD, we sanely set them to zero for SDHC(CSD version
>>> 2.0), all the calculation for timeout_ns and timeout_clk is zero
>>> as well. So what we actually want to limit here is either SDHC
>>> case or unreasonable timeout reported by the cards. In principle
>>> we should at least be able to remove the bogus check for the
>>> mmc_card_blockaddr.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - rephrase the changelog and only remove mmc_card_blockaddr.
>>>
>>> drivers/mmc/core/core.c | 12 ++++--------
>>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 6177eb0..edc2e75 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -761,15 +761,11 @@ void mmc_set_data_timeout(struct mmc_data
>>> *data, const struct mmc_card *card)
>>> limit_us = 100000;
>>> /*
>>> - * SDHC cards always use these fixed values.
>>> + * Assign limit value if invalid. Note that for the SDHC case,
>>> + * we set taac and nasc to zero when parsing CSD, so it's safe
>>> + * to fall through here.
>>> */
>>> - if (timeout_us > limit_us || mmc_card_blockaddr(card)) {
>>> - data->timeout_ns = limit_us * 1000;
>>> - data->timeout_clks = 0;
>>> - }
>>> -
>>> - /* assign limit value if invalid */
>>> - if (timeout_us == 0)
>>> + if (timeout_us == 0 || timeout_us > limit_us)
>>> data->timeout_ns = limit_us * 1000;
>>
>> Shouldn't we still be ensuring 'data->timeout_clks = 0' for the
>> 'timeout_us
>>> limit_us' case
>
> I have a question that why we expose data->timeout_clks for host
> drivers?
>
> I quick look at how the host drivers use it and find it almost does
> the same thing as the core layer if card->host->ios.clock is present.
> So shouldn't we *always* set data->timeout_clks to zero and fold the
> extra cycle(actually it's NSAC) into data->timeout_ns so that the host
> drivers only need to care about data->timeout_clks?
>
Sorry. s/care about data->timeout_clks/care about data->timeout_ns?
>>
>>> }
>>>
>>
>>
>>
>>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mmc: core: not need to check timeout for SDHC
2017-08-04 7:24 ` Shawn Lin
@ 2017-08-04 7:47 ` Adrian Hunter
2017-08-04 8:18 ` Shawn Lin
0 siblings, 1 reply; 6+ messages in thread
From: Adrian Hunter @ 2017-08-04 7:47 UTC (permalink / raw)
To: Shawn Lin, Ulf Hansson; +Cc: linux-mmc
On 04/08/17 10:24, Shawn Lin wrote:
>
> On 2017/8/4 15:22, Shawn Lin wrote:
>> On 2017/8/4 14:38, Adrian Hunter wrote:
>>> On 04/08/17 05:08, Shawn Lin wrote:
>>>> Per the SD physical layer simplified specification V4.10,
>>>> section 4.6.2, CSD version 1.0 SD card should use taac, nsac
>>>> and r2w_factor for calculating the data access time. But the
>>>> taac and nsac for SDHC(CSD version 2.0) are always fixed and
>>>> the software should use the recommended value for timeout. When
>>>> parsing the CSD, we sanely set them to zero for SDHC(CSD version
>>>> 2.0), all the calculation for timeout_ns and timeout_clk is zero
>>>> as well. So what we actually want to limit here is either SDHC
>>>> case or unreasonable timeout reported by the cards. In principle
>>>> we should at least be able to remove the bogus check for the
>>>> mmc_card_blockaddr.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - rephrase the changelog and only remove mmc_card_blockaddr.
>>>>
>>>> drivers/mmc/core/core.c | 12 ++++--------
>>>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index 6177eb0..edc2e75 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -761,15 +761,11 @@ void mmc_set_data_timeout(struct mmc_data *data,
>>>> const struct mmc_card *card)
>>>> limit_us = 100000;
>>>> /*
>>>> - * SDHC cards always use these fixed values.
>>>> + * Assign limit value if invalid. Note that for the SDHC case,
>>>> + * we set taac and nasc to zero when parsing CSD, so it's safe
>>>> + * to fall through here.
>>>> */
>>>> - if (timeout_us > limit_us || mmc_card_blockaddr(card)) {
>>>> - data->timeout_ns = limit_us * 1000;
>>>> - data->timeout_clks = 0;
>>>> - }
>>>> -
>>>> - /* assign limit value if invalid */
>>>> - if (timeout_us == 0)
>>>> + if (timeout_us == 0 || timeout_us > limit_us)
>>>> data->timeout_ns = limit_us * 1000;
>>>
>>> Shouldn't we still be ensuring 'data->timeout_clks = 0' for the 'timeout_us
>>>> limit_us' case
>>
>> I have a question that why we expose data->timeout_clks for host
>> drivers?
>>
>> I quick look at how the host drivers use it and find it almost does
>> the same thing as the core layer if card->host->ios.clock is present.
>> So shouldn't we *always* set data->timeout_clks to zero and fold the
>> extra cycle(actually it's NSAC) into data->timeout_ns so that the host
>> drivers only need to care about data->timeout_clks?
>>
>
> Sorry. s/care about data->timeout_clks/care about data->timeout_ns?
Pedantically, the calculation should, in fact, be using the actual clock
frequency not the target frequency card->host->ios.clock.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mmc: core: not need to check timeout for SDHC
2017-08-04 7:47 ` Adrian Hunter
@ 2017-08-04 8:18 ` Shawn Lin
0 siblings, 0 replies; 6+ messages in thread
From: Shawn Lin @ 2017-08-04 8:18 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson; +Cc: shawn.lin, linux-mmc
On 2017/8/4 15:47, Adrian Hunter wrote:
> On 04/08/17 10:24, Shawn Lin wrote:
>>
>> On 2017/8/4 15:22, Shawn Lin wrote:
>>> On 2017/8/4 14:38, Adrian Hunter wrote:
>>>> On 04/08/17 05:08, Shawn Lin wrote:
>>>>> Per the SD physical layer simplified specification V4.10,
>>>>> section 4.6.2, CSD version 1.0 SD card should use taac, nsac
>>>>> and r2w_factor for calculating the data access time. But the
>>>>> taac and nsac for SDHC(CSD version 2.0) are always fixed and
>>>>> the software should use the recommended value for timeout. When
>>>>> parsing the CSD, we sanely set them to zero for SDHC(CSD version
>>>>> 2.0), all the calculation for timeout_ns and timeout_clk is zero
>>>>> as well. So what we actually want to limit here is either SDHC
>>>>> case or unreasonable timeout reported by the cards. In principle
>>>>> we should at least be able to remove the bogus check for the
>>>>> mmc_card_blockaddr.
>>>>>
>>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - rephrase the changelog and only remove mmc_card_blockaddr.
>>>>>
>>>>> drivers/mmc/core/core.c | 12 ++++--------
>>>>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>> index 6177eb0..edc2e75 100644
>>>>> --- a/drivers/mmc/core/core.c
>>>>> +++ b/drivers/mmc/core/core.c
>>>>> @@ -761,15 +761,11 @@ void mmc_set_data_timeout(struct mmc_data *data,
>>>>> const struct mmc_card *card)
>>>>> limit_us = 100000;
>>>>> /*
>>>>> - * SDHC cards always use these fixed values.
>>>>> + * Assign limit value if invalid. Note that for the SDHC case,
>>>>> + * we set taac and nasc to zero when parsing CSD, so it's safe
>>>>> + * to fall through here.
>>>>> */
>>>>> - if (timeout_us > limit_us || mmc_card_blockaddr(card)) {
>>>>> - data->timeout_ns = limit_us * 1000;
>>>>> - data->timeout_clks = 0;
>>>>> - }
>>>>> -
>>>>> - /* assign limit value if invalid */
>>>>> - if (timeout_us == 0)
>>>>> + if (timeout_us == 0 || timeout_us > limit_us)
>>>>> data->timeout_ns = limit_us * 1000;
>>>>
>>>> Shouldn't we still be ensuring 'data->timeout_clks = 0' for the 'timeout_us
>>>>> limit_us' case
>>>
>>> I have a question that why we expose data->timeout_clks for host
>>> drivers?
>>>
>>> I quick look at how the host drivers use it and find it almost does
>>> the same thing as the core layer if card->host->ios.clock is present.
>>> So shouldn't we *always* set data->timeout_clks to zero and fold the
>>> extra cycle(actually it's NSAC) into data->timeout_ns so that the host
>>> drivers only need to care about data->timeout_clks?
>>>
>>
>> Sorry. s/care about data->timeout_clks/care about data->timeout_ns?
>
> Pedantically, the calculation should, in fact, be using the actual clock
> frequency not the target frequency card->host->ios.clock.
yes. I assumed generally the actual clock shouldn't be less too much
than target frequency, otherwise timeout_us may actually be much larger
than limit_us if the actual clock is lower enough, even if the core
think it shouldn't be that case estimated by target clock. But it seems
pointless to argue this here since that's too pedantical as you said.
Thanks for sharing your thought.
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-04 8:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-04 2:08 [PATCH v2] mmc: core: not need to check timeout for SDHC Shawn Lin
2017-08-04 6:38 ` Adrian Hunter
2017-08-04 7:22 ` Shawn Lin
2017-08-04 7:24 ` Shawn Lin
2017-08-04 7:47 ` Adrian Hunter
2017-08-04 8:18 ` Shawn Lin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.