[PATCH v1 0/5] Add tuning algorithm for delay chain

[PATCH v1 0/5] Add tuning algorithm for delay chain

[Judith Mendez]

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

[PATCH v1 1/5] mmc: sdhci_am654: 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 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: 13ebeae68ac9 ("mmc: sdhci_am654: Add support for software tuning")
Signed-off-by: Judith Mendez <jm@ti.com>
---
 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..a3798c9912f6 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 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
+			  *fail_window, u8 num_fails, bool circular_buffer)
+{
+	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;
+	u8 i;
+
+	memset(&pass_window, 0, sizeof(pass_window));
+	memset(&first_fail, 0, sizeof(first_fail));
+	memset(&last_fail, 0, sizeof(last_fail));
+
+	if (!num_fails) {
+		itap = ITAPDLY_LAST_INDEX >> 1;
+	} else if (fail_window->length == ITAPDLY_LENGTH) {
+		dev_err(dev, "No passing ITAPDLY, return 0\n");
+		itap = 0;
+	} else {
+		for (i = 0; i < num_fails; i++) {
+			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;
+
+	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++;
+
+	itap = sdhci_am654_calculate_itap(host, fail_window, fail_index,
+					  (sdhci_am654->dll_enable ? true : false));
+
 	sdhci_am654_write_itapdly(sdhci_am654, itap);
 
 	return 0;
-- 
2.34.1

[PATCH v1 2/5] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

[Judith Mendez]

For DDR52 timing, DLL is enabled but tuning is not carried
out, therefore the ITAPDLY value in PHY CTRL 4 register is
not correct. Fix this by writing ITAPDLY after enabling DLL.

Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed modes")
Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/host/sdhci_am654.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index a3798c9912f6..ff18a274b6f2 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -170,7 +170,19 @@ struct sdhci_am654_driver_data {
 #define DLL_CALIB	(1 << 4)
 };
 
-static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned int clock)
+static void sdhci_am654_write_itapdly(struct sdhci_am654_data *sdhci_am654,
+				      u32 itapdly)
+{
+	/* 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, ITAPDLYSEL_MASK,
+			   itapdly << ITAPDLYSEL_SHIFT);
+	regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
+}
+
+static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned int clock,
+				  unsigned char timing)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
@@ -236,17 +248,8 @@ static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned int clock)
 		dev_err(mmc_dev(host->mmc), "DLL failed to relock\n");
 		return;
 	}
-}
 
-static void sdhci_am654_write_itapdly(struct sdhci_am654_data *sdhci_am654,
-				      u32 itapdly)
-{
-	/* Set ITAPCHGWIN before writing to ITAPDLY */
-	regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK,
-			   1 << ITAPCHGWIN_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);
+	sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]);
 }
 
 static void sdhci_am654_setup_delay_chain(struct sdhci_am654_data *sdhci_am654,
@@ -298,7 +301,7 @@ 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) {
-		sdhci_am654_setup_dll(host, clock);
+		sdhci_am654_setup_dll(host, clock, timing);
 		sdhci_am654->dll_enable = true;
 	} else {
 		sdhci_am654_setup_delay_chain(sdhci_am654, timing);
-- 
2.34.1

[PATCH v1 3/5] mmc: sdhci_am654: Add missing OTAP/ITAP enable

[Judith Mendez]

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@ti.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)];
 	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

[PATCH v1 4/5] mmc: sdhci_am654: Add ITAPDLYSEL in sdhci_j721e_4bit_set_clock

[Judith Mendez]

Add ITAPDLYSEL to sdhci_j721e_4bit_set_clock function.
This allows to set the correct ITAPDLY for timings that
do not carry out tuning.

Fixes: 1accbced1c32 ("mmc: sdhci_am654: Add Support for 4 bit IP on J721E")
Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/host/sdhci_am654.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 5ac82bc70706..f5dc981c470d 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -321,6 +321,7 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
 	unsigned char timing = host->mmc->ios.timing;
 	u32 otap_del_sel;
 	u32 itap_del_ena;
+	u32 itap_del_sel;
 	u32 mask, val;
 
 	/* Setup Output TAP delay */
@@ -329,12 +330,17 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
 	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
 	val = (0x1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
 
+	/* Setup Input TAP delay */
 	itap_del_ena = sdhci_am654->itap_del_ena[timing];
+	itap_del_sel = sdhci_am654->itap_del_sel[timing];
 
-	mask |= ITAPDLYENA_MASK;
-	val |= (itap_del_ena << ITAPDLYENA_SHIFT);
+	mask |= ITAPDLYENA_MASK | ITAPDLYSEL_MASK;
+	val |= (itap_del_ena << ITAPDLYENA_SHIFT) | (itap_del_sel << ITAPDLYSEL_SHIFT);
 
+	regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK,
+			   1 << ITAPCHGWIN_SHIFT);
 	regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val);
+	regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
 
 	regmap_update_bits(sdhci_am654->base, PHY_CTRL5, CLKBUFSEL_MASK,
 			   sdhci_am654->clkbuf_sel);
-- 
2.34.1

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

[Judith Mendez]

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 <jm@ti.com>
---
 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

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

[Andrew Davis]

On 1/31/24 3:50 PM, 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
> 
> Fixes: 13ebeae68ac9 ("mmc: sdhci_am654: Add support for software tuning")
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>   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..a3798c9912f6 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 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
> +			  *fail_window, u8 num_fails, bool circular_buffer)
> +{
> +	struct device *dev = mmc_dev(host->mmc);
> +	struct window pass_window, first_fail, last_fail;

struct window pass_window = {}, ..

Then you can drop the memset()s below.

> +	u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0;
> +	int prev_end_fail = -1;
> +	u8 i;
> +
> +	memset(&pass_window, 0, sizeof(pass_window));
> +	memset(&first_fail, 0, sizeof(first_fail));
> +	memset(&last_fail, 0, sizeof(last_fail));
> +
> +	if (!num_fails) {
> +		itap = ITAPDLY_LAST_INDEX >> 1;

return ITAPDLY_LAST_INDEX >> 1;

> +	} else if (fail_window->length == ITAPDLY_LENGTH) {
> +		dev_err(dev, "No passing ITAPDLY, return 0\n");
> +		itap = 0;

return 0;

> +	} else {

If you shortcut return directly in the above to branches, then
this all below doesn't need to be in the else {} and you won't
have to indent it all out so far.

> +		for (i = 0; i < num_fails; i++) {
> +			start_fail = fail_window[i].start;
> +			end_fail = fail_window[i].end;
> +
> +			if (i == 0) {

Move this first case to before the loop, we already know what
first_fail will be filled with. No need to check i == 0 every iteration
of the loop. Same for last_fail, just move to after the loop.

> +				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;
> +
> +	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++;
> +
> +	itap = sdhci_am654_calculate_itap(host, fail_window, fail_index,
> +					  (sdhci_am654->dll_enable ? true : false));

dll_enable is already a bool, the line:

(sdhci_am654->dll_enable ? true : false)

has no effect, just use sdhci_am654->dll_enable directly.

Andrew

> +
>   	sdhci_am654_write_itapdly(sdhci_am654, itap);
>   
>   	return 0;

Re: [PATCH v1 2/5] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

[Andrew Davis]

On 1/31/24 3:50 PM, Judith Mendez wrote:
> For DDR52 timing, DLL is enabled but tuning is not carried
> out, therefore the ITAPDLY value in PHY CTRL 4 register is
> not correct. Fix this by writing ITAPDLY after enabling DLL.
> 
> Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed modes")
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>   drivers/mmc/host/sdhci_am654.c | 27 +++++++++++++++------------
>   1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index a3798c9912f6..ff18a274b6f2 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -170,7 +170,19 @@ struct sdhci_am654_driver_data {
>   #define DLL_CALIB	(1 << 4)
>   };
>   
> -static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned int clock)
> +static void sdhci_am654_write_itapdly(struct sdhci_am654_data *sdhci_am654,
> +				      u32 itapdly)

This patch is confusing, looks like you switched the place of these two
functions, but diff is not really liking that. You can mess with
--diff-algorithm and the like to get a more readable patch. But in
this case why switch their spots at all?

Seems to be so you can call sdhci_am654_write_itapdly() from
sdhci_am654_setup_dll() without a forward declaration, instead
why not just call sdhci_am654_write_itapdly() after calling
sdhci_am654_setup_dll() below. That also saves to from having
to pass in `timing` to sdhci_am654_write_itapdly() just to
have it pass it right through to sdhci_am654_setup_dll().

Andrew

> +{
> +	/* 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, ITAPDLYSEL_MASK,
> +			   itapdly << ITAPDLYSEL_SHIFT);
> +	regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
> +}
> +
> +static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned int clock,
> +				  unsigned char timing)
>   {
>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>   	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> @@ -236,17 +248,8 @@ static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned int clock)
>   		dev_err(mmc_dev(host->mmc), "DLL failed to relock\n");
>   		return;
>   	}
> -}
>   
> -static void sdhci_am654_write_itapdly(struct sdhci_am654_data *sdhci_am654,
> -				      u32 itapdly)
> -{
> -	/* Set ITAPCHGWIN before writing to ITAPDLY */
> -	regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK,
> -			   1 << ITAPCHGWIN_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);
> +	sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]);
>   }
>   
>   static void sdhci_am654_setup_delay_chain(struct sdhci_am654_data *sdhci_am654,
> @@ -298,7 +301,7 @@ 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) {
> -		sdhci_am654_setup_dll(host, clock);
> +		sdhci_am654_setup_dll(host, clock, timing);
>   		sdhci_am654->dll_enable = true;
>   	} else {
>   		sdhci_am654_setup_delay_chain(sdhci_am654, timing);

Re: [PATCH v1 3/5] mmc: sdhci_am654: Add missing OTAP/ITAP enable

[Andrew Davis]

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@ti.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;

Re: [PATCH v1 4/5] mmc: sdhci_am654: Add ITAPDLYSEL in sdhci_j721e_4bit_set_clock

[Andrew Davis]

On 1/31/24 3:50 PM, Judith Mendez wrote:
> Add ITAPDLYSEL to sdhci_j721e_4bit_set_clock function.
> This allows to set the correct ITAPDLY for timings that
> do not carry out tuning.
> 
> Fixes: 1accbced1c32 ("mmc: sdhci_am654: Add Support for 4 bit IP on J721E")

You are adding this as a new feature, and not having a feature doesn't mean
the initial patch was broken. If this patch was backported to kernels only
containing the above patch it would cause more issues, so no need for the
fixes tags on this nor the last patch.

Andrew

> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>   drivers/mmc/host/sdhci_am654.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 5ac82bc70706..f5dc981c470d 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -321,6 +321,7 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>   	unsigned char timing = host->mmc->ios.timing;
>   	u32 otap_del_sel;
>   	u32 itap_del_ena;
> +	u32 itap_del_sel;
>   	u32 mask, val;
>   
>   	/* Setup Output TAP delay */
> @@ -329,12 +330,17 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>   	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
>   	val = (0x1 << OTAPDLYENA_SHIFT) | (otap_del_sel << OTAPDLYSEL_SHIFT);
>   
> +	/* Setup Input TAP delay */
>   	itap_del_ena = sdhci_am654->itap_del_ena[timing];
> +	itap_del_sel = sdhci_am654->itap_del_sel[timing];
>   
> -	mask |= ITAPDLYENA_MASK;
> -	val |= (itap_del_ena << ITAPDLYENA_SHIFT);
> +	mask |= ITAPDLYENA_MASK | ITAPDLYSEL_MASK;
> +	val |= (itap_del_ena << ITAPDLYENA_SHIFT) | (itap_del_sel << ITAPDLYSEL_SHIFT);
>   
> +	regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK,
> +			   1 << ITAPCHGWIN_SHIFT);
>   	regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val);
> +	regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK, 0);
>   
>   	regmap_update_bits(sdhci_am654->base, PHY_CTRL5, CLKBUFSEL_MASK,
>   			   sdhci_am654->clkbuf_sel);

Re: [PATCH v1 4/5] mmc: sdhci_am654: Add ITAPDLYSEL in sdhci_j721e_4bit_set_clock

[Judith Mendez]

Hi Andrew,

On 2/1/24 1:57 PM, Andrew Davis wrote:
> On 1/31/24 3:50 PM, Judith Mendez wrote:
>> Add ITAPDLYSEL to sdhci_j721e_4bit_set_clock function.
>> This allows to set the correct ITAPDLY for timings that
>> do not carry out tuning.
>>
>> Fixes: 1accbced1c32 ("mmc: sdhci_am654: Add Support for 4 bit IP on 
>> J721E")
> 
> You are adding this as a new feature, and not having a feature doesn't mean
> the initial patch was broken. If this patch was backported to kernels only
> containing the above patch it would cause more issues, so no need for the
> fixes tags on this nor the last patch.
> 

Sure, will fix, thanks.

>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c 
>> b/drivers/mmc/host/sdhci_am654.c
>> index 5ac82bc70706..f5dc981c470d 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -321,6 +321,7 @@ static void sdhci_j721e_4bit_set_clock(struct 
>> sdhci_host *host,
>>       unsigned char timing = host->mmc->ios.timing;
>>       u32 otap_del_sel;
>>       u32 itap_del_ena;
>> +    u32 itap_del_sel;
>>       u32 mask, val;
>>       /* Setup Output TAP delay */
>> @@ -329,12 +330,17 @@ static void sdhci_j721e_4bit_set_clock(struct 
>> sdhci_host *host,
>>       mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
>>       val = (0x1 << OTAPDLYENA_SHIFT) | (otap_del_sel << 
>> OTAPDLYSEL_SHIFT);
>> +    /* Setup Input TAP delay */
>>       itap_del_ena = sdhci_am654->itap_del_ena[timing];
>> +    itap_del_sel = sdhci_am654->itap_del_sel[timing];
>> -    mask |= ITAPDLYENA_MASK;
>> -    val |= (itap_del_ena << ITAPDLYENA_SHIFT);
>> +    mask |= ITAPDLYENA_MASK | ITAPDLYSEL_MASK;
>> +    val |= (itap_del_ena << ITAPDLYENA_SHIFT) | (itap_del_sel << 
>> ITAPDLYSEL_SHIFT);
>> +    regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>> +               1 << ITAPCHGWIN_SHIFT);
>>       regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val);
>> +    regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK, 
>> 0);
>>       regmap_update_bits(sdhci_am654->base, PHY_CTRL5, CLKBUFSEL_MASK,
>>                  sdhci_am654->clkbuf_sel);

~ Judith

Re: [PATCH v1 4/5] mmc: sdhci_am654: Add ITAPDLYSEL in sdhci_j721e_4bit_set_clock

[Vignesh Raghavendra]

On 02/02/24 01:27, Andrew Davis wrote:
> On 1/31/24 3:50 PM, Judith Mendez wrote:
>> Add ITAPDLYSEL to sdhci_j721e_4bit_set_clock function.
>> This allows to set the correct ITAPDLY for timings that
>> do not carry out tuning.
>>
>> Fixes: 1accbced1c32 ("mmc: sdhci_am654: Add Support for 4 bit IP on
>> J721E")
> 
> You are adding this as a new feature, and not having a feature doesn't mean
> the initial patch was broken. If this patch was backported to kernels only
> containing the above patch it would cause more issues, so no need for the
> fixes tags on this nor the last patch.
> 

Not really a new features. Devices Datasheets have always been clear
that static ITAPDLY needs to be configured when tuning isn't performed.
Hence a bug as the initial patch (Fixes line) does enable such
(affected) modes where tuning isn't performed but ITAPDLY isn't set either.

> Andrew
> 
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c
>> b/drivers/mmc/host/sdhci_am654.c
>> index 5ac82bc70706..f5dc981c470d 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -321,6 +321,7 @@ static void sdhci_j721e_4bit_set_clock(struct
>> sdhci_host *host,
>>       unsigned char timing = host->mmc->ios.timing;
>>       u32 otap_del_sel;
>>       u32 itap_del_ena;
>> +    u32 itap_del_sel;
>>       u32 mask, val;
>>         /* Setup Output TAP delay */
>> @@ -329,12 +330,17 @@ static void sdhci_j721e_4bit_set_clock(struct
>> sdhci_host *host,
>>       mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
>>       val = (0x1 << OTAPDLYENA_SHIFT) | (otap_del_sel <<
>> OTAPDLYSEL_SHIFT);
>>   +    /* Setup Input TAP delay */
>>       itap_del_ena = sdhci_am654->itap_del_ena[timing];
>> +    itap_del_sel = sdhci_am654->itap_del_sel[timing];
>>   -    mask |= ITAPDLYENA_MASK;
>> -    val |= (itap_del_ena << ITAPDLYENA_SHIFT);
>> +    mask |= ITAPDLYENA_MASK | ITAPDLYSEL_MASK;
>> +    val |= (itap_del_ena << ITAPDLYENA_SHIFT) | (itap_del_sel <<
>> ITAPDLYSEL_SHIFT);
>>   +    regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>> +               1 << ITAPCHGWIN_SHIFT);
>>       regmap_update_bits(sdhci_am654->base, PHY_CTRL4, mask, val);
>> +    regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>> 0);
>>         regmap_update_bits(sdhci_am654->base, PHY_CTRL5, CLKBUFSEL_MASK,
>>                  sdhci_am654->clkbuf_sel);

-- 
Regards
Vignesh

Re: [PATCH v1 2/5] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

[Judith Mendez]

Hi Andrew,

On 2/1/24 1:36 PM, Andrew Davis wrote:
> On 1/31/24 3:50 PM, Judith Mendez wrote:
>> For DDR52 timing, DLL is enabled but tuning is not carried
>> out, therefore the ITAPDLY value in PHY CTRL 4 register is
>> not correct. Fix this by writing ITAPDLY after enabling DLL.
>>
>> Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed 
>> modes")
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 27 +++++++++++++++------------
>>   1 file changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c 
>> b/drivers/mmc/host/sdhci_am654.c
>> index a3798c9912f6..ff18a274b6f2 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -170,7 +170,19 @@ struct sdhci_am654_driver_data {
>>   #define DLL_CALIB    (1 << 4)
>>   };
>> -static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned 
>> int clock)
>> +static void sdhci_am654_write_itapdly(struct sdhci_am654_data 
>> *sdhci_am654,
>> +                      u32 itapdly)
> 
> This patch is confusing, looks like you switched the place of these two
> functions, but diff is not really liking that. You can mess with
> --diff-algorithm and the like to get a more readable patch. But in
> this case why switch their spots at all?
> 
> Seems to be so you can call sdhci_am654_write_itapdly() from
> sdhci_am654_setup_dll() without a forward declaration, instead
> why not just call sdhci_am654_write_itapdly() after calling
> sdhci_am654_setup_dll() below. That also saves to from having
> to pass in `timing` to sdhci_am654_write_itapdly() just to
> have it pass it right through to sdhci_am654_setup_dll().

Really the only reason I did this is because we call
sdhci_am654_write_itapdly() in sdhci_am654_setup_delay_chain and
I wanted to keep the flow for setting up DLL the same.
I agree the patch looks confusing, so I will fix this for v2.

~ Judith

> Andrew
> 
>> +{
>> +    /* 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, ITAPDLYSEL_MASK,
>> +               itapdly << ITAPDLYSEL_SHIFT);
>> +    regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK, 
>> 0);
>> +}
>> +
>> +static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned 
>> int clock,
>> +                  unsigned char timing)
>>   {
>>       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>       struct sdhci_am654_data *sdhci_am654 = 
>> sdhci_pltfm_priv(pltfm_host);
>> @@ -236,17 +248,8 @@ static void sdhci_am654_setup_dll(struct 
>> sdhci_host *host, unsigned int clock)
>>           dev_err(mmc_dev(host->mmc), "DLL failed to relock\n");
>>           return;
>>       }
>> -}
>> -static void sdhci_am654_write_itapdly(struct sdhci_am654_data 
>> *sdhci_am654,
>> -                      u32 itapdly)
>> -{
>> -    /* Set ITAPCHGWIN before writing to ITAPDLY */
>> -    regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>> -               1 << ITAPCHGWIN_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);
>> +    sdhci_am654_write_itapdly(sdhci_am654, 
>> sdhci_am654->itap_del_sel[timing]);
>>   }
>>   static void sdhci_am654_setup_delay_chain(struct sdhci_am654_data 
>> *sdhci_am654,
>> @@ -298,7 +301,7 @@ 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) {
>> -        sdhci_am654_setup_dll(host, clock);
>> +        sdhci_am654_setup_dll(host, clock, timing);
>>           sdhci_am654->dll_enable = true;
>>       } else {
>>           sdhci_am654_setup_delay_chain(sdhci_am654, timing);

Re: [PATCH v1 3/5] mmc: sdhci_am654: Add missing OTAP/ITAP enable

[Judith Mendez]

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@ti.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.

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;

Re: [PATCH v1 2/5] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

[Judith Mendez]

On 2/6/24 3:58 PM, Judith Mendez wrote:
> Hi Andrew,
> 
> On 2/1/24 1:36 PM, Andrew Davis wrote:
>> On 1/31/24 3:50 PM, Judith Mendez wrote:
>>> For DDR52 timing, DLL is enabled but tuning is not carried
>>> out, therefore the ITAPDLY value in PHY CTRL 4 register is
>>> not correct. Fix this by writing ITAPDLY after enabling DLL.
>>>
>>> Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some 
>>> speed modes")
>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>> ---
>>>   drivers/mmc/host/sdhci_am654.c | 27 +++++++++++++++------------
>>>   1 file changed, 15 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci_am654.c 
>>> b/drivers/mmc/host/sdhci_am654.c
>>> index a3798c9912f6..ff18a274b6f2 100644
>>> --- a/drivers/mmc/host/sdhci_am654.c
>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>> @@ -170,7 +170,19 @@ struct sdhci_am654_driver_data {
>>>   #define DLL_CALIB    (1 << 4)
>>>   };
>>> -static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned 
>>> int clock)
>>> +static void sdhci_am654_write_itapdly(struct sdhci_am654_data 
>>> *sdhci_am654,
>>> +                      u32 itapdly)
>>
>> This patch is confusing, looks like you switched the place of these two
>> functions, but diff is not really liking that. You can mess with
>> --diff-algorithm and the like to get a more readable patch. But in
>> this case why switch their spots at all?
>>
>> Seems to be so you can call sdhci_am654_write_itapdly() from
>> sdhci_am654_setup_dll() without a forward declaration, instead
>> why not just call sdhci_am654_write_itapdly() after calling
>> sdhci_am654_setup_dll() below. That also saves to from having
>> to pass in `timing` to sdhci_am654_write_itapdly() just to
>> have it pass it right through to sdhci_am654_setup_dll().
> 
> Really the only reason I did this is because we call
> sdhci_am654_write_itapdly() in sdhci_am654_setup_delay_chain and
> I wanted to keep the flow for setting up DLL the same.
> I agree the patch looks confusing, so I will fix this for v2.

TBH I think it is a good idea to keep the flow the same as it
is for sdhci_am654_setup_delay_chain(). Unless you know of a
strong enough reason to change, I am leaning towards leaving the
patch as is.

> 
> ~ Judith
> 
>> Andrew
>>
>>> +{
>>> +    /* 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, ITAPDLYSEL_MASK,
>>> +               itapdly << ITAPDLYSEL_SHIFT);
>>> +    regmap_update_bits(sdhci_am654->base, PHY_CTRL4, 
>>> ITAPCHGWIN_MASK, 0);
>>> +}
>>> +
>>> +static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned 
>>> int clock,
>>> +                  unsigned char timing)
>>>   {
>>>       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>       struct sdhci_am654_data *sdhci_am654 = 
>>> sdhci_pltfm_priv(pltfm_host);
>>> @@ -236,17 +248,8 @@ static void sdhci_am654_setup_dll(struct 
>>> sdhci_host *host, unsigned int clock)
>>>           dev_err(mmc_dev(host->mmc), "DLL failed to relock\n");
>>>           return;
>>>       }
>>> -}
>>> -static void sdhci_am654_write_itapdly(struct sdhci_am654_data 
>>> *sdhci_am654,
>>> -                      u32 itapdly)
>>> -{
>>> -    /* Set ITAPCHGWIN before writing to ITAPDLY */
>>> -    regmap_update_bits(sdhci_am654->base, PHY_CTRL4, ITAPCHGWIN_MASK,
>>> -               1 << ITAPCHGWIN_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);
>>> +    sdhci_am654_write_itapdly(sdhci_am654, 
>>> sdhci_am654->itap_del_sel[timing]);
>>>   }
>>>   static void sdhci_am654_setup_delay_chain(struct sdhci_am654_data 
>>> *sdhci_am654,
>>> @@ -298,7 +301,7 @@ 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) {
>>> -        sdhci_am654_setup_dll(host, clock);
>>> +        sdhci_am654_setup_dll(host, clock, timing);
>>>           sdhci_am654->dll_enable = true;
>>>       } else {
>>>           sdhci_am654_setup_delay_chain(sdhci_am654, timing);
> 
> 

Re: [PATCH v1 3/5] mmc: sdhci_am654: Add missing OTAP/ITAP enable

[Andrew Davis]

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 <jm@ti.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.
> 
> 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;
>