* [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC [not found] <1541073999-11752-1-git-send-email-vbadigan@codeaurora.org> @ 2018-11-01 12:06 ` Veerabhadrarao Badiganti 2018-11-01 20:29 ` Doug Anderson 2018-11-01 12:06 ` [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically Veerabhadrarao Badiganti 1 sibling, 1 reply; 10+ messages in thread From: Veerabhadrarao Badiganti @ 2018-11-01 12:06 UTC (permalink / raw) To: adrian.hunter, ulf.hansson, robh+dt, evgreen, dianders Cc: asutoshd, riteshh, stummala, sayalil, Veerabhadrarao Badiganti, Mark Rutland, open list:MULTIMEDIA CARD (MMC), SECURE DIGITAL (SD) AND..., open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list For SDM845 SOC, new compatible string "qcom,sdm845-sdhci" is added. Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> --- Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt index 502b3b8..f2ffbeb 100644 --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt @@ -7,6 +7,7 @@ Required properties: - compatible: Should contain: "qcom,sdhci-msm-v4" for sdcc versions less than 5.0 "qcom,sdhci-msm-v5" for sdcc versions >= 5.0 + "qcom,sdm845-sdhci" for sdm845 SOC For SDCC version 5.0.0, MCI registers are removed from SDCC interface and some registers are moved to HC. New compatible string is added to support this change - "qcom,sdhci-msm-v5". -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC 2018-11-01 12:06 ` [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC Veerabhadrarao Badiganti @ 2018-11-01 20:29 ` Doug Anderson 2018-11-05 20:36 ` Rob Herring 0 siblings, 1 reply; 10+ messages in thread From: Doug Anderson @ 2018-11-01 20:29 UTC (permalink / raw) To: vbadigan Cc: Adrian Hunter, Ulf Hansson, Rob Herring, Evan Green, Asutosh Das, riteshh, stummala, sayalil, Mark Rutland, linux-mmc, devicetree, LKML Hi, On Thu, Nov 1, 2018 at 5:07 AM Veerabhadrarao Badiganti <vbadigan@codeaurora.org> wrote: > > For SDM845 SOC, new compatible string "qcom,sdm845-sdhci" is added. > > Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> > --- > Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt > index 502b3b8..f2ffbeb 100644 > --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt > +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt > @@ -7,6 +7,7 @@ Required properties: > - compatible: Should contain: > "qcom,sdhci-msm-v4" for sdcc versions less than 5.0 > "qcom,sdhci-msm-v5" for sdcc versions >= 5.0 > + "qcom,sdm845-sdhci" for sdm845 SOC It's up to Rob of course, but IMO it seems a nicer way forward to include both the SoC-specific string and the "version" string in all cases. I'd write this for the full text: - compatible: Should contain a SoC-specific string and a IP version string: version strings: "qcom,sdhci-msm-v4" for sdcc versions less than 5.0 "qcom,sdhci-msm-v5" for sdcc version 5.0 full compatible strings with SoC and version: "qcom,apq8084", "qcom,sdhci-msm-v4" "qcom,msm8974", "qcom,sdhci-msm-v4" "qcom,msm8916", "qcom,sdhci-msm-v4" "qcom,msm8992", "qcom,sdhci-msm-v4" "qcom,msm8996", "qcom,sdhci-msm-v4" "qcom,sdm845-sdhci", "qcom,sdhci-msm-v5" NOTE that some old device tree files may be floating around that only have the string "qcom,sdhci-msm-v4" without the SoC compatible string but doing that should be considered a deprecated practice. -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC 2018-11-01 20:29 ` Doug Anderson @ 2018-11-05 20:36 ` Rob Herring 2018-11-05 21:11 ` Doug Anderson 0 siblings, 1 reply; 10+ messages in thread From: Rob Herring @ 2018-11-05 20:36 UTC (permalink / raw) To: Doug Anderson Cc: vbadigan, Adrian Hunter, Ulf Hansson, Evan Green, Asutosh Das, riteshh, stummala, sayalil, Mark Rutland, linux-mmc, devicetree, LKML On Thu, Nov 01, 2018 at 01:29:54PM -0700, Doug Anderson wrote: > Hi, > > On Thu, Nov 1, 2018 at 5:07 AM Veerabhadrarao Badiganti > <vbadigan@codeaurora.org> wrote: > > > > For SDM845 SOC, new compatible string "qcom,sdm845-sdhci" is added. > > > > Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> > > --- > > Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt > > index 502b3b8..f2ffbeb 100644 > > --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt > > +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt > > @@ -7,6 +7,7 @@ Required properties: > > - compatible: Should contain: > > "qcom,sdhci-msm-v4" for sdcc versions less than 5.0 > > "qcom,sdhci-msm-v5" for sdcc versions >= 5.0 > > + "qcom,sdm845-sdhci" for sdm845 SOC > > It's up to Rob of course, but IMO it seems a nicer way forward to > include both the SoC-specific string and the "version" string in all > cases. I'd write this for the full text: Fine by me if you update all the dts files. > > - compatible: Should contain a SoC-specific string and a IP version string: > version strings: > "qcom,sdhci-msm-v4" for sdcc versions less than 5.0 > "qcom,sdhci-msm-v5" for sdcc version 5.0 > full compatible strings with SoC and version: > "qcom,apq8084", "qcom,sdhci-msm-v4" > "qcom,msm8974", "qcom,sdhci-msm-v4" > "qcom,msm8916", "qcom,sdhci-msm-v4" > "qcom,msm8992", "qcom,sdhci-msm-v4" > "qcom,msm8996", "qcom,sdhci-msm-v4" I assume you meant to append '-sdhci' here? > "qcom,sdm845-sdhci", "qcom,sdhci-msm-v5" > > NOTE that some old device tree files may be floating around that only > have the string "qcom,sdhci-msm-v4" without the SoC compatible string > but doing that should be considered a deprecated practice. > > -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC 2018-11-05 20:36 ` Rob Herring @ 2018-11-05 21:11 ` Doug Anderson 2018-11-06 17:59 ` Veerabhadrarao Badiganti 0 siblings, 1 reply; 10+ messages in thread From: Doug Anderson @ 2018-11-05 21:11 UTC (permalink / raw) To: Rob Herring Cc: vbadigan, Adrian Hunter, Ulf Hansson, Evan Green, Asutosh Das, riteshh, stummala, sayalil, Mark Rutland, linux-mmc, devicetree, LKML Hi, On Mon, Nov 5, 2018 at 12:37 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Nov 01, 2018 at 01:29:54PM -0700, Doug Anderson wrote: > > Hi, > > > > On Thu, Nov 1, 2018 at 5:07 AM Veerabhadrarao Badiganti > > <vbadigan@codeaurora.org> wrote: > > > > > > For SDM845 SOC, new compatible string "qcom,sdm845-sdhci" is added. > > > > > > Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> > > > --- > > > Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt > > > index 502b3b8..f2ffbeb 100644 > > > --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt > > > +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt > > > @@ -7,6 +7,7 @@ Required properties: > > > - compatible: Should contain: > > > "qcom,sdhci-msm-v4" for sdcc versions less than 5.0 > > > "qcom,sdhci-msm-v5" for sdcc versions >= 5.0 > > > + "qcom,sdm845-sdhci" for sdm845 SOC > > > > It's up to Rob of course, but IMO it seems a nicer way forward to > > include both the SoC-specific string and the "version" string in all > > cases. I'd write this for the full text: > > Fine by me if you update all the dts files. Done and done. https://lkml.kernel.org/r/20181105210921.253707-1-dianders@chromium.org https://lkml.kernel.org/r/20181105210921.253707-2-dianders@chromium.org > > - compatible: Should contain a SoC-specific string and a IP version string: > > version strings: > > "qcom,sdhci-msm-v4" for sdcc versions less than 5.0 > > "qcom,sdhci-msm-v5" for sdcc version 5.0 > > full compatible strings with SoC and version: > > "qcom,apq8084", "qcom,sdhci-msm-v4" > > "qcom,msm8974", "qcom,sdhci-msm-v4" > > "qcom,msm8916", "qcom,sdhci-msm-v4" > > "qcom,msm8992", "qcom,sdhci-msm-v4" > > "qcom,msm8996", "qcom,sdhci-msm-v4" > > I assume you meant to append '-sdhci' here? Oops, yes! "qcom,apq8084-sdhci", "qcom,sdhci-msm-v4" "qcom,msm8974-sdhci", "qcom,sdhci-msm-v4" "qcom,msm8916-sdhci", "qcom,sdhci-msm-v4" "qcom,msm8992-sdhci", "qcom,sdhci-msm-v4" "qcom,msm8996-sdhci", "qcom,sdhci-msm-v4" -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC 2018-11-05 21:11 ` Doug Anderson @ 2018-11-06 17:59 ` Veerabhadrarao Badiganti 0 siblings, 0 replies; 10+ messages in thread From: Veerabhadrarao Badiganti @ 2018-11-06 17:59 UTC (permalink / raw) To: Doug Anderson, Rob Herring Cc: Adrian Hunter, Ulf Hansson, Evan Green, Asutosh Das, riteshh, stummala, sayalil, Mark Rutland, linux-mmc, devicetree, LKML On 11/6/2018 2:41 AM, Doug Anderson wrote: > Hi, > > On Mon, Nov 5, 2018 at 12:37 PM Rob Herring <robh@kernel.org> wrote: >> On Thu, Nov 01, 2018 at 01:29:54PM -0700, Doug Anderson wrote: >>> Hi, >>> >>> On Thu, Nov 1, 2018 at 5:07 AM Veerabhadrarao Badiganti >>> <vbadigan@codeaurora.org> wrote: >>>> For SDM845 SOC, new compatible string "qcom,sdm845-sdhci" is added. >>>> >>>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> >>>> --- >>>> Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt >>>> index 502b3b8..f2ffbeb 100644 >>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt >>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt >>>> @@ -7,6 +7,7 @@ Required properties: >>>> - compatible: Should contain: >>>> "qcom,sdhci-msm-v4" for sdcc versions less than 5.0 >>>> "qcom,sdhci-msm-v5" for sdcc versions >= 5.0 >>>> + "qcom,sdm845-sdhci" for sdm845 SOC >>> It's up to Rob of course, but IMO it seems a nicer way forward to >>> include both the SoC-specific string and the "version" string in all >>> cases. I'd write this for the full text: >> Fine by me if you update all the dts files. > Done and done. > > https://lkml.kernel.org/r/20181105210921.253707-1-dianders@chromium.org > https://lkml.kernel.org/r/20181105210921.253707-2-dianders@chromium.org > > >>> - compatible: Should contain a SoC-specific string and a IP version string: >>> version strings: >>> "qcom,sdhci-msm-v4" for sdcc versions less than 5.0 >>> "qcom,sdhci-msm-v5" for sdcc version 5.0 >>> full compatible strings with SoC and version: >>> "qcom,apq8084", "qcom,sdhci-msm-v4" >>> "qcom,msm8974", "qcom,sdhci-msm-v4" >>> "qcom,msm8916", "qcom,sdhci-msm-v4" >>> "qcom,msm8992", "qcom,sdhci-msm-v4" >>> "qcom,msm8996", "qcom,sdhci-msm-v4" >> I assume you meant to append '-sdhci' here? > Oops, yes! > > "qcom,apq8084-sdhci", "qcom,sdhci-msm-v4" > "qcom,msm8974-sdhci", "qcom,sdhci-msm-v4" > "qcom,msm8916-sdhci", "qcom,sdhci-msm-v4" > "qcom,msm8992-sdhci", "qcom,sdhci-msm-v4" > "qcom,msm8996-sdhci", "qcom,sdhci-msm-v4" Thank you. Will update the documentation. > -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically [not found] <1541073999-11752-1-git-send-email-vbadigan@codeaurora.org> 2018-11-01 12:06 ` [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC Veerabhadrarao Badiganti @ 2018-11-01 12:06 ` Veerabhadrarao Badiganti 2018-11-01 17:10 ` Evan Green ` (2 more replies) 1 sibling, 3 replies; 10+ messages in thread From: Veerabhadrarao Badiganti @ 2018-11-01 12:06 UTC (permalink / raw) To: adrian.hunter, ulf.hansson, robh+dt, evgreen, dianders Cc: asutoshd, riteshh, stummala, sayalil, Veerabhadrarao Badiganti, open list On few SDHCI-MSM controllers, the host controller's clock tuning circuit may go out of sync if controller clocks are gated which eventually will result in data CRC, command CRC/timeout errors. To overcome this h/w limitation, the DLL needs to be re-initialized and restored with its old settings once clocks are ungated. Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> --- drivers/mmc/host/sdhci-msm.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 2 deletions(-) diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 3cc8bfe..e38a4e8 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -232,6 +232,7 @@ struct sdhci_msm_variant_ops { */ struct sdhci_msm_variant_info { bool mci_removed; + bool restore_dll_config; const struct sdhci_msm_variant_ops *var_ops; const struct sdhci_msm_offset *offset; }; @@ -256,6 +257,7 @@ struct sdhci_msm_host { bool pwr_irq_flag; u32 caps_0; bool mci_removed; + bool restore_dll_config; const struct sdhci_msm_variant_ops *var_ops; const struct sdhci_msm_offset *offset; }; @@ -1025,6 +1027,36 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host) return ret; } +static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host) +{ + struct mmc_ios ios = host->mmc->ios; + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); + int ret; + + /* + * SDR DLL comes into picure only if clock frequency is greater than + * 100MHz. And its needed only for SDR104, HS200 and HS400 cards. + * Its not needed for HS400es cards. + */ + if (host->clock <= CORE_FREQ_100MHZ || + !(ios.timing == MMC_TIMING_MMC_HS400 || + ios.timing == MMC_TIMING_MMC_HS200 || + ios.timing == MMC_TIMING_UHS_SDR104) || + ios.enhanced_strobe) + return 0; + + /* Reset the tuning block */ + ret = msm_init_cm_dll(host); + if (ret) + return ret; + + /* Restore the tuning block */ + ret = msm_config_cm_dll_phase(host, msm_host->saved_tuning_phase); + + return ret; +} + static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode) { struct sdhci_host *host = mmc_priv(mmc); @@ -1069,7 +1101,6 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode) if (rc) return rc; - msm_host->saved_tuning_phase = phase; rc = mmc_send_tuning(mmc, opcode, NULL); if (!rc) { /* Tuning is successful at this tuning point */ @@ -1094,6 +1125,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode) rc = msm_config_cm_dll_phase(host, phase); if (rc) return rc; + msm_host->saved_tuning_phase = phase; dev_dbg(mmc_dev(mmc), "%s: Setting the tuning phase to %d\n", mmc_hostname(mmc), phase); } else { @@ -1617,12 +1649,21 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) static const struct sdhci_msm_variant_info sdhci_msm_mci_var = { .mci_removed = false, + .restore_dll_config = false, .var_ops = &mci_var_ops, .offset = &sdhci_msm_mci_offset, }; static const struct sdhci_msm_variant_info sdhci_msm_v5_var = { .mci_removed = true, + .restore_dll_config = false, + .var_ops = &v5_var_ops, + .offset = &sdhci_msm_v5_offset, +}; + +static const struct sdhci_msm_variant_info sdm845_sdhci_var = { + .mci_removed = true, + .restore_dll_config = true, .var_ops = &v5_var_ops, .offset = &sdhci_msm_v5_offset, }; @@ -1630,6 +1671,7 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) static const struct of_device_id sdhci_msm_dt_match[] = { {.compatible = "qcom,sdhci-msm-v4", .data = &sdhci_msm_mci_var}, {.compatible = "qcom,sdhci-msm-v5", .data = &sdhci_msm_v5_var}, + {.compatible = "qcom,sdm845-sdhci", .data = &sdm845_sdhci_var}, {}, }; @@ -1689,6 +1731,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) var_info = of_device_get_match_data(&pdev->dev); msm_host->mci_removed = var_info->mci_removed; + msm_host->restore_dll_config = var_info->restore_dll_config; msm_host->var_ops = var_info->var_ops; msm_host->offset = var_info->offset; @@ -1928,9 +1971,21 @@ static int sdhci_msm_runtime_resume(struct device *dev) struct sdhci_host *host = dev_get_drvdata(dev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); + int ret; - return clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks), + ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks), msm_host->bulk_clks); + if (ret) + goto out; + /* + * Whenever core-clock is gated dynamically, it's needed to + * restore the SDR DLL settings when the clock is ungated. + */ + if (msm_host->restore_dll_config && msm_host->clk_rate) + ret = sdhci_msm_restore_sdr_dll_config(host); + +out: + return ret; } #endif -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically 2018-11-01 12:06 ` [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically Veerabhadrarao Badiganti @ 2018-11-01 17:10 ` Evan Green 2018-11-01 20:16 ` Doug Anderson 2018-11-01 20:33 ` Doug Anderson 2 siblings, 0 replies; 10+ messages in thread From: Evan Green @ 2018-11-01 17:10 UTC (permalink / raw) To: vbadigan Cc: adrian.hunter, Ulf Hansson, robh+dt, Doug Anderson, asutoshd, riteshh, stummala, sayali, linux-kernel On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti <vbadigan@codeaurora.org> wrote: > > On few SDHCI-MSM controllers, the host controller's clock tuning > circuit may go out of sync if controller clocks are gated which > eventually will result in data CRC, command CRC/timeout errors. > To overcome this h/w limitation, the DLL needs to be re-initialized > and restored with its old settings once clocks are ungated. > > Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> > --- > drivers/mmc/host/sdhci-msm.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 57 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c Reviewed-by: Evan Green <evgreen@chromium.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically 2018-11-01 12:06 ` [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically Veerabhadrarao Badiganti 2018-11-01 17:10 ` Evan Green @ 2018-11-01 20:16 ` Doug Anderson 2018-11-01 20:33 ` Doug Anderson 2 siblings, 0 replies; 10+ messages in thread From: Doug Anderson @ 2018-11-01 20:16 UTC (permalink / raw) To: vbadigan Cc: Adrian Hunter, Ulf Hansson, Rob Herring, Evan Green, Asutosh Das, riteshh, stummala, sayalil, LKML Hi, On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti <vbadigan@codeaurora.org> wrote: > > On few SDHCI-MSM controllers, the host controller's clock tuning > circuit may go out of sync if controller clocks are gated which > eventually will result in data CRC, command CRC/timeout errors. > To overcome this h/w limitation, the DLL needs to be re-initialized > and restored with its old settings once clocks are ungated. > > Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> > --- > drivers/mmc/host/sdhci-msm.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 57 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index 3cc8bfe..e38a4e8 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -232,6 +232,7 @@ struct sdhci_msm_variant_ops { > */ > struct sdhci_msm_variant_info { > bool mci_removed; > + bool restore_dll_config; > const struct sdhci_msm_variant_ops *var_ops; > const struct sdhci_msm_offset *offset; > }; > @@ -256,6 +257,7 @@ struct sdhci_msm_host { > bool pwr_irq_flag; > u32 caps_0; > bool mci_removed; > + bool restore_dll_config; > const struct sdhci_msm_variant_ops *var_ops; > const struct sdhci_msm_offset *offset; > }; > @@ -1025,6 +1027,36 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host) > return ret; > } > > +static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host) > +{ > + struct mmc_ios ios = host->mmc->ios; Please use a pointer. Right now you're copying the whole structure onto the stack. Sure it's maybe only 20 bytes, but why? ...for bonus points add a separate patch that fixes all the other places in this file that do the same thing. A grep can see that msm is the only one that does this, everyone else uses a pointer: $ git grep 'struct mmc_ios.*=' -- drivers/mmc/host drivers/mmc/host/omap_hsmmc.c: struct mmc_ios *ios = &mmc->ios; drivers/mmc/host/omap_hsmmc.c: struct mmc_ios *ios = &host->mmc->ios; drivers/mmc/host/omap_hsmmc.c: struct mmc_ios *ios = &host->mmc->ios; drivers/mmc/host/omap_hsmmc.c: struct mmc_ios *ios = &host->mmc->ios; drivers/mmc/host/omap_hsmmc.c: struct mmc_ios *ios = &host->mmc->ios; drivers/mmc/host/sdhci-msm.c: struct mmc_ios ios = host->mmc->ios; drivers/mmc/host/sdhci-msm.c: struct mmc_ios curr_ios = host->mmc->ios; drivers/mmc/host/sdhci-msm.c: struct mmc_ios ios = host->mmc->ios; drivers/mmc/host/sdhci-msm.c: struct mmc_ios ios = host->mmc->ios; drivers/mmc/host/sdhci-msm.c: struct mmc_ios ios = host->mmc->ios; drivers/mmc/host/sdhci-msm.c: struct mmc_ios ios = host->mmc->ios; drivers/mmc/host/sdhci-omap.c: struct mmc_ios *ios = &mmc->ios; drivers/mmc/host/sdhci.c: struct mmc_ios *ios = &mmc->ios; > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); > + int ret; > + > + /* > + * SDR DLL comes into picure only if clock frequency is greater than > + * 100MHz. And its needed only for SDR104, HS200 and HS400 cards. > + * Its not needed for HS400es cards. > + */ > + if (host->clock <= CORE_FREQ_100MHZ || > + !(ios.timing == MMC_TIMING_MMC_HS400 || > + ios.timing == MMC_TIMING_MMC_HS200 || > + ios.timing == MMC_TIMING_UHS_SDR104) || > + ios.enhanced_strobe) This if test is nearly copied from sdhci_msm_execute_tuning(). My first thought it that this is complicated enough logic that you should write a helper. ...looking at this is makes me feel like there's a bug in sdhci_msm_execute_tuning() because it doesn't check for "enhanced_strobe". ...but maybe the answer is that the test in sdhci_msm_execute_tuning() is pointless anyway because the core code should only call execute_tuning() if we're using a mode that needs tuning. So maybe the answer is to remove the code from sdhci_msm_execute_tuning()... ...but thinking even more: do we really need this logic? Basically you want to invalidate the saved dll config whenever the timing mode changes, right? ...because execute_tuning() won't be called if you go down to a timing change that doesn't use tuning? Maybe you can just save the timing mode at the same time you save the phase. If the current timing mode doesn't equal the saved timing mode then you don't restore the phase. > + return 0; > + > + /* Reset the tuning block */ > + ret = msm_init_cm_dll(host); > + if (ret) > + return ret; > + > + /* Restore the tuning block */ > + ret = msm_config_cm_dll_phase(host, msm_host->saved_tuning_phase); > + > + return ret; > +} > + > static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode) > { > struct sdhci_host *host = mmc_priv(mmc); > @@ -1069,7 +1101,6 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode) > if (rc) > return rc; > > - msm_host->saved_tuning_phase = phase; > rc = mmc_send_tuning(mmc, opcode, NULL); > if (!rc) { > /* Tuning is successful at this tuning point */ > @@ -1094,6 +1125,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode) > rc = msm_config_cm_dll_phase(host, phase); > if (rc) > return rc; > + msm_host->saved_tuning_phase = phase; > dev_dbg(mmc_dev(mmc), "%s: Setting the tuning phase to %d\n", > mmc_hostname(mmc), phase); > } else { > @@ -1617,12 +1649,21 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) > > static const struct sdhci_msm_variant_info sdhci_msm_mci_var = { > .mci_removed = false, > + .restore_dll_config = false, nit: Linux convention is to rely on static structures to be initted to 0 / false / NULL. ...so no need to add an "= false" here. > .var_ops = &mci_var_ops, > .offset = &sdhci_msm_mci_offset, > }; > > static const struct sdhci_msm_variant_info sdhci_msm_v5_var = { > .mci_removed = true, > + .restore_dll_config = false, ...and here... > + .var_ops = &v5_var_ops, > + .offset = &sdhci_msm_v5_offset, > +}; > + > +static const struct sdhci_msm_variant_info sdm845_sdhci_var = { > + .mci_removed = true, > + .restore_dll_config = true, > .var_ops = &v5_var_ops, > .offset = &sdhci_msm_v5_offset, > }; > @@ -1630,6 +1671,7 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) > static const struct of_device_id sdhci_msm_dt_match[] = { > {.compatible = "qcom,sdhci-msm-v4", .data = &sdhci_msm_mci_var}, > {.compatible = "qcom,sdhci-msm-v5", .data = &sdhci_msm_v5_var}, > + {.compatible = "qcom,sdm845-sdhci", .data = &sdm845_sdhci_var}, > {}, > }; > > @@ -1689,6 +1731,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) > var_info = of_device_get_match_data(&pdev->dev); > > msm_host->mci_removed = var_info->mci_removed; > + msm_host->restore_dll_config = var_info->restore_dll_config; > msm_host->var_ops = var_info->var_ops; > msm_host->offset = var_info->offset; > > @@ -1928,9 +1971,21 @@ static int sdhci_msm_runtime_resume(struct device *dev) > struct sdhci_host *host = dev_get_drvdata(dev); > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); > + int ret; > > - return clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks), > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks), > msm_host->bulk_clks); > + if (ret) > + goto out; no need for a goto since you're not undoing anything. Just: if (ret) return ret; > + /* > + * Whenever core-clock is gated dynamically, it's needed to > + * restore the SDR DLL settings when the clock is ungated. > + */ > + if (msm_host->restore_dll_config && msm_host->clk_rate) > + ret = sdhci_msm_restore_sdr_dll_config(host); > + > +out: > + return ret; No need for 'ret' variable: if (msm_host->restore_dll_config && msm_host->clk_rate) return sdhci_msm_restore_sdr_dll_config(host); return 0; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically 2018-11-01 12:06 ` [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically Veerabhadrarao Badiganti 2018-11-01 17:10 ` Evan Green 2018-11-01 20:16 ` Doug Anderson @ 2018-11-01 20:33 ` Doug Anderson 2018-11-06 17:51 ` Veerabhadrarao Badiganti 2 siblings, 1 reply; 10+ messages in thread From: Doug Anderson @ 2018-11-01 20:33 UTC (permalink / raw) To: vbadigan Cc: Adrian Hunter, Ulf Hansson, Rob Herring, Evan Green, Asutosh Das, riteshh, stummala, sayalil, LKML Hi, On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti <vbadigan@codeaurora.org> wrote: > static const struct sdhci_msm_variant_info sdhci_msm_v5_var = { > .mci_removed = true, > + .restore_dll_config = false, > + .var_ops = &v5_var_ops, > + .offset = &sdhci_msm_v5_offset, > +}; > + > +static const struct sdhci_msm_variant_info sdm845_sdhci_var = { > + .mci_removed = true, > + .restore_dll_config = true, > .var_ops = &v5_var_ops, > .offset = &sdhci_msm_v5_offset, > }; One last thing: are there actually any "v5" controllers that _don't_ require restoring the DLL? Since "sdm845" is currently the only v5 controller maybe just set "restore_dll_config = true" for all v5 controllers and when there's a new v5 controller that doesn't need it then match off the SoC-specific compatible string. As per my review of the bindings patch IMO you should include both the "v5" and the SoC-specific string for SDM845 (and all future SoCs) so you could make the generic v5 case do this... -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically 2018-11-01 20:33 ` Doug Anderson @ 2018-11-06 17:51 ` Veerabhadrarao Badiganti 0 siblings, 0 replies; 10+ messages in thread From: Veerabhadrarao Badiganti @ 2018-11-06 17:51 UTC (permalink / raw) To: Doug Anderson Cc: Adrian Hunter, Ulf Hansson, Rob Herring, Evan Green, Asutosh Das, riteshh, stummala, sayalil, LKML On 11/2/2018 2:03 AM, Doug Anderson wrote: > Hi, > > On Thu, Nov 1, 2018 at 5:08 AM Veerabhadrarao Badiganti > <vbadigan@codeaurora.org> wrote: >> static const struct sdhci_msm_variant_info sdhci_msm_v5_var = { >> .mci_removed = true, >> + .restore_dll_config = false, >> + .var_ops = &v5_var_ops, >> + .offset = &sdhci_msm_v5_offset, >> +}; >> + >> +static const struct sdhci_msm_variant_info sdm845_sdhci_var = { >> + .mci_removed = true, >> + .restore_dll_config = true, >> .var_ops = &v5_var_ops, >> .offset = &sdhci_msm_v5_offset, >> }; > One last thing: are there actually any "v5" controllers that _don't_ > require restoring the DLL? Since "sdm845" is currently the only v5 > controller maybe just set "restore_dll_config = true" for all v5 > controllers and when there's a new v5 controller that doesn't need it > then match off the SoC-specific compatible string. As per my review > of the bindings patch IMO you should include both the "v5" and the > SoC-specific string for SDM845 (and all future SoCs) so you could make > the generic v5 case do this... > Yes. QCS404 is one of the target which uses "V5" controller and it doesn't need restoring of DLL. I checked your comments on bindings patch. Will update it in the next patchset. > -Doug Thanks, Veera ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-11-06 17:59 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1541073999-11752-1-git-send-email-vbadigan@codeaurora.org> 2018-11-01 12:06 ` [PATCH V3 1/2] dt-bindings: mmc: sdhci-msm: Add new compatible string for SDM845 SOC Veerabhadrarao Badiganti 2018-11-01 20:29 ` Doug Anderson 2018-11-05 20:36 ` Rob Herring 2018-11-05 21:11 ` Doug Anderson 2018-11-06 17:59 ` Veerabhadrarao Badiganti 2018-11-01 12:06 ` [PATCH V3 2/2] mmc: sdhci-msm: Re-initialize DLL if MCLK is gated dynamically Veerabhadrarao Badiganti 2018-11-01 17:10 ` Evan Green 2018-11-01 20:16 ` Doug Anderson 2018-11-01 20:33 ` Doug Anderson 2018-11-06 17:51 ` Veerabhadrarao Badiganti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).