[PATCH 0/5] Fix MMC tuning algorithm

[PATCH 0/5] Fix MMC tuning algorithm

[Judith Mendez]

The following patch series includes a MMC tuning algorithm
fix according to the following published paper [0].

This seris also includes fixes for OTAP/ITAP delay values
in j721e_4bit_sdhci_set_ios_post and for HS400 mode.

For DDR52 mode, also set ENDLL=1 and call am654_sdhci_setup_dll()
instead of am654_sdhci_setup_delay_chain() according to
device datasheet[1].

[0] https://www.ti.com/lit/an/spract9/spract9.pdf
[1] https://www.ti.com/lit/ds/symlink/am62p.pdf

Judith Mendez (4):
  mmc: am654_sdhci: Add tuning algorithm for delay chain
  mmc: am654_sdhci: Add itap_del_ena[] to store itapdlyena bit
  mmc: am654_sdhci: Set ENDLL=1 for DDR52 mode
  mmc: am654_sdhci: Fix ITAPDLY for HS400 timing

Nitin Yadav (1):
  mmc: am654_sdhci: Fix OTAP/ITAP delay values

 drivers/mmc/am654_sdhci.c | 170 +++++++++++++++++++++++++++++++-------
 1 file changed, 138 insertions(+), 32 deletions(-)


base-commit: 27795dd717dadc73091e1b4d6c50952b93aaa819
-- 
2.43.2

[PATCH 1/5] mmc: am654_sdhci: Add tuning algorithm for delay chain

[Judith Mendez]

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 tuning 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

Fixes: a759abf569d4 ("mmc: am654_sdhci: Add support for software tuning")
Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/am654_sdhci.c | 107 +++++++++++++++++++++++++++++++-------
 1 file changed, 89 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index 05595bdac39..e5ad00e2531 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -97,6 +97,7 @@ struct am654_sdhci_plat {
 	u32 strb_sel;
 	u32 clkbuf_sel;
 	u32 flags;
+	bool dll_enable;
 #define DLL_PRESENT	BIT(0)
 #define IOMUX_PRESENT	BIT(1)
 #define FREQSEL_2_BIT	BIT(2)
@@ -110,6 +111,12 @@ struct timing_data {
 	u32 capability;
 };
 
+struct window {
+	u8 start;
+	u8 end;
+	u8 length;
+};
+
 static const struct timing_data td[] = {
 	[MMC_LEGACY]	= {"ti,otap-del-sel-legacy",
 			   "ti,itap-del-sel-legacy",
@@ -280,8 +287,11 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
 		ret = am654_sdhci_setup_dll(plat, speed);
 		if (ret)
 			return ret;
+
+		plat->dll_enable = true;
 	} else {
 		am654_sdhci_setup_delay_chain(plat, mode);
+		plat->dll_enable = false;
 	}
 
 	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
@@ -375,38 +385,99 @@ static void am654_sdhci_write_b(struct sdhci_host *host, u8 val, int reg)
 	writeb(val, host->ioaddr + reg);
 }
 #ifdef MMC_SUPPORTS_TUNING
-#define ITAP_MAX	32
+#define ITAPDLY_LENGTH 32
+#define ITAPDLY_LAST_INDEX (ITAPDLY_LENGTH - 1)
+
+static u32 am654_sdhci_calculate_itap(struct udevice *dev, struct window
+			  *fail_window, u8 num_fails, bool circular_buffer)
+{
+	u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0;
+	u8 first_fail_start = 0, last_fail_end = 0;
+	struct window pass_window = {0, 0, 0};
+	int prev_fail_end = -1;
+	u8 i;
+
+	if (!num_fails)
+		return ITAPDLY_LAST_INDEX >> 1;
+
+	if (fail_window->length == ITAPDLY_LENGTH) {
+		dev_err(dev, "No passing ITAPDLY, return 0\n");
+		return 0;
+	}
+
+	first_fail_start = fail_window->start;
+	last_fail_end = fail_window[num_fails - 1].end;
+
+	for (i = 0; i < num_fails; i++) {
+		start_fail = fail_window[i].start;
+		end_fail = fail_window[i].end;
+		pass_length = start_fail - (prev_fail_end + 1);
+
+		if (pass_length > pass_window.length) {
+			pass_window.start = prev_fail_end + 1;
+			pass_window.length = pass_length;
+		}
+		prev_fail_end = end_fail;
+	}
+
+	if (!circular_buffer)
+		pass_length = ITAPDLY_LAST_INDEX - last_fail_end;
+	else
+		pass_length = ITAPDLY_LAST_INDEX - last_fail_end + 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;
+
+	return (itap > ITAPDLY_LAST_INDEX) ? ITAPDLY_LAST_INDEX >> 1 : itap;
+}
+
 static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 {
 	struct udevice *dev = mmc->dev;
 	struct am654_sdhci_plat *plat = dev_get_plat(dev);
-	int cur_val, prev_val = 1, fail_len = 0, pass_window = 0, pass_len;
-	u32 itap;
+	struct window fail_window[ITAPDLY_LENGTH];
+	u8 curr_pass, itap;
+	u8 fail_index = 0;
+	u8 prev_pass = 1;
+
+	memset(fail_window, 0, sizeof(fail_window));
 
 	/* Enable ITAPDLY */
 	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYENA_MASK,
 			   1 << ITAPDLYENA_SHIFT);
 
-	for (itap = 0; itap < ITAP_MAX; itap++) {
+	for (itap = 0; itap < ITAPDLY_LENGTH; itap++) {
 		am654_sdhci_write_itapdly(plat, itap);
 
-		cur_val = !mmc_send_tuning(mmc, opcode, NULL);
-		if (cur_val && !prev_val)
-			pass_window = itap;
+		curr_pass = !mmc_send_tuning(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 = am654_sdhci_calculate_itap(dev, fail_window, fail_index,
+					  plat->dll_enable);
+
 	am654_sdhci_write_itapdly(plat, itap);
 
 	return 0;
-- 
2.43.2

[PATCH 2/5] mmc: am654_sdhci: Fix OTAP/ITAP delay values

[Judith Mendez]

From: Nitin Yadav <n-yadav@ti.com>

U-Boot is failing to boot class U1 UHS SD cards due to incorrect
OTAP and ITAP delay select values. Update OTAP and ITAP delay select
values from DT.

Fixes: c7d106b4eb3 ("mmc: am654_sdhci: Update output tap delay writes")
Signed-off-by: Nitin Yadav <n-yadav@ti.com>
Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/am654_sdhci.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index e5ad00e2531..1dd032e1e36 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -513,12 +513,27 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
 {
 	struct udevice *dev = host->mmc->dev;
 	struct am654_sdhci_plat *plat = dev_get_plat(dev);
-	u32 otap_del_sel, mask, val;
+	int mode = host->mmc->selected_mode;
+	u32 otap_del_sel;
+	u32 itap_del_sel;
+	u32 mask, val;
+
+	otap_del_sel = plat->otap_del_sel[mode];
 
-	otap_del_sel = plat->otap_del_sel[host->mmc->selected_mode];
 	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
-	val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
+	val = (1 << OTAPDLYENA_SHIFT) |
+	      (otap_del_sel << OTAPDLYSEL_SHIFT);
+
+	itap_del_sel = plat->itap_del_sel[mode];
+
+	mask |= ITAPDLYENA_MASK | ITAPDLYSEL_MASK;
+	val = (1 << ITAPDLYENA_SHIFT) |
+	      (itap_del_sel << ITAPDLYSEL_SHIFT);
+
+	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
+			   1 << ITAPCHGWIN_SHIFT);
 	regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
+	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
 
 	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
 			   plat->clkbuf_sel);
@@ -572,7 +587,7 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
 	 * Remove the corresponding capability if an otap-del-sel
 	 * value is not found
 	 */
-	for (i = MMC_HS; i <= MMC_HS_400; i++) {
+	for (i = MMC_LEGACY; i <= MMC_HS_400; i++) {
 		ret = dev_read_u32(dev, td[i].otap_binding,
 				   &plat->otap_del_sel[i]);
 		if (ret) {
-- 
2.43.2

[PATCH 3/5] mmc: am654_sdhci: Add itap_del_ena[] to store itapdlyena bit

[Judith Mendez]

Set itap_del_ena if ITAPDLY is found in DT or if the tuning
algorithm was executed and found the optimal ITAPDLY. Add the
functionality to save ITAPDLYENA that can be referenced later
by storing the bit in array itap_del_ena[].

Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/am654_sdhci.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index 1dd032e1e36..38f1ad28ec4 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -92,6 +92,7 @@ struct am654_sdhci_plat {
 	bool non_removable;
 	u32 otap_del_sel[MMC_MODES_END];
 	u32 itap_del_sel[MMC_MODES_END];
+	u32 itap_del_ena[MMC_MODES_END];
 	u32 trm_icp;
 	u32 drv_strength;
 	u32 strb_sel;
@@ -223,8 +224,10 @@ static int am654_sdhci_setup_dll(struct am654_sdhci_plat *plat,
 }
 
 static void am654_sdhci_write_itapdly(struct am654_sdhci_plat *plat,
-				      u32 itapdly)
+				      u32 itapdly, u32 enable)
 {
+	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYENA_MASK,
+			   enable << ITAPDLYENA_SHIFT);
 	/* Set ITAPCHGWIN before writing to ITAPDLY */
 	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
 			   1 << ITAPCHGWIN_SHIFT);
@@ -242,7 +245,8 @@ static void am654_sdhci_setup_delay_chain(struct am654_sdhci_plat *plat,
 	mask = SELDLYTXCLK_MASK | SELDLYRXCLK_MASK;
 	regmap_update_bits(plat->base, PHY_CTRL5, mask, val);
 
-	am654_sdhci_write_itapdly(plat, plat->itap_del_sel[mode]);
+	am654_sdhci_write_itapdly(plat, plat->itap_del_sel[mode],
+				  plat->itap_del_ena[mode]);
 }
 
 static int am654_sdhci_set_ios_post(struct sdhci_host *host)
@@ -443,6 +447,7 @@ static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 	struct udevice *dev = mmc->dev;
 	struct am654_sdhci_plat *plat = dev_get_plat(dev);
 	struct window fail_window[ITAPDLY_LENGTH];
+	int mode = mmc->selected_mode;
 	u8 curr_pass, itap;
 	u8 fail_index = 0;
 	u8 prev_pass = 1;
@@ -450,11 +455,10 @@ static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 	memset(fail_window, 0, sizeof(fail_window));
 
 	/* Enable ITAPDLY */
-	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYENA_MASK,
-			   1 << ITAPDLYENA_SHIFT);
+	plat->itap_del_ena[mode] = 0x1;
 
 	for (itap = 0; itap < ITAPDLY_LENGTH; itap++) {
-		am654_sdhci_write_itapdly(plat, itap);
+		am654_sdhci_write_itapdly(plat, itap, plat->itap_del_ena[mode]);
 
 		curr_pass = !mmc_send_tuning(mmc, opcode, NULL);
 
@@ -478,7 +482,7 @@ static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 	itap = am654_sdhci_calculate_itap(dev, fail_window, fail_index,
 					  plat->dll_enable);
 
-	am654_sdhci_write_itapdly(plat, itap);
+	am654_sdhci_write_itapdly(plat, itap, plat->itap_del_ena[mode]);
 
 	return 0;
 }
@@ -515,6 +519,7 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
 	struct am654_sdhci_plat *plat = dev_get_plat(dev);
 	int mode = host->mmc->selected_mode;
 	u32 otap_del_sel;
+	u32 itap_del_ena;
 	u32 itap_del_sel;
 	u32 mask, val;
 
@@ -524,10 +529,11 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
 	val = (1 << OTAPDLYENA_SHIFT) |
 	      (otap_del_sel << OTAPDLYSEL_SHIFT);
 
+	itap_del_ena = plat->itap_del_ena[mode];
 	itap_del_sel = plat->itap_del_sel[mode];
 
 	mask |= ITAPDLYENA_MASK | ITAPDLYSEL_MASK;
-	val = (1 << ITAPDLYENA_SHIFT) |
+	val = (itap_del_ena << ITAPDLYENA_SHIFT) |
 	      (itap_del_sel << ITAPDLYSEL_SHIFT);
 
 	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
@@ -599,9 +605,13 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
 			cfg->host_caps &= ~td[i].capability;
 		}
 
-		if (td[i].itap_binding)
-			dev_read_u32(dev, td[i].itap_binding,
-				     &plat->itap_del_sel[i]);
+		if (td[i].itap_binding) {
+			ret = dev_read_u32(dev, td[i].itap_binding,
+					   &plat->itap_del_sel[i]);
+
+			if (!ret)
+				plat->itap_del_ena[i] = 0x1;
+		}
 	}
 
 	return 0;
-- 
2.43.2

[PATCH 4/5] mmc: am654_sdhci: Set ENDLL=1 for DDR52 mode

[Judith Mendez]

According to the device datasheet [0], ENDLL=1 for
DDR52 mode, so call am654_sdhci_setup_dll() and write
itapdly after since we do not carry out tuning.

[0] https://www.ti.com/lit/ds/symlink/am62p.pdf
Fixes: c964447ea3d6 ("mmc: am654_sdhci: Add support for input tap delay")
Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/am654_sdhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index 38f1ad28ec4..dee56dfdbaa 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -287,12 +287,14 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
 
 	regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
 
-	if (mode > UHS_SDR25 && speed >= CLOCK_TOO_SLOW_HZ) {
+	if ((mode > UHS_SDR25 || mode == MMC_DDR_52) && speed >= CLOCK_TOO_SLOW_HZ) {
 		ret = am654_sdhci_setup_dll(plat, speed);
 		if (ret)
 			return ret;
 
 		plat->dll_enable = true;
+		am654_sdhci_write_itapdly(plat, plat->itap_del_sel[mode],
+					  plat->itap_del_ena[mode]);
 	} else {
 		am654_sdhci_setup_delay_chain(plat, mode);
 		plat->dll_enable = false;
-- 
2.43.2

[PATCH 5/5] mmc: am654_sdhci: Fix ITAPDLY for HS400 timing

[Judith Mendez]

At HS400 mode the ITAPDLY value is that from High Speed mode
which is incorrect and may cause boot failures.

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: c964447ea3d6 ("mmc: am654_sdhci: Add support for input tap delay")
Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/am654_sdhci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
index dee56dfdbaa..ce3813ea3d0 100644
--- a/drivers/mmc/am654_sdhci.c
+++ b/drivers/mmc/am654_sdhci.c
@@ -293,6 +293,11 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
 			return ret;
 
 		plat->dll_enable = true;
+		if (mode == MMC_HS_400) {
+			plat->itap_del_ena[mode] = 0x1;
+			plat->itap_del_sel[mode] = plat->itap_del_sel[mode - 1];
+		}
+
 		am654_sdhci_write_itapdly(plat, plat->itap_del_sel[mode],
 					  plat->itap_del_ena[mode]);
 	} else {
@@ -484,6 +489,9 @@ static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 	itap = am654_sdhci_calculate_itap(dev, fail_window, fail_index,
 					  plat->dll_enable);
 
+	/* Save ITAPDLY */
+	plat->itap_del_sel[mode] = itap;
+
 	am654_sdhci_write_itapdly(plat, itap, plat->itap_del_ena[mode]);
 
 	return 0;
-- 
2.43.2

RE: [PATCH 1/5] mmc: am654_sdhci: Add tuning algorithm for delay chain

[Jaehoon Chung]

> -----Original Message-----
> From: Judith Mendez <jm@ti.com>
> Sent: Tuesday, April 16, 2024 6:28 AM
> To: Peng Fan <peng.fan@nxp.com>; Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
> Cc: Nitin Yadav <n-yadav@ti.com>; Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de
> Subject: [PATCH 1/5] mmc: am654_sdhci: Add tuning algorithm for delay chain
> 
> 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 tuning 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
> 
> Fixes: a759abf569d4 ("mmc: am654_sdhci: Add support for software tuning")
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  drivers/mmc/am654_sdhci.c | 107 +++++++++++++++++++++++++++++++-------
>  1 file changed, 89 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
> index 05595bdac39..e5ad00e2531 100644
> --- a/drivers/mmc/am654_sdhci.c
> +++ b/drivers/mmc/am654_sdhci.c
> @@ -97,6 +97,7 @@ struct am654_sdhci_plat {
>  	u32 strb_sel;
>  	u32 clkbuf_sel;
>  	u32 flags;
> +	bool dll_enable;
>  #define DLL_PRESENT	BIT(0)
>  #define IOMUX_PRESENT	BIT(1)
>  #define FREQSEL_2_BIT	BIT(2)
> @@ -110,6 +111,12 @@ struct timing_data {
>  	u32 capability;
>  };
> 
> +struct window {
> +	u8 start;
> +	u8 end;
> +	u8 length;
> +};
> +
>  static const struct timing_data td[] = {
>  	[MMC_LEGACY]	= {"ti,otap-del-sel-legacy",
>  			   "ti,itap-del-sel-legacy",
> @@ -280,8 +287,11 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
>  		ret = am654_sdhci_setup_dll(plat, speed);
>  		if (ret)
>  			return ret;
> +
> +		plat->dll_enable = true;
>  	} else {
>  		am654_sdhci_setup_delay_chain(plat, mode);
> +		plat->dll_enable = false;
>  	}
> 
>  	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
> @@ -375,38 +385,99 @@ static void am654_sdhci_write_b(struct sdhci_host *host, u8 val, int reg)
>  	writeb(val, host->ioaddr + reg);
>  }
>  #ifdef MMC_SUPPORTS_TUNING
> -#define ITAP_MAX	32
> +#define ITAPDLY_LENGTH 32
> +#define ITAPDLY_LAST_INDEX (ITAPDLY_LENGTH - 1)
> +
> +static u32 am654_sdhci_calculate_itap(struct udevice *dev, struct window
> +			  *fail_window, u8 num_fails, bool circular_buffer)
> +{
> +	u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0;
> +	u8 first_fail_start = 0, last_fail_end = 0;
> +	struct window pass_window = {0, 0, 0};
> +	int prev_fail_end = -1;
> +	u8 i;
> +
> +	if (!num_fails)
> +		return ITAPDLY_LAST_INDEX >> 1;
> +
> +	if (fail_window->length == ITAPDLY_LENGTH) {
> +		dev_err(dev, "No passing ITAPDLY, return 0\n");
> +		return 0;
> +	}
> +
> +	first_fail_start = fail_window->start;
> +	last_fail_end = fail_window[num_fails - 1].end;
> +
> +	for (i = 0; i < num_fails; i++) {
> +		start_fail = fail_window[i].start;
> +		end_fail = fail_window[i].end;
> +		pass_length = start_fail - (prev_fail_end + 1);
> +
> +		if (pass_length > pass_window.length) {
> +			pass_window.start = prev_fail_end + 1;
> +			pass_window.length = pass_length;
> +		}
> +		prev_fail_end = end_fail;
> +	}
> +
> +	if (!circular_buffer)
> +		pass_length = ITAPDLY_LAST_INDEX - last_fail_end;
> +	else
> +		pass_length = ITAPDLY_LAST_INDEX - last_fail_end + 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;
> +
> +	return (itap > ITAPDLY_LAST_INDEX) ? ITAPDLY_LAST_INDEX >> 1 : itap;
> +}
> +
>  static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>  {
>  	struct udevice *dev = mmc->dev;
>  	struct am654_sdhci_plat *plat = dev_get_plat(dev);
> -	int cur_val, prev_val = 1, fail_len = 0, pass_window = 0, pass_len;
> -	u32 itap;
> +	struct window fail_window[ITAPDLY_LENGTH];
> +	u8 curr_pass, itap;
> +	u8 fail_index = 0;
> +	u8 prev_pass = 1;
> +
> +	memset(fail_window, 0, sizeof(fail_window));
> 
>  	/* Enable ITAPDLY */
>  	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYENA_MASK,
>  			   1 << ITAPDLYENA_SHIFT);
> 
> -	for (itap = 0; itap < ITAP_MAX; itap++) {
> +	for (itap = 0; itap < ITAPDLY_LENGTH; itap++) {
>  		am654_sdhci_write_itapdly(plat, itap);
> 
> -		cur_val = !mmc_send_tuning(mmc, opcode, NULL);
> -		if (cur_val && !prev_val)
> -			pass_window = itap;
> +		curr_pass = !mmc_send_tuning(mmc, opcode, NULL);

It's changed to !mmc_send_tuing(mmc, opcode). Could you change this based on latest u-boot?

Sorry for bothering you.

commit a3b2786651c7 "mmc: Drop unused mmc_send_tuning() cmd_error parameter"

Best Regards,
Jaehoon Chung

> 
> -		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 = am654_sdhci_calculate_itap(dev, fail_window, fail_index,
> +					  plat->dll_enable);
> +
>  	am654_sdhci_write_itapdly(plat, itap);
> 
>  	return 0;
> --
> 2.43.2

RE: [PATCH 2/5] mmc: am654_sdhci: Fix OTAP/ITAP delay values

[Jaehoon Chung]

Hi Judith,

> -----Original Message-----
> From: Judith Mendez <jm@ti.com>
> Sent: Tuesday, April 16, 2024 6:28 AM
> To: Peng Fan <peng.fan@nxp.com>; Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
> Cc: Nitin Yadav <n-yadav@ti.com>; Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de
> Subject: [PATCH 2/5] mmc: am654_sdhci: Fix OTAP/ITAP delay values
> 
> From: Nitin Yadav <n-yadav@ti.com>
> 
> U-Boot is failing to boot class U1 UHS SD cards due to incorrect
> OTAP and ITAP delay select values. Update OTAP and ITAP delay select
> values from DT.
> 
> Fixes: c7d106b4eb3 ("mmc: am654_sdhci: Update output tap delay writes")
> Signed-off-by: Nitin Yadav <n-yadav@ti.com>
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  drivers/mmc/am654_sdhci.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
> index e5ad00e2531..1dd032e1e36 100644
> --- a/drivers/mmc/am654_sdhci.c
> +++ b/drivers/mmc/am654_sdhci.c
> @@ -513,12 +513,27 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
>  {
>  	struct udevice *dev = host->mmc->dev;
>  	struct am654_sdhci_plat *plat = dev_get_plat(dev);
> -	u32 otap_del_sel, mask, val;
> +	int mode = host->mmc->selected_mode;
> +	u32 otap_del_sel;
> +	u32 itap_del_sel;
> +	u32 mask, val;
> +
> +	otap_del_sel = plat->otap_del_sel[mode];
> 
> -	otap_del_sel = plat->otap_del_sel[host->mmc->selected_mode];
>  	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
> -	val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
> +	val = (1 << OTAPDLYENA_SHIFT) |
> +	      (otap_del_sel << OTAPDLYSEL_SHIFT);

Is there any reason to touch this? And I can't understood this, this val doesn’t use anywhere.
Because val is resetting the below. It seems same value, right?

> +
> +	itap_del_sel = plat->itap_del_sel[mode];
> +
> +	mask |= ITAPDLYENA_MASK | ITAPDLYSEL_MASK;

Can it be set at above?

mask |= OTAPDLYENA_MASK | OTAPDLYSEL_MASK | 
	ITAPDLYENA_MASK | ITAPDLYSEL_MASK;

Best Regards,
Jaehoon Chung



> +	val = (1 << ITAPDLYENA_SHIFT) |
> +	      (itap_del_sel << ITAPDLYSEL_SHIFT);
> +
> +	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
> +			   1 << ITAPCHGWIN_SHIFT);
>  	regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
> +	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
> 
>  	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
>  			   plat->clkbuf_sel);
> @@ -572,7 +587,7 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
>  	 * Remove the corresponding capability if an otap-del-sel
>  	 * value is not found
>  	 */
> -	for (i = MMC_HS; i <= MMC_HS_400; i++) {
> +	for (i = MMC_LEGACY; i <= MMC_HS_400; i++) {
>  		ret = dev_read_u32(dev, td[i].otap_binding,
>  				   &plat->otap_del_sel[i]);
>  		if (ret) {
> --
> 2.43.2

RE: [PATCH 3/5] mmc: am654_sdhci: Add itap_del_ena[] to store itapdlyena bit

[Jaehoon Chung]

Hi,

> -----Original Message-----
> From: Judith Mendez <jm@ti.com>
> Sent: Tuesday, April 16, 2024 6:28 AM
> To: Peng Fan <peng.fan@nxp.com>; Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
> Cc: Nitin Yadav <n-yadav@ti.com>; Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de
> Subject: [PATCH 3/5] mmc: am654_sdhci: Add itap_del_ena[] to store itapdlyena bit
> 
> Set itap_del_ena if ITAPDLY is found in DT or if the tuning
> algorithm was executed and found the optimal ITAPDLY. Add the
> functionality to save ITAPDLYENA that can be referenced later
> by storing the bit in array itap_del_ena[].
> 
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  drivers/mmc/am654_sdhci.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
> index 1dd032e1e36..38f1ad28ec4 100644
> --- a/drivers/mmc/am654_sdhci.c
> +++ b/drivers/mmc/am654_sdhci.c
> @@ -92,6 +92,7 @@ struct am654_sdhci_plat {
>  	bool non_removable;
>  	u32 otap_del_sel[MMC_MODES_END];
>  	u32 itap_del_sel[MMC_MODES_END];
> +	u32 itap_del_ena[MMC_MODES_END];
>  	u32 trm_icp;
>  	u32 drv_strength;
>  	u32 strb_sel;
> @@ -223,8 +224,10 @@ static int am654_sdhci_setup_dll(struct am654_sdhci_plat *plat,
>  }
> 
>  static void am654_sdhci_write_itapdly(struct am654_sdhci_plat *plat,
> -				      u32 itapdly)
> +				      u32 itapdly, u32 enable)
>  {
> +	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYENA_MASK,
> +			   enable << ITAPDLYENA_SHIFT);
>  	/* Set ITAPCHGWIN before writing to ITAPDLY */
>  	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>  			   1 << ITAPCHGWIN_SHIFT);
> @@ -242,7 +245,8 @@ static void am654_sdhci_setup_delay_chain(struct am654_sdhci_plat *plat,
>  	mask = SELDLYTXCLK_MASK | SELDLYRXCLK_MASK;
>  	regmap_update_bits(plat->base, PHY_CTRL5, mask, val);
> 
> -	am654_sdhci_write_itapdly(plat, plat->itap_del_sel[mode]);
> +	am654_sdhci_write_itapdly(plat, plat->itap_del_sel[mode],
> +				  plat->itap_del_ena[mode]);
>  }
> 
>  static int am654_sdhci_set_ios_post(struct sdhci_host *host)
> @@ -443,6 +447,7 @@ static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>  	struct udevice *dev = mmc->dev;
>  	struct am654_sdhci_plat *plat = dev_get_plat(dev);
>  	struct window fail_window[ITAPDLY_LENGTH];
> +	int mode = mmc->selected_mode;
>  	u8 curr_pass, itap;
>  	u8 fail_index = 0;
>  	u8 prev_pass = 1;
> @@ -450,11 +455,10 @@ static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>  	memset(fail_window, 0, sizeof(fail_window));
> 
>  	/* Enable ITAPDLY */
> -	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYENA_MASK,
> -			   1 << ITAPDLYENA_SHIFT);
> +	plat->itap_del_ena[mode] = 0x1;

0x1 means "enable"? I want to use a macro with meaning.

> 
>  	for (itap = 0; itap < ITAPDLY_LENGTH; itap++) {
> -		am654_sdhci_write_itapdly(plat, itap);
> +		am654_sdhci_write_itapdly(plat, itap, plat->itap_del_ena[mode]);
> 
>  		curr_pass = !mmc_send_tuning(mmc, opcode, NULL);
> 
> @@ -478,7 +482,7 @@ static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>  	itap = am654_sdhci_calculate_itap(dev, fail_window, fail_index,
>  					  plat->dll_enable);
> 
> -	am654_sdhci_write_itapdly(plat, itap);
> +	am654_sdhci_write_itapdly(plat, itap, plat->itap_del_ena[mode]);
> 
>  	return 0;
>  }
> @@ -515,6 +519,7 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
>  	struct am654_sdhci_plat *plat = dev_get_plat(dev);
>  	int mode = host->mmc->selected_mode;
>  	u32 otap_del_sel;
> +	u32 itap_del_ena;
>  	u32 itap_del_sel;
>  	u32 mask, val;
> 
> @@ -524,10 +529,11 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
>  	val = (1 << OTAPDLYENA_SHIFT) |
>  	      (otap_del_sel << OTAPDLYSEL_SHIFT);
> 
> +	itap_del_ena = plat->itap_del_ena[mode];
>  	itap_del_sel = plat->itap_del_sel[mode];
> 
>  	mask |= ITAPDLYENA_MASK | ITAPDLYSEL_MASK;
> -	val = (1 << ITAPDLYENA_SHIFT) |
> +	val = (itap_del_ena << ITAPDLYENA_SHIFT) |
>  	      (itap_del_sel << ITAPDLYSEL_SHIFT);
> 
>  	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
> @@ -599,9 +605,13 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
>  			cfg->host_caps &= ~td[i].capability;
>  		}
> 
> -		if (td[i].itap_binding)
> -			dev_read_u32(dev, td[i].itap_binding,
> -				     &plat->itap_del_sel[i]);
> +		if (td[i].itap_binding) {
> +			ret = dev_read_u32(dev, td[i].itap_binding,
> +					   &plat->itap_del_sel[i]);
> +
> +			if (!ret)
> +				plat->itap_del_ena[i] = 0x1;

Ditto.

Best Regards,
Jaehoon Chung

> +		}
>  	}
> 
>  	return 0;
> --
> 2.43.2

RE: [PATCH 4/5] mmc: am654_sdhci: Set ENDLL=1 for DDR52 mode

[Jaehoon Chung]

> -----Original Message-----
> From: Judith Mendez <jm@ti.com>
> Sent: Tuesday, April 16, 2024 6:28 AM
> To: Peng Fan <peng.fan@nxp.com>; Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
> Cc: Nitin Yadav <n-yadav@ti.com>; Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de
> Subject: [PATCH 4/5] mmc: am654_sdhci: Set ENDLL=1 for DDR52 mode
> 
> According to the device datasheet [0], ENDLL=1 for
> DDR52 mode, so call am654_sdhci_setup_dll() and write
> itapdly after since we do not carry out tuning.
> 
> [0] https://www.ti.com/lit/ds/symlink/am62p.pdf
> Fixes: c964447ea3d6 ("mmc: am654_sdhci: Add support for input tap delay")
> Signed-off-by: Judith Mendez <jm@ti.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
>  drivers/mmc/am654_sdhci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
> index 38f1ad28ec4..dee56dfdbaa 100644
> --- a/drivers/mmc/am654_sdhci.c
> +++ b/drivers/mmc/am654_sdhci.c
> @@ -287,12 +287,14 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
> 
>  	regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
> 
> -	if (mode > UHS_SDR25 && speed >= CLOCK_TOO_SLOW_HZ) {
> +	if ((mode > UHS_SDR25 || mode == MMC_DDR_52) && speed >= CLOCK_TOO_SLOW_HZ) {
>  		ret = am654_sdhci_setup_dll(plat, speed);
>  		if (ret)
>  			return ret;
> 
>  		plat->dll_enable = true;
> +		am654_sdhci_write_itapdly(plat, plat->itap_del_sel[mode],
> +					  plat->itap_del_ena[mode]);
>  	} else {
>  		am654_sdhci_setup_delay_chain(plat, mode);
>  		plat->dll_enable = false;
> --
> 2.43.2

Re: [PATCH 1/5] mmc: am654_sdhci: Add tuning algorithm for delay chain

[Judith Mendez]

Hi,

On 4/17/24 6:23 AM, Jaehoon Chung wrote:
> 
> 
>> -----Original Message-----
>> From: Judith Mendez <jm@ti.com>
>> Sent: Tuesday, April 16, 2024 6:28 AM
>> To: Peng Fan <peng.fan@nxp.com>; Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
>> Cc: Nitin Yadav <n-yadav@ti.com>; Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de
>> Subject: [PATCH 1/5] mmc: am654_sdhci: Add tuning algorithm for delay chain
>>
>> 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 tuning 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
>>
>> Fixes: a759abf569d4 ("mmc: am654_sdhci: Add support for software tuning")
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/am654_sdhci.c | 107 +++++++++++++++++++++++++++++++-------
>>   1 file changed, 89 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
>> index 05595bdac39..e5ad00e2531 100644
>> --- a/drivers/mmc/am654_sdhci.c
>> +++ b/drivers/mmc/am654_sdhci.c
>> @@ -97,6 +97,7 @@ struct am654_sdhci_plat {
>>   	u32 strb_sel;
>>   	u32 clkbuf_sel;
>>   	u32 flags;
>> +	bool dll_enable;
>>   #define DLL_PRESENT	BIT(0)
>>   #define IOMUX_PRESENT	BIT(1)
>>   #define FREQSEL_2_BIT	BIT(2)
>> @@ -110,6 +111,12 @@ struct timing_data {
>>   	u32 capability;
>>   };
>>
>> +struct window {
>> +	u8 start;
>> +	u8 end;
>> +	u8 length;
>> +};
>> +
>>   static const struct timing_data td[] = {
>>   	[MMC_LEGACY]	= {"ti,otap-del-sel-legacy",
>>   			   "ti,itap-del-sel-legacy",
>> @@ -280,8 +287,11 @@ static int am654_sdhci_set_ios_post(struct sdhci_host *host)
>>   		ret = am654_sdhci_setup_dll(plat, speed);
>>   		if (ret)
>>   			return ret;
>> +
>> +		plat->dll_enable = true;
>>   	} else {
>>   		am654_sdhci_setup_delay_chain(plat, mode);
>> +		plat->dll_enable = false;
>>   	}
>>
>>   	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
>> @@ -375,38 +385,99 @@ static void am654_sdhci_write_b(struct sdhci_host *host, u8 val, int reg)
>>   	writeb(val, host->ioaddr + reg);
>>   }
>>   #ifdef MMC_SUPPORTS_TUNING
>> -#define ITAP_MAX	32
>> +#define ITAPDLY_LENGTH 32
>> +#define ITAPDLY_LAST_INDEX (ITAPDLY_LENGTH - 1)
>> +
>> +static u32 am654_sdhci_calculate_itap(struct udevice *dev, struct window
>> +			  *fail_window, u8 num_fails, bool circular_buffer)
>> +{
>> +	u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0;
>> +	u8 first_fail_start = 0, last_fail_end = 0;
>> +	struct window pass_window = {0, 0, 0};
>> +	int prev_fail_end = -1;
>> +	u8 i;
>> +
>> +	if (!num_fails)
>> +		return ITAPDLY_LAST_INDEX >> 1;
>> +
>> +	if (fail_window->length == ITAPDLY_LENGTH) {
>> +		dev_err(dev, "No passing ITAPDLY, return 0\n");
>> +		return 0;
>> +	}
>> +
>> +	first_fail_start = fail_window->start;
>> +	last_fail_end = fail_window[num_fails - 1].end;
>> +
>> +	for (i = 0; i < num_fails; i++) {
>> +		start_fail = fail_window[i].start;
>> +		end_fail = fail_window[i].end;
>> +		pass_length = start_fail - (prev_fail_end + 1);
>> +
>> +		if (pass_length > pass_window.length) {
>> +			pass_window.start = prev_fail_end + 1;
>> +			pass_window.length = pass_length;
>> +		}
>> +		prev_fail_end = end_fail;
>> +	}
>> +
>> +	if (!circular_buffer)
>> +		pass_length = ITAPDLY_LAST_INDEX - last_fail_end;
>> +	else
>> +		pass_length = ITAPDLY_LAST_INDEX - last_fail_end + 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;
>> +
>> +	return (itap > ITAPDLY_LAST_INDEX) ? ITAPDLY_LAST_INDEX >> 1 : itap;
>> +}
>> +
>>   static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>>   {
>>   	struct udevice *dev = mmc->dev;
>>   	struct am654_sdhci_plat *plat = dev_get_plat(dev);
>> -	int cur_val, prev_val = 1, fail_len = 0, pass_window = 0, pass_len;
>> -	u32 itap;
>> +	struct window fail_window[ITAPDLY_LENGTH];
>> +	u8 curr_pass, itap;
>> +	u8 fail_index = 0;
>> +	u8 prev_pass = 1;
>> +
>> +	memset(fail_window, 0, sizeof(fail_window));
>>
>>   	/* Enable ITAPDLY */
>>   	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYENA_MASK,
>>   			   1 << ITAPDLYENA_SHIFT);
>>
>> -	for (itap = 0; itap < ITAP_MAX; itap++) {
>> +	for (itap = 0; itap < ITAPDLY_LENGTH; itap++) {
>>   		am654_sdhci_write_itapdly(plat, itap);
>>
>> -		cur_val = !mmc_send_tuning(mmc, opcode, NULL);
>> -		if (cur_val && !prev_val)
>> -			pass_window = itap;
>> +		curr_pass = !mmc_send_tuning(mmc, opcode, NULL);
> 
> It's changed to !mmc_send_tuing(mmc, opcode). Could you change this based on latest u-boot?
> 
> Sorry for bothering you.
> 
> commit a3b2786651c7 "mmc: Drop unused mmc_send_tuning() cmd_error parameter"

Thanks, will fix.

~ Judith

> 
> Best Regards,
> Jaehoon Chung
> 
>>
>> -		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 = am654_sdhci_calculate_itap(dev, fail_window, fail_index,
>> +					  plat->dll_enable);
>> +
>>   	am654_sdhci_write_itapdly(plat, itap);
>>
>>   	return 0;
>> --
>> 2.43.2
> 
> 

Re: [PATCH 2/5] mmc: am654_sdhci: Fix OTAP/ITAP delay values

[Judith Mendez]

On 4/17/24 6:28 AM, Jaehoon Chung wrote:
> Hi Judith,
> 
>> -----Original Message-----
>> From: Judith Mendez <jm@ti.com>
>> Sent: Tuesday, April 16, 2024 6:28 AM
>> To: Peng Fan <peng.fan@nxp.com>; Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
>> Cc: Nitin Yadav <n-yadav@ti.com>; Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de
>> Subject: [PATCH 2/5] mmc: am654_sdhci: Fix OTAP/ITAP delay values
>>
>> From: Nitin Yadav <n-yadav@ti.com>
>>
>> U-Boot is failing to boot class U1 UHS SD cards due to incorrect
>> OTAP and ITAP delay select values. Update OTAP and ITAP delay select
>> values from DT.
>>
>> Fixes: c7d106b4eb3 ("mmc: am654_sdhci: Update output tap delay writes")
>> Signed-off-by: Nitin Yadav <n-yadav@ti.com>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/am654_sdhci.c | 23 +++++++++++++++++++----
>>   1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
>> index e5ad00e2531..1dd032e1e36 100644
>> --- a/drivers/mmc/am654_sdhci.c
>> +++ b/drivers/mmc/am654_sdhci.c
>> @@ -513,12 +513,27 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
>>   {
>>   	struct udevice *dev = host->mmc->dev;
>>   	struct am654_sdhci_plat *plat = dev_get_plat(dev);
>> -	u32 otap_del_sel, mask, val;
>> +	int mode = host->mmc->selected_mode;
>> +	u32 otap_del_sel;
>> +	u32 itap_del_sel;
>> +	u32 mask, val;
>> +
>> +	otap_del_sel = plat->otap_del_sel[mode];
>>
>> -	otap_del_sel = plat->otap_del_sel[host->mmc->selected_mode];
>>   	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
>> -	val = (1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
>> +	val = (1 << OTAPDLYENA_SHIFT) |
>> +	      (otap_del_sel << OTAPDLYSEL_SHIFT);
> 
> Is there any reason to touch this? And I can't understood this, this val doesn’t use anywhere.
> Because val is resetting the below. It seems same value, right?

This change is syncing with upstream kernel driver.

The second val assignment is supposed to be val |=,
will fix for v2.

> 
>> +
>> +	itap_del_sel = plat->itap_del_sel[mode];
>> +
>> +	mask |= ITAPDLYENA_MASK | ITAPDLYSEL_MASK;
> 
> Can it be set at above?
> 
> mask |= OTAPDLYENA_MASK | OTAPDLYSEL_MASK |
> 	ITAPDLYENA_MASK | ITAPDLYSEL_MASK;

This is also syncing with upstream. I am hoping
to leave this as is.

~ Judith

> 
> Best Regards,
> Jaehoon Chung
> 
> 
> 
>> +	val = (1 << ITAPDLYENA_SHIFT) |
>> +	      (itap_del_sel << ITAPDLYSEL_SHIFT);
>> +
>> +	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>> +			   1 << ITAPCHGWIN_SHIFT);
>>   	regmap_update_bits(plat->base, PHY_CTRL4, mask, val);
>> +	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
>>
>>   	regmap_update_bits(plat->base, PHY_CTRL5, CLKBUFSEL_MASK,
>>   			   plat->clkbuf_sel);
>> @@ -572,7 +587,7 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
>>   	 * Remove the corresponding capability if an otap-del-sel
>>   	 * value is not found
>>   	 */
>> -	for (i = MMC_HS; i <= MMC_HS_400; i++) {
>> +	for (i = MMC_LEGACY; i <= MMC_HS_400; i++) {
>>   		ret = dev_read_u32(dev, td[i].otap_binding,
>>   				   &plat->otap_del_sel[i]);
>>   		if (ret) {
>> --
>> 2.43.2
> 
> 
> 

Re: [PATCH 3/5] mmc: am654_sdhci: Add itap_del_ena[] to store itapdlyena bit

[Judith Mendez]

On 4/17/24 6:34 AM, Jaehoon Chung wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Judith Mendez <jm@ti.com>
>> Sent: Tuesday, April 16, 2024 6:28 AM
>> To: Peng Fan <peng.fan@nxp.com>; Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
>> Cc: Nitin Yadav <n-yadav@ti.com>; Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de
>> Subject: [PATCH 3/5] mmc: am654_sdhci: Add itap_del_ena[] to store itapdlyena bit
>>
>> Set itap_del_ena if ITAPDLY is found in DT or if the tuning
>> algorithm was executed and found the optimal ITAPDLY. Add the
>> functionality to save ITAPDLYENA that can be referenced later
>> by storing the bit in array itap_del_ena[].
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/am654_sdhci.c | 30 ++++++++++++++++++++----------
>>   1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mmc/am654_sdhci.c b/drivers/mmc/am654_sdhci.c
>> index 1dd032e1e36..38f1ad28ec4 100644
>> --- a/drivers/mmc/am654_sdhci.c
>> +++ b/drivers/mmc/am654_sdhci.c
>> @@ -92,6 +92,7 @@ struct am654_sdhci_plat {
>>   	bool non_removable;
>>   	u32 otap_del_sel[MMC_MODES_END];
>>   	u32 itap_del_sel[MMC_MODES_END];
>> +	u32 itap_del_ena[MMC_MODES_END];
>>   	u32 trm_icp;
>>   	u32 drv_strength;
>>   	u32 strb_sel;
>> @@ -223,8 +224,10 @@ static int am654_sdhci_setup_dll(struct am654_sdhci_plat *plat,
>>   }
>>
>>   static void am654_sdhci_write_itapdly(struct am654_sdhci_plat *plat,
>> -				      u32 itapdly)
>> +				      u32 itapdly, u32 enable)
>>   {
>> +	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYENA_MASK,
>> +			   enable << ITAPDLYENA_SHIFT);
>>   	/* Set ITAPCHGWIN before writing to ITAPDLY */
>>   	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>>   			   1 << ITAPCHGWIN_SHIFT);
>> @@ -242,7 +245,8 @@ static void am654_sdhci_setup_delay_chain(struct am654_sdhci_plat *plat,
>>   	mask = SELDLYTXCLK_MASK | SELDLYRXCLK_MASK;
>>   	regmap_update_bits(plat->base, PHY_CTRL5, mask, val);
>>
>> -	am654_sdhci_write_itapdly(plat, plat->itap_del_sel[mode]);
>> +	am654_sdhci_write_itapdly(plat, plat->itap_del_sel[mode],
>> +				  plat->itap_del_ena[mode]);
>>   }
>>
>>   static int am654_sdhci_set_ios_post(struct sdhci_host *host)
>> @@ -443,6 +447,7 @@ static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>>   	struct udevice *dev = mmc->dev;
>>   	struct am654_sdhci_plat *plat = dev_get_plat(dev);
>>   	struct window fail_window[ITAPDLY_LENGTH];
>> +	int mode = mmc->selected_mode;
>>   	u8 curr_pass, itap;
>>   	u8 fail_index = 0;
>>   	u8 prev_pass = 1;
>> @@ -450,11 +455,10 @@ static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>>   	memset(fail_window, 0, sizeof(fail_window));
>>
>>   	/* Enable ITAPDLY */
>> -	regmap_update_bits(plat->base, PHY_CTRL4, ITAPDLYENA_MASK,
>> -			   1 << ITAPDLYENA_SHIFT);
>> +	plat->itap_del_ena[mode] = 0x1;
> 
> 0x1 means "enable"? I want to use a macro with meaning.

Sure, can do for V2, thanks.
~ Judith

> 
>>
>>   	for (itap = 0; itap < ITAPDLY_LENGTH; itap++) {
>> -		am654_sdhci_write_itapdly(plat, itap);
>> +		am654_sdhci_write_itapdly(plat, itap, plat->itap_del_ena[mode]);
>>
>>   		curr_pass = !mmc_send_tuning(mmc, opcode, NULL);
>>
>> @@ -478,7 +482,7 @@ static int am654_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>>   	itap = am654_sdhci_calculate_itap(dev, fail_window, fail_index,
>>   					  plat->dll_enable);
>>
>> -	am654_sdhci_write_itapdly(plat, itap);
>> +	am654_sdhci_write_itapdly(plat, itap, plat->itap_del_ena[mode]);
>>
>>   	return 0;
>>   }
>> @@ -515,6 +519,7 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
>>   	struct am654_sdhci_plat *plat = dev_get_plat(dev);
>>   	int mode = host->mmc->selected_mode;
>>   	u32 otap_del_sel;
>> +	u32 itap_del_ena;
>>   	u32 itap_del_sel;
>>   	u32 mask, val;
>>
>> @@ -524,10 +529,11 @@ static int j721e_4bit_sdhci_set_ios_post(struct sdhci_host *host)
>>   	val = (1 << OTAPDLYENA_SHIFT) |
>>   	      (otap_del_sel << OTAPDLYSEL_SHIFT);
>>
>> +	itap_del_ena = plat->itap_del_ena[mode];
>>   	itap_del_sel = plat->itap_del_sel[mode];
>>
>>   	mask |= ITAPDLYENA_MASK | ITAPDLYSEL_MASK;
>> -	val = (1 << ITAPDLYENA_SHIFT) |
>> +	val = (itap_del_ena << ITAPDLYENA_SHIFT) |
>>   	      (itap_del_sel << ITAPDLYSEL_SHIFT);
>>
>>   	regmap_update_bits(plat->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>> @@ -599,9 +605,13 @@ static int sdhci_am654_get_otap_delay(struct udevice *dev,
>>   			cfg->host_caps &= ~td[i].capability;
>>   		}
>>
>> -		if (td[i].itap_binding)
>> -			dev_read_u32(dev, td[i].itap_binding,
>> -				     &plat->itap_del_sel[i]);
>> +		if (td[i].itap_binding) {
>> +			ret = dev_read_u32(dev, td[i].itap_binding,
>> +					   &plat->itap_del_sel[i]);
>> +
>> +			if (!ret)
>> +				plat->itap_del_ena[i] = 0x1;
> 
> Ditto.
> 
> Best Regards,
> Jaehoon Chung
> 
>> +		}
>>   	}
>>
>>   	return 0;
>> --
>> 2.43.2
> 
>