Hi Judith, On 31/01/24 06:07, 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 > > Signed-off-by: Judith Mendez <jm@xxxxxx> > --- $subject prefix should be mmc: sdhci_am654: See git log --oneline drivers/mmc/host/sdhci_am654.c for precedence. > drivers/mmc/host/sdhci_am654.c | 128 +++++++++++++++++++++++++++------ > 1 file changed, 108 insertions(+), 20 deletions(-) > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > index d659c59422e1..59d205511312 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,13 @@ 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); > + sdhci_am654->dll_enable = false; > + } > > regmap_update_bits(sdhci_am654->base, PHY_CTRL5, CLKBUFSEL_MASK, > sdhci_am654->clkbuf_sel); > @@ -408,39 +418,117 @@ 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 31 > +static u32 calculate_itap(struct sdhci_host *host, struct window *fail_window, > + u8 num_fails, bool circular_buffer) s/calculate_itap/sdhci_am654_calculate_itap/ to keep function naming consistent within the file > +{ > + struct device *dev = mmc_dev(host->mmc); > + struct window pass_window, first_fail, last_fail; > + u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0; > + int prev_end_fail = -1; > + > + memset(&pass_window, 0, sizeof(pass_window)); > + memset(&first_fail, 0, sizeof(first_fail)); > + memset(&last_fail, 0, sizeof(last_fail)); > + > + if (!num_fails) { > + return ITAPDLY_LAST_INDEX >> 1; > + } else if (fail_window->length == ITAPDLY_LENGTH) { > + dev_warn(dev, "No passing ITAPDLY, return 0\n"); Drop return 0 in dev_warn() > + return 0; Should this be an error? > + } else { > + for (int i = 0; i < num_fails; i++) { please avoid inline declaration to align with rest of the file. > + start_fail = fail_window[i].start; > + end_fail = fail_window[i].end; > + > + if (i == 0) { > + first_fail.start = start_fail; > + first_fail.end = end_fail; > + first_fail.length = fail_window[0].length; > + } > + > + if (i == num_fails - 1) { > + last_fail.start = start_fail; > + last_fail.end = end_fail; > + last_fail.length = fail_window[i].length; > + } > + > + pass_length = start_fail - (prev_end_fail + 1); > + if (pass_length > pass_window.length) { > + pass_window.start = prev_end_fail + 1; > + pass_window.length = pass_length; > + } > + prev_end_fail = end_fail; > + } > + > + if (!circular_buffer) { > + if (ITAPDLY_LAST_INDEX - end_fail > pass_window.length) { > + pass_window.start = end_fail + 1; > + pass_window.length = ITAPDLY_LAST_INDEX - end_fail; > + } > + } else { > + pass_length = ITAPDLY_LAST_INDEX - end_fail + 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; > + > + if (itap < 0 || itap > ITAPDLY_LAST_INDEX) > + itap = 0; > + > + return 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 prev_pass = 1; > + u8 fail_index = 0; > + u8 curr_pass, itap, i; > + > + for (i = 0; i < ITAPDLY_LENGTH; i++) > + memset(&fail_window[i], 0, sizeof(fail_window[0])); Don't need for() loop, just memset entire array at once. Something like: memset(fail_window, 0, sizeof(fail_window[0]) * ITAPDLY_LENGTH); > > /* 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++; These is some tab vs space mangling here. > + > + itap = calculate_itap(host, &fail_window[0], fail_index, > + (sdhci_am654->dll_enable ? true : false)); > + > sdhci_am654_write_itapdly(sdhci_am654, itap); > > return 0; -- Regards Vignesh
