[PATCH 0/2] Add retry tuning sequence

[PATCH 0/2] Add retry tuning sequence

[Judith Mendez]

Due to failures to find failing region issues seen on some
boards, add retry tuning sequence to make the tuning algorithm
more robust. The tuning algorithm will re-execute up to 10
times if there is no failing or passing itapdly.

Due to the same issue above, add debug prints to make debugging
these corner-cases easier.

Judith Mendez (2):
  mmc: sdhci_am654: Add retry tuning
  mmc: sdhci_am654: Add tuning debug prints

 drivers/mmc/host/sdhci_am654.c | 35 +++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

-- 
2.46.0

[PATCH 1/2] mmc: sdhci_am654: Add retry tuning

[Judith Mendez]

Add retry tuning up to 10 times if we fail to find
a failing region or no passing itapdly. This is
necessary since some eMMC's have been observed to never
find a failing itapdly on the first couple of tuning
iterations, but eventually do. It been observed that the
tuning algorithm does not need to loop more than 10 times
before finding a failing itapdly.

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

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 64e10f7c9faa3..c3d485bd4d553 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -86,6 +86,7 @@
 
 #define CLOCK_TOO_SLOW_HZ	50000000
 #define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
+#define RETRY_TUNING_MAX	10
 
 /* Command Queue Host Controller Interface Base address */
 #define SDHCI_AM654_CQE_BASE_ADDR 0x200
@@ -151,6 +152,7 @@ struct sdhci_am654_data {
 	u32 flags;
 	u32 quirks;
 	bool dll_enable;
+	u32 tuning_loop;
 
 #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
 };
@@ -453,12 +455,14 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
 	int prev_fail_end = -1;
 	u8 i;
 
-	if (!num_fails)
-		return ITAPDLY_LAST_INDEX >> 1;
+	if (!num_fails) {
+		/* Retry tuning */
+		return -1;
+	}
 
 	if (fail_window->length == ITAPDLY_LENGTH) {
-		dev_err(dev, "No passing ITAPDLY, return 0\n");
-		return 0;
+		/* Retry tuning */
+		return -1;
 	}
 
 	first_fail_start = fail_window->start;
@@ -504,6 +508,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
 	u8 curr_pass, itap;
 	u8 fail_index = 0;
 	u8 prev_pass = 1;
+	int ret;
 
 	memset(fail_window, 0, sizeof(fail_window));
 
@@ -532,10 +537,20 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
 	if (fail_window[fail_index].length != 0)
 		fail_index++;
 
-	itap = sdhci_am654_calculate_itap(host, fail_window, fail_index,
-					  sdhci_am654->dll_enable);
+	ret = sdhci_am654_calculate_itap(host, fail_window, fail_index,
+					 sdhci_am654->dll_enable);
 
-	sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
+	if (ret >= 0) {
+		itap = ret;
+		sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
+	} else {
+		if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) {
+			sdhci_am654->tuning_loop++;
+			sdhci_am654_platform_execute_tuning(host, opcode);
+		} else {
+			return -1;
+		}
+	}
 
 	/* Save ITAPDLY */
 	sdhci_am654->itap_del_sel[timing] = itap;
@@ -908,6 +923,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
 		goto err_pltfm_free;
 	}
 
+	sdhci_am654->tuning_loop = 0;
 	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
 
 	pm_runtime_get_noresume(dev);
-- 
2.46.0

[PATCH 2/2] mmc: sdhci_am654: Add tuning debug prints

[Judith Mendez]

Add debug prints to tuning algorithm for debugging.

Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/host/sdhci_am654.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index c3d485bd4d553..a909f8de0eabe 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -457,11 +457,13 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
 
 	if (!num_fails) {
 		/* Retry tuning */
+		dev_err(dev, "No failing region found, retry tuning\n");
 		return -1;
 	}
 
 	if (fail_window->length == ITAPDLY_LENGTH) {
 		/* Retry tuning */
+		dev_err(dev, "No passing ITAPDLY, retry tuning\n");
 		return -1;
 	}
 
@@ -505,6 +507,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
 	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
 	unsigned char timing = host->mmc->ios.timing;
 	struct window fail_window[ITAPDLY_LENGTH];
+	struct device *dev = mmc_dev(host->mmc);
 	u8 curr_pass, itap;
 	u8 fail_index = 0;
 	u8 prev_pass = 1;
@@ -542,12 +545,14 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
 
 	if (ret >= 0) {
 		itap = ret;
+		dev_dbg(dev, "Final ITAPDLY=%d\n", itap);
 		sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
 	} else {
 		if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) {
 			sdhci_am654->tuning_loop++;
 			sdhci_am654_platform_execute_tuning(host, opcode);
 		} else {
+			dev_err(dev, "Failed to find ITAPDLY, fail tuning\n");
 			return -1;
 		}
 	}
-- 
2.46.0

Re: [PATCH 2/2] mmc: sdhci_am654: Add tuning debug prints

[Ulf Hansson]

On Thu, 15 Aug 2024 at 22:15, Judith Mendez <jm@ti.com> wrote:
>
> Add debug prints to tuning algorithm for debugging.
>
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  drivers/mmc/host/sdhci_am654.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index c3d485bd4d553..a909f8de0eabe 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -457,11 +457,13 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
>
>         if (!num_fails) {
>                 /* Retry tuning */
> +               dev_err(dev, "No failing region found, retry tuning\n");

A dev_err seems to be too heavy, but I am not sure at what frequency
this could occur?

Why isn't a dev_dbg sufficient?

>                 return -1;
>         }
>
>         if (fail_window->length == ITAPDLY_LENGTH) {
>                 /* Retry tuning */
> +               dev_err(dev, "No passing ITAPDLY, retry tuning\n");

Ditto.

>                 return -1;
>         }
>
> @@ -505,6 +507,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>         struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>         unsigned char timing = host->mmc->ios.timing;
>         struct window fail_window[ITAPDLY_LENGTH];
> +       struct device *dev = mmc_dev(host->mmc);
>         u8 curr_pass, itap;
>         u8 fail_index = 0;
>         u8 prev_pass = 1;
> @@ -542,12 +545,14 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>
>         if (ret >= 0) {
>                 itap = ret;
> +               dev_dbg(dev, "Final ITAPDLY=%d\n", itap);
>                 sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
>         } else {
>                 if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) {
>                         sdhci_am654->tuning_loop++;
>                         sdhci_am654_platform_execute_tuning(host, opcode);
>                 } else {
> +                       dev_err(dev, "Failed to find ITAPDLY, fail tuning\n");

The commit message only talks about debug messages, but this is an
error message. Perhaps update the commit message a bit?

>                         return -1;
>                 }
>         }
> --
> 2.46.0
>

Kind regards
Uffe

Re: [PATCH 2/2] mmc: sdhci_am654: Add tuning debug prints

[Judith Mendez]

Hi Ulf Hansson,

On 8/20/24 6:33 AM, Ulf Hansson wrote:
> On Thu, 15 Aug 2024 at 22:15, Judith Mendez <jm@ti.com> wrote:
>>
>> Add debug prints to tuning algorithm for debugging.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index c3d485bd4d553..a909f8de0eabe 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -457,11 +457,13 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
>>
>>          if (!num_fails) {
>>                  /* Retry tuning */
>> +               dev_err(dev, "No failing region found, retry tuning\n");
> 
> A dev_err seems to be too heavy, but I am not sure at what frequency
> this could occur?

Having no failing region is what we call a corner case, it rarely 
happens. The one case where it did happen, it took a good amount
of time to discover there were no failing regions found. The tuning
algorithm had to be looped 3 times before finding a failing itapdly.

> 
> Why isn't a dev_dbg sufficient?

I thought about using dev_dbg, but based on some feedback after coming
upon this issue on a board bring up case, we think it would help
enormously if we make it as obvious as possible when no failing region
is found.

The one case where this came up, the dev_err print would only print 3
times... Now this is only one case and we are not aware of any more
cases like this, also we cannot replicate on TI EVM's.

> 
>>                  return -1;
>>          }
>>
>>          if (fail_window->length == ITAPDLY_LENGTH) {
>>                  /* Retry tuning */
>> +               dev_err(dev, "No passing ITAPDLY, retry tuning\n");
> 
> Ditto.

Same idea as above..

But with this print, the maximum amount of prints that could be printed
is 20, is this too many prints in your opinion?


> 
>>                  return -1;
>>          }
>>
>> @@ -505,6 +507,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>          struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>          unsigned char timing = host->mmc->ios.timing;
>>          struct window fail_window[ITAPDLY_LENGTH];
>> +       struct device *dev = mmc_dev(host->mmc);
>>          u8 curr_pass, itap;
>>          u8 fail_index = 0;
>>          u8 prev_pass = 1;
>> @@ -542,12 +545,14 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>
>>          if (ret >= 0) {
>>                  itap = ret;
>> +               dev_dbg(dev, "Final ITAPDLY=%d\n", itap);
>>                  sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
>>          } else {
>>                  if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) {
>>                          sdhci_am654->tuning_loop++;
>>                          sdhci_am654_platform_execute_tuning(host, opcode);
>>                  } else {
>> +                       dev_err(dev, "Failed to find ITAPDLY, fail tuning\n");
> 
> The commit message only talks about debug messages, but this is an
> error message. Perhaps update the commit message a bit?

Sure will do, after we conclude the discussion above and in v2.

Thanks so much for reviewing.

~ Judith

> 
>>                          return -1;
>>                  }
>>          }
>> --
>> 2.46.0
>>
> 
> Kind regards
> Uffe

Re: [PATCH 1/2] mmc: sdhci_am654: Add retry tuning

[Judith Mendez]

Hi all,

On 8/15/24 3:15 PM, Judith Mendez wrote:
> Add retry tuning up to 10 times if we fail to find
> a failing region or no passing itapdly. This is
> necessary since some eMMC's have been observed to never
> find a failing itapdly on the first couple of tuning
> iterations, but eventually do. It been observed that the
> tuning algorithm does not need to loop more than 10 times
> before finding a failing itapdly.

Will fix-up this commit message as well, there has only
been one case like this so far.

~ Judith

Re: [PATCH 2/2] mmc: sdhci_am654: Add tuning debug prints

[Ulf Hansson]

On Tue, 20 Aug 2024 at 16:41, Judith Mendez <jm@ti.com> wrote:
>
> Hi Ulf Hansson,
>
> On 8/20/24 6:33 AM, Ulf Hansson wrote:
> > On Thu, 15 Aug 2024 at 22:15, Judith Mendez <jm@ti.com> wrote:
> >>
> >> Add debug prints to tuning algorithm for debugging.
> >>
> >> Signed-off-by: Judith Mendez <jm@ti.com>
> >> ---
> >>   drivers/mmc/host/sdhci_am654.c | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> >> index c3d485bd4d553..a909f8de0eabe 100644
> >> --- a/drivers/mmc/host/sdhci_am654.c
> >> +++ b/drivers/mmc/host/sdhci_am654.c
> >> @@ -457,11 +457,13 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
> >>
> >>          if (!num_fails) {
> >>                  /* Retry tuning */
> >> +               dev_err(dev, "No failing region found, retry tuning\n");
> >
> > A dev_err seems to be too heavy, but I am not sure at what frequency
> > this could occur?
>
> Having no failing region is what we call a corner case, it rarely
> happens. The one case where it did happen, it took a good amount
> of time to discover there were no failing regions found. The tuning
> algorithm had to be looped 3 times before finding a failing itapdly.
>
> >
> > Why isn't a dev_dbg sufficient?
>
> I thought about using dev_dbg, but based on some feedback after coming
> upon this issue on a board bring up case, we think it would help
> enormously if we make it as obvious as possible when no failing region
> is found.
>
> The one case where this came up, the dev_err print would only print 3
> times... Now this is only one case and we are not aware of any more
> cases like this, also we cannot replicate on TI EVM's.

What happens if/when we fail here? Do we fail to detect the card or do
we end up running it in some degraded mode?

If the latter a dev_warn, the former a dev_err(). Does that make sense?

>
> >
> >>                  return -1;
> >>          }
> >>
> >>          if (fail_window->length == ITAPDLY_LENGTH) {
> >>                  /* Retry tuning */
> >> +               dev_err(dev, "No passing ITAPDLY, retry tuning\n");
> >
> > Ditto.
>
> Same idea as above..
>
> But with this print, the maximum amount of prints that could be printed
> is 20, is this too many prints in your opinion?

This sounds like dev_dbg to me. We are not really failing, as we are
making a re-try and will most likely succeed then, right?

>
>
> >
> >>                  return -1;
> >>          }
> >>
> >> @@ -505,6 +507,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
> >>          struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> >>          unsigned char timing = host->mmc->ios.timing;
> >>          struct window fail_window[ITAPDLY_LENGTH];
> >> +       struct device *dev = mmc_dev(host->mmc);
> >>          u8 curr_pass, itap;
> >>          u8 fail_index = 0;
> >>          u8 prev_pass = 1;
> >> @@ -542,12 +545,14 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
> >>
> >>          if (ret >= 0) {
> >>                  itap = ret;
> >> +               dev_dbg(dev, "Final ITAPDLY=%d\n", itap);
> >>                  sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
> >>          } else {
> >>                  if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) {
> >>                          sdhci_am654->tuning_loop++;
> >>                          sdhci_am654_platform_execute_tuning(host, opcode);
> >>                  } else {
> >> +                       dev_err(dev, "Failed to find ITAPDLY, fail tuning\n");
> >
> > The commit message only talks about debug messages, but this is an
> > error message. Perhaps update the commit message a bit?
>
> Sure will do, after we conclude the discussion above and in v2.
>
> Thanks so much for reviewing.
>

Kind regards
Uffe

Re: [PATCH 2/2] mmc: sdhci_am654: Add tuning debug prints

[Judith Mendez]

Hi Ulf Hansson,

On 8/20/24 10:03 AM, Ulf Hansson wrote:
> On Tue, 20 Aug 2024 at 16:41, Judith Mendez <jm@ti.com> wrote:
>>
>> Hi Ulf Hansson,
>>
>> On 8/20/24 6:33 AM, Ulf Hansson wrote:
>>> On Thu, 15 Aug 2024 at 22:15, Judith Mendez <jm@ti.com> wrote:
>>>>
>>>> Add debug prints to tuning algorithm for debugging.
>>>>
>>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>>> ---
>>>>    drivers/mmc/host/sdhci_am654.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>>> index c3d485bd4d553..a909f8de0eabe 100644
>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>> @@ -457,11 +457,13 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
>>>>
>>>>           if (!num_fails) {
>>>>                   /* Retry tuning */
>>>> +               dev_err(dev, "No failing region found, retry tuning\n");
>>>
>>> A dev_err seems to be too heavy, but I am not sure at what frequency
>>> this could occur?
>>
>> Having no failing region is what we call a corner case, it rarely
>> happens. The one case where it did happen, it took a good amount
>> of time to discover there were no failing regions found. The tuning
>> algorithm had to be looped 3 times before finding a failing itapdly.
>>
>>>
>>> Why isn't a dev_dbg sufficient?
>>
>> I thought about using dev_dbg, but based on some feedback after coming
>> upon this issue on a board bring up case, we think it would help
>> enormously if we make it as obvious as possible when no failing region
>> is found.
>>
>> The one case where this came up, the dev_err print would only print 3
>> times... Now this is only one case and we are not aware of any more
>> cases like this, also we cannot replicate on TI EVM's.
> 
> What happens if/when we fail here? Do we fail to detect the card or do
> we end up running it in some degraded mode?
> 
> If the latter a dev_warn, the former a dev_err(). Does that make sense?

What was happening was, we enumerated to HS200, but then cqe error was
triggered and this re-triggers the tuning algo repeatedly until the
failing region was found.

After thinking about this some more, I think I will switch to dev_dbg,
if we fail to find failing region and we re-run the tuning algorithm,
don't want to confuse people by printing error logs if we are re-running
the tuning algo anyways. Thanks so much for your feedback!

~ Judith


> 
>>
>>>
>>>>                   return -1;
>>>>           }
>>>>
>>>>           if (fail_window->length == ITAPDLY_LENGTH) {
>>>>                   /* Retry tuning */
>>>> +               dev_err(dev, "No passing ITAPDLY, retry tuning\n");
>>>
>>> Ditto.
>>
>> Same idea as above..
>>
>> But with this print, the maximum amount of prints that could be printed
>> is 20, is this too many prints in your opinion?
> 
> This sounds like dev_dbg to me. We are not really failing, as we are
> making a re-try and will most likely succeed then, right?
> 
>>
>>
>>>
>>>>                   return -1;
>>>>           }
>>>>
>>>> @@ -505,6 +507,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>>>           struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>>>           unsigned char timing = host->mmc->ios.timing;
>>>>           struct window fail_window[ITAPDLY_LENGTH];
>>>> +       struct device *dev = mmc_dev(host->mmc);
>>>>           u8 curr_pass, itap;
>>>>           u8 fail_index = 0;
>>>>           u8 prev_pass = 1;
>>>> @@ -542,12 +545,14 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>>>
>>>>           if (ret >= 0) {
>>>>                   itap = ret;
>>>> +               dev_dbg(dev, "Final ITAPDLY=%d\n", itap);
>>>>                   sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
>>>>           } else {
>>>>                   if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) {
>>>>                           sdhci_am654->tuning_loop++;
>>>>                           sdhci_am654_platform_execute_tuning(host, opcode);
>>>>                   } else {
>>>> +                       dev_err(dev, "Failed to find ITAPDLY, fail tuning\n");
>>>
>>> The commit message only talks about debug messages, but this is an
>>> error message. Perhaps update the commit message a bit?
>>
>> Sure will do, after we conclude the discussion above and in v2.
>>
>> Thanks so much for reviewing.
>>
> 
> Kind regards
> Uffe

Re: [PATCH 2/2] mmc: sdhci_am654: Add tuning debug prints

[Judith Mendez]

On 8/20/24 10:03 AM, Ulf Hansson wrote:
> On Tue, 20 Aug 2024 at 16:41, Judith Mendez <jm@ti.com> wrote:
>>
>> Hi Ulf Hansson,
>>
>> On 8/20/24 6:33 AM, Ulf Hansson wrote:
>>> On Thu, 15 Aug 2024 at 22:15, Judith Mendez <jm@ti.com> wrote:
>>>>
>>>> Add debug prints to tuning algorithm for debugging.
>>>>
>>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>>> ---
>>>>    drivers/mmc/host/sdhci_am654.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>>> index c3d485bd4d553..a909f8de0eabe 100644
>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>> @@ -457,11 +457,13 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
>>>>
>>>>           if (!num_fails) {
>>>>                   /* Retry tuning */
>>>> +               dev_err(dev, "No failing region found, retry tuning\n");
>>>
>>> A dev_err seems to be too heavy, but I am not sure at what frequency
>>> this could occur?
>>
>> Having no failing region is what we call a corner case, it rarely
>> happens. The one case where it did happen, it took a good amount
>> of time to discover there were no failing regions found. The tuning
>> algorithm had to be looped 3 times before finding a failing itapdly.
>>
>>>
>>> Why isn't a dev_dbg sufficient?
>>
>> I thought about using dev_dbg, but based on some feedback after coming
>> upon this issue on a board bring up case, we think it would help
>> enormously if we make it as obvious as possible when no failing region
>> is found.
>>
>> The one case where this came up, the dev_err print would only print 3
>> times... Now this is only one case and we are not aware of any more
>> cases like this, also we cannot replicate on TI EVM's.
> 
> What happens if/when we fail here? Do we fail to detect the card or do
> we end up running it in some degraded mode?
> 
> If the latter a dev_warn, the former a dev_err(). Does that make sense?
> 
>>
>>>
>>>>                   return -1;
>>>>           }
>>>>
>>>>           if (fail_window->length == ITAPDLY_LENGTH) {
>>>>                   /* Retry tuning */
>>>> +               dev_err(dev, "No passing ITAPDLY, retry tuning\n");
>>>
>>> Ditto.
>>
>> Same idea as above..
>>
>> But with this print, the maximum amount of prints that could be printed
>> is 20, is this too many prints in your opinion?
> 
> This sounds like dev_dbg to me. We are not really failing, as we are
> making a re-try and will most likely succeed then, right?

Yes absolutely right, will fix for v2, thanks.

~ Judith

> 
>>
>>
>>>
>>>>                   return -1;
>>>>           }
>>>>
>>>> @@ -505,6 +507,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>>>           struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>>>           unsigned char timing = host->mmc->ios.timing;
>>>>           struct window fail_window[ITAPDLY_LENGTH];
>>>> +       struct device *dev = mmc_dev(host->mmc);
>>>>           u8 curr_pass, itap;
>>>>           u8 fail_index = 0;
>>>>           u8 prev_pass = 1;
>>>> @@ -542,12 +545,14 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>>>
>>>>           if (ret >= 0) {
>>>>                   itap = ret;
>>>> +               dev_dbg(dev, "Final ITAPDLY=%d\n", itap);
>>>>                   sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
>>>>           } else {
>>>>                   if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) {
>>>>                           sdhci_am654->tuning_loop++;
>>>>                           sdhci_am654_platform_execute_tuning(host, opcode);
>>>>                   } else {
>>>> +                       dev_err(dev, "Failed to find ITAPDLY, fail tuning\n");
>>>
>>> The commit message only talks about debug messages, but this is an
>>> error message. Perhaps update the commit message a bit?
>>
>> Sure will do, after we conclude the discussion above and in v2.
>>
>> Thanks so much for reviewing.
>>
> 
> Kind regards
> Uffe

Re: [PATCH 1/2] mmc: sdhci_am654: Add retry tuning

[Adrian Hunter]

On 15/08/24 23:15, Judith Mendez wrote:
> Add retry tuning up to 10 times if we fail to find
> a failing region or no passing itapdly. This is
> necessary since some eMMC's have been observed to never
> find a failing itapdly on the first couple of tuning
> iterations, but eventually do. It been observed that the
> tuning algorithm does not need to loop more than 10 times
> before finding a failing itapdly.
> 
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  drivers/mmc/host/sdhci_am654.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 64e10f7c9faa3..c3d485bd4d553 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -86,6 +86,7 @@
>  
>  #define CLOCK_TOO_SLOW_HZ	50000000
>  #define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
> +#define RETRY_TUNING_MAX	10
>  
>  /* Command Queue Host Controller Interface Base address */
>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
> @@ -151,6 +152,7 @@ struct sdhci_am654_data {
>  	u32 flags;
>  	u32 quirks;
>  	bool dll_enable;
> +	u32 tuning_loop;
>  
>  #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>  };
> @@ -453,12 +455,14 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
>  	int prev_fail_end = -1;
>  	u8 i;
>  
> -	if (!num_fails)
> -		return ITAPDLY_LAST_INDEX >> 1;
> +	if (!num_fails) {
> +		/* Retry tuning */
> +		return -1;
> +	}
>  
>  	if (fail_window->length == ITAPDLY_LENGTH) {
> -		dev_err(dev, "No passing ITAPDLY, return 0\n");
> -		return 0;
> +		/* Retry tuning */
> +		return -1;
>  	}
>  
>  	first_fail_start = fail_window->start;
> @@ -504,6 +508,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>  	u8 curr_pass, itap;
>  	u8 fail_index = 0;
>  	u8 prev_pass = 1;
> +	int ret;
>  
>  	memset(fail_window, 0, sizeof(fail_window));
>  
> @@ -532,10 +537,20 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>  	if (fail_window[fail_index].length != 0)
>  		fail_index++;
>  
> -	itap = sdhci_am654_calculate_itap(host, fail_window, fail_index,
> -					  sdhci_am654->dll_enable);
> +	ret = sdhci_am654_calculate_itap(host, fail_window, fail_index,
> +					 sdhci_am654->dll_enable);
>  
> -	sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
> +	if (ret >= 0) {
> +		itap = ret;
> +		sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
> +	} else {
> +		if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) {
> +			sdhci_am654->tuning_loop++;
> +			sdhci_am654_platform_execute_tuning(host, opcode);

The kernel uses very small stack size, so recursive function calls
should not be used.  It would be better to put the loop in a separate
function, or add a retry: label and goto retry.

> +		} else {
> +			return -1;
> +		}
> +	}
>  
>  	/* Save ITAPDLY */
>  	sdhci_am654->itap_del_sel[timing] = itap;
> @@ -908,6 +923,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>  		goto err_pltfm_free;
>  	}
>  
> +	sdhci_am654->tuning_loop = 0;

So this is 10 retries ever, since sdhci_am654->tuning_loop is never
set back to 0.  Is that the intention?

>  	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>  
>  	pm_runtime_get_noresume(dev);

Re: [PATCH 1/2] mmc: sdhci_am654: Add retry tuning

[Judith Mendez]

Hi Adrian,

On 8/21/24 12:37 AM, Adrian Hunter wrote:
> On 15/08/24 23:15, Judith Mendez wrote:
>> Add retry tuning up to 10 times if we fail to find
>> a failing region or no passing itapdly. This is
>> necessary since some eMMC's have been observed to never
>> find a failing itapdly on the first couple of tuning
>> iterations, but eventually do. It been observed that the
>> tuning algorithm does not need to loop more than 10 times
>> before finding a failing itapdly.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/mmc/host/sdhci_am654.c | 30 +++++++++++++++++++++++-------
>>   1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index 64e10f7c9faa3..c3d485bd4d553 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -86,6 +86,7 @@
>>   
>>   #define CLOCK_TOO_SLOW_HZ	50000000
>>   #define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
>> +#define RETRY_TUNING_MAX	10
>>   
>>   /* Command Queue Host Controller Interface Base address */
>>   #define SDHCI_AM654_CQE_BASE_ADDR 0x200
>> @@ -151,6 +152,7 @@ struct sdhci_am654_data {
>>   	u32 flags;
>>   	u32 quirks;
>>   	bool dll_enable;
>> +	u32 tuning_loop;
>>   
>>   #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>>   };
>> @@ -453,12 +455,14 @@ static u32 sdhci_am654_calculate_itap(struct sdhci_host *host, struct window
>>   	int prev_fail_end = -1;
>>   	u8 i;
>>   
>> -	if (!num_fails)
>> -		return ITAPDLY_LAST_INDEX >> 1;
>> +	if (!num_fails) {
>> +		/* Retry tuning */
>> +		return -1;
>> +	}
>>   
>>   	if (fail_window->length == ITAPDLY_LENGTH) {
>> -		dev_err(dev, "No passing ITAPDLY, return 0\n");
>> -		return 0;
>> +		/* Retry tuning */
>> +		return -1;
>>   	}
>>   
>>   	first_fail_start = fail_window->start;
>> @@ -504,6 +508,7 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>   	u8 curr_pass, itap;
>>   	u8 fail_index = 0;
>>   	u8 prev_pass = 1;
>> +	int ret;
>>   
>>   	memset(fail_window, 0, sizeof(fail_window));
>>   
>> @@ -532,10 +537,20 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>   	if (fail_window[fail_index].length != 0)
>>   		fail_index++;
>>   
>> -	itap = sdhci_am654_calculate_itap(host, fail_window, fail_index,
>> -					  sdhci_am654->dll_enable);
>> +	ret = sdhci_am654_calculate_itap(host, fail_window, fail_index,
>> +					 sdhci_am654->dll_enable);
>>   
>> -	sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
>> +	if (ret >= 0) {
>> +		itap = ret;
>> +		sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
>> +	} else {
>> +		if (sdhci_am654->tuning_loop < RETRY_TUNING_MAX) {
>> +			sdhci_am654->tuning_loop++;
>> +			sdhci_am654_platform_execute_tuning(host, opcode);
> 
> The kernel uses very small stack size, so recursive function calls
> should not be used.  It would be better to put the loop in a separate
> function, or add a retry: label and goto retry.

Ok, can change to this method, I was not sure of recursive function
call but had opted for that since the code was to be simpler.

> 
>> +		} else {
>> +			return -1;
>> +		}
>> +	}
>>   
>>   	/* Save ITAPDLY */
>>   	sdhci_am654->itap_del_sel[timing] = itap;
>> @@ -908,6 +923,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>   		goto err_pltfm_free;
>>   	}
>>   
>> +	sdhci_am654->tuning_loop = 0;
> 
> So this is 10 retries ever, since sdhci_am654->tuning_loop is never
> set back to 0.  Is that the intention?

Yes, maximum of 10 re-tries. So far we have only seen issues during
boot.

~ Judith

> 
>>   	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>   
>>   	pm_runtime_get_noresume(dev);
>