This patch series introduces a new tuning algorithm for
mmc. The new algorithm should be used when delay chain is
enabled. The ITAPDLY is selected from the largest passing
window and the buffer is not viewed as a circular buffer.
The new tuning algorithm is implemented as per the paper
published here [0] and has been tested on the following
platforms: AM62x SK, AM62A SK, AM62p SK, AM64x SK, and AM64x
EVM.
The series also includes a few fixes in the sdhci_am654
driver on OTAPDLYEN/ITAPDLYEN and ITAPDELSEL.
[0] https://www.ti.com/lit/an/spract9/spract9.pdf
Judith Mendez (5):
mmc: sdhci_am654: Add tuning algorithm for delay chain
mmc: sdhci_am654: Write ITAPDLY for DDR52 timing
mmc: sdhci_am654: Add missing OTAP/ITAP enable
mmc: sdhci_am654: Add ITAPDLYSEL in sdhci_j721e_4bit_set_clock
mmc: sdhci_am654: Fix ITAPDLY for HS400 timing
drivers/mmc/host/sdhci_am654.c | 215 +++++++++++++++++++++++++--------
1 file changed, 165 insertions(+), 50 deletions(-)
--
2.34.1
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 <[email protected]>
---
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)];
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);
+
+ 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;
--
2.34.1
While STRB is currently used for DATA and CRC responses, the CMD
responses from the device to the host still require ITAPDLY for
HS400 timing.
Currently what is stored for HS400 is the ITAPDLY from High Speed
mode which is incorrect. The ITAPDLY for HS400 speed mode should
be the same as ITAPDLY as HS200 timing after tuning is executed.
Add the functionality to save ITAPDLY from HS200 tuning and save
as HS400 ITAPDLY.
Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed modes")
Signed-off-by: Judith Mendez <[email protected]>
---
drivers/mmc/host/sdhci_am654.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index f5dc981c470d..beb0ca88ba1b 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -151,6 +151,7 @@ struct sdhci_am654_data {
u32 flags;
u32 quirks;
bool dll_enable;
+ bool hs200_tunning;
#define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
};
@@ -252,6 +253,10 @@ static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned int clock,
return;
}
+ /* HS400 ITAPDLY should be the same as HS200 ITAPDLY*/
+ if (timing == MMC_TIMING_MMC_HS400)
+ sdhci_am654->itap_del_sel[timing] = sdhci_am654->itap_del_sel[timing - 1];
+
sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing],
sdhci_am654->itap_del_ena[timing]);
}
@@ -311,6 +316,9 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
regmap_update_bits(sdhci_am654->base, PHY_CTRL5, CLKBUFSEL_MASK,
sdhci_am654->clkbuf_sel);
+
+ if (timing == MMC_TIMING_MMC_HS200 && sdhci_am654->dll_enable)
+ sdhci_am654->hs200_tunning = true;
}
static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
@@ -543,6 +551,10 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
sdhci_am654_write_itapdly(sdhci_am654, itap, 1);
+ /* Save ITAPDLY for HS200 */
+ if (sdhci_am654->hs200_tunning)
+ sdhci_am654->itap_del_sel[MMC_TIMING_MMC_HS200] = itap;
+
return 0;
}
--
2.34.1
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 <[email protected]>
> ---
> 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;
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 <[email protected]>
>> ---
>> 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.
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;
On 2/6/24 4:00 PM, Judith Mendez wrote:
> 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 <[email protected]>
>>> ---
>>> 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.
>
> 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.
>
It might be easier to read, and patches that make the code easier
to read are always welcome. The issue was since you didn't touch
this line as part of what this patch does, cleaning it up should
go in a different patch. Fixups like that make this patch overly
busy.
Andrew
> ~ 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;
>