dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC

PATCH In review Veerabhadrarao Badiganti 4 participants 10 messages Nov 1, 2018 – Nov 6, 2018 1 message not in the archive

Subsystems (4)

Device tree bindings Arm Qualcomm mailing list MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Veerabhadrarao Badiganti

author

2018-11-01 12:06 UTC #1 lore raw

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

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

Signed-off-by Veerabhadrarao Badiganti <[email protected]>

---
 Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 +
 1 file changed, 1 insertion(+)

Documentation/devicetree/bindings/mmc/sdhci-msm.txt +1 -0

index 502b3b8..f2ffbeb 100644
@@ -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".

Signature · 2 lines

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.

Doug Anderson

2018-11-01 20:29 UTC #2 lore raw

Hi,

Quoted 13 lines · Veerabhadrarao Badiganti

For SDM845 SOC, new compatible string "qcom,sdm845-sdhci" is added.
Signed-off-by Veerabhadrarao Badiganti <[email protected]>
---
 Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 +
 1 file changed, 1 insertion(+)
Documentation/devicetree/bindings/mmc/sdhci-msm.txt +0 -0
index 502b3b8..f2ffbeb 100644

Documentation/devicetree/bindings/mmc/sdhci-msm.txt Required properties: · line 10

Quoted 4 lines

Documentation/devicetree/bindings/mmc/sdhci-msm.txt +1 -0

@@ -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

Rob Herring

maintainer

2018-11-05 20:36 UTC #3 lore raw

Quoted 12 lines · Doug Anderson

Hi,
Quoted 8 lines · Veerabhadrarao Badiganti
For SDM845 SOC, new compatible string "qcom,sdm845-sdhci" is added.
Signed-off-by Veerabhadrarao Badiganti <[email protected]>
---
 Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 +
 1 file changed, 1 insertion(+)

Quoted 13 lines

Documentation/devicetree/bindings/mmc/sdhci-msm.txt Required properties: · line 10
Quoted 9 lines
Documentation/devicetree/bindings/mmc/sdhci-msm.txt +1 -0
index 502b3b8..f2ffbeb 100644
@@ -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.

Quoted 11 lines

- 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?

Quoted 7 lines

           "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

Veerabhadrarao Badiganti

author

2018-11-06 17:59 UTC #5 lore raw

Quoted 44 lines · Doug Anderson

Hi,
Quoted 24 lines · Rob Herring
Quoted 11 lines · Doug Anderson
Hi,
Quoted 7 lines · Veerabhadrarao Badiganti
For SDM845 SOC, new compatible string "qcom,sdm845-sdhci" is added.
Signed-off-by Veerabhadrarao Badiganti <[email protected]>
---
  Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 +
  1 file changed, 1 insertion(+)
Quoted 12 lines
Documentation/devicetree/bindings/mmc/sdhci-msm.txt Required properties: · line 10
Quoted 9 lines
Documentation/devicetree/bindings/mmc/sdhci-msm.txt +1 -0
index 502b3b8..f2ffbeb 100644
@@ -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:
Quoted 1 line
Fine by me if you update all the dts files.
Done and done.
https://lkml.kernel.org/r/[email protected]
https://lkml.kernel.org/r/[email protected]
Quoted 10 lines
Quoted 10 lines
- 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"

Quoted 8 lines

Quoted 1 line

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.

Quoted 1 line

-Doug

Veerabhadrarao Badiganti

author

2018-11-01 12:08 UTC #6 lore raw

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

In reply to a message not in the archive

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 <[email protected]>

---
 drivers/mmc/host/sdhci-msm.c | 59 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

drivers/mmc/host/sdhci-msm.c +57 -2

index 3cc8bfe..e38a4e8 100644
@@ -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

Signature · 2 lines

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

Reviewed-by on v3 2/2

2018-11-01 17:11 UTC #7 lore raw

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

drivers/mmc/host/sdhci-msm.c

Quoted 13 lines · 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 <[email protected]>
---
 drivers/mmc/host/sdhci-msm.c | 59 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci-msm.c +0 -0

Reviewed-by Evan Green <[email protected]>

Doug Anderson

2018-11-01 20:16 UTC #8 lore raw

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

In reply to Veerabhadrarao Badiganti

Hi,

Quoted 33 lines · 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 <[email protected]>
---
 drivers/mmc/host/sdhci-msm.c | 59 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci-msm.c +2 -0
index 3cc8bfe..e38a4e8 100644
@@ -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;
 };

drivers/mmc/host/sdhci-msm.c static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host) · line 1032

Quoted 6 lines

drivers/mmc/host/sdhci-msm.c +3 -0

@@ -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;

Quoted 4 lines

drivers/mmc/host/sdhci-msm.c +4 -0

+       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+       int ret;
+

drivers/mmc/host/sdhci-msm.c static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host) · line 1046

Quoted 10 lines

drivers/mmc/host/sdhci-msm.c +10 -0

+       /*
+        * 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.

Quoted 33 lines

drivers/mmc/host/sdhci-msm.c +14 -1

+               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 {

drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1652

Quoted 4 lines

drivers/mmc/host/sdhci-msm.c +1 -0

@@ -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.

drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1659

Quoted 7 lines

drivers/mmc/host/sdhci-msm.c +1 -0

        .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...

Quoted 27 lines

drivers/mmc/host/sdhci-msm.c +9 -0

+       .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;

drivers/mmc/host/sdhci-msm.c static int sdhci_msm_runtime_resume(struct device *dev) · line 1979

Quoted 10 lines

drivers/mmc/host/sdhci-msm.c +4 -1

@@ -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;

drivers/mmc/host/sdhci-msm.c static int sdhci_msm_runtime_resume(struct device *dev) · line 1988

Quoted 9 lines

drivers/mmc/host/sdhci-msm.c +9 -0

+       /*
+        * 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;

Doug Anderson

2018-11-01 20:33 UTC #9 lore raw

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

In reply to Veerabhadrarao Badiganti

Hi,

drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1669

Quoted 13 lines · Veerabhadrarao Badiganti

static const struct sdhci_msm_variant_info sdhci_msm_v5_var = {
.mci_removed = true,

drivers/mmc/host/sdhci-msm.c +8 -0

+       .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

Veerabhadrarao Badiganti

author

2018-11-06 17:51 UTC #10 lore raw

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

Quoted 17 lines · Doug Anderson

Hi,

drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1669

Quoted 13 lines · Veerabhadrarao Badiganti

  static const struct sdhci_msm_variant_info sdhci_msm_v5_var = {
         .mci_removed = true,

drivers/mmc/host/sdhci-msm.c +8 -0

+       .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,
  };

Quoted 9 lines

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.

Quoted 1 line

-Doug

Thanks,
Veera