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

[Aaron Lu <aaron.lwe@gmail.com>]

On 11/13/2013 07:25 PM, Dong Aisheng wrote:
> On Wed, Nov 13, 2013 at 7:18 PM, Arend van Spriel <arend@broadcom.com> wrote:
>> 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.
>>
> 
> Right, so that means we could remove the tuning_mode check in the
> else-statement.

I agree.

-Aaron

> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 2d55e6a..b2928ef 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2015,12 +2015,11 @@ 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 timeout workqueue */
> -               if (host->tuning_mode == SDHCI_TUNING_MODE_1)
> -                       schedule_delayed_work(&host->tuning_timeout_work,
> -                               host->tuning_count * HZ);
> +               schedule_delayed_work(&host->tuning_timeout_work,
> +                       host->tuning_count * HZ);
>         }
> 
>         /*
> 
> Regards
> Dong Aisheng
> 
>>
>> 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
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
> --
> 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
>