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

[RFC PATCH V2 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 V2 2/2] mmc: allow card to disable tuning

[Erick Shepherd]

Add a new field to the mmc_card struct to disable tuning for the card.
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/core.c  | 3 +++
 drivers/mmc/core/sd.c    | 1 +
 include/linux/mmc/card.h | 1 +
 3 files changed, 5 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5241528f8b90..ee91d53c45d5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -934,6 +934,9 @@ int mmc_execute_tuning(struct mmc_card *card)
 	u32 opcode;
 	int err;
 
+	if (card->disable_tuning)
+		return 0;
+
 	if (!host->ops->execute_tuning)
 		return 0;
 
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index cc757b850e79..dd65485c61d8 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -676,6 +676,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->disable_tuning = true;
 			err = 0;
 		}
 	}
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 526fce581657..f9733c7ce430 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -332,6 +332,7 @@ struct mmc_card {
 
 	bool			written_flag;	/* Indicates eMMC has been written since power on */
 	bool			reenable_cmdq;	/* Re-enable Command Queue */
+	bool			disable_tuning;	/* Disables tuning for the card */
 
 	unsigned int		erase_size;	/* erase size in sectors */
  	unsigned int		erase_shift;	/* if erase unit is power 2 */
-- 
2.43.0

Re: [RFC PATCH V2 2/2] mmc: allow card to disable tuning

[Adrian Hunter]

On 6/02/25 23:08, Erick Shepherd wrote:
> Add a new field to the mmc_card struct to disable tuning for the card.
> Currently the new field only gets set when a DDR50 card fails to tune,
> which indicates the card does not support tuning.

You need to explain why this is needed.  Presumably it speeds up
runtime-resume in some cases?

> 
> Signed-off-by: Erick Shepherd <erick.shepherd@ni.com>
> ---
>  drivers/mmc/core/core.c  | 3 +++
>  drivers/mmc/core/sd.c    | 1 +
>  include/linux/mmc/card.h | 1 +
>  3 files changed, 5 insertions(+)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5241528f8b90..ee91d53c45d5 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -934,6 +934,9 @@ int mmc_execute_tuning(struct mmc_card *card)
>  	u32 opcode;
>  	int err;
>  
> +	if (card->disable_tuning)
> +		return 0;
> +
>  	if (!host->ops->execute_tuning)
>  		return 0;
>  
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index cc757b850e79..dd65485c61d8 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -676,6 +676,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->disable_tuning = true;
>  			err = 0;
>  		}
>  	}
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 526fce581657..f9733c7ce430 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -332,6 +332,7 @@ struct mmc_card {
>  
>  	bool			written_flag;	/* Indicates eMMC has been written since power on */
>  	bool			reenable_cmdq;	/* Re-enable Command Queue */
> +	bool			disable_tuning;	/* Disables tuning for the card */
>  
>  	unsigned int		erase_size;	/* erase size in sectors */
>   	unsigned int		erase_shift;	/* if erase unit is power 2 */

Re: [RFC PATCH V2 2/2] mmc: allow card to disable tuning

[Erick Shepherd]

Sorry about that, the context for this change is that I have been
working with a DDR50 swissbit SD card that does not support tuning.
The case I'm seeing is that the first tuning times out and any
further tuning attempts cause an async page read I/O error. I used
this change to prevent the card from attempting to tune again if it
is ever reset in the case where we know tuning isn't supported.

Regards
Erick

Re: [RFC PATCH V2 2/2] mmc: allow card to disable tuning

[Adrian Hunter]

On 18/02/25 20:41, Erick Shepherd wrote:
> Sorry about that, the context for this change is that I have been
> working with a DDR50 swissbit SD card that does not support tuning.
> The case I'm seeing is that the first tuning times out and any
> further tuning attempts cause an async page read I/O error. I used
> this change to prevent the card from attempting to tune again if it
> is ever reset in the case where we know tuning isn't supported.

Sorry for the slow reply.

I would expect if there was a general problem with DDR50 SD cards,
it would have come to light before now.

Does the card work with any other host controllers with linux?

If it is specific to a particular kind of card, a card quirk
could be added, say MMC_QUIRK_BROKEN_DDR50_TUNING

Re: [RFC PATCH V2 2/2] mmc: allow card to disable tuning

[Erick Shepherd]

> Sorry for the slow reply.

> I would expect if there was a general problem with DDR50 SD cards,
> it would have come to light before now.

> Does the card work with any other host controllers with linux?

> If it is specific to a particular kind of card, a card quirk
> could be added, say MMC_QUIRK_BROKEN_DDR50_TUNING

No worries. I have not tested this with other host controllers but
can try to get something set up. This issue has only appeared on
one particular SD card model for us so I would not be surprised if
the I/O errors we see on subsequent tune requests are specific to this
card. I can put together a solution using the card quirk you suggested
if you think that is the best way forward.

My fix is currently spread across two commits, one to return the error
code thrown by the tune request timing out, which prevents the card from
retuning, and this one that prevents the initial card tuning if it has
already failed. Should both parts be controlled by the new card quirk?

Regards,
Erick

Re: [RFC PATCH V2 2/2] mmc: allow card to disable tuning

[Adrian Hunter]

On 7/03/25 19:45, Erick Shepherd wrote:
>> Sorry for the slow reply.
> 
>> I would expect if there was a general problem with DDR50 SD cards,
>> it would have come to light before now.
> 
>> Does the card work with any other host controllers with linux?
> 
>> If it is specific to a particular kind of card, a card quirk
>> could be added, say MMC_QUIRK_BROKEN_DDR50_TUNING
> 
> No worries. I have not tested this with other host controllers but
> can try to get something set up. This issue has only appeared on
> one particular SD card model for us so I would not be surprised if
> the I/O errors we see on subsequent tune requests are specific to this
> card. I can put together a solution using the card quirk you suggested
> if you think that is the best way forward.
> 
> My fix is currently spread across two commits, one to return the error
> code thrown by the tune request timing out, which prevents the card from
> retuning, and this one that prevents the initial card tuning if it has
> already failed. Should both parts be controlled by the new card quirk?

Does it tuning at all?  Maybe MMC_QUIRK_NO_UHS_DDR50_TUNING is a better
name, then at the top of mmc_execute_tuning()

	if ((card->quirks & MMC_QUIRK_NO_UHS_DDR50_TUNING) &&
	    host->ios->timing == MMC_TIMING_UHS_DDR50)
		return 0;

Re: [RFC PATCH V2 2/2] mmc: allow card to disable tuning

[Erick Shepherd]

> Does it tuning at all?  Maybe MMC_QUIRK_NO_UHS_DDR50_TUNING is a better
> name, then at the top of mmc_execute_tuning()

> 	if ((card->quirks & MMC_QUIRK_NO_UHS_DDR50_TUNING) &&
> 	    host->ios->timing == MMC_TIMING_UHS_DDR50)
> 		return 0;

The card doesn't need to tune at all so I think this would work for us.
I'm not very familiar with using card quirks. What would be the best
way to set MMC_QUIRK_NO_UHS_DDR50_TUNING? Would it be set based on the
model of the card or should it be set after the initial tuning times
out?

Regards,
Erick

Re: [RFC PATCH V2 2/2] mmc: allow card to disable tuning

[Adrian Hunter]

On 7/03/25 23:17, Erick Shepherd wrote:
>> Does it tuning at all?  Maybe MMC_QUIRK_NO_UHS_DDR50_TUNING is a better
>> name, then at the top of mmc_execute_tuning()
> 
>> 	if ((card->quirks & MMC_QUIRK_NO_UHS_DDR50_TUNING) &&
>> 	    host->ios->timing == MMC_TIMING_UHS_DDR50)
>> 		return 0;
> 
> The card doesn't need to tune at all so I think this would work for us.
> I'm not very familiar with using card quirks. What would be the best
> way to set MMC_QUIRK_NO_UHS_DDR50_TUNING? Would it be set based on the
> model of the card or should it be set after the initial tuning times
> out?

Based on the model of the card i.e. add a new entry to mmc_sd_fixups[]

Re: [RFC PATCH V2 2/2] mmc: allow card to disable tuning

[Erick Shepherd]

How does this look? I confirmed the changes stop the card from tuning
and prevent the I/O errors. I can submit a new patch since these
changes are substantially different than my initial ones.

diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
index 3205feb1e8ff..756f80024635 100644
--- a/drivers/mmc/core/card.h
+++ b/drivers/mmc/core/card.h
@@ -89,6 +89,7 @@ struct mmc_fixup {
 #define CID_MANFID_MICRON       0x13
 #define CID_MANFID_SAMSUNG      0x15
 #define CID_MANFID_APACER       0x27
+#define CID_MANFID_SWISSBIT     0x5D
 #define CID_MANFID_KINGSTON     0x70
 #define CID_MANFID_HYNIX	0x90
 #define CID_MANFID_KINGSTON_SD	0x9F


diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5241528f8b90..8962992f05aa 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -937,6 +937,10 @@ int mmc_execute_tuning(struct mmc_card *card)
 	if (!host->ops->execute_tuning)
 		return 0;
 
+	if ((card->quirks & MMC_QUIRK_NO_UHS_DDR50_TUNING) &&
+	    host->ios.timing == MMC_TIMING_UHS_DDR50)
+		return 0;
+
 	if (host->cqe_on)
 		host->cqe_ops->cqe_off(host);

 
diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
index 89b512905be1..7f893bafaa60 100644
--- a/drivers/mmc/core/quirks.h
+++ b/drivers/mmc/core/quirks.h
@@ -34,6 +34,16 @@ static const struct mmc_fixup __maybe_unused mmc_sd_fixups[] = {
 		   MMC_QUIRK_BROKEN_SD_CACHE | MMC_QUIRK_BROKEN_SD_POWEROFF_NOTIFY,
 		   EXT_CSD_REV_ANY),
 
+	/*
+	 * Swissbit series S46-u cards throw I/O errors during tuning requests
+	 * after the initial tuning request expectedly times out. This has
+	 * only been observed on cards manufactured on 01/2019 that are using
+	 * Bay Trail host controllers.
+	 */
+	_FIXUP_EXT("0016G", CID_MANFID_SWISSBIT, 0x5342, 2019, 1,
+		   0, -1ull, SDIO_ANY_ID, SDIO_ANY_ID, add_quirk_sd,
+		   MMC_QUIRK_NO_UHS_DDR50_TUNING, EXT_CSD_REV_ANY),
+
 	END_FIXUP
 };

 
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 526fce581657..ddcdf23d731c 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -329,6 +329,7 @@ struct mmc_card {
 #define MMC_QUIRK_BROKEN_SD_CACHE	(1<<15)	/* Disable broken SD cache support */
 #define MMC_QUIRK_BROKEN_CACHE_FLUSH	(1<<16)	/* Don't flush cache until the write has occurred */
 #define MMC_QUIRK_BROKEN_SD_POWEROFF_NOTIFY	(1<<17) /* Disable broken SD poweroff notify support */
+#define MMC_QUIRK_NO_UHS_DDR50_TUNING	(1<<18) /* Disable DDR50 tuning */
 
 	bool			written_flag;	/* Indicates eMMC has been written since power on */
 	bool			reenable_cmdq;	/* Re-enable Command Queue */
-- 

Regards,
Erick

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

[Ricardo B. Marlière]

On Thu Feb 6, 2025 at 6:08 PM -03, 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>
> ---
>  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;

Reviewed-by: Ricardo B. Marlière <ricardo@marliere.net>

>  
>  	sdhci_end_tuning(host);
>  out:

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

[Adrian Hunter]

On 6/02/25 23:08, 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>
> ---
>  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 uses fixed sampling clock if tuning fails.

It is up to drivers to decide what to do with tuning_err.  A driver
can provide the tuning mmc host op, call sdhci_execute_tuning()
and return tuning_err it that is preferred.

>  
>  	sdhci_end_tuning(host);
>  out:

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

[Erick Shepherd]

I see, in my case intel_execute_tuning() in sdhci-pci-core.c is what
is calling into sdhci_execute_tuning() so I should check the value of
tuning_err there and possibly return it? The issue I'm trying to solve
is only for DDR50 cards that do not support tuning so I could
conditionally return tuning_err if the timing mode is DDR50. Maybe
something like this?

---
 drivers/mmc/host/sdhci-pci-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 1f0bd723f011..9aedb476bd5d 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -725,6 +725,9 @@ static int intel_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	if (err)
 		return err;
 
+	if (host->tuning_err && mmc->ios.timing == MMC_TIMING_UHS_DDR50)
+		return host->tuning_err;
+
 	/*
 	 * Tuning can leave the IP in an active state (Buffer Read Enable bit
 	 * set) which prevents the entry to low power states (i.e. S0i3). Data
-- 

Regards
Erick