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

["Arend van Spriel" <arend@broadcom.com>]

On 11/13/2013 12:12 PM, Dong Aisheng wrote:
> Hi Arend,
>
> On Wed, Nov 13, 2013 at 6:21 PM, Arend van Spriel <arend@broadcom.com> 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@broadcom.com>
>>> 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@broadcom.com>
>>>> ---
>>>> 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?

The flag SDHCI_USING_RETUNING_TIMER is only set after the initial tuning 
run so in the if-statement. So currently in the else-statement the fact 
that SDHCI_USING_RETUNING_TIMER is set implies SDHCI_TUNING_MODE_1.

Regards,
Arend

> 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@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>>
>