* [RFT v2 PATCH] mmc: sd: try to enable UHS mode w/o power cycle from S3
@ 2017-08-29 3:53 Shawn Lin
[not found] ` <CAAXpJNvfSWHrJiLHoxmm+ENUDsGayQROnRTL=SoHr-7dX7R9+Q@mail.gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Shawn Lin @ 2017-08-29 3:53 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
CMD0 couldn't put the card back to 3.3v if can't do power
cycle for cards. The card is already in SDR12 then, so still
try to enable UHS mode.
Pleas note that, if the platform use a hardware fixed
sd power, you should still include a regulator-fixed
regulator with always-on property there. So mmc core
could handle this as well. Otherwise you shouldn't
expect UHS to work for that case.
Just compile only, please have a look and test.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/card.h | 17 +++++++++++++++++
drivers/mmc/core/core.c | 8 ++++++++
drivers/mmc/core/sd.c | 28 ++++++++++++++++++++++++++--
drivers/mmc/core/sd.h | 3 ++-
drivers/mmc/core/sdio.c | 2 +-
drivers/regulator/core.c | 14 ++++++++++++++
include/linux/mmc/host.h | 1 +
include/linux/regulator/consumer.h | 6 ++++++
8 files changed, 75 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
index f06cd91..a562110 100644
--- a/drivers/mmc/core/card.h
+++ b/drivers/mmc/core/card.h
@@ -218,4 +218,21 @@ static inline int mmc_card_broken_hpi(const struct mmc_card *c)
return c->quirks & MMC_QUIRK_BROKEN_HPI;
}
+static inline bool mmc_card_support_uhs(struct mmc_card *card)
+{
+ if (WARN_ON(!card))
+ return false;
+
+ switch (card->sd_bus_speed) {
+ case UHS_SDR104_BUS_SPEED:
+ case UHS_DDR50_BUS_SPEED:
+ case UHS_SDR50_BUS_SPEED:
+ case UHS_SDR25_BUS_SPEED:
+ case UHS_SDR12_BUS_SPEED:
+ return true;
+ default:
+ return false;
+ }
+}
+
#endif
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2643126..6b5c6c5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1424,6 +1424,14 @@ int mmc_regulator_get_supply(struct mmc_host *mmc)
}
EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
+bool mmc_regulator_can_power_off(struct regulator *supply)
+{
+ if (!IS_ERR(supply))
+ return regulator_is_always_on(supply);
+ return false;
+}
+EXPORT_SYMBOL_GPL(mmc_regulator_can_power_off);
+
/*
* Mask off any voltages we don't support and select
* the lowest voltage
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index a1b0aa1..7bec61b 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -721,7 +721,8 @@ struct device_type sd_type = {
/*
* Fetch CID from card.
*/
-int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
+int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
+ u32 ocr, u32 *cid, u32 *rocr)
{
int err;
u32 max_current;
@@ -773,6 +774,27 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
return err;
/*
+ * Per SD physical specification version 4.10, section
+ * 3.9.4 UHS-I Bus Speed Mode Selection Sequence, once
+ * the signal voltage is switched to 1V8, the card continues
+ * 1.8V signaling regardless of CMD0. Power cycle resets the
+ * signal voltage to 3.3V. As a fact that some vmmc is always
+ * on so the card would still work with SDR12 after CMD0. So
+ * let's attempt to enable enable UHS mode if vmmc can't be
+ * powered off.
+ */
+ if (card && mmc_card_support_uhs(card) && mmc_host_uhs(host) &&
+ !mmc_regulator_can_power_off(host->supply.vmmc) &&
+ (ocr & SD_OCR_S18R)) {
+ *rocr |= SD_ROCR_S18A;
+ if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) {
+ retries = 0;
+ goto try_again;
+ }
+
+ goto done;
+ }
+ /*
* In case CCS and S18A in the response is set, start Signal Voltage
* Switch procedure. SPI mode doesn't support CMD11.
*/
@@ -788,6 +810,7 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
}
}
+done:
err = mmc_send_cid(host, cid);
return err;
}
@@ -924,7 +947,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
WARN_ON(!host->claimed);
- err = mmc_sd_get_cid(host, ocr, cid, &rocr);
+ err = mmc_sd_get_cid(host, oldcard, ocr, cid, &rocr);
if (err)
return err;
@@ -995,6 +1018,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
if (err)
goto free_card;
} else {
+
/*
* Attempt to change to high-speed (if supported)
*/
diff --git a/drivers/mmc/core/sd.h b/drivers/mmc/core/sd.h
index 1ada980..9cd2482 100644
--- a/drivers/mmc/core/sd.h
+++ b/drivers/mmc/core/sd.h
@@ -8,7 +8,8 @@
struct mmc_host;
struct mmc_card;
-int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr);
+int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
+ u32 ocr, u32 *cid, u32 *rocr);
int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card);
void mmc_decode_cid(struct mmc_card *card);
int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index cc43687..08e82fe 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -608,7 +608,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
}
if ((rocr & R4_MEMORY_PRESENT) &&
- mmc_sd_get_cid(host, ocr & rocr, card->raw_cid, NULL) == 0) {
+ mmc_sd_get_cid(host, NULL, ocr & rocr, card->raw_cid, NULL) == 0) {
card->type = MMC_TYPE_SD_COMBO;
if (oldcard && (oldcard->type != MMC_TYPE_SD_COMBO ||
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e567fa5..edea93b0 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2522,6 +2522,20 @@ int regulator_is_enabled(struct regulator *regulator)
EXPORT_SYMBOL_GPL(regulator_is_enabled);
/**
+ * regulator_is_always_on - is the regulator output always enabled
+ * @regulator: regulator source
+ *
+ * Returns positive if the regulator driver backing the source/client
+ * has requested that the device be always on, zero if it isn't.
+ *
+ */
+inline int regulator_is_always_on(struct regulator *regulator)
+{
+ return (regulator->always_on) ? 1 : 0;
+}
+EXPORT_SYMBOL_GPL(regulator_is_always_on);
+
+/**
* regulator_count_voltages - count regulator_list_voltage() selectors
* @regulator: regulator source
*
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index ebd1ceb..2f1206b 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -462,6 +462,7 @@ static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc,
u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
int mmc_regulator_get_supply(struct mmc_host *mmc);
+bool mmc_regulator_can_power_off(struct regulator *supply);
static inline int mmc_card_is_removable(struct mmc_host *host)
{
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index df176d7..46126e3 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -226,6 +226,7 @@ void devm_regulator_bulk_unregister_supply_alias(struct device *dev,
int regulator_disable(struct regulator *regulator);
int regulator_force_disable(struct regulator *regulator);
int regulator_is_enabled(struct regulator *regulator);
+int regulator_is_always_on(struct regulator *regulator);
int regulator_disable_deferred(struct regulator *regulator, int ms);
int __must_check regulator_bulk_get(struct device *dev, int num_consumers,
@@ -418,6 +419,11 @@ static inline int regulator_is_enabled(struct regulator *regulator)
return 1;
}
+static inline int regulator_is_always_on(struct regulator *regulator)
+{
+ return 0;
+}
+
static inline int regulator_bulk_get(struct device *dev,
int num_consumers,
struct regulator_bulk_data *consumers)
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [EXT] [RFT v2 PATCH] mmc: sd: try to enable UHS mode w/o power cycle from S3
[not found] ` <CAAXpJNvfSWHrJiLHoxmm+ENUDsGayQROnRTL=SoHr-7dX7R9+Q@mail.gmail.com>
@ 2017-08-29 22:13 ` Zhoujie Wu
2017-08-30 0:14 ` Shawn Lin
0 siblings, 1 reply; 10+ messages in thread
From: Zhoujie Wu @ 2017-08-29 22:13 UTC (permalink / raw)
To: Shawn Lin, Ulf Hansson; +Cc: linux-mmc
Hi Shawn,
thanks for your patch.
On 08/29/2017 02:56 PM, zhoujie wu wrote:
>
>
> CMD0 couldn't put the card back to 3.3v if can't do power
> cycle for cards. The card is already in SDR12 then, so still
> try to enable UHS mode.
>
> Pleas note that, if the platform use a hardware fixed
> sd power, you should still include a regulator-fixed
> regulator with always-on property there. So mmc core
> could handle this as well. Otherwise you shouldn't
> expect UHS to work for that case.
>
> Just compile only, please have a look and test.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> ---
>
> drivers/mmc/core/card.h | 17 +++++++++++++++++
> drivers/mmc/core/core.c | 8 ++++++++
> drivers/mmc/core/sd.c | 28 ++++++++++++++++++++++++++--
> drivers/mmc/core/sd.h | 3 ++-
> drivers/mmc/core/sdio.c | 2 +-
> drivers/regulator/core.c | 14 ++++++++++++++
> include/linux/mmc/host.h | 1 +
> include/linux/regulator/consumer.h | 6 ++++++
> 8 files changed, 75 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index f06cd91..a562110 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -218,4 +218,21 @@ static inline int mmc_card_broken_hpi(const struct
> mmc_card *c)
> return c->quirks & MMC_QUIRK_BROKEN_HPI;
> }
>
> +static inline bool mmc_card_support_uhs(struct mmc_card *card)
> +{
> + if (WARN_ON(!card))
> + return false;
> +
> + switch (card->sd_bus_speed) {
> + case UHS_SDR104_BUS_SPEED:
> + case UHS_DDR50_BUS_SPEED:
> + case UHS_SDR50_BUS_SPEED:
> + case UHS_SDR25_BUS_SPEED:
> + case UHS_SDR12_BUS_SPEED:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> #endif
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 2643126..6b5c6c5 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1424,6 +1424,14 @@ int mmc_regulator_get_supply(struct mmc_host *mmc)
> }
> EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
>
> +bool mmc_regulator_can_power_off(struct regulator *supply)
> +{
> + if (!IS_ERR(supply))
> + return regulator_is_always_on(supply);
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(mmc_regulator_can_power_off);
> +
> /*
> * Mask off any voltages we don't support and select
> * the lowest voltage
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index a1b0aa1..7bec61b 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -721,7 +721,8 @@ struct device_type sd_type = {
> /*
> * Fetch CID from card.
> */
> -int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
> +int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
> + u32 ocr, u32 *cid, u32 *rocr)
> {
> int err;
> u32 max_current;
> @@ -773,6 +774,27 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32
> *cid, u32 *rocr)
> return err;
>
> /*
> + * Per SD physical specification version 4.10, section
> + * 3.9.4 UHS-I Bus Speed Mode Selection Sequence, once
> + * the signal voltage is switched to 1V8, the card continues
> + * 1.8V signaling regardless of CMD0. Power cycle resets the
> + * signal voltage to 3.3V. As a fact that some vmmc is always
> + * on so the card would still work with SDR12 after CMD0. So
> + * let's attempt to enable enable UHS mode if vmmc can't be
> + * powered off.
> + */
> + if (card && mmc_card_support_uhs(card) && mmc_host_uhs(host) &&
> + !mmc_regulator_can_power_off(host->supply.vmmc) &&
> + (ocr & SD_OCR_S18R)) {
> + *rocr |= SD_ROCR_S18A;
> + if (mmc_set_signal_voltage(host,
> MMC_SIGNAL_VOLTAGE_180)) {
> + retries = 0;
> + goto try_again;
> + }
> +
> + goto done;
> + }
Good to have a higher level solution if the issue is common.
I did basic test with your patch(added your regulator modification you
sent to me) and add vmmc-supply in board dts, I saw warning when boot up.
I think in below case, the card is not added yet, the card is null and
print out the warning. I don't quite understand mmc_card_support_uhs, it
return true for all card speed.
[ 4.310003] WARNING: CPU: 1 PID: 48 at drivers/mmc/core/card.h:224
mmc_sd_get_cid+0x270/0x298
.....
[ 4.549657] [<ffffff8008757e24>] mmc_sd_get_cid+0x270/0x298
[ 4.555510] [<ffffff800875847c>] mmc_sd_init_card+0x44/0x614
[ 4.561456] [<ffffff8008758bd4>] mmc_attach_sd+0x7c/0x134
[ 4.566951] [<ffffff8008751fb8>] mmc_rescan+0x2e4/0x35c
[ 4.572451] [<ffffff80080d58d8>] process_one_work+0x12c/0x28c
[ 4.578304] [<ffffff80080d5a90>] worker_thread+0x58/0x3b8
[ 4.583801] [<ffffff80080db394>] kthread+0x100/0x12c
[ 4.589029] [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
[ 4.594793] ---[ end trace 58af756d1037eb51 ]---
I did some modifications and let code enter the if condition any way, I
added some log and you can see, rocr is updated, but when it tried to
switch to 1.8V signal, it failed. I don't know why yet.
So the code will try again, and use high speed to init card again.
[ 4.259194] before rocr = c0ff8000, ocr = 41200000
[ 4.359909] after rocr = c1ff8000 -- if condition meets, updated rocr
[ 4.364230] mmc0: 1.8V regulator output did not became stable --
mmc_set_signal_voltage 1.8V failed.
[ 4.372603] mmc0: Skipping voltage switch
[ 4.403225] before rocr = c0ff8000, ocr = 40200000 -- tried again
with no SD_OCR_S18R .
> + /*
> * In case CCS and S18A in the response is set, start Signal Voltage
> * Switch procedure. SPI mode doesn't support CMD11.
> */
> @@ -788,6 +810,7 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32
> *cid, u32 *rocr)
> }
> }
>
> +done:
> err = mmc_send_cid(host, cid);
> return err;
> }
> @@ -924,7 +947,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32
> ocr,
>
> WARN_ON(!host->claimed);
>
> - err = mmc_sd_get_cid(host, ocr, cid, &rocr);
> + err = mmc_sd_get_cid(host, oldcard, ocr, cid, &rocr);
> if (err)
> return err;
>
> @@ -995,6 +1018,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32
> ocr,
> if (err)
> goto free_card;
> } else {
> +
> /*
> * Attempt to change to high-speed (if supported)
> */
> diff --git a/drivers/mmc/core/sd.h b/drivers/mmc/core/sd.h
> index 1ada980..9cd2482 100644
> --- a/drivers/mmc/core/sd.h
> +++ b/drivers/mmc/core/sd.h
> @@ -8,7 +8,8 @@
> struct mmc_host;
> struct mmc_card;
>
> -int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr);
> +int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
> + u32 ocr, u32 *cid, u32 *rocr);
> int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card);
> void mmc_decode_cid(struct mmc_card *card);
> int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index cc43687..08e82fe 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -608,7 +608,7 @@ static int mmc_sdio_init_card(struct mmc_host *host,
> u32 ocr,
> }
>
> if ((rocr & R4_MEMORY_PRESENT) &&
> - mmc_sd_get_cid(host, ocr & rocr, card->raw_cid, NULL) == 0) {
> + mmc_sd_get_cid(host, NULL, ocr & rocr, card->raw_cid, NULL) ==
> 0) {
> card->type = MMC_TYPE_SD_COMBO;
>
> if (oldcard && (oldcard->type != MMC_TYPE_SD_COMBO ||
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index e567fa5..edea93b0 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2522,6 +2522,20 @@ int regulator_is_enabled(struct regulator *regulator)
> EXPORT_SYMBOL_GPL(regulator_is_enabled);
>
> /**
> + * regulator_is_always_on - is the regulator output always enabled
> + * @regulator: regulator source
> + *
> + * Returns positive if the regulator driver backing the source/client
> + * has requested that the device be always on, zero if it isn't.
> + *
> + */
> +inline int regulator_is_always_on(struct regulator *regulator)
> +{
> + return (regulator->always_on) ? 1 : 0;
> +}
> +EXPORT_SYMBOL_GPL(regulator_is_always_on);
> +
> +/**
> * regulator_count_voltages - count regulator_list_voltage() selectors
> * @regulator: regulator source
> *
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index ebd1ceb..2f1206b 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -462,6 +462,7 @@ static inline int mmc_regulator_set_vqmmc(struct
> mmc_host *mmc,
>
> u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
> int mmc_regulator_get_supply(struct mmc_host *mmc);
> +bool mmc_regulator_can_power_off(struct regulator *supply);
>
> static inline int mmc_card_is_removable(struct mmc_host *host)
> {
> diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/
> consumer.h
> index df176d7..46126e3 100644
> --- a/include/linux/regulator/consumer.h
> +++ b/include/linux/regulator/consumer.h
> @@ -226,6 +226,7 @@ void devm_regulator_bulk_unregister_supply_alias(struct
> device *dev,
> int regulator_disable(struct regulator *regulator);
> int regulator_force_disable(struct regulator *regulator);
> int regulator_is_enabled(struct regulator *regulator);
> +int regulator_is_always_on(struct regulator *regulator);
> int regulator_disable_deferred(struct regulator *regulator, int ms);
>
> int __must_check regulator_bulk_get(struct device *dev, int num_consumers,
> @@ -418,6 +419,11 @@ static inline int regulator_is_enabled(struct
> regulator *regulator)
> return 1;
> }
>
> +static inline int regulator_is_always_on(struct regulator *regulator)
> +{
> + return 0;
> +}
> +
> static inline int regulator_bulk_get(struct device *dev,
> int num_consumers,
> struct regulator_bulk_data *consumers)
> --
> 1.9.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [EXT] [RFT v2 PATCH] mmc: sd: try to enable UHS mode w/o power cycle from S3
2017-08-29 22:13 ` [EXT] " Zhoujie Wu
@ 2017-08-30 0:14 ` Shawn Lin
2017-08-30 22:32 ` Zhoujie Wu
2017-09-01 22:57 ` Zhoujie Wu
0 siblings, 2 replies; 10+ messages in thread
From: Shawn Lin @ 2017-08-30 0:14 UTC (permalink / raw)
To: Zhoujie Wu; +Cc: Ulf Hansson, shawn.lin, linux-mmc
On 2017/8/30 6:13, Zhoujie Wu wrote:
> Hi Shawn,
> thanks for your patch.
>
> On 08/29/2017 02:56 PM, zhoujie wu wrote:
>>
>>
>> CMD0 couldn't put the card back to 3.3v if can't do power
>> cycle for cards. The card is already in SDR12 then, so still
>> try to enable UHS mode.
>>
>> Pleas note that, if the platform use a hardware fixed
>> sd power, you should still include a regulator-fixed
>> regulator with always-on property there. So mmc core
>> could handle this as well. Otherwise you shouldn't
>> expect UHS to work for that case.
>>
>> Just compile only, please have a look and test.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> drivers/mmc/core/card.h | 17 +++++++++++++++++
>> drivers/mmc/core/core.c | 8 ++++++++
>> drivers/mmc/core/sd.c | 28 ++++++++++++++++++++++++++--
>> drivers/mmc/core/sd.h | 3 ++-
>> drivers/mmc/core/sdio.c | 2 +-
>> drivers/regulator/core.c | 14 ++++++++++++++
>> include/linux/mmc/host.h | 1 +
>> include/linux/regulator/consumer.h | 6 ++++++
>> 8 files changed, 75 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
>> index f06cd91..a562110 100644
>> --- a/drivers/mmc/core/card.h
>> +++ b/drivers/mmc/core/card.h
>> @@ -218,4 +218,21 @@ static inline int mmc_card_broken_hpi(const struct
>> mmc_card *c)
>> return c->quirks & MMC_QUIRK_BROKEN_HPI;
>> }
>>
>> +static inline bool mmc_card_support_uhs(struct mmc_card *card)
>> +{
>> + if (WARN_ON(!card))
>> + return false;
>> +
>> + switch (card->sd_bus_speed) {
>> + case UHS_SDR104_BUS_SPEED:
>> + case UHS_DDR50_BUS_SPEED:
>> + case UHS_SDR50_BUS_SPEED:
>> + case UHS_SDR25_BUS_SPEED:
>> + case UHS_SDR12_BUS_SPEED:
>> + return true;
>> + default:
>> + return false;
>> + }
>> +}
>> +
>> #endif
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 2643126..6b5c6c5 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1424,6 +1424,14 @@ int mmc_regulator_get_supply(struct mmc_host *mmc)
>> }
>> EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
>>
>> +bool mmc_regulator_can_power_off(struct regulator *supply)
>> +{
>> + if (!IS_ERR(supply))
>> + return regulator_is_always_on(supply);
>> + return false;
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_regulator_can_power_off);
>> +
>> /*
>> * Mask off any voltages we don't support and select
>> * the lowest voltage
>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>> index a1b0aa1..7bec61b 100644
>> --- a/drivers/mmc/core/sd.c
>> +++ b/drivers/mmc/core/sd.c
>> @@ -721,7 +721,8 @@ struct device_type sd_type = {
>> /*
>> * Fetch CID from card.
>> */
>> -int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr)
>> +int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
>> + u32 ocr, u32 *cid, u32 *rocr)
>> {
>> int err;
>> u32 max_current;
>> @@ -773,6 +774,27 @@ int mmc_sd_get_cid(struct mmc_host *host, u32
>> ocr, u32
>> *cid, u32 *rocr)
>> return err;
>>
>> /*
>> + * Per SD physical specification version 4.10, section
>> + * 3.9.4 UHS-I Bus Speed Mode Selection Sequence, once
>> + * the signal voltage is switched to 1V8, the card continues
>> + * 1.8V signaling regardless of CMD0. Power cycle resets the
>> + * signal voltage to 3.3V. As a fact that some vmmc is always
>> + * on so the card would still work with SDR12 after CMD0. So
>> + * let's attempt to enable enable UHS mode if vmmc can't be
>> + * powered off.
>> + */
>> + if (card && mmc_card_support_uhs(card) && mmc_host_uhs(host) &&
>> + !mmc_regulator_can_power_off(host->supply.vmmc) &&
>> + (ocr & SD_OCR_S18R)) {
>> + *rocr |= SD_ROCR_S18A;
>> + if (mmc_set_signal_voltage(host,
>> MMC_SIGNAL_VOLTAGE_180)) {
>> + retries = 0;
>> + goto try_again;
>> + }
>> +
>> + goto done;
>> + }
> Good to have a higher level solution if the issue is common.
> I did basic test with your patch(added your regulator modification you
> sent to me) and add vmmc-supply in board dts, I saw warning when boot up.
> I think in below case, the card is not added yet, the card is null and
> print out the warning. I don't quite understand mmc_card_support_uhs, it
> return true for all card speed.
>
> [ 4.310003] WARNING: CPU: 1 PID: 48 at drivers/mmc/core/card.h:224
> mmc_sd_get_cid+0x270/0x298
it's just for debug, and won't be a big deal.
And I find mmc_host_uhs here is redundant as oldcard's
card->sd_bus_speed is already the result of checking uhs cap for both of
controller and card. So mmc_card_support_uhs return true means both
of your controller and the cards support UHS mode, didn't they?
>
> .....
>
> [ 4.549657] [<ffffff8008757e24>] mmc_sd_get_cid+0x270/0x298
>
> [ 4.555510] [<ffffff800875847c>] mmc_sd_init_card+0x44/0x614
>
> [ 4.561456] [<ffffff8008758bd4>] mmc_attach_sd+0x7c/0x134
>
> [ 4.566951] [<ffffff8008751fb8>] mmc_rescan+0x2e4/0x35c
>
> [ 4.572451] [<ffffff80080d58d8>] process_one_work+0x12c/0x28c
>
> [ 4.578304] [<ffffff80080d5a90>] worker_thread+0x58/0x3b8
>
> [ 4.583801] [<ffffff80080db394>] kthread+0x100/0x12c
>
> [ 4.589029] [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
>
> [ 4.594793] ---[ end trace 58af756d1037eb51 ]---
>
> I did some modifications and let code enter the if condition any way, I
> added some log and you can see, rocr is updated, but when it tried to
> switch to 1.8V signal, it failed. I don't know why yet.
Ok, sounds the key point is to see why it fails to switch to 1.8v signal
> So the code will try again, and use high speed to init card again.
Thanks for testing this, but I haven' had enough time to put my hand
on it. But I will be back and check it out by myself.. Anyway you may
still be able to do more debugging if this approach looks good to you:)
>
> [ 4.259194] before rocr = c0ff8000, ocr = 41200000
>
> [ 4.359909] after rocr = c1ff8000 -- if condition meets, updated rocr
>
> [ 4.364230] mmc0: 1.8V regulator output did not became stable --
> mmc_set_signal_voltage 1.8V failed.
> [ 4.372603] mmc0: Skipping voltage switch
>
> [ 4.403225] before rocr = c0ff8000, ocr = 40200000 -- tried again
> with no SD_OCR_S18R .
>> + /*
>> * In case CCS and S18A in the response is set, start Signal
>> Voltage
>> * Switch procedure. SPI mode doesn't support CMD11.
>> */
>> @@ -788,6 +810,7 @@ int mmc_sd_get_cid(struct mmc_host *host, u32 ocr,
>> u32
>> *cid, u32 *rocr)
>> }
>> }
>>
>> +done:
>> err = mmc_send_cid(host, cid);
>> return err;
>> }
>> @@ -924,7 +947,7 @@ static int mmc_sd_init_card(struct mmc_host *host,
>> u32
>> ocr,
>>
>> WARN_ON(!host->claimed);
>>
>> - err = mmc_sd_get_cid(host, ocr, cid, &rocr);
>> + err = mmc_sd_get_cid(host, oldcard, ocr, cid, &rocr);
>> if (err)
>> return err;
>>
>> @@ -995,6 +1018,7 @@ static int mmc_sd_init_card(struct mmc_host
>> *host, u32
>> ocr,
>> if (err)
>> goto free_card;
>> } else {
>> +
>> /*
>> * Attempt to change to high-speed (if supported)
>> */
>> diff --git a/drivers/mmc/core/sd.h b/drivers/mmc/core/sd.h
>> index 1ada980..9cd2482 100644
>> --- a/drivers/mmc/core/sd.h
>> +++ b/drivers/mmc/core/sd.h
>> @@ -8,7 +8,8 @@
>> struct mmc_host;
>> struct mmc_card;
>>
>> -int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr);
>> +int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
>> + u32 ocr, u32 *cid, u32 *rocr);
>> int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card);
>> void mmc_decode_cid(struct mmc_card *card);
>> int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index cc43687..08e82fe 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -608,7 +608,7 @@ static int mmc_sdio_init_card(struct mmc_host *host,
>> u32 ocr,
>> }
>>
>> if ((rocr & R4_MEMORY_PRESENT) &&
>> - mmc_sd_get_cid(host, ocr & rocr, card->raw_cid, NULL) == 0) {
>> + mmc_sd_get_cid(host, NULL, ocr & rocr, card->raw_cid,
>> NULL) ==
>> 0) {
>> card->type = MMC_TYPE_SD_COMBO;
>>
>> if (oldcard && (oldcard->type != MMC_TYPE_SD_COMBO ||
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index e567fa5..edea93b0 100644
>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -2522,6 +2522,20 @@ int regulator_is_enabled(struct regulator
>> *regulator)
>> EXPORT_SYMBOL_GPL(regulator_is_enabled);
>>
>> /**
>> + * regulator_is_always_on - is the regulator output always enabled
>> + * @regulator: regulator source
>> + *
>> + * Returns positive if the regulator driver backing the source/client
>> + * has requested that the device be always on, zero if it isn't.
>> + *
>> + */
>> +inline int regulator_is_always_on(struct regulator *regulator)
>> +{
>> + return (regulator->always_on) ? 1 : 0;
>> +}
>> +EXPORT_SYMBOL_GPL(regulator_is_always_on);
>> +
>> +/**
>> * regulator_count_voltages - count regulator_list_voltage() selectors
>> * @regulator: regulator source
>> *
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index ebd1ceb..2f1206b 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -462,6 +462,7 @@ static inline int mmc_regulator_set_vqmmc(struct
>> mmc_host *mmc,
>>
>> u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
>> int mmc_regulator_get_supply(struct mmc_host *mmc);
>> +bool mmc_regulator_can_power_off(struct regulator *supply);
>>
>> static inline int mmc_card_is_removable(struct mmc_host *host)
>> {
>> diff --git a/include/linux/regulator/consumer.h
>> b/include/linux/regulator/
>> consumer.h
>> index df176d7..46126e3 100644
>> --- a/include/linux/regulator/consumer.h
>> +++ b/include/linux/regulator/consumer.h
>> @@ -226,6 +226,7 @@ void
>> devm_regulator_bulk_unregister_supply_alias(struct
>> device *dev,
>> int regulator_disable(struct regulator *regulator);
>> int regulator_force_disable(struct regulator *regulator);
>> int regulator_is_enabled(struct regulator *regulator);
>> +int regulator_is_always_on(struct regulator *regulator);
>> int regulator_disable_deferred(struct regulator *regulator, int ms);
>>
>> int __must_check regulator_bulk_get(struct device *dev, int
>> num_consumers,
>> @@ -418,6 +419,11 @@ static inline int regulator_is_enabled(struct
>> regulator *regulator)
>> return 1;
>> }
>>
>> +static inline int regulator_is_always_on(struct regulator *regulator)
>> +{
>> + return 0;
>> +}
>> +
>> static inline int regulator_bulk_get(struct device *dev,
>> int num_consumers,
>> struct regulator_bulk_data
>> *consumers)
>> --
>> 1.9.1
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [EXT] [RFT v2 PATCH] mmc: sd: try to enable UHS mode w/o power cycle from S3
2017-08-30 0:14 ` Shawn Lin
@ 2017-08-30 22:32 ` Zhoujie Wu
2017-09-01 22:57 ` Zhoujie Wu
1 sibling, 0 replies; 10+ messages in thread
From: Zhoujie Wu @ 2017-08-30 22:32 UTC (permalink / raw)
To: Shawn Lin; +Cc: Ulf Hansson, linux-mmc
On 08/29/2017 05:14 PM, Shawn Lin wrote:
> On 2017/8/30 6:13, Zhoujie Wu wrote:
>> Hi Shawn,
>> thanks for your patch.
>>
>> On 08/29/2017 02:56 PM, zhoujie wu wrote:
>>>
>>>
>>> CMD0 couldn't put the card back to 3.3v if can't do power
>>> cycle for cards. The card is already in SDR12 then, so still
>>> try to enable UHS mode.
>>>
>>> Pleas note that, if the platform use a hardware fixed
>>> sd power, you should still include a regulator-fixed
>>> regulator with always-on property there. So mmc core
>>> could handle this as well. Otherwise you shouldn't
>>> expect UHS to work for that case.
>>>
>>> Just compile only, please have a look and test.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>
>>> ---
>>>
>>> drivers/mmc/core/card.h | 17 +++++++++++++++++
>>> drivers/mmc/core/core.c | 8 ++++++++
>>> drivers/mmc/core/sd.c | 28 ++++++++++++++++++++++++++--
>>> drivers/mmc/core/sd.h | 3 ++-
>>> drivers/mmc/core/sdio.c | 2 +-
>>> drivers/regulator/core.c | 14 ++++++++++++++
>>> include/linux/mmc/host.h | 1 +
>>> include/linux/regulator/consumer.h | 6 ++++++
>>> 8 files changed, 75 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
>>> index f06cd91..a562110 100644
>>> --- a/drivers/mmc/core/card.h
>>> +++ b/drivers/mmc/core/card.h
>>> @@ -218,4 +218,21 @@ static inline int mmc_card_broken_hpi(const struct
>>> mmc_card *c)
>>> return c->quirks & MMC_QUIRK_BROKEN_HPI;
>>> }
>>>
>>> +static inline bool mmc_card_support_uhs(struct mmc_card *card)
>>> +{
>>> + if (WARN_ON(!card))
>>> + return false;
>>> +
>>> + switch (card->sd_bus_speed) {
>>> + case UHS_SDR104_BUS_SPEED:
>>> + case UHS_DDR50_BUS_SPEED:
>>> + case UHS_SDR50_BUS_SPEED:
>>> + case UHS_SDR25_BUS_SPEED:
>>> + case UHS_SDR12_BUS_SPEED:
>>> + return true;
>>> + default:
>>> + return false;
>>> + }
>>> +}
>>> +
>>> #endif
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 2643126..6b5c6c5 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1424,6 +1424,14 @@ int mmc_regulator_get_supply(struct mmc_host
>>> *mmc)
>>> }
>>> EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
>>>
>>> +bool mmc_regulator_can_power_off(struct regulator *supply)
>>> +{
>>> + if (!IS_ERR(supply))
>>> + return regulator_is_always_on(supply);
>>> + return false;
>>> +}
>>> +EXPORT_SYMBOL_GPL(mmc_regulator_can_power_off);
>>> +
>>> /*
>>> * Mask off any voltages we don't support and select
>>> * the lowest voltage
>>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>>> index a1b0aa1..7bec61b 100644
>>> --- a/drivers/mmc/core/sd.c
>>> +++ b/drivers/mmc/core/sd.c
>>> @@ -721,7 +721,8 @@ struct device_type sd_type = {
>>> /*
>>> * Fetch CID from card.
>>> */
>>> -int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32
>>> *rocr)
>>> +int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
>>> + u32 ocr, u32 *cid, u32 *rocr)
>>> {
>>> int err;
>>> u32 max_current;
>>> @@ -773,6 +774,27 @@ int mmc_sd_get_cid(struct mmc_host *host, u32
>>> ocr, u32
>>> *cid, u32 *rocr)
>>> return err;
>>>
>>> /*
>>> + * Per SD physical specification version 4.10, section
>>> + * 3.9.4 UHS-I Bus Speed Mode Selection Sequence, once
>>> + * the signal voltage is switched to 1V8, the card continues
>>> + * 1.8V signaling regardless of CMD0. Power cycle resets the
>>> + * signal voltage to 3.3V. As a fact that some vmmc is always
>>> + * on so the card would still work with SDR12 after CMD0. So
>>> + * let's attempt to enable enable UHS mode if vmmc can't be
>>> + * powered off.
>>> + */
>>> + if (card && mmc_card_support_uhs(card) && mmc_host_uhs(host) &&
>>> + !mmc_regulator_can_power_off(host->supply.vmmc) &&
>>> + (ocr & SD_OCR_S18R)) {
>>> + *rocr |= SD_ROCR_S18A;
>>> + if (mmc_set_signal_voltage(host,
>>> MMC_SIGNAL_VOLTAGE_180)) {
>>> + retries = 0;
>>> + goto try_again;
>>> + }
>>> +
>>> + goto done;
>>> + }
>> Good to have a higher level solution if the issue is common.
>> I did basic test with your patch(added your regulator modification
>> you sent to me) and add vmmc-supply in board dts, I saw warning when
>> boot up.
>> I think in below case, the card is not added yet, the card is null
>> and print out the warning. I don't quite understand
>> mmc_card_support_uhs, it return true for all card speed.
>>
>> [ 4.310003] WARNING: CPU: 1 PID: 48 at drivers/mmc/core/card.h:224
>> mmc_sd_get_cid+0x270/0x298
>
> it's just for debug, and won't be a big deal.
>
> And I find mmc_host_uhs here is redundant as oldcard's
> card->sd_bus_speed is already the result of checking uhs cap for both of
> controller and card. So mmc_card_support_uhs return true means both
> of your controller and the cards support UHS mode, didn't they?
>
>>
>> .....
>>
>> [ 4.549657] [<ffffff8008757e24>] mmc_sd_get_cid+0x270/0x298
>>
>> [ 4.555510] [<ffffff800875847c>] mmc_sd_init_card+0x44/0x614
>>
>> [ 4.561456] [<ffffff8008758bd4>] mmc_attach_sd+0x7c/0x134
>>
>> [ 4.566951] [<ffffff8008751fb8>] mmc_rescan+0x2e4/0x35c
>>
>> [ 4.572451] [<ffffff80080d58d8>] process_one_work+0x12c/0x28c
>>
>> [ 4.578304] [<ffffff80080d5a90>] worker_thread+0x58/0x3b8
>>
>> [ 4.583801] [<ffffff80080db394>] kthread+0x100/0x12c
>>
>> [ 4.589029] [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
>>
>> [ 4.594793] ---[ end trace 58af756d1037eb51 ]---
>>
>> I did some modifications and let code enter the if condition any way,
>> I added some log and you can see, rocr is updated, but when it tried
>> to switch to 1.8V signal, it failed. I don't know why yet.
>
> Ok, sounds the key point is to see why it fails to switch to 1.8v signal
>
>> So the code will try again, and use high speed to init card again.
>
> Thanks for testing this, but I haven' had enough time to put my hand
> on it. But I will be back and check it out by myself.. Anyway you may
> still be able to do more debugging if this approach looks good to you:)
The solution is ok for me, I will check more details later. thanks.
>
>>
>> [ 4.259194] before rocr = c0ff8000, ocr = 41200000
>>
>> [ 4.359909] after rocr = c1ff8000 -- if condition meets, updated
>> rocr
>>
>> [ 4.364230] mmc0: 1.8V regulator output did not became stable --
>> mmc_set_signal_voltage 1.8V failed.
>> [ 4.372603] mmc0: Skipping voltage switch
>>
>> [ 4.403225] before rocr = c0ff8000, ocr = 40200000 -- tried
>> again with no SD_OCR_S18R .
>>> + /*
>>> * In case CCS and S18A in the response is set, start
>>> Signal Voltage
>>> * Switch procedure. SPI mode doesn't support CMD11.
>>> */
>>> @@ -788,6 +810,7 @@ int mmc_sd_get_cid(struct mmc_host *host, u32
>>> ocr, u32
>>> *cid, u32 *rocr)
>>> }
>>> }
>>>
>>> +done:
>>> err = mmc_send_cid(host, cid);
>>> return err;
>>> }
>>> @@ -924,7 +947,7 @@ static int mmc_sd_init_card(struct mmc_host
>>> *host, u32
>>> ocr,
>>>
>>> WARN_ON(!host->claimed);
>>>
>>> - err = mmc_sd_get_cid(host, ocr, cid, &rocr);
>>> + err = mmc_sd_get_cid(host, oldcard, ocr, cid, &rocr);
>>> if (err)
>>> return err;
>>>
>>> @@ -995,6 +1018,7 @@ static int mmc_sd_init_card(struct mmc_host
>>> *host, u32
>>> ocr,
>>> if (err)
>>> goto free_card;
>>> } else {
>>> +
>>> /*
>>> * Attempt to change to high-speed (if supported)
>>> */
>>> diff --git a/drivers/mmc/core/sd.h b/drivers/mmc/core/sd.h
>>> index 1ada980..9cd2482 100644
>>> --- a/drivers/mmc/core/sd.h
>>> +++ b/drivers/mmc/core/sd.h
>>> @@ -8,7 +8,8 @@
>>> struct mmc_host;
>>> struct mmc_card;
>>>
>>> -int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32
>>> *rocr);
>>> +int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
>>> + u32 ocr, u32 *cid, u32 *rocr);
>>> int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card);
>>> void mmc_decode_cid(struct mmc_card *card);
>>> int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>> index cc43687..08e82fe 100644
>>> --- a/drivers/mmc/core/sdio.c
>>> +++ b/drivers/mmc/core/sdio.c
>>> @@ -608,7 +608,7 @@ static int mmc_sdio_init_card(struct mmc_host
>>> *host,
>>> u32 ocr,
>>> }
>>>
>>> if ((rocr & R4_MEMORY_PRESENT) &&
>>> - mmc_sd_get_cid(host, ocr & rocr, card->raw_cid, NULL) ==
>>> 0) {
>>> + mmc_sd_get_cid(host, NULL, ocr & rocr, card->raw_cid,
>>> NULL) ==
>>> 0) {
>>> card->type = MMC_TYPE_SD_COMBO;
>>>
>>> if (oldcard && (oldcard->type != MMC_TYPE_SD_COMBO ||
>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>> index e567fa5..edea93b0 100644
>>> --- a/drivers/regulator/core.c
>>> +++ b/drivers/regulator/core.c
>>> @@ -2522,6 +2522,20 @@ int regulator_is_enabled(struct regulator
>>> *regulator)
>>> EXPORT_SYMBOL_GPL(regulator_is_enabled);
>>>
>>> /**
>>> + * regulator_is_always_on - is the regulator output always enabled
>>> + * @regulator: regulator source
>>> + *
>>> + * Returns positive if the regulator driver backing the source/client
>>> + * has requested that the device be always on, zero if it isn't.
>>> + *
>>> + */
>>> +inline int regulator_is_always_on(struct regulator *regulator)
>>> +{
>>> + return (regulator->always_on) ? 1 : 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(regulator_is_always_on);
>>> +
>>> +/**
>>> * regulator_count_voltages - count regulator_list_voltage()
>>> selectors
>>> * @regulator: regulator source
>>> *
>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>> index ebd1ceb..2f1206b 100644
>>> --- a/include/linux/mmc/host.h
>>> +++ b/include/linux/mmc/host.h
>>> @@ -462,6 +462,7 @@ static inline int mmc_regulator_set_vqmmc(struct
>>> mmc_host *mmc,
>>>
>>> u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
>>> int mmc_regulator_get_supply(struct mmc_host *mmc);
>>> +bool mmc_regulator_can_power_off(struct regulator *supply);
>>>
>>> static inline int mmc_card_is_removable(struct mmc_host *host)
>>> {
>>> diff --git a/include/linux/regulator/consumer.h
>>> b/include/linux/regulator/
>>> consumer.h
>>> index df176d7..46126e3 100644
>>> --- a/include/linux/regulator/consumer.h
>>> +++ b/include/linux/regulator/consumer.h
>>> @@ -226,6 +226,7 @@ void
>>> devm_regulator_bulk_unregister_supply_alias(struct
>>> device *dev,
>>> int regulator_disable(struct regulator *regulator);
>>> int regulator_force_disable(struct regulator *regulator);
>>> int regulator_is_enabled(struct regulator *regulator);
>>> +int regulator_is_always_on(struct regulator *regulator);
>>> int regulator_disable_deferred(struct regulator *regulator, int ms);
>>>
>>> int __must_check regulator_bulk_get(struct device *dev, int
>>> num_consumers,
>>> @@ -418,6 +419,11 @@ static inline int regulator_is_enabled(struct
>>> regulator *regulator)
>>> return 1;
>>> }
>>>
>>> +static inline int regulator_is_always_on(struct regulator *regulator)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> static inline int regulator_bulk_get(struct device *dev,
>>> int num_consumers,
>>> struct regulator_bulk_data
>>> *consumers)
>>> --
>>> 1.9.1
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [EXT] [RFT v2 PATCH] mmc: sd: try to enable UHS mode w/o power cycle from S3
2017-08-30 0:14 ` Shawn Lin
2017-08-30 22:32 ` Zhoujie Wu
@ 2017-09-01 22:57 ` Zhoujie Wu
2017-09-04 1:06 ` Shawn Lin
1 sibling, 1 reply; 10+ messages in thread
From: Zhoujie Wu @ 2017-09-01 22:57 UTC (permalink / raw)
To: Shawn Lin; +Cc: Ulf Hansson, linux-mmc
Hi Shawn,
On 08/29/2017 05:14 PM, Shawn Lin wrote:
> On 2017/8/30 6:13, Zhoujie Wu wrote:
>> Hi Shawn,
>> thanks for your patch.
>>
>> On 08/29/2017 02:56 PM, zhoujie wu wrote:
>>>
>>>
>>> CMD0 couldn't put the card back to 3.3v if can't do power
>>> cycle for cards. The card is already in SDR12 then, so still
>>> try to enable UHS mode.
>>>
>>> Pleas note that, if the platform use a hardware fixed
>>> sd power, you should still include a regulator-fixed
>>> regulator with always-on property there. So mmc core
>>> could handle this as well. Otherwise you shouldn't
>>> expect UHS to work for that case.
>>>
>>> Just compile only, please have a look and test.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>
>>> ---
>>>
>>> drivers/mmc/core/card.h | 17 +++++++++++++++++
>>> drivers/mmc/core/core.c | 8 ++++++++
>>> drivers/mmc/core/sd.c | 28 ++++++++++++++++++++++++++--
>>> drivers/mmc/core/sd.h | 3 ++-
>>> drivers/mmc/core/sdio.c | 2 +-
>>> drivers/regulator/core.c | 14 ++++++++++++++
>>> include/linux/mmc/host.h | 1 +
>>> include/linux/regulator/consumer.h | 6 ++++++
>>> 8 files changed, 75 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
>>> index f06cd91..a562110 100644
>>> --- a/drivers/mmc/core/card.h
>>> +++ b/drivers/mmc/core/card.h
>>> @@ -218,4 +218,21 @@ static inline int mmc_card_broken_hpi(const struct
>>> mmc_card *c)
>>> return c->quirks & MMC_QUIRK_BROKEN_HPI;
>>> }
>>>
>>> +static inline bool mmc_card_support_uhs(struct mmc_card *card)
>>> +{
>>> + if (WARN_ON(!card))
>>> + return false;
>>> +
>>> + switch (card->sd_bus_speed) {
>>> + case UHS_SDR104_BUS_SPEED:
>>> + case UHS_DDR50_BUS_SPEED:
>>> + case UHS_SDR50_BUS_SPEED:
>>> + case UHS_SDR25_BUS_SPEED:
>>> + case UHS_SDR12_BUS_SPEED:
>>> + return true;
>>> + default:
>>> + return false;
>>> + }
>>> +}
>>> +
>>> #endif
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 2643126..6b5c6c5 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1424,6 +1424,14 @@ int mmc_regulator_get_supply(struct mmc_host
>>> *mmc)
>>> }
>>> EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
>>>
>>> +bool mmc_regulator_can_power_off(struct regulator *supply)
>>> +{
>>> + if (!IS_ERR(supply))
>>> + return regulator_is_always_on(supply);
>>> + return false;
>>> +}
>>> +EXPORT_SYMBOL_GPL(mmc_regulator_can_power_off);
>>> +
>>> /*
>>> * Mask off any voltages we don't support and select
>>> * the lowest voltage
>>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>>> index a1b0aa1..7bec61b 100644
>>> --- a/drivers/mmc/core/sd.c
>>> +++ b/drivers/mmc/core/sd.c
>>> @@ -721,7 +721,8 @@ struct device_type sd_type = {
>>> /*
>>> * Fetch CID from card.
>>> */
>>> -int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32
>>> *rocr)
>>> +int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
>>> + u32 ocr, u32 *cid, u32 *rocr)
>>> {
>>> int err;
>>> u32 max_current;
>>> @@ -773,6 +774,27 @@ int mmc_sd_get_cid(struct mmc_host *host, u32
>>> ocr, u32
>>> *cid, u32 *rocr)
>>> return err;
>>>
>>> /*
>>> + * Per SD physical specification version 4.10, section
>>> + * 3.9.4 UHS-I Bus Speed Mode Selection Sequence, once
>>> + * the signal voltage is switched to 1V8, the card continues
>>> + * 1.8V signaling regardless of CMD0. Power cycle resets the
>>> + * signal voltage to 3.3V. As a fact that some vmmc is always
>>> + * on so the card would still work with SDR12 after CMD0. So
>>> + * let's attempt to enable enable UHS mode if vmmc can't be
>>> + * powered off.
>>> + */
>>> + if (card && mmc_card_support_uhs(card) && mmc_host_uhs(host) &&
>>> + !mmc_regulator_can_power_off(host->supply.vmmc) &&
>>> + (ocr & SD_OCR_S18R)) {
>>> + *rocr |= SD_ROCR_S18A;
>>> + if (mmc_set_signal_voltage(host,
>>> MMC_SIGNAL_VOLTAGE_180)) {
>>> + retries = 0;
>>> + goto try_again;
>>> + }
>>> +
>>> + goto done;
>>> + }
>> Good to have a higher level solution if the issue is common.
>> I did basic test with your patch(added your regulator modification
>> you sent to me) and add vmmc-supply in board dts, I saw warning when
>> boot up.
>> I think in below case, the card is not added yet, the card is null
>> and print out the warning. I don't quite understand
>> mmc_card_support_uhs, it return true for all card speed.
>>
>> [ 4.310003] WARNING: CPU: 1 PID: 48 at drivers/mmc/core/card.h:224
>> mmc_sd_get_cid+0x270/0x298
>
> it's just for debug, and won't be a big deal.
>
> And I find mmc_host_uhs here is redundant as oldcard's
> card->sd_bus_speed is already the result of checking uhs cap for both of
> controller and card. So mmc_card_support_uhs return true means both
> of your controller and the cards support UHS mode, didn't they?
>
>>
>> .....
>>
>> [ 4.549657] [<ffffff8008757e24>] mmc_sd_get_cid+0x270/0x298
>>
>> [ 4.555510] [<ffffff800875847c>] mmc_sd_init_card+0x44/0x614
>>
>> [ 4.561456] [<ffffff8008758bd4>] mmc_attach_sd+0x7c/0x134
>>
>> [ 4.566951] [<ffffff8008751fb8>] mmc_rescan+0x2e4/0x35c
>>
>> [ 4.572451] [<ffffff80080d58d8>] process_one_work+0x12c/0x28c
>>
>> [ 4.578304] [<ffffff80080d5a90>] worker_thread+0x58/0x3b8
>>
>> [ 4.583801] [<ffffff80080db394>] kthread+0x100/0x12c
>>
>> [ 4.589029] [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
>>
>> [ 4.594793] ---[ end trace 58af756d1037eb51 ]---
>>
>> I did some modifications and let code enter the if condition any way,
>> I added some log and you can see, rocr is updated, but when it tried
>> to switch to 1.8V signal, it failed. I don't know why yet.
>
> Ok, sounds the key point is to see why it fails to switch to 1.8v signal
>
>> So the code will try again, and use high speed to init card again.
>
> Thanks for testing this, but I haven' had enough time to put my hand
> on it. But I will be back and check it out by myself.. Anyway you may
> still be able to do more debugging if this approach looks good to you:)
I have checked why 1.8V signal switch failed here, as SD spec mentioned
it needs to gate the clock for a while when switching the voltage, I
added some logic to gate the clock before switch signal,
then re-enable the clock after that, just like what set_uhs_voltage does
and it could work now.
and I adjust the if condition logic as below, hacked
mmc_card_support_uhs now always return true.
This can be a fix, but to be a generic solution, I am wondering from sd
card perspective, except for no power cycle, if there is any other
reason that may cause SD_OCR_S18A not set for uhs card?
+ if (mmc_card_support_uhs(card)
+ !mmc_regulator_can_power_off(host->supply.vmmc) &&
+ !(*rocr & SD_OCR_S18A))
>
>>
>> [ 4.259194] before rocr = c0ff8000, ocr = 41200000
>>
>> [ 4.359909] after rocr = c1ff8000 -- if condition meets, updated
>> rocr
>>
>> [ 4.364230] mmc0: 1.8V regulator output did not became stable --
>> mmc_set_signal_voltage 1.8V failed.
>> [ 4.372603] mmc0: Skipping voltage switch
>>
>> [ 4.403225] before rocr = c0ff8000, ocr = 40200000 -- tried
>> again with no SD_OCR_S18R .
>>> + /*
>>> * In case CCS and S18A in the response is set, start
>>> Signal Voltage
>>> * Switch procedure. SPI mode doesn't support CMD11.
>>> */
>>> @@ -788,6 +810,7 @@ int mmc_sd_get_cid(struct mmc_host *host, u32
>>> ocr, u32
>>> *cid, u32 *rocr)
>>> }
>>> }
>>>
>>> +done:
>>> err = mmc_send_cid(host, cid);
>>> return err;
>>> }
>>> @@ -924,7 +947,7 @@ static int mmc_sd_init_card(struct mmc_host
>>> *host, u32
>>> ocr,
>>>
>>> WARN_ON(!host->claimed);
>>>
>>> - err = mmc_sd_get_cid(host, ocr, cid, &rocr);
>>> + err = mmc_sd_get_cid(host, oldcard, ocr, cid, &rocr);
>>> if (err)
>>> return err;
>>>
>>> @@ -995,6 +1018,7 @@ static int mmc_sd_init_card(struct mmc_host
>>> *host, u32
>>> ocr,
>>> if (err)
>>> goto free_card;
>>> } else {
>>> +
>>> /*
>>> * Attempt to change to high-speed (if supported)
>>> */
>>> diff --git a/drivers/mmc/core/sd.h b/drivers/mmc/core/sd.h
>>> index 1ada980..9cd2482 100644
>>> --- a/drivers/mmc/core/sd.h
>>> +++ b/drivers/mmc/core/sd.h
>>> @@ -8,7 +8,8 @@
>>> struct mmc_host;
>>> struct mmc_card;
>>>
>>> -int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32
>>> *rocr);
>>> +int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
>>> + u32 ocr, u32 *cid, u32 *rocr);
>>> int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card);
>>> void mmc_decode_cid(struct mmc_card *card);
>>> int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>> index cc43687..08e82fe 100644
>>> --- a/drivers/mmc/core/sdio.c
>>> +++ b/drivers/mmc/core/sdio.c
>>> @@ -608,7 +608,7 @@ static int mmc_sdio_init_card(struct mmc_host
>>> *host,
>>> u32 ocr,
>>> }
>>>
>>> if ((rocr & R4_MEMORY_PRESENT) &&
>>> - mmc_sd_get_cid(host, ocr & rocr, card->raw_cid, NULL) ==
>>> 0) {
>>> + mmc_sd_get_cid(host, NULL, ocr & rocr, card->raw_cid,
>>> NULL) ==
>>> 0) {
>>> card->type = MMC_TYPE_SD_COMBO;
>>>
>>> if (oldcard && (oldcard->type != MMC_TYPE_SD_COMBO ||
>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>> index e567fa5..edea93b0 100644
>>> --- a/drivers/regulator/core.c
>>> +++ b/drivers/regulator/core.c
>>> @@ -2522,6 +2522,20 @@ int regulator_is_enabled(struct regulator
>>> *regulator)
>>> EXPORT_SYMBOL_GPL(regulator_is_enabled);
>>>
>>> /**
>>> + * regulator_is_always_on - is the regulator output always enabled
>>> + * @regulator: regulator source
>>> + *
>>> + * Returns positive if the regulator driver backing the source/client
>>> + * has requested that the device be always on, zero if it isn't.
>>> + *
>>> + */
>>> +inline int regulator_is_always_on(struct regulator *regulator)
>>> +{
>>> + return (regulator->always_on) ? 1 : 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(regulator_is_always_on);
>>> +
>>> +/**
>>> * regulator_count_voltages - count regulator_list_voltage()
>>> selectors
>>> * @regulator: regulator source
>>> *
>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>> index ebd1ceb..2f1206b 100644
>>> --- a/include/linux/mmc/host.h
>>> +++ b/include/linux/mmc/host.h
>>> @@ -462,6 +462,7 @@ static inline int mmc_regulator_set_vqmmc(struct
>>> mmc_host *mmc,
>>>
>>> u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
>>> int mmc_regulator_get_supply(struct mmc_host *mmc);
>>> +bool mmc_regulator_can_power_off(struct regulator *supply);
>>>
>>> static inline int mmc_card_is_removable(struct mmc_host *host)
>>> {
>>> diff --git a/include/linux/regulator/consumer.h
>>> b/include/linux/regulator/
>>> consumer.h
>>> index df176d7..46126e3 100644
>>> --- a/include/linux/regulator/consumer.h
>>> +++ b/include/linux/regulator/consumer.h
>>> @@ -226,6 +226,7 @@ void
>>> devm_regulator_bulk_unregister_supply_alias(struct
>>> device *dev,
>>> int regulator_disable(struct regulator *regulator);
>>> int regulator_force_disable(struct regulator *regulator);
>>> int regulator_is_enabled(struct regulator *regulator);
>>> +int regulator_is_always_on(struct regulator *regulator);
>>> int regulator_disable_deferred(struct regulator *regulator, int ms);
>>>
>>> int __must_check regulator_bulk_get(struct device *dev, int
>>> num_consumers,
>>> @@ -418,6 +419,11 @@ static inline int regulator_is_enabled(struct
>>> regulator *regulator)
>>> return 1;
>>> }
>>>
>>> +static inline int regulator_is_always_on(struct regulator *regulator)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> static inline int regulator_bulk_get(struct device *dev,
>>> int num_consumers,
>>> struct regulator_bulk_data
>>> *consumers)
>>> --
>>> 1.9.1
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [EXT] [RFT v2 PATCH] mmc: sd: try to enable UHS mode w/o power cycle from S3
2017-09-01 22:57 ` Zhoujie Wu
@ 2017-09-04 1:06 ` Shawn Lin
2017-09-21 7:44 ` [PATCH RFC] mmc: sd: Fix signal voltage when there is no power cycle Adrian Hunter
0 siblings, 1 reply; 10+ messages in thread
From: Shawn Lin @ 2017-09-04 1:06 UTC (permalink / raw)
To: Zhoujie Wu; +Cc: shawn.lin, Ulf Hansson, linux-mmc
On 2017/9/2 6:57, Zhoujie Wu wrote:
> Hi Shawn,
>
> On 08/29/2017 05:14 PM, Shawn Lin wrote:
>> On 2017/8/30 6:13, Zhoujie Wu wrote:
>>> Hi Shawn,
>>> thanks for your patch.
>>>
>>> On 08/29/2017 02:56 PM, zhoujie wu wrote:
>>>>
>>>>
>>>> CMD0 couldn't put the card back to 3.3v if can't do power
>>>> cycle for cards. The card is already in SDR12 then, so still
>>>> try to enable UHS mode.
>>>>
>>>> Pleas note that, if the platform use a hardware fixed
>>>> sd power, you should still include a regulator-fixed
>>>> regulator with always-on property there. So mmc core
>>>> could handle this as well. Otherwise you shouldn't
>>>> expect UHS to work for that case.
>>>>
>>>> Just compile only, please have a look and test.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>
>>>> ---
>>>>
>>>> drivers/mmc/core/card.h | 17 +++++++++++++++++
>>>> drivers/mmc/core/core.c | 8 ++++++++
>>>> drivers/mmc/core/sd.c | 28 ++++++++++++++++++++++++++--
>>>> drivers/mmc/core/sd.h | 3 ++-
>>>> drivers/mmc/core/sdio.c | 2 +-
>>>> drivers/regulator/core.c | 14 ++++++++++++++
>>>> include/linux/mmc/host.h | 1 +
>>>> include/linux/regulator/consumer.h | 6 ++++++
>>>> 8 files changed, 75 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
>>>> index f06cd91..a562110 100644
>>>> --- a/drivers/mmc/core/card.h
>>>> +++ b/drivers/mmc/core/card.h
>>>> @@ -218,4 +218,21 @@ static inline int mmc_card_broken_hpi(const struct
>>>> mmc_card *c)
>>>> return c->quirks & MMC_QUIRK_BROKEN_HPI;
>>>> }
>>>>
>>>> +static inline bool mmc_card_support_uhs(struct mmc_card *card)
>>>> +{
>>>> + if (WARN_ON(!card))
>>>> + return false;
>>>> +
>>>> + switch (card->sd_bus_speed) {
>>>> + case UHS_SDR104_BUS_SPEED:
>>>> + case UHS_DDR50_BUS_SPEED:
>>>> + case UHS_SDR50_BUS_SPEED:
>>>> + case UHS_SDR25_BUS_SPEED:
>>>> + case UHS_SDR12_BUS_SPEED:
>>>> + return true;
>>>> + default:
>>>> + return false;
>>>> + }
>>>> +}
>>>> +
>>>> #endif
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index 2643126..6b5c6c5 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -1424,6 +1424,14 @@ int mmc_regulator_get_supply(struct mmc_host
>>>> *mmc)
>>>> }
>>>> EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);
>>>>
>>>> +bool mmc_regulator_can_power_off(struct regulator *supply)
>>>> +{
>>>> + if (!IS_ERR(supply))
>>>> + return regulator_is_always_on(supply);
>>>> + return false;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(mmc_regulator_can_power_off);
>>>> +
>>>> /*
>>>> * Mask off any voltages we don't support and select
>>>> * the lowest voltage
>>>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>>>> index a1b0aa1..7bec61b 100644
>>>> --- a/drivers/mmc/core/sd.c
>>>> +++ b/drivers/mmc/core/sd.c
>>>> @@ -721,7 +721,8 @@ struct device_type sd_type = {
>>>> /*
>>>> * Fetch CID from card.
>>>> */
>>>> -int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32
>>>> *rocr)
>>>> +int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
>>>> + u32 ocr, u32 *cid, u32 *rocr)
>>>> {
>>>> int err;
>>>> u32 max_current;
>>>> @@ -773,6 +774,27 @@ int mmc_sd_get_cid(struct mmc_host *host, u32
>>>> ocr, u32
>>>> *cid, u32 *rocr)
>>>> return err;
>>>>
>>>> /*
>>>> + * Per SD physical specification version 4.10, section
>>>> + * 3.9.4 UHS-I Bus Speed Mode Selection Sequence, once
>>>> + * the signal voltage is switched to 1V8, the card continues
>>>> + * 1.8V signaling regardless of CMD0. Power cycle resets the
>>>> + * signal voltage to 3.3V. As a fact that some vmmc is always
>>>> + * on so the card would still work with SDR12 after CMD0. So
>>>> + * let's attempt to enable enable UHS mode if vmmc can't be
>>>> + * powered off.
>>>> + */
>>>> + if (card && mmc_card_support_uhs(card) && mmc_host_uhs(host) &&
>>>> + !mmc_regulator_can_power_off(host->supply.vmmc) &&
>>>> + (ocr & SD_OCR_S18R)) {
>>>> + *rocr |= SD_ROCR_S18A;
>>>> + if (mmc_set_signal_voltage(host,
>>>> MMC_SIGNAL_VOLTAGE_180)) {
>>>> + retries = 0;
>>>> + goto try_again;
>>>> + }
>>>> +
>>>> + goto done;
>>>> + }
>>> Good to have a higher level solution if the issue is common.
>>> I did basic test with your patch(added your regulator modification
>>> you sent to me) and add vmmc-supply in board dts, I saw warning when
>>> boot up.
>>> I think in below case, the card is not added yet, the card is null
>>> and print out the warning. I don't quite understand
>>> mmc_card_support_uhs, it return true for all card speed.
>>>
>>> [ 4.310003] WARNING: CPU: 1 PID: 48 at drivers/mmc/core/card.h:224
>>> mmc_sd_get_cid+0x270/0x298
>>
>> it's just for debug, and won't be a big deal.
>>
>> And I find mmc_host_uhs here is redundant as oldcard's
>> card->sd_bus_speed is already the result of checking uhs cap for both of
>> controller and card. So mmc_card_support_uhs return true means both
>> of your controller and the cards support UHS mode, didn't they?
>>
>>>
>>> .....
>>>
>>> [ 4.549657] [<ffffff8008757e24>] mmc_sd_get_cid+0x270/0x298
>>>
>>> [ 4.555510] [<ffffff800875847c>] mmc_sd_init_card+0x44/0x614
>>>
>>> [ 4.561456] [<ffffff8008758bd4>] mmc_attach_sd+0x7c/0x134
>>>
>>> [ 4.566951] [<ffffff8008751fb8>] mmc_rescan+0x2e4/0x35c
>>>
>>> [ 4.572451] [<ffffff80080d58d8>] process_one_work+0x12c/0x28c
>>>
>>> [ 4.578304] [<ffffff80080d5a90>] worker_thread+0x58/0x3b8
>>>
>>> [ 4.583801] [<ffffff80080db394>] kthread+0x100/0x12c
>>>
>>> [ 4.589029] [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
>>>
>>> [ 4.594793] ---[ end trace 58af756d1037eb51 ]---
>>>
>>> I did some modifications and let code enter the if condition any way,
>>> I added some log and you can see, rocr is updated, but when it tried
>>> to switch to 1.8V signal, it failed. I don't know why yet.
>>
>> Ok, sounds the key point is to see why it fails to switch to 1.8v signal
>>
>>> So the code will try again, and use high speed to init card again.
>>
>> Thanks for testing this, but I haven' had enough time to put my hand
>> on it. But I will be back and check it out by myself.. Anyway you may
>> still be able to do more debugging if this approach looks good to you:)
>
> I have checked why 1.8V signal switch failed here, as SD spec mentioned
> it needs to gate the clock for a while when switching the voltage, I
> added some logic to gate the clock before switch signal,
> then re-enable the clock after that, just like what set_uhs_voltage does
> and it could work now.
Good to know that.
> and I adjust the if condition logic as below, hacked
> mmc_card_support_uhs now always return true.
> This can be a fix, but to be a generic solution, I am wondering from sd
> card perspective, except for no power cycle, if there is any other
> reason that may cause SD_OCR_S18A not set for uhs card?
I'm not aware of another reason except for that UHS-II card always
response with S18A=0, but it isn't supportted yet.
Would you mind submitting proper patch(es) for this solution?
>
> + if (mmc_card_support_uhs(card)
> + !mmc_regulator_can_power_off(host->supply.vmmc) &&
> + !(*rocr & SD_OCR_S18A))
>>
>>>
>>> [ 4.259194] before rocr = c0ff8000, ocr = 41200000
>>>
>>> [ 4.359909] after rocr = c1ff8000 -- if condition meets, updated
>>> rocr
>>>
>>> [ 4.364230] mmc0: 1.8V regulator output did not became stable --
>>> mmc_set_signal_voltage 1.8V failed.
>>> [ 4.372603] mmc0: Skipping voltage switch
>>>
>>> [ 4.403225] before rocr = c0ff8000, ocr = 40200000 -- tried
>>> again with no SD_OCR_S18R .
>>>> + /*
>>>> * In case CCS and S18A in the response is set, start
>>>> Signal Voltage
>>>> * Switch procedure. SPI mode doesn't support CMD11.
>>>> */
>>>> @@ -788,6 +810,7 @@ int mmc_sd_get_cid(struct mmc_host *host, u32
>>>> ocr, u32
>>>> *cid, u32 *rocr)
>>>> }
>>>> }
>>>>
>>>> +done:
>>>> err = mmc_send_cid(host, cid);
>>>> return err;
>>>> }
>>>> @@ -924,7 +947,7 @@ static int mmc_sd_init_card(struct mmc_host
>>>> *host, u32
>>>> ocr,
>>>>
>>>> WARN_ON(!host->claimed);
>>>>
>>>> - err = mmc_sd_get_cid(host, ocr, cid, &rocr);
>>>> + err = mmc_sd_get_cid(host, oldcard, ocr, cid, &rocr);
>>>> if (err)
>>>> return err;
>>>>
>>>> @@ -995,6 +1018,7 @@ static int mmc_sd_init_card(struct mmc_host
>>>> *host, u32
>>>> ocr,
>>>> if (err)
>>>> goto free_card;
>>>> } else {
>>>> +
>>>> /*
>>>> * Attempt to change to high-speed (if supported)
>>>> */
>>>> diff --git a/drivers/mmc/core/sd.h b/drivers/mmc/core/sd.h
>>>> index 1ada980..9cd2482 100644
>>>> --- a/drivers/mmc/core/sd.h
>>>> +++ b/drivers/mmc/core/sd.h
>>>> @@ -8,7 +8,8 @@
>>>> struct mmc_host;
>>>> struct mmc_card;
>>>>
>>>> -int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32
>>>> *rocr);
>>>> +int mmc_sd_get_cid(struct mmc_host *host, struct mmc_card *card,
>>>> + u32 ocr, u32 *cid, u32 *rocr);
>>>> int mmc_sd_get_csd(struct mmc_host *host, struct mmc_card *card);
>>>> void mmc_decode_cid(struct mmc_card *card);
>>>> int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>> index cc43687..08e82fe 100644
>>>> --- a/drivers/mmc/core/sdio.c
>>>> +++ b/drivers/mmc/core/sdio.c
>>>> @@ -608,7 +608,7 @@ static int mmc_sdio_init_card(struct mmc_host
>>>> *host,
>>>> u32 ocr,
>>>> }
>>>>
>>>> if ((rocr & R4_MEMORY_PRESENT) &&
>>>> - mmc_sd_get_cid(host, ocr & rocr, card->raw_cid, NULL) ==
>>>> 0) {
>>>> + mmc_sd_get_cid(host, NULL, ocr & rocr, card->raw_cid,
>>>> NULL) ==
>>>> 0) {
>>>> card->type = MMC_TYPE_SD_COMBO;
>>>>
>>>> if (oldcard && (oldcard->type != MMC_TYPE_SD_COMBO ||
>>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>>> index e567fa5..edea93b0 100644
>>>> --- a/drivers/regulator/core.c
>>>> +++ b/drivers/regulator/core.c
>>>> @@ -2522,6 +2522,20 @@ int regulator_is_enabled(struct regulator
>>>> *regulator)
>>>> EXPORT_SYMBOL_GPL(regulator_is_enabled);
>>>>
>>>> /**
>>>> + * regulator_is_always_on - is the regulator output always enabled
>>>> + * @regulator: regulator source
>>>> + *
>>>> + * Returns positive if the regulator driver backing the source/client
>>>> + * has requested that the device be always on, zero if it isn't.
>>>> + *
>>>> + */
>>>> +inline int regulator_is_always_on(struct regulator *regulator)
>>>> +{
>>>> + return (regulator->always_on) ? 1 : 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(regulator_is_always_on);
>>>> +
>>>> +/**
>>>> * regulator_count_voltages - count regulator_list_voltage()
>>>> selectors
>>>> * @regulator: regulator source
>>>> *
>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>> index ebd1ceb..2f1206b 100644
>>>> --- a/include/linux/mmc/host.h
>>>> +++ b/include/linux/mmc/host.h
>>>> @@ -462,6 +462,7 @@ static inline int mmc_regulator_set_vqmmc(struct
>>>> mmc_host *mmc,
>>>>
>>>> u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
>>>> int mmc_regulator_get_supply(struct mmc_host *mmc);
>>>> +bool mmc_regulator_can_power_off(struct regulator *supply);
>>>>
>>>> static inline int mmc_card_is_removable(struct mmc_host *host)
>>>> {
>>>> diff --git a/include/linux/regulator/consumer.h
>>>> b/include/linux/regulator/
>>>> consumer.h
>>>> index df176d7..46126e3 100644
>>>> --- a/include/linux/regulator/consumer.h
>>>> +++ b/include/linux/regulator/consumer.h
>>>> @@ -226,6 +226,7 @@ void
>>>> devm_regulator_bulk_unregister_supply_alias(struct
>>>> device *dev,
>>>> int regulator_disable(struct regulator *regulator);
>>>> int regulator_force_disable(struct regulator *regulator);
>>>> int regulator_is_enabled(struct regulator *regulator);
>>>> +int regulator_is_always_on(struct regulator *regulator);
>>>> int regulator_disable_deferred(struct regulator *regulator, int ms);
>>>>
>>>> int __must_check regulator_bulk_get(struct device *dev, int
>>>> num_consumers,
>>>> @@ -418,6 +419,11 @@ static inline int regulator_is_enabled(struct
>>>> regulator *regulator)
>>>> return 1;
>>>> }
>>>>
>>>> +static inline int regulator_is_always_on(struct regulator *regulator)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> static inline int regulator_bulk_get(struct device *dev,
>>>> int num_consumers,
>>>> struct regulator_bulk_data
>>>> *consumers)
>>>> --
>>>> 1.9.1
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RFC] mmc: sd: Fix signal voltage when there is no power cycle
2017-09-04 1:06 ` Shawn Lin
@ 2017-09-21 7:44 ` Adrian Hunter
2017-09-21 21:52 ` [EXT] " Zhoujie Wu
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Adrian Hunter @ 2017-09-21 7:44 UTC (permalink / raw)
To: Shawn Lin, Zhoujie Wu; +Cc: Ulf Hansson, linux-mmc
If the card has not been power cycled, it may still be using 1.8V
signaling. According to the SD spec., the Bus Speed Mode (function group 1)
bits 2 to 4 are zero if the card is initialized at 3.3V signal level. Thus
they can be used to determine if the card has already switched to 1.8V
signaling. Detect that situation and try to initialize a UHS-I (1.8V)
transfer mode.
Tested with the following cards:
Transcend 4GB High Speed
Kingston 64GB SDR104
Lexar by Micron HIGH-PERFORMANCE 300x 16GB DDR50
SanDisk Ultra 8GB DDR50
Transcend Ultimate 600x 16GB SDR104
Transcend Premium 300x 64GB SDR104
Lexar by Micron Professional 1000x 32GB UHS-II SDR104
SanDisk Extreme Pro 16GB SDR104
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
Here is an alternative approach to Shawn's that uses CMD6 to identify the
card's signal voltage.
It has the advantages that it does not depend on knowing about the regulator,
and it works even when unbinding / re-binding the host controller.
drivers/mmc/core/core.c | 38 ++++++++++++++++++++++++--------------
drivers/mmc/core/core.h | 1 +
drivers/mmc/core/sd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 70 insertions(+), 16 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 24a73f387482..161c1d105d91 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1723,11 +1723,33 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
}
+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;
+
+ /* 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;
+}
+
int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
{
struct mmc_command cmd = {};
int err = 0;
- u32 clock;
/*
* If we cannot switch voltages, return failure so the caller
@@ -1759,15 +1781,8 @@ int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
err = -EAGAIN;
goto power_cycle;
}
- /*
- * 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)) {
+ if (mmc_host_set_uhs_voltage(host)) {
/*
* Voltages may not have been switched, but we've already
* sent CMD11, so a power cycle is required anyway
@@ -1776,11 +1791,6 @@ int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
goto power_cycle;
}
- /* 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);
-
/* Wait for at least 1 ms according to spec */
mmc_delay(1);
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 03e0f8384b1c..2b2690d0d877 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -51,6 +51,7 @@ struct device_node *mmc_of_find_child_device(struct mmc_host *host,
void mmc_set_bus_width(struct mmc_host *host, unsigned int width);
u32 mmc_select_voltage(struct mmc_host *host, u32 ocr);
int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr);
+int mmc_host_set_uhs_voltage(struct mmc_host *host);
int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
void mmc_set_timing(struct mmc_host *host, unsigned int timing);
void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 4fd1620b732d..35e1f52cb3f9 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -908,6 +908,18 @@ unsigned mmc_sd_get_max_clock(struct mmc_card *card)
return max_dtr;
}
+static bool mmc_sd_card_using_v18(struct mmc_card *card)
+{
+ /*
+ * According to the SD spec., the Bus Speed Mode (function group 1) bits
+ * 2 to 4 are zero if the card is initialized at 3.3V signal level. Thus
+ * they can be used to determine if the card has already switched to
+ * 1.8V signaling.
+ */
+ return card->sw_caps.sd3_bus_mode &
+ (SD_MODE_UHS_SDR50 | SD_MODE_UHS_SDR104 | SD_MODE_UHS_DDR50);
+}
+
/*
* Handle the detection and initialisation of a card.
*
@@ -921,9 +933,10 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
int err;
u32 cid[4];
u32 rocr = 0;
+ bool v18_fixup_failed = false;
WARN_ON(!host->claimed);
-
+retry:
err = mmc_sd_get_cid(host, ocr, cid, &rocr);
if (err)
return err;
@@ -989,6 +1002,36 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
if (err)
goto free_card;
+ /*
+ * If the card has not been power cycled, it may still be using 1.8V
+ * signaling. Detect that situation and try to initialize a UHS-I (1.8V)
+ * transfer mode.
+ */
+ if (!v18_fixup_failed && !mmc_host_is_spi(host) && mmc_host_uhs(host) &&
+ mmc_sd_card_using_v18(card) &&
+ host->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
+ /*
+ * Re-read switch information in case it has changed since
+ * oldcard was initialized.
+ */
+ if (oldcard) {
+ err = mmc_read_switch(card);
+ if (err)
+ goto free_card;
+ }
+ if (mmc_sd_card_using_v18(card)) {
+ if (mmc_host_set_uhs_voltage(host) ||
+ mmc_sd_init_uhs_card(card)) {
+ v18_fixup_failed = true;
+ mmc_power_cycle(host, ocr);
+ if (!oldcard)
+ mmc_remove_card(card);
+ goto retry;
+ }
+ goto done;
+ }
+ }
+
/* Initialization sequence for UHS-I cards */
if (rocr & SD_ROCR_S18A) {
err = mmc_sd_init_uhs_card(card);
@@ -1021,7 +1064,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
}
}
-
+done:
host->card = card;
return 0;
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [EXT] [PATCH RFC] mmc: sd: Fix signal voltage when there is no power cycle
2017-09-21 7:44 ` [PATCH RFC] mmc: sd: Fix signal voltage when there is no power cycle Adrian Hunter
@ 2017-09-21 21:52 ` Zhoujie Wu
2017-09-22 0:39 ` Shawn Lin
2017-09-22 10:00 ` Ulf Hansson
2 siblings, 0 replies; 10+ messages in thread
From: Zhoujie Wu @ 2017-09-21 21:52 UTC (permalink / raw)
To: Adrian Hunter, Shawn Lin; +Cc: Ulf Hansson, linux-mmc
Hi Adrian,
This patch can solve uhs card can only be recognized as hs card on
armada3700 board after warm reset or suspend/resume sequence(both have
no power cycle for sd card).
It looks good for me and tested on armada3700 board with following cards:
Two sandisk 8G uhs card DDR50
Apacer 8G uhs card DDR50
Can it be an official patch? Thanks for your help.
Tested-by: Zhoujie Wu <zjwu@marvell.com>
On 09/21/2017 12:44 AM, Adrian Hunter wrote:
> External Email
>
> ----------------------------------------------------------------------
> If the card has not been power cycled, it may still be using 1.8V
> signaling. According to the SD spec., the Bus Speed Mode (function group 1)
> bits 2 to 4 are zero if the card is initialized at 3.3V signal level. Thus
> they can be used to determine if the card has already switched to 1.8V
> signaling. Detect that situation and try to initialize a UHS-I (1.8V)
> transfer mode.
>
> Tested with the following cards:
> Transcend 4GB High Speed
> Kingston 64GB SDR104
> Lexar by Micron HIGH-PERFORMANCE 300x 16GB DDR50
> SanDisk Ultra 8GB DDR50
> Transcend Ultimate 600x 16GB SDR104
> Transcend Premium 300x 64GB SDR104
> Lexar by Micron Professional 1000x 32GB UHS-II SDR104
> SanDisk Extreme Pro 16GB SDR104
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>
>
> Here is an alternative approach to Shawn's that uses CMD6 to identify the
> card's signal voltage.
>
> It has the advantages that it does not depend on knowing about the regulator,
> and it works even when unbinding / re-binding the host controller.
>
>
> drivers/mmc/core/core.c | 38 ++++++++++++++++++++++++--------------
> drivers/mmc/core/core.h | 1 +
> drivers/mmc/core/sd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 70 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 24a73f387482..161c1d105d91 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1723,11 +1723,33 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
>
> }
>
> +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;
> +
> + /* 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;
> +}
> +
> int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
> {
> struct mmc_command cmd = {};
> int err = 0;
> - u32 clock;
>
> /*
> * If we cannot switch voltages, return failure so the caller
> @@ -1759,15 +1781,8 @@ int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
> err = -EAGAIN;
> goto power_cycle;
> }
> - /*
> - * 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)) {
> + if (mmc_host_set_uhs_voltage(host)) {
> /*
> * Voltages may not have been switched, but we've already
> * sent CMD11, so a power cycle is required anyway
> @@ -1776,11 +1791,6 @@ int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
> goto power_cycle;
> }
>
> - /* 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);
> -
> /* Wait for at least 1 ms according to spec */
> mmc_delay(1);
>
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 03e0f8384b1c..2b2690d0d877 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -51,6 +51,7 @@ struct device_node *mmc_of_find_child_device(struct mmc_host *host,
> void mmc_set_bus_width(struct mmc_host *host, unsigned int width);
> u32 mmc_select_voltage(struct mmc_host *host, u32 ocr);
> int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr);
> +int mmc_host_set_uhs_voltage(struct mmc_host *host);
> int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
> void mmc_set_timing(struct mmc_host *host, unsigned int timing);
> void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 4fd1620b732d..35e1f52cb3f9 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -908,6 +908,18 @@ unsigned mmc_sd_get_max_clock(struct mmc_card *card)
> return max_dtr;
> }
>
> +static bool mmc_sd_card_using_v18(struct mmc_card *card)
> +{
> + /*
> + * According to the SD spec., the Bus Speed Mode (function group 1) bits
> + * 2 to 4 are zero if the card is initialized at 3.3V signal level. Thus
> + * they can be used to determine if the card has already switched to
> + * 1.8V signaling.
> + */
> + return card->sw_caps.sd3_bus_mode &
> + (SD_MODE_UHS_SDR50 | SD_MODE_UHS_SDR104 | SD_MODE_UHS_DDR50);
> +}
> +
> /*
> * Handle the detection and initialisation of a card.
> *
> @@ -921,9 +933,10 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> int err;
> u32 cid[4];
> u32 rocr = 0;
> + bool v18_fixup_failed = false;
>
> WARN_ON(!host->claimed);
> -
> +retry:
> err = mmc_sd_get_cid(host, ocr, cid, &rocr);
> if (err)
> return err;
> @@ -989,6 +1002,36 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> if (err)
> goto free_card;
>
> + /*
> + * If the card has not been power cycled, it may still be using 1.8V
> + * signaling. Detect that situation and try to initialize a UHS-I (1.8V)
> + * transfer mode.
> + */
> + if (!v18_fixup_failed && !mmc_host_is_spi(host) && mmc_host_uhs(host) &&
> + mmc_sd_card_using_v18(card) &&
> + host->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> + /*
> + * Re-read switch information in case it has changed since
> + * oldcard was initialized.
> + */
> + if (oldcard) {
> + err = mmc_read_switch(card);
> + if (err)
> + goto free_card;
> + }
> + if (mmc_sd_card_using_v18(card)) {
> + if (mmc_host_set_uhs_voltage(host) ||
> + mmc_sd_init_uhs_card(card)) {
> + v18_fixup_failed = true;
> + mmc_power_cycle(host, ocr);
> + if (!oldcard)
> + mmc_remove_card(card);
> + goto retry;
> + }
> + goto done;
> + }
> + }
> +
> /* Initialization sequence for UHS-I cards */
> if (rocr & SD_ROCR_S18A) {
> err = mmc_sd_init_uhs_card(card);
> @@ -1021,7 +1064,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
> }
> }
> -
> +done:
> host->card = card;
> return 0;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] mmc: sd: Fix signal voltage when there is no power cycle
2017-09-21 7:44 ` [PATCH RFC] mmc: sd: Fix signal voltage when there is no power cycle Adrian Hunter
2017-09-21 21:52 ` [EXT] " Zhoujie Wu
@ 2017-09-22 0:39 ` Shawn Lin
2017-09-22 10:00 ` Ulf Hansson
2 siblings, 0 replies; 10+ messages in thread
From: Shawn Lin @ 2017-09-22 0:39 UTC (permalink / raw)
To: Adrian Hunter; +Cc: Zhoujie Wu, shawn.lin, Ulf Hansson, linux-mmc
On 2017/9/21 15:44, Adrian Hunter wrote:
> If the card has not been power cycled, it may still be using 1.8V
> signaling. According to the SD spec., the Bus Speed Mode (function group 1)
> bits 2 to 4 are zero if the card is initialized at 3.3V signal level. Thus
> they can be used to determine if the card has already switched to 1.8V
> signaling. Detect that situation and try to initialize a UHS-I (1.8V)
> transfer mode.
>
> Tested with the following cards:
> Transcend 4GB High Speed
> Kingston 64GB SDR104
> Lexar by Micron HIGH-PERFORMANCE 300x 16GB DDR50
> SanDisk Ultra 8GB DDR50
> Transcend Ultimate 600x 16GB SDR104
> Transcend Premium 300x 64GB SDR104
> Lexar by Micron Professional 1000x 32GB UHS-II SDR104
> SanDisk Extreme Pro 16GB SDR104
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>
>
> Here is an alternative approach to Shawn's that uses CMD6 to identify the
> card's signal voltage.
>
> It has the advantages that it does not depend on knowing about the regulator,
> and it works even when unbinding / re-binding the host controller.
>
Your approach looks great!
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> drivers/mmc/core/core.c | 38 ++++++++++++++++++++++++--------------
> drivers/mmc/core/core.h | 1 +
> drivers/mmc/core/sd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 70 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 24a73f387482..161c1d105d91 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1723,11 +1723,33 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
>
> }
>
> +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;
> +
> + /* 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;
> +}
> +
> int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
> {
> struct mmc_command cmd = {};
> int err = 0;
> - u32 clock;
>
> /*
> * If we cannot switch voltages, return failure so the caller
> @@ -1759,15 +1781,8 @@ int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
> err = -EAGAIN;
> goto power_cycle;
> }
> - /*
> - * 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)) {
> + if (mmc_host_set_uhs_voltage(host)) {
> /*
> * Voltages may not have been switched, but we've already
> * sent CMD11, so a power cycle is required anyway
> @@ -1776,11 +1791,6 @@ int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
> goto power_cycle;
> }
>
> - /* 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);
> -
> /* Wait for at least 1 ms according to spec */
> mmc_delay(1);
>
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 03e0f8384b1c..2b2690d0d877 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -51,6 +51,7 @@ struct device_node *mmc_of_find_child_device(struct mmc_host *host,
> void mmc_set_bus_width(struct mmc_host *host, unsigned int width);
> u32 mmc_select_voltage(struct mmc_host *host, u32 ocr);
> int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr);
> +int mmc_host_set_uhs_voltage(struct mmc_host *host);
> int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
> void mmc_set_timing(struct mmc_host *host, unsigned int timing);
> void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 4fd1620b732d..35e1f52cb3f9 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -908,6 +908,18 @@ unsigned mmc_sd_get_max_clock(struct mmc_card *card)
> return max_dtr;
> }
>
> +static bool mmc_sd_card_using_v18(struct mmc_card *card)
> +{
> + /*
> + * According to the SD spec., the Bus Speed Mode (function group 1) bits
> + * 2 to 4 are zero if the card is initialized at 3.3V signal level. Thus
> + * they can be used to determine if the card has already switched to
> + * 1.8V signaling.
> + */
> + return card->sw_caps.sd3_bus_mode &
> + (SD_MODE_UHS_SDR50 | SD_MODE_UHS_SDR104 | SD_MODE_UHS_DDR50);
> +}
> +
> /*
> * Handle the detection and initialisation of a card.
> *
> @@ -921,9 +933,10 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> int err;
> u32 cid[4];
> u32 rocr = 0;
> + bool v18_fixup_failed = false;
>
> WARN_ON(!host->claimed);
> -
> +retry:
> err = mmc_sd_get_cid(host, ocr, cid, &rocr);
> if (err)
> return err;
> @@ -989,6 +1002,36 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> if (err)
> goto free_card;
>
> + /*
> + * If the card has not been power cycled, it may still be using 1.8V
> + * signaling. Detect that situation and try to initialize a UHS-I (1.8V)
> + * transfer mode.
> + */
> + if (!v18_fixup_failed && !mmc_host_is_spi(host) && mmc_host_uhs(host) &&
> + mmc_sd_card_using_v18(card) &&
> + host->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> + /*
> + * Re-read switch information in case it has changed since
> + * oldcard was initialized.
> + */
> + if (oldcard) {
> + err = mmc_read_switch(card);
> + if (err)
> + goto free_card;
> + }
> + if (mmc_sd_card_using_v18(card)) {
> + if (mmc_host_set_uhs_voltage(host) ||
> + mmc_sd_init_uhs_card(card)) {
> + v18_fixup_failed = true;
> + mmc_power_cycle(host, ocr);
> + if (!oldcard)
> + mmc_remove_card(card);
> + goto retry;
> + }
> + goto done;
> + }
> + }
> +
> /* Initialization sequence for UHS-I cards */
> if (rocr & SD_ROCR_S18A) {
> err = mmc_sd_init_uhs_card(card);
> @@ -1021,7 +1064,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
> }
> }
> -
> +done:
> host->card = card;
> return 0;
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] mmc: sd: Fix signal voltage when there is no power cycle
2017-09-21 7:44 ` [PATCH RFC] mmc: sd: Fix signal voltage when there is no power cycle Adrian Hunter
2017-09-21 21:52 ` [EXT] " Zhoujie Wu
2017-09-22 0:39 ` Shawn Lin
@ 2017-09-22 10:00 ` Ulf Hansson
2 siblings, 0 replies; 10+ messages in thread
From: Ulf Hansson @ 2017-09-22 10:00 UTC (permalink / raw)
To: Adrian Hunter; +Cc: Shawn Lin, Zhoujie Wu, linux-mmc
On 21 September 2017 at 09:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
> If the card has not been power cycled, it may still be using 1.8V
> signaling. According to the SD spec., the Bus Speed Mode (function group 1)
Could you elaborate a bit more on what particular use case this is
about in the change log. My first impression was that this is about
suspend/resume, but that isn't the case.
> bits 2 to 4 are zero if the card is initialized at 3.3V signal level. Thus
> they can be used to determine if the card has already switched to 1.8V
> signaling. Detect that situation and try to initialize a UHS-I (1.8V)
> transfer mode.
Seems like a good approach to fix the problem.
>
> Tested with the following cards:
> Transcend 4GB High Speed
> Kingston 64GB SDR104
> Lexar by Micron HIGH-PERFORMANCE 300x 16GB DDR50
> SanDisk Ultra 8GB DDR50
> Transcend Ultimate 600x 16GB SDR104
> Transcend Premium 300x 64GB SDR104
> Lexar by Micron Professional 1000x 32GB UHS-II SDR104
> SanDisk Extreme Pro 16GB SDR104
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>
>
> Here is an alternative approach to Shawn's that uses CMD6 to identify the
> card's signal voltage.
>
> It has the advantages that it does not depend on knowing about the regulator,
> and it works even when unbinding / re-binding the host controller.
>
>
> drivers/mmc/core/core.c | 38 ++++++++++++++++++++++++--------------
> drivers/mmc/core/core.h | 1 +
> drivers/mmc/core/sd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 70 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 24a73f387482..161c1d105d91 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1723,11 +1723,33 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
>
> }
>
> +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;
> +
> + /* 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;
> +}
> +
> int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
> {
> struct mmc_command cmd = {};
> int err = 0;
> - u32 clock;
>
> /*
> * If we cannot switch voltages, return failure so the caller
> @@ -1759,15 +1781,8 @@ int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
> err = -EAGAIN;
> goto power_cycle;
> }
> - /*
> - * 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)) {
> + if (mmc_host_set_uhs_voltage(host)) {
> /*
> * Voltages may not have been switched, but we've already
> * sent CMD11, so a power cycle is required anyway
> @@ -1776,11 +1791,6 @@ int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr)
> goto power_cycle;
> }
>
> - /* 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);
> -
> /* Wait for at least 1 ms according to spec */
> mmc_delay(1);
The above seems like a nice re-factoring, could you perhaps make that
a separate change, as it then becomes more clear what change is needed
to really fix the problem.
>
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 03e0f8384b1c..2b2690d0d877 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -51,6 +51,7 @@ struct device_node *mmc_of_find_child_device(struct mmc_host *host,
> void mmc_set_bus_width(struct mmc_host *host, unsigned int width);
> u32 mmc_select_voltage(struct mmc_host *host, u32 ocr);
> int mmc_set_uhs_voltage(struct mmc_host *host, u32 ocr);
> +int mmc_host_set_uhs_voltage(struct mmc_host *host);
> int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
> void mmc_set_timing(struct mmc_host *host, unsigned int timing);
> void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 4fd1620b732d..35e1f52cb3f9 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -908,6 +908,18 @@ unsigned mmc_sd_get_max_clock(struct mmc_card *card)
> return max_dtr;
> }
>
> +static bool mmc_sd_card_using_v18(struct mmc_card *card)
> +{
> + /*
> + * According to the SD spec., the Bus Speed Mode (function group 1) bits
> + * 2 to 4 are zero if the card is initialized at 3.3V signal level. Thus
> + * they can be used to determine if the card has already switched to
> + * 1.8V signaling.
> + */
> + return card->sw_caps.sd3_bus_mode &
> + (SD_MODE_UHS_SDR50 | SD_MODE_UHS_SDR104 | SD_MODE_UHS_DDR50);
> +}
> +
> /*
> * Handle the detection and initialisation of a card.
> *
> @@ -921,9 +933,10 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> int err;
> u32 cid[4];
> u32 rocr = 0;
> + bool v18_fixup_failed = false;
>
> WARN_ON(!host->claimed);
> -
> +retry:
> err = mmc_sd_get_cid(host, ocr, cid, &rocr);
> if (err)
> return err;
> @@ -989,6 +1002,36 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> if (err)
> goto free_card;
>
> + /*
> + * If the card has not been power cycled, it may still be using 1.8V
> + * signaling. Detect that situation and try to initialize a UHS-I (1.8V)
> + * transfer mode.
> + */
> + if (!v18_fixup_failed && !mmc_host_is_spi(host) && mmc_host_uhs(host) &&
> + mmc_sd_card_using_v18(card) &&
> + host->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> + /*
> + * Re-read switch information in case it has changed since
> + * oldcard was initialized.
> + */
> + if (oldcard) {
> + err = mmc_read_switch(card);
> + if (err)
> + goto free_card;
> + }
> + if (mmc_sd_card_using_v18(card)) {
> + if (mmc_host_set_uhs_voltage(host) ||
> + mmc_sd_init_uhs_card(card)) {
> + v18_fixup_failed = true;
> + mmc_power_cycle(host, ocr);
> + if (!oldcard)
> + mmc_remove_card(card);
> + goto retry;
> + }
> + goto done;
> + }
> + }
> +
> /* Initialization sequence for UHS-I cards */
> if (rocr & SD_ROCR_S18A) {
> err = mmc_sd_init_uhs_card(card);
> @@ -1021,7 +1064,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
> }
> }
> -
> +done:
> host->card = card;
> return 0;
>
> --
> 1.9.1
>
Besides the minor nitpicks, this looks good to me.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-09-22 10:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-29 3:53 [RFT v2 PATCH] mmc: sd: try to enable UHS mode w/o power cycle from S3 Shawn Lin
[not found] ` <CAAXpJNvfSWHrJiLHoxmm+ENUDsGayQROnRTL=SoHr-7dX7R9+Q@mail.gmail.com>
2017-08-29 22:13 ` [EXT] " Zhoujie Wu
2017-08-30 0:14 ` Shawn Lin
2017-08-30 22:32 ` Zhoujie Wu
2017-09-01 22:57 ` Zhoujie Wu
2017-09-04 1:06 ` Shawn Lin
2017-09-21 7:44 ` [PATCH RFC] mmc: sd: Fix signal voltage when there is no power cycle Adrian Hunter
2017-09-21 21:52 ` [EXT] " Zhoujie Wu
2017-09-22 0:39 ` Shawn Lin
2017-09-22 10:00 ` Ulf Hansson
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.