[PATCH v2 0/7] Add tuning algorithm for delay chain

[PATCH v2 0/7] 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.

Changelog:
v1->v2:
- Remove unnecessary indentations and if/else in
 sdhci_am654_calculate_itap
- Optimize sdhci_am654_calculate_itap()
- Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock()
 instead of sdhci_am654_setup_dll()
- Change otap_del_sel[], itap_del_sel[], and itap_del_ena[]
 to type u32
- Revert unnecessary reformating in sdhci_am654_set_clock()
 and sdhci_j721e_4bit_set_clock()

Judith Mendez (7):
  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: Fix itapdly/otapdly array type
  mmc: sdhci_am654: Update comments in sdhci_am654_set_clock
  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 | 176 ++++++++++++++++++++++++++-------
 1 file changed, 138 insertions(+), 38 deletions(-)


base-commit: 40d83f40c44ba8aa79cba409ace619db3a7f86f2
-- 
2.43.0

[PATCH v2 1/7] 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>
---
Changelog:
v1->v2:
- Remove unnecessary indentations and if/else in
 sdhci_am654_calculate_itap()
- Optimize sdhci_am654_calculate_itap()
---
 drivers/mmc/host/sdhci_am654.c | 111 +++++++++++++++++++++++++++------
 1 file changed, 91 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index d659c59422e1..2c66a965c225 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,100 @@ 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 = {0, 0, 0};
+	u8 first_fail_start = 0, last_fail_end = 0;
+	int prev_fail_end = -1;
+	u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0;
+	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 < 0 || itap > ITAPDLY_LAST_INDEX ? 0 : 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));
+
 	sdhci_am654_write_itapdly(sdhci_am654, itap);
 
 	return 0;
-- 
2.43.0

Re: [PATCH v2 1/7] mmc: sdhci_am654: Add tuning algorithm for delay chain

[Adrian Hunter]

On 7/02/24 03:15, 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>
> ---
> Changelog:
> v1->v2:
> - Remove unnecessary indentations and if/else in
>  sdhci_am654_calculate_itap()
> - Optimize sdhci_am654_calculate_itap()
> ---
>  drivers/mmc/host/sdhci_am654.c | 111 +++++++++++++++++++++++++++------
>  1 file changed, 91 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index d659c59422e1..2c66a965c225 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,100 @@ 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

Presumably ITAPDLY_LAST_INDEX == ITAPDLY_LENGTH - 1, so perhaps:

#define ITAPDLY_LAST_INDEX (ITAPDLY_LENGTH - 1)

Blank line here perhaps.

> +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 = {0, 0, 0};
> +	u8 first_fail_start = 0, last_fail_end = 0;
> +	int prev_fail_end = -1;
> +	u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0;
> +	u8 i;

Some prefer ordering of variable declarations at the beginning of a
function to be "reverse fir tree" order, in other words, longer lines
first, e.g.

	u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0;
	u8 first_fail_start = 0, last_fail_end = 0;
	struct device *dev = mmc_dev(host->mmc);
	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 < 0 || itap > ITAPDLY_LAST_INDEX ? 0 : itap);

Parentheses are not needed where they are but putting
them around the condition would make it more readable e.g.

	return (itap < 0 || itap > ITAPDLY_LAST_INDEX) ? 0 : itap;

However (itap < 0) is not possible because itap is an unsigned type
and if (itap > ITAPDLY_LAST_INDEX) then maybe it would be better
to return ITAPDLY_LAST_INDEX

> +}
> +
>  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;

Perhaps reverse fir tree

> +
> +	memset(fail_window, 0, sizeof(fail_window[0]) * ITAPDLY_LENGTH);

This can be:

	memset(fail_window, 0, sizeof(fail_window));

>  
>  	/* 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));

Parentheses around sdhci_am654->dll_enable are not needed.

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

Re: [PATCH v2 1/7] mmc: sdhci_am654: Add tuning algorithm for delay chain

[Judith Mendez]

Hi Adrian,

On 2/16/24 11:09 AM, Adrian Hunter wrote:
> On 7/02/24 03:15, 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>
>> ---
>> Changelog:
>> v1->v2:
>> - Remove unnecessary indentations and if/else in
>>   sdhci_am654_calculate_itap()
>> - Optimize sdhci_am654_calculate_itap()
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 111 +++++++++++++++++++++++++++------
>>   1 file changed, 91 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index d659c59422e1..2c66a965c225 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,100 @@ 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
> 
> Presumably ITAPDLY_LAST_INDEX == ITAPDLY_LENGTH - 1, so perhaps:
> 
> #define ITAPDLY_LAST_INDEX (ITAPDLY_LENGTH - 1)
> 
> Blank line here perhaps.

This does seem easier to read, will add for v3.

> 
>> +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 = {0, 0, 0};
>> +	u8 first_fail_start = 0, last_fail_end = 0;
>> +	int prev_fail_end = -1;
>> +	u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0;
>> +	u8 i;
> 
> Some prefer ordering of variable declarations at the beginning of a
> function to be "reverse fir tree" order, in other words, longer lines
> first, e.g.
> 
> 	u8 itap = 0, start_fail = 0, end_fail = 0, pass_length = 0;
> 	u8 first_fail_start = 0, last_fail_end = 0;
> 	struct device *dev = mmc_dev(host->mmc);
> 	struct window pass_window = {0, 0, 0};
> 	int prev_fail_end = -1;
> 	u8 i;

Understood, will add for v3.


> 
>> +
>> +	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 < 0 || itap > ITAPDLY_LAST_INDEX ? 0 : itap);
> 
> Parentheses are not needed where they are but putting
> them around the condition would make it more readable e.g.
> 
> 	return (itap < 0 || itap > ITAPDLY_LAST_INDEX) ? 0 : itap;
> 
> However (itap < 0) is not possible because itap is an unsigned type
> and if (itap > ITAPDLY_LAST_INDEX) then maybe it would be better
> to return ITAPDLY_LAST_INDEX

You are right about itap < 0, thanks will fix.

About itap > ITAPDLY_LAST_INDEX, this is an error. Why
return ITAPDLY_LAST_INDEX instead of 0?


> 
>> +}
>> +
>>   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;
> 
> Perhaps reverse fir tree

Will add fix here as well.

> 
>> +
>> +	memset(fail_window, 0, sizeof(fail_window[0]) * ITAPDLY_LENGTH);
> 
> This can be:
> 
> 	memset(fail_window, 0, sizeof(fail_window));

This does look simpler, will add for v3.

> 
>>   
>>   	/* 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));
> 
> Parentheses around sdhci_am654->dll_enable are not needed.

Agree, I can remove for v3.

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


My apologies for the late reply and thanks for reviewing.

~ Judith

Re: [PATCH v2 1/7] mmc: sdhci_am654: Add tuning algorithm for delay chain

[Adrian Hunter]

On 20/02/24 22:10, Judith Mendez wrote:
> On 2/16/24 11:09 AM, Adrian Hunter wrote:
>> On 7/02/24 03:15, Judith Mendez wrote:
>>> +
>>> +    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 < 0 || itap > ITAPDLY_LAST_INDEX ? 0 : itap);
>>
>> Parentheses are not needed where they are but putting
>> them around the condition would make it more readable e.g.
>>
>>     return (itap < 0 || itap > ITAPDLY_LAST_INDEX) ? 0 : itap;
>>
>> However (itap < 0) is not possible because itap is an unsigned type
>> and if (itap > ITAPDLY_LAST_INDEX) then maybe it would be better
>> to return ITAPDLY_LAST_INDEX
> 
> You are right about itap < 0, thanks will fix.
> 
> About itap > ITAPDLY_LAST_INDEX, this is an error. Why
> return ITAPDLY_LAST_INDEX instead of 0?

It doesn't matter.  Just if a value has a better chance to work
if the calculation fails, like maybe ITAPDLY_LAST_INDEX / 2, but
presumably it should not fail.

Re: [PATCH v2 1/7] mmc: sdhci_am654: Add tuning algorithm for delay chain

[Judith Mendez]

Hello Adrian,

On 2/28/24 7:21 AM, Adrian Hunter wrote:
> On 20/02/24 22:10, Judith Mendez wrote:
>> On 2/16/24 11:09 AM, Adrian Hunter wrote:
>>> On 7/02/24 03:15, Judith Mendez wrote:
>>>> +
>>>> +    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 < 0 || itap > ITAPDLY_LAST_INDEX ? 0 : itap);
>>>
>>> Parentheses are not needed where they are but putting
>>> them around the condition would make it more readable e.g.
>>>
>>>      return (itap < 0 || itap > ITAPDLY_LAST_INDEX) ? 0 : itap;
>>>
>>> However (itap < 0) is not possible because itap is an unsigned type
>>> and if (itap > ITAPDLY_LAST_INDEX) then maybe it would be better
>>> to return ITAPDLY_LAST_INDEX
>>
>> You are right about itap < 0, thanks will fix.
>>
>> About itap > ITAPDLY_LAST_INDEX, this is an error. Why
>> return ITAPDLY_LAST_INDEX instead of 0?
> 
> It doesn't matter.  Just if a value has a better chance to work
> if the calculation fails, like maybe ITAPDLY_LAST_INDEX / 2, but
> presumably it should not fail.

Ok, ITAPDLY_LAST_INDEX / sounds good to me, I will add this instead.

Thanks,
~ Judith

[PATCH v2 2/7] 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>
---
Changelog:
v1->v2:
- Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock()
 instead of sdhci_am654_setup_dll()
---
 drivers/mmc/host/sdhci_am654.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 2c66a965c225..b50db5d4a452 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) {
 		sdhci_am654_setup_dll(host, clock);
+		sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]);
 		sdhci_am654->dll_enable = true;
 	} else {
 		sdhci_am654_setup_delay_chain(sdhci_am654, timing);
-- 
2.43.0

Re: [PATCH v2 2/7] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

[Andrew Davis]

On 2/6/24 7:15 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>
> ---
> Changelog:
> v1->v2:
> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock()
>   instead of sdhci_am654_setup_dll()
> ---
>   drivers/mmc/host/sdhci_am654.c | 1 +
>   1 file changed, 1 insertion(+)

See how much easier this patch is this way :)

Reviewed-by: Andrew Davis <afd@ti.com>

> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 2c66a965c225..b50db5d4a452 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
>   
>   	if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) {
>   		sdhci_am654_setup_dll(host, clock);
> +		sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]);
>   		sdhci_am654->dll_enable = true;
>   	} else {
>   		sdhci_am654_setup_delay_chain(sdhci_am654, timing);

Re: [PATCH v2 2/7] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

[Judith Mendez]

Hi Andrew,

On 2/12/24 11:13 AM, Andrew Davis wrote:
> On 2/6/24 7:15 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>
>> ---
>> Changelog:
>> v1->v2:
>> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock()
>>   instead of sdhci_am654_setup_dll()
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 1 +
>>   1 file changed, 1 insertion(+)
> 
> See how much easier this patch is this way :)

Thanks for your review. :D It does look simpler.

> 
> Reviewed-by: Andrew Davis <afd@ti.com>
> 
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c 
>> b/drivers/mmc/host/sdhci_am654.c
>> index 2c66a965c225..b50db5d4a452 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct 
>> sdhci_host *host, unsigned int clock)
>>       if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) {
>>           sdhci_am654_setup_dll(host, clock);
>> +        sdhci_am654_write_itapdly(sdhci_am654, 
>> sdhci_am654->itap_del_sel[timing]);
>>           sdhci_am654->dll_enable = true;
>>       } else {
>>           sdhci_am654_setup_delay_chain(sdhci_am654, timing);

Re: [PATCH v2 2/7] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

[Adrian Hunter]

On 7/02/24 03:15, 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")

Note that the Fixes tags make a different ordering
than the patch order i.e.

Patch	Fixes		in version
1 	13ebeae68ac9 	v5.10-rc1
2 	a161c45f2979 	v5.7-rc1
3 	8ee5fc0e0b3b 	v5.7-rc1
4 	8ee5fc0e0b3b 	v5.7-rc1
4 	a0a62497f6aa 	v5.10-rc1
5 	fe52e2fbc6ef 	v5.9-rc1
6 	1accbced1c32 	v5.3-rc1
7 	a161c45f2979 	v5.7-rc1

That might make backporting these patches more challenging.

> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
> Changelog:
> v1->v2:
> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock()
>  instead of sdhci_am654_setup_dll()
> ---
>  drivers/mmc/host/sdhci_am654.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 2c66a965c225..b50db5d4a452 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
>  
>  	if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) {
>  		sdhci_am654_setup_dll(host, clock);
> +		sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]);
>  		sdhci_am654->dll_enable = true;
>  	} else {
>  		sdhci_am654_setup_delay_chain(sdhci_am654, timing);

Re: [PATCH v2 2/7] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

[Judith Mendez]

Hi Adrian,

On 2/16/24 11:09 AM, Adrian Hunter wrote:
> On 7/02/24 03:15, 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")
> 
> Note that the Fixes tags make a different ordering
> than the patch order i.e.
> 
> Patch	Fixes		in version
> 1 	13ebeae68ac9 	v5.10-rc1
> 2 	a161c45f2979 	v5.7-rc1
> 3 	8ee5fc0e0b3b 	v5.7-rc1
> 4 	8ee5fc0e0b3b 	v5.7-rc1
> 4 	a0a62497f6aa 	v5.10-rc1
> 5 	fe52e2fbc6ef 	v5.9-rc1
> 6 	1accbced1c32 	v5.3-rc1
> 7 	a161c45f2979 	v5.7-rc1
> 
> That might make backporting these patches more challenging.

Are you suggesting to remove the fixes tag here?

> 
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>> Changelog:
>> v1->v2:
>> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock()
>>   instead of sdhci_am654_setup_dll()
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index 2c66a965c225..b50db5d4a452 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
>>   
>>   	if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) {
>>   		sdhci_am654_setup_dll(host, clock);
>> +		sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]);
>>   		sdhci_am654->dll_enable = true;
>>   	} else {
>>   		sdhci_am654_setup_delay_chain(sdhci_am654, timing);
> 

~ Judith

Re: [PATCH v2 2/7] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

[Adrian Hunter]

On 20/02/24 23:05, Judith Mendez wrote:
> Hi Adrian,
> 
> On 2/16/24 11:09 AM, Adrian Hunter wrote:
>> On 7/02/24 03:15, 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")
>>
>> Note that the Fixes tags make a different ordering
>> than the patch order i.e.
>>
>> Patch    Fixes        in version
>> 1     13ebeae68ac9     v5.10-rc1
>> 2     a161c45f2979     v5.7-rc1
>> 3     8ee5fc0e0b3b     v5.7-rc1
>> 4     8ee5fc0e0b3b     v5.7-rc1
>> 4     a0a62497f6aa     v5.10-rc1
>> 5     fe52e2fbc6ef     v5.9-rc1
>> 6     1accbced1c32     v5.3-rc1
>> 7     a161c45f2979     v5.7-rc1
>>
>> That might make backporting these patches more challenging.
> 
> Are you suggesting to remove the fixes tag here?

No, it is just something to think about if you intend to
backport these patches to older kernels.

> 
>>
>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>> ---
>>> Changelog:
>>> v1->v2:
>>> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock()
>>>   instead of sdhci_am654_setup_dll()
>>> ---
>>>   drivers/mmc/host/sdhci_am654.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>> index 2c66a965c225..b50db5d4a452 100644
>>> --- a/drivers/mmc/host/sdhci_am654.c
>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
>>>         if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) {
>>>           sdhci_am654_setup_dll(host, clock);
>>> +        sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]);
>>>           sdhci_am654->dll_enable = true;
>>>       } else {
>>>           sdhci_am654_setup_delay_chain(sdhci_am654, timing);
>>
> 
> ~ Judith

Re: [PATCH v2 2/7] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

[Judith Mendez]

Hi Adrian,

On 2/28/24 7:21 AM, Adrian Hunter wrote:
> On 20/02/24 23:05, Judith Mendez wrote:
>> Hi Adrian,
>>
>> On 2/16/24 11:09 AM, Adrian Hunter wrote:
>>> On 7/02/24 03:15, 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")
>>>
>>> Note that the Fixes tags make a different ordering
>>> than the patch order i.e.
>>>
>>> Patch    Fixes        in version
>>> 1     13ebeae68ac9     v5.10-rc1
>>> 2     a161c45f2979     v5.7-rc1
>>> 3     8ee5fc0e0b3b     v5.7-rc1
>>> 4     8ee5fc0e0b3b     v5.7-rc1
>>> 4     a0a62497f6aa     v5.10-rc1
>>> 5     fe52e2fbc6ef     v5.9-rc1
>>> 6     1accbced1c32     v5.3-rc1
>>> 7     a161c45f2979     v5.7-rc1
>>>
>>> That might make backporting these patches more challenging.
>>
>> Are you suggesting to remove the fixes tag here?
> 
> No, it is just something to think about if you intend to
> backport these patches to older kernels.

Thanks, Ill keep this in mind for v3.

> 
>>
>>>
>>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>>> ---
>>>> Changelog:
>>>> v1->v2:
>>>> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock()
>>>>    instead of sdhci_am654_setup_dll()
>>>> ---
>>>>    drivers/mmc/host/sdhci_am654.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>>> index 2c66a965c225..b50db5d4a452 100644
>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
>>>>          if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) {
>>>>            sdhci_am654_setup_dll(host, clock);
>>>> +        sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]);
>>>>            sdhci_am654->dll_enable = true;
>>>>        } else {
>>>>            sdhci_am654_setup_delay_chain(sdhci_am654, timing);
>>>
>>
>> ~ Judith
> 

[PATCH v2 3/7] 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>
---
Changelog:
v1->v2:
- Change itap_del_ena[] to type u32
- Revert unnecessary reformating in sdhci_am654_set_clock()
 and sdhci_j721e_4bit_set_clock()
---
 drivers/mmc/host/sdhci_am654.c | 40 ++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index b50db5d4a452..935f581c05d8 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)];
+	u32 itap_del_ena[ARRAY_SIZE(td)];
 	int clkbuf_sel;
 	int trm_icp;
 	int drv_strength;
@@ -239,11 +240,13 @@ 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)
+				      u32 itapdly, u32 enable)
 {
 	/* 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, 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);
@@ -260,8 +263,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)
@@ -270,7 +273,6 @@ 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);
@@ -279,10 +281,9 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	/* Setup DLL 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) |
+	val = (0x1 << OTAPDLYENA_SHIFT) |
 	      (otap_del_sel << OTAPDLYSEL_SHIFT);
 
 	/* Write to STRBSEL for HS400 speed mode */
@@ -299,7 +300,8 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) {
 		sdhci_am654_setup_dll(host, clock);
-		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]);
 		sdhci_am654->dll_enable = true;
 	} else {
 		sdhci_am654_setup_delay_chain(sdhci_am654, timing);
@@ -317,6 +319,7 @@ 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 */
@@ -325,6 +328,12 @@ 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);
+
+	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,
@@ -484,12 +493,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);
 
@@ -513,7 +518,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));
 
-	sdhci_am654_write_itapdly(sdhci_am654, itap);
+	sdhci_am654_write_itapdly(sdhci_am654, itap, 1);
 
 	return 0;
 }
@@ -662,9 +667,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.43.0

[PATCH v2 4/7] mmc: sdhci_am654: Fix itapdly/otapdly array type

[Judith Mendez]

While integer type works, the otap_del_sel and itap_del_sel
arrays are manipulated as u32, so change array types to u32.

Fixes: 8ee5fc0e0b3b ("mmc: sdhci_am654: Update OTAPDLY writes")
Fixes: a0a62497f6aa ("mmc: sdhci_am654: Add support for input tap delay")
Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/host/sdhci_am654.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 935f581c05d8..35ba7d921690 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -141,8 +141,8 @@ static const struct timing_data td[] = {
 
 struct sdhci_am654_data {
 	struct regmap *base;
-	int otap_del_sel[ARRAY_SIZE(td)];
-	int itap_del_sel[ARRAY_SIZE(td)];
+	u32 otap_del_sel[ARRAY_SIZE(td)];
+	u32 itap_del_sel[ARRAY_SIZE(td)];
 	u32 itap_del_ena[ARRAY_SIZE(td)];
 	int clkbuf_sel;
 	int trm_icp;
-- 
2.43.0

Re: [PATCH v2 4/7] mmc: sdhci_am654: Fix itapdly/otapdly array type

[Adrian Hunter]

On 7/02/24 03:15, Judith Mendez wrote:
> While integer type works, the otap_del_sel and itap_del_sel
> arrays are manipulated as u32, so change array types to u32.

If it doesn't make any practical difference, then it is not
generally considered a "fix", at least according to stable
kernel rules, so Fixes tags are probably not warranted here.

> 
> Fixes: 8ee5fc0e0b3b ("mmc: sdhci_am654: Update OTAPDLY writes")
> Fixes: a0a62497f6aa ("mmc: sdhci_am654: Add support for input tap delay")
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  drivers/mmc/host/sdhci_am654.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 935f581c05d8..35ba7d921690 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -141,8 +141,8 @@ static const struct timing_data td[] = {
>  
>  struct sdhci_am654_data {
>  	struct regmap *base;
> -	int otap_del_sel[ARRAY_SIZE(td)];
> -	int itap_del_sel[ARRAY_SIZE(td)];
> +	u32 otap_del_sel[ARRAY_SIZE(td)];
> +	u32 itap_del_sel[ARRAY_SIZE(td)];
>  	u32 itap_del_ena[ARRAY_SIZE(td)];
>  	int clkbuf_sel;
>  	int trm_icp;

Re: [PATCH v2 4/7] mmc: sdhci_am654: Fix itapdly/otapdly array type

[Judith Mendez]

Hi Adrian,

On 2/16/24 11:10 AM, Adrian Hunter wrote:
> On 7/02/24 03:15, Judith Mendez wrote:
>> While integer type works, the otap_del_sel and itap_del_sel
>> arrays are manipulated as u32, so change array types to u32.
> 
> If it doesn't make any practical difference, then it is not
> generally considered a "fix", at least according to stable
> kernel rules, so Fixes tags are probably not warranted here.

Understood, will remove fixes tag here then, thanks.

> 
>>
>> Fixes: 8ee5fc0e0b3b ("mmc: sdhci_am654: Update OTAPDLY writes")
>> Fixes: a0a62497f6aa ("mmc: sdhci_am654: Add support for input tap delay")
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index 935f581c05d8..35ba7d921690 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -141,8 +141,8 @@ static const struct timing_data td[] = {
>>   
>>   struct sdhci_am654_data {
>>   	struct regmap *base;
>> -	int otap_del_sel[ARRAY_SIZE(td)];
>> -	int itap_del_sel[ARRAY_SIZE(td)];
>> +	u32 otap_del_sel[ARRAY_SIZE(td)];
>> +	u32 itap_del_sel[ARRAY_SIZE(td)];
>>   	u32 itap_del_ena[ARRAY_SIZE(td)];
>>   	int clkbuf_sel;
>>   	int trm_icp;
> 

~ Judith

[PATCH v2 5/7] mmc: sdhci_am654: Update comments in sdhci_am654_set_clock

[Judith Mendez]

The sdhci_am654_set_clock function is also used to enable
delay chain, therefore fix comments to be more generic in
case we are not enabling DLL.

Fixes: fe52e2fbc6ef ("mmc: sdhci_am654: Fix conditions for enabling dll")
Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/host/sdhci_am654.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 35ba7d921690..3755a015f328 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -279,7 +279,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	sdhci_set_clock(host, clock);
 
-	/* Setup DLL Output TAP delay */
+	/* Setup Output TAP delay */
 	otap_del_sel = sdhci_am654->otap_del_sel[timing];
 
 	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
@@ -322,7 +322,7 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
 	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;
-- 
2.43.0

Re: [PATCH v2 5/7] mmc: sdhci_am654: Update comments in sdhci_am654_set_clock

[Adrian Hunter]

On 7/02/24 03:15, Judith Mendez wrote:
> The sdhci_am654_set_clock function is also used to enable
> delay chain, therefore fix comments to be more generic in
> case we are not enabling DLL.
> 
> Fixes: fe52e2fbc6ef ("mmc: sdhci_am654: Fix conditions for enabling dll")

Similar to patch 4, Fixes tag is probably not warranted.

> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  drivers/mmc/host/sdhci_am654.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 35ba7d921690..3755a015f328 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -279,7 +279,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
>  
>  	sdhci_set_clock(host, clock);
>  
> -	/* Setup DLL Output TAP delay */
> +	/* Setup Output TAP delay */
>  	otap_del_sel = sdhci_am654->otap_del_sel[timing];
>  
>  	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
> @@ -322,7 +322,7 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>  	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;

Re: [PATCH v2 5/7] mmc: sdhci_am654: Update comments in sdhci_am654_set_clock

[Judith Mendez]

Hi Adrian,

On 2/16/24 11:11 AM, Adrian Hunter wrote:
> On 7/02/24 03:15, Judith Mendez wrote:
>> The sdhci_am654_set_clock function is also used to enable
>> delay chain, therefore fix comments to be more generic in
>> case we are not enabling DLL.
>>
>> Fixes: fe52e2fbc6ef ("mmc: sdhci_am654: Fix conditions for enabling dll")
> 
> Similar to patch 4, Fixes tag is probably not warranted.

Will fix for V3, thanks.

> 
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index 35ba7d921690..3755a015f328 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -279,7 +279,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
>>   
>>   	sdhci_set_clock(host, clock);
>>   
>> -	/* Setup DLL Output TAP delay */
>> +	/* Setup Output TAP delay */
>>   	otap_del_sel = sdhci_am654->otap_del_sel[timing];
>>   
>>   	mask = OTAPDLYENA_MASK | OTAPDLYSEL_MASK;
>> @@ -322,7 +322,7 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>   	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;
> 

~ Judith

[PATCH v2 6/7] 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 | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 3755a015f328..f98cce69a286 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -320,6 +320,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,18 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
 	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.43.0

[PATCH v2 7/7] 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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index f98cce69a286..280fd4c86c95 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)
 };
@@ -175,6 +176,7 @@ static void sdhci_am654_setup_dll(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
+	unsigned char timing = host->mmc->ios.timing;
 	int sel50, sel100, freqsel;
 	u32 mask, val;
 	int ret;
@@ -237,6 +239,10 @@ 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;
 	}
+
+	/* 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];
 }
 
 static void sdhci_am654_write_itapdly(struct sdhci_am654_data *sdhci_am654,
@@ -310,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,
@@ -527,6 +536,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.43.0

Re: [PATCH v2 0/7] Add tuning algorithm for delay chain

[Francesco Dolcini]

On Tue, Feb 06, 2024 at 07:15:13PM -0600, Judith Mendez wrote:
> 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

Where is this `[0]`?

> platforms: AM62x SK, AM62A SK, AM62p SK, AM64x SK, and AM64x
> EVM.

In the other patches you link some document, but I was not able to find
anything related to AM62, can you provide some reference on this
specific SOC?

Francesco

Re: [PATCH v2 0/7] Add tuning algorithm for delay chain

[Judith Mendez]

Hi Francesco,

On 2/11/24 10:02 AM, Francesco Dolcini wrote:
> On Tue, Feb 06, 2024 at 07:15:13PM -0600, Judith Mendez wrote:
>> 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
> 
> Where is this `[0]`?

I must have missed linking the ref doc here, will add in next
iteration, thanks.

> 
>> platforms: AM62x SK, AM62A SK, AM62p SK, AM64x SK, and AM64x
>> EVM.
> 
> In the other patches you link some document, but I was not able to find
> anything related to AM62, can you provide some reference on this
> specific SOC?

This patch series fixes issues that affect all Sitara SoCs, not only
AM62x. However, I could use AM62x for reference, no problem.

> 
> Francesco
> 

~ Judith

Re: [PATCH v2 0/7] Add tuning algorithm for delay chain

[Francesco Dolcini]

Hi Judith,

On Mon, Feb 12, 2024 at 10:33:35AM -0600, Judith Mendez wrote:
> Hi Francesco,
> 
> On 2/11/24 10:02 AM, Francesco Dolcini wrote:
> > On Tue, Feb 06, 2024 at 07:15:13PM -0600, Judith Mendez wrote:
> > > 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
> > 
> > Where is this `[0]`?
> 
> I must have missed linking the ref doc here, will add in next
> iteration, thanks.
> 
> > 
> > > platforms: AM62x SK, AM62A SK, AM62p SK, AM64x SK, and AM64x
> > > EVM.
> > 
> > In the other patches you link some document, but I was not able to find
> > anything related to AM62, can you provide some reference on this
> > specific SOC?
> 
> This patch series fixes issues that affect all Sitara SoCs, not only
> AM62x. However, I could use AM62x for reference, no problem.

I am really looking for documentation here that is related to the AM62
because I was not able to find anything and this is a topic of interest
for me.

Can you share something?

Francesco

Re: [PATCH v2 0/7] Add tuning algorithm for delay chain

[Judith Mendez]

Hi Francesco,

On 2/12/24 11:32 AM, Francesco Dolcini wrote:
> Hi Judith,
> 
> On Mon, Feb 12, 2024 at 10:33:35AM -0600, Judith Mendez wrote:
>> Hi Francesco,
>>
>> On 2/11/24 10:02 AM, Francesco Dolcini wrote:
>>> On Tue, Feb 06, 2024 at 07:15:13PM -0600, Judith Mendez wrote:
>>>> 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
>>>
>>> Where is this `[0]`?
>>
>> I must have missed linking the ref doc here, will add in next
>> iteration, thanks.
>>
>>>
>>>> platforms: AM62x SK, AM62A SK, AM62p SK, AM64x SK, and AM64x
>>>> EVM.
>>>
>>> In the other patches you link some document, but I was not able to find
>>> anything related to AM62, can you provide some reference on this
>>> specific SOC?
>>
>> This patch series fixes issues that affect all Sitara SoCs, not only
>> AM62x. However, I could use AM62x for reference, no problem.
> 
> I am really looking for documentation here that is related to the AM62
> because I was not able to find anything and this is a topic of interest
> for me.
> 

The AM62x device datasheet: https://www.ti.com/lit/ds/symlink/am625.pdf,
reference Table 7-79 for MMC0 and Table 7-97 for MMC1.

TRM, reference section 12.4.5 for MMCSD and section 12.4.5.5.2.3.2
Tuning Sequence that we currently implement in the sdhci_am654 driver.
Section 14.8.4.6 for MMCSD registers, SS_CFG_PHY_CTRL_1,
SS_CFG_PHY_CTRL_5, and SS_CFG_PHY_CTRL_5 are usually of higher interest.

Tuning algorithm, reference: 
https://www.ti.com/lit/an/spract9/spract9.pdf which is applicable for 
AM62x, DLL is not applicable
for AM62x, so the tuning algorithm should not consider the buffer
as a circular buffer.

~ Judith

> Can you share something?
> 
> Francesco
>