Re: [PATCH V2] sdhci: only reprogram retuning timer when flag is set

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux