Re: [PATCH 1/4] mmc: sdhci: Fix voltage switch delay

[Adrian Hunter <adrian.hunter@intel.com>]

On 28/11/22 13:13, Wenchao Chen wrote:
> On Fri, Nov 25, 2022 at 1:12 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> Commit 20b92a30b561 ("mmc: sdhci: update signal voltage switch code")
>> removed voltage switch delays from sdhci because mmc core had been
>> enhanced to support them. However that assumed that sdhci_set_ios()
>> did a single clock change, which it did not, and so the delays in mmc
>> core, which should have come after the first clock change, were not
>> effective.
>>
>> Fix by avoiding re-configuring UHS and preset settings when the clock
>> is turning on and the settings have not changed. That then also avoids
>> the associated clock changes, so that then sdhci_set_ios() does a single
>> clock change when voltage switching, and the mmc core delays become
>> effective.
>>
>> To do that has meant keeping track of driver strength (host->drv_type),
>> and cases of reinitialization (host->reinit_uhs).
>>
>> Note also, the 'turning_on_clk' restriction should not be necessary
>> but is done to minimize the impact of the change on stable kernels.
>>
>> Fixes: 20b92a30b561 ("mmc: sdhci: update signal voltage switch code")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/host/sdhci.c | 51 ++++++++++++++++++++++++++++++++++------
>>  drivers/mmc/host/sdhci.h |  2 ++
>>  2 files changed, 46 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 2108e8075609..351b09b90d76 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -373,6 +373,7 @@ static void sdhci_init(struct sdhci_host *host, int soft)
>>         if (soft) {
>>                 /* force clock reconfiguration */
>>                 host->clock = 0;
>> +               host->reinit_uhs = true;
>>                 mmc->ops->set_ios(mmc, &mmc->ios);
>>         }
>>  }
>> @@ -2279,11 +2280,35 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>>  }
>>  EXPORT_SYMBOL_GPL(sdhci_set_uhs_signaling);
>>
>> +static bool sdhci_timing_has_preset(unsigned char timing)
>> +{
>> +       switch (timing) {
>> +       case MMC_TIMING_UHS_SDR12:
>> +       case MMC_TIMING_UHS_SDR25:
>> +       case MMC_TIMING_UHS_SDR50:
>> +       case MMC_TIMING_UHS_SDR104:
>> +       case MMC_TIMING_UHS_DDR50:
>> +       case MMC_TIMING_MMC_DDR52:
>> +               return true;
>> +       };
>> +       return false;
>> +}
>> +
>> +static bool sdhci_preset_needed(struct sdhci_host *host, unsigned char timing)
>> +{
>> +       return !(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) &&
>> +              sdhci_timing_has_preset(timing);
>> +}
>> +
>>  void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>  {
>>         struct sdhci_host *host = mmc_priv(mmc);
>> +       bool reinit_uhs = host->reinit_uhs;
>> +       bool turning_on_clk = false;
>>         u8 ctrl;
>>
>> +       host->reinit_uhs = false;
>> +
>>         if (ios->power_mode == MMC_POWER_UNDEFINED)
>>                 return;
>>
>> @@ -2309,6 +2334,8 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>                 sdhci_enable_preset_value(host, false);
>>
>>         if (!ios->clock || ios->clock != host->clock) {
>> +               turning_on_clk = ios->clock && !host->clock;
>> +
>>                 host->ops->set_clock(host, ios->clock);
>>                 host->clock = ios->clock;
>>
>> @@ -2335,6 +2362,18 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>
>>         host->ops->set_bus_width(host, ios->bus_width);
>>
>> +       /*
>> +        * Special case to avoid multiple clock changes during voltage
>> +        * switching.
>> +        */
>> +       if (!reinit_uhs &&
>> +           turning_on_clk &&
>> +           host->timing == ios->timing &&
>> +           host->version >= SDHCI_SPEC_300 &&
>> +           (host->preset_enabled || host->drv_type == ios->drv_type) &&
>> +           (host->preset_enabled || !sdhci_preset_needed(host, ios->timing)))
>> +               return;
>> +
>>         ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>>
>>         if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT)) {
>> @@ -2378,6 +2417,7 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>                         }
>>
>>                         sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>> +                       host->drv_type = ios->drv_type;
>>                 } else {
>>                         /*
>>                          * According to SDHC Spec v3.00, if the Preset Value
>> @@ -2405,19 +2445,14 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>                 host->ops->set_uhs_signaling(host, ios->timing);
>>                 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))) {
>> +               if (sdhci_preset_needed(host, ios->timing)) {
>>                         u16 preset;
>>
>>                         sdhci_enable_preset_value(host, true);
>>                         preset = sdhci_get_preset_value(host);
>>                         ios->drv_type = FIELD_GET(SDHCI_PRESET_DRV_MASK,
>>                                                   preset);
>> +                       host->drv_type = ios->drv_type;
>>                 }
>>
>>                 /* Re-enable SD Clock */
>> @@ -3754,6 +3789,7 @@ int sdhci_resume_host(struct sdhci_host *host)
>>                 sdhci_init(host, 0);
>>                 host->pwr = 0;
>>                 host->clock = 0;
>> +               host->reinit_uhs = true;
>>                 mmc->ops->set_ios(mmc, &mmc->ios);
>>         } else {
>>                 sdhci_init(host, (mmc->pm_flags & MMC_PM_KEEP_POWER));
>> @@ -3816,6 +3852,7 @@ int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset)
>>                 /* Force clock and power re-program */
>>                 host->pwr = 0;
>>                 host->clock = 0;
>> +               host->reinit_uhs = true;
>>                 mmc->ops->start_signal_voltage_switch(mmc, &mmc->ios);
>>                 mmc->ops->set_ios(mmc, &mmc->ios);
>>
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 908da47ac5ba..b6f31a7d6152 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -524,6 +524,8 @@ struct sdhci_host {
>>
>>         unsigned int clock;     /* Current clock (MHz) */
>>         u8 pwr;                 /* Current voltage */
>> +       u8 drv_type;            /* Current UHS-I driver type */
>> +       bool reinit_uhs;        /* Force UHS-related re-initialization */
>>
>>         bool runtime_suspended; /* Host is runtime suspended */
>>         bool bus_on;            /* Bus power prevents runtime suspend */
>> --
>> 2.34.1
>>
> Hi Adrian
> If the return value is -EAGAIN, host->reinit_uhs has been set to
> false, will it affect trying to initialize?

No it is fine.
SDHCI host->reinit_uhs is usually false. It is set on
PM / reset paths when the registers can have been reset.

> int mmc_host_set_uhs_voltage(struct mmc_host *host)
> {
> u32 clock;
> 
> /*
> * During a signal voltage level switch, the clock must be gated
> * for 5 ms according to the SD spec
> */
> clock = host->ios.clock;
> host->ios.clock = 0;
> mmc_set_ios(host);
> 
> if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180))
> return -EAGAIN;    //<======= If returned here.
> 
> /* Keep clock gated for at least 10 ms, though spec only says 5 ms */
> mmc_delay(10);
> host->ios.clock = clock;
> mmc_set_ios(host);
> 
> return 0;
> }
> 
> static int mmc_sd_init_card(struct mmc_host *host, u32 ocr, struct
> mmc_card *oldcard) {
> ...
> retry:
> ...
>     host->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> if (mmc_host_set_uhs_voltage(host) ||
>     mmc_sd_init_uhs_card(card)) {
> ...
> }