diff mbox series

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

Message ID 1537424558-17989-2-git-send-email-vbadigan@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series Internal voltage control for platform drivers | expand

Commit Message

Veerabhadrarao Badiganti Sept. 20, 2018, 6:22 a.m. UTC 
From: Vijay Viswanath <vviswana@codeaurora.org>

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 <vviswana@codeaurora.org>
Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
---
 drivers/mmc/host/sdhci.c | 22 +++++++++++++++-------
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 16 insertions(+), 7 deletions(-)

Comments

Evan Green Sept. 21, 2018, 8:08 p.m. UTC | #1 
On Wed, Sep 19, 2018 at 11:24 PM Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
>
> From: Vijay Viswanath <vviswana@codeaurora.org>
>
> 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 <vviswana@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci.c | 22 +++++++++++++++-------
>  drivers/mmc/host/sdhci.h |  1 +
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 99bdae5..04b3fd2 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.vmmc) {
> +               ret = mmc_regulator_get_supply(mmc);
> +               if (ret)
> +                       return ret;
> +               enable_vqmmc  = true;

The coupling of logic strikes me as a little bit odd, it's saying if
vmmc is already present, then don't turn on vqmmc. I guess what it's
trying to say is "hands off all the regulators, I've got this". It
might be cleaner to set enable_vqmmc based on whether or not
mmc->supply.vqmmc exists before the get_supply call, rather than
coupling it into whether or not vmmc exists.

Actually, what might be even nicer is to change
mmc_regulator_get_supply to only get supplies that it doesn't already
have. You'd still have your enable_vqmmc local, but the
mmc_regulator_get_supply call would be outside the conditional, and
the logic of "don't enable vqmmc if it existed before" would make more
sense.

> +       }
>
>         DBG("Version:   0x%08x | Present:  0x%08x\n",
>             sdhci_readw(host, SDHCI_HOST_VERSION),
> @@ -3880,7 +3884,11 @@ 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);
> +                       host->vqmmc_enabled = !ret;
> +               } else
> +                       ret = 0;

I think it's preferred that if your "if" had curly braces, then the
else part needs it too. Actually, can you move the if (ret) pr_warn
stuff up and inside of your if statement above? That keeps the logic
together, and then you don't need an else case at all!

>
>                 /* If vqmmc provides no 1.8V signalling, then there's no UHS */
>                 if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
> @@ -4136,7 +4144,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 +4162,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 +4295,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 */

This is kind of unpleasant. It's confused by the fact that other host
controllers have a vqmmc_enabled member, but they use it to mean what
it sounds like, "is vqmmc currently enabled". Here you're really using
it to mean "don't ever disable vqmmc in sdhci, because the platform
code sort of owns vqmmc". It only "sort of" owns it in that sdhci is
still free to call regulator_is_supported_voltage and
mmc_regulator_set_vqmmc, but somehow not regulator_disable. Is there
any way to clean up the semantics here?

>
>         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., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>
Veerabhadrarao Badiganti Oct. 4, 2018, 12:46 p.m. UTC | #2 
Hi Evan,


On 9/22/2018 1:38 AM, Evan Green wrote:
> On Wed, Sep 19, 2018 at 11:24 PM Veerabhadrarao Badiganti
> <vbadigan@codeaurora.org> wrote:
>> From: Vijay Viswanath <vviswana@codeaurora.org>
>>
>> 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 <vviswana@codeaurora.org>
>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci.c | 22 +++++++++++++++-------
>>   drivers/mmc/host/sdhci.h |  1 +
>>   2 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 99bdae5..04b3fd2 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.vmmc) {
>> +               ret = mmc_regulator_get_supply(mmc);
>> +               if (ret)
>> +                       return ret;
>> +               enable_vqmmc  = true;
> The coupling of logic strikes me as a little bit odd, it's saying if
> vmmc is already present, then don't turn on vqmmc. I guess what it's
> trying to say is "hands off all the regulators, I've got this". It
> might be cleaner to set enable_vqmmc based on whether or not
> mmc->supply.vqmmc exists before the get_supply call, rather than
> coupling it into whether or not vmmc exists.
>
> Actually, what might be even nicer is to change
> mmc_regulator_get_supply to only get supplies that it doesn't already
> have. You'd still have your enable_vqmmc local, but the
> mmc_regulator_get_supply call would be outside the conditional, and
> the logic of "don't enable vqmmc if it existed before" would make more
> sense.

Yes, its saying if platform driver has already controlling the regulators,
don't enable/disable them anymore.
Agree with you on the conditional check.  Will update it to Vcmmc 
instead of vmmc.

>> +       }
>>
>>          DBG("Version:   0x%08x | Present:  0x%08x\n",
>>              sdhci_readw(host, SDHCI_HOST_VERSION),
>> @@ -3880,7 +3884,11 @@ 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);
>> +                       host->vqmmc_enabled = !ret;
>> +               } else
>> +                       ret = 0;
> I think it's preferred that if your "if" had curly braces, then the
> else part needs it too. Actually, can you move the if (ret) pr_warn
> stuff up and inside of your if statement above? That keeps the logic
> together, and then you don't need an else case at all!

sure. Will update.

>>                  /* If vqmmc provides no 1.8V signalling, then there's no UHS */
>>                  if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
>> @@ -4136,7 +4144,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 +4162,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 +4295,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 */
> This is kind of unpleasant. It's confused by the fact that other host
> controllers have a vqmmc_enabled member, but they use it to mean what
> it sounds like, "is vqmmc currently enabled". Here you're really using
> it to mean "don't ever disable vqmmc in sdhci, because the platform
> code sort of owns vqmmc". It only "sort of" owns it in that sdhci is
> still free to call regulator_is_supported_voltage and
> mmc_regulator_set_vqmmc, but somehow not regulator_disable. Is there
> any way to clean up the semantics here?

Except the regualtor_enable()/disable() calls other regulator-operations 
are direct
operations. Only enable() & disable() uses a reference couters to track
whether number regualtor_enable() calls matches with number of 
regualtor_disable()
calls or not.

In V1 patch-set this was done without need to this flag but it was suggested
have this flag for ensuring enable/disable counters are in sync.

>>          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., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>>
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 99bdae5..04b3fd2 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.vmmc) {
+		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,11 @@  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);
+			host->vqmmc_enabled = !ret;
+		} else
+			ret = 0;
 
 		/* If vqmmc provides no 1.8V signalling, then there's no UHS */
 		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
@@ -4136,7 +4144,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 +4162,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 +4295,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 */