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

[Raul Rangel]

On Tue, Sep 1, 2020 at 4:54 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> 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@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);
> >
>