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: <1541073999-11752-1-git-send-email-vbadigan@codeaurora.org> <1541073999-11752-3-git-send-email-vbadigan@codeaurora.org>
In-Reply-To: <1541073999-11752-3-git-send-email-vbadigan@codeaurora.org>
From:   Doug Anderson <dianders@google.com>
Date:   Thu, 1 Nov 2018 13:16:37 -0700
Message-ID: <CAD=FV=XbCGQ2v1EAGOxiY_7PVH0WmOy0MMnUtKi9TkJYmhe4=A@mail.gmail.com>
Subject: Re: [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically
To:     vbadigan@codeaurora.org
Cc:     Adrian Hunter <adrian.hunter@intel.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Evan Green <evgreen@chromium.org>,
        Asutosh Das <asutoshd@codeaurora.org>, riteshh@codeaurora.org,
        stummala@codeaurora.org, sayalil@codeaurora.org,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
>
> On few SDHCI-MSM controllers, the host controller's clock tuning
> circuit may go out of sync if controller clocks are gated which
> eventually will result in data CRC, command CRC/timeout errors.
> To overcome this h/w limitation, the DLL needs to be re-initialized
> and restored with its old settings once clocks are ungated.
>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 59 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 3cc8bfe..e38a4e8 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -232,6 +232,7 @@ struct sdhci_msm_variant_ops {
>   */
>  struct sdhci_msm_variant_info {
>         bool mci_removed;
> +       bool restore_dll_config;
>         const struct sdhci_msm_variant_ops *var_ops;
>         const struct sdhci_msm_offset *offset;
>  };
> @@ -256,6 +257,7 @@ struct sdhci_msm_host {
>         bool pwr_irq_flag;
>         u32 caps_0;
>         bool mci_removed;
> +       bool restore_dll_config;
>         const struct sdhci_msm_variant_ops *var_ops;
>         const struct sdhci_msm_offset *offset;
>  };
> @@ -1025,6 +1027,36 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>         return ret;
>  }
>
> +static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
> +{
> +       struct mmc_ios ios = host->mmc->ios;

Please use a pointer.  Right now you're copying the whole structure
onto the stack.  Sure it's maybe only 20 bytes, but why?

...for bonus points add a separate patch that fixes all the other
places in this file that do the same thing.  A grep can see that msm
is the only one that does this, everyone else uses a pointer:

$ git grep 'struct mmc_ios.*=' -- drivers/mmc/host
drivers/mmc/host/omap_hsmmc.c:  struct mmc_ios *ios = &mmc->ios;
drivers/mmc/host/omap_hsmmc.c:  struct mmc_ios *ios = &host->mmc->ios;
drivers/mmc/host/omap_hsmmc.c:  struct mmc_ios *ios = &host->mmc->ios;
drivers/mmc/host/omap_hsmmc.c:  struct mmc_ios *ios = &host->mmc->ios;
drivers/mmc/host/omap_hsmmc.c:  struct mmc_ios *ios = &host->mmc->ios;
drivers/mmc/host/sdhci-msm.c:   struct mmc_ios ios = host->mmc->ios;
drivers/mmc/host/sdhci-msm.c:   struct mmc_ios curr_ios = host->mmc->ios;
drivers/mmc/host/sdhci-msm.c:   struct mmc_ios ios = host->mmc->ios;
drivers/mmc/host/sdhci-msm.c:   struct mmc_ios ios = host->mmc->ios;
drivers/mmc/host/sdhci-msm.c:   struct mmc_ios ios = host->mmc->ios;
drivers/mmc/host/sdhci-msm.c:   struct mmc_ios ios = host->mmc->ios;
drivers/mmc/host/sdhci-omap.c:  struct mmc_ios *ios = &mmc->ios;
drivers/mmc/host/sdhci.c:       struct mmc_ios *ios = &mmc->ios;


> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +       int ret;
> +
> +       /*
> +        * SDR DLL comes into picure only if clock frequency is greater than
> +        * 100MHz. And its needed only for SDR104, HS200 and HS400 cards.
> +        * Its not needed for HS400es cards.
> +        */
> +       if (host->clock <= CORE_FREQ_100MHZ ||
> +           !(ios.timing == MMC_TIMING_MMC_HS400 ||
> +           ios.timing == MMC_TIMING_MMC_HS200 ||
> +           ios.timing == MMC_TIMING_UHS_SDR104) ||
> +           ios.enhanced_strobe)

This if test is nearly copied from sdhci_msm_execute_tuning().  My
first thought it that this is complicated enough logic that you should
write a helper.

...looking at this is makes me feel like there's a bug in
sdhci_msm_execute_tuning() because it doesn't check for
"enhanced_strobe".  ...but maybe the answer is that the test in
sdhci_msm_execute_tuning() is pointless anyway because the core code
should only call execute_tuning() if we're using a mode that needs
tuning.  So maybe the answer is to remove the code from
sdhci_msm_execute_tuning()...

...but thinking even more: do we really need this logic?  Basically
you want to invalidate the saved dll config whenever the timing mode
changes, right?  ...because execute_tuning() won't be called if you go
down to a timing change that doesn't use tuning?  Maybe you can just
save the timing mode at the same time you save the phase.  If the
current timing mode doesn't equal the saved timing mode then you don't
restore the phase.


> +               return 0;
> +
> +       /* Reset the tuning block */
> +       ret = msm_init_cm_dll(host);
> +       if (ret)
> +               return ret;
> +
> +       /* Restore the tuning block */
> +       ret = msm_config_cm_dll_phase(host, msm_host->saved_tuning_phase);
> +
> +       return ret;
> +}
> +
>  static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  {
>         struct sdhci_host *host = mmc_priv(mmc);
> @@ -1069,7 +1101,6 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>                 if (rc)
>                         return rc;
>
> -               msm_host->saved_tuning_phase = phase;
>                 rc = mmc_send_tuning(mmc, opcode, NULL);
>                 if (!rc) {
>                         /* Tuning is successful at this tuning point */
> @@ -1094,6 +1125,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>                 rc = msm_config_cm_dll_phase(host, phase);
>                 if (rc)
>                         return rc;
> +               msm_host->saved_tuning_phase = phase;
>                 dev_dbg(mmc_dev(mmc), "%s: Setting the tuning phase to %d\n",
>                          mmc_hostname(mmc), phase);
>         } else {
> @@ -1617,12 +1649,21 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>
>  static const struct sdhci_msm_variant_info sdhci_msm_mci_var = {
>         .mci_removed = false,
> +       .restore_dll_config = false,

nit: Linux convention is to rely on static structures to be initted to
0 / false / NULL.  ...so no need to add an "= false" here.


>         .var_ops = &mci_var_ops,
>         .offset = &sdhci_msm_mci_offset,
>  };
>
>  static const struct sdhci_msm_variant_info sdhci_msm_v5_var = {
>         .mci_removed = true,
> +       .restore_dll_config = false,

...and here...


> +       .var_ops = &v5_var_ops,
> +       .offset = &sdhci_msm_v5_offset,
> +};
> +
> +static const struct sdhci_msm_variant_info sdm845_sdhci_var = {
> +       .mci_removed = true,
> +       .restore_dll_config = true,
>         .var_ops = &v5_var_ops,
>         .offset = &sdhci_msm_v5_offset,
>  };
> @@ -1630,6 +1671,7 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>  static const struct of_device_id sdhci_msm_dt_match[] = {
>         {.compatible = "qcom,sdhci-msm-v4", .data = &sdhci_msm_mci_var},
>         {.compatible = "qcom,sdhci-msm-v5", .data = &sdhci_msm_v5_var},
> +       {.compatible = "qcom,sdm845-sdhci", .data = &sdm845_sdhci_var},
>         {},
>  };
>
> @@ -1689,6 +1731,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>         var_info = of_device_get_match_data(&pdev->dev);
>
>         msm_host->mci_removed = var_info->mci_removed;
> +       msm_host->restore_dll_config = var_info->restore_dll_config;
>         msm_host->var_ops = var_info->var_ops;
>         msm_host->offset = var_info->offset;
>
> @@ -1928,9 +1971,21 @@ static int sdhci_msm_runtime_resume(struct device *dev)
>         struct sdhci_host *host = dev_get_drvdata(dev);
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +       int ret;
>
> -       return clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks),
> +       ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks),
>                                        msm_host->bulk_clks);
> +       if (ret)
> +               goto out;

no need for a goto since you're not undoing anything.  Just:

if (ret)
  return ret;


> +       /*
> +        * Whenever core-clock is gated dynamically, it's needed to
> +        * restore the SDR DLL settings when the clock is ungated.
> +        */
> +       if (msm_host->restore_dll_config && msm_host->clk_rate)
> +               ret = sdhci_msm_restore_sdr_dll_config(host);
> +
> +out:
> +       return ret;

No need for 'ret' variable:

if (msm_host->restore_dll_config && msm_host->clk_rate)
  return sdhci_msm_restore_sdr_dll_config(host);

return 0;