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

PATCH Arend van Spriel 4 participants 7 messages Feb 21, 2014 – Mar 25, 2014

Subsystems (2)

MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Arend van Spriel

author

2014-02-21 19:59 UTC #0 lore raw

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 <[email protected]>

Cc Aaron Lu <[email protected]>

Signed-off-by Arend van Spriel <[email protected]>

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

drivers/mmc/host/sdhci.c +3 -4

index 9ddef47..d5b421d 100644
@@ -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);
 	}
 
 	/*

Signature · 1 line

1.8.1.3

Arend van Spriel

author

2014-03-06 09:29 UTC #1 lore raw

Quoted 17 lines · 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.

Quoted 7 lines

Cc Dong Aisheng <[email protected]>
Cc Aaron Lu <[email protected]>
Signed-off-by Arend van Spriel <[email protected]>
---
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

Quoted 34 lines

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

drivers/mmc/host/sdhci.c +3 -4

index 9ddef47..d5b421d 100644
@@ -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);
 	}
 
 	/*

Ulf Hansson

Acked-by

2014-03-07 04:06 UTC #2 lore raw

Quoted 63 lines · Arend van Spriel

Quoted 17 lines · 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.
Quoted 7 lines
Cc Dong Aisheng <[email protected]>
Cc Aaron Lu <[email protected]>
Signed-off-by Arend van Spriel <[email protected]>
---
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
Quoted 32 lines
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(-)
drivers/mmc/host/sdhci.c +3 -4
index 9ddef47..d5b421d 100644
@@ -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);
      }
 

Quoted 3 lines

drivers/mmc/host/sdhci.c out: · line 2036
Quoted 2 lines
drivers/mmc/host/sdhci.c +0 -0
      /*

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

Acked-by Ulf Hansson <[email protected]>

Arend van Spriel

author

2014-03-18 21:02 UTC #3 lore raw

Quoted 27 lines · Ulf Hansson

Quoted 26 lines · Arend van Spriel
Quoted 24 lines · 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<[email protected]>
Cc Aaron Lu<[email protected]>
Signed-off-by Arend van Spriel<[email protected]>
---
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.

Quoted 1 line

Quoted 1 line
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

Quoted 43 lines

Quoted 35 lines

Regards,
Arend

Quoted 32 lines

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

drivers/mmc/host/sdhci.c +3 -4

index 9ddef47..d5b421d 100644
@@ -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);
       }

Quoted 3 lines

drivers/mmc/host/sdhci.c out: · line 2036
Quoted 2 lines
drivers/mmc/host/sdhci.c +0 -0
       /*

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

Acked-by Ulf Hansson<[email protected]>

Aaron Lu

Reviewed-by

2014-03-20 05:32 UTC #4 lore raw

In reply to Arend van Spriel

Quoted 13 lines · 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.

Quoted 7 lines

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 <[email protected]>
Cc Aaron Lu <[email protected]>
Signed-off-by Arend van Spriel <[email protected]>

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

Reviewed-by Aaron Lu <[email protected]>

Arend van Spriel

author

2014-03-25 20:49 UTC #5 lore raw

Quoted 18 lines · Aaron Lu

Quoted 17 lines · 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.

Quoted 8 lines

Quoted 3 lines
Cc Dong Aisheng<[email protected]>
Cc Aaron Lu<[email protected]>
Signed-off-by Arend van Spriel<[email protected]>
In addition to solve your problem, this patch also makes sense in the common case, so:
Reviewed-by Aaron Lu<[email protected]>

Hi Chris,

Is this patch still in your queue?

Regards,
Arend

Chris Ball

2014-03-25 20:56 UTC #6 lore raw

Hi Arend,

Quoted 23 lines · Arend van Spriel

Quoted 22 lines · Aaron Lu
Quoted 20 lines · 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<[email protected]>
Cc Aaron Lu<[email protected]>
Signed-off-by Arend van Spriel<[email protected]>

Quoted 8 lines

Quoted 4 lines
In addition to solve your problem, this patch also makes sense in the common case, so:
Reviewed-by Aaron Lu<[email protected]>
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.

Signature · 1 line

Chris Ball   <[email protected]>   <http://printf.net/>