| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d58f1501-ea64-4964-ac32-a3604178dc6f@ti.com>
Date: Wed, 31 Jan 2024 16:34:01 +0530
From: Vignesh Raghavendra <vigneshr@...com>
To: Judith Mendez <jm@...com>, Ulf Hansson <ulf.hansson@...aro.org>,
Rob
Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski
<krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>
CC: Adrian Hunter <adrian.hunter@...el.com>, <linux-mmc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Nishanth Menon <nm@...com>,
Andrew Davis
<afd@...com>, Udit Kumar <u-kumar1@...com>,
Roger Quadros
<rogerq@...nel.org>, <devicetree@...r.kernel.org>,
Randolph Sapp <rs@...com>
Subject: Re: [RFC PATCH 01/13] drivers: mmc: host: sdhci_am654: Add tuning
algorithm for delay chain
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@...com>
> ---
$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
Powered by blists - more mailing lists