Hi Andrew, On 2/1/24 1:46 PM, Andrew Davis wrote:
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@xxxxxx> --- 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.cindex 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.
Sure, I can fix for v2.
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.
Ok, I thought it was easier to read, but I can revert for v2. ~ judith
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;
