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