diff mbox series

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

Message ID 20221124170649.63851-2-adrian.hunter@intel.com (mailing list archive)
State New, archived
Headers show
Series mmc: sdhci: Fix voltage switch delay | expand

Commit Message

Adrian Hunter Nov. 24, 2022, 5:06 p.m. UTC
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(-)

Comments

Bough Chen Nov. 25, 2022, 9:41 a.m. UTC | #1
> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: 2022年11月25日 1:07
> To: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sarthak Garg <quic_sartgarg@quicinc.com>; Florian Fainelli
> <f.fainelli@gmail.com>; Al Cooper <alcooperx@gmail.com>; Bough Chen
> <haibo.chen@nxp.com>; Andrew Jeffery <andrew@aj.id.au>; Eugen Hristev
> <eugen.hristev@microchip.com>; Vignesh Raghavendra <vigneshr@ti.com>;
> Prabu Thangamuthu <prabu.t@synopsys.com>; Manjunath M B
> <manjumb@synopsys.com>; Ben Dooks <ben-linux@fluff.org>; Jaehoon Chung
> <jh80.chung@samsung.com>; Viresh Kumar <vireshk@kernel.org>; Thierry
> Reding <thierry.reding@gmail.com>; Hu Ziji <huziji@marvell.com>; Wolfram
> Sang <wsa+renesas@sang-engineering.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Brian Norris <briannorris@chromium.org>;
> Wenchao Chen <wenchao.chen@unisoc.com>; Chevron Li
> <chevron.li@bayhubtech.com>; linux-mmc@vger.kernel.org
> Subject: [PATCH 1/4] mmc: sdhci: Fix voltage switch delay
> 
> 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>

Hi Adrian,

These patch set take care of the card clock on/off by CARD_EN bit, but for the USDHC on i.MX series, it do not implement these bits. Instead, it use another bit named force_clk_on to gate on/off the card clock.
So logically, USDHC do not has this issue.
I apply your patch and did a quick test on i.MX platform, do not find any side effect.

So for the patch set:
Tested-by: Haibo Chen <haibo.chen@nxp.com>

Best Regards
Haibo Chen 
> ---
>  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
Adrian Hunter Nov. 25, 2022, 10:04 a.m. UTC | #2
On 25/11/22 11:41, Bough Chen wrote:
>> -----Original Message-----
>> From: Adrian Hunter <adrian.hunter@intel.com>
>> Sent: 2022年11月25日 1:07
>> To: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Sarthak Garg <quic_sartgarg@quicinc.com>; Florian Fainelli
>> <f.fainelli@gmail.com>; Al Cooper <alcooperx@gmail.com>; Bough Chen
>> <haibo.chen@nxp.com>; Andrew Jeffery <andrew@aj.id.au>; Eugen Hristev
>> <eugen.hristev@microchip.com>; Vignesh Raghavendra <vigneshr@ti.com>;
>> Prabu Thangamuthu <prabu.t@synopsys.com>; Manjunath M B
>> <manjumb@synopsys.com>; Ben Dooks <ben-linux@fluff.org>; Jaehoon Chung
>> <jh80.chung@samsung.com>; Viresh Kumar <vireshk@kernel.org>; Thierry
>> Reding <thierry.reding@gmail.com>; Hu Ziji <huziji@marvell.com>; Wolfram
>> Sang <wsa+renesas@sang-engineering.com>; Sascha Hauer
>> <s.hauer@pengutronix.de>; Brian Norris <briannorris@chromium.org>;
>> Wenchao Chen <wenchao.chen@unisoc.com>; Chevron Li
>> <chevron.li@bayhubtech.com>; linux-mmc@vger.kernel.org
>> Subject: [PATCH 1/4] mmc: sdhci: Fix voltage switch delay
>>
>> 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>
> 
> Hi Adrian,
> 
> These patch set take care of the card clock on/off by CARD_EN bit, but for the USDHC on i.MX series, it do not implement these bits. Instead, it use another bit named force_clk_on to gate on/off the card clock.
> So logically, USDHC do not has this issue.
> I apply your patch and did a quick test on i.MX platform, do not find any side effect.
> 
> So for the patch set:
> Tested-by: Haibo Chen <haibo.chen@nxp.com>

Thanks for testings this!

> 
> Best Regards
> Haibo Chen 
>> ---
>>  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
>
Ulf Hansson Nov. 25, 2022, 12:01 p.m. UTC | #3
On Thu, 24 Nov 2022 at 18:07, 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>

[...]

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

This caught my eyes, as it looks a bit messy. Not sure if it's
possible to simplify, but the last two lines could be replaced with:

(host->preset_enabled ||
(host->drv_type == ios->drv_type && !sdhci_preset_needed(host, ios->timing))))

But, I am not sure that really helps...

[...]

Kind regards
Uffe
Wenchao Chen Nov. 28, 2022, 11:13 a.m. UTC | #4
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?
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)) {
...
}
Adrian Hunter Nov. 28, 2022, 1:38 p.m. UTC | #5
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)) {
> ...
> }
diff mbox series

Patch

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 */