[PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC

[PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC

[Veerabhadrarao Badiganti]

For SDM845 SOC, new compatible string  "qcom,sdm845-sdhci" is added.

Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
---
 Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index 502b3b8..f2ffbeb 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -7,6 +7,7 @@ Required properties:
 - compatible: Should contain:
 		"qcom,sdhci-msm-v4" for sdcc versions less than 5.0
 		"qcom,sdhci-msm-v5" for sdcc versions >= 5.0
+		"qcom,sdm845-sdhci" for sdm845 SOC
 		For SDCC version 5.0.0, MCI registers are removed from SDCC
 		interface and some registers are moved to HC. New compatible
 		string is added to support this change - "qcom,sdhci-msm-v5".
-- 
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.

[PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically

[Veerabhadrarao Badiganti]

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;
+	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)
+		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,
 	.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,
+	.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;
+	/*
+	 * 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;
 }
 #endif
 
-- 
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.

Re: [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically

[Evan Green]

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

Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically

[Doug Anderson]

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;

Re: [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC

[Doug Anderson]

Hi,

On Thu, Nov 1, 2018 at 5:07 AM Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
>
> For SDM845 SOC, new compatible string  "qcom,sdm845-sdhci" is added.
>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> index 502b3b8..f2ffbeb 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -7,6 +7,7 @@ Required properties:
>  - compatible: Should contain:
>                 "qcom,sdhci-msm-v4" for sdcc versions less than 5.0
>                 "qcom,sdhci-msm-v5" for sdcc versions >= 5.0
> +               "qcom,sdm845-sdhci" for sdm845 SOC

It's up to Rob of course, but IMO it seems a nicer way forward to
include both the SoC-specific string and the "version" string in all
cases.  I'd write this for the full text:

 - compatible: Should contain a SoC-specific string and a IP version string:
      version strings:
           "qcom,sdhci-msm-v4" for sdcc versions less than 5.0
           "qcom,sdhci-msm-v5" for sdcc version 5.0
      full compatible strings with SoC and version:
           "qcom,apq8084", "qcom,sdhci-msm-v4"
           "qcom,msm8974", "qcom,sdhci-msm-v4"
           "qcom,msm8916", "qcom,sdhci-msm-v4"
           "qcom,msm8992", "qcom,sdhci-msm-v4"
           "qcom,msm8996", "qcom,sdhci-msm-v4"
           "qcom,sdm845-sdhci", "qcom,sdhci-msm-v5"

   NOTE that some old device tree files may be floating around that only
   have the string "qcom,sdhci-msm-v4" without the SoC compatible string
   but doing that should be considered a deprecated practice.

-Doug

Re: [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically

[Doug Anderson]

Hi,

On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
>  static const struct sdhci_msm_variant_info sdhci_msm_v5_var = {
>         .mci_removed = true,
> +       .restore_dll_config = false,
> +       .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,
>  };

One last thing: are there actually any "v5" controllers that _don't_
require restoring the DLL?  Since "sdm845" is currently the only v5
controller maybe just set "restore_dll_config = true" for all v5
controllers and when there's a new v5 controller that doesn't need it
then match off the SoC-specific compatible string.  As per my review
of the bindings patch IMO you should include both the "v5" and the
SoC-specific string for SDM845 (and all future SoCs) so you could make
the generic v5 case do this...


-Doug

Re: [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC

[Rob Herring]

On Thu, Nov 01, 2018 at 01:29:54PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Nov 1, 2018 at 5:07 AM Veerabhadrarao Badiganti
> <vbadigan@codeaurora.org> wrote:
> >
> > For SDM845 SOC, new compatible string  "qcom,sdm845-sdhci" is added.
> >
> > Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> > ---
> >  Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> > index 502b3b8..f2ffbeb 100644
> > --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> > +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> > @@ -7,6 +7,7 @@ Required properties:
> >  - compatible: Should contain:
> >                 "qcom,sdhci-msm-v4" for sdcc versions less than 5.0
> >                 "qcom,sdhci-msm-v5" for sdcc versions >= 5.0
> > +               "qcom,sdm845-sdhci" for sdm845 SOC
> 
> It's up to Rob of course, but IMO it seems a nicer way forward to
> include both the SoC-specific string and the "version" string in all
> cases.  I'd write this for the full text:

Fine by me if you update all the dts files.

> 
>  - compatible: Should contain a SoC-specific string and a IP version string:
>       version strings:
>            "qcom,sdhci-msm-v4" for sdcc versions less than 5.0
>            "qcom,sdhci-msm-v5" for sdcc version 5.0
>       full compatible strings with SoC and version:
>            "qcom,apq8084", "qcom,sdhci-msm-v4"
>            "qcom,msm8974", "qcom,sdhci-msm-v4"
>            "qcom,msm8916", "qcom,sdhci-msm-v4"
>            "qcom,msm8992", "qcom,sdhci-msm-v4"
>            "qcom,msm8996", "qcom,sdhci-msm-v4"

I assume you meant to append '-sdhci' here?

>            "qcom,sdm845-sdhci", "qcom,sdhci-msm-v5"
> 
>    NOTE that some old device tree files may be floating around that only
>    have the string "qcom,sdhci-msm-v4" without the SoC compatible string
>    but doing that should be considered a deprecated practice.
> 
> -Doug

Re: [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC

[Doug Anderson]

Hi,

On Mon, Nov 5, 2018 at 12:37 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Nov 01, 2018 at 01:29:54PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Nov 1, 2018 at 5:07 AM Veerabhadrarao Badiganti
> > <vbadigan@codeaurora.org> wrote:
> > >
> > > For SDM845 SOC, new compatible string  "qcom,sdm845-sdhci" is added.
> > >
> > > Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> > > ---
> > >  Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> > > index 502b3b8..f2ffbeb 100644
> > > --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> > > +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> > > @@ -7,6 +7,7 @@ Required properties:
> > >  - compatible: Should contain:
> > >                 "qcom,sdhci-msm-v4" for sdcc versions less than 5.0
> > >                 "qcom,sdhci-msm-v5" for sdcc versions >= 5.0
> > > +               "qcom,sdm845-sdhci" for sdm845 SOC
> >
> > It's up to Rob of course, but IMO it seems a nicer way forward to
> > include both the SoC-specific string and the "version" string in all
> > cases.  I'd write this for the full text:
>
> Fine by me if you update all the dts files.

Done and done.

https://lkml.kernel.org/r/20181105210921.253707-1-dianders@chromium.org
https://lkml.kernel.org/r/20181105210921.253707-2-dianders@chromium.org


> >  - compatible: Should contain a SoC-specific string and a IP version string:
> >       version strings:
> >            "qcom,sdhci-msm-v4" for sdcc versions less than 5.0
> >            "qcom,sdhci-msm-v5" for sdcc version 5.0
> >       full compatible strings with SoC and version:
> >            "qcom,apq8084", "qcom,sdhci-msm-v4"
> >            "qcom,msm8974", "qcom,sdhci-msm-v4"
> >            "qcom,msm8916", "qcom,sdhci-msm-v4"
> >            "qcom,msm8992", "qcom,sdhci-msm-v4"
> >            "qcom,msm8996", "qcom,sdhci-msm-v4"
>
> I assume you meant to append '-sdhci' here?

Oops, yes!

           "qcom,apq8084-sdhci", "qcom,sdhci-msm-v4"
           "qcom,msm8974-sdhci", "qcom,sdhci-msm-v4"
           "qcom,msm8916-sdhci", "qcom,sdhci-msm-v4"
           "qcom,msm8992-sdhci", "qcom,sdhci-msm-v4"
           "qcom,msm8996-sdhci", "qcom,sdhci-msm-v4"

-Doug

Re: [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically

[Veerabhadrarao Badiganti]

On 11/2/2018 2:03 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti
> <vbadigan@codeaurora.org> wrote:
>>   static const struct sdhci_msm_variant_info sdhci_msm_v5_var = {
>>          .mci_removed = true,
>> +       .restore_dll_config = false,
>> +       .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,
>>   };
> One last thing: are there actually any "v5" controllers that _don't_
> require restoring the DLL?  Since "sdm845" is currently the only v5
> controller maybe just set "restore_dll_config = true" for all v5
> controllers and when there's a new v5 controller that doesn't need it
> then match off the SoC-specific compatible string.  As per my review
> of the bindings patch IMO you should include both the "v5" and the
> SoC-specific string for SDM845 (and all future SoCs) so you could make
> the generic v5 case do this...
>

Yes. QCS404 is one of the target which uses "V5" controller and it 
doesn't need restoring of DLL.
I checked your comments on bindings patch. Will update it in the next 
patchset.

> -Doug

Thanks,
Veera

Re: [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC

[Veerabhadrarao Badiganti]

On 11/6/2018 2:41 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Nov 5, 2018 at 12:37 PM Rob Herring <robh@kernel.org> wrote:
>> On Thu, Nov 01, 2018 at 01:29:54PM -0700, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Thu, Nov 1, 2018 at 5:07 AM Veerabhadrarao Badiganti
>>> <vbadigan@codeaurora.org> wrote:
>>>> For SDM845 SOC, new compatible string  "qcom,sdm845-sdhci" is added.
>>>>
>>>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>>>> ---
>>>>   Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>>>> index 502b3b8..f2ffbeb 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>>>> @@ -7,6 +7,7 @@ Required properties:
>>>>   - compatible: Should contain:
>>>>                  "qcom,sdhci-msm-v4" for sdcc versions less than 5.0
>>>>                  "qcom,sdhci-msm-v5" for sdcc versions >= 5.0
>>>> +               "qcom,sdm845-sdhci" for sdm845 SOC
>>> It's up to Rob of course, but IMO it seems a nicer way forward to
>>> include both the SoC-specific string and the "version" string in all
>>> cases.  I'd write this for the full text:
>> Fine by me if you update all the dts files.
> Done and done.
>
> https://lkml.kernel.org/r/20181105210921.253707-1-dianders@chromium.org
> https://lkml.kernel.org/r/20181105210921.253707-2-dianders@chromium.org
>
>
>>>   - compatible: Should contain a SoC-specific string and a IP version string:
>>>        version strings:
>>>             "qcom,sdhci-msm-v4" for sdcc versions less than 5.0
>>>             "qcom,sdhci-msm-v5" for sdcc version 5.0
>>>        full compatible strings with SoC and version:
>>>             "qcom,apq8084", "qcom,sdhci-msm-v4"
>>>             "qcom,msm8974", "qcom,sdhci-msm-v4"
>>>             "qcom,msm8916", "qcom,sdhci-msm-v4"
>>>             "qcom,msm8992", "qcom,sdhci-msm-v4"
>>>             "qcom,msm8996", "qcom,sdhci-msm-v4"
>> I assume you meant to append '-sdhci' here?
> Oops, yes!
>
>             "qcom,apq8084-sdhci", "qcom,sdhci-msm-v4"
>             "qcom,msm8974-sdhci", "qcom,sdhci-msm-v4"
>             "qcom,msm8916-sdhci", "qcom,sdhci-msm-v4"
>             "qcom,msm8992-sdhci", "qcom,sdhci-msm-v4"
>             "qcom,msm8996-sdhci", "qcom,sdhci-msm-v4"

Thank you. Will update the documentation.

> -Doug