[U-Boot] [PATCH 1/3] COSMETIC: mmc: sdhci: Add CONFIG_ prefix to SDHCI_READ_STATUS_TIMEOUT

[U-Boot] [PATCH 1/3] COSMETIC: mmc: sdhci: Add CONFIG_ prefix to SDHCI_READ_STATUS_TIMEOUT

[Lukasz Majewski]

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 {
-- 
2.0.0.rc2

[U-Boot] [PATCH 2/3] FIX: mmc: sdhci: Board specific definitions for SDHCI CMD and READ TIMEOUTS

[Lukasz Majewski]

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)
-- 
2.0.0.rc2

[U-Boot] [PATCH 3/3] FIX: mmc: sdhci: exynos4: Define smaller SDHCI timeouts for Exynos4 based boards

[Lukasz Majewski]

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

[U-Boot] [PATCH 2/3] FIX: mmc: sdhci: Board specific definitions for SDHCI CMD and READ TIMEOUTS

[Jaehoon Chung]

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

[U-Boot] [PATCH 1/3] COSMETIC: mmc: sdhci: Add CONFIG_ prefix to SDHCI_READ_STATUS_TIMEOUT

[Tom Rini]

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!

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

[U-Boot] [PATCH 1/3] COSMETIC: mmc: sdhci: Add CONFIG_ prefix to SDHCI_READ_STATUS_TIMEOUT

[Lukasz Majewski]

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.

-- 
Best regards,

Lukasz Majewski

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