| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7317bfd0-8d81-42f4-a22e-d6e1be6b5e5b@quicinc.com>
Date: Thu, 22 Jan 2026 18:59:53 +0530
From: Ram Prakash Gupta <quic_rampraka@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: Ulf Hansson <ulf.hansson@...aro.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Adrian Hunter
<adrian.hunter@...el.com>, <linux-mmc@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <dmitry.baryshkov@....qualcomm.com>
Subject: Re: [PATCH v6 5/5] mmc: sdhci-msm: Rectify DLL programming sequence
for SDCC
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@...cinc.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@...cinc.com>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@...cinc.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
>>
Powered by blists - more mailing lists