Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1537424558-17989-1-git-send-email-vbadigan@codeaurora.org> <1537424558-17989-2-git-send-email-vbadigan@codeaurora.org>
In-Reply-To: <1537424558-17989-2-git-send-email-vbadigan@codeaurora.org>
From:   Evan Green <evgreen@chromium.org>
Date:   Fri, 21 Sep 2018 13:08:43 -0700
Message-ID: <CAE=gft7k6gsX-xbW1cr+z2SYeHh=zbmNoUxxXOuG9zZz+DYcWQ@mail.gmail.com>
Subject: Re: [PATCH V2 1/3] mmc: sdhci: Allow platform controlled voltage switching
To:     vbadigan@codeaurora.org
Cc:     adrian.hunter@intel.com, Ulf Hansson <ulf.hansson@linaro.org>,
        robh+dt@kernel.org, linux-mmc@vger.kernel.org,
        asutoshd@codeaurora.org, riteshh@codeaurora.org,
        stummala@codeaurora.org, sayali <sayalil@codeaurora.org>,
        Doug Anderson <dianders@google.com>, vviswana@codeaurora.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>