Re: [PATCH v2] mmc: sdhci: Don't enable presets while tuning

[Adrian Hunter]

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@xxxxxxxxxxxx>
> ---
> 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);
>