Re: [EXT] [RFT v2 PATCH] mmc: sd: try to enable UHS mode w/o power cycle from S3

[Zhoujie Wu <zjwu@marvell.com>]

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