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(-)
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.
On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti
<[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
Hi,
On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti
<[email protected]> 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 <[email protected]>
> ---
> 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;
Hi,
On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti
<[email protected]> 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
On 11/2/2018 2:03 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti
> <[email protected]> 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