On 11/13/2013 07:25 PM, Dong Aisheng wrote:
> On Wed, Nov 13, 2013 at 7:18 PM, Arend van Spriel <arend@xxxxxxxxxxxx> 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@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?
>>
>>
>> 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@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
>
--
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
