Re: [PATCH] sdhci: do not program timer when tuning_count is zero

[Aaron Lu <aaron.lu@intel.com>]

On 11/08/2013 07:55 PM, Arend van Spriel wrote:
> On 11/08/2013 12:49 PM, Aaron Lu wrote:
>> On 11/08/2013 05:10 PM, Arend van Spriel wrote:
>>> On 11/08/2013 07:50 AM, Aaron Lu wrote:
>>>> On 11/07/2013 06:59 PM, Arend van Spriel wrote:
>>>>> When the host->tuning_count is zero it means that the
>>>>
>>>> If the tuning_count is zero, then the retuning timer shouldn't be
>>>> started in the first place and not possible to run code there. Or is the
>>>> tuning_count dynamically changed?
>>>
>>> Actually, the sdhci_execute_tuning() must run once to do the initial
>>> tuning procedure. This is mandatory for SDR104. However, *re*tuning is
>>> not and a zero tuning_count disables it.
>>
>> So the host in question doesn't need do retuning while in SDR104 mode?
>> It seems your host's retuning mode is mode_1.
>>
>>>
>>> The function is executed initially. The 'if' statement above the patched
>>> 'else' statement is actually responsible for programming the retuning
>>> timer for the first time. However, it requires tuning_count to be
>>> non-zero. The 'else' statement is actually for reloading the retuning
>>> timer, which is not the case. Adding the non-zero check assures the
>>> retuning timer is never started.
>>
>> OK, I see.
>> The SDHCI_USING_RETUNING_TIMER flag is supposed to mean if the host is
>> currently using retuning timer to do retuning, it could also be used to
>> decide if retuning timer needs be re-programmed.
> 
> True. I can go for that approach.
> 
>> Anyway, a host in retuning mode 1 does not have tuning_count set seems a
>> little odd to me...
> 
> Looking at sdhci.h it actually seems sdhci code is missing support for 
> the other modes:
> 
> 	unsigned int		tuning_count;	/* Timer count for re-tuning */
> 	unsigned int		tuning_mode;	/* Re-tuning mode supported by host */
> #define SDHCI_TUNING_MODE_1	0
> 	struct timer_list	tuning_timer;	/* Timer for tuning */
> 
> At least the other modes are not defined here.

Right, only retuning mode 1 is implemented currently.

Thanks,
Aaron

> 
> Regards,
> Arend
> 
>> Thanks,
>> Aaron
>>
>>>
>>> I guess the fact that this needs explaining indicates that the commit
>>> message should be updated. I will send a V2 for this.
>>>
>>> Regards,
>>> Arend
>>>
>>>> Thanks,
>>>> Aaron
>>>>
>>>>> retuning is disabled. Doing a mod_timer() with a zero
>>>>> tuning_count does something else.
>>>>>
>>>>> Signed-off-by: Arend van Spriel <arend@broadcom.com>
>>>>> ---
>>>>>    drivers/mmc/host/sdhci.c |    3 ++-
>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>>> index 7a7fb4f..9803e7a 100644
>>>>> --- a/drivers/mmc/host/sdhci.c
>>>>> +++ b/drivers/mmc/host/sdhci.c
>>>>> @@ -2007,7 +2007,8 @@ out:
>>>>>    	} else {
>>>>>    		host->flags &= ~SDHCI_NEEDS_RETUNING;
>>>>>    		/* Reload the new initial value for timer */
>>>>> -		if (host->tuning_mode == SDHCI_TUNING_MODE_1)
>>>>> +		if (host->tuning_count &&
>>>>> +		    host->tuning_mode == SDHCI_TUNING_MODE_1)
>>>>>    			mod_timer(&host->tuning_timer, jiffies +
>>>>>    				host->tuning_count * HZ);
>>>>>    	}
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 
>