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