COSMETIC: mmc: sdhci: Add CONFIG_ prefix to SDHCI_READ_STATUS_TIMEOUT

This change gives common prefix for SDHCI_READ_STATUS_TIMEOUT.

---
 drivers/mmc/sdhci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 604f18d..aa4cd4f 100644
@@ -127,7 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data,
 #define CONFIG_SDHCI_CMD_MAX_TIMEOUT		3200
 #endif
 #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT	100
-#define SDHCI_READ_STATUS_TIMEOUT		1000
+#define CONFIG_SDHCI_READ_STATUS_TIMEOUT	1000
 
 static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 		       struct mmc_data *data)

@@ -244,9 +244,9 @@ static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 		if (stat & SDHCI_INT_ERROR)
 			break;
 	} while (((stat & mask) != mask) &&
-		 (get_timer(start) < SDHCI_READ_STATUS_TIMEOUT));
+		 (get_timer(start) < CONFIG_SDHCI_READ_STATUS_TIMEOUT));
 
-	if (get_timer(start) >= SDHCI_READ_STATUS_TIMEOUT) {
+	if (get_timer(start) >= CONFIG_SDHCI_READ_STATUS_TIMEOUT) {
 		if (host->quirks & SDHCI_QUIRK_BROKEN_R1B)
 			return 0;
 		else {

2.0.0.rc2

For some boards - e.g. odroid u3, it is necessary to adjust manually those two timeouts.

Exynos4 based boards, which use SDHCI controller to read data from SD cards, have SDHCI_QUIRK_BROKEN_R1B flag set. This quirk requires short timeout values, since in fact it relies on timeout exit, because the controller is not able to read status bit properly for this kind of response.

Change: 29905a451b7ecf86785a4404e926fb14a8daced3, introduced longer timeouts for boards with SDHCI_QUIRK_BROKEN_R1B, which resulted in write speed regression (to ext4 fs via DFU):

Before (31 MiB test file):
32505856 bytes written in 4342 ms (7.1 MiB/s)

After this change:
32505856 bytes written in 20466 ms (1.5 MiB/s)

Such performance regression caused timeouts during DFU write (observed at HWT test setup).

This commit, however introduces possibility to define different timeout values for SDHCI mmc IP blocks embedded in different SoCs (and boards).

This problem was observed only in SDHCI controller, when target board was running solely from SD card.

---
 drivers/mmc/sdhci.c | 4 ++++
 1 file changed, 4 insertions(+)

index aa4cd4f..9f38ecb 100644
@@ -126,8 +126,12 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data,
 #ifndef CONFIG_SDHCI_CMD_MAX_TIMEOUT
 #define CONFIG_SDHCI_CMD_MAX_TIMEOUT		3200
 #endif
+#ifndef CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT
 #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT	100
+#endif
+#ifndef CONFIG_SDHCI_READ_STATUS_TIMEOUT
 #define CONFIG_SDHCI_READ_STATUS_TIMEOUT	1000
+#endif
 
 static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 		       struct mmc_data *data)

2.0.0.rc2

Hi Lukasz,

@@ -68,7 +68,7 @@ static int s5p_sdhci_core_init(struct sdhci_host *host)
        host->name = S5P_NAME;
 
        host->quirks = SDHCI_QUIRK_NO_HISPD_BIT | SDHCI_QUIRK_BROKEN_VOLTAGE |
-               SDHCI_QUIRK_BROKEN_R1B | SDHCI_QUIRK_32BIT_DMA_ADDR |
+               SDHCI_QUIRK_32BIT_DMA_ADDR |
                SDHCI_QUIRK_WAIT_SEND_CMD | SDHCI_QUIRK_USE_WIDE8;
        host->voltages = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195;
        host->version = sdhci_readw(host, SDHCI_HOST_VERSION);

index 604f18d..0a38a56 100644
@@ -175,7 +175,8 @@ static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
                flags = SDHCI_CMD_RESP_LONG;
        else if (cmd->resp_type & MMC_RSP_BUSY) {
                flags = SDHCI_CMD_RESP_SHORT_BUSY;
-               mask |= SDHCI_INT_DATA_END;
+               if (data)
+                       mask |= SDHCI_INT_DATA_END;
        } else
                flags = SDHCI_CMD_RESP_SHORT;
 

Those values are necessary to provide SDHCI controller SD card write speed comparable to those before introducing the commit:
29905a451b7ecf86785a4404e926fb14a8daced3.

Such adjustments are required on boards with SDHCI's SDHCI_QUIRK_BROKEN_R1B quirk flag set.

---
 include/configs/exynos4-common.h | 2 ++
 1 file changed, 2 insertions(+)

index fbe0fa9..9cf2d2d 100644
@@ -21,6 +21,8 @@
 /* SD/MMC configuration */
 #define CONFIG_MMC_SDMA
 #define CONFIG_MMC_DEFAULT_DEV	0
+#define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT 10
+#define CONFIG_SDHCI_READ_STATUS_TIMEOUT 5
 
 #undef CONFIG_CMD_ONENAND
 #undef CONFIG_CMD_MTDPARTS

2.0.0.rc2

Hang on, didn't we just intentionally not CONFIG_ this option and not add it to Kconfig? If we're making these tunable they need to be in Kconfig and if needed, non-asked questions, ie:
int SDHCI_READ_STATUS_TIMEOUT
default 100 if FOO_PLATFORM
default 500 if BAR_PLATFORM
default 1000

Thanks!

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160712/b4e8637f/attachment.sig>

Hi Tom,

Apparently, I was not aware of such intentions :-).

I do agree that we should add new options to Kconfig, so with second thoughts I think that this patch was not so good idea...

BTW: I'm testing Jeahoon's patches for fixing this issue, so probably this patch series could be dropped.

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group