* [PATCH 0/4] mmc: sdhci: Fix voltage switch delay
@ 2022-11-24 17:06 Adrian Hunter
2022-11-24 17:06 ` [PATCH 1/4] " Adrian Hunter
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Adrian Hunter @ 2022-11-24 17:06 UTC (permalink / raw)
To: Ulf Hansson
Cc: Sarthak Garg, Florian Fainelli, Al Cooper, Haibo Chen,
Andrew Jeffery, Eugen Hristev, Vignesh Raghavendra,
Prabu Thangamuthu, Manjunath M B, Ben Dooks, Jaehoon Chung,
Viresh Kumar, Thierry Reding, Hu Ziji, Wolfram Sang,
Sascha Hauer, Brian Norris, Wenchao Chen, Chevron Li, linux-mmc
Hi
Here is a fix for the voltage switch delay issue raised here:
https://lore.kernel.org/linux-mmc/20221117094859.20582-1-quic_sartgarg@quicinc.com/
Plus some subsequent tidy-ups.
Adrian Hunter (4):
mmc: sdhci: Fix voltage switch delay
mmc: sdhci: Avoid unnecessary re-configuration
mmc: sdhci: Avoid unnecessary ->set_clock()
mmc: sdhci: Enable card clock instead of ->set_clock()
drivers/mmc/host/sdhci.c | 91 +++++++++++++++++++++++++++++++-----------------
drivers/mmc/host/sdhci.h | 2 ++
2 files changed, 61 insertions(+), 32 deletions(-)
Regards
Adrian
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/4] mmc: sdhci: Fix voltage switch delay
2022-11-24 17:06 [PATCH 0/4] mmc: sdhci: Fix voltage switch delay Adrian Hunter
@ 2022-11-24 17:06 ` Adrian Hunter
2022-11-25 9:41 ` Bough Chen
` (2 more replies)
2022-11-24 17:06 ` [PATCH 2/4] mmc: sdhci: Avoid unnecessary re-configuration Adrian Hunter
` (2 subsequent siblings)
3 siblings, 3 replies; 10+ messages in thread
From: Adrian Hunter @ 2022-11-24 17:06 UTC (permalink / raw)
To: Ulf Hansson
Cc: Sarthak Garg, Florian Fainelli, Al Cooper, Haibo Chen,
Andrew Jeffery, Eugen Hristev, Vignesh Raghavendra,
Prabu Thangamuthu, Manjunath M B, Ben Dooks, Jaehoon Chung,
Viresh Kumar, Thierry Reding, Hu Ziji, Wolfram Sang,
Sascha Hauer, Brian Norris, Wenchao Chen, Chevron Li, linux-mmc
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] mmc: sdhci: Avoid unnecessary re-configuration
2022-11-24 17:06 [PATCH 0/4] mmc: sdhci: Fix voltage switch delay Adrian Hunter
2022-11-24 17:06 ` [PATCH 1/4] " Adrian Hunter
@ 2022-11-24 17:06 ` Adrian Hunter
2022-11-24 17:06 ` [PATCH 3/4] mmc: sdhci: Avoid unnecessary ->set_clock() Adrian Hunter
2022-11-24 17:06 ` [PATCH 4/4] mmc: sdhci: Enable card clock instead of ->set_clock() Adrian Hunter
3 siblings, 0 replies; 10+ messages in thread
From: Adrian Hunter @ 2022-11-24 17:06 UTC (permalink / raw)
To: Ulf Hansson
Cc: Sarthak Garg, Florian Fainelli, Al Cooper, Haibo Chen,
Andrew Jeffery, Eugen Hristev, Vignesh Raghavendra,
Prabu Thangamuthu, Manjunath M B, Ben Dooks, Jaehoon Chung,
Viresh Kumar, Thierry Reding, Hu Ziji, Wolfram Sang,
Sascha Hauer, Brian Norris, Wenchao Chen, Chevron Li, linux-mmc
Avoid re-configuring UHS and preset settings when the settings have not
changed, irrespective of whether the clock is turning on.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 351b09b90d76..3675d69fb590 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2304,7 +2304,6 @@ 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;
@@ -2334,8 +2333,6 @@ 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;
@@ -2363,11 +2360,10 @@ 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.
+ * Avoid unnecessary changes. In particular, this avoids 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) &&
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] mmc: sdhci: Avoid unnecessary ->set_clock()
2022-11-24 17:06 [PATCH 0/4] mmc: sdhci: Fix voltage switch delay Adrian Hunter
2022-11-24 17:06 ` [PATCH 1/4] " Adrian Hunter
2022-11-24 17:06 ` [PATCH 2/4] mmc: sdhci: Avoid unnecessary re-configuration Adrian Hunter
@ 2022-11-24 17:06 ` Adrian Hunter
2022-11-24 17:06 ` [PATCH 4/4] mmc: sdhci: Enable card clock instead of ->set_clock() Adrian Hunter
3 siblings, 0 replies; 10+ messages in thread
From: Adrian Hunter @ 2022-11-24 17:06 UTC (permalink / raw)
To: Ulf Hansson
Cc: Sarthak Garg, Florian Fainelli, Al Cooper, Haibo Chen,
Andrew Jeffery, Eugen Hristev, Vignesh Raghavendra,
Prabu Thangamuthu, Manjunath M B, Ben Dooks, Jaehoon Chung,
Viresh Kumar, Thierry Reding, Hu Ziji, Wolfram Sang,
Sascha Hauer, Brian Norris, Wenchao Chen, Chevron Li, linux-mmc
To avoid glitches on the clock line, the card clock is disabled when making
timing changes. Do not do that separately for HISPD and UHS settings.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci.c | 37 ++++++++++++++-----------------------
1 file changed, 14 insertions(+), 23 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 3675d69fb590..79a50d461ee4 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2390,8 +2390,21 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
if (host->version >= SDHCI_SPEC_300) {
u16 clk, ctrl_2;
+ /*
+ * According to SDHCI Spec v3.00, if the Preset Value
+ * Enable in the Host Control 2 register is set, we
+ * need to reset SD Clock Enable before changing High
+ * Speed Enable to avoid generating clock glitches.
+ */
+ clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+ if (clk & SDHCI_CLOCK_CARD_EN) {
+ clk &= ~SDHCI_CLOCK_CARD_EN;
+ sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+ }
+
+ sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+
if (!host->preset_enabled) {
- sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
/*
* We only need to set Driver Strength if the
* preset value enable is not set.
@@ -2414,30 +2427,8 @@ 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
- * Enable in the Host Control 2 register is set, we
- * need to reset SD Clock Enable before changing High
- * Speed Enable to avoid generating clock gliches.
- */
-
- /* Reset SD Clock Enable */
- clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
- clk &= ~SDHCI_CLOCK_CARD_EN;
- sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
-
- sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
-
- /* Re-enable SD Clock */
- host->ops->set_clock(host, host->clock);
}
- /* Reset SD Clock Enable */
- clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
- clk &= ~SDHCI_CLOCK_CARD_EN;
- sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
-
host->ops->set_uhs_signaling(host, ios->timing);
host->timing = ios->timing;
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] mmc: sdhci: Enable card clock instead of ->set_clock()
2022-11-24 17:06 [PATCH 0/4] mmc: sdhci: Fix voltage switch delay Adrian Hunter
` (2 preceding siblings ...)
2022-11-24 17:06 ` [PATCH 3/4] mmc: sdhci: Avoid unnecessary ->set_clock() Adrian Hunter
@ 2022-11-24 17:06 ` Adrian Hunter
3 siblings, 0 replies; 10+ messages in thread
From: Adrian Hunter @ 2022-11-24 17:06 UTC (permalink / raw)
To: Ulf Hansson
Cc: Sarthak Garg, Florian Fainelli, Al Cooper, Haibo Chen,
Andrew Jeffery, Eugen Hristev, Vignesh Raghavendra,
Prabu Thangamuthu, Manjunath M B, Ben Dooks, Jaehoon Chung,
Viresh Kumar, Thierry Reding, Hu Ziji, Wolfram Sang,
Sascha Hauer, Brian Norris, Wenchao Chen, Chevron Li, linux-mmc
SDHCI has separate controls for the internal clock and enabling the
clock signal to the card.
The card clock signal was disabled via SDHCI_CLOCK_CARD_EN to avoid
glitches on the clock line. It is not necessary to reset the internal clock
to re-enable it. Instead re-enable by re-asserting SDHCI_CLOCK_CARD_EN.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 79a50d461ee4..19e0cb7add94 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2442,8 +2442,11 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
host->drv_type = ios->drv_type;
}
- /* Re-enable SD Clock */
- host->ops->set_clock(host, host->clock);
+ if (ios->clock) {
+ clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+ clk |= SDHCI_CLOCK_CARD_EN;
+ sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+ }
} else
sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH 1/4] mmc: sdhci: Fix voltage switch delay
2022-11-24 17:06 ` [PATCH 1/4] " Adrian Hunter
@ 2022-11-25 9:41 ` Bough Chen
2022-11-25 10:04 ` Adrian Hunter
2022-11-25 12:01 ` Ulf Hansson
2022-11-28 11:13 ` Wenchao Chen
2 siblings, 1 reply; 10+ messages in thread
From: Bough Chen @ 2022-11-25 9:41 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson
Cc: Sarthak Garg, Florian Fainelli, Al Cooper, Andrew Jeffery,
Eugen Hristev, Vignesh Raghavendra, Prabu Thangamuthu,
Manjunath M B, Ben Dooks, Jaehoon Chung, Viresh Kumar,
Thierry Reding, Hu Ziji, Wolfram Sang, Sascha Hauer,
Brian Norris, Wenchao Chen, Chevron Li, linux-mmc
> -----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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] mmc: sdhci: Fix voltage switch delay
2022-11-25 9:41 ` Bough Chen
@ 2022-11-25 10:04 ` Adrian Hunter
0 siblings, 0 replies; 10+ messages in thread
From: Adrian Hunter @ 2022-11-25 10:04 UTC (permalink / raw)
To: Bough Chen, Ulf Hansson
Cc: Sarthak Garg, Florian Fainelli, Al Cooper, Andrew Jeffery,
Eugen Hristev, Vignesh Raghavendra, Prabu Thangamuthu,
Manjunath M B, Ben Dooks, Jaehoon Chung, Viresh Kumar,
Thierry Reding, Hu Ziji, Wolfram Sang, Sascha Hauer,
Brian Norris, Wenchao Chen, Chevron Li, linux-mmc
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] mmc: sdhci: Fix voltage switch delay
2022-11-24 17:06 ` [PATCH 1/4] " Adrian Hunter
2022-11-25 9:41 ` Bough Chen
@ 2022-11-25 12:01 ` Ulf Hansson
2022-11-28 11:13 ` Wenchao Chen
2 siblings, 0 replies; 10+ messages in thread
From: Ulf Hansson @ 2022-11-25 12:01 UTC (permalink / raw)
To: Adrian Hunter
Cc: Sarthak Garg, Florian Fainelli, Al Cooper, Haibo Chen,
Andrew Jeffery, Eugen Hristev, Vignesh Raghavendra,
Prabu Thangamuthu, Manjunath M B, Ben Dooks, Jaehoon Chung,
Viresh Kumar, Thierry Reding, Hu Ziji, Wolfram Sang,
Sascha Hauer, Brian Norris, Wenchao Chen, Chevron Li, linux-mmc
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] mmc: sdhci: Fix voltage switch delay
2022-11-24 17:06 ` [PATCH 1/4] " Adrian Hunter
2022-11-25 9:41 ` Bough Chen
2022-11-25 12:01 ` Ulf Hansson
@ 2022-11-28 11:13 ` Wenchao Chen
2022-11-28 13:38 ` Adrian Hunter
2 siblings, 1 reply; 10+ messages in thread
From: Wenchao Chen @ 2022-11-28 11:13 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, Sarthak Garg, Florian Fainelli, Al Cooper,
Haibo Chen, Andrew Jeffery, Eugen Hristev, Vignesh Raghavendra,
Prabu Thangamuthu, Manjunath M B, Ben Dooks, Jaehoon Chung,
Viresh Kumar, Thierry Reding, Hu Ziji, Wolfram Sang,
Sascha Hauer, Brian Norris, Wenchao Chen, Chevron Li, linux-mmc
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)) {
...
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] mmc: sdhci: Fix voltage switch delay
2022-11-28 11:13 ` Wenchao Chen
@ 2022-11-28 13:38 ` Adrian Hunter
0 siblings, 0 replies; 10+ messages in thread
From: Adrian Hunter @ 2022-11-28 13:38 UTC (permalink / raw)
To: Wenchao Chen
Cc: Ulf Hansson, Sarthak Garg, Florian Fainelli, Al Cooper,
Haibo Chen, Andrew Jeffery, Eugen Hristev, Vignesh Raghavendra,
Prabu Thangamuthu, Manjunath M B, Ben Dooks, Jaehoon Chung,
Viresh Kumar, Thierry Reding, Hu Ziji, Wolfram Sang,
Sascha Hauer, Brian Norris, Wenchao Chen, Chevron Li, linux-mmc
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)) {
> ...
> }
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-11-28 13:39 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-24 17:06 [PATCH 0/4] mmc: sdhci: Fix voltage switch delay Adrian Hunter
2022-11-24 17:06 ` [PATCH 1/4] " Adrian Hunter
2022-11-25 9:41 ` Bough Chen
2022-11-25 10:04 ` Adrian Hunter
2022-11-25 12:01 ` Ulf Hansson
2022-11-28 11:13 ` Wenchao Chen
2022-11-28 13:38 ` Adrian Hunter
2022-11-24 17:06 ` [PATCH 2/4] mmc: sdhci: Avoid unnecessary re-configuration Adrian Hunter
2022-11-24 17:06 ` [PATCH 3/4] mmc: sdhci: Avoid unnecessary ->set_clock() Adrian Hunter
2022-11-24 17:06 ` [PATCH 4/4] mmc: sdhci: Enable card clock instead of ->set_clock() Adrian Hunter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.