| Message ID | CAAGaQKBUuYfhq-shicUUtdHuEvZd1VqV0gCgpK31x=FkwEjqvw@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, Jul 25, 2016 at 4:23 PM, Steve Rae <steve.rae@raedomain.com> wrote: > Hi Jaehoon, > > On Mon, Jul 25, 2016 at 3:21 AM, Lukasz Majewski <l.majewski@samsung.com> wrote: >> Hi Jaehoon, >> >>> Hi All, >>> >>> On 07/12/2016 09:55 PM, Lukasz Majewski wrote: >>> > Hi Jaehoon, >>> > >>> >> There is no data, it doesn't needs to wait for completing data >>> >> transfer. (It seems that it can be removed.) >>> >> Almost all timeout error is occured from stop command without data. >>> >> After applied this patch, I hope that we don't need to increase >>> >> timeout value anymore. >>> > >>> > This patch fixes issue (in a better way) for Odroid U3 write >>> > performance (http://patchwork.ozlabs.org/patch/646932/) and hence >>> > should be used. >>> > >>> >>> Is there any other opinion for this patch? >>> Who is maintaining u-boot-mmc? Is Pantelis still maintaining >>> u-boot-mmc? >>> >>> Who can apply this patch and patch relevant to mmc? >> >> To be honest, I do look forward to see this patch included to u-boot >> master branch. >> >> Addition of some extra Odroid U3 tests hinge on it. >> >>> >>> Best Regards, >>> Jaehoon Chung >>> >>> > >>> >> >>> >> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >>> >> --- >>> >> drivers/mmc/sdhci.c | 3 ++- >>> >> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >> >>> >> 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; >>> >> >>> > >>> > Acked-by: Lukasz Majewski <l.majewski@samsung.com> >>> > Tested-by: Lukasz Majewski <l.majewski@samsung.com> >>> > >>> > Test HW: Odroid U3 (Exynos4): >>> > >>> > Odroid # ext4load mmc 0:2 0x41000000 dat_31M.img >>> > 32505856 bytes read in 1471 ms (21.1 MiB/s) >>> > Odroid # ext4write mmc 0:2 0x41000000 /dat_w55.img 0x1f00000 >>> > File System is consistent >>> > update journal finished >>> > 32505856 bytes written in 3528 ms (8.8 MiB/s) >>> > >>> > Performance improvement is even better than with previously proposed >>> > patches. >>> > >>> > Test HW: Odroid XU3 (Exynos5): >>> > >>> > ODROID-XU3 # ext4load mmc 0:2 0x41000000 dat_31M.img >>> > 32505856 bytes read in 6309 ms (4.9 MiB/s) >>> > ODROID-XU3 # ext4write mmc 0:2 0x41000000 /dat_w1.img 0x1f00000 >>> > File System is consistent >>> > update journal finished >>> > 32505856 bytes written in 4884 ms (6.3 MiB/s) >>> > >>> > >>> >> >> >> >> -- >> Best regards, >> >> Lukasz Majewski >> >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group > > with this change, I can also set the following back to 100: > > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c > index de8d8ea..d593dc6 100644 > --- a/drivers/mmc/sdhci.c > +++ b/drivers/mmc/sdhci.c > @@ -128,7 +128,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 SDHCI_READ_STATUS_TIMEOUT 100 > > #ifdef CONFIG_DM_MMC_OPS > static int sdhci_send_command(struct udevice *dev, struct mmc_cmd *cmd, > > And it still works on my board ! Thanks ! > > Tested-by: Steve Rae <steve.rae@raedomain.com> [Test HW: bcm235xx board] patchworks added a new entry; so try again: Tested-by: Steve Rae <steve.rae@raedomain.com> [Test HW: bcm235xx board]
Hi Steve, > with this change, I can also set the following back to 100: > > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c > index de8d8ea..d593dc6 100644 > --- a/drivers/mmc/sdhci.c > +++ b/drivers/mmc/sdhci.c > @@ -128,7 +128,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 SDHCI_READ_STATUS_TIMEOUT 100 > > #ifdef CONFIG_DM_MMC_OPS > static int sdhci_send_command(struct udevice *dev, struct mmc_cmd > *cmd, > > And it still works on my board ! Thanks ! Could you prepare proper revert patch?
HI Lukasz, On Tue, Jul 26, 2016 at 2:13 AM, Lukasz Majewski <l.majewski@samsung.com> wrote: > Hi Steve, > >> with this change, I can also set the following back to 100: >> >> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c >> index de8d8ea..d593dc6 100644 >> --- a/drivers/mmc/sdhci.c >> +++ b/drivers/mmc/sdhci.c >> @@ -128,7 +128,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 SDHCI_READ_STATUS_TIMEOUT 100 >> >> #ifdef CONFIG_DM_MMC_OPS >> static int sdhci_send_command(struct udevice *dev, struct mmc_cmd >> *cmd, >> >> And it still works on my board ! Thanks ! > > Could you prepare proper revert patch? > > -- > Best regards, > > Lukasz Majewski > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group Looking at the code, I don't think there is any value changing the SDHCI_READ_STATUS_TIMEOUT from 1000 to 100. But maybe someone (Jaehoon ?) could comment on the impact of this SDHCI_READ_STATUS_TIMEOUT value in the SDHCI_QUIRK_BROKEN_R1B case... Does it affect performance in anyway? If it does, the I'll prepare a patch.... Thanks, Steve
Hi On 07/27/2016 01:10 AM, Steve Rae wrote: > HI Lukasz, > > On Tue, Jul 26, 2016 at 2:13 AM, Lukasz Majewski <l.majewski@samsung.com> wrote: >> Hi Steve, >> >>> with this change, I can also set the following back to 100: >>> >>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c >>> index de8d8ea..d593dc6 100644 >>> --- a/drivers/mmc/sdhci.c >>> +++ b/drivers/mmc/sdhci.c >>> @@ -128,7 +128,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 SDHCI_READ_STATUS_TIMEOUT 100 >>> >>> #ifdef CONFIG_DM_MMC_OPS >>> static int sdhci_send_command(struct udevice *dev, struct mmc_cmd >>> *cmd, >>> >>> And it still works on my board ! Thanks ! >> >> Could you prepare proper revert patch? >> >> -- >> Best regards, >> >> Lukasz Majewski >> >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group > > Looking at the code, I don't think there is any value changing the > SDHCI_READ_STATUS_TIMEOUT from 1000 to 100. > But maybe someone (Jaehoon ?) could comment on the impact of this > SDHCI_READ_STATUS_TIMEOUT value in the SDHCI_QUIRK_BROKEN_R1B case... > Does it affect performance in anyway? There is no performance effect whatever SDHCI_READ_STATUS_TIMEOUT is using value. So i think that we don't need to revert it.. SDHCI_QUIRK_BROKEN_R1B had been added from me. At that time, i didn't know why added SDHCI_INT_DAT_END. I'm not sure but if remove the SDHCI_INT_DAT_END, I guess that SDHCI_QUIRK_BROKEN_R1B can be also removed. (quirks is workaround flags, so if it can be removed, it's best.) In future, I want to remove SDHCI_QUIRK_BROKEN_R1B. Best Regards, Jaehoon Chung > If it does, the I'll prepare a patch.... > > Thanks, Steve > > >
diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c index de8d8ea..d593dc6 100644 --- a/drivers/mmc/sdhci.c +++ b/drivers/mmc/sdhci.c @@ -128,7 +128,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 SDHCI_READ_STATUS_TIMEOUT 100 #ifdef CONFIG_DM_MMC_OPS