| 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
| ||
|
Date: Thu, 1 Feb 2024 13:46:43 -0600
From: Andrew Davis <afd@...com>
To: Judith Mendez <jm@...com>, Ulf Hansson <ulf.hansson@...aro.org>
CC: Adrian Hunter <adrian.hunter@...el.com>, <linux-mmc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Randolph Sapp <rs@...com>,
Vignesh
Raghavendra <vigneshr@...com>
Subject: Re: [PATCH v1 3/5] mmc: sdhci_am654: Add missing OTAP/ITAP enable
On 1/31/24 3:50 PM, Judith Mendez wrote:
> Currently the OTAP/ITAP delay enable functionality is missing in
> the am654_set_clock function which is used for MMC0 on AM62p
> and AM64x devices. The OTAP delay is not enabled when timing <
> SDR25 bus speed mode. The ITAP delay is not enabled for all bus
> speed modes.
>
> Add this OTAP/ITAP delay functionality according to the datasheet
> [1] OTAPDLYENA and ITAPDLYENA for MMC0.
>
> [1] https://www.ti.com/lit/ds/symlink/am62p.pdf
>
> Fixes: 8ee5fc0e0b3b ("mmc: sdhci_am654: Update OTAPDLY writes")
> Signed-off-by: Judith Mendez <jm@...com>
> ---
> drivers/mmc/host/sdhci_am654.c | 48 +++++++++++++++++++---------------
> 1 file changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index ff18a274b6f2..5ac82bc70706 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -143,6 +143,7 @@ struct sdhci_am654_data {
> struct regmap *base;
> int otap_del_sel[ARRAY_SIZE(td)];
> int itap_del_sel[ARRAY_SIZE(td)];
> + u8 itap_del_ena[ARRAY_SIZE(td)];
Why u8? Seems this is always manipulated as a u32. In fact
the same is true for `otap_del_sel` and `itap_del_sel` above.
Those needed fixed also.
> int clkbuf_sel;
> int trm_icp;
> int drv_strength;
> @@ -171,11 +172,13 @@ struct sdhci_am654_driver_data {
> };
>
> static void sdhci_am654_write_itapdly(struct sdhci_am654_data *sdhci_am654,
> - u32 itapdly)
> + u32 itapdly, u32 enable)
> {
> /* Set ITAPCHGWIN before writing to ITAPDLY */
> regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK,
> 0x1 << ITAPCHGWIN_SHIFT);
> + regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPDLYENA_MASK,
> + enable << ITAPDLYENA_SHIFT);
> regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPDLYSEL_MASK,
> itapdly << ITAPDLYSEL_SHIFT);
> regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
> @@ -249,7 +252,8 @@ static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned int clock,
> return;
> }
>
> - sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]);
> + sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing],
> + sdhci_am654->itap_del_ena[timing]);
> }
>
> static void sdhci_am654_setup_delay_chain(struct sdhci_am654_data *sdhci_am654,
> @@ -263,8 +267,8 @@ static void sdhci_am654_setup_delay_chain(struct sdhci_am654_data *sdhci_am654,
> mask = SELDLYTXCLK_MASK | SELDLYRXCLK_MASK;
> regmap_update_bits(sdhci_am654->base, PHY_CTRL5, mask, val);
>
> - sdhci_am654_write_itapdly(sdhci_am654,
> - sdhci_am654->itap_del_sel[timing]);
> + sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing],
> + sdhci_am654->itap_del_ena[timing]);
> }
>
> static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
> @@ -273,20 +277,17 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> unsigned char timing = host->mmc->ios.timing;
> u32 otap_del_sel;
> - u32 otap_del_ena;
> u32 mask, val;
>
> regmap_update_bits(sdhci_am654->base, PHY_CTRL1, ENDLL_MASK, 0);
>
> sdhci_set_clock(host, clock);
>
> - /* Setup DLL Output TAP delay */
> + /* Setup Output TAP delay */
> otap_del_sel = sdhci_am654->otap_del_sel[timing];
> - otap_del_ena = (timing > MMC_TIMING_UHS_SDR25) ? 1 : 0;
>
> mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
> - val = (otap_del_ena << OTAPDLYENA_SHIFT) |
> - (otap_del_sel << OTAPDLYSEL_SHIFT);
> + val = (0x1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
>
> /* Write to STRBSEL for HS400 speed mode */
> if (timing == MMC_TIMING_MMC_HS400) {
> @@ -319,14 +320,20 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> unsigned char timing = host->mmc->ios.timing;
> u32 otap_del_sel;
> + u32 itap_del_ena;
> u32 mask, val;
>
> - /* Setup DLL Output TAP delay */
> + /* Setup Output TAP delay */
> otap_del_sel = sdhci_am654->otap_del_sel[timing];
>
> mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
> - val = (0x1 << OTAPDLYENA_SHIFT) |
> - (otap_del_sel << OTAPDLYSEL_SHIFT);
> + val = (0x1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
You are not changing anything in this line, so why reformat it? If
you want to do some reformatting put it in a separate patch. And in
this case, I like it better how it was.
Andrew
> +
> + itap_del_ena = sdhci_am654->itap_del_ena[timing];
> +
> + mask |= ITAPDLYENA_MASK;
> + val |= (itap_del_ena << ITAPDLYENA_SHIFT);
> +
> regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val);
>
> regmap_update_bits(sdhci_am654->base, PHY_CTRL5, CLKBUFSEL_MASK,
> @@ -503,12 +510,8 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>
> 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 < ITAPDLY_LENGTH; itap++) {
> - sdhci_am654_write_itapdly(sdhci_am654, itap);
> + sdhci_am654_write_itapdly(sdhci_am654, itap, 1);
>
> curr_pass = !mmc_send_tuning(host->mmc, opcode, NULL);
>
> @@ -532,7 +535,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
> itap = sdhci_am654_calculate_itap(host, fail_window, fail_index,
> (sdhci_am654->dll_enable ? true : false));
>
> - sdhci_am654_write_itapdly(sdhci_am654, itap);
> + sdhci_am654_write_itapdly(sdhci_am654, itap, 1);
>
> return 0;
> }
> @@ -681,9 +684,12 @@ static int sdhci_am654_get_otap_delay(struct sdhci_host *host,
> host->mmc->caps2 &= ~td[i].capability;
> }
>
> - if (td[i].itap_binding)
> - device_property_read_u32(dev, td[i].itap_binding,
> - &sdhci_am654->itap_del_sel[i]);
> + if (td[i].itap_binding) {
> + ret = device_property_read_u32(dev, td[i].itap_binding,
> + &sdhci_am654->itap_del_sel[i]);
> + if (!ret)
> + sdhci_am654->itap_del_ena[i] = 0x1;
> + }
> }
>
> return 0;
Powered by blists - more mailing lists