[U-Boot] [PATCH 1/3] COSMETIC: mmc: sdhci: Add CONFIG_ prefix to SDHCI_READ_STATUS_TIMEOUT
This change gives common prefix for SDHCI_READ_STATUS_TIMEOUT.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com --- drivers/mmc/sdhci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index 604f18d..aa4cd4f 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -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 {
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.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com --- drivers/mmc/sdhci.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index aa4cd4f..9f38ecb 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -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)
Hi Lukasz,
On 07/11/2016 09:49 PM, Lukasz Majewski wrote:
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.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com
drivers/mmc/sdhci.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index aa4cd4f..9f38ecb 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -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)
I have checked this..it's right. how about the below code? I just applied the below patch..then i saw the similar performance.
--- a/drivers/mmc/s5p_sdhci.c +++ b/drivers/mmc/s5p_sdhci.c @@ -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); diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index 604f18d..0a38a56 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -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;
I think it doesn't needs to set SDHCI_INT_DATA_END.(I think that it can be just removed.)
I have tested with this on Exynos4 boards. It's working fine for eMMC/SD. Could you check this patch?
Best Regards, Jaehoon Chung
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.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com --- include/configs/exynos4-common.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/configs/exynos4-common.h b/include/configs/exynos4-common.h index fbe0fa9..9cf2d2d 100644 --- a/include/configs/exynos4-common.h +++ b/include/configs/exynos4-common.h @@ -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
On Mon, Jul 11, 2016 at 02:49:03PM +0200, Lukasz Majewski wrote:
This change gives common prefix for SDHCI_READ_STATUS_TIMEOUT.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com
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!
Hi Tom,
On Mon, Jul 11, 2016 at 02:49:03PM +0200, Lukasz Majewski wrote:
This change gives common prefix for SDHCI_READ_STATUS_TIMEOUT.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com
Hang on, didn't we just intentionally not CONFIG_ this option and not add it to Kconfig?
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...
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!
BTW: I'm testing Jeahoon's patches for fixing this issue, so probably this patch series could be dropped.
participants (3)
-
Jaehoon Chung -
Lukasz Majewski -
Tom Rini