| Message ID | 20240815201542.421653-2-jm@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add retry tuning sequence | expand |
Hi all, On 8/15/24 3:15 PM, Judith Mendez wrote: > Add retry tuning up to 10 times if we fail to find > a failing region or no passing itapdly. This is > necessary since some eMMC's have been observed to never > find a failing itapdly on the first couple of tuning > iterations, but eventually do. It been observed that the > tuning algorithm does not need to loop more than 10 times > before finding a failing itapdly. Will fix-up this commit message as well, there has only been one case like this so far. ~ Judith
On 15/08/24 23:15, Judith Mendez wrote: > Add retry tuning up to 10 times if we fail to find > a failing region or no passing itapdly. This is > necessary since some eMMC's have been observed to never > find a failing itapdly on the first couple of tuning > iterations, but eventually do. It been observed that the > tuning algorithm does not need to loop more than 10 times > before finding a failing itapdly. > > Signed-off-by: Judith Mendez <jm@ti.com> > --- > drivers/mmc/host/sdhci_am654.c | 30 +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > index 64e10f7c9faa3..c3d485bd4d553 100644 > --- a/drivers/mmc/host/sdhci_am654.c > +++ b/drivers/mmc/host/sdhci_am654.c > @@ -86,6 +86,7 @@ > > #define CLOCK_TOO_SLOW_HZ 50000000 > #define SDHCI_AM654_AUTOSUSPEND_DELAY -1 > +#define RETRY_TUNING_MAX 10 > > /* Command Queue Host Controller Interface Base address */ > #define SDHCI_AM654_CQE_BASE_ADDR 0x200 > @@ -151,6 +152,7 @@ struct sdhci_am654_data { > u32 flags; > u32 quirks; > bool dll_enable; > + u32 tuning_loop; > > #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0) > }; > @@ -453,12 +455,14 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window > int prev_fail_end = -1; > u8 i; > > - if (!num_fails) > - return ITAPDLY_LAST_INDEX >> 1; > + if (!num_fails) { > + /* Retry tuning */ > + return -1; > + } > > if (fail_window->length == ITAPDLY_LENGTH) { > - dev_err(dev, "No passing ITAPDLY, return 0\n"); > - return 0; > + /* Retry tuning */ > + return -1; > } > > first_fail_start = fail_window->start; > @@ -504,6 +508,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host, > u8 curr_pass, itap; > u8 fail_index = 0; > u8 prev_pass = 1; > + int ret; > > memset(fail_window, 0, sizeof(fail_window)); > > @@ -532,10 +537,20 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host, > if (fail_window[fail_index].length != 0) > fail_index++; > > - itap = sdhci_am654_calculate_itap(host, fail_window, fail_index, > - sdhci_am654->dll_enable); > + ret = sdhci_am654_calculate_itap(host, fail_window, fail_index, > + sdhci_am654->dll_enable); > > - sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]); > + if (ret >= 0) { > + itap = ret; > + sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]); > + } else { > + if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) { > + sdhci_am654->tuning_loop++; > + sdhci_am654_platform_execute_tuning(host, opcode); The kernel uses very small stack size, so recursive function calls should not be used. It would be better to put the loop in a separate function, or add a retry: label and goto retry. > + } else { > + return -1; > + } > + } > > /* Save ITAPDLY */ > sdhci_am654->itap_del_sel[timing] = itap; > @@ -908,6 +923,7 @@ static int sdhci_am654_probe(struct platform_device *pdev) > goto err_pltfm_free; > } > > + sdhci_am654->tuning_loop = 0; So this is 10 retries ever, since sdhci_am654->tuning_loop is never set back to 0. Is that the intention? > host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning; > > pm_runtime_get_noresume(dev);
Hi Adrian, On 8/21/24 12:37 AM, Adrian Hunter wrote: > On 15/08/24 23:15, Judith Mendez wrote: >> Add retry tuning up to 10 times if we fail to find >> a failing region or no passing itapdly. This is >> necessary since some eMMC's have been observed to never >> find a failing itapdly on the first couple of tuning >> iterations, but eventually do. It been observed that the >> tuning algorithm does not need to loop more than 10 times >> before finding a failing itapdly. >> >> Signed-off-by: Judith Mendez <jm@ti.com> >> --- >> drivers/mmc/host/sdhci_am654.c | 30 +++++++++++++++++++++++------- >> 1 file changed, 23 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >> index 64e10f7c9faa3..c3d485bd4d553 100644 >> --- a/drivers/mmc/host/sdhci_am654.c >> +++ b/drivers/mmc/host/sdhci_am654.c >> @@ -86,6 +86,7 @@ >> >> #define CLOCK_TOO_SLOW_HZ 50000000 >> #define SDHCI_AM654_AUTOSUSPEND_DELAY -1 >> +#define RETRY_TUNING_MAX 10 >> >> /* Command Queue Host Controller Interface Base address */ >> #define SDHCI_AM654_CQE_BASE_ADDR 0x200 >> @@ -151,6 +152,7 @@ struct sdhci_am654_data { >> u32 flags; >> u32 quirks; >> bool dll_enable; >> + u32 tuning_loop; >> >> #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0) >> }; >> @@ -453,12 +455,14 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window >> int prev_fail_end = -1; >> u8 i; >> >> - if (!num_fails) >> - return ITAPDLY_LAST_INDEX >> 1; >> + if (!num_fails) { >> + /* Retry tuning */ >> + return -1; >> + } >> >> if (fail_window->length == ITAPDLY_LENGTH) { >> - dev_err(dev, "No passing ITAPDLY, return 0\n"); >> - return 0; >> + /* Retry tuning */ >> + return -1; >> } >> >> first_fail_start = fail_window->start; >> @@ -504,6 +508,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host, >> u8 curr_pass, itap; >> u8 fail_index = 0; >> u8 prev_pass = 1; >> + int ret; >> >> memset(fail_window, 0, sizeof(fail_window)); >> >> @@ -532,10 +537,20 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host, >> if (fail_window[fail_index].length != 0) >> fail_index++; >> >> - itap = sdhci_am654_calculate_itap(host, fail_window, fail_index, >> - sdhci_am654->dll_enable); >> + ret = sdhci_am654_calculate_itap(host, fail_window, fail_index, >> + sdhci_am654->dll_enable); >> >> - sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]); >> + if (ret >= 0) { >> + itap = ret; >> + sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]); >> + } else { >> + if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) { >> + sdhci_am654->tuning_loop++; >> + sdhci_am654_platform_execute_tuning(host, opcode); > > The kernel uses very small stack size, so recursive function calls > should not be used. It would be better to put the loop in a separate > function, or add a retry: label and goto retry. Ok, can change to this method, I was not sure of recursive function call but had opted for that since the code was to be simpler. > >> + } else { >> + return -1; >> + } >> + } >> >> /* Save ITAPDLY */ >> sdhci_am654->itap_del_sel[timing] = itap; >> @@ -908,6 +923,7 @@ static int sdhci_am654_probe(struct platform_device *pdev) >> goto err_pltfm_free; >> } >> >> + sdhci_am654->tuning_loop = 0; > > So this is 10 retries ever, since sdhci_am654->tuning_loop is never > set back to 0. Is that the intention? Yes, maximum of 10 re-tries. So far we have only seen issues during boot. ~ Judith > >> host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning; >> >> pm_runtime_get_noresume(dev); >
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c index 64e10f7c9faa3..c3d485bd4d553 100644 --- a/drivers/mmc/host/sdhci_am654.c +++ b/drivers/mmc/host/sdhci_am654.c @@ -86,6 +86,7 @@ #define CLOCK_TOO_SLOW_HZ 50000000 #define SDHCI_AM654_AUTOSUSPEND_DELAY -1 +#define RETRY_TUNING_MAX 10 /* Command Queue Host Controller Interface Base address */ #define SDHCI_AM654_CQE_BASE_ADDR 0x200 @@ -151,6 +152,7 @@ struct sdhci_am654_data { u32 flags; u32 quirks; bool dll_enable; + u32 tuning_loop; #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0) }; @@ -453,12 +455,14 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window int prev_fail_end = -1; u8 i; - if (!num_fails) - return ITAPDLY_LAST_INDEX >> 1; + if (!num_fails) { + /* Retry tuning */ + return -1; + } if (fail_window->length == ITAPDLY_LENGTH) { - dev_err(dev, "No passing ITAPDLY, return 0\n"); - return 0; + /* Retry tuning */ + return -1; } first_fail_start = fail_window->start; @@ -504,6 +508,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host, u8 curr_pass, itap; u8 fail_index = 0; u8 prev_pass = 1; + int ret; memset(fail_window, 0, sizeof(fail_window)); @@ -532,10 +537,20 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host, if (fail_window[fail_index].length != 0) fail_index++; - itap = sdhci_am654_calculate_itap(host, fail_window, fail_index, - sdhci_am654->dll_enable); + ret = sdhci_am654_calculate_itap(host, fail_window, fail_index, + sdhci_am654->dll_enable); - sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]); + if (ret >= 0) { + itap = ret; + sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]); + } else { + if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) { + sdhci_am654->tuning_loop++; + sdhci_am654_platform_execute_tuning(host, opcode); + } else { + return -1; + } + } /* Save ITAPDLY */ sdhci_am654->itap_del_sel[timing] = itap; @@ -908,6 +923,7 @@ static int sdhci_am654_probe(struct platform_device *pdev) goto err_pltfm_free; } + sdhci_am654->tuning_loop = 0; host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning; pm_runtime_get_noresume(dev);
Add retry tuning up to 10 times if we fail to find a failing region or no passing itapdly. This is necessary since some eMMC's have been observed to never find a failing itapdly on the first couple of tuning iterations, but eventually do. It been observed that the tuning algorithm does not need to loop more than 10 times before finding a failing itapdly. Signed-off-by: Judith Mendez <jm@ti.com> --- drivers/mmc/host/sdhci_am654.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-)