On 28/11/22 13:13, Wenchao Chen wrote:
> On Fri, Nov 25, 2022 at 1:12 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
>> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>> ---
>> 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)) {
> ...
> }
