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

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

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.

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);
>>>>    	}
>>>>
>>>
>>>
>>
>>
>
>