Hi Arend,
On Wed, Nov 13, 2013 at 6:21 PM, Arend van Spriel <arend@xxxxxxxxxxxx> wrote:
> On 11/13/2013 06:02 AM, Dong Aisheng wrote:
>>
>> Hi Arend,
>>
>> On Mon, Nov 11, 2013 at 5:49 PM, Arend van Spriel <arend@xxxxxxxxxxxx>
>> wrote:
>>>
>>> When the host->tuning_count is zero it means that the
>>> retuning is disabled. This is checked on the first
>>> run of sdhci_execute_tuning() by the if statement below:
>>>
>>> if (!(host->flags & SDHCI_NEEDS_RETUNING) && host->tuning_count
>>> &&
>>> (host->tuning_mode == SDHCI_TUNING_MODE_1)) {
>>>
>>> So only when tuning_count is non-zero it will set the host
>>> flag SDHCI_USING_RETUNING_TIMER. The else statement is only
>>> for re-programming the timer, which means that flag must be
>>> set. Because that is not checked the else statement is executed
>>> in the first run when tuning_count is zero.
>>>
>>> This was seen on a host controller which indicated
>>> SDHCI_TUNING_MODE_1 (0) and tuning_count being zero. Suspect
>>> that (one of) these registers is not properly set.
>>>
>>> Signed-off-by: Arend van Spriel <arend@xxxxxxxxxxxx>
>>> ---
>>> This patch applies to the mmc-next branch.
>>>
>>> V2:
>>> - add more explanation to the commit message
>>> - check host flag SDHCI_USING_RETUNING_TIMER
>>> ---
>>> drivers/mmc/host/sdhci.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index bd8a098..5974599 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -2014,7 +2014,7 @@ out:
>>> host->tuning_count * HZ);
>>> /* Tuning mode 1 limits the maximum data length to 4MB
>>> */
>>> mmc->max_blk_count = (4 * 1024 * 1024) /
>>> mmc->max_blk_size;
>>> - } else {
>>> + } else if (host->flags & SDHCI_USING_RETUNING_TIMER) {
>>> host->flags &= ~SDHCI_NEEDS_RETUNING;
>>> /* Reload the new initial value for timer */
>>> if (host->tuning_mode == SDHCI_TUNING_MODE_1)
>>
>>
>> I wonder if we could also remove this line?
>> It looks to me it's not neccesary to check the tuning_mode again since
>> we already check the flag
>> above and SDHCI_TUNING_MODE_1 seems like the prerequisite of
>> SDHCI_USING_RETUNING_TIMER.
>
>
> According the spec the other tuning modes also can use retuning timer.
> Currently, the mmc stack in upstream linux only supports tuning mode 1. When
> adding the other modes this if statement will probably go.
>
For currently code, it looks like also not necessary to check it since
SDHCI_USING_RETUNING_TIMER will only be set when tunning_mode is
SDHCI_TUNING_MODE_1.
And SDHCI_TUNING_MODE_1 just indicates the tuning mode while the flag
SDHCI_USING_RETUNING_TIMER represents the retuning timer implementation.
So check the flag to invoke the timer seems make more sense to me.
do you agree?
Regards
Dong Aisheng
> Regards,
> Arend
>
>
>> Regards
>> Dong Aisheng
>>
>>> --
>>> 1.7.10.4
>>>
>>>
>>> --
>>> 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
>>
>>
>
>
--
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
