[RFC PATCH 1/2] mmc: Update sdhci tune function to return errors

[RFC PATCH 1/2] mmc: Update sdhci tune function to return errors

[Erick Shepherd]

Updates the sdhci_execute_tuning function to return the error code
that was returned by the __sdhci_execute_tuning function.
Previously this code was only stored in host->tuning_err and not
actually returned.

Signed-off-by: Erick Shepherd <erick.shepherd@ni.com>
---
 drivers/mmc/host/sdhci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f4a7733a8ad2..b35b8917fa1e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2967,7 +2967,8 @@ int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 
 	sdhci_start_tuning(host);
 
-	host->tuning_err = __sdhci_execute_tuning(host, opcode);
+	err = __sdhci_execute_tuning(host, opcode);
+	host->tuning_err = err;
 
 	sdhci_end_tuning(host);
 out:
-- 
2.43.0

[RFC PATCH 2/2] mmc: Allow tuning to be skipped during card init

[Erick Shepherd]

Add a new field to the mmc_host struct to track when the card should
skip the initial tuning and use it to conditionally stop tuning in the
mmc_sd_init_uhs_card function. Currently the new field only gets set
when a DDR50 card fails to tune, which indicates the card does not
support tuning.

Signed-off-by: Erick Shepherd <erick.shepherd@ni.com>
---
 drivers/mmc/core/sd.c    | 4 +++-
 include/linux/mmc/host.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index cc757b850e79..353715fd8f53 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -663,7 +663,8 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
 	if (!mmc_host_is_spi(card->host) &&
 		(card->host->ios.timing == MMC_TIMING_UHS_SDR50 ||
 		 card->host->ios.timing == MMC_TIMING_UHS_DDR50 ||
-		 card->host->ios.timing == MMC_TIMING_UHS_SDR104)) {
+		 card->host->ios.timing == MMC_TIMING_UHS_SDR104) &&
+		!card->host->skip_init_tune) {
 		err = mmc_execute_tuning(card);
 
 		/*
@@ -676,6 +677,7 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
 		if (err && card->host->ios.timing == MMC_TIMING_UHS_DDR50) {
 			pr_warn("%s: ddr50 tuning failed\n",
 				mmc_hostname(card->host));
+			card->host->skip_init_tune = 1;
 			err = 0;
 		}
 	}
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 68f09a955a90..91c4db6837c9 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -486,6 +486,7 @@ struct mmc_host {
 	unsigned int		use_spi_crc:1;
 	unsigned int		claimed:1;	/* host exclusively claimed */
 	unsigned int		doing_init_tune:1; /* initial tuning in progress */
+	unsigned int		skip_init_tune:1;	/* skip the initial tuning */
 	unsigned int		can_retune:1;	/* re-tuning can be used */
 	unsigned int		doing_retune:1;	/* re-tuning in progress */
 	unsigned int		retune_now:1;	/* do re-tuning at next req */
-- 
2.43.0

Re: [RFC PATCH 2/2] mmc: Allow tuning to be skipped during card init

[Judith Mendez]

Hi Erick,

On 1/27/25 4:36 PM, Erick Shepherd wrote:
> Add a new field to the mmc_host struct to track when the card should
> skip the initial tuning and use it to conditionally stop tuning in the
> mmc_sd_init_uhs_card function. Currently the new field only gets set
> when a DDR50 card fails to tune, which indicates the card does not
> support tuning.

Just out of curiosity, why do we want to stop the initial tuning?

~ Judith

> 
> Signed-off-by: Erick Shepherd <erick.shepherd@ni.com>
> ---
>   drivers/mmc/core/sd.c    | 4 +++-
>   include/linux/mmc/host.h | 1 +
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index cc757b850e79..353715fd8f53 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -663,7 +663,8 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
>   	if (!mmc_host_is_spi(card->host) &&
>   		(card->host->ios.timing == MMC_TIMING_UHS_SDR50 ||
>   		 card->host->ios.timing == MMC_TIMING_UHS_DDR50 ||
> -		 card->host->ios.timing == MMC_TIMING_UHS_SDR104)) {
> +		 card->host->ios.timing == MMC_TIMING_UHS_SDR104) &&
> +		!card->host->skip_init_tune) {
>   		err = mmc_execute_tuning(card);
>   
>   		/*
> @@ -676,6 +677,7 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
>   		if (err && card->host->ios.timing == MMC_TIMING_UHS_DDR50) {
>   			pr_warn("%s: ddr50 tuning failed\n",
>   				mmc_hostname(card->host));
> +			card->host->skip_init_tune = 1;
>   			err = 0;
>   		}
>   	}
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 68f09a955a90..91c4db6837c9 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -486,6 +486,7 @@ struct mmc_host {
>   	unsigned int		use_spi_crc:1;
>   	unsigned int		claimed:1;	/* host exclusively claimed */
>   	unsigned int		doing_init_tune:1; /* initial tuning in progress */
> +	unsigned int		skip_init_tune:1;	/* skip the initial tuning */
>   	unsigned int		can_retune:1;	/* re-tuning can be used */
>   	unsigned int		doing_retune:1;	/* re-tuning in progress */
>   	unsigned int		retune_now:1;	/* do re-tuning at next req */

RE: [RFC PATCH 2/2] mmc: Allow tuning to be skipped during card init

[Erick Shepherd]

I have been working with a DDR50 SD card that does not support tuning.
The card's first tuning attempt fails, which is expected, but subsequent
tuning attempts cause I/O errors due to async page reads. My first patch
mostly solves this issue but I was still seeing I/O errors in the case
where the card is reset and attempts to do the initial tuning again
despite it failing previously. This second patch allows the initial tuning
to be skipped if it has already failed for a DDR50 card.

Re: [RFC PATCH 2/2] mmc: Allow tuning to be skipped during card init

[Ulf Hansson]

On Mon, 27 Jan 2025 at 23:41, Erick Shepherd <erick.shepherd@ni.com> wrote:
>
> Add a new field to the mmc_host struct to track when the card should
> skip the initial tuning and use it to conditionally stop tuning in the
> mmc_sd_init_uhs_card function. Currently the new field only gets set
> when a DDR50 card fails to tune, which indicates the card does not
> support tuning.
>
> Signed-off-by: Erick Shepherd <erick.shepherd@ni.com>
> ---
>  drivers/mmc/core/sd.c    | 4 +++-
>  include/linux/mmc/host.h | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index cc757b850e79..353715fd8f53 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -663,7 +663,8 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
>         if (!mmc_host_is_spi(card->host) &&
>                 (card->host->ios.timing == MMC_TIMING_UHS_SDR50 ||
>                  card->host->ios.timing == MMC_TIMING_UHS_DDR50 ||
> -                card->host->ios.timing == MMC_TIMING_UHS_SDR104)) {
> +                card->host->ios.timing == MMC_TIMING_UHS_SDR104) &&
> +               !card->host->skip_init_tune) {
>                 err = mmc_execute_tuning(card);
>
>                 /*
> @@ -676,6 +677,7 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
>                 if (err && card->host->ios.timing == MMC_TIMING_UHS_DDR50) {
>                         pr_warn("%s: ddr50 tuning failed\n",
>                                 mmc_hostname(card->host));
> +                       card->host->skip_init_tune = 1;

Rather than adding a host specific flag to let the core signal to the
host driver that it should skip tuning, I think it would be better if
we can avoid the core to request a tuning instead.

So we need to get the information from the host, during the first
tuning attempt if it fails and then the tuning should be disabled for
the *card*, forever.

>                         err = 0;
>                 }
>         }
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 68f09a955a90..91c4db6837c9 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -486,6 +486,7 @@ struct mmc_host {
>         unsigned int            use_spi_crc:1;
>         unsigned int            claimed:1;      /* host exclusively claimed */
>         unsigned int            doing_init_tune:1; /* initial tuning in progress */
> +       unsigned int            skip_init_tune:1;       /* skip the initial tuning */
>         unsigned int            can_retune:1;   /* re-tuning can be used */
>         unsigned int            doing_retune:1; /* re-tuning in progress */
>         unsigned int            retune_now:1;   /* do re-tuning at next req */
> --
> 2.43.0
>

Again, please re-submit and loop in Adrian to get his opinion on this.

Kind regards
Uffe

Re: [RFC PATCH 1/2] mmc: Update sdhci tune function to return errors

[Ulf Hansson]

On Mon, 27 Jan 2025 at 23:40, Erick Shepherd <erick.shepherd@ni.com> wrote:
>
> Updates the sdhci_execute_tuning function to return the error code
> that was returned by the __sdhci_execute_tuning function.
> Previously this code was only stored in host->tuning_err and not
> actually returned.
>
> Signed-off-by: Erick Shepherd <erick.shepherd@ni.com>
> ---
>  drivers/mmc/host/sdhci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f4a7733a8ad2..b35b8917fa1e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2967,7 +2967,8 @@ int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>
>         sdhci_start_tuning(host);
>
> -       host->tuning_err = __sdhci_execute_tuning(host, opcode);
> +       err = __sdhci_execute_tuning(host, opcode);
> +       host->tuning_err = err;

Hmm, this code was deliberately not returning an error in this path,
so I am pretty sure things will break with this change.

Anyway, please re-submit and add Adrian (the sdhci maintainer) and
possibly a couple other contributors that have been working with
tunings for host drivers. We need their feedback on this.

>
>         sdhci_end_tuning(host);
>  out:
> --
> 2.43.0
>

Kind regards
Uffe

RE: [RFC PATCH 1/2] mmc: Update sdhci tune function to return errors

[Erick Shepherd]

Thank you for taking the time to review this. I'll resubmit the patch, pull in Adrian,
and move the new field to the mmc_card struct.

Regards
Erick

Re: [RFC PATCH 1/2] mmc: Update sdhci tune function to return errors

[Judith Mendez]

Hi Erick,

On 1/27/25 4:36 PM, Erick Shepherd wrote:
> Updates the sdhci_execute_tuning function to return the error code
> that was returned by the __sdhci_execute_tuning function.
> Previously this code was only stored in host->tuning_err and not
> actually returned.
> 
> Signed-off-by: Erick Shepherd <erick.shepherd@ni.com>

Reviewed-by: Judith Mendez <jm@ti.com>

> ---
>   drivers/mmc/host/sdhci.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f4a7733a8ad2..b35b8917fa1e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2967,7 +2967,8 @@ int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>   
>   	sdhci_start_tuning(host);
>   
> -	host->tuning_err = __sdhci_execute_tuning(host, opcode);
> +	err = __sdhci_execute_tuning(host, opcode);
> +	host->tuning_err = err;
>   
>   	sdhci_end_tuning(host);
>   out: