| Message ID | 20240308005746.1059813-2-jm@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add tuning algorithm for delay chain | expand |
On 8/03/24 02:57, Judith Mendez wrote: > Currently the sdhci_am654 driver only supports one tuning > algorithm which should be used only when DLL is enabled. The > ITAPDLY is selected from the largest passing window and the > buffer is viewed as a circular buffer. > > The new algorithm should be used when the delay chain > is enabled. The ITAPDLY is selected from the largest passing > window and the buffer is not viewed as a circular buffer. > > This implementation is based off of the following paper: [1]. > > Also add support for multiple failing windows. > > [1] https://www.ti.com/lit/an/spract9/spract9.pdf > > Fixes: 13ebeae68ac9 ("mmc: sdhci_am654: Add support for software tuning") > Signed-off-by: Judith Mendez <jm@ti.com> One question further below, and one cosmetic change, but resolve those and you may add to all patches in this patch set: Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > Changelog: > v2->v3: > - Fix return for tuning algorithm > - Fix ITAPDLY_LAST_INDEX > - Use reverse fir tree order for variable declarations > - Remove unnecessary parenthesis > --- > drivers/mmc/host/sdhci_am654.c | 112 +++++++++++++++++++++++++++------ > 1 file changed, 92 insertions(+), 20 deletions(-) > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > index d659c59422e1..d11b0d769e6c 100644 > --- a/drivers/mmc/host/sdhci_am654.c > +++ b/drivers/mmc/host/sdhci_am654.c > @@ -149,10 +149,17 @@ struct sdhci_am654_data { > int strb_sel; > u32 flags; > u32 quirks; > + bool dll_enable; > > #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0) > }; > > +struct window { > + u8 start; > + u8 end; > + u8 length; > +}; > + > struct sdhci_am654_driver_data { > const struct sdhci_pltfm_data *pdata; > u32 flags; > @@ -290,10 +297,12 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) > > regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val); > > - if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) > + if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { > sdhci_am654_setup_dll(host, clock); > - else > + sdhci_am654->dll_enable = true; > + } else { > sdhci_am654_setup_delay_chain(sdhci_am654, timing); V2 patch had here: sdhci_am654->dll_enable = false; Was its removal intended? > + } > > regmap_update_bits(sdhci_am654->base, PHY_CTRL5, CLKBUFSEL_MASK, > sdhci_am654->clkbuf_sel); > @@ -408,39 +417,102 @@ static u32 sdhci_am654_cqhci_irq(struct sdhci_host *host, u32 intmask) > return 0; > } > > -#define ITAP_MAX 32 > +#define ITAPDLY_LENGTH 32 > +#define ITAPDLY_LAST_INDEX (ITAPDLY_LENGTH - 1) > + > +static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window > + *fail_window, u8 num_fails, bool circular_buffer) > +{ > + u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0; > + u8 first_fail_start = 0, last_fail_end = 0; > + struct device *dev = mmc_dev(host->mmc); > + struct window pass_window = {0, 0, 0}; > + int prev_fail_end = -1; > + Unnecessary blank line > + u8 i; > + > + if (!num_fails) > + return ITAPDLY_LAST_INDEX >> 1; > + > + if (fail_window->length == ITAPDLY_LENGTH) { > + dev_err(dev, "No passing ITAPDLY, return 0\n"); > + return 0; > + } > + > + first_fail_start = fail_window->start; > + last_fail_end = fail_window[num_fails - 1].end; > + > + for (i = 0; i < num_fails; i++) { > + start_fail = fail_window[i].start; > + end_fail = fail_window[i].end; > + pass_length = start_fail - (prev_fail_end + 1); > + > + if (pass_length > pass_window.length) { > + pass_window.start = prev_fail_end + 1; > + pass_window.length = pass_length; > + } > + prev_fail_end = end_fail; > + } > + > + if (!circular_buffer) > + pass_length = ITAPDLY_LAST_INDEX - last_fail_end; > + else > + pass_length = ITAPDLY_LAST_INDEX - last_fail_end + first_fail_start; > + > + if (pass_length > pass_window.length) { > + pass_window.start = last_fail_end + 1; > + pass_window.length = pass_length; > + } > + > + if (!circular_buffer) > + itap = pass_window.start + (pass_window.length >> 1); > + else > + itap = (pass_window.start + (pass_window.length >> 1)) % ITAPDLY_LENGTH; > + > + return (itap > ITAPDLY_LAST_INDEX) ? ITAPDLY_LAST_INDEX >> 1 : itap; > +} > + > static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host, > u32 opcode) > { > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); > - int cur_val, prev_val = 1, fail_len = 0, pass_window = 0, pass_len; > - u32 itap; > + struct window fail_window[ITAPDLY_LENGTH]; > + u8 curr_pass, itap; > + u8 fail_index = 0; > + u8 prev_pass = 1; > + > + memset(fail_window, 0, sizeof(fail_window)); > > /* Enable ITAPDLY */ > regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPDLYENA_MASK, > 1 << ITAPDLYENA_SHIFT); > > - for (itap = 0; itap < ITAP_MAX; itap++) { > + for (itap = 0; itap < ITAPDLY_LENGTH; itap++) { > sdhci_am654_write_itapdly(sdhci_am654, itap); > > - cur_val = !mmc_send_tuning(host->mmc, opcode, NULL); > - if (cur_val && !prev_val) > - pass_window = itap; > + curr_pass = !mmc_send_tuning(host->mmc, opcode, NULL); > > - if (!cur_val) > - fail_len++; > + if (!curr_pass && prev_pass) > + fail_window[fail_index].start = itap; > > - prev_val = cur_val; > + if (!curr_pass) { > + fail_window[fail_index].end = itap; > + fail_window[fail_index].length++; > + } > + > + if (curr_pass && !prev_pass) > + fail_index++; > + > + prev_pass = curr_pass; > } > - /* > - * Having determined the length of the failing window and start of > - * the passing window calculate the length of the passing window and > - * set the final value halfway through it considering the range as a > - * circular buffer > - */ > - pass_len = ITAP_MAX - fail_len; > - itap = (pass_window + (pass_len >> 1)) % ITAP_MAX; > + > + if (fail_window[fail_index].length != 0) > + fail_index++; > + > + itap = sdhci_am654_calculate_itap(host, fail_window, fail_index, > + sdhci_am654->dll_enable); > + > sdhci_am654_write_itapdly(sdhci_am654, itap); > > return 0;
Hi Adrian, On 3/14/24 9:18 AM, Adrian Hunter wrote: > On 8/03/24 02:57, Judith Mendez wrote: >> Currently the sdhci_am654 driver only supports one tuning >> algorithm which should be used only when DLL is enabled. The >> ITAPDLY is selected from the largest passing window and the >> buffer is viewed as a circular buffer. >> >> The new algorithm should be used when the delay chain >> is enabled. The ITAPDLY is selected from the largest passing >> window and the buffer is not viewed as a circular buffer. >> >> This implementation is based off of the following paper: [1]. >> >> Also add support for multiple failing windows. >> >> [1] https://www.ti.com/lit/an/spract9/spract9.pdf >> >> Fixes: 13ebeae68ac9 ("mmc: sdhci_am654: Add support for software tuning") >> Signed-off-by: Judith Mendez <jm@ti.com> > > One question further below, and one cosmetic change, but resolve > those and you may add to all patches in this patch set: Appreciate your review, thanks. > > Acked-by: Adrian Hunter <adrian.hunter@intel.com> > >> --- >> Changelog: >> v2->v3: >> - Fix return for tuning algorithm >> - Fix ITAPDLY_LAST_INDEX >> - Use reverse fir tree order for variable declarations >> - Remove unnecessary parenthesis >> --- >> drivers/mmc/host/sdhci_am654.c | 112 +++++++++++++++++++++++++++------ >> 1 file changed, 92 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >> index d659c59422e1..d11b0d769e6c 100644 >> --- a/drivers/mmc/host/sdhci_am654.c >> +++ b/drivers/mmc/host/sdhci_am654.c >> @@ -149,10 +149,17 @@ struct sdhci_am654_data { >> int strb_sel; >> u32 flags; >> u32 quirks; >> + bool dll_enable; >> >> #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0) >> }; >> >> +struct window { >> + u8 start; >> + u8 end; >> + u8 length; >> +}; >> + >> struct sdhci_am654_driver_data { >> const struct sdhci_pltfm_data *pdata; >> u32 flags; >> @@ -290,10 +297,12 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) >> >> regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val); >> >> - if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) >> + if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { >> sdhci_am654_setup_dll(host, clock); >> - else >> + sdhci_am654->dll_enable = true; >> + } else { >> sdhci_am654_setup_delay_chain(sdhci_am654, timing); > > V2 patch had here: > > sdhci_am654->dll_enable = false; > > Was its removal intended? I did remove on purpose since it did not seem to be necessary. > >> + } >> >> regmap_update_bits(sdhci_am654->base, PHY_CTRL5, CLKBUFSEL_MASK, >> sdhci_am654->clkbuf_sel); >> @@ -408,39 +417,102 @@ static u32 sdhci_am654_cqhci_irq(struct sdhci_host *host, u32 intmask) >> return 0; >> } >> >> -#define ITAP_MAX 32 >> +#define ITAPDLY_LENGTH 32 >> +#define ITAPDLY_LAST_INDEX (ITAPDLY_LENGTH - 1) >> + >> +static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window >> + *fail_window, u8 num_fails, bool circular_buffer) >> +{ >> + u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0; >> + u8 first_fail_start = 0, last_fail_end = 0; >> + struct device *dev = mmc_dev(host->mmc); >> + struct window pass_window = {0, 0, 0}; >> + int prev_fail_end = -1; >> + > > Unnecessary blank line Will remove for v4. > >> + u8 i; >> + >> + if (!num_fails) >> + return ITAPDLY_LAST_INDEX >> 1; >> + >> + if (fail_window->length == ITAPDLY_LENGTH) { >> + dev_err(dev, "No passing ITAPDLY, return 0\n"); >> + return 0; >> + } >> + >> + first_fail_start = fail_window->start; >> + last_fail_end = fail_window[num_fails - 1].end; >> + >> + for (i = 0; i < num_fails; i++) { >> + start_fail = fail_window[i].start; >> + end_fail = fail_window[i].end; >> + pass_length = start_fail - (prev_fail_end + 1); >> + >> + if (pass_length > pass_window.length) { >> + pass_window.start = prev_fail_end + 1; >> + pass_window.length = pass_length; >> + } >> + prev_fail_end = end_fail; >> + } >> + >> + if (!circular_buffer) >> + pass_length = ITAPDLY_LAST_INDEX - last_fail_end; >> + else >> + pass_length = ITAPDLY_LAST_INDEX - last_fail_end + first_fail_start; >> + >> + if (pass_length > pass_window.length) { >> + pass_window.start = last_fail_end + 1; >> + pass_window.length = pass_length; >> + } >> + >> + if (!circular_buffer) >> + itap = pass_window.start + (pass_window.length >> 1); >> + else >> + itap = (pass_window.start + (pass_window.length >> 1)) % ITAPDLY_LENGTH; >> + >> + return (itap > ITAPDLY_LAST_INDEX) ? ITAPDLY_LAST_INDEX >> 1 : itap; >> +} >> + >> static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host, >> u32 opcode) >> { >> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); >> - int cur_val, prev_val = 1, fail_len = 0, pass_window = 0, pass_len; >> - u32 itap; >> + struct window fail_window[ITAPDLY_LENGTH]; >> + u8 curr_pass, itap; >> + u8 fail_index = 0; >> + u8 prev_pass = 1; >> + >> + memset(fail_window, 0, sizeof(fail_window)); >> >> /* Enable ITAPDLY */ >> regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPDLYENA_MASK, >> 1 << ITAPDLYENA_SHIFT); >> >> - for (itap = 0; itap < ITAP_MAX; itap++) { >> + for (itap = 0; itap < ITAPDLY_LENGTH; itap++) { >> sdhci_am654_write_itapdly(sdhci_am654, itap); >> >> - cur_val = !mmc_send_tuning(host->mmc, opcode, NULL); >> - if (cur_val && !prev_val) >> - pass_window = itap; >> + curr_pass = !mmc_send_tuning(host->mmc, opcode, NULL); >> >> - if (!cur_val) >> - fail_len++; >> + if (!curr_pass && prev_pass) >> + fail_window[fail_index].start = itap; >> >> - prev_val = cur_val; >> + if (!curr_pass) { >> + fail_window[fail_index].end = itap; >> + fail_window[fail_index].length++; >> + } >> + >> + if (curr_pass && !prev_pass) >> + fail_index++; >> + >> + prev_pass = curr_pass; >> } >> - /* >> - * Having determined the length of the failing window and start of >> - * the passing window calculate the length of the passing window and >> - * set the final value halfway through it considering the range as a >> - * circular buffer >> - */ >> - pass_len = ITAP_MAX - fail_len; >> - itap = (pass_window + (pass_len >> 1)) % ITAP_MAX; >> + >> + if (fail_window[fail_index].length != 0) >> + fail_index++; >> + >> + itap = sdhci_am654_calculate_itap(host, fail_window, fail_index, >> + sdhci_am654->dll_enable); >> + >> sdhci_am654_write_itapdly(sdhci_am654, itap); >> >> return 0; >
On 18/03/24 16:04, Judith Mendez wrote: > On 3/14/24 9:18 AM, Adrian Hunter wrote: >> On 8/03/24 02:57, Judith Mendez wrote: >>> @@ -290,10 +297,12 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) >>> regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val); >>> - if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) >>> + if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { >>> sdhci_am654_setup_dll(host, clock); >>> - else >>> + sdhci_am654->dll_enable = true; >>> + } else { >>> sdhci_am654_setup_delay_chain(sdhci_am654, timing); >> >> V2 patch had here: >> >> sdhci_am654->dll_enable = false; >> >> Was its removal intended? > > I did remove on purpose since it did not seem to be necessary. I suspect it is necessary because ->set_clock() can be called in when the timing has changed (e.g. recovery resets and reinitializes the card device, or the card changes etc.) but it seems like dll_enable would be stuck as always true once it is set to true.
On 3/19/24 1:35 AM, Adrian Hunter wrote: > On 18/03/24 16:04, Judith Mendez wrote: >> On 3/14/24 9:18 AM, Adrian Hunter wrote: >>> On 8/03/24 02:57, Judith Mendez wrote: >>>> @@ -290,10 +297,12 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) >>>> regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val); >>>> - if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) >>>> + if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { >>>> sdhci_am654_setup_dll(host, clock); >>>> - else >>>> + sdhci_am654->dll_enable = true; >>>> + } else { >>>> sdhci_am654_setup_delay_chain(sdhci_am654, timing); >>> >>> V2 patch had here: >>> >>> sdhci_am654->dll_enable = false; >>> >>> Was its removal intended? >> >> I did remove on purpose since it did not seem to be necessary. > > I suspect it is necessary because ->set_clock() can be called in > when the timing has changed (e.g. recovery resets and reinitializes > the card device, or the card changes etc.) but it seems like > dll_enable would be stuck as always true once it is set to true. > Thinking about this some more, you are right. Will add back, thanks. ~ Judith
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c index d659c59422e1..d11b0d769e6c 100644 --- a/drivers/mmc/host/sdhci_am654.c +++ b/drivers/mmc/host/sdhci_am654.c @@ -149,10 +149,17 @@ struct sdhci_am654_data { int strb_sel; u32 flags; u32 quirks; + bool dll_enable; #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0) }; +struct window { + u8 start; + u8 end; + u8 length; +}; + struct sdhci_am654_driver_data { const struct sdhci_pltfm_data *pdata; u32 flags; @@ -290,10 +297,12 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock) regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val); - if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) + if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) { sdhci_am654_setup_dll(host, clock); - else + sdhci_am654->dll_enable = true; + } else { sdhci_am654_setup_delay_chain(sdhci_am654, timing); + } regmap_update_bits(sdhci_am654->base, PHY_CTRL5, CLKBUFSEL_MASK, sdhci_am654->clkbuf_sel); @@ -408,39 +417,102 @@ static u32 sdhci_am654_cqhci_irq(struct sdhci_host *host, u32 intmask) return 0; } -#define ITAP_MAX 32 +#define ITAPDLY_LENGTH 32 +#define ITAPDLY_LAST_INDEX (ITAPDLY_LENGTH - 1) + +static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window + *fail_window, u8 num_fails, bool circular_buffer) +{ + u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0; + u8 first_fail_start = 0, last_fail_end = 0; + struct device *dev = mmc_dev(host->mmc); + struct window pass_window = {0, 0, 0}; + int prev_fail_end = -1; + + u8 i; + + if (!num_fails) + return ITAPDLY_LAST_INDEX >> 1; + + if (fail_window->length == ITAPDLY_LENGTH) { + dev_err(dev, "No passing ITAPDLY, return 0\n"); + return 0; + } + + first_fail_start = fail_window->start; + last_fail_end = fail_window[num_fails - 1].end; + + for (i = 0; i < num_fails; i++) { + start_fail = fail_window[i].start; + end_fail = fail_window[i].end; + pass_length = start_fail - (prev_fail_end + 1); + + if (pass_length > pass_window.length) { + pass_window.start = prev_fail_end + 1; + pass_window.length = pass_length; + } + prev_fail_end = end_fail; + } + + if (!circular_buffer) + pass_length = ITAPDLY_LAST_INDEX - last_fail_end; + else + pass_length = ITAPDLY_LAST_INDEX - last_fail_end + first_fail_start; + + if (pass_length > pass_window.length) { + pass_window.start = last_fail_end + 1; + pass_window.length = pass_length; + } + + if (!circular_buffer) + itap = pass_window.start + (pass_window.length >> 1); + else + itap = (pass_window.start + (pass_window.length >> 1)) % ITAPDLY_LENGTH; + + return (itap > ITAPDLY_LAST_INDEX) ? ITAPDLY_LAST_INDEX >> 1 : itap; +} + static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host, u32 opcode) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); - int cur_val, prev_val = 1, fail_len = 0, pass_window = 0, pass_len; - u32 itap; + struct window fail_window[ITAPDLY_LENGTH]; + u8 curr_pass, itap; + u8 fail_index = 0; + u8 prev_pass = 1; + + memset(fail_window, 0, sizeof(fail_window)); /* Enable ITAPDLY */ regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPDLYENA_MASK, 1 << ITAPDLYENA_SHIFT); - for (itap = 0; itap < ITAP_MAX; itap++) { + for (itap = 0; itap < ITAPDLY_LENGTH; itap++) { sdhci_am654_write_itapdly(sdhci_am654, itap); - cur_val = !mmc_send_tuning(host->mmc, opcode, NULL); - if (cur_val && !prev_val) - pass_window = itap; + curr_pass = !mmc_send_tuning(host->mmc, opcode, NULL); - if (!cur_val) - fail_len++; + if (!curr_pass && prev_pass) + fail_window[fail_index].start = itap; - prev_val = cur_val; + if (!curr_pass) { + fail_window[fail_index].end = itap; + fail_window[fail_index].length++; + } + + if (curr_pass && !prev_pass) + fail_index++; + + prev_pass = curr_pass; } - /* - * Having determined the length of the failing window and start of - * the passing window calculate the length of the passing window and - * set the final value halfway through it considering the range as a - * circular buffer - */ - pass_len = ITAP_MAX - fail_len; - itap = (pass_window + (pass_len >> 1)) % ITAP_MAX; + + if (fail_window[fail_index].length != 0) + fail_index++; + + itap = sdhci_am654_calculate_itap(host, fail_window, fail_index, + sdhci_am654->dll_enable); + sdhci_am654_write_itapdly(sdhci_am654, itap); return 0;
Currently the sdhci_am654 driver only supports one tuning algorithm which should be used only when DLL is enabled. The ITAPDLY is selected from the largest passing window and the buffer is viewed as a circular buffer. The new algorithm should be used when the delay chain is enabled. The ITAPDLY is selected from the largest passing window and the buffer is not viewed as a circular buffer. This implementation is based off of the following paper: [1]. Also add support for multiple failing windows. [1] https://www.ti.com/lit/an/spract9/spract9.pdf Fixes: 13ebeae68ac9 ("mmc: sdhci_am654: Add support for software tuning") Signed-off-by: Judith Mendez <jm@ti.com> --- Changelog: v2->v3: - Fix return for tuning algorithm - Fix ITAPDLY_LAST_INDEX - Use reverse fir tree order for variable declarations - Remove unnecessary parenthesis --- drivers/mmc/host/sdhci_am654.c | 112 +++++++++++++++++++++++++++------ 1 file changed, 92 insertions(+), 20 deletions(-)