From: Sachin Gupta <quic_sachgupt@quicinc.com>
With the current DLL sequence stability issues for data
transfer seen in HS400 and HS200 modes.
"mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
data error 0"
Rectify the DLL programming sequence as per latest hardware
programming guide
Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
---
drivers/mmc/host/sdhci-msm.c | 277 ++++++++++++++++++++++++++++++++---
1 file changed, 260 insertions(+), 17 deletions(-)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 1fcd92158bee..f459b40db41c 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -28,6 +28,7 @@
#define CORE_VERSION_MAJOR_SHIFT 28
#define CORE_VERSION_MAJOR_MASK (0xf << CORE_VERSION_MAJOR_SHIFT)
#define CORE_VERSION_MINOR_MASK 0xff
+#define SDHCI_MSM_MIN_V_7FF 0x6e
#define CORE_MCI_GENERICS 0x70
#define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
@@ -119,7 +120,8 @@
#define CORE_PWRSAVE_DLL BIT(3)
#define DDR_CONFIG_POR_VAL 0x80040873
-
+#define DLL_CONFIG_3_POR_VAL 0x10
+#define TCXO_FREQ 19200000
#define INVALID_TUNING_PHASE -1
#define SDHCI_MSM_MIN_CLOCK 400000
@@ -319,6 +321,15 @@ struct sdhci_msm_host {
bool artanis_dll;
};
+enum dll_init_context {
+ DLL_INIT_NORMAL,
+};
+
+enum mode {
+ HS400, // equivalent to SDR104 mode for DLL.
+ HS200, // equivalent to SDR50 mode for DLL.
+};
+
static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -805,6 +816,207 @@ static int msm_init_cm_dll(struct sdhci_host *host)
return 0;
}
+static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
+{
+ return SDHCI_MSM_MIN_CLOCK;
+}
+
+static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
+{
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+ struct clk *core_clk = msm_host->bulk_clks[0].clk;
+ struct mmc_ios ios = host->mmc->ios;
+ unsigned int sup_clk;
+
+ if (req_clk < sdhci_msm_get_min_clock(host))
+ return sdhci_msm_get_min_clock(host);
+
+ sup_clk = clk_get_rate(core_clk);
+
+ if (ios.timing == MMC_TIMING_MMC_HS400 ||
+ host->flags & SDHCI_HS400_TUNING)
+ sup_clk = sup_clk / 2;
+
+ return sup_clk;
+}
+
+/* Initialize the DLL (Programmable Delay Line) */
+static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
+ init_context, enum mode index)
+{
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+ const struct sdhci_msm_offset *msm_offset = msm_host->offset;
+ struct mmc_host *mmc = host->mmc;
+ u32 ddr_cfg_offset, core_vendor_spec, config;
+ void __iomem *ioaddr = host->ioaddr;
+ unsigned long flags, dll_clock;
+ int rc = 0;
+ u32 dll_lock;
+
+ dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
+ spin_lock_irqsave(&host->lock, flags);
+
+ core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
+
+ /*
+ * Always disable PWRSAVE during the DLL power
+ * up regardless of its current setting.
+ */
+ core_vendor_spec &= ~CORE_CLK_PWRSAVE;
+ writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
+
+ if (msm_host->use_14lpp_dll_reset) {
+ /* Disable CK_OUT */
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
+ config &= ~CORE_CK_OUT_EN;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /* Disable the DLL clock */
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
+ config |= CORE_DLL_CLOCK_DISABLE;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
+ }
+
+ /*
+ * Write 1 to DLL_RST bit of DLL_CONFIG register
+ * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
+ */
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
+ config |= (CORE_DLL_RST | CORE_DLL_PDN);
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /*
+ * Configure DLL_CONFIG_3 and USER_CTRL
+ * (Only applicable for 7FF projects).
+ */
+ if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
+ writel_relaxed(msm_host->dll[index].dll_config_3,
+ ioaddr + msm_offset->core_dll_config_3);
+ writel_relaxed(msm_host->dll[index].dll_usr_ctl,
+ ioaddr + msm_offset->core_dll_usr_ctl);
+ }
+
+ /*
+ * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
+ */
+ ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
+ : msm_offset->core_ddr_config_old;
+
+ config = msm_host->dll[index].ddr_config;
+ writel_relaxed(config, ioaddr + ddr_cfg_offset);
+
+ /* Set DLL_CONFIG_2 */
+ if (msm_host->use_14lpp_dll_reset) {
+ u32 mclk_freq;
+ int cycle_cnt;
+
+ /*
+ * Only configure the mclk_freq in normal DLL init
+ * context. If the DLL init is coming from
+ * CX Collapse Exit context, the host->clock may be zero.
+ * The DLL_CONFIG_2 register has already been restored to
+ * proper value prior to getting here.
+ */
+ if (init_context == DLL_INIT_NORMAL) {
+ cycle_cnt = readl_relaxed(ioaddr +
+ msm_offset->core_dll_config_2)
+ & CORE_FLL_CYCLE_CNT ? 8 : 4;
+
+ mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
+
+ if (dll_clock < 100000000) {
+ pr_err("%s: %s: Non standard clk freq =%u\n",
+ mmc_hostname(mmc), __func__, dll_clock);
+ rc = -EINVAL;
+ goto out;
+ }
+
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
+ config = (config & ~GENMASK(17, 10)) |
+ FIELD_PREP(GENMASK(17, 10), mclk_freq);
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
+ }
+ /* wait for 5us before enabling DLL clock */
+ udelay(5);
+ }
+
+ config = msm_host->dll[index].dll_config;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /* Wait for 52us */
+ spin_unlock_irqrestore(&host->lock, flags);
+ usleep_range(60, 70);
+ spin_lock_irqsave(&host->lock, flags);
+
+ /*
+ * Write 0 to DLL_RST bit of DLL_CONFIG register
+ * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
+ */
+ config &= ~CORE_DLL_RST;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ config &= ~CORE_DLL_PDN;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+ /* Write 1 to DLL_RST bit of DLL_CONFIG register */
+ config |= CORE_DLL_RST;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /* Write 0 to DLL_RST bit of DLL_CONFIG register */
+ config &= ~CORE_DLL_RST;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /* Set CORE_DLL_CLOCK_DISABLE to 0 */
+ if (msm_host->use_14lpp_dll_reset) {
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
+ config &= ~CORE_DLL_CLOCK_DISABLE;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
+ }
+
+ /* Set DLL_EN bit to 1. */
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
+ config |= CORE_DLL_EN;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /*
+ * Wait for 8000 input clock. Here we calculate the
+ * delay from fixed clock freq 192MHz, which turns out 42us.
+ */
+ spin_unlock_irqrestore(&host->lock, flags);
+ usleep_range(50, 60);
+ spin_lock_irqsave(&host->lock, flags);
+
+ /* Set CK_OUT_EN bit to 1. */
+ config |= CORE_CK_OUT_EN;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /*
+ * Wait until DLL_LOCK bit of DLL_STATUS register
+ * becomes '1'.
+ */
+ rc = readl_relaxed_poll_timeout(ioaddr +
+ msm_offset->core_dll_status,
+ dll_lock,
+ dll_lock & CORE_DLL_LOCK,
+ 10,
+ 100);
+ if (rc == -ETIMEDOUT)
+ pr_err("%s: Unable to get DLL_LOCK, dll_status: 0x%08x\n",
+ mmc_hostname(host->mmc), dll_lock);
+
+out:
+ if (core_vendor_spec & CORE_CLK_PWRSAVE) {
+ /* Reenable PWRSAVE as needed */
+ config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
+ config |= CORE_CLK_PWRSAVE;
+ writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
+ }
+
+ spin_unlock_irqrestore(&host->lock, flags);
+ return rc;
+}
+
static void msm_hc_select_default(struct sdhci_host *host)
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -927,6 +1139,24 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
msm_hc_select_default(host);
}
+static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
+{
+ if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104 ||
+ host->mmc->ios.timing == MMC_TIMING_MMC_HS400)
+ return sdhci_msm_configure_dll(host, init_context, HS400);
+
+ return sdhci_msm_configure_dll(host, init_context, HS200);
+}
+
+static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
+{
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+ return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
+ msm_init_cm_dll(host);
+}
+
static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -942,7 +1172,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
* Retuning in HS400 (DDR mode) will fail, just reset the
* tuning block and restore the saved tuning phase.
*/
- ret = msm_init_cm_dll(host);
+ ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
if (ret)
goto out;
@@ -1030,7 +1260,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
return ret;
}
-static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
+static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
{
struct mmc_host *mmc = host->mmc;
u32 dll_status, config, ddr_cfg_offset;
@@ -1053,7 +1283,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
ddr_cfg_offset = msm_offset->core_ddr_config;
else
ddr_cfg_offset = msm_offset->core_ddr_config_old;
- writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
+
+ if (msm_host->artanis_dll)
+ writel_relaxed(msm_host->dll[index].ddr_config, host->ioaddr + ddr_cfg_offset);
+ else
+ writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
if (mmc->ios.enhanced_strobe) {
config = readl_relaxed(host->ioaddr +
@@ -1122,7 +1356,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
* Retuning in HS400 (DDR mode) will fail, just reset the
* tuning block and restore the saved tuning phase.
*/
- ret = msm_init_cm_dll(host);
+ ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
+
if (ret)
goto out;
@@ -1142,7 +1377,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
if (msm_host->use_cdclp533)
ret = sdhci_msm_cdclp533_calibration(host);
else
- ret = sdhci_msm_cm_dll_sdc4_calibration(host);
+ ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
out:
pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
__func__, ret);
@@ -1185,7 +1420,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
return 0;
/* Reset the tuning block */
- ret = msm_init_cm_dll(host);
+ ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
+
if (ret)
return ret;
@@ -1227,6 +1463,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
const struct sdhci_msm_offset *msm_offset = msm_host->offset;
u32 config;
+ bool hs400_tuning;
if (!sdhci_msm_is_tuning_needed(host)) {
msm_host->use_cdr = false;
@@ -1256,17 +1493,23 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
* - select MCLK/2 in VENDOR_SPEC
* - program MCLK to 400MHz (or nearest supported) in GCC
*/
- if (host->flags & SDHCI_HS400_TUNING) {
+ hs400_tuning = !!(host->flags & SDHCI_HS400_TUNING);
+ if (hs400_tuning) {
sdhci_msm_hc_select_mode(host);
msm_set_clock_rate_for_bus_mode(host, ios.clock, ios.timing);
- host->flags &= ~SDHCI_HS400_TUNING;
}
retry:
/* First of all reset the tuning block */
- rc = msm_init_cm_dll(host);
- if (rc)
+ rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
+ if (rc) {
+ if (hs400_tuning)
+ host->flags &= ~SDHCI_HS400_TUNING;
return rc;
+ }
+
+ if (hs400_tuning)
+ host->flags &= ~SDHCI_HS400_TUNING;
phase = 0;
do {
@@ -1297,6 +1540,8 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
mmc_hostname(mmc));
if (--tuning_seq_cnt) {
tuned_phase_cnt = 0;
+ if (hs400_tuning)
+ host->flags |= SDHCI_HS400_TUNING;
goto retry;
}
}
@@ -1319,8 +1564,11 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
dev_dbg(mmc_dev(mmc), "%s: Setting the tuning phase to %d\n",
mmc_hostname(mmc), phase);
} else {
- if (--tuning_seq_cnt)
+ if (--tuning_seq_cnt) {
+ if (hs400_tuning)
+ host->flags |= SDHCI_HS400_TUNING;
goto retry;
+ }
/* Tuning failed */
dev_dbg(mmc_dev(mmc), "%s: No tuning point found\n",
mmc_hostname(mmc));
@@ -1847,11 +2095,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
return clk_round_rate(core_clk, ULONG_MAX);
}
-static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
-{
- return SDHCI_MSM_MIN_CLOCK;
-}
-
/*
* __sdhci_msm_set_clock - sdhci_msm clock control.
*
--
2.34.1On Mon, Dec 15, 2025 at 05:30:09PM +0530, Ram Prakash Gupta wrote:
> From: Sachin Gupta <quic_sachgupt@quicinc.com>
>
> With the current DLL sequence stability issues for data
> transfer seen in HS400 and HS200 modes.
>
> "mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
> data error 0"
>
> Rectify the DLL programming sequence as per latest hardware
> programming guide
You're talking about how things are unstable, but what the commit
message completely fails to mention is that all this new spaghetti is
conditional on msm_host->artanis_dll, so I believe this statement
only applies to a single supported SoC?
No where in this series do you mention that this only (?) applies to
QDU1000.
>
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
> drivers/mmc/host/sdhci-msm.c | 277 ++++++++++++++++++++++++++++++++---
> 1 file changed, 260 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 1fcd92158bee..f459b40db41c 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -28,6 +28,7 @@
> #define CORE_VERSION_MAJOR_SHIFT 28
> #define CORE_VERSION_MAJOR_MASK (0xf << CORE_VERSION_MAJOR_SHIFT)
> #define CORE_VERSION_MINOR_MASK 0xff
> +#define SDHCI_MSM_MIN_V_7FF 0x6e
>
> #define CORE_MCI_GENERICS 0x70
> #define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
> @@ -119,7 +120,8 @@
> #define CORE_PWRSAVE_DLL BIT(3)
>
> #define DDR_CONFIG_POR_VAL 0x80040873
> -
> +#define DLL_CONFIG_3_POR_VAL 0x10
> +#define TCXO_FREQ 19200000
If you read the current implementation of msm_init_cm_dll() you'll see
that we don't hard code the XO clock...
>
> #define INVALID_TUNING_PHASE -1
> #define SDHCI_MSM_MIN_CLOCK 400000
> @@ -319,6 +321,15 @@ struct sdhci_msm_host {
> bool artanis_dll;
> };
>
> +enum dll_init_context {
> + DLL_INIT_NORMAL,
What is the purpose of a single-entry enum to capture the value 0?
> +};
> +
> +enum mode {
> + HS400, // equivalent to SDR104 mode for DLL.
> + HS200, // equivalent to SDR50 mode for DLL.
This is effectively a bool, with a terribly generic name.
> +};
> +
> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> {
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -805,6 +816,207 @@ static int msm_init_cm_dll(struct sdhci_host *host)
> return 0;
> }
>
> +static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> +{
> + return SDHCI_MSM_MIN_CLOCK;
> +}
> +
> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
> + struct mmc_ios ios = host->mmc->ios;
> + unsigned int sup_clk;
> +
> + if (req_clk < sdhci_msm_get_min_clock(host))
> + return sdhci_msm_get_min_clock(host);
> +
> + sup_clk = clk_get_rate(core_clk);
> +
> + if (ios.timing == MMC_TIMING_MMC_HS400 ||
> + host->flags & SDHCI_HS400_TUNING)
> + sup_clk = sup_clk / 2;
> +
> + return sup_clk;
Isn't sup_clk the same as host->mmc->actual_clock? Isn't this already
calculated for you?
> +}
> +
> +/* Initialize the DLL (Programmable Delay Line) */
So now we're going to have two functions that are named pretty much the
same and look pretty similar?
With the main difference that the slightly more generically named
sdhci_msm_configure_dll() is only called if we have a artanis_dll,
otherwise msm_init_cm_dll() is used?
> +static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
> + init_context, enum mode index)
> +{
Large parts of this function is duplicated from msm_init_cm_dll(). This
isn't okay.
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> + const struct sdhci_msm_offset *msm_offset = msm_host->offset;
> + struct mmc_host *mmc = host->mmc;
> + u32 ddr_cfg_offset, core_vendor_spec, config;
> + void __iomem *ioaddr = host->ioaddr;
> + unsigned long flags, dll_clock;
> + int rc = 0;
> + u32 dll_lock;
> +
> + dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
> + spin_lock_irqsave(&host->lock, flags);
> +
> + core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
> +
> + /*
> + * Always disable PWRSAVE during the DLL power
> + * up regardless of its current setting.
> + */
> + core_vendor_spec &= ~CORE_CLK_PWRSAVE;
> + writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
> +
> + if (msm_host->use_14lpp_dll_reset) {
> + /* Disable CK_OUT */
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> + config &= ~CORE_CK_OUT_EN;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /* Disable the DLL clock */
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> + config |= CORE_DLL_CLOCK_DISABLE;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> + }
> +
> + /*
> + * Write 1 to DLL_RST bit of DLL_CONFIG register
> + * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
> + */
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> + config |= (CORE_DLL_RST | CORE_DLL_PDN);
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /*
> + * Configure DLL_CONFIG_3 and USER_CTRL
> + * (Only applicable for 7FF projects).
What is a 7FF project?
> + */
> + if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
> + writel_relaxed(msm_host->dll[index].dll_config_3,
> + ioaddr + msm_offset->core_dll_config_3);
> + writel_relaxed(msm_host->dll[index].dll_usr_ctl,
> + ioaddr + msm_offset->core_dll_usr_ctl);
> + }
> +
> + /*
> + * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
> + */
> + ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
> + : msm_offset->core_ddr_config_old;
> +
> + config = msm_host->dll[index].ddr_config;
> + writel_relaxed(config, ioaddr + ddr_cfg_offset);
> +
> + /* Set DLL_CONFIG_2 */
> + if (msm_host->use_14lpp_dll_reset) {
> + u32 mclk_freq;
> + int cycle_cnt;
> +
> + /*
> + * Only configure the mclk_freq in normal DLL init
> + * context. If the DLL init is coming from
> + * CX Collapse Exit context, the host->clock may be zero.
> + * The DLL_CONFIG_2 register has already been restored to
> + * proper value prior to getting here.
> + */
> + if (init_context == DLL_INIT_NORMAL) {
Guess what...enum mode only has a single possible value, so init_context
is DLL_INIT_NORMAL...
> + cycle_cnt = readl_relaxed(ioaddr +
> + msm_offset->core_dll_config_2)
> + & CORE_FLL_CYCLE_CNT ? 8 : 4;
> +
> + mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
> +
> + if (dll_clock < 100000000) {
> + pr_err("%s: %s: Non standard clk freq =%u\n",
> + mmc_hostname(mmc), __func__, dll_clock);
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> + config = (config & ~GENMASK(17, 10)) |
> + FIELD_PREP(GENMASK(17, 10), mclk_freq);
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> + }
> + /* wait for 5us before enabling DLL clock */
> + udelay(5);
> + }
> +
> + config = msm_host->dll[index].dll_config;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /* Wait for 52us */
> + spin_unlock_irqrestore(&host->lock, flags);
How come this isn't racy? What is the purpose of holding this spinlock
throughout this method and are you certain that whatever concurrent
execution might happen doesn't happen here?
> + usleep_range(60, 70);
> + spin_lock_irqsave(&host->lock, flags);
> +
> + /*
> + * Write 0 to DLL_RST bit of DLL_CONFIG register
> + * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
> + */
> + config &= ~CORE_DLL_RST;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + config &= ~CORE_DLL_PDN;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> + /* Write 1 to DLL_RST bit of DLL_CONFIG register */
> + config |= CORE_DLL_RST;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /* Write 0 to DLL_RST bit of DLL_CONFIG register */
> + config &= ~CORE_DLL_RST;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /* Set CORE_DLL_CLOCK_DISABLE to 0 */
> + if (msm_host->use_14lpp_dll_reset) {
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> + config &= ~CORE_DLL_CLOCK_DISABLE;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> + }
> +
> + /* Set DLL_EN bit to 1. */
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> + config |= CORE_DLL_EN;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /*
> + * Wait for 8000 input clock. Here we calculate the
> + * delay from fixed clock freq 192MHz, which turns out 42us.
> + */
> + spin_unlock_irqrestore(&host->lock, flags);
As above, how do we know that this isn't racy?
> + usleep_range(50, 60);
> + spin_lock_irqsave(&host->lock, flags);
> +
> + /* Set CK_OUT_EN bit to 1. */
> + config |= CORE_CK_OUT_EN;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /*
> + * Wait until DLL_LOCK bit of DLL_STATUS register
> + * becomes '1'.
> + */
> + rc = readl_relaxed_poll_timeout(ioaddr +
> + msm_offset->core_dll_status,
> + dll_lock,
> + dll_lock & CORE_DLL_LOCK,
> + 10,
> + 100);
> + if (rc == -ETIMEDOUT)
> + pr_err("%s: Unable to get DLL_LOCK, dll_status: 0x%08x\n",
Don't pr_err() when you have a struct device.
> + mmc_hostname(host->mmc), dll_lock);
> +
> +out:
> + if (core_vendor_spec & CORE_CLK_PWRSAVE) {
> + /* Reenable PWRSAVE as needed */
> + config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
> + config |= CORE_CLK_PWRSAVE;
> + writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
> + }
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> + return rc;
> +}
> +
> static void msm_hc_select_default(struct sdhci_host *host)
> {
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -927,6 +1139,24 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
> msm_hc_select_default(host);
> }
>
> +static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
> +{
> + if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104 ||
> + host->mmc->ios.timing == MMC_TIMING_MMC_HS400)
> + return sdhci_msm_configure_dll(host, init_context, HS400);
> +
> + return sdhci_msm_configure_dll(host, init_context, HS200);
> +}
> +
> +static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> + return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
> + msm_init_cm_dll(host);
So dll_config() calls init_dll() or init_cm_dll() which then might call
configure_dll(). Is it init or config?!
> +}
> +
> static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
> {
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -942,7 +1172,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
> * Retuning in HS400 (DDR mode) will fail, just reset the
> * tuning block and restore the saved tuning phase.
> */
> - ret = msm_init_cm_dll(host);
> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> if (ret)
> goto out;
>
> @@ -1030,7 +1260,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
> return ret;
> }
>
> -static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> +static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
> {
> struct mmc_host *mmc = host->mmc;
> u32 dll_status, config, ddr_cfg_offset;
> @@ -1053,7 +1283,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> ddr_cfg_offset = msm_offset->core_ddr_config;
> else
> ddr_cfg_offset = msm_offset->core_ddr_config_old;
> - writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
> +
> + if (msm_host->artanis_dll)
> + writel_relaxed(msm_host->dll[index].ddr_config, host->ioaddr + ddr_cfg_offset);
> + else
> + writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>
> if (mmc->ios.enhanced_strobe) {
> config = readl_relaxed(host->ioaddr +
> @@ -1122,7 +1356,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
> * Retuning in HS400 (DDR mode) will fail, just reset the
> * tuning block and restore the saved tuning phase.
> */
> - ret = msm_init_cm_dll(host);
> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> +
> if (ret)
> goto out;
>
> @@ -1142,7 +1377,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
> if (msm_host->use_cdclp533)
> ret = sdhci_msm_cdclp533_calibration(host);
> else
> - ret = sdhci_msm_cm_dll_sdc4_calibration(host);
> + ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
> out:
> pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
> __func__, ret);
> @@ -1185,7 +1420,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
> return 0;
>
> /* Reset the tuning block */
> - ret = msm_init_cm_dll(host);
> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> +
> if (ret)
> return ret;
>
> @@ -1227,6 +1463,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
> struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> const struct sdhci_msm_offset *msm_offset = msm_host->offset;
> u32 config;
> + bool hs400_tuning;
>
> if (!sdhci_msm_is_tuning_needed(host)) {
> msm_host->use_cdr = false;
> @@ -1256,17 +1493,23 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
> * - select MCLK/2 in VENDOR_SPEC
> * - program MCLK to 400MHz (or nearest supported) in GCC
> */
> - if (host->flags & SDHCI_HS400_TUNING) {
> + hs400_tuning = !!(host->flags & SDHCI_HS400_TUNING);
I don't see the reason for this change. If it actually impact the
behavior of the init/config dll, did you ensure that it doesn't impact
all currently supported targets?
Regards,
Bjorn
> + if (hs400_tuning) {
> sdhci_msm_hc_select_mode(host);
> msm_set_clock_rate_for_bus_mode(host, ios.clock, ios.timing);
> - host->flags &= ~SDHCI_HS400_TUNING;
> }
>
> retry:
> /* First of all reset the tuning block */
> - rc = msm_init_cm_dll(host);
> - if (rc)
> + rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> + if (rc) {
> + if (hs400_tuning)
> + host->flags &= ~SDHCI_HS400_TUNING;
> return rc;
> + }
> +
> + if (hs400_tuning)
> + host->flags &= ~SDHCI_HS400_TUNING;
>
> phase = 0;
> do {
> @@ -1297,6 +1540,8 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
> mmc_hostname(mmc));
> if (--tuning_seq_cnt) {
> tuned_phase_cnt = 0;
> + if (hs400_tuning)
> + host->flags |= SDHCI_HS400_TUNING;
> goto retry;
> }
> }
> @@ -1319,8 +1564,11 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
> dev_dbg(mmc_dev(mmc), "%s: Setting the tuning phase to %d\n",
> mmc_hostname(mmc), phase);
> } else {
> - if (--tuning_seq_cnt)
> + if (--tuning_seq_cnt) {
> + if (hs400_tuning)
> + host->flags |= SDHCI_HS400_TUNING;
> goto retry;
> + }
> /* Tuning failed */
> dev_dbg(mmc_dev(mmc), "%s: No tuning point found\n",
> mmc_hostname(mmc));
> @@ -1847,11 +2095,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
> return clk_round_rate(core_clk, ULONG_MAX);
> }
>
> -static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> -{
> - return SDHCI_MSM_MIN_CLOCK;
> -}
> -
> /*
> * __sdhci_msm_set_clock - sdhci_msm clock control.
> *
> --
> 2.34.1
>
On 12/18/2025 7:48 PM, Bjorn Andersson wrote:
> On Mon, Dec 15, 2025 at 05:30:09PM +0530, Ram Prakash Gupta wrote:
>> From: Sachin Gupta <quic_sachgupt@quicinc.com>
>>
>> With the current DLL sequence stability issues for data
>> transfer seen in HS400 and HS200 modes.
>>
>> "mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
>> data error 0"
>>
>> Rectify the DLL programming sequence as per latest hardware
>> programming guide
> You're talking about how things are unstable, but what the commit
> message completely fails to mention is that all this new spaghetti is
> conditional on msm_host->artanis_dll, so I believe this statement
> only applies to a single supported SoC?
>
> No where in this series do you mention that this only (?) applies to
> QDU1000.
This DLL change is required to work with SoCs having artanis DLL.
This applies to multiple targets but QDU1000 is what is picked up to
start with, and gradually after testing this with other artanis DLL
supported target, dt change will be updated for other targets.
And this doesn't apply only to QDU1000, this would go for SM8650, SM8750
and upcoming SoCs for SDC2 instance (SDCard) and other targets for SDC1
instance if supported.
I will further update the commit message in to show, artanis dll
support addition with this work.
>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> ---
>> drivers/mmc/host/sdhci-msm.c | 277 ++++++++++++++++++++++++++++++++---
>> 1 file changed, 260 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 1fcd92158bee..f459b40db41c 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -28,6 +28,7 @@
>> #define CORE_VERSION_MAJOR_SHIFT 28
>> #define CORE_VERSION_MAJOR_MASK (0xf << CORE_VERSION_MAJOR_SHIFT)
>> #define CORE_VERSION_MINOR_MASK 0xff
>> +#define SDHCI_MSM_MIN_V_7FF 0x6e
>>
>> #define CORE_MCI_GENERICS 0x70
>> #define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
>> @@ -119,7 +120,8 @@
>> #define CORE_PWRSAVE_DLL BIT(3)
>>
>> #define DDR_CONFIG_POR_VAL 0x80040873
>> -
>> +#define DLL_CONFIG_3_POR_VAL 0x10
>> +#define TCXO_FREQ 19200000
> If you read the current implementation of msm_init_cm_dll() you'll see
> that we don't hard code the XO clock...
Thanks for point out.I will remove this and use xo_clock.
>
>>
>> #define INVALID_TUNING_PHASE -1
>> #define SDHCI_MSM_MIN_CLOCK 400000
>> @@ -319,6 +321,15 @@ struct sdhci_msm_host {
>> bool artanis_dll;
>> };
>>
>> +enum dll_init_context {
>> + DLL_INIT_NORMAL,
> What is the purpose of a single-entry enum to capture the value 0?
I will remove this and make the further changes in code accordingly.
>
>> +};
>> +
>> +enum mode {
>> + HS400, // equivalent to SDR104 mode for DLL.
>> + HS200, // equivalent to SDR50 mode for DLL.
> This is effectively a bool, with a terribly generic name.
ok, will update to HS400_DLL_CONFIG_VAL and HS200_DLL_CONFIG_VAL if this
is ok.
>
>> +};
>> +
>> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>> {
>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -805,6 +816,207 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>> return 0;
>> }
>>
>> +static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>> +{
>> + return SDHCI_MSM_MIN_CLOCK;
>> +}
>> +
>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
>> + struct mmc_ios ios = host->mmc->ios;
>> + unsigned int sup_clk;
>> +
>> + if (req_clk < sdhci_msm_get_min_clock(host))
>> + return sdhci_msm_get_min_clock(host);
>> +
>> + sup_clk = clk_get_rate(core_clk);
>> +
>> + if (ios.timing == MMC_TIMING_MMC_HS400 ||
>> + host->flags & SDHCI_HS400_TUNING)
>> + sup_clk = sup_clk / 2;
>> +
>> + return sup_clk;
> Isn't sup_clk the same as host->mmc->actual_clock? Isn't this already
> calculated for you?
Thanks for pointing out, I will use this. And remove this code, will update
this in next patchset.
>
>> +}
>> +
>> +/* Initialize the DLL (Programmable Delay Line) */
> So now we're going to have two functions that are named pretty much the
> same and look pretty similar?
>
> With the main difference that the slightly more generically named
> sdhci_msm_configure_dll() is only called if we have a artanis_dll,
> otherwise msm_init_cm_dll() is used?
They appear similar as artanis_dll have multiple common sequence
with the previous one. The new addition in the existing function
was adding multiple if/else checks making the function cluttered.
This approach keeps the implementation cleaner.
>> +static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
>> + init_context, enum mode index)
>> +{
> Large parts of this function is duplicated from msm_init_cm_dll(). This
> isn't okay.
Right it may look like a duplicate but the new required sequence shares many
steps with the existing sequence but the new addition in the existing function
was adding multiple if/else checks making the function cluttered and hard to
read. To keep the code clean, a separate function was created instead.
>
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> + const struct sdhci_msm_offset *msm_offset = msm_host->offset;
>> + struct mmc_host *mmc = host->mmc;
>> + u32 ddr_cfg_offset, core_vendor_spec, config;
>> + void __iomem *ioaddr = host->ioaddr;
>> + unsigned long flags, dll_clock;
>> + int rc = 0;
>> + u32 dll_lock;
>> +
>> + dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
>> + spin_lock_irqsave(&host->lock, flags);
>> +
>> + core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
>> +
>> + /*
>> + * Always disable PWRSAVE during the DLL power
>> + * up regardless of its current setting.
>> + */
>> + core_vendor_spec &= ~CORE_CLK_PWRSAVE;
>> + writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
>> +
>> + if (msm_host->use_14lpp_dll_reset) {
>> + /* Disable CK_OUT */
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>> + config &= ~CORE_CK_OUT_EN;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /* Disable the DLL clock */
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>> + config |= CORE_DLL_CLOCK_DISABLE;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>> + }
>> +
>> + /*
>> + * Write 1 to DLL_RST bit of DLL_CONFIG register
>> + * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
>> + */
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>> + config |= (CORE_DLL_RST | CORE_DLL_PDN);
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /*
>> + * Configure DLL_CONFIG_3 and USER_CTRL
>> + * (Only applicable for 7FF projects).
> What is a 7FF project?
This refers to 7nm technode.
>
>> + */
>> + if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
>> + writel_relaxed(msm_host->dll[index].dll_config_3,
>> + ioaddr + msm_offset->core_dll_config_3);
>> + writel_relaxed(msm_host->dll[index].dll_usr_ctl,
>> + ioaddr + msm_offset->core_dll_usr_ctl);
>> + }
>> +
>> + /*
>> + * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
>> + */
>> + ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
>> + : msm_offset->core_ddr_config_old;
>> +
>> + config = msm_host->dll[index].ddr_config;
>> + writel_relaxed(config, ioaddr + ddr_cfg_offset);
>> +
>> + /* Set DLL_CONFIG_2 */
>> + if (msm_host->use_14lpp_dll_reset) {
>> + u32 mclk_freq;
>> + int cycle_cnt;
>> +
>> + /*
>> + * Only configure the mclk_freq in normal DLL init
>> + * context. If the DLL init is coming from
>> + * CX Collapse Exit context, the host->clock may be zero.
>> + * The DLL_CONFIG_2 register has already been restored to
>> + * proper value prior to getting here.
>> + */
>> + if (init_context == DLL_INIT_NORMAL) {
> Guess what...enum mode only has a single possible value, so init_context
> is DLL_INIT_NORMAL...
I will remove this completely and update the code.
>> + cycle_cnt = readl_relaxed(ioaddr +
>> + msm_offset->core_dll_config_2)
>> + & CORE_FLL_CYCLE_CNT ? 8 : 4;
>> +
>> + mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
>> +
>> + if (dll_clock < 100000000) {
>> + pr_err("%s: %s: Non standard clk freq =%u\n",
>> + mmc_hostname(mmc), __func__, dll_clock);
>> + rc = -EINVAL;
>> + goto out;
>> + }
>> +
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>> + config = (config & ~GENMASK(17, 10)) |
>> + FIELD_PREP(GENMASK(17, 10), mclk_freq);
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>> + }
>> + /* wait for 5us before enabling DLL clock */
>> + udelay(5);
>> + }
>> +
>> + config = msm_host->dll[index].dll_config;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /* Wait for 52us */
>> + spin_unlock_irqrestore(&host->lock, flags);
> How come this isn't racy? What is the purpose of holding this spinlock
> throughout this method and are you certain that whatever concurrent
> execution might happen doesn't happen here?
Since the delay needed is 60u, usleep_range was added, and to avoid sleep
holding lock, unlock and lock were used. And I got your point. But is it ok
to use udelay here for 60u second? I don't see any other way.
>
>> + usleep_range(60, 70);
>> + spin_lock_irqsave(&host->lock, flags);
>> +
>> + /*
>> + * Write 0 to DLL_RST bit of DLL_CONFIG register
>> + * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
>> + */
>> + config &= ~CORE_DLL_RST;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + config &= ~CORE_DLL_PDN;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> + /* Write 1 to DLL_RST bit of DLL_CONFIG register */
>> + config |= CORE_DLL_RST;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /* Write 0 to DLL_RST bit of DLL_CONFIG register */
>> + config &= ~CORE_DLL_RST;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /* Set CORE_DLL_CLOCK_DISABLE to 0 */
>> + if (msm_host->use_14lpp_dll_reset) {
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>> + config &= ~CORE_DLL_CLOCK_DISABLE;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>> + }
>> +
>> + /* Set DLL_EN bit to 1. */
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>> + config |= CORE_DLL_EN;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /*
>> + * Wait for 8000 input clock. Here we calculate the
>> + * delay from fixed clock freq 192MHz, which turns out 42us.
>> + */
>> + spin_unlock_irqrestore(&host->lock, flags);
> As above, how do we know that this isn't racy?
Will update as per other review comment about race.
>
>> + usleep_range(50, 60);
>> + spin_lock_irqsave(&host->lock, flags);
>> +
>> + /* Set CK_OUT_EN bit to 1. */
>> + config |= CORE_CK_OUT_EN;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /*
>> + * Wait until DLL_LOCK bit of DLL_STATUS register
>> + * becomes '1'.
>> + */
>> + rc = readl_relaxed_poll_timeout(ioaddr +
>> + msm_offset->core_dll_status,
>> + dll_lock,
>> + dll_lock & CORE_DLL_LOCK,
>> + 10,
>> + 100);
>> + if (rc == -ETIMEDOUT)
>> + pr_err("%s: Unable to get DLL_LOCK, dll_status: 0x%08x\n",
> Don't pr_err() when you have a struct device.
ok, will update to dev_err.
>
>> + mmc_hostname(host->mmc), dll_lock);
>> +
>> +out:
>> + if (core_vendor_spec & CORE_CLK_PWRSAVE) {
>> + /* Reenable PWRSAVE as needed */
>> + config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
>> + config |= CORE_CLK_PWRSAVE;
>> + writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
>> + }
>> +
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + return rc;
>> +}
>> +
>> static void msm_hc_select_default(struct sdhci_host *host)
>> {
>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -927,6 +1139,24 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
>> msm_hc_select_default(host);
>> }
>>
>> +static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
>> +{
>> + if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104 ||
>> + host->mmc->ios.timing == MMC_TIMING_MMC_HS400)
>> + return sdhci_msm_configure_dll(host, init_context, HS400);
>> +
>> + return sdhci_msm_configure_dll(host, init_context, HS200);
>> +}
>> +
>> +static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> + return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
>> + msm_init_cm_dll(host);
> So dll_config() calls init_dll() or init_cm_dll() which then might call
> configure_dll(). Is it init or config?!
init is what is needed, but I see this can be improved.
I will use dll_config -> init_dll, will remove the other function calls.
>
>> +}
>> +
>> static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>> {
>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -942,7 +1172,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>> * Retuning in HS400 (DDR mode) will fail, just reset the
>> * tuning block and restore the saved tuning phase.
>> */
>> - ret = msm_init_cm_dll(host);
>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> if (ret)
>> goto out;
>>
>> @@ -1030,7 +1260,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>> return ret;
>> }
>>
>> -static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>> +static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
>> {
>> struct mmc_host *mmc = host->mmc;
>> u32 dll_status, config, ddr_cfg_offset;
>> @@ -1053,7 +1283,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>> ddr_cfg_offset = msm_offset->core_ddr_config;
>> else
>> ddr_cfg_offset = msm_offset->core_ddr_config_old;
>> - writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>> +
>> + if (msm_host->artanis_dll)
>> + writel_relaxed(msm_host->dll[index].ddr_config, host->ioaddr + ddr_cfg_offset);
>> + else
>> + writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>>
>> if (mmc->ios.enhanced_strobe) {
>> config = readl_relaxed(host->ioaddr +
>> @@ -1122,7 +1356,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>> * Retuning in HS400 (DDR mode) will fail, just reset the
>> * tuning block and restore the saved tuning phase.
>> */
>> - ret = msm_init_cm_dll(host);
>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> +
>> if (ret)
>> goto out;
>>
>> @@ -1142,7 +1377,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>> if (msm_host->use_cdclp533)
>> ret = sdhci_msm_cdclp533_calibration(host);
>> else
>> - ret = sdhci_msm_cm_dll_sdc4_calibration(host);
>> + ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
>> out:
>> pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
>> __func__, ret);
>> @@ -1185,7 +1420,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
>> return 0;
>>
>> /* Reset the tuning block */
>> - ret = msm_init_cm_dll(host);
>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> +
>> if (ret)
>> return ret;
>>
>> @@ -1227,6 +1463,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> const struct sdhci_msm_offset *msm_offset = msm_host->offset;
>> u32 config;
>> + bool hs400_tuning;
>>
>> if (!sdhci_msm_is_tuning_needed(host)) {
>> msm_host->use_cdr = false;
>> @@ -1256,17 +1493,23 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> * - select MCLK/2 in VENDOR_SPEC
>> * - program MCLK to 400MHz (or nearest supported) in GCC
>> */
>> - if (host->flags & SDHCI_HS400_TUNING) {
>> + hs400_tuning = !!(host->flags & SDHCI_HS400_TUNING);
> I don't see the reason for this change. If it actually impact the
> behavior of the init/config dll, did you ensure that it doesn't impact
> all currently supported targets?
>
> Regards,
> Bjorn
I had changed this to get right sup_clk value by retaining the tuning flag,
but will use actual_clk as suggested in top review comment and this change won't
be required anymore.
Thanks,
Ram
>
>> + if (hs400_tuning) {
>> sdhci_msm_hc_select_mode(host);
>> msm_set_clock_rate_for_bus_mode(host, ios.clock, ios.timing);
>> - host->flags &= ~SDHCI_HS400_TUNING;
>> }
>>
>> retry:
>> /* First of all reset the tuning block */
>> - rc = msm_init_cm_dll(host);
>> - if (rc)
>> + rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> + if (rc) {
>> + if (hs400_tuning)
>> + host->flags &= ~SDHCI_HS400_TUNING;
>> return rc;
>> + }
>> +
>> + if (hs400_tuning)
>> + host->flags &= ~SDHCI_HS400_TUNING;
>>
>> phase = 0;
>> do {
>> @@ -1297,6 +1540,8 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> mmc_hostname(mmc));
>> if (--tuning_seq_cnt) {
>> tuned_phase_cnt = 0;
>> + if (hs400_tuning)
>> + host->flags |= SDHCI_HS400_TUNING;
>> goto retry;
>> }
>> }
>> @@ -1319,8 +1564,11 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> dev_dbg(mmc_dev(mmc), "%s: Setting the tuning phase to %d\n",
>> mmc_hostname(mmc), phase);
>> } else {
>> - if (--tuning_seq_cnt)
>> + if (--tuning_seq_cnt) {
>> + if (hs400_tuning)
>> + host->flags |= SDHCI_HS400_TUNING;
>> goto retry;
>> + }
>> /* Tuning failed */
>> dev_dbg(mmc_dev(mmc), "%s: No tuning point found\n",
>> mmc_hostname(mmc));
>> @@ -1847,11 +2095,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
>> return clk_round_rate(core_clk, ULONG_MAX);
>> }
>>
>> -static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>> -{
>> - return SDHCI_MSM_MIN_CLOCK;
>> -}
>> -
>> /*
>> * __sdhci_msm_set_clock - sdhci_msm clock control.
>> *
>> --
>> 2.34.1
>>
Hi Ram,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on krzk-dt/for-next v6.19-rc1]
[cannot apply to robh/for-next krzk/for-next ulf-hansson-mmc-mirror/next next-20251216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ram-Prakash-Gupta/dt-bindings-mmc-Add-dll-presets-values-for-HS400-and-HS200-modes/20251215-200814
base: linus/master
patch link: https://lore.kernel.org/r/20251215120009.3877889-6-quic_rampraka%40quicinc.com
patch subject: [PATCH v6 5/5] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20251217/202512171135.hzSvlDE9-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251217/202512171135.hzSvlDE9-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202512171135.hzSvlDE9-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:31,
from arch/x86/include/asm/bug.h:193,
from arch/x86/include/asm/alternative.h:9,
from arch/x86/include/asm/barrier.h:5,
from include/linux/list.h:11,
from include/linux/module.h:12,
from drivers/mmc/host/sdhci-msm.c:8:
drivers/mmc/host/sdhci-msm.c: In function 'sdhci_msm_configure_dll':
>> include/linux/kern_levels.h:5:25: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:484:25: note: in definition of macro 'printk_index_wrap'
484 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:555:9: note: in expansion of macro 'printk'
555 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:555:16: note: in expansion of macro 'KERN_ERR'
555 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
drivers/mmc/host/sdhci-msm.c:930:33: note: in expansion of macro 'pr_err'
930 | pr_err("%s: %s: Non standard clk freq =%u\n",
| ^~~~~~
--
In file included from include/asm-generic/bug.h:31,
from arch/x86/include/asm/bug.h:193,
from arch/x86/include/asm/alternative.h:9,
from arch/x86/include/asm/barrier.h:5,
from include/linux/list.h:11,
from include/linux/module.h:12,
from sdhci-msm.c:8:
sdhci-msm.c: In function 'sdhci_msm_configure_dll':
>> include/linux/kern_levels.h:5:25: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:484:25: note: in definition of macro 'printk_index_wrap'
484 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:555:9: note: in expansion of macro 'printk'
555 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
11 | #define KERN_ERR KERN_SOH "3" /* error conditions */
| ^~~~~~~~
include/linux/printk.h:555:16: note: in expansion of macro 'KERN_ERR'
555 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~
sdhci-msm.c:930:33: note: in expansion of macro 'pr_err'
930 | pr_err("%s: %s: Non standard clk freq =%u\n",
| ^~~~~~
vim +5 include/linux/kern_levels.h
314ba3520e513a Joe Perches 2012-07-30 4
04d2c8c83d0e3a Joe Perches 2012-07-30 @5 #define KERN_SOH "\001" /* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30 6 #define KERN_SOH_ASCII '\001'
04d2c8c83d0e3a Joe Perches 2012-07-30 7
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2026 Red Hat, Inc.