| Message ID | 20200824122131.v2.1.Id6f3c92fecf4acc60c3b7f57d5f4e4c854ace765@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mmc: sdhci: Don't enable presets while tuning | expand |
On 24/08/20 9:21 pm, Raul E Rangel wrote: > SDHCI presets are not currently used for eMMC HS/HS200/HS400, but are > used for DDR52. The HS400 retuning sequence is: > > HS400->DDR52->HS->HS200->Perform Tuning->HS->HS400 > > This means that when HS400 tuning happens, we transition through DDR52 > for a very brief period. This causes presets to be enabled > unintentionally and stay enabled when transitioning back to HS200 or > HS400. > > This patch prevents enabling presets while tuning is in progress. Preset value should not generally have to depend on tuning, so this seems less than ideal. Also I am not sure you can say some controllers are not accidentally benefiting from the current situation. What about just letting drivers choose the timing modes that support preset values? e.g. using the change below, a driver could alter host->preset_value_support as needed diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 3ad394b40eb1..3e69c25c90a3 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2360,12 +2360,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) host->timing = ios->timing; if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) && - ((ios->timing == MMC_TIMING_UHS_SDR12) || - (ios->timing == MMC_TIMING_UHS_SDR25) || - (ios->timing == MMC_TIMING_UHS_SDR50) || - (ios->timing == MMC_TIMING_UHS_SDR104) || - (ios->timing == MMC_TIMING_UHS_DDR50) || - (ios->timing == MMC_TIMING_MMC_DDR52))) { + sdhci_preset_value_support(host, ios->timing)) { u16 preset; sdhci_enable_preset_value(host, true); @@ -3934,6 +3929,13 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev, */ host->adma_table_cnt = SDHCI_MAX_SEGS * 2 + 1; + host->preset_value_support = (1 << MMC_TIMING_UHS_SDR12 ) | + (1 << MMC_TIMING_UHS_SDR25 ) | + (1 << MMC_TIMING_UHS_SDR50 ) | + (1 << MMC_TIMING_UHS_SDR104) | + (1 << MMC_TIMING_UHS_DDR50 ) | + (1 << MMC_TIMING_MMC_DDR52 ); + return host; } diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index 0770c036e2ff..79be471ff934 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -603,6 +603,9 @@ struct sdhci_host { /* Host ADMA table count */ u32 adma_table_cnt; + /* Which transfer modes support preset value */ + u32 preset_value_support; + u64 data_timeout; unsigned long private[] ____cacheline_aligned; @@ -760,6 +763,14 @@ static inline void sdhci_read_caps(struct sdhci_host *host) __sdhci_read_caps(host, NULL, NULL, NULL); } +static inline bool sdhci_preset_value_support(struct sdhci_host *host, + unsigned char timing) +{ + if (timing < 32) + return host->preset_value_support & (1 << timing); + return false; +} + u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock, unsigned int *actual_clock); void sdhci_set_clock(struct sdhci_host *host, unsigned int clock); > > Fixes: 0dafa60eb2506 ("mmc: sdhci: also get preset value and driver type for MMC_DDR52") > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > --- > The indentation changed because I ran clang-format > > Changes in v2: > - Fixed commit message. Patman didn't properly strip off the TEST= line. > > drivers/mmc/host/sdhci.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 37b1158c1c0c9..fd702c436c165 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2360,12 +2360,13 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > host->timing = ios->timing; > > if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) && > - ((ios->timing == MMC_TIMING_UHS_SDR12) || > - (ios->timing == MMC_TIMING_UHS_SDR25) || > - (ios->timing == MMC_TIMING_UHS_SDR50) || > - (ios->timing == MMC_TIMING_UHS_SDR104) || > - (ios->timing == MMC_TIMING_UHS_DDR50) || > - (ios->timing == MMC_TIMING_MMC_DDR52))) { > + !mmc_doing_retune(mmc) && > + ((ios->timing == MMC_TIMING_UHS_SDR12) || > + (ios->timing == MMC_TIMING_UHS_SDR25) || > + (ios->timing == MMC_TIMING_UHS_SDR50) || > + (ios->timing == MMC_TIMING_UHS_SDR104) || > + (ios->timing == MMC_TIMING_UHS_DDR50) || > + (ios->timing == MMC_TIMING_MMC_DDR52))) { > u16 preset; > > sdhci_enable_preset_value(host, true); >
On Tue, Sep 1, 2020 at 4:54 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 24/08/20 9:21 pm, Raul E Rangel wrote: > > SDHCI presets are not currently used for eMMC HS/HS200/HS400, but are > > used for DDR52. The HS400 retuning sequence is: > > > > HS400->DDR52->HS->HS200->Perform Tuning->HS->HS400 > > > > This means that when HS400 tuning happens, we transition through DDR52 > > for a very brief period. This causes presets to be enabled > > unintentionally and stay enabled when transitioning back to HS200 or > > HS400. > > > > This patch prevents enabling presets while tuning is in progress. > > Preset value should not generally have to depend on tuning, so this > seems less than ideal. Also I am not sure you can say some controllers > are not accidentally benefiting from the current situation. > > What about just letting drivers choose the timing modes that support > preset values? e.g. using the change below, a driver could alter > host->preset_value_support as needed Sorry for the late reply, I'm just getting back to this. I like the patch. I have a few other patches I'm going to push up soon. Do you want me to include this in the chain, or do you want to push it up? Thanks, Raul > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 3ad394b40eb1..3e69c25c90a3 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2360,12 +2360,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > host->timing = ios->timing; > > if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) && > - ((ios->timing == MMC_TIMING_UHS_SDR12) || > - (ios->timing == MMC_TIMING_UHS_SDR25) || > - (ios->timing == MMC_TIMING_UHS_SDR50) || > - (ios->timing == MMC_TIMING_UHS_SDR104) || > - (ios->timing == MMC_TIMING_UHS_DDR50) || > - (ios->timing == MMC_TIMING_MMC_DDR52))) { > + sdhci_preset_value_support(host, ios->timing)) { > u16 preset; > > sdhci_enable_preset_value(host, true); > @@ -3934,6 +3929,13 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev, > */ > host->adma_table_cnt = SDHCI_MAX_SEGS * 2 + 1; > > + host->preset_value_support = (1 << MMC_TIMING_UHS_SDR12 ) | > + (1 << MMC_TIMING_UHS_SDR25 ) | > + (1 << MMC_TIMING_UHS_SDR50 ) | > + (1 << MMC_TIMING_UHS_SDR104) | > + (1 << MMC_TIMING_UHS_DDR50 ) | > + (1 << MMC_TIMING_MMC_DDR52 ); > + > return host; > } > > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index 0770c036e2ff..79be471ff934 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -603,6 +603,9 @@ struct sdhci_host { > /* Host ADMA table count */ > u32 adma_table_cnt; > > + /* Which transfer modes support preset value */ > + u32 preset_value_support; > + > u64 data_timeout; > > unsigned long private[] ____cacheline_aligned; > @@ -760,6 +763,14 @@ static inline void sdhci_read_caps(struct sdhci_host *host) > __sdhci_read_caps(host, NULL, NULL, NULL); > } > > +static inline bool sdhci_preset_value_support(struct sdhci_host *host, > + unsigned char timing) > +{ > + if (timing < 32) > + return host->preset_value_support & (1 << timing); > + return false; > +} > + > u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock, > unsigned int *actual_clock); > void sdhci_set_clock(struct sdhci_host *host, unsigned int clock); > > > > > > > > > Fixes: 0dafa60eb2506 ("mmc: sdhci: also get preset value and driver type for MMC_DDR52") > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > --- > > The indentation changed because I ran clang-format > > > > Changes in v2: > > - Fixed commit message. Patman didn't properly strip off the TEST= line. > > > > drivers/mmc/host/sdhci.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index 37b1158c1c0c9..fd702c436c165 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -2360,12 +2360,13 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > > host->timing = ios->timing; > > > > if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) && > > - ((ios->timing == MMC_TIMING_UHS_SDR12) || > > - (ios->timing == MMC_TIMING_UHS_SDR25) || > > - (ios->timing == MMC_TIMING_UHS_SDR50) || > > - (ios->timing == MMC_TIMING_UHS_SDR104) || > > - (ios->timing == MMC_TIMING_UHS_DDR50) || > > - (ios->timing == MMC_TIMING_MMC_DDR52))) { > > + !mmc_doing_retune(mmc) && > > + ((ios->timing == MMC_TIMING_UHS_SDR12) || > > + (ios->timing == MMC_TIMING_UHS_SDR25) || > > + (ios->timing == MMC_TIMING_UHS_SDR50) || > > + (ios->timing == MMC_TIMING_UHS_SDR104) || > > + (ios->timing == MMC_TIMING_UHS_DDR50) || > > + (ios->timing == MMC_TIMING_MMC_DDR52))) { > > u16 preset; > > > > sdhci_enable_preset_value(host, true); > > >
On 18/09/20 8:57 pm, Raul Rangel wrote: > On Tue, Sep 1, 2020 at 4:54 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 24/08/20 9:21 pm, Raul E Rangel wrote: >>> SDHCI presets are not currently used for eMMC HS/HS200/HS400, but are >>> used for DDR52. The HS400 retuning sequence is: >>> >>> HS400->DDR52->HS->HS200->Perform Tuning->HS->HS400 >>> >>> This means that when HS400 tuning happens, we transition through DDR52 >>> for a very brief period. This causes presets to be enabled >>> unintentionally and stay enabled when transitioning back to HS200 or >>> HS400. >>> >>> This patch prevents enabling presets while tuning is in progress. >> >> Preset value should not generally have to depend on tuning, so this >> seems less than ideal. Also I am not sure you can say some controllers >> are not accidentally benefiting from the current situation. >> >> What about just letting drivers choose the timing modes that support >> preset values? e.g. using the change below, a driver could alter >> host->preset_value_support as needed > > Sorry for the late reply, I'm just getting back to this. I like the > patch. I have a few other patches I'm > going to push up soon. Do you want me to include this in the chain, or > do you want to push it up? I'm snowed. You will have to do it I am afraid.
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 37b1158c1c0c9..fd702c436c165 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2360,12 +2360,13 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) host->timing = ios->timing; if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) && - ((ios->timing == MMC_TIMING_UHS_SDR12) || - (ios->timing == MMC_TIMING_UHS_SDR25) || - (ios->timing == MMC_TIMING_UHS_SDR50) || - (ios->timing == MMC_TIMING_UHS_SDR104) || - (ios->timing == MMC_TIMING_UHS_DDR50) || - (ios->timing == MMC_TIMING_MMC_DDR52))) { + !mmc_doing_retune(mmc) && + ((ios->timing == MMC_TIMING_UHS_SDR12) || + (ios->timing == MMC_TIMING_UHS_SDR25) || + (ios->timing == MMC_TIMING_UHS_SDR50) || + (ios->timing == MMC_TIMING_UHS_SDR104) || + (ios->timing == MMC_TIMING_UHS_DDR50) || + (ios->timing == MMC_TIMING_MMC_DDR52))) { u16 preset; sdhci_enable_preset_value(host, true);
SDHCI presets are not currently used for eMMC HS/HS200/HS400, but are used for DDR52. The HS400 retuning sequence is: HS400->DDR52->HS->HS200->Perform Tuning->HS->HS400 This means that when HS400 tuning happens, we transition through DDR52 for a very brief period. This causes presets to be enabled unintentionally and stay enabled when transitioning back to HS200 or HS400. This patch prevents enabling presets while tuning is in progress. Fixes: 0dafa60eb2506 ("mmc: sdhci: also get preset value and driver type for MMC_DDR52") Signed-off-by: Raul E Rangel <rrangel@chromium.org> --- The indentation changed because I ran clang-format Changes in v2: - Fixed commit message. Patman didn't properly strip off the TEST= line. drivers/mmc/host/sdhci.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)