[PATCH V2 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for sdcdc10 DLL

[PATCH V2 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for sdcdc10 DLL

[Veerabhadrarao Badiganti]

On the SDHC-MSM controllers which makes use of sdcdc10 variant DLLs,
the DLL configuration needs to be restored whenever controller clocks
are gated. This new compatible string denotes the sdhc-msm controller
variant which uses sdcdc10 DLL.

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

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index 3720385..49b0a43 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -6,7 +6,11 @@ and the properties used by the sdhci-msm driver.
 Required properties:
 - compatible: Should contain:
 		"qcom,sdhci-msm-v4" for sdcc versions less than 5.0
+		"qcom,sdhci-msm-v4-sdcdc10" for sdcc versions < 5.0 and
+		which makes use of sdcdc10 variant DLLs.
 		"qcom,sdhci-msm-v5" for sdcc versions >= 5.0
+		"qcom,sdhci-msm-v5-sdcdc10" for sdcc versions >= 5.0 and
+		which makes use of sdcdc10 variant DLLs.
 		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.

Re: [PATCH V2 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for sdcdc10 DLL

[Rob Herring]

On Mon, Oct 08, 2018 at 06:55:37PM +0530, Veerabhadrarao Badiganti wrote:
> On the SDHC-MSM controllers which makes use of sdcdc10 variant DLLs,
> the DLL configuration needs to be restored whenever controller clocks
> are gated. This new compatible string denotes the sdhc-msm controller
> variant which uses sdcdc10 DLL.

What is sdcdc10 DLL? 

Seems like a work-around to not having SoC specific compatible strings.

> 
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> index 3720385..49b0a43 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -6,7 +6,11 @@ and the properties used by the sdhci-msm driver.
>  Required properties:
>  - compatible: Should contain:
>  		"qcom,sdhci-msm-v4" for sdcc versions less than 5.0
> +		"qcom,sdhci-msm-v4-sdcdc10" for sdcc versions < 5.0 and
> +		which makes use of sdcdc10 variant DLLs.
>  		"qcom,sdhci-msm-v5" for sdcc versions >= 5.0
> +		"qcom,sdhci-msm-v5-sdcdc10" for sdcc versions >= 5.0 and
> +		which makes use of sdcdc10 variant DLLs.
>  		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.
> 

Re: [PATCH V2 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for sdcdc10 DLL

[Veerabhadrarao Badiganti]

On 10/13/2018 2:18 AM, Rob Herring wrote:
> On Mon, Oct 08, 2018 at 06:55:37PM +0530, Veerabhadrarao Badiganti wrote:
>> On the SDHC-MSM controllers which makes use of sdcdc10 variant DLLs,
>> the DLL configuration needs to be restored whenever controller clocks
>> are gated. This new compatible string denotes the sdhc-msm controller
>> variant which uses sdcdc10 DLL.
> What is sdcdc10 DLL?
>
> Seems like a work-around to not having SoC specific compatible strings.

Will update this to SOC specific compatible string.

>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>> ---
>>   Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> index 3720385..49b0a43 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -6,7 +6,11 @@ and the properties used by the sdhci-msm driver.
>>   Required properties:
>>   - compatible: Should contain:
>>   		"qcom,sdhci-msm-v4" for sdcc versions less than 5.0
>> +		"qcom,sdhci-msm-v4-sdcdc10" for sdcc versions < 5.0 and
>> +		which makes use of sdcdc10 variant DLLs.
>>   		"qcom,sdhci-msm-v5" for sdcc versions >= 5.0
>> +		"qcom,sdhci-msm-v5-sdcdc10" for sdcc versions >= 5.0 and
>> +		which makes use of sdcdc10 variant DLLs.
>>   		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.
>>

Thanks
Veera

[PATCH V2 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 | 67 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 6918e70..1eb70c0 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -234,6 +234,7 @@ struct sdhci_msm_variant_ops {
  */
 struct sdhci_msm_variant_info {
 	bool mci_removed;
+	bool uses_sdcdc10;
 	const struct sdhci_msm_variant_ops *var_ops;
 	const struct sdhci_msm_offset *offset;
 };
@@ -264,6 +265,8 @@ struct sdhci_msm_host {
 	u32 vmmc_level[2];
 	bool vqmmc_load;
 	u32 vqmmc_level[2];
+	bool uses_sdcdc10_dll;
+	bool restore_sdr_dll_cfg;
 };
 
 static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
@@ -1031,6 +1034,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);
@@ -1075,7 +1108,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 */
@@ -1100,6 +1132,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 {
@@ -1807,19 +1840,39 @@ static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
 
 static const struct sdhci_msm_variant_info sdhci_msm_mci_var = {
 	.mci_removed = false,
+	.uses_sdcdc10 = false,
+	.var_ops = &mci_var_ops,
+	.offset = &sdhci_msm_mci_offset,
+};
+
+static const struct sdhci_msm_variant_info sdhci_msm_mci_sdcdc10_var = {
+	.mci_removed = false,
+	.uses_sdcdc10 = true,
 	.var_ops = &mci_var_ops,
 	.offset = &sdhci_msm_mci_offset,
 };
 
 static const struct sdhci_msm_variant_info sdhci_msm_v5_var = {
 	.mci_removed = true,
+	.uses_sdcdc10 = false,
+	.var_ops = &v5_var_ops,
+	.offset = &sdhci_msm_v5_offset,
+};
+
+static const struct sdhci_msm_variant_info sdhci_msm_v5_sdcdc10_var = {
+	.mci_removed = true,
+	.uses_sdcdc10 = true,
 	.var_ops = &v5_var_ops,
 	.offset = &sdhci_msm_v5_offset,
 };
 
 static const struct of_device_id sdhci_msm_dt_match[] = {
 	{.compatible = "qcom,sdhci-msm-v4", .data = &sdhci_msm_mci_var},
+	{.compatible = "qcom,sdhci-msm-v4-sdcdc10",
+					    .data = &sdhci_msm_mci_sdcdc10_var},
 	{.compatible = "qcom,sdhci-msm-v5", .data = &sdhci_msm_v5_var},
+	{.compatible = "qcom,sdhci-msm-v5-sdcdc10",
+					    .data = &sdhci_msm_v5_sdcdc10_var},
 	{},
 };
 
@@ -1880,6 +1933,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->uses_sdcdc10_dll = var_info->uses_sdcdc10;
 	msm_host->var_ops = var_info->var_ops;
 	msm_host->offset = var_info->offset;
 
@@ -2124,9 +2178,18 @@ 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);
+	/*
+	 * Whenever core-clock is gated dynamically, it's needed to
+	 * restore the SDR DLL settings when the clock is ungated.
+	 */
+	if (!ret && msm_host->uses_sdcdc10_dll && msm_host->clk_rate)
+		ret = sdhci_msm_restore_sdr_dll_config(host);
+
+	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 V2 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically

[Evan Green]

On Mon, Oct 8, 2018 at 6:26 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 | 67 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 6918e70..1eb70c0 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -234,6 +234,7 @@ struct sdhci_msm_variant_ops {
>   */
>  struct sdhci_msm_variant_info {
>         bool mci_removed;
> +       bool uses_sdcdc10;

What does that flag mean? Couldn't this just be called something like
restore_dll_config?

>         const struct sdhci_msm_variant_ops *var_ops;
>         const struct sdhci_msm_offset *offset;
>  };
> @@ -264,6 +265,8 @@ struct sdhci_msm_host {
>         u32 vmmc_level[2];
>         bool vqmmc_load;
>         u32 vqmmc_level[2];
> +       bool uses_sdcdc10_dll;
> +       bool restore_sdr_dll_cfg;

restore_sdr_dll_cfg appears to not be used.

>  };
>
>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -1031,6 +1034,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);

Is this reinit required? It's kind of meaty.

> +       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);
> @@ -1075,7 +1108,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 */
> @@ -1100,6 +1132,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 {
> @@ -1807,19 +1840,39 @@ static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
>
>  static const struct sdhci_msm_variant_info sdhci_msm_mci_var = {
>         .mci_removed = false,
> +       .uses_sdcdc10 = false,
> +       .var_ops = &mci_var_ops,
> +       .offset = &sdhci_msm_mci_offset,
> +};
> +
> +static const struct sdhci_msm_variant_info sdhci_msm_mci_sdcdc10_var = {
> +       .mci_removed = false,
> +       .uses_sdcdc10 = true,
>         .var_ops = &mci_var_ops,
>         .offset = &sdhci_msm_mci_offset,
>  };
>
>  static const struct sdhci_msm_variant_info sdhci_msm_v5_var = {
>         .mci_removed = true,
> +       .uses_sdcdc10 = false,
> +       .var_ops = &v5_var_ops,
> +       .offset = &sdhci_msm_v5_offset,
> +};
> +
> +static const struct sdhci_msm_variant_info sdhci_msm_v5_sdcdc10_var = {
> +       .mci_removed = true,
> +       .uses_sdcdc10 = true,
>         .var_ops = &v5_var_ops,
>         .offset = &sdhci_msm_v5_offset,
>  };
>
>  static const struct of_device_id sdhci_msm_dt_match[] = {
>         {.compatible = "qcom,sdhci-msm-v4", .data = &sdhci_msm_mci_var},
> +       {.compatible = "qcom,sdhci-msm-v4-sdcdc10",
> +                                           .data = &sdhci_msm_mci_sdcdc10_var},
>         {.compatible = "qcom,sdhci-msm-v5", .data = &sdhci_msm_v5_var},
> +       {.compatible = "qcom,sdhci-msm-v5-sdcdc10",
> +                                           .data = &sdhci_msm_v5_sdcdc10_var},
>         {},
>  };
>
> @@ -1880,6 +1933,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->uses_sdcdc10_dll = var_info->uses_sdcdc10;
>         msm_host->var_ops = var_info->var_ops;
>         msm_host->offset = var_info->offset;
>
> @@ -2124,9 +2178,18 @@ 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);

It might be nicer to just put an if (ret) return ret; here, instead of
rolling it into the next conditional.

> +       /*
> +        * Whenever core-clock is gated dynamically, it's needed to
> +        * restore the SDR DLL settings when the clock is ungated.
> +        */
> +       if (!ret && msm_host->uses_sdcdc10_dll && msm_host->clk_rate)
> +               ret = sdhci_msm_restore_sdr_dll_config(host);
> +
> +       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 V2 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically

[Veerabhadrarao Badiganti]

On 10/17/2018 3:58 AM, Evan Green wrote:
> On Mon, Oct 8, 2018 at 6:26 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 | 67 ++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 6918e70..1eb70c0 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -234,6 +234,7 @@ struct sdhci_msm_variant_ops {
>>    */
>>   struct sdhci_msm_variant_info {
>>          bool mci_removed;
>> +       bool uses_sdcdc10;
> What does that flag mean? Couldn't this just be called something like
> restore_dll_config?

'sdcdc10' is the DLL code-name that is being used in SDHC controller on 
SDM845.
I should have named it as uses_sdcdc10_dll.
I will update the name to be more meaningful.

>
>>          const struct sdhci_msm_variant_ops *var_ops;
>>          const struct sdhci_msm_offset *offset;
>>   };
>> @@ -264,6 +265,8 @@ struct sdhci_msm_host {
>>          u32 vmmc_level[2];
>>          bool vqmmc_load;
>>          u32 vqmmc_level[2];
>> +       bool uses_sdcdc10_dll;
>> +       bool restore_sdr_dll_cfg;
> restore_sdr_dll_cfg appears to not be used.

Yes, this needs to be removed, will remove it. Thanks for pointing.

>>   };
>>
>>   static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>> @@ -1031,6 +1034,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);
> Is this reinit required? It's kind of meaty.

DLL re-initialization is very-much needed.
Its as per DLL h/w team instructions to support sdcdc10 DLL (10nm DLL)

>> +       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);
>> @@ -1075,7 +1108,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 */
>> @@ -1100,6 +1132,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 {
>> @@ -1807,19 +1840,39 @@ static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
>>
>>   static const struct sdhci_msm_variant_info sdhci_msm_mci_var = {
>>          .mci_removed = false,
>> +       .uses_sdcdc10 = false,
>> +       .var_ops = &mci_var_ops,
>> +       .offset = &sdhci_msm_mci_offset,
>> +};
>> +
>> +static const struct sdhci_msm_variant_info sdhci_msm_mci_sdcdc10_var = {
>> +       .mci_removed = false,
>> +       .uses_sdcdc10 = true,
>>          .var_ops = &mci_var_ops,
>>          .offset = &sdhci_msm_mci_offset,
>>   };
>>
>>   static const struct sdhci_msm_variant_info sdhci_msm_v5_var = {
>>          .mci_removed = true,
>> +       .uses_sdcdc10 = false,
>> +       .var_ops = &v5_var_ops,
>> +       .offset = &sdhci_msm_v5_offset,
>> +};
>> +
>> +static const struct sdhci_msm_variant_info sdhci_msm_v5_sdcdc10_var = {
>> +       .mci_removed = true,
>> +       .uses_sdcdc10 = true,
>>          .var_ops = &v5_var_ops,
>>          .offset = &sdhci_msm_v5_offset,
>>   };
>>
>>   static const struct of_device_id sdhci_msm_dt_match[] = {
>>          {.compatible = "qcom,sdhci-msm-v4", .data = &sdhci_msm_mci_var},
>> +       {.compatible = "qcom,sdhci-msm-v4-sdcdc10",
>> +                                           .data = &sdhci_msm_mci_sdcdc10_var},
>>          {.compatible = "qcom,sdhci-msm-v5", .data = &sdhci_msm_v5_var},
>> +       {.compatible = "qcom,sdhci-msm-v5-sdcdc10",
>> +                                           .data = &sdhci_msm_v5_sdcdc10_var},
>>          {},
>>   };
>>
>> @@ -1880,6 +1933,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->uses_sdcdc10_dll = var_info->uses_sdcdc10;
>>          msm_host->var_ops = var_info->var_ops;
>>          msm_host->offset = var_info->offset;
>>
>> @@ -2124,9 +2178,18 @@ 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);
> It might be nicer to just put an if (ret) return ret; here, instead of
> rolling it into the next conditional.

Sure. Will update it.

>
>> +       /*
>> +        * Whenever core-clock is gated dynamically, it's needed to
>> +        * restore the SDR DLL settings when the clock is ungated.
>> +        */
>> +       if (!ret && msm_host->uses_sdcdc10_dll && msm_host->clk_rate)
>> +               ret = sdhci_msm_restore_sdr_dll_config(host);
>> +
>> +       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.
>>

[PATCH V2 0/2] Re-initialize DLL when 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, result in data CRC and command CRC/timeout errors.
To fix this issue, the DLL needs to be re-initialized and restored with
its old settings once clocks are ungated. SDHC cotroller on SDM845 SOC
has this limitation.

Changes since V3:
	- Introduced SoC specific dt compatible string for sdhci-msm
	controller
	- Moved the condition which checks whether tuning is needed or
	not, to a common function.

Changes since V2:
	- Removed an unused variable.
	- Renamed the newly introduced flags to be more meaningful.
	- Added new dt compatible string for SDM845 SOC.

Changes since V1:
	- Replaced dt flag with a dt compatible string to
	indicate SHDCI-MSM controller varients which need this fix.
	- Moved the DLL settings restoration logic from request 
	context to platform driver runtime pm context.

Tested on: sdm845
Veerabhadrarao Badiganti (2):
  dt-bindings: mmc: sdhci-msm: Add SoC-specific compatible strings
  mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically

 .../devicetree/bindings/mmc/sdhci-msm.txt          | 21 ++++--
 drivers/mmc/host/sdhci-msm.c                       | 78 +++++++++++++++++++---
 2 files changed, 84 insertions(+), 15 deletions(-)

-- 
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 V2 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 | 78 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 67 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 3cc8bfe..4cac593 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,48 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
 	return ret;
 }
 
+static bool sdhci_msm_is_tuning_needed(struct sdhci_host *host)
+{
+	struct mmc_ios *ios = &host->mmc->ios;
+
+	/*
+	 * Tuning is required for SDR104, HS200 and HS400 cards and
+	 * if clock frequency is greater than 100MHz in these modes.
+	 */
+	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 false;
+
+	return true;
+}
+
+static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
+{
+	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 picture only for timing modes which needs
+	 * tuning.
+	 */
+	if (!sdhci_msm_is_tuning_needed(host))
+		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);
@@ -1035,14 +1079,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 
-	/*
-	 * Tuning is required for SDR104, HS200 and HS400 cards and
-	 * if clock frequency is greater than 100MHz in these modes.
-	 */
-	if (host->clock <= CORE_FREQ_100MHZ ||
-	    !(ios.timing == MMC_TIMING_MMC_HS400 ||
-	    ios.timing == MMC_TIMING_MMC_HS200 ||
-	    ios.timing == MMC_TIMING_UHS_SDR104))
+	if (!sdhci_msm_is_tuning_needed(host))
 		return 0;
 
 	/*
@@ -1069,7 +1106,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 +1130,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 {
@@ -1616,7 +1653,6 @@ 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,
 	.var_ops = &mci_var_ops,
 	.offset = &sdhci_msm_mci_offset,
 };
@@ -1627,9 +1663,17 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
 	.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,
+};
+
 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 +1733,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 +1973,20 @@ 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)
+		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)
+		return sdhci_msm_restore_sdr_dll_config(host);
+
+	return 0;
 }
 #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 V2 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically

[Veerabhadrarao Badiganti]

Please ignore this patch set. Right patch-set is V5. 
https://patchwork.kernel.org/project/linux-mmc/list/?series=41661

Sorry for the inconvenience.

Thanks

Veera


On 11/12/2018 12:08 PM, Veerabhadrarao Badiganti 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 | 78 +++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 67 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 3cc8bfe..4cac593 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,48 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>   	return ret;
>   }
>   
> +static bool sdhci_msm_is_tuning_needed(struct sdhci_host *host)
> +{
> +	struct mmc_ios *ios = &host->mmc->ios;
> +
> +	/*
> +	 * Tuning is required for SDR104, HS200 and HS400 cards and
> +	 * if clock frequency is greater than 100MHz in these modes.
> +	 */
> +	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 false;
> +
> +	return true;
> +}
> +
> +static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
> +{
> +	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 picture only for timing modes which needs
> +	 * tuning.
> +	 */
> +	if (!sdhci_msm_is_tuning_needed(host))
> +		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);
> @@ -1035,14 +1079,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>   	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>   
> -	/*
> -	 * Tuning is required for SDR104, HS200 and HS400 cards and
> -	 * if clock frequency is greater than 100MHz in these modes.
> -	 */
> -	if (host->clock <= CORE_FREQ_100MHZ ||
> -	    !(ios.timing == MMC_TIMING_MMC_HS400 ||
> -	    ios.timing == MMC_TIMING_MMC_HS200 ||
> -	    ios.timing == MMC_TIMING_UHS_SDR104))
> +	if (!sdhci_msm_is_tuning_needed(host))
>   		return 0;
>   
>   	/*
> @@ -1069,7 +1106,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 +1130,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 {
> @@ -1616,7 +1653,6 @@ 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,
>   	.var_ops = &mci_var_ops,
>   	.offset = &sdhci_msm_mci_offset,
>   };
> @@ -1627,9 +1663,17 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>   	.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,
> +};
> +
>   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 +1733,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 +1973,20 @@ 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)
> +		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)
> +		return sdhci_msm_restore_sdr_dll_config(host);
> +
> +	return 0;
>   }
>   #endif
>