linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] mmc: sdhci: Don't enable presets while tuning
@ 2020-08-24 18:21 Raul E Rangel
  2020-09-01 10:53 ` Adrian Hunter
  0 siblings, 1 reply; 4+ messages in thread
From: Raul E Rangel @ 2020-08-24 18:21 UTC (permalink / raw)
  To: adrian.hunter
  Cc: Nehal-bakulchandra.Shah, chris.wang, Akshu.Agrawal,
	Raul E Rangel, Jisheng Zhang, Ulf Hansson, clang-built-linux,
	linux-kernel, linux-mmc

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(-)

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);
-- 
2.28.0.297.g1956fa8f8d-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] mmc: sdhci: Don't enable presets while tuning
  2020-08-24 18:21 [PATCH v2] mmc: sdhci: Don't enable presets while tuning Raul E Rangel
@ 2020-09-01 10:53 ` Adrian Hunter
  2020-09-18 17:57   ` Raul Rangel
  0 siblings, 1 reply; 4+ messages in thread
From: Adrian Hunter @ 2020-09-01 10:53 UTC (permalink / raw)
  To: Raul E Rangel
  Cc: Nehal-bakulchandra.Shah, chris.wang, Akshu.Agrawal,
	Jisheng Zhang, Ulf Hansson, clang-built-linux, linux-kernel,
	linux-mmc

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] mmc: sdhci: Don't enable presets while tuning
  2020-09-01 10:53 ` Adrian Hunter
@ 2020-09-18 17:57   ` Raul Rangel
  2020-09-25  9:31     ` Adrian Hunter
  0 siblings, 1 reply; 4+ messages in thread
From: Raul Rangel @ 2020-09-18 17:57 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Shah, Nehal-bakulchandra, Wang, Chris, Akshu Agrawal,
	Jisheng Zhang, Ulf Hansson, clang-built-linux, linux-kernel,
	linux-mmc

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] mmc: sdhci: Don't enable presets while tuning
  2020-09-18 17:57   ` Raul Rangel
@ 2020-09-25  9:31     ` Adrian Hunter
  0 siblings, 0 replies; 4+ messages in thread
From: Adrian Hunter @ 2020-09-25  9:31 UTC (permalink / raw)
  To: Raul Rangel
  Cc: Shah, Nehal-bakulchandra, Wang, Chris, Akshu Agrawal,
	Jisheng Zhang, Ulf Hansson, clang-built-linux, linux-kernel,
	linux-mmc

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-25  9:32 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-24 18:21 [PATCH v2] mmc: sdhci: Don't enable presets while tuning Raul E Rangel
2020-09-01 10:53 ` Adrian Hunter
2020-09-18 17:57   ` Raul Rangel
2020-09-25  9:31     ` Adrian Hunter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).