[PATCH 1/2] dt-bindings: mmc: sdhci-msm: Add flag for restoring dll

[PATCH 1/2] dt-bindings: mmc: sdhci-msm: Add flag for restoring dll

[Veerabhadrarao Badiganti]

The dll settings of SDHC controller needs to be restored whenever
controller clocks are gated. This restoration is needed only on
few SDHCI-MSM controllers. This dt flag indicates whether dll
restoration is needed or not.

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..207ce36 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -31,6 +31,8 @@ Optional properties:
 					BUS_OFF states in power irq. Should be specified in
 					pairs (lpm, hpm), for BUS_OFF and BUS_ON respectively.
 					Units uA.
+- qcom,restore-dll-config - Flag indicates that restoration of dll config after clock gating
+			    is needed on given platform. This wouldn't be needed for every MSM.
 Example:
 
 	sdhc_1: sdhci@f9824900 {
@@ -49,6 +51,8 @@ Example:
 
 		clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>;
 		clock-names = "core", "iface";
+
+		qcom,restore-dll-config;
 	};
 
 	sdhc_2: sdhci@f98a4900 {
-- 
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 1/2] dt-bindings: mmc: sdhci-msm: Add flag for restoring dll

[Evan Green]

On Tue, Sep 25, 2018 at 8:05 AM Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
>
> The dll settings of SDHC controller needs to be restored whenever
> controller clocks are gated. This restoration is needed only on
> few SDHCI-MSM controllers. This dt flag indicates whether dll
> restoration is needed or not.
>
> 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..207ce36 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -31,6 +31,8 @@ Optional properties:
>                                         BUS_OFF states in power irq. Should be specified in
>                                         pairs (lpm, hpm), for BUS_OFF and BUS_ON respectively.
>                                         Units uA.
> +- qcom,restore-dll-config - Flag indicates that restoration of dll config after clock gating
> +                           is needed on given platform. This wouldn't be needed for every MSM.
>  Example:
>
>         sdhc_1: sdhci@f9824900 {
> @@ -49,6 +51,8 @@ Example:
>
>                 clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>;
>                 clock-names = "core", "iface";
> +
> +               qcom,restore-dll-config;

Hi Veera,
I'm not sure this is the best approach. It might be better to key this
behavior off of the compatible string. Actually I'm noticing now that
the binding for this device doesn't include an SoC-specific compatible
string, which I think is now the preferred way (eg
"qcom,sdm845-sdhci", "qcom,sdhci-msm-v5"). I think we should add that
to sdhci-msm.txt, then use the compatible string to enable this
behavior you're adding now.

-Evan

Re: [PATCH 1/2] dt-bindings: mmc: sdhci-msm: Add flag for restoring dll

[Veerabhadrarao Badiganti]

On 9/25/2018 11:08 PM, Evan Green wrote:
> On Tue, Sep 25, 2018 at 8:05 AM Veerabhadrarao Badiganti
> <vbadigan@codeaurora.org> wrote:
>> The dll settings of SDHC controller needs to be restored whenever
>> controller clocks are gated. This restoration is needed only on
>> few SDHCI-MSM controllers. This dt flag indicates whether dll
>> restoration is needed or not.
>>
>> 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..207ce36 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -31,6 +31,8 @@ Optional properties:
>>                                          BUS_OFF states in power irq. Should be specified in
>>                                          pairs (lpm, hpm), for BUS_OFF and BUS_ON respectively.
>>                                          Units uA.
>> +- qcom,restore-dll-config - Flag indicates that restoration of dll config after clock gating
>> +                           is needed on given platform. This wouldn't be needed for every MSM.
>>   Example:
>>
>>          sdhc_1: sdhci@f9824900 {
>> @@ -49,6 +51,8 @@ Example:
>>
>>                  clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>;
>>                  clock-names = "core", "iface";
>> +
>> +               qcom,restore-dll-config;
> Hi Veera,
> I'm not sure this is the best approach. It might be better to key this
> behavior off of the compatible string. Actually I'm noticing now that
> the binding for this device doesn't include an SoC-specific compatible
> string, which I think is now the preferred way (eg
> "qcom,sdm845-sdhci", "qcom,sdhci-msm-v5"). I think we should add that
> to sdhci-msm.txt, then use the compatible string to enable this
> behavior you're adding now.

Thanks Evan. I will update this one.

> -Evan

Thanks,
Veera

[PATCH 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 | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 486462d..4a83850 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -18,6 +18,7 @@
 #include <linux/of_device.h>
 #include <linux/delay.h>
 #include <linux/mmc/mmc.h>
+#include <linux/mmc/host.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/iopoll.h>
@@ -264,6 +265,8 @@ struct sdhci_msm_host {
 	u32 vmmc_level[2];
 	bool vqmmc_load;
 	u32 vqmmc_level[2];
+	bool restore_dll_cfg_needed;
+	bool restore_dll_cfg;
 };
 
 static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
@@ -1068,6 +1071,20 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	if (rc)
 		return rc;
 
+	/*
+	 * If restore dll config flag is set, then just program the DLL with
+	 * last saved tuning phase. If this operation fails, proceed with
+	 * full-fledged tuning.
+	 */
+	if (msm_host->restore_dll_cfg) {
+		rc = msm_config_cm_dll_phase(host,
+				msm_host->saved_tuning_phase);
+		msm_host->restore_dll_cfg = false;
+		if (!rc)
+			return rc;
+
+	}
+
 	phase = 0;
 	do {
 		/* Set the phase in delay line hw block */
@@ -1075,7 +1092,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 +1116,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 {
@@ -2049,6 +2066,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		goto clk_disable;
 	}
 
+	if (device_property_read_bool(&pdev->dev, "qcom,restore-dll-config"))
+		msm_host->restore_dll_cfg_needed = true;
+
 	pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
@@ -2124,6 +2144,15 @@ static int sdhci_msm_runtime_resume(struct device *dev)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 
+	/*
+	 * Whenever core-clock is gated dynamically, it's needed to
+	 * re-initialize the DLL when the clock is ungated.
+	 */
+	if (msm_host->restore_dll_cfg_needed && msm_host->clk_rate) {
+		msm_host->restore_dll_cfg = true;
+		mmc_retune_needed(host->mmc);
+	}
+
 	return clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks),
 				       msm_host->bulk_clks);
 }
-- 
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 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically

[Doug Anderson]

Hi,

On Tue, Sep 25, 2018 at 8:05 AM Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
> +       /*
> +        * Whenever core-clock is gated dynamically, it's needed to
> +        * re-initialize the DLL when the clock is ungated.
> +        */
> +       if (msm_host->restore_dll_cfg_needed && msm_host->clk_rate) {
> +               msm_host->restore_dll_cfg = true;
> +               mmc_retune_needed(host->mmc);

Using the boolean "restore_dll_cfg" to communicate like this seems
really fragile.  I have no basis in fact, but I worry that something
will happen in the meantime that really ought to invalidate the
"saved_tuning_phase" but the boolean will still be set.

Is there a reason you can't just call msm_config_cm_dll_phase()
directly from sdhci_msm_runtime_resume()?  Perhaps after the
clk_bulk_prepare_enable() call below?


-Doug

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

[Veerabhadrarao Badiganti]

Hi Doug,


On 9/26/2018 3:58 AM, Doug Anderson wrote:
> Hi,
>
> On Tue, Sep 25, 2018 at 8:05 AM Veerabhadrarao Badiganti
> <vbadigan@codeaurora.org> wrote:
>> +       /*
>> +        * Whenever core-clock is gated dynamically, it's needed to
>> +        * re-initialize the DLL when the clock is ungated.
>> +        */
>> +       if (msm_host->restore_dll_cfg_needed && msm_host->clk_rate) {
>> +               msm_host->restore_dll_cfg = true;
>> +               mmc_retune_needed(host->mmc);
> Using the boolean "restore_dll_cfg" to communicate like this seems
> really fragile.  I have no basis in fact, but I worry that something
> will happen in the meantime that really ought to invalidate the
> "saved_tuning_phase" but the boolean will still be set.
>
> Is there a reason you can't just call msm_config_cm_dll_phase()
> directly from sdhci_msm_runtime_resume()?  Perhaps after the
> clk_bulk_prepare_enable() call below?

I can do that.

But we don't need to restore the clock phase for all speed modes since 
every mode
doesn't use DLL. Only few speed modes (only SDR104 mode for SD card)
would need to restore DLL setting. So I would need to do some extra checks
for performing this operation for only required speed modes.
And we should re-initialize the DLL first before programming the phase.

I wanted to reuse the existing logic which does all these things. So did 
this way.

>
> -Doug
Thanks
Veera

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

[Doug Anderson]

Hi,
On Thu, Sep 27, 2018 at 4:00 AM Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
>
> Hi Doug,
>
>
> On 9/26/2018 3:58 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Sep 25, 2018 at 8:05 AM Veerabhadrarao Badiganti
> > <vbadigan@codeaurora.org> wrote:
> >> +       /*
> >> +        * Whenever core-clock is gated dynamically, it's needed to
> >> +        * re-initialize the DLL when the clock is ungated.
> >> +        */
> >> +       if (msm_host->restore_dll_cfg_needed && msm_host->clk_rate) {
> >> +               msm_host->restore_dll_cfg = true;
> >> +               mmc_retune_needed(host->mmc);
> > Using the boolean "restore_dll_cfg" to communicate like this seems
> > really fragile.  I have no basis in fact, but I worry that something
> > will happen in the meantime that really ought to invalidate the
> > "saved_tuning_phase" but the boolean will still be set.
> >
> > Is there a reason you can't just call msm_config_cm_dll_phase()
> > directly from sdhci_msm_runtime_resume()?  Perhaps after the
> > clk_bulk_prepare_enable() call below?
>
> I can do that.
>
> But we don't need to restore the clock phase for all speed modes since
> every mode
> doesn't use DLL. Only few speed modes (only SDR104 mode for SD card)
> would need to restore DLL setting. So I would need to do some extra checks
> for performing this operation for only required speed modes.
> And we should re-initialize the DLL first before programming the phase.
>
> I wanted to reuse the existing logic which does all these things. So did
> this way.

Personally I'd rather see the extra checks or even restore the DLL
settings some cases when it wasn't truly needed.

In theory, though, I'd expect that there's a hardware register
somewhere that says whether the DLL settings matter or not.  Can't you
just key off of that?  ...or if that register is also lost when the
clock turns off then whenever you change that register to turn the DLL
off then clear your saved phase?


-Doug