| Message ID | 20250127223654.290904-2-erick.shepherd@ni.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/2] mmc: Update sdhci tune function to return errors | expand |
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 */
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.
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
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 */
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(-)