| Message ID | 20240131003714.2779593-2-jm@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add tuning algorithm for delay chain | expand |
On 1/31/24 5:04 AM, Vignesh Raghavendra wrote: > 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@ti.com> >> --- > > > $subject prefix should be > > mmc: sdhci_am654: > > See git log --oneline drivers/mmc/host/sdhci_am654.c for precedence. Thanks, will add in v1. > >> 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 Will add. > >> +{ >> + 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() Good idea, will update itap value here then. > >> + return 0; > > Should this be an error? It is an error, but I thought it was a good idea to return 0 in case all ITAPDLY values fail and report this via dev_warn or dev_error. For now I can use dev_error. > >> + } else { >> + for (int i = 0; i < num_fails; i++) { > > > please avoid inline declaration to align with rest of the file. Will do. > >> + 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); Great, will add this, thanks. > >> >> /* 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. I will fix this. (: > >> + >> + itap = calculate_itap(host, &fail_window[0], fail_index, >> + (sdhci_am654->dll_enable ? true : false)); >> + >> sdhci_am654_write_itapdly(sdhci_am654, itap); >> >> return 0; >
On 31/01/2024 02:37, Judith Mendez wrote: > Currently the sdhci_am654 driver only supports one tuning > algorithm which should be used only when DLL is enabled. The What does DLL stand for? > 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@ti.com> > --- > 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) > +{ > + 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"); This is a helper function, instead of printing a warning here, you should return an error value, and allow the caller to decide what to print and if this is serious enough to bail out. > + return 0; > + } else { > + for (int i = 0; i < num_fails; i++) { > + 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])); > > /* 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 = calculate_itap(host, &fail_window[0], fail_index, > + (sdhci_am654->dll_enable ? true : false)); > + If calculate_itap fails, you should return error so caller can decide what to do. > sdhci_am654_write_itapdly(sdhci_am654, itap); > > return 0;
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) +{ + 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"); + return 0; + } else { + for (int i = 0; i < num_fails; i++) { + 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])); /* 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 = calculate_itap(host, &fail_window[0], fail_index, + (sdhci_am654->dll_enable ? true : false)); + 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 Signed-off-by: Judith Mendez <jm@ti.com> --- drivers/mmc/host/sdhci_am654.c | 128 +++++++++++++++++++++++++++------ 1 file changed, 108 insertions(+), 20 deletions(-)