RESEND:[PATCH V3] sdhci: only reprogram retuning timer when flag is set

RESEND:[PATCH V3] sdhci: only reprogram retuning timer when flag is set

[Arend van Spriel]

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.

Cc: Dong Aisheng <dongas86@gmail.com>
Cc: Aaron Lu <aaron.lwe@gmail.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
Noticed this patch was still not applied so please reconsider
taking it in and let me know. The patch has been rebased and
applies to the mmc-next branch.

Regards,
Arend

V3:
- remote tuning mode check for retuning timer reload

V2:
- add more explanation to the commit message
- check host flag SDHCI_USING_RETUNING_TIMER
---
 drivers/mmc/host/sdhci.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9ddef47..d5b421d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2026,12 +2026,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 timer */
-		if (host->tuning_mode == SDHCI_TUNING_MODE_1)
-			mod_timer(&host->tuning_timer, jiffies +
-				host->tuning_count * HZ);
+		mod_timer(&host->tuning_timer, jiffies +
+			  host->tuning_count * HZ);
 	}
 
 	/*
-- 
1.8.1.3

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

[Arend van Spriel]

On 02/21/2014 08:59 PM, Arend van Spriel 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.
> 
> Cc: Dong Aisheng <dongas86@gmail.com>
> Cc: Aaron Lu <aaron.lwe@gmail.com>
> Signed-off-by: Arend van Spriel <arend@broadcom.com>
> ---
> Noticed this patch was still not applied so please reconsider
> taking it in and let me know. The patch has been rebased and
> applies to the mmc-next branch.

ping? Am I on some spam filter? What is needed to get this change applied?

Regards,
Arend

> Regards,
> Arend
> 
> V3:
> - remote tuning mode check for retuning timer reload
> 
> V2:
> - add more explanation to the commit message
> - check host flag SDHCI_USING_RETUNING_TIMER
> ---
>  drivers/mmc/host/sdhci.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 9ddef47..d5b421d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2026,12 +2026,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 timer */
> -		if (host->tuning_mode == SDHCI_TUNING_MODE_1)
> -			mod_timer(&host->tuning_timer, jiffies +
> -				host->tuning_count * HZ);
> +		mod_timer(&host->tuning_timer, jiffies +
> +			  host->tuning_count * HZ);
>  	}
>  
>  	/*
> 

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

[Ulf Hansson]

On 6 March 2014 10:29, Arend van Spriel <arend@broadcom.com> wrote:
> On 02/21/2014 08:59 PM, Arend van Spriel 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.
>>
>> Cc: Dong Aisheng <dongas86@gmail.com>
>> Cc: Aaron Lu <aaron.lwe@gmail.com>
>> Signed-off-by: Arend van Spriel <arend@broadcom.com>
>> ---
>> Noticed this patch was still not applied so please reconsider
>> taking it in and let me know. The patch has been rebased and
>> applies to the mmc-next branch.
>
> ping? Am I on some spam filter? What is needed to get this change applied?
>
> Regards,
> Arend
>
>> Regards,
>> Arend
>>
>> V3:
>> - remote tuning mode check for retuning timer reload
>>
>> V2:
>> - add more explanation to the commit message
>> - check host flag SDHCI_USING_RETUNING_TIMER
>> ---
>>  drivers/mmc/host/sdhci.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 9ddef47..d5b421d 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2026,12 +2026,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 timer */
>> -             if (host->tuning_mode == SDHCI_TUNING_MODE_1)
>> -                     mod_timer(&host->tuning_timer, jiffies +
>> -                             host->tuning_count * HZ);
>> +             mod_timer(&host->tuning_timer, jiffies +
>> +                       host->tuning_count * HZ);
>>       }
>>
>>       /*
>>
>

I don't have any deeper insight about the retuning mechanism for
sdhci, still this seems reasonable.

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

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

[Arend van Spriel]

On 03/07/14 05:06, Ulf Hansson wrote:
> On 6 March 2014 10:29, Arend van Spriel<arend@broadcom.com>  wrote:
>> On 02/21/2014 08:59 PM, Arend van Spriel 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.
>>>
>>> Cc: Dong Aisheng<dongas86@gmail.com>
>>> Cc: Aaron Lu<aaron.lwe@gmail.com>
>>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>>> ---
>>> Noticed this patch was still not applied so please reconsider
>>> taking it in and let me know. The patch has been rebased and
>>> applies to the mmc-next branch.
>>
>> ping? Am I on some spam filter? What is needed to get this change applied?

Hi Chris,

Did this patch fall between the cracks. If needed I can rebase and 
resend it once more.

Regards,
Arend

>> Regards,
>> Arend
>>
>>> Regards,
>>> Arend
>>>
>>> V3:
>>> - remote tuning mode check for retuning timer reload
>>>
>>> V2:
>>> - add more explanation to the commit message
>>> - check host flag SDHCI_USING_RETUNING_TIMER
>>> ---
>>>   drivers/mmc/host/sdhci.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 9ddef47..d5b421d 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -2026,12 +2026,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 timer */
>>> -             if (host->tuning_mode == SDHCI_TUNING_MODE_1)
>>> -                     mod_timer(&host->tuning_timer, jiffies +
>>> -                             host->tuning_count * HZ);
>>> +             mod_timer(&host->tuning_timer, jiffies +
>>> +                       host->tuning_count * HZ);
>>>        }
>>>
>>>        /*
>>>
>>
>
> I don't have any deeper insight about the retuning mechanism for
> sdhci, still this seems reasonable.
>
> Acked-by: Ulf Hansson<ulf.hansson@linaro.org>

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

[Aaron Lu]

On 02/22/2014 03:59 AM, Arend van Spriel 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.
> 
> Cc: Dong Aisheng <dongas86@gmail.com>
> Cc: Aaron Lu <aaron.lwe@gmail.com>
> Signed-off-by: Arend van Spriel <arend@broadcom.com>

In addition to solve your problem, this patch also makes sense in the
common case, so:

Reviewed-by: Aaron Lu <aaron.lu@intel.com>

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

[Arend van Spriel]

On 03/20/14 06:32, Aaron Lu wrote:
> On 02/22/2014 03:59 AM, Arend van Spriel 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.
>>
>> Cc: Dong Aisheng<dongas86@gmail.com>
>> Cc: Aaron Lu<aaron.lwe@gmail.com>
>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>
> In addition to solve your problem, this patch also makes sense in the
> common case, so:
>
> Reviewed-by: Aaron Lu<aaron.lu@intel.com>

Hi Chris,

Is this patch still in your queue?

Regards,
Arend

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

[Chris Ball]

Hi Arend,

On Tue, Mar 25 2014, Arend van Spriel wrote:
> On 03/20/14 06:32, Aaron Lu wrote:
>> On 02/22/2014 03:59 AM, Arend van Spriel 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.
>>>
>>> Cc: Dong Aisheng<dongas86@gmail.com>
>>> Cc: Aaron Lu<aaron.lwe@gmail.com>
>>> Signed-off-by: Arend van Spriel<arend@broadcom.com>
>>
>> In addition to solve your problem, this patch also makes sense in the
>> common case, so:
>>
>> Reviewed-by: Aaron Lu<aaron.lu@intel.com>
>
> Hi Chris,
>
> Is this patch still in your queue?

Thanks!  I've pushed this to mmc-next for 3.15 with Aaron/Ulf's ACKs now.

- Chris.
-- 
Chris Ball   <chris@printf.net>   <http://printf.net/>