[V3,1/3] mmc: sdhci: Allow platform controlled voltage switching

Commit Message

Veerabhadrarao Badiganti Oct. 8, 2018, 1:18 p.m. UTC

From: Vijay Viswanath <[e-mail]>

Some controllers can have internal mechanism to inform the SW that it
is ready for voltage switching. For such controllers, changing voltage
before the HW is ready can result in various issues.

During setup/cleanup of host, check whether regulator enable/disable
was already done by platform driver.

Signed-off-by: Vijay Viswanath <[e-mail]>
Signed-off-by: Veerabhadrarao Badiganti <[e-mail]>
---
 drivers/mmc/host/sdhci.c | 32 +++++++++++++++++++-------------
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 20 insertions(+), 13 deletions(-)

Comments

Evan Green Oct. 16, 2018, 10:09 p.m. UTC | #1

On Mon, Oct 8, 2018 at 6:22 AM Veerabhadrarao Badiganti
<[e-mail]> wrote:
>
> From: Vijay Viswanath <[e-mail]>
>
> Some controllers can have internal mechanism to inform the SW that it
> is ready for voltage switching. For such controllers, changing voltage
> before the HW is ready can result in various issues.
>
> During setup/cleanup of host, check whether regulator enable/disable
> was already done by platform driver.
>
> Signed-off-by: Vijay Viswanath <[e-mail]>
> Signed-off-by: Veerabhadrarao Badiganti <[e-mail]>
> ---
>  drivers/mmc/host/sdhci.c | 32 +++++++++++++++++++-------------
>  drivers/mmc/host/sdhci.h |  1 +
>  2 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 99bdae5..ea7ce1d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3616,6 +3616,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>         unsigned int override_timeout_clk;
>         u32 max_clk;
>         int ret;
> +       bool enable_vqmmc = false;
>
>         WARN_ON(host == NULL);
>         if (host == NULL)
> @@ -3629,9 +3630,12 @@ int sdhci_setup_host(struct sdhci_host *host)
>          * the host can take the appropriate action if regulators are not
>          * available.
>          */
> -       ret = mmc_regulator_get_supply(mmc);
> -       if (ret)
> -               return ret;
> +       if (!mmc->supply.vqmmc) {
> +               ret = mmc_regulator_get_supply(mmc);
> +               if (ret)
> +                       return ret;
> +               enable_vqmmc  = true;
> +       }

Did you not like my suggestion in the previous patch of changing
mmc_regulator_get_supply to only get supplies that were not set
previously? What you have here is almost the same, except you've got
this weird coupling where if vqmmc is already present, you implicitly
skip looking at vmmc entirely (since mmc_regulator_get_supply does
more than just get vqmmc).

>
>         DBG("Version:   0x%08x | Present:  0x%08x\n",
>             sdhci_readw(host, SDHCI_HOST_VERSION),
> @@ -3880,7 +3884,15 @@ int sdhci_setup_host(struct sdhci_host *host)
>                 mmc->caps |= MMC_CAP_NEEDS_POLL;
>
>         if (!IS_ERR(mmc->supply.vqmmc)) {
> -               ret = regulator_enable(mmc->supply.vqmmc);
> +               if (enable_vqmmc) {
> +                       ret = regulator_enable(mmc->supply.vqmmc);
> +                       if (ret) {
> +                               pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
> +                                       mmc_hostname(mmc), ret);
> +                               mmc->supply.vqmmc = ERR_PTR(-EINVAL);
> +                       }
> +                       host->vqmmc_enabled = !ret;
> +               }
>
>                 /* If vqmmc provides no 1.8V signalling, then there's no UHS */
>                 if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
> @@ -3893,12 +3905,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>                 if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000,
>                                                     3600000))
>                         host->flags &= ~SDHCI_SIGNALING_330;
> -
> -               if (ret) {
> -                       pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
> -                               mmc_hostname(mmc), ret);
> -                       mmc->supply.vqmmc = ERR_PTR(-EINVAL);
> -               }
>         }
>
>         if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
> @@ -4136,7 +4142,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>         return 0;
>
>  unreg:
> -       if (!IS_ERR(mmc->supply.vqmmc))
> +       if (host->vqmmc_enabled)
>                 regulator_disable(mmc->supply.vqmmc);
>  undma:
>         if (host->align_buffer)
> @@ -4154,7 +4160,7 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>  {
>         struct mmc_host *mmc = host->mmc;
>
> -       if (!IS_ERR(mmc->supply.vqmmc))
> +       if (host->vqmmc_enabled)
>                 regulator_disable(mmc->supply.vqmmc);
>
>         if (host->align_buffer)
> @@ -4287,7 +4293,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>
>         tasklet_kill(&host->finish_tasklet);
>
> -       if (!IS_ERR(mmc->supply.vqmmc))
> +       if (host->vqmmc_enabled)
>                 regulator_disable(mmc->supply.vqmmc);
>
>         if (host->align_buffer)
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index b001cf4..3c28152 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -524,6 +524,7 @@ struct sdhci_host {
>         bool pending_reset;     /* Cmd/data reset is pending */
>         bool irq_wake_enabled;  /* IRQ wakeup is enabled */
>         bool v4_mode;           /* Host Version 4 Enable */
> +       bool vqmmc_enabled;     /* Vqmmc is enabled */

I still don't love this, since it doesn't mean what it says. Everyone
else that has a vqmmc_enabled member uses it to actually mean "vqmmc
is enabled", but this doesn't mean that. For example, you don't clear
this when you disable the regulator in patch 3, so this would be set
even if the regulator is disabled, and you don't set it when sdhci
enables the regulator, so the regulator is on when this flag is not
set.

At the very least, I think a rename is in order. I'm struggling with
the rename, since
"vqmmc_pltfrm_enable_but_feel_free_to_call_set_voltage_anywhere" is
too long. I guess vqmmc_pltfrm_enabled is the best I've got for now.

>
>         struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];  /* Requests done */
>         struct mmc_command *cmd;        /* Current command */
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>

Veerabhadrarao Badiganti Nov. 12, 2018, 5:19 p.m. UTC | #2

On 10/17/2018 3:39 AM, Evan Green wrote:
> On Mon, Oct 8, 2018 at 6:22 AM Veerabhadrarao Badiganti
> <[e-mail]> wrote:
>> From: Vijay Viswanath <[e-mail]>
>>
>> Some controllers can have internal mechanism to inform the SW that it
>> is ready for voltage switching. For such controllers, changing voltage
>> before the HW is ready can result in various issues.
>>
>> During setup/cleanup of host, check whether regulator enable/disable
>> was already done by platform driver.
>>
>> Signed-off-by: Vijay Viswanath <[e-mail]>
>> Signed-off-by: Veerabhadrarao Badiganti <[e-mail]>
>> ---
>>   drivers/mmc/host/sdhci.c | 32 +++++++++++++++++++-------------
>>   drivers/mmc/host/sdhci.h |  1 +
>>   2 files changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 99bdae5..ea7ce1d 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -3616,6 +3616,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>>          unsigned int override_timeout_clk;
>>          u32 max_clk;
>>          int ret;
>> +       bool enable_vqmmc = false;
>>
>>          WARN_ON(host == NULL);
>>          if (host == NULL)
>> @@ -3629,9 +3630,12 @@ int sdhci_setup_host(struct sdhci_host *host)
>>           * the host can take the appropriate action if regulators are not
>>           * available.
>>           */
>> -       ret = mmc_regulator_get_supply(mmc);
>> -       if (ret)
>> -               return ret;
>> +       if (!mmc->supply.vqmmc) {
>> +               ret = mmc_regulator_get_supply(mmc);
>> +               if (ret)
>> +                       return ret;
>> +               enable_vqmmc  = true;
>> +       }
> Did you not like my suggestion in the previous patch of changing
> mmc_regulator_get_supply to only get supplies that were not set
> previously? What you have here is almost the same, except you've got
> this weird coupling where if vqmmc is already present, you implicitly
> skip looking at vmmc entirely (since mmc_regulator_get_supply does
> more than just get vqmmc).

sure. Will update mmc_regulator_get_supply() as you suggested.

>>          DBG("Version:   0x%08x | Present:  0x%08x\n",
>>              sdhci_readw(host, SDHCI_HOST_VERSION),
>> @@ -3880,7 +3884,15 @@ int sdhci_setup_host(struct sdhci_host *host)
>>                  mmc->caps |= MMC_CAP_NEEDS_POLL;
>>
>>          if (!IS_ERR(mmc->supply.vqmmc)) {
>> -               ret = regulator_enable(mmc->supply.vqmmc);
>> +               if (enable_vqmmc) {
>> +                       ret = regulator_enable(mmc->supply.vqmmc);
>> +                       if (ret) {
>> +                               pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
>> +                                       mmc_hostname(mmc), ret);
>> +                               mmc->supply.vqmmc = ERR_PTR(-EINVAL);
>> +                       }
>> +                       host->vqmmc_enabled = !ret;
>> +               }
>>
>>                  /* If vqmmc provides no 1.8V signalling, then there's no UHS */
>>                  if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
>> @@ -3893,12 +3905,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>>                  if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000,
>>                                                      3600000))
>>                          host->flags &= ~SDHCI_SIGNALING_330;
>> -
>> -               if (ret) {
>> -                       pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
>> -                               mmc_hostname(mmc), ret);
>> -                       mmc->supply.vqmmc = ERR_PTR(-EINVAL);
>> -               }
>>          }
>>
>>          if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
>> @@ -4136,7 +4142,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>>          return 0;
>>
>>   unreg:
>> -       if (!IS_ERR(mmc->supply.vqmmc))
>> +       if (host->vqmmc_enabled)
>>                  regulator_disable(mmc->supply.vqmmc);
>>   undma:
>>          if (host->align_buffer)
>> @@ -4154,7 +4160,7 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>>   {
>>          struct mmc_host *mmc = host->mmc;
>>
>> -       if (!IS_ERR(mmc->supply.vqmmc))
>> +       if (host->vqmmc_enabled)
>>                  regulator_disable(mmc->supply.vqmmc);
>>
>>          if (host->align_buffer)
>> @@ -4287,7 +4293,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>>
>>          tasklet_kill(&host->finish_tasklet);
>>
>> -       if (!IS_ERR(mmc->supply.vqmmc))
>> +       if (host->vqmmc_enabled)
>>                  regulator_disable(mmc->supply.vqmmc);
>>
>>          if (host->align_buffer)
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index b001cf4..3c28152 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -524,6 +524,7 @@ struct sdhci_host {
>>          bool pending_reset;     /* Cmd/data reset is pending */
>>          bool irq_wake_enabled;  /* IRQ wakeup is enabled */
>>          bool v4_mode;           /* Host Version 4 Enable */
>> +       bool vqmmc_enabled;     /* Vqmmc is enabled */
> I still don't love this, since it doesn't mean what it says. Everyone
> else that has a vqmmc_enabled member uses it to actually mean "vqmmc
> is enabled", but this doesn't mean that. For example, you don't clear
> this when you disable the regulator in patch 3, so this would be set
> even if the regulator is disabled, and you don't set it when sdhci
> enables the regulator, so the regulator is on when this flag is not
> set.
>
> At the very least, I think a rename is in order. I'm struggling with
> the rename, since
> "vqmmc_pltfrm_enable_but_feel_free_to_call_set_voltage_anywhere" is
> too long. I guess vqmmc_pltfrm_enabled is the best I've got for now.

Sure. Will rename it.

>>          struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];  /* Requests done */
>>          struct mmc_command *cmd;        /* Current command */
>> --
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>>

Veerabhadrarao Badiganti Nov. 14, 2018, 2:36 p.m. UTC | #3

On 11/12/2018 10:49 PM, Veerabhadrarao Badiganti wrote:
>
> On 10/17/2018 3:39 AM, Evan Green wrote:
>> On Mon, Oct 8, 2018 at 6:22 AM Veerabhadrarao Badiganti
>> <[e-mail]> wrote:
>>> From: Vijay Viswanath <[e-mail]>
>>>
>>> Some controllers can have internal mechanism to inform the SW that it
>>> is ready for voltage switching. For such controllers, changing voltage
>>> before the HW is ready can result in various issues.
>>>
>>> During setup/cleanup of host, check whether regulator enable/disable
>>> was already done by platform driver.
>>>
>>> Signed-off-by: Vijay Viswanath <[e-mail]>
>>> Signed-off-by: Veerabhadrarao Badiganti <[e-mail]>
>>> ---
>>>   drivers/mmc/host/sdhci.c | 32 +++++++++++++++++++-------------
>>>   drivers/mmc/host/sdhci.h |  1 +
>>>   2 files changed, 20 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 99bdae5..ea7ce1d 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -3616,6 +3616,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>          unsigned int override_timeout_clk;
>>>          u32 max_clk;
>>>          int ret;
>>> +       bool enable_vqmmc = false;
>>>
>>>          WARN_ON(host == NULL);
>>>          if (host == NULL)
>>> @@ -3629,9 +3630,12 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>           * the host can take the appropriate action if regulators 
>>> are not
>>>           * available.
>>>           */
>>> -       ret = mmc_regulator_get_supply(mmc);
>>> -       if (ret)
>>> -               return ret;
>>> +       if (!mmc->supply.vqmmc) {
>>> +               ret = mmc_regulator_get_supply(mmc);
>>> +               if (ret)
>>> +                       return ret;
>>> +               enable_vqmmc  = true;
>>> +       }
>> Did you not like my suggestion in the previous patch of changing
>> mmc_regulator_get_supply to only get supplies that were not set
>> previously? What you have here is almost the same, except you've got
>> this weird coupling where if vqmmc is already present, you implicitly
>> skip looking at vmmc entirely (since mmc_regulator_get_supply does
>> more than just get vqmmc).
>
> sure. Will update mmc_regulator_get_supply() as you suggested.
>
>>>          DBG("Version:   0x%08x | Present:  0x%08x\n",
>>>              sdhci_readw(host, SDHCI_HOST_VERSION),
>>> @@ -3880,7 +3884,15 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>                  mmc->caps |= MMC_CAP_NEEDS_POLL;
>>>
>>>          if (!IS_ERR(mmc->supply.vqmmc)) {
>>> -               ret = regulator_enable(mmc->supply.vqmmc);
>>> +               if (enable_vqmmc) {
>>> +                       ret = regulator_enable(mmc->supply.vqmmc);
>>> +                       if (ret) {
>>> +                               pr_warn("%s: Failed to enable vqmmc 
>>> regulator: %d\n",
>>> +                                       mmc_hostname(mmc), ret);
>>> +                               mmc->supply.vqmmc = ERR_PTR(-EINVAL);
>>> +                       }
>>> +                       host->vqmmc_enabled = !ret;
>>> +               }
>>>
>>>                  /* If vqmmc provides no 1.8V signalling, then 
>>> there's no UHS */
>>>                  if 
>>> (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
>>> @@ -3893,12 +3905,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>                  if 
>>> (!regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000,
>>>                                                      3600000))
>>>                          host->flags &= ~SDHCI_SIGNALING_330;
>>> -
>>> -               if (ret) {
>>> -                       pr_warn("%s: Failed to enable vqmmc 
>>> regulator: %d\n",
>>> -                               mmc_hostname(mmc), ret);
>>> -                       mmc->supply.vqmmc = ERR_PTR(-EINVAL);
>>> -               }
>>>          }
>>>
>>>          if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
>>> @@ -4136,7 +4142,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>          return 0;
>>>
>>>   unreg:
>>> -       if (!IS_ERR(mmc->supply.vqmmc))
>>> +       if (host->vqmmc_enabled)
>>>                  regulator_disable(mmc->supply.vqmmc);
>>>   undma:
>>>          if (host->align_buffer)
>>> @@ -4154,7 +4160,7 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>>>   {
>>>          struct mmc_host *mmc = host->mmc;
>>>
>>> -       if (!IS_ERR(mmc->supply.vqmmc))
>>> +       if (host->vqmmc_enabled)
>>>                  regulator_disable(mmc->supply.vqmmc);
>>>
>>>          if (host->align_buffer)
>>> @@ -4287,7 +4293,7 @@ void sdhci_remove_host(struct sdhci_host 
>>> *host, int dead)
>>>
>>>          tasklet_kill(&host->finish_tasklet);
>>>
>>> -       if (!IS_ERR(mmc->supply.vqmmc))
>>> +       if (host->vqmmc_enabled)
>>>                  regulator_disable(mmc->supply.vqmmc);
>>>
>>>          if (host->align_buffer)
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index b001cf4..3c28152 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -524,6 +524,7 @@ struct sdhci_host {
>>>          bool pending_reset;     /* Cmd/data reset is pending */
>>>          bool irq_wake_enabled;  /* IRQ wakeup is enabled */
>>>          bool v4_mode;           /* Host Version 4 Enable */
>>> +       bool vqmmc_enabled;     /* Vqmmc is enabled */
>> I still don't love this, since it doesn't mean what it says. Everyone
>> else that has a vqmmc_enabled member uses it to actually mean "vqmmc
>> is enabled", but this doesn't mean that. For example, you don't clear
>> this when you disable the regulator in patch 3, so this would be set
>> even if the regulator is disabled, and you don't set it when sdhci
>> enables the regulator, so the regulator is on when this flag is not
>> set.
>>
Hi Evan

This flag is meant to say "disable vqmmc *only* if it is enabled by host 
driver (sdhci_host)".
If host driver doesn't enable vqmmc (enabled by platfrm driver) or if it 
fails to enable it, then don't call disable vqmmc.

Agree with you, the present name is not conveying its purpose.
It must be something like "vqmmc_enabled_by_host".

Please let me know if you have any suggestions on this name.


>> At the very least, I think a rename is in order. I'm struggling with
>> the rename, since
>> "vqmmc_pltfrm_enable_but_feel_free_to_call_set_voltage_anywhere" is
>> too long. I guess vqmmc_pltfrm_enabled is the best I've got for now.
>
> Sure. Will rename it.
>
>>>          struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];  /* Requests 
>>> done */
>>>          struct mmc_command *cmd;        /* Current command */
>>> -- 
>>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation 
>>> Center, Inc.
>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
>>> Linux Foundation Collaborative Project.
>>>
>

Evan Green Nov. 15, 2018, 11:17 p.m. UTC | #4

On Wed, Nov 14, 2018 at 6:36 AM Veerabhadrarao Badiganti
<[e-mail]> wrote:
>
> >>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> >>> index b001cf4..3c28152 100644
> >>> --- a/drivers/mmc/host/sdhci.h
> >>> +++ b/drivers/mmc/host/sdhci.h
> >>> @@ -524,6 +524,7 @@ struct sdhci_host {
> >>>          bool pending_reset;     /* Cmd/data reset is pending */
> >>>          bool irq_wake_enabled;  /* IRQ wakeup is enabled */
> >>>          bool v4_mode;           /* Host Version 4 Enable */
> >>> +       bool vqmmc_enabled;     /* Vqmmc is enabled */
> >> I still don't love this, since it doesn't mean what it says. Everyone
> >> else that has a vqmmc_enabled member uses it to actually mean "vqmmc
> >> is enabled", but this doesn't mean that. For example, you don't clear
> >> this when you disable the regulator in patch 3, so this would be set
> >> even if the regulator is disabled, and you don't set it when sdhci
> >> enables the regulator, so the regulator is on when this flag is not
> >> set.
> >>
> Hi Evan
>
> This flag is meant to say "disable vqmmc *only* if it is enabled by host
> driver (sdhci_host)".
> If host driver doesn't enable vqmmc (enabled by platfrm driver) or if it
> fails to enable it, then don't call disable vqmmc.
>
> Agree with you, the present name is not conveying its purpose.
> It must be something like "vqmmc_enabled_by_host".
>
> Please let me know if you have any suggestions on this name.

Yeah. Maybe vqmmc_pltfrm_controlled? Or vqmmc_enabled_by_platfrm as
you suggested?
-Evan

Adrian Hunter Nov. 16, 2018, 7:10 a.m. UTC | #5

On 16/11/18 1:17 AM, Evan Green wrote:
> On Wed, Nov 14, 2018 at 6:36 AM Veerabhadrarao Badiganti
> <[e-mail]> wrote:
>>
>>>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>>>> index b001cf4..3c28152 100644
>>>>> --- a/drivers/mmc/host/sdhci.h
>>>>> +++ b/drivers/mmc/host/sdhci.h
>>>>> @@ -524,6 +524,7 @@ struct sdhci_host {
>>>>>          bool pending_reset;     /* Cmd/data reset is pending */
>>>>>          bool irq_wake_enabled;  /* IRQ wakeup is enabled */
>>>>>          bool v4_mode;           /* Host Version 4 Enable */
>>>>> +       bool vqmmc_enabled;     /* Vqmmc is enabled */
>>>> I still don't love this, since it doesn't mean what it says. Everyone
>>>> else that has a vqmmc_enabled member uses it to actually mean "vqmmc
>>>> is enabled", but this doesn't mean that. For example, you don't clear
>>>> this when you disable the regulator in patch 3, so this would be set
>>>> even if the regulator is disabled, and you don't set it when sdhci
>>>> enables the regulator, so the regulator is on when this flag is not
>>>> set.
>>>>
>> Hi Evan
>>
>> This flag is meant to say "disable vqmmc *only* if it is enabled by host
>> driver (sdhci_host)".
>> If host driver doesn't enable vqmmc (enabled by platfrm driver) or if it
>> fails to enable it, then don't call disable vqmmc.
>>
>> Agree with you, the present name is not conveying its purpose.
>> It must be something like "vqmmc_enabled_by_host".
>>
>> Please let me know if you have any suggestions on this name.
> 
> Yeah. Maybe vqmmc_pltfrm_controlled? Or vqmmc_enabled_by_platfrm as
> you suggested?

"pltfrm" doesn't mean anything here.  Just change the comment "vqmmc enabled
in sdhci.c"

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 99bdae5..ea7ce1d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3616,6 +3616,7 @@  int sdhci_setup_host(struct sdhci_host *host)
 	unsigned int override_timeout_clk;
 	u32 max_clk;
 	int ret;
+	bool enable_vqmmc = false;
 
 	WARN_ON(host == NULL);
 	if (host == NULL)
@@ -3629,9 +3630,12 @@  int sdhci_setup_host(struct sdhci_host *host)
 	 * the host can take the appropriate action if regulators are not
 	 * available.
 	 */
-	ret = mmc_regulator_get_supply(mmc);
-	if (ret)
-		return ret;
+	if (!mmc->supply.vqmmc) {
+		ret = mmc_regulator_get_supply(mmc);
+		if (ret)
+			return ret;
+		enable_vqmmc  = true;
+	}
 
 	DBG("Version:   0x%08x | Present:  0x%08x\n",
 	    sdhci_readw(host, SDHCI_HOST_VERSION),
@@ -3880,7 +3884,15 @@  int sdhci_setup_host(struct sdhci_host *host)
 		mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	if (!IS_ERR(mmc->supply.vqmmc)) {
-		ret = regulator_enable(mmc->supply.vqmmc);
+		if (enable_vqmmc) {
+			ret = regulator_enable(mmc->supply.vqmmc);
+			if (ret) {
+				pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
+					mmc_hostname(mmc), ret);
+				mmc->supply.vqmmc = ERR_PTR(-EINVAL);
+			}
+			host->vqmmc_enabled = !ret;
+		}
 
 		/* If vqmmc provides no 1.8V signalling, then there's no UHS */
 		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
@@ -3893,12 +3905,6 @@  int sdhci_setup_host(struct sdhci_host *host)
 		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000,
 						    3600000))
 			host->flags &= ~SDHCI_SIGNALING_330;
-
-		if (ret) {
-			pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
-				mmc_hostname(mmc), ret);
-			mmc->supply.vqmmc = ERR_PTR(-EINVAL);
-		}
 	}
 
 	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
@@ -4136,7 +4142,7 @@  int sdhci_setup_host(struct sdhci_host *host)
 	return 0;
 
 unreg:
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->vqmmc_enabled)
 		regulator_disable(mmc->supply.vqmmc);
 undma:
 	if (host->align_buffer)
@@ -4154,7 +4160,7 @@  void sdhci_cleanup_host(struct sdhci_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
 
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->vqmmc_enabled)
 		regulator_disable(mmc->supply.vqmmc);
 
 	if (host->align_buffer)
@@ -4287,7 +4293,7 @@  void sdhci_remove_host(struct sdhci_host *host, int dead)
 
 	tasklet_kill(&host->finish_tasklet);
 
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->vqmmc_enabled)
 		regulator_disable(mmc->supply.vqmmc);
 
 	if (host->align_buffer)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index b001cf4..3c28152 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -524,6 +524,7 @@  struct sdhci_host {
 	bool pending_reset;	/* Cmd/data reset is pending */
 	bool irq_wake_enabled;	/* IRQ wakeup is enabled */
 	bool v4_mode;		/* Host Version 4 Enable */
+	bool vqmmc_enabled;	/* Vqmmc is enabled */
 
 	struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];	/* Requests done */
 	struct mmc_command *cmd;	/* Current command */