[PATCH V3 0/4] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[PATCH V3 0/4] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Sachin Gupta]

From: sachgupt <quic_sachgupt@quicinc.com>

With the current DLL sequence stability issues are seen in
HS400 and HS200 mode for data transfers.

Rectify the DLL programming sequence as per latest hardware
programming guide and also incorporate support for HS200 and
HS400 DLL settings using the device tree.

Changes from v2:
1. Addressed Dmitry Baryshkov comments:
   a. Regarding TCXO frequency.
   b. Regarding clock rate.
   c. regarding checkpatch.

Changes from v1:
1. Addressed Tengfei Fan comment, added missing semicolocon
   in sdhci_msm_host structure.

Sachin Gupta (4):
  dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes
  mmc: sdhci-msm: Add core_major, minor to msm_host structure
  mmc: sdhci-msm: Add Device tree parsing logic for DLL settings
  mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

 .../devicetree/bindings/mmc/sdhci-msm.yaml    |   5 +
 drivers/mmc/host/sdhci-msm.c                  | 362 +++++++++++++++++-
 2 files changed, 349 insertions(+), 18 deletions(-)

-- 
2.17.1

[PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Sachin Gupta]

Document the 'dll-hsr-list' property for MMC device tree bindings.
The 'dll-hsr-list' property defines the DLL configurations for HS400
and HS200 modes.

Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
---
 Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
index 8b393e26e025..65dc3053df75 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
@@ -133,6 +133,11 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: platform specific settings for DLL_CONFIG reg.
 
+  qcom,dll-hsr-list:
+    maxItems: 10
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: platform specific settings for DLL registers.
+
   iommus:
     minItems: 1
     maxItems: 8
-- 
2.17.1

[PATCH V3 2/4] mmc: sdhci-msm: Add core_major, minor to msm_host structure

[Sachin Gupta]

This change adds the core_major and core_minor variables to
the msm_host structure, allowing these variables to be
accessed more easily throughout the msm_host context.
This update is necessary for an upcoming follow-up patch.

Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
---
 drivers/mmc/host/sdhci-msm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index e00208535bd1..2a5e588779fc 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -273,6 +273,8 @@ struct sdhci_msm_host {
 	bool tuning_done;
 	bool calibration_done;
 	u8 saved_tuning_phase;
+	u8 core_major;
+	u16 core_minor;
 	bool use_cdclp533;
 	u32 curr_pwr_state;
 	u32 curr_io_level;
@@ -2557,6 +2559,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	core_major = (core_version & CORE_VERSION_MAJOR_MASK) >>
 		      CORE_VERSION_MAJOR_SHIFT;
 	core_minor = core_version & CORE_VERSION_MINOR_MASK;
+
+	msm_host->core_major = core_major;
+	msm_host->core_minor = core_minor;
+
 	dev_dbg(&pdev->dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
 		core_version, core_major, core_minor);
 
-- 
2.17.1

[PATCH V3 3/4] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Sachin Gupta]

This update introduces the capability to configure HS200
and HS400 DLL settings via the device tree and parsing it.

Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
---
 drivers/mmc/host/sdhci-msm.c | 86 ++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 2a5e588779fc..cc7756a59c55 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -256,6 +256,19 @@ struct sdhci_msm_variant_info {
 	const struct sdhci_msm_offset *offset;
 };
 
+/*
+ * DLL registers which needs be programmed with HSR settings.
+ * Add any new register only at the end and don't change the
+ * sequence.
+ */
+struct sdhci_msm_dll {
+	u32 dll_config[2];
+	u32 dll_config_2[2];
+	u32 dll_config_3[2];
+	u32 dll_usr_ctl[2];
+	u32 ddr_config[2];
+};
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -264,6 +277,7 @@ struct sdhci_msm_host {
 	struct clk *xo_clk;	/* TCXO clk needed for FLL feature of cm_dll*/
 	/* core, iface, cal and sleep clocks */
 	struct clk_bulk_data bulk_clks[4];
+	struct sdhci_msm_dll dll;
 #ifdef CONFIG_MMC_CRYPTO
 	struct qcom_ice *ice;
 #endif
@@ -292,6 +306,7 @@ struct sdhci_msm_host {
 	u32 dll_config;
 	u32 ddr_config;
 	bool vqmmc_enabled;
+	bool artanis_dll;
 };
 
 static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
@@ -2400,6 +2415,74 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
 	return ret;
 }
 
+static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
+				  u32 **bw_vecs, int *len)
+{
+	struct device_node *np = dev->of_node;
+	u32 *arr = NULL;
+	int ret = 0;
+	int sz;
+
+	if (!np)
+		return -ENODEV;
+
+	if (!of_get_property(np, prop_name, &sz))
+		return -EINVAL;
+
+	sz = sz / sizeof(*arr);
+	if (sz <= 0)
+		return -EINVAL;
+
+	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
+	if (!arr)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(np, prop_name, arr, sz);
+	if (ret) {
+		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
+		*len = 0;
+		return ret;
+	}
+
+	*bw_vecs = arr;
+	*len = sz;
+	ret = 0;
+
+	return ret;
+}
+
+static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
+{
+	int dll_table_len, dll_reg_count;
+	u32 *dll_table = NULL;
+	int i;
+
+	msm_host->artanis_dll = false;
+
+	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
+				   &dll_table, &dll_table_len))
+		return -EINVAL;
+
+	dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
+
+	if (dll_table_len != dll_reg_count) {
+		dev_err(dev, "Number of HSR entries are not matching\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < 2; i++) {
+		msm_host->dll.dll_config[i] = dll_table[i];
+		msm_host->dll.dll_config_2[i] = dll_table[i + 1];
+		msm_host->dll.dll_config_3[i] = dll_table[i + 2];
+		msm_host->dll.dll_usr_ctl[i] = dll_table[i + 3];
+		msm_host->dll.ddr_config[i] = dll_table[i + 4];
+	}
+
+	msm_host->artanis_dll = true;
+
+	return 0;
+}
+
 static int sdhci_msm_probe(struct platform_device *pdev)
 {
 	struct sdhci_host *host;
@@ -2446,6 +2529,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
 
+	if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
+		goto pltfm_free;
+
 	ret = sdhci_msm_gcc_reset(&pdev->dev, host);
 	if (ret)
 		goto pltfm_free;
-- 
2.17.1

[PATCH V3 4/4] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Sachin Gupta]

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>
---
 drivers/mmc/host/sdhci-msm.c | 270 ++++++++++++++++++++++++++++++++---
 1 file changed, 252 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index cc7756a59c55..17f17a635d83 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)
@@ -118,7 +119,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
@@ -309,6 +311,16 @@ struct sdhci_msm_host {
 	bool artanis_dll;
 };
 
+enum dll_init_context {
+	DLL_INIT_NORMAL,
+	DLL_INIT_FROM_CX_COLLAPSE_EXIT,
+};
+
+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);
@@ -793,6 +805,211 @@ 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;
+	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 (host->clock != msm_host->clk_rate)
+		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, wait_cnt = 50;
+
+	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.dll_config_3[index],
+			       ioaddr + msm_offset->core_dll_config_3);
+		writel_relaxed(msm_host->dll.dll_usr_ctl[index],
+			       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.ddr_config[index];
+	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);
+	}
+
+	/*
+	 * Update the lower two bytes of DLL_CONFIG only with
+	 * HSR values. Since these are the static settings.
+	 */
+	config = (readl_relaxed(ioaddr + msm_offset->core_dll_config));
+	config |= (msm_host->dll.dll_config[index] & 0xffff);
+	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+	/* Wait for 52us */
+	spin_unlock_irqrestore(&host->lock, flags);
+	udelay(60);
+	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);
+	udelay(50);
+	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'.
+	 */
+	while (!(readl_relaxed(ioaddr + msm_offset->core_dll_status) &
+		 CORE_DLL_LOCK)) {
+		/* max. wait for 50us sec for LOCK bit to be set */
+		if (--wait_cnt == 0) {
+			dev_err(mmc_dev(mmc), "%s: DLL failed to LOCK\n",
+				mmc_hostname(mmc));
+			rc = -ETIMEDOUT;
+			goto out;
+		}
+		/* wait for 1us before polling again */
+		udelay(1);
+	}
+
+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);
@@ -915,14 +1132,31 @@ 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);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
 	u32 config, calib_done;
 	int ret;
-	const struct sdhci_msm_offset *msm_offset =
-					msm_host->offset;
 
 	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
 
@@ -930,7 +1164,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;
 
@@ -1018,7 +1252,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;
@@ -1041,7 +1275,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.ddr_config[index], 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 +
@@ -1098,11 +1336,10 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
 {
 	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;
-	int ret;
 	u32 config;
-	const struct sdhci_msm_offset *msm_offset =
-					msm_host->offset;
+	int ret;
 
 	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
 
@@ -1110,7 +1347,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;
 
@@ -1130,7 +1368,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);
@@ -1169,7 +1407,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;
 
@@ -1238,7 +1477,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 
 retry:
 	/* First of all reset the tuning block */
-	rc = msm_init_cm_dll(host);
+	rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
 	if (rc)
 		return rc;
 
@@ -1767,11 +2006,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.17.1

Re: [PATCH V3 2/4] mmc: sdhci-msm: Add core_major, minor to msm_host structure

[Dmitry Baryshkov]

On Wed, Jan 22, 2025 at 03:17:05PM +0530, Sachin Gupta wrote:
> This change adds the core_major and core_minor variables to

Please see Documentation/process/submitting-patches.rst, look for "[This
patch] makes xyzzy do frotz", then update your internal documentation so
that other engineers stop making the same mistake.

> the msm_host structure, allowing these variables to be
> accessed more easily throughout the msm_host context.
> This update is necessary for an upcoming follow-up patch.
> 
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> ---
>  drivers/mmc/host/sdhci-msm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index e00208535bd1..2a5e588779fc 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -273,6 +273,8 @@ struct sdhci_msm_host {
>  	bool tuning_done;
>  	bool calibration_done;
>  	u8 saved_tuning_phase;
> +	u8 core_major;
> +	u16 core_minor;
>  	bool use_cdclp533;
>  	u32 curr_pwr_state;
>  	u32 curr_io_level;
> @@ -2557,6 +2559,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  	core_major = (core_version & CORE_VERSION_MAJOR_MASK) >>
>  		      CORE_VERSION_MAJOR_SHIFT;
>  	core_minor = core_version & CORE_VERSION_MINOR_MASK;
> +
> +	msm_host->core_major = core_major;
> +	msm_host->core_minor = core_minor;
> +
>  	dev_dbg(&pdev->dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
>  		core_version, core_major, core_minor);
>  
> -- 
> 2.17.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH V3 3/4] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Dmitry Baryshkov]

On Wed, Jan 22, 2025 at 03:17:06PM +0530, Sachin Gupta wrote:
> This update introduces the capability to configure HS200
> and HS400 DLL settings via the device tree and parsing it.
> 
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> ---
>  drivers/mmc/host/sdhci-msm.c | 86 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 2a5e588779fc..cc7756a59c55 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -256,6 +256,19 @@ struct sdhci_msm_variant_info {
>  	const struct sdhci_msm_offset *offset;
>  };
>  
> +/*
> + * DLL registers which needs be programmed with HSR settings.
> + * Add any new register only at the end and don't change the
> + * sequence.
> + */
> +struct sdhci_msm_dll {
> +	u32 dll_config[2];
> +	u32 dll_config_2[2];
> +	u32 dll_config_3[2];
> +	u32 dll_usr_ctl[2];
> +	u32 ddr_config[2];
> +};
> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -264,6 +277,7 @@ struct sdhci_msm_host {
>  	struct clk *xo_clk;	/* TCXO clk needed for FLL feature of cm_dll*/
>  	/* core, iface, cal and sleep clocks */
>  	struct clk_bulk_data bulk_clks[4];
> +	struct sdhci_msm_dll dll;
>  #ifdef CONFIG_MMC_CRYPTO
>  	struct qcom_ice *ice;
>  #endif
> @@ -292,6 +306,7 @@ struct sdhci_msm_host {
>  	u32 dll_config;
>  	u32 ddr_config;
>  	bool vqmmc_enabled;
> +	bool artanis_dll;
>  };
>  
>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -2400,6 +2415,74 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>  	return ret;
>  }
>  
> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
> +				  u32 **bw_vecs, int *len)

It just reads an array from the DT, please rename the bw_vecs param
which is inaccurate in this case.

> +{
> +	struct device_node *np = dev->of_node;
> +	u32 *arr = NULL;
> +	int ret = 0;
> +	int sz;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	if (!of_get_property(np, prop_name, &sz))
> +		return -EINVAL;
> +
> +	sz = sz / sizeof(*arr);
> +	if (sz <= 0)
> +		return -EINVAL;
> +
> +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> +	if (!arr)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
> +	if (ret) {
> +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> +		*len = 0;
> +		return ret;
> +	}
> +
> +	*bw_vecs = arr;
> +	*len = sz;
> +	ret = 0;
> +
> +	return ret;
> +}
> +
> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
> +{
> +	int dll_table_len, dll_reg_count;
> +	u32 *dll_table = NULL;
> +	int i;
> +
> +	msm_host->artanis_dll = false;
> +
> +	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
> +				   &dll_table, &dll_table_len))
> +		return -EINVAL;
> +
> +	dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
> +
> +	if (dll_table_len != dll_reg_count) {
> +		dev_err(dev, "Number of HSR entries are not matching\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < 2; i++) {
> +		msm_host->dll.dll_config[i] = dll_table[i];
> +		msm_host->dll.dll_config_2[i] = dll_table[i + 1];
> +		msm_host->dll.dll_config_3[i] = dll_table[i + 2];
> +		msm_host->dll.dll_usr_ctl[i] = dll_table[i + 3];
> +		msm_host->dll.ddr_config[i] = dll_table[i + 4];
> +	}
> +
> +	msm_host->artanis_dll = true;

And the pointer to dll_table is lost, lingering for the driver lifetime.
Please drop the devm_ part and kfree() it once it is not used anymore.

> +
> +	return 0;
> +}
> +
>  static int sdhci_msm_probe(struct platform_device *pdev)
>  {
>  	struct sdhci_host *host;
> @@ -2446,6 +2529,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  
>  	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>  
> +	if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
> +		goto pltfm_free;
> +
>  	ret = sdhci_msm_gcc_reset(&pdev->dev, host);
>  	if (ret)
>  		goto pltfm_free;
> -- 
> 2.17.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH V3 4/4] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Dmitry Baryshkov]

On Wed, Jan 22, 2025 at 03:17:07PM +0530, Sachin Gupta wrote:
> 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>
> ---
>  drivers/mmc/host/sdhci-msm.c | 270 ++++++++++++++++++++++++++++++++---
>  1 file changed, 252 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index cc7756a59c55..17f17a635d83 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)
> @@ -118,7 +119,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
> @@ -309,6 +311,16 @@ struct sdhci_msm_host {
>  	bool artanis_dll;
>  };
>  
> +enum dll_init_context {
> +	DLL_INIT_NORMAL,
> +	DLL_INIT_FROM_CX_COLLAPSE_EXIT,
> +};
> +
> +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);
> @@ -793,6 +805,211 @@ 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;
> +	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 (host->clock != msm_host->clk_rate)
> +		sup_clk = sup_clk / 2;

Please resolve previous discussions before sending new versions. Just
sending a response and then sending next iteration of the patchset is
not a proper way to communicate.

NAK until the discussion is resolved in the previous thread.

> +
> +	return sup_clk;
> +}
> +

-- 
With best wishes
Dmitry

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Krzysztof Kozlowski]

On 22/01/2025 10:47, Sachin Gupta wrote:
> Document the 'dll-hsr-list' property for MMC device tree bindings.
> The 'dll-hsr-list' property defines the DLL configurations for HS400
> and HS200 modes.
> 
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> index 8b393e26e025..65dc3053df75 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> @@ -133,6 +133,11 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: platform specific settings for DLL_CONFIG reg.
>  
> +  qcom,dll-hsr-list:
> +    maxItems: 10
> +    $ref: /schemas/types.yaml#/definitions/uint32

uint32 has only one item. Anyway, there is already DLL there, so don't
duplicate or explain why this is different. Explain also why this is not
deducible from the compatible.

Please provide here link to DTS user so we can validate how you use it.


Best regards,
Krzysztof

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Rob Herring (Arm)]

On Wed, 22 Jan 2025 15:17:04 +0530, Sachin Gupta wrote:
> Document the 'dll-hsr-list' property for MMC device tree bindings.
> The 'dll-hsr-list' property defines the DLL configurations for HS400
> and HS200 modes.
> 
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml: properties:qcom,dll-hsr-list:maxItems: False schema does not allow 10
	hint: Scalar properties should not have array keywords
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250122094707.24859-2-quic_sachgupt@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH V3 3/4] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Ram Prakash Gupta]

Hi Dmitry,

As updated in [PATCH V3 2/2] of this series, I have started now to continue
this work. Will address your comment next.

Thanks,
Ram

On 1/22/2025 3:27 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 22, 2025 at 03:17:06PM +0530, Sachin Gupta wrote:
>> This update introduces the capability to configure HS200
>> and HS400 DLL settings via the device tree and parsing it.
>>
>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 86 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 86 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 2a5e588779fc..cc7756a59c55 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -256,6 +256,19 @@ struct sdhci_msm_variant_info {
>>  	const struct sdhci_msm_offset *offset;
>>  };
>>  
>> +/*
>> + * DLL registers which needs be programmed with HSR settings.
>> + * Add any new register only at the end and don't change the
>> + * sequence.
>> + */
>> +struct sdhci_msm_dll {
>> +	u32 dll_config[2];
>> +	u32 dll_config_2[2];
>> +	u32 dll_config_3[2];
>> +	u32 dll_usr_ctl[2];
>> +	u32 ddr_config[2];
>> +};
>> +
>>  struct sdhci_msm_host {
>>  	struct platform_device *pdev;
>>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
>> @@ -264,6 +277,7 @@ struct sdhci_msm_host {
>>  	struct clk *xo_clk;	/* TCXO clk needed for FLL feature of cm_dll*/
>>  	/* core, iface, cal and sleep clocks */
>>  	struct clk_bulk_data bulk_clks[4];
>> +	struct sdhci_msm_dll dll;
>>  #ifdef CONFIG_MMC_CRYPTO
>>  	struct qcom_ice *ice;
>>  #endif
>> @@ -292,6 +306,7 @@ struct sdhci_msm_host {
>>  	u32 dll_config;
>>  	u32 ddr_config;
>>  	bool vqmmc_enabled;
>> +	bool artanis_dll;
>>  };
>>  
>>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>> @@ -2400,6 +2415,74 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>>  	return ret;
>>  }
>>  
>> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
>> +				  u32 **bw_vecs, int *len)
> It just reads an array from the DT, please rename the bw_vecs param
> which is inaccurate in this case.
>
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	u32 *arr = NULL;
>> +	int ret = 0;
>> +	int sz;
>> +
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	if (!of_get_property(np, prop_name, &sz))
>> +		return -EINVAL;
>> +
>> +	sz = sz / sizeof(*arr);
>> +	if (sz <= 0)
>> +		return -EINVAL;
>> +
>> +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
>> +	if (!arr)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
>> +	if (ret) {
>> +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
>> +		*len = 0;
>> +		return ret;
>> +	}
>> +
>> +	*bw_vecs = arr;
>> +	*len = sz;
>> +	ret = 0;
>> +
>> +	return ret;
>> +}
>> +
>> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
>> +{
>> +	int dll_table_len, dll_reg_count;
>> +	u32 *dll_table = NULL;
>> +	int i;
>> +
>> +	msm_host->artanis_dll = false;
>> +
>> +	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
>> +				   &dll_table, &dll_table_len))
>> +		return -EINVAL;
>> +
>> +	dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
>> +
>> +	if (dll_table_len != dll_reg_count) {
>> +		dev_err(dev, "Number of HSR entries are not matching\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < 2; i++) {
>> +		msm_host->dll.dll_config[i] = dll_table[i];
>> +		msm_host->dll.dll_config_2[i] = dll_table[i + 1];
>> +		msm_host->dll.dll_config_3[i] = dll_table[i + 2];
>> +		msm_host->dll.dll_usr_ctl[i] = dll_table[i + 3];
>> +		msm_host->dll.ddr_config[i] = dll_table[i + 4];
>> +	}
>> +
>> +	msm_host->artanis_dll = true;
> And the pointer to dll_table is lost, lingering for the driver lifetime.
> Please drop the devm_ part and kfree() it once it is not used anymore.
>
>> +
>> +	return 0;
>> +}
>> +
>>  static int sdhci_msm_probe(struct platform_device *pdev)
>>  {
>>  	struct sdhci_host *host;
>> @@ -2446,6 +2529,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>  
>>  	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>>  
>> +	if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
>> +		goto pltfm_free;
>> +
>>  	ret = sdhci_msm_gcc_reset(&pdev->dev, host);
>>  	if (ret)
>>  		goto pltfm_free;
>> -- 
>> 2.17.1
>>

Re: [PATCH V3 4/4] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Ram Prakash Gupta]

Hi Dmitry,

I will start on this with addressing your comments in previous version as
suggested.

Thanks,
Ram

On 1/22/2025 3:30 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 22, 2025 at 03:17:07PM +0530, Sachin Gupta wrote:
>> 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>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 270 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 252 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index cc7756a59c55..17f17a635d83 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)
>> @@ -118,7 +119,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
>> @@ -309,6 +311,16 @@ struct sdhci_msm_host {
>>  	bool artanis_dll;
>>  };
>>  
>> +enum dll_init_context {
>> +	DLL_INIT_NORMAL,
>> +	DLL_INIT_FROM_CX_COLLAPSE_EXIT,
>> +};
>> +
>> +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);
>> @@ -793,6 +805,211 @@ 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;
>> +	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 (host->clock != msm_host->clk_rate)
>> +		sup_clk = sup_clk / 2;
> Please resolve previous discussions before sending new versions. Just
> sending a response and then sending next iteration of the patchset is
> not a proper way to communicate.
>
> NAK until the discussion is resolved in the previous thread.
>
>> +
>> +	return sup_clk;
>> +}
>> +

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Krzysztof Kozlowski]

On 10/06/2025 14:07, Ram Prakash Gupta wrote:
> Hi Krzysztof,
> 
> Thanks for your comment, The Qualcomm Engineer who initiated this work,
> is no longer working on this and I am taking up the responsibility to continue
> on this work. I have started to check this and will start with addressing your
> comment next.
> 
> Thanks,
> Ram
> 
> On 1/22/2025 3:56 PM, Krzysztof Kozlowski wrote:
>> On 22/01/2025 10:47, Sachin Gupta wrote:

Above timeline is interesting:
1. Patch sent on 22nd January.
2. I provided comments few hours later, the same day.
3. Silence.
4. Employee changes job.
5. Five months later...

Not your fault Ram of course, but above timeline is not a responsible
way of upstreaming patches.

Best regards,
Krzysztof

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Ram Prakash Gupta]

On 6/10/2025 6:23 PM, Krzysztof Kozlowski wrote:
> On 10/06/2025 14:07, Ram Prakash Gupta wrote:
>> Hi Krzysztof,
>>
>> Thanks for your comment, The Qualcomm Engineer who initiated this work,
>> is no longer working on this and I am taking up the responsibility to continue
>> on this work. I have started to check this and will start with addressing your
>> comment next.
>>
>> Thanks,
>> Ram
>>
>> On 1/22/2025 3:56 PM, Krzysztof Kozlowski wrote:
>>> On 22/01/2025 10:47, Sachin Gupta wrote:
> Above timeline is interesting:
> 1. Patch sent on 22nd January.
> 2. I provided comments few hours later, the same day.
> 3. Silence.
> 4. Employee changes job.
> 5. Five months later...
>
> Not your fault Ram of course, but above timeline is not a responsible
> way of upstreaming patches.
>
> Best regards,
> Krzysztof

my apologies for this, I will give inputs internally to improve process
for changing hands.

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Krzysztof Kozlowski]

On 10/06/2025 15:07, Ram Prakash Gupta wrote:
> 
> On 6/10/2025 6:23 PM, Krzysztof Kozlowski wrote:
>> On 10/06/2025 14:07, Ram Prakash Gupta wrote:
>>> Hi Krzysztof,
>>>
>>> Thanks for your comment, The Qualcomm Engineer who initiated this work,
>>> is no longer working on this and I am taking up the responsibility to continue
>>> on this work. I have started to check this and will start with addressing your
>>> comment next.
>>>
>>> Thanks,
>>> Ram
>>>
>>> On 1/22/2025 3:56 PM, Krzysztof Kozlowski wrote:
>>>> On 22/01/2025 10:47, Sachin Gupta wrote:
>> Above timeline is interesting:
>> 1. Patch sent on 22nd January.
>> 2. I provided comments few hours later, the same day.
>> 3. Silence.
>> 4. Employee changes job.
>> 5. Five months later...
>>
>> Not your fault Ram of course, but above timeline is not a responsible
>> way of upstreaming patches.
>>
>> Best regards,
>> Krzysztof
> 
> my apologies for this, I will give inputs internally to improve process
> for changing hands.


Not your fault, don't worry.

Best regards,
Krzysztof

Re: [PATCH V3 4/4] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Dmitry Baryshkov]

On Tue, Jun 10, 2025 at 05:52:02PM +0530, Ram Prakash Gupta wrote:
> Hi Dmitry,
> 
> I will start on this with addressing your comments in previous version as
> suggested.

- Please don't top-post. Ever.

- Please provide some actual response the comments where they were
  posted. From the upstream community side that's more important than 'I
  will address comments' announcement.

> 
> Thanks,
> Ram
> 
> On 1/22/2025 3:30 PM, Dmitry Baryshkov wrote:
> > On Wed, Jan 22, 2025 at 03:17:07PM +0530, Sachin Gupta wrote:
> >> 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>
> >> ---
> >>  drivers/mmc/host/sdhci-msm.c | 270 ++++++++++++++++++++++++++++++++---
> >>  1 file changed, 252 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> >> index cc7756a59c55..17f17a635d83 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)
> >> @@ -118,7 +119,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
> >> @@ -309,6 +311,16 @@ struct sdhci_msm_host {
> >>  	bool artanis_dll;
> >>  };
> >>  
> >> +enum dll_init_context {
> >> +	DLL_INIT_NORMAL,
> >> +	DLL_INIT_FROM_CX_COLLAPSE_EXIT,
> >> +};
> >> +
> >> +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);
> >> @@ -793,6 +805,211 @@ 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;
> >> +	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 (host->clock != msm_host->clk_rate)
> >> +		sup_clk = sup_clk / 2;
> > Please resolve previous discussions before sending new versions. Just
> > sending a response and then sending next iteration of the patchset is
> > not a proper way to communicate.
> >
> > NAK until the discussion is resolved in the previous thread.
> >
> >> +
> >> +	return sup_clk;
> >> +}
> >> +

-- 
With best wishes
Dmitry

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Ram Prakash Gupta]

On 1/22/2025 3:56 PM, Krzysztof Kozlowski wrote:
> On 22/01/2025 10:47, Sachin Gupta wrote:
>> Document the 'dll-hsr-list' property for MMC device tree bindings.
>> The 'dll-hsr-list' property defines the DLL configurations for HS400
>> and HS200 modes.
>>
>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>> ---
>>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>> index 8b393e26e025..65dc3053df75 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>> @@ -133,6 +133,11 @@ properties:
>>      $ref: /schemas/types.yaml#/definitions/uint32
>>      description: platform specific settings for DLL_CONFIG reg.
>>  
>> +  qcom,dll-hsr-list:
>> +    maxItems: 10
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> uint32 has only one item. Anyway, there is already DLL there, so don't
> duplicate or explain why this is different. Explain also why this is not
> deducible from the compatible.

I will change it to reflect array from uint32.
There is change with artanis DLL hw addition where it need total of 5 entries
(dll_config, dll_config_2, dll_config_3, dll_usr_ctl, ddr_config)
for each HS400 and HS200 modes, hence the new addition in dt. And these values
are not fixed and varies for every SoC, hence this needs to be passed through
dt like it was passed earlier for qcom,dll-config & qcom,ddr-config.

And backward compatibility would be taken care by check for qcom,dll-hsr-list,
where new settings would apply for soc where qcom,dll-hsr-list is added into
dt.

>
> Please provide here link to DTS user so we can validate how you use it.
>
>
> Best regards,
> Krzysztof

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Ram Prakash Gupta]

On 6/26/2025 7:46 PM, Ram Prakash Gupta wrote:
> On 1/22/2025 3:56 PM, Krzysztof Kozlowski wrote:
>> On 22/01/2025 10:47, Sachin Gupta wrote:
>>> Document the 'dll-hsr-list' property for MMC device tree bindings.
>>> The 'dll-hsr-list' property defines the DLL configurations for HS400
>>> and HS200 modes.
>>>
>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>> ---
>>>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>> index 8b393e26e025..65dc3053df75 100644
>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>> @@ -133,6 +133,11 @@ properties:
>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>      description: platform specific settings for DLL_CONFIG reg.
>>>  
>>> +  qcom,dll-hsr-list:
>>> +    maxItems: 10
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> uint32 has only one item. Anyway, there is already DLL there, so don't
>> duplicate or explain why this is different. Explain also why this is not
>> deducible from the compatible.
> I will change it to reflect array from uint32.
> There is change with artanis DLL hw addition where it need total of 5 entries
> (dll_config, dll_config_2, dll_config_3, dll_usr_ctl, ddr_config)
> for each HS400 and HS200 modes, hence the new addition in dt. And these values
> are not fixed and varies for every SoC, hence this needs to be passed through
> dt like it was passed earlier for qcom,dll-config & qcom,ddr-config.
>
> And backward compatibility would be taken care by check for qcom,dll-hsr-list,
> where new settings would apply for soc where qcom,dll-hsr-list is added into
> dt.
>
>> Please provide here link to DTS user so we can validate how you use it.
>>
>>
>> Best regards,
>> Krzysztof

sure, will provide with V4 patchset.

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Krzysztof Kozlowski]

On 26/06/2025 16:16, Ram Prakash Gupta wrote:
> On 1/22/2025 3:56 PM, Krzysztof Kozlowski wrote:
>> On 22/01/2025 10:47, Sachin Gupta wrote:
>>> Document the 'dll-hsr-list' property for MMC device tree bindings.
>>> The 'dll-hsr-list' property defines the DLL configurations for HS400
>>> and HS200 modes.
>>>
>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>> ---
>>>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>> index 8b393e26e025..65dc3053df75 100644
>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>> @@ -133,6 +133,11 @@ properties:
>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>      description: platform specific settings for DLL_CONFIG reg.
>>>  
>>> +  qcom,dll-hsr-list:
>>> +    maxItems: 10
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> uint32 has only one item. Anyway, there is already DLL there, so don't
>> duplicate or explain why this is different. Explain also why this is not
>> deducible from the compatible.
> 


Timeline still amazes me. I will be grumpy on this thread.

> I will change it to reflect array from uint32.
> There is change with artanis DLL hw addition where it need total of 5 entries
> (dll_config, dll_config_2, dll_config_3, dll_usr_ctl, ddr_config)
> for each HS400 and HS200 modes, hence the new addition in dt. And these values
> are not fixed and varies for every SoC, hence this needs to be passed through
> dt like it was passed earlier for qcom,dll-config & qcom,ddr-config.


Eh, no. That's not a valid reason. It's still SoC deducible. Don't bring
your downstream practices here, but remove EVERYTHING from downstream
and start doing things like upstream is doing.

Best regards,
Krzysztof

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Ram Prakash Gupta]

On 6/26/2025 11:12 PM, Krzysztof Kozlowski wrote:
> On 26/06/2025 16:16, Ram Prakash Gupta wrote:
>> On 1/22/2025 3:56 PM, Krzysztof Kozlowski wrote:
>>> On 22/01/2025 10:47, Sachin Gupta wrote:
>>>> Document the 'dll-hsr-list' property for MMC device tree bindings.
>>>> The 'dll-hsr-list' property defines the DLL configurations for HS400
>>>> and HS200 modes.
>>>>
>>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>> index 8b393e26e025..65dc3053df75 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>> @@ -133,6 +133,11 @@ properties:
>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>      description: platform specific settings for DLL_CONFIG reg.
>>>>  
>>>> +  qcom,dll-hsr-list:
>>>> +    maxItems: 10
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> uint32 has only one item. Anyway, there is already DLL there, so don't
>>> duplicate or explain why this is different. Explain also why this is not
>>> deducible from the compatible.
>
> Timeline still amazes me. I will be grumpy on this thread.
>
>> I will change it to reflect array from uint32.
>> There is change with artanis DLL hw addition where it need total of 5 entries
>> (dll_config, dll_config_2, dll_config_3, dll_usr_ctl, ddr_config)
>> for each HS400 and HS200 modes, hence the new addition in dt. And these values
>> are not fixed and varies for every SoC, hence this needs to be passed through
>> dt like it was passed earlier for qcom,dll-config & qcom,ddr-config.
>
> Eh, no. That's not a valid reason. It's still SoC deducible. Don't bring
> your downstream practices here, but remove EVERYTHING from downstream
> and start doing things like upstream is doing.
>
> Best regards,
> Krzysztof

Sorry I did not get it - you mean to say keep these values in driver file?
how is it possible to tie these value with only one compatible which can vary
with every soc or you are suggesting me to make code change in driver for every
target having artanis dll hw.

And sorry but considering upstream only this design was put in place, its not
about downstream, since there are already dll_config and ddr_config which are
passed through dt, its logical here to pass rest of the dll related parameters
through dt only.

Thanks,
Ram

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Konrad Dybcio]

On 6/26/25 7:42 PM, Krzysztof Kozlowski wrote:
> On 26/06/2025 16:16, Ram Prakash Gupta wrote:
>> On 1/22/2025 3:56 PM, Krzysztof Kozlowski wrote:
>>> On 22/01/2025 10:47, Sachin Gupta wrote:
>>>> Document the 'dll-hsr-list' property for MMC device tree bindings.
>>>> The 'dll-hsr-list' property defines the DLL configurations for HS400
>>>> and HS200 modes.
>>>>
>>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>> index 8b393e26e025..65dc3053df75 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>> @@ -133,6 +133,11 @@ properties:
>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>      description: platform specific settings for DLL_CONFIG reg.
>>>>  
>>>> +  qcom,dll-hsr-list:
>>>> +    maxItems: 10
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> uint32 has only one item. Anyway, there is already DLL there, so don't
>>> duplicate or explain why this is different. Explain also why this is not
>>> deducible from the compatible.
>>
> 
> 
> Timeline still amazes me. I will be grumpy on this thread.
> 
>> I will change it to reflect array from uint32.
>> There is change with artanis DLL hw addition where it need total of 5 entries
>> (dll_config, dll_config_2, dll_config_3, dll_usr_ctl, ddr_config)
>> for each HS400 and HS200 modes, hence the new addition in dt. And these values
>> are not fixed and varies for every SoC, hence this needs to be passed through
>> dt like it was passed earlier for qcom,dll-config & qcom,ddr-config.
> 
> 
> Eh, no. That's not a valid reason. It's still SoC deducible. Don't bring
> your downstream practices here, but remove EVERYTHING from downstream
> and start doing things like upstream is doing.

QC SoCs have between 0 and 4 SDHCI instances, each one potentially requiring
different tuning, let's keep this data in DT

Konrad

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Krzysztof Kozlowski]

On 27/06/2025 15:57, Konrad Dybcio wrote:
> On 6/26/25 7:42 PM, Krzysztof Kozlowski wrote:
>> On 26/06/2025 16:16, Ram Prakash Gupta wrote:
>>> On 1/22/2025 3:56 PM, Krzysztof Kozlowski wrote:
>>>> On 22/01/2025 10:47, Sachin Gupta wrote:
>>>>> Document the 'dll-hsr-list' property for MMC device tree bindings.
>>>>> The 'dll-hsr-list' property defines the DLL configurations for HS400
>>>>> and HS200 modes.
>>>>>
>>>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>>> index 8b393e26e025..65dc3053df75 100644
>>>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>>> @@ -133,6 +133,11 @@ properties:
>>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>>      description: platform specific settings for DLL_CONFIG reg.
>>>>>  
>>>>> +  qcom,dll-hsr-list:
>>>>> +    maxItems: 10
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> uint32 has only one item. Anyway, there is already DLL there, so don't
>>>> duplicate or explain why this is different. Explain also why this is not
>>>> deducible from the compatible.
>>>
>>
>>
>> Timeline still amazes me. I will be grumpy on this thread.
>>
>>> I will change it to reflect array from uint32.
>>> There is change with artanis DLL hw addition where it need total of 5 entries
>>> (dll_config, dll_config_2, dll_config_3, dll_usr_ctl, ddr_config)
>>> for each HS400 and HS200 modes, hence the new addition in dt. And these values
>>> are not fixed and varies for every SoC, hence this needs to be passed through
>>> dt like it was passed earlier for qcom,dll-config & qcom,ddr-config.
>>
>>
>> Eh, no. That's not a valid reason. It's still SoC deducible. Don't bring
>> your downstream practices here, but remove EVERYTHING from downstream
>> and start doing things like upstream is doing.
> 
> QC SoCs have between 0 and 4 SDHCI instances, each one potentially requiring
> different tuning, let's keep this data in DT


OK, this should be explained in commit msg.

Best regards,
Krzysztof

Re: [PATCH V3 1/4] dt-bindings: mmc: Add dll-hsr-list for HS400 and HS200 modes

[Ram Prakash Gupta]

On 6/27/2025 8:20 PM, Krzysztof Kozlowski wrote:
> On 27/06/2025 15:57, Konrad Dybcio wrote:
>> On 6/26/25 7:42 PM, Krzysztof Kozlowski wrote:
>>> On 26/06/2025 16:16, Ram Prakash Gupta wrote:
>>>> On 1/22/2025 3:56 PM, Krzysztof Kozlowski wrote:
>>>>> On 22/01/2025 10:47, Sachin Gupta wrote:
>>>>>> Document the 'dll-hsr-list' property for MMC device tree bindings.
>>>>>> The 'dll-hsr-list' property defines the DLL configurations for HS400
>>>>>> and HS200 modes.
>>>>>>
>>>>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>>>>>>  1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>>>> index 8b393e26e025..65dc3053df75 100644
>>>>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>>>> @@ -133,6 +133,11 @@ properties:
>>>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>      description: platform specific settings for DLL_CONFIG reg.
>>>>>>  
>>>>>> +  qcom,dll-hsr-list:
>>>>>> +    maxItems: 10
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> uint32 has only one item. Anyway, there is already DLL there, so don't
>>>>> duplicate or explain why this is different. Explain also why this is not
>>>>> deducible from the compatible.
>>>
>>> Timeline still amazes me. I will be grumpy on this thread.
>>>
>>>> I will change it to reflect array from uint32.
>>>> There is change with artanis DLL hw addition where it need total of 5 entries
>>>> (dll_config, dll_config_2, dll_config_3, dll_usr_ctl, ddr_config)
>>>> for each HS400 and HS200 modes, hence the new addition in dt. And these values
>>>> are not fixed and varies for every SoC, hence this needs to be passed through
>>>> dt like it was passed earlier for qcom,dll-config & qcom,ddr-config.
>>>
>>> Eh, no. That's not a valid reason. It's still SoC deducible. Don't bring
>>> your downstream practices here, but remove EVERYTHING from downstream
>>> and start doing things like upstream is doing.
>> QC SoCs have between 0 and 4 SDHCI instances, each one potentially requiring
>> different tuning, let's keep this data in DT
>
> OK, this should be explained in commit msg.
>
> Best regards,
> Krzysztof

sure, I will update this into commit message.

Thanks,
Ram

Re: [PATCH V3 3/4] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Ram Prakash Gupta]

On 6/10/2025 5:47 PM, Ram Prakash Gupta wrote:
> Hi Dmitry,
>
> As updated in [PATCH V3 2/2] of this series, I have started now to continue
> this work. Will address your comment next.
>
> Thanks,
> Ram
>
> On 1/22/2025 3:27 PM, Dmitry Baryshkov wrote:
>> On Wed, Jan 22, 2025 at 03:17:06PM +0530, Sachin Gupta wrote:
>>> This update introduces the capability to configure HS200
>>> and HS400 DLL settings via the device tree and parsing it.
>>>
>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>> ---
>>>  drivers/mmc/host/sdhci-msm.c | 86 ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 86 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>> index 2a5e588779fc..cc7756a59c55 100644
>>> --- a/drivers/mmc/host/sdhci-msm.c
>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>> @@ -256,6 +256,19 @@ struct sdhci_msm_variant_info {
>>>  	const struct sdhci_msm_offset *offset;
>>>  };
>>>  
>>> +/*
>>> + * DLL registers which needs be programmed with HSR settings.
>>> + * Add any new register only at the end and don't change the
>>> + * sequence.
>>> + */
>>> +struct sdhci_msm_dll {
>>> +	u32 dll_config[2];
>>> +	u32 dll_config_2[2];
>>> +	u32 dll_config_3[2];
>>> +	u32 dll_usr_ctl[2];
>>> +	u32 ddr_config[2];
>>> +};
>>> +
>>>  struct sdhci_msm_host {
>>>  	struct platform_device *pdev;
>>>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
>>> @@ -264,6 +277,7 @@ struct sdhci_msm_host {
>>>  	struct clk *xo_clk;	/* TCXO clk needed for FLL feature of cm_dll*/
>>>  	/* core, iface, cal and sleep clocks */
>>>  	struct clk_bulk_data bulk_clks[4];
>>> +	struct sdhci_msm_dll dll;
>>>  #ifdef CONFIG_MMC_CRYPTO
>>>  	struct qcom_ice *ice;
>>>  #endif
>>> @@ -292,6 +306,7 @@ struct sdhci_msm_host {
>>>  	u32 dll_config;
>>>  	u32 ddr_config;
>>>  	bool vqmmc_enabled;
>>> +	bool artanis_dll;
>>>  };
>>>  
>>>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>>> @@ -2400,6 +2415,74 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>>>  	return ret;
>>>  }
>>>  
>>> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
>>> +				  u32 **bw_vecs, int *len)
>> It just reads an array from the DT, please rename the bw_vecs param
>> which is inaccurate in this case.

I will rename this to "dll_table".

>>> +{
>>> +	struct device_node *np = dev->of_node;
>>> +	u32 *arr = NULL;
>>> +	int ret = 0;
>>> +	int sz;
>>> +
>>> +	if (!np)
>>> +		return -ENODEV;
>>> +
>>> +	if (!of_get_property(np, prop_name, &sz))
>>> +		return -EINVAL;
>>> +
>>> +	sz = sz / sizeof(*arr);
>>> +	if (sz <= 0)
>>> +		return -EINVAL;
>>> +
>>> +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
>>> +	if (!arr)
>>> +		return -ENOMEM;
>>> +
>>> +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
>>> +	if (ret) {
>>> +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
>>> +		*len = 0;
>>> +		return ret;
>>> +	}
>>> +
>>> +	*bw_vecs = arr;
>>> +	*len = sz;
>>> +	ret = 0;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
>>> +{
>>> +	int dll_table_len, dll_reg_count;
>>> +	u32 *dll_table = NULL;
>>> +	int i;
>>> +
>>> +	msm_host->artanis_dll = false;
>>> +
>>> +	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
>>> +				   &dll_table, &dll_table_len))
>>> +		return -EINVAL;
>>> +
>>> +	dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
>>> +
>>> +	if (dll_table_len != dll_reg_count) {
>>> +		dev_err(dev, "Number of HSR entries are not matching\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	for (i = 0; i < 2; i++) {
>>> +		msm_host->dll.dll_config[i] = dll_table[i];
>>> +		msm_host->dll.dll_config_2[i] = dll_table[i + 1];
>>> +		msm_host->dll.dll_config_3[i] = dll_table[i + 2];
>>> +		msm_host->dll.dll_usr_ctl[i] = dll_table[i + 3];
>>> +		msm_host->dll.ddr_config[i] = dll_table[i + 4];
>>> +	}
>>> +
>>> +	msm_host->artanis_dll = true;
>> And the pointer to dll_table is lost, lingering for the driver lifetime.
>> Please drop the devm_ part and kfree() it once it is not used anymore.

ok, I ll allocate memory using kzalloc in function  sdhci_msm_dt_get_array
 and kfree() after copying data in this function.

Also the logic to copy the data in msm_host->dll.dll_config[x] is not
correct above, had to fix it as I was observing DLL related issues,
when testing different eMMC modes. Below is the correct code to copy
data correctly from dll_table.

"for (i = 0, j = 0; j < 2; i=i+5, j++) {
         msm_host->dll.dll_config[j] = dll_table[i];
         msm_host->dll.dll_config_2[j] = dll_table[i + 1];
         msm_host->dll.dll_config_3[j] = dll_table[i + 2];
         msm_host->dll.dll_usr_ctl[j] = dll_table[i + 3];
         msm_host->dll.ddr_config[j] = dll_table[i + 4];
}"

since the parsing itself was not correct, had to go through whole code
again and test all the modes of eMMC and SDCard.

All registers values of DLL are now matching with expected values passed
in dt, in all modes of eMMC and SDCard post required modes tuning.

eMMC tested modes are HS400ES/HS400/HS200/HS50/DDR52.
SDCard tested modes are SDR104/SDR50.

Thanks,
Ram

>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int sdhci_msm_probe(struct platform_device *pdev)
>>>  {
>>>  	struct sdhci_host *host;
>>> @@ -2446,6 +2529,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>>  
>>>  	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>>>  
>>> +	if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
>>> +		goto pltfm_free;
>>> +
>>>  	ret = sdhci_msm_gcc_reset(&pdev->dev, host);
>>>  	if (ret)
>>>  		goto pltfm_free;
>>> -- 
>>> 2.17.1
>>>

Re: [PATCH V3 3/4] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Konrad Dybcio]

On 7/23/25 12:14 PM, Ram Prakash Gupta wrote:
> 
> On 6/10/2025 5:47 PM, Ram Prakash Gupta wrote:
>> Hi Dmitry,
>>
>> As updated in [PATCH V3 2/2] of this series, I have started now to continue
>> this work. Will address your comment next.
>>
>> Thanks,
>> Ram

[...]

>>> And the pointer to dll_table is lost, lingering for the driver lifetime.
>>> Please drop the devm_ part and kfree() it once it is not used anymore.
> 
> ok, I ll allocate memory using kzalloc in function  sdhci_msm_dt_get_array
>  and kfree() after copying data in this function.

You can use __free() nowadays (see e.g. drivers/soc/qcom/mdt_loader.c :
mdt_load_split_segment), which will dispose of the memory when it goes
out of scope, limiting the need to clean it up every error path
separately

Konrad

Re: [PATCH V3 3/4] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Ram Prakash Gupta]

On 7/23/2025 4:07 PM, Konrad Dybcio wrote:
> On 7/23/25 12:14 PM, Ram Prakash Gupta wrote:
>> On 6/10/2025 5:47 PM, Ram Prakash Gupta wrote:
>>> Hi Dmitry,
>>>
>>> As updated in [PATCH V3 2/2] of this series, I have started now to continue
>>> this work. Will address your comment next.
>>>
>>> Thanks,
>>> Ram
> [...]
>
>>>> And the pointer to dll_table is lost, lingering for the driver lifetime.
>>>> Please drop the devm_ part and kfree() it once it is not used anymore.
>> ok, I ll allocate memory using kzalloc in function  sdhci_msm_dt_get_array
>>  and kfree() after copying data in this function.
> You can use __free() nowadays (see e.g. drivers/soc/qcom/mdt_loader.c :
> mdt_load_split_segment), which will dispose of the memory when it goes
> out of scope, limiting the need to clean it up every error path
> separately
>
> Konrad

ok thanks, I will use __free()