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

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

[Arend van Spriel]

When the host->tuning_count is zero it means that the
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);
 	}
-- 
1.7.10.4

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

[Aaron Lu]

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?

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

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

[Arend van Spriel]

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.

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.

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

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

[Aaron Lu]

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.

Anyway, a host in retuning mode 1 does not have tuning_count set seems a
little odd to me...

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

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

[Arend van Spriel]

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

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

[Aaron Lu]

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

[PATCH V2] 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.

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)
-- 
1.7.10.4

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

[Aaron Lu]

On 11/11/2013 05:49 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.
> 
> Signed-off-by: Arend van Spriel <arend@broadcom.com>

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

-Aaron

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

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

[Dong Aisheng]

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.

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

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

[Arend van Spriel]

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.

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
>

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

[Dong Aisheng]

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?

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
>>
>>
>
>

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

[Arend van Spriel]

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.

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
>>>
>>>
>>
>>
>

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

[Dong Aisheng]

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.
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
>>>>
>>>>
>>>>
>>>
>>>
>>
>
>

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

[Aaron Lu]

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
>