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

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

[Sachin Gupta]

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 v1:
1. Addressed Tengfei Fan comment, added missing semicolocon
 in sdhci_msm_host structure.

Sachin Gupta (2):
  mmc: sdhci-msm: Add core_major, minor to msm_host structure
  mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

 drivers/mmc/host/sdhci-msm.c | 378 +++++++++++++++++++++++++++++++++--
 1 file changed, 359 insertions(+), 19 deletions(-)

-- 
2.17.1

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

[Sachin Gupta]

Add the core_major and core_minor variables from local
scope to the msm_host structure. This change improves
the organization of the code and allows these variables
to be accessed more easily throughout the msm_host context.

core_major will have information related to sdcc controller
major version number.
core_minor will have information related to sdcc controller
minor version number.

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

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

[Dmitry Baryshkov]

On Wed, Dec 18, 2024 at 02:40:56PM +0530, Sachin Gupta wrote:
> Add the core_major and core_minor variables from local
> scope to the msm_host structure. This change improves
> the organization of the code and allows these variables
> to be accessed more easily throughout the msm_host context.

It doesn't improve anything, it is necessary for the followup patch.

> 
> core_major will have information related to sdcc controller
> major version number.
> core_minor will have information related to sdcc controller
> minor version number.
> 
> 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 V2 1/2] mmc: sdhci-msm: Add core_major, minor to msm_host structure

[Sachin Gupta]

On 12/19/2024 8:39 AM, Dmitry Baryshkov wrote:
> On Wed, Dec 18, 2024 at 02:40:56PM +0530, Sachin Gupta wrote:
>> Add the core_major and core_minor variables from local
>> scope to the msm_host structure. This change improves
>> the organization of the code and allows these variables
>> to be accessed more easily throughout the msm_host context.
> 
> It doesn't improve anything, it is necessary for the followup patch.

Thanks for review. I will update the commit message to reflect the 
current patch information.

> 
>>
>> core_major will have information related to sdcc controller
>> major version number.
>> core_minor will have information related to sdcc controller
>> minor version number.
>>
>> 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 V2 2/2] 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 and also incorporate support for HS200 and
HS400 DLL settings using the device tree.

Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
Signed-off-by: Jun Li <liju@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
 1 file changed, 353 insertions(+), 19 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 2a5e588779fc..4ecb362f7f2a 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
@@ -256,6 +258,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 +279,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 +308,17 @@ struct sdhci_msm_host {
 	u32 dll_config;
 	u32 ddr_config;
 	bool vqmmc_enabled;
+	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)
@@ -778,6 +805,210 @@ 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_round_rate(core_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 & ~(0xFF << 10)) | (mclk_freq << 10);
+			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);
@@ -900,14 +1131,35 @@ 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)
+{
+	unsigned char timing = host->mmc->ios.timing;
+	int ret;
+
+	if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
+		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
+	else
+		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
+
+	return ret;
+}
+
+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__);
 
@@ -915,7 +1167,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;
 
@@ -1003,7 +1255,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;
@@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
 					sdhci_priv_msm_offset(host);
 
 	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
-
 	/*
 	 * Currently the core_ddr_config register defaults to desired
 	 * configuration on reset. Currently reprogramming the power on
@@ -1026,7 +1277,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 +
@@ -1083,11 +1338,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__);
 
@@ -1095,7 +1349,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;
 
@@ -1115,7 +1370,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);
@@ -1154,7 +1409,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;
 
@@ -1223,7 +1479,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;
 
@@ -1752,11 +2008,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.
  *
@@ -2400,6 +2651,86 @@ 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, u32 size)
+{
+	struct device_node *np = dev->of_node;
+	u32 *arr = NULL;
+	int ret = 0;
+	size_t sz;
+
+	if (!np) {
+		ret = -ENODEV;
+		goto out;
+	}
+	if (!of_get_property(np, prop_name, len)) {
+		ret = -EINVAL;
+		goto out;
+	}
+	sz = *len = *len / sizeof(*arr);
+	if (sz <= 0 || (size > 0 && (sz > size))) {
+		dev_err(dev, "%s invalid size\n", prop_name);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
+	if (!arr) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = of_property_read_u32_array(np, prop_name, arr, sz);
+	if (ret < 0) {
+		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
+		goto out;
+	}
+	*bw_vecs = arr;
+out:
+	if (ret)
+		*len = 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;
+	u32 dll_values[10];
+	int ret = 0, i;
+
+	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
+		&dll_table, &dll_table_len, 0))
+		goto skip_dll;
+
+	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");
+		ret = -EINVAL;
+		goto skip_dll;
+	}
+
+	for (i = 0; i < 5; i++) {
+		dll_values[2 * i] = dll_table[i];
+		dll_values[2 * i + 1] = dll_table[i + 5];
+	}
+
+	for (i = 0; i < 10; i++)
+		dll_table[i] = dll_values[i];
+
+	memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
+	msm_host->artanis_dll = true;
+
+skip_dll:
+	if (!dll_table) {
+		msm_host->artanis_dll = false;
+		dev_err(dev, "Failed to get dll hsr settings from dt\n");
+	}
+
+	return ret;
+}
+
 static int sdhci_msm_probe(struct platform_device *pdev)
 {
 	struct sdhci_host *host;
@@ -2446,6 +2777,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 V2 2/2] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Dmitry Baryshkov]

On Wed, Dec 18, 2024 at 02:40:57PM +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 and also incorporate support for HS200 and
> HS400 DLL settings using the device tree.

"foo also bar" usually points out that there should be two separate
commits.

> 
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
> Signed-off-by: Jun Li <liju@codeaurora.org>

This is very strange and incorrect.

> ---
>  drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
>  1 file changed, 353 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 2a5e588779fc..4ecb362f7f2a 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

What about the platforms where TCXO has different frequency?

>  
>  #define INVALID_TUNING_PHASE	-1
>  #define SDHCI_MSM_MIN_CLOCK	400000
> @@ -256,6 +258,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.

Why?

> + */
> +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 +279,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 +308,17 @@ struct sdhci_msm_host {
>  	u32 dll_config;
>  	u32 ddr_config;
>  	bool vqmmc_enabled;
> +	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)
> @@ -778,6 +805,210 @@ 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;
> +}

Why??? Why do you need a function to return a static value?

> +
> +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_round_rate(core_clk, clk_get_rate(core_clk));
> +
> +	if (host->clock != msm_host->clk_rate)
> +		sup_clk = sup_clk / 2;
> +
> +	return sup_clk;

Why?

> +}
> +
> +/* 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 & ~(0xFF << 10)) | (mclk_freq << 10);

GENMASK, FIELD_PREP?

> +			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);
> @@ -900,14 +1131,35 @@ 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)
> +{
> +	unsigned char timing = host->mmc->ios.timing;
> +	int ret;
> +
> +	if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
> +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
> +	else
> +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
> +
> +	return ret;
> +}
> +
> +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__);
>  
> @@ -915,7 +1167,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;
>  
> @@ -1003,7 +1255,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;
> @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>  					sdhci_priv_msm_offset(host);
>  
>  	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
> -

Unrelated, please drop.

>  	/*
>  	 * Currently the core_ddr_config register defaults to desired
>  	 * configuration on reset. Currently reprogramming the power on
> @@ -1026,7 +1277,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 +
> @@ -1083,11 +1338,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__);
>  
> @@ -1095,7 +1349,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;
>  
> @@ -1115,7 +1370,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);
> @@ -1154,7 +1409,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;
>  
> @@ -1223,7 +1479,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;
>  
> @@ -1752,11 +2008,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.
>   *
> @@ -2400,6 +2651,86 @@ 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, u32 size)
> +{
> +	struct device_node *np = dev->of_node;
> +	u32 *arr = NULL;
> +	int ret = 0;
> +	size_t sz;
> +
> +	if (!np) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +	if (!of_get_property(np, prop_name, len)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	sz = *len = *len / sizeof(*arr);

You obviously skipped checkpatch run. Please don't do that.

> +	if (sz <= 0 || (size > 0 && (sz > size))) {
> +		dev_err(dev, "%s invalid size\n", prop_name);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> +	if (!arr) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
> +	if (ret < 0) {
> +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> +		goto out;
> +	}
> +	*bw_vecs = arr;
> +out:
> +	if (ret)
> +		*len = 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;
> +	u32 dll_values[10];
> +	int ret = 0, i;
> +
> +	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
> +		&dll_table, &dll_table_len, 0))
> +		goto skip_dll;

Missing update for the bindings.

> +
> +	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");
> +		ret = -EINVAL;
> +		goto skip_dll;
> +	}
> +
> +	for (i = 0; i < 5; i++) {

Magic value 5, replace with ARRAY_SIZE

> +		dll_values[2 * i] = dll_table[i];
> +		dll_values[2 * i + 1] = dll_table[i + 5];
> +	}
> +
> +	for (i = 0; i < 10; i++)
> +		dll_table[i] = dll_values[i];

So three memory copies to rearrange the order of values? That sounds
like a horrible solution.

> +
> +	memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
> +	msm_host->artanis_dll = true;
> +
> +skip_dll:
> +	if (!dll_table) {
> +		msm_host->artanis_dll = false;
> +		dev_err(dev, "Failed to get dll hsr settings from dt\n");
> +	}
> +
> +	return ret;
> +}
> +
>  static int sdhci_msm_probe(struct platform_device *pdev)
>  {
>  	struct sdhci_host *host;
> @@ -2446,6 +2777,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 V2 2/2] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Sachin Gupta]

On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> On Wed, Dec 18, 2024 at 02:40:57PM +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 and also incorporate support for HS200 and
>> HS400 DLL settings using the device tree.
> 
> "foo also bar" usually points out that there should be two separate
> commits.

Thanks for review. I will split it into two patches.

> 
>>
>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
>> Signed-off-by: Jun Li <liju@codeaurora.org>
> 
> This is very strange and incorrect.

Thanks for review. I will fix the format.

> 
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
>>   1 file changed, 353 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 2a5e588779fc..4ecb362f7f2a 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
> 
> What about the platforms where TCXO has different frequency?
> 

All emmc targets have 192 Mhz as TCXO freq.

>>   
>>   #define INVALID_TUNING_PHASE	-1
>>   #define SDHCI_MSM_MIN_CLOCK	400000
>> @@ -256,6 +258,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.
> 
> Why?

I will update the comment message in next patchset.

> 
>> + */
>> +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 +279,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 +308,17 @@ struct sdhci_msm_host {
>>   	u32 dll_config;
>>   	u32 ddr_config;
>>   	bool vqmmc_enabled;
>> +	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)
>> @@ -778,6 +805,210 @@ 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;
>> +}
> 
> Why??? Why do you need a function to return a static value?
> 

This is just rearrangement of the function. This function already exist, 
moving here to avoid predeclaration.

>> +
>> +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_round_rate(core_clk, clk_get_rate(core_clk));
>> +
>> +	if (host->clock != msm_host->clk_rate)
>> +		sup_clk = sup_clk / 2;
>> +
>> +	return sup_clk;
> 
> Why?

Sorry, I did not understand your question. Can you please explain in detail.

> 
>> +}
>> +
>> +/* 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 & ~(0xFF << 10)) | (mclk_freq << 10);
> 
> GENMASK, FIELD_PREP?

Sure I will use the suggested macros.

> 
>> +			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);
>> @@ -900,14 +1131,35 @@ 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)
>> +{
>> +	unsigned char timing = host->mmc->ios.timing;
>> +	int ret;
>> +
>> +	if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
>> +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
>> +	else
>> +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
>> +
>> +	return ret;
>> +}
>> +
>> +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__);
>>   
>> @@ -915,7 +1167,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;
>>   
>> @@ -1003,7 +1255,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;
>> @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>>   					sdhci_priv_msm_offset(host);
>>   
>>   	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
>> -
> 
> Unrelated, please drop.

I will fix it in next patchset.

> 
>>   	/*
>>   	 * Currently the core_ddr_config register defaults to desired
>>   	 * configuration on reset. Currently reprogramming the power on
>> @@ -1026,7 +1277,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 +
>> @@ -1083,11 +1338,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__);
>>   
>> @@ -1095,7 +1349,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;
>>   
>> @@ -1115,7 +1370,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);
>> @@ -1154,7 +1409,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;
>>   
>> @@ -1223,7 +1479,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;
>>   
>> @@ -1752,11 +2008,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.
>>    *
>> @@ -2400,6 +2651,86 @@ 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, u32 size)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	u32 *arr = NULL;
>> +	int ret = 0;
>> +	size_t sz;
>> +
>> +	if (!np) {
>> +		ret = -ENODEV;
>> +		goto out;
>> +	}
>> +	if (!of_get_property(np, prop_name, len)) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +	sz = *len = *len / sizeof(*arr);
> 
> You obviously skipped checkpatch run. Please don't do that.

Before submitting the patchset I have already executed the checkpatch 
please find the output.
	 $ ./scripts/checkpatch.pl patch/*
	-----------------------------
	patch/0000-cover-letter.patch
	-----------------------------
	total: 0 errors, 0 warnings, 0 lines checked
	
	patch/0000-cover-letter.patch has no obvious style problems and is 
ready for submission.
	---------------------------------------------------------------------
	patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
	---------------------------------------------------------------------
	total: 0 errors, 0 warnings, 18 lines checked
	
	patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch 
has no obvious style problems and is ready for submission.
	---------------------------------------------------------------------
	patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
	---------------------------------------------------------------------
	total: 0 errors, 0 warnings, 494 lines checked
	
	patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch 
has no obvious style problems and is ready for submission.

> 
>> +	if (sz <= 0 || (size > 0 && (sz > size))) {
>> +		dev_err(dev, "%s invalid size\n", prop_name);
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
>> +	if (!arr) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
>> +	if (ret < 0) {
>> +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
>> +		goto out;
>> +	}
>> +	*bw_vecs = arr;
>> +out:
>> +	if (ret)
>> +		*len = 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;
>> +	u32 dll_values[10];
>> +	int ret = 0, i;
>> +
>> +	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
>> +		&dll_table, &dll_table_len, 0))
>> +		goto skip_dll;
> 
> Missing update for the bindings.

I will update in the next patchset.

> 
>> +
>> +	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");
>> +		ret = -EINVAL;
>> +		goto skip_dll;
>> +	}
>> +
>> +	for (i = 0; i < 5; i++) {
> 
> Magic value 5, replace with ARRAY_SIZE

I will fix in next patchset.

> 
>> +		dll_values[2 * i] = dll_table[i];
>> +		dll_values[2 * i + 1] = dll_table[i + 5];
>> +	}
>> +
>> +	for (i = 0; i < 10; i++)
>> +		dll_table[i] = dll_values[i];
> 
> So three memory copies to rearrange the order of values? That sounds
> like a horrible solution.

I will fix in next patchset.

> 
>> +
>> +	memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
>> +	msm_host->artanis_dll = true;
>> +
>> +skip_dll:
>> +	if (!dll_table) {
>> +		msm_host->artanis_dll = false;
>> +		dev_err(dev, "Failed to get dll hsr settings from dt\n");
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   static int sdhci_msm_probe(struct platform_device *pdev)
>>   {
>>   	struct sdhci_host *host;
>> @@ -2446,6 +2777,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 V2 2/2] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Dmitry Baryshkov]

On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> 
> 
> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> > On Wed, Dec 18, 2024 at 02:40:57PM +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 and also incorporate support for HS200 and
> > > HS400 DLL settings using the device tree.
> > 
> > "foo also bar" usually points out that there should be two separate
> > commits.
> 
> Thanks for review. I will split it into two patches.
> 
> > 
> > > 
> > > Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > > Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
> > > Signed-off-by: Jun Li <liju@codeaurora.org>
> > 
> > This is very strange and incorrect.
> 
> Thanks for review. I will fix the format.

Well. If you write that you will fix the format, may I ask, how or what
do you plan to fix?

> 
> > 
> > > ---
> > >   drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
> > >   1 file changed, 353 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > > index 2a5e588779fc..4ecb362f7f2a 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
> > 
> > What about the platforms where TCXO has different frequency?
> > 
> 
> All emmc targets have 192 Mhz as TCXO freq.

So, it's not a TCXO freq, but some other base freq?

> 
> > >   #define INVALID_TUNING_PHASE	-1
> > >   #define SDHCI_MSM_MIN_CLOCK	400000
> > > @@ -256,6 +258,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.
> > 
> > Why?
> 
> I will update the comment message in next patchset.

Well, you can respond to a question first. And once something is settled
you can get that to the commit message. It might save some round-trip
time.

> 
> > 
> > > + */
> > > +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 +279,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 +308,17 @@ struct sdhci_msm_host {
> > >   	u32 dll_config;
> > >   	u32 ddr_config;
> > >   	bool vqmmc_enabled;
> > > +	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)
> > > @@ -778,6 +805,210 @@ 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;
> > > +}
> > 
> > Why??? Why do you need a function to return a static value?
> > 
> 
> This is just rearrangement of the function. This function already exist,
> moving here to avoid predeclaration.

Okay. 

> > > +
> > > +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_round_rate(core_clk, clk_get_rate(core_clk));
> > > +
> > > +	if (host->clock != msm_host->clk_rate)
> > > +		sup_clk = sup_clk / 2;
> > > +
> > > +	return sup_clk;
> > 
> > Why?
> 
> Sorry, I did not understand your question. Can you please explain in detail.

Please explain the maths. You get the rate from the clock, then you
round it, but it is the rate that has just been returned, so there
should be no need to round it. And after that there a division by two
for some reason. So I've asked for an explanation for that code.

> 
> > 
> > > +}
> > > +
> > > +/* 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 & ~(0xFF << 10)) | (mclk_freq << 10);
> > 
> > GENMASK, FIELD_PREP?
> 
> Sure I will use the suggested macros.
> 
> > 
> > > +			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);
> > > @@ -900,14 +1131,35 @@ 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)
> > > +{
> > > +	unsigned char timing = host->mmc->ios.timing;
> > > +	int ret;
> > > +
> > > +	if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
> > > +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
> > > +	else
> > > +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +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__);
> > > @@ -915,7 +1167,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;
> > > @@ -1003,7 +1255,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;
> > > @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> > >   					sdhci_priv_msm_offset(host);
> > >   	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
> > > -
> > 
> > Unrelated, please drop.
> 
> I will fix it in next patchset.
> 
> > 
> > >   	/*
> > >   	 * Currently the core_ddr_config register defaults to desired
> > >   	 * configuration on reset. Currently reprogramming the power on
> > > @@ -1026,7 +1277,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 +
> > > @@ -1083,11 +1338,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__);
> > > @@ -1095,7 +1349,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;
> > > @@ -1115,7 +1370,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);
> > > @@ -1154,7 +1409,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;
> > > @@ -1223,7 +1479,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;
> > > @@ -1752,11 +2008,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.
> > >    *
> > > @@ -2400,6 +2651,86 @@ 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, u32 size)
> > > +{
> > > +	struct device_node *np = dev->of_node;
> > > +	u32 *arr = NULL;
> > > +	int ret = 0;
> > > +	size_t sz;
> > > +
> > > +	if (!np) {
> > > +		ret = -ENODEV;
> > > +		goto out;
> > > +	}
> > > +	if (!of_get_property(np, prop_name, len)) {
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +	sz = *len = *len / sizeof(*arr);
> > 
> > You obviously skipped checkpatch run. Please don't do that.
> 
> Before submitting the patchset I have already executed the checkpatch please
> find the output.
> 	 $ ./scripts/checkpatch.pl patch/*
> 	-----------------------------
> 	patch/0000-cover-letter.patch
> 	-----------------------------
> 	total: 0 errors, 0 warnings, 0 lines checked
> 	
> 	patch/0000-cover-letter.patch has no obvious style problems and is ready
> for submission.
> 	---------------------------------------------------------------------
> 	patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
> 	---------------------------------------------------------------------
> 	total: 0 errors, 0 warnings, 18 lines checked
> 	
> 	patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch has
> no obvious style problems and is ready for submission.
> 	---------------------------------------------------------------------
> 	patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
> 	---------------------------------------------------------------------
> 	total: 0 errors, 0 warnings, 494 lines checked
> 	
> 	patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch has
> no obvious style problems and is ready for submission.

Strangely enogh checkpatch.pl doesn't warn about this line, although it
has the check for it:

                if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
                        CHK("MULTIPLE_ASSIGNMENTS",
                            "multiple assignments should be avoided\n" . $herecurr);
                }

Running checkpatch.pl --strict will give you more things to fix though.

And anyway, the API not so logical. You pass the size, then you return
the number of elements through the len pointer. Please pass and return
the same thing (e.g. pass the number of elements in the passed array,
return the number of elements retrieved from DT).

> 
> > 
> > > +	if (sz <= 0 || (size > 0 && (sz > size))) {
> > > +		dev_err(dev, "%s invalid size\n", prop_name);
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +
> > > +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> > > +	if (!arr) {
> > > +		ret = -ENOMEM;
> > > +		goto out;
> > > +	}
> > > +
> > > +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> > > +		goto out;
> > > +	}
> > > +	*bw_vecs = arr;
> > > +out:
> > > +	if (ret)
> > > +		*len = 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;
> > > +	u32 dll_values[10];
> > > +	int ret = 0, i;
> > > +
> > > +	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
> > > +		&dll_table, &dll_table_len, 0))
> > > +		goto skip_dll;
> > 
> > Missing update for the bindings.
> 
> I will update in the next patchset.

Please update your internal upstreaming site: bindings changes MUST
always come before the corresponding driver changes. If it is already
documented there, you probably have a demerit for not following the
documented process.

> 
> > 
> > > +
> > > +	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");
> > > +		ret = -EINVAL;
> > > +		goto skip_dll;
> > > +	}
> > > +
> > > +	for (i = 0; i < 5; i++) {
> > 
> > Magic value 5, replace with ARRAY_SIZE
> 
> I will fix in next patchset.
> 
> > 
> > > +		dll_values[2 * i] = dll_table[i];
> > > +		dll_values[2 * i + 1] = dll_table[i + 5];
> > > +	}
> > > +
> > > +	for (i = 0; i < 10; i++)
> > > +		dll_table[i] = dll_values[i];
> > 
> > So three memory copies to rearrange the order of values? That sounds
> > like a horrible solution.
> 
> I will fix in next patchset.
> 
> > 
> > > +
> > > +	memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
> > > +	msm_host->artanis_dll = true;
> > > +
> > > +skip_dll:
> > > +	if (!dll_table) {
> > > +		msm_host->artanis_dll = false;
> > > +		dev_err(dev, "Failed to get dll hsr settings from dt\n");
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >   static int sdhci_msm_probe(struct platform_device *pdev)
> > >   {
> > >   	struct sdhci_host *host;
> > > @@ -2446,6 +2777,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 V2 2/2] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Sachin Gupta]

On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>
>>
>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>> On Wed, Dec 18, 2024 at 02:40:57PM +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 and also incorporate support for HS200 and
>>>> HS400 DLL settings using the device tree.
>>>
>>> "foo also bar" usually points out that there should be two separate
>>> commits.
>>
>> Thanks for review. I will split it into two patches.
>>
>>>
>>>>
>>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>>>> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
>>>> Signed-off-by: Jun Li <liju@codeaurora.org>
>>>
>>> This is very strange and incorrect.
>>
>> Thanks for review. I will fix the format.
> 
> Well. If you write that you will fix the format, may I ask, how or what
> do you plan to fix?
> 

I will add Co-developed-by and Signed-off-by for co-authors and add 
signed-off-by for author at the last.

>>
>>>
>>>> ---
>>>>    drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
>>>>    1 file changed, 353 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>> index 2a5e588779fc..4ecb362f7f2a 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
>>>
>>> What about the platforms where TCXO has different frequency?
>>>
>>
>> All emmc targets have 192 Mhz as TCXO freq.
> 
> So, it's not a TCXO freq, but some other base freq?
> 

It’s a TCXO frequency, this is as per hardware team recommendation.

>>
>>>>    #define INVALID_TUNING_PHASE	-1
>>>>    #define SDHCI_MSM_MIN_CLOCK	400000
>>>> @@ -256,6 +258,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.
>>>
>>> Why?
>>
>> I will update the comment message in next patchset.
> 
> Well, you can respond to a question first. And once something is settled
> you can get that to the commit message. It might save some round-trip
> time.
> 

My intention for the comment is that as per Hardware Documents, as part 
of DLL sequence DLL registers should be configured first. My above 
comment is confusing, will remove it.

>>
>>>
>>>> + */
>>>> +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 +279,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 +308,17 @@ struct sdhci_msm_host {
>>>>    	u32 dll_config;
>>>>    	u32 ddr_config;
>>>>    	bool vqmmc_enabled;
>>>> +	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)
>>>> @@ -778,6 +805,210 @@ 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;
>>>> +}
>>>
>>> Why??? Why do you need a function to return a static value?
>>>
>>
>> This is just rearrangement of the function. This function already exist,
>> moving here to avoid predeclaration.
> 
> Okay.
> 
>>>> +
>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
>>>> +
>>>> +	if (host->clock != msm_host->clk_rate)
>>>> +		sup_clk = sup_clk / 2;
>>>> +
>>>> +	return sup_clk;
>>>
>>> Why?
>>
>> Sorry, I did not understand your question. Can you please explain in detail.
> 
> Please explain the maths. You get the rate from the clock, then you
> round it, but it is the rate that has just been returned, so there
> should be no need to round it. And after that there a division by two
> for some reason. So I've asked for an explanation for that code.
> 

clk_round_rate is used in case of over clocking issue we can round it to 
the usable frequency. Divide by 2 is used as for HS400 the tuning 
happens in HS200 mode only so to update the frequency to 192 Mhz.

>>
>>>
>>>> +}
>>>> +
>>>> +/* 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 & ~(0xFF << 10)) | (mclk_freq << 10);
>>>
>>> GENMASK, FIELD_PREP?
>>
>> Sure I will use the suggested macros.
>>
>>>
>>>> +			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);
>>>> @@ -900,14 +1131,35 @@ 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)
>>>> +{
>>>> +	unsigned char timing = host->mmc->ios.timing;
>>>> +	int ret;
>>>> +
>>>> +	if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
>>>> +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
>>>> +	else
>>>> +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +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__);
>>>> @@ -915,7 +1167,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;
>>>> @@ -1003,7 +1255,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;
>>>> @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>>>>    					sdhci_priv_msm_offset(host);
>>>>    	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
>>>> -
>>>
>>> Unrelated, please drop.
>>
>> I will fix it in next patchset.
>>
>>>
>>>>    	/*
>>>>    	 * Currently the core_ddr_config register defaults to desired
>>>>    	 * configuration on reset. Currently reprogramming the power on
>>>> @@ -1026,7 +1277,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 +
>>>> @@ -1083,11 +1338,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__);
>>>> @@ -1095,7 +1349,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;
>>>> @@ -1115,7 +1370,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);
>>>> @@ -1154,7 +1409,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;
>>>> @@ -1223,7 +1479,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;
>>>> @@ -1752,11 +2008,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.
>>>>     *
>>>> @@ -2400,6 +2651,86 @@ 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, u32 size)
>>>> +{
>>>> +	struct device_node *np = dev->of_node;
>>>> +	u32 *arr = NULL;
>>>> +	int ret = 0;
>>>> +	size_t sz;
>>>> +
>>>> +	if (!np) {
>>>> +		ret = -ENODEV;
>>>> +		goto out;
>>>> +	}
>>>> +	if (!of_get_property(np, prop_name, len)) {
>>>> +		ret = -EINVAL;
>>>> +		goto out;
>>>> +	}
>>>> +	sz = *len = *len / sizeof(*arr);
>>>
>>> You obviously skipped checkpatch run. Please don't do that.
>>
>> Before submitting the patchset I have already executed the checkpatch please
>> find the output.
>> 	 $ ./scripts/checkpatch.pl patch/*
>> 	-----------------------------
>> 	patch/0000-cover-letter.patch
>> 	-----------------------------
>> 	total: 0 errors, 0 warnings, 0 lines checked
>> 	
>> 	patch/0000-cover-letter.patch has no obvious style problems and is ready
>> for submission.
>> 	---------------------------------------------------------------------
>> 	patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
>> 	---------------------------------------------------------------------
>> 	total: 0 errors, 0 warnings, 18 lines checked
>> 	
>> 	patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch has
>> no obvious style problems and is ready for submission.
>> 	---------------------------------------------------------------------
>> 	patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
>> 	---------------------------------------------------------------------
>> 	total: 0 errors, 0 warnings, 494 lines checked
>> 	
>> 	patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch has
>> no obvious style problems and is ready for submission.
> 
> Strangely enogh checkpatch.pl doesn't warn about this line, although it
> has the check for it:
> 
>                  if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
>                          CHK("MULTIPLE_ASSIGNMENTS",
>                              "multiple assignments should be avoided\n" . $herecurr);
>                  }
> 
> Running checkpatch.pl --strict will give you more things to fix though.
> 

Sorry, even with the --strict checkpatch does not throw error or 
warning. But I will modify the line to make it simple assignment.

> And anyway, the API not so logical. You pass the size, then you return
> the number of elements through the len pointer. Please pass and return
> the same thing (e.g. pass the number of elements in the passed array,
> return the number of elements retrieved from DT).
> 

Thank you for the comment,  I will modify the API and remove size input 
variable. we can use only data and length.


>>
>>>
>>>> +	if (sz <= 0 || (size > 0 && (sz > size))) {
>>>> +		dev_err(dev, "%s invalid size\n", prop_name);
>>>> +		ret = -EINVAL;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
>>>> +	if (!arr) {
>>>> +		ret = -ENOMEM;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
>>>> +	if (ret < 0) {
>>>> +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
>>>> +		goto out;
>>>> +	}
>>>> +	*bw_vecs = arr;
>>>> +out:
>>>> +	if (ret)
>>>> +		*len = 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;
>>>> +	u32 dll_values[10];
>>>> +	int ret = 0, i;
>>>> +
>>>> +	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
>>>> +		&dll_table, &dll_table_len, 0))
>>>> +		goto skip_dll;
>>>
>>> Missing update for the bindings.
>>
>> I will update in the next patchset.
> 
> Please update your internal upstreaming site: bindings changes MUST
> always come before the corresponding driver changes. If it is already
> documented there, you probably have a demerit for not following the
> documented process.
> 

Sure I will push DT binding change as a first patch in a new patch series.

>>
>>>
>>>> +
>>>> +	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");
>>>> +		ret = -EINVAL;
>>>> +		goto skip_dll;
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < 5; i++) {
>>>
>>> Magic value 5, replace with ARRAY_SIZE
>>
>> I will fix in next patchset.
>>
>>>
>>>> +		dll_values[2 * i] = dll_table[i];
>>>> +		dll_values[2 * i + 1] = dll_table[i + 5];
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < 10; i++)
>>>> +		dll_table[i] = dll_values[i];
>>>
>>> So three memory copies to rearrange the order of values? That sounds
>>> like a horrible solution.
>>
>> I will fix in next patchset.
>>
>>>
>>>> +
>>>> +	memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
>>>> +	msm_host->artanis_dll = true;
>>>> +
>>>> +skip_dll:
>>>> +	if (!dll_table) {
>>>> +		msm_host->artanis_dll = false;
>>>> +		dev_err(dev, "Failed to get dll hsr settings from dt\n");
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>>    static int sdhci_msm_probe(struct platform_device *pdev)
>>>>    {
>>>>    	struct sdhci_host *host;
>>>> @@ -2446,6 +2777,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 V2 2/2] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Dmitry Baryshkov]

On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
> 
> 
> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> > On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> > > 
> > > 
> > > On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> > > > On Wed, Dec 18, 2024 at 02:40:57PM +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 and also incorporate support for HS200 and
> > > > > HS400 DLL settings using the device tree.
> > > > 
> > > > "foo also bar" usually points out that there should be two separate
> > > > commits.
> > > 
> > > Thanks for review. I will split it into two patches.
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> > > > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > > > > Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
> > > > > Signed-off-by: Jun Li <liju@codeaurora.org>
> > > > 
> > > > This is very strange and incorrect.
> > > 
> > > Thanks for review. I will fix the format.
> > 
> > Well. If you write that you will fix the format, may I ask, how or what
> > do you plan to fix?
> > 
> 
> I will add Co-developed-by and Signed-off-by for co-authors and add
> signed-off-by for author at the last.
> 
> > > 
> > > > 
> > > > > ---
> > > > >    drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
> > > > >    1 file changed, 353 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > > > > index 2a5e588779fc..4ecb362f7f2a 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
> > > > 
> > > > What about the platforms where TCXO has different frequency?
> > > > 
> > > 
> > > All emmc targets have 192 Mhz as TCXO freq.
> > 
> > So, it's not a TCXO freq, but some other base freq?
> > 
> 
> It’s a TCXO frequency, this is as per hardware team recommendation.

First of all, it's not 192 MHz. Second, is it an actual XO freq or some
other interim frequency? I'm asking since some platforms use higher
frequencies for the XO.

> 
> > > 
> > > > >    #define INVALID_TUNING_PHASE	-1
> > > > >    #define SDHCI_MSM_MIN_CLOCK	400000
> > > > > @@ -256,6 +258,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.
> > > > 
> > > > Why?
> > > 
> > > I will update the comment message in next patchset.
> > 
> > Well, you can respond to a question first. And once something is settled
> > you can get that to the commit message. It might save some round-trip
> > time.
> > 
> 
> My intention for the comment is that as per Hardware Documents, as part of
> DLL sequence DLL registers should be configured first. My above comment is
> confusing, will remove it.
> 
> > > 
> > > > 
> > > > > + */
> > > > > +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 +279,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 +308,17 @@ struct sdhci_msm_host {
> > > > >    	u32 dll_config;
> > > > >    	u32 ddr_config;
> > > > >    	bool vqmmc_enabled;
> > > > > +	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)
> > > > > @@ -778,6 +805,210 @@ 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;
> > > > > +}
> > > > 
> > > > Why??? Why do you need a function to return a static value?
> > > > 
> > > 
> > > This is just rearrangement of the function. This function already exist,
> > > moving here to avoid predeclaration.
> > 
> > Okay.
> > 
> > > > > +
> > > > > +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_round_rate(core_clk, clk_get_rate(core_clk));
> > > > > +
> > > > > +	if (host->clock != msm_host->clk_rate)
> > > > > +		sup_clk = sup_clk / 2;
> > > > > +
> > > > > +	return sup_clk;
> > > > 
> > > > Why?
> > > 
> > > Sorry, I did not understand your question. Can you please explain in detail.
> > 
> > Please explain the maths. You get the rate from the clock, then you
> > round it, but it is the rate that has just been returned, so there
> > should be no need to round it. And after that there a division by two
> > for some reason. So I've asked for an explanation for that code.
> > 
> 
> clk_round_rate is used in case of over clocking issue we can round it to the
> usable frequency.

If it is a frequency _returned_ by the clock driver, why do you need to
round it? It sounds like that freq should be usable anyway.

> Divide by 2 is used as for HS400 the tuning happens in
> HS200 mode only so to update the frequency to 192 Mhz.

Again, is it really 192 MHz? Or 19.2 MHz?
Also if it is for HS400, then shouldn't /2 be limited to that mode?

> 
> > > 
> > > > 
> > > > > +}
> > > > > +
> > > > > +/* 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 & ~(0xFF << 10)) | (mclk_freq << 10);
> > > > 
> > > > GENMASK, FIELD_PREP?
> > > 
> > > Sure I will use the suggested macros.
> > > 
> > > > 
> > > > > +			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);
> > > > > @@ -900,14 +1131,35 @@ 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)
> > > > > +{
> > > > > +	unsigned char timing = host->mmc->ios.timing;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
> > > > > +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
> > > > > +	else
> > > > > +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +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__);
> > > > > @@ -915,7 +1167,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;
> > > > > @@ -1003,7 +1255,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;
> > > > > @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> > > > >    					sdhci_priv_msm_offset(host);
> > > > >    	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
> > > > > -
> > > > 
> > > > Unrelated, please drop.
> > > 
> > > I will fix it in next patchset.
> > > 
> > > > 
> > > > >    	/*
> > > > >    	 * Currently the core_ddr_config register defaults to desired
> > > > >    	 * configuration on reset. Currently reprogramming the power on
> > > > > @@ -1026,7 +1277,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 +
> > > > > @@ -1083,11 +1338,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__);
> > > > > @@ -1095,7 +1349,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;
> > > > > @@ -1115,7 +1370,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);
> > > > > @@ -1154,7 +1409,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;
> > > > > @@ -1223,7 +1479,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;
> > > > > @@ -1752,11 +2008,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.
> > > > >     *
> > > > > @@ -2400,6 +2651,86 @@ 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, u32 size)
> > > > > +{
> > > > > +	struct device_node *np = dev->of_node;
> > > > > +	u32 *arr = NULL;
> > > > > +	int ret = 0;
> > > > > +	size_t sz;
> > > > > +
> > > > > +	if (!np) {
> > > > > +		ret = -ENODEV;
> > > > > +		goto out;
> > > > > +	}
> > > > > +	if (!of_get_property(np, prop_name, len)) {
> > > > > +		ret = -EINVAL;
> > > > > +		goto out;
> > > > > +	}
> > > > > +	sz = *len = *len / sizeof(*arr);
> > > > 
> > > > You obviously skipped checkpatch run. Please don't do that.
> > > 
> > > Before submitting the patchset I have already executed the checkpatch please
> > > find the output.
> > > 	 $ ./scripts/checkpatch.pl patch/*
> > > 	-----------------------------
> > > 	patch/0000-cover-letter.patch
> > > 	-----------------------------
> > > 	total: 0 errors, 0 warnings, 0 lines checked
> > > 	
> > > 	patch/0000-cover-letter.patch has no obvious style problems and is ready
> > > for submission.
> > > 	---------------------------------------------------------------------
> > > 	patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
> > > 	---------------------------------------------------------------------
> > > 	total: 0 errors, 0 warnings, 18 lines checked
> > > 	
> > > 	patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch has
> > > no obvious style problems and is ready for submission.
> > > 	---------------------------------------------------------------------
> > > 	patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
> > > 	---------------------------------------------------------------------
> > > 	total: 0 errors, 0 warnings, 494 lines checked
> > > 	
> > > 	patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch has
> > > no obvious style problems and is ready for submission.
> > 
> > Strangely enogh checkpatch.pl doesn't warn about this line, although it
> > has the check for it:
> > 
> >                  if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
> >                          CHK("MULTIPLE_ASSIGNMENTS",
> >                              "multiple assignments should be avoided\n" . $herecurr);
> >                  }
> > 
> > Running checkpatch.pl --strict will give you more things to fix though.
> > 
> 
> Sorry, even with the --strict checkpatch does not throw error or warning.
> But I will modify the line to make it simple assignment.

And all other checkpatch issues too, please.

> 
> > And anyway, the API not so logical. You pass the size, then you return
> > the number of elements through the len pointer. Please pass and return
> > the same thing (e.g. pass the number of elements in the passed array,
> > return the number of elements retrieved from DT).
> > 
> 
> Thank you for the comment,  I will modify the API and remove size input
> variable. we can use only data and length.
> 
> 
> > > 
> > > > 
> > > > > +	if (sz <= 0 || (size > 0 && (sz > size))) {
> > > > > +		dev_err(dev, "%s invalid size\n", prop_name);
> > > > > +		ret = -EINVAL;
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> > > > > +	if (!arr) {
> > > > > +		ret = -ENOMEM;
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
> > > > > +	if (ret < 0) {
> > > > > +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> > > > > +		goto out;
> > > > > +	}
> > > > > +	*bw_vecs = arr;
> > > > > +out:
> > > > > +	if (ret)
> > > > > +		*len = 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;
> > > > > +	u32 dll_values[10];
> > > > > +	int ret = 0, i;
> > > > > +
> > > > > +	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
> > > > > +		&dll_table, &dll_table_len, 0))
> > > > > +		goto skip_dll;
> > > > 
> > > > Missing update for the bindings.
> > > 
> > > I will update in the next patchset.
> > 
> > Please update your internal upstreaming site: bindings changes MUST
> > always come before the corresponding driver changes. If it is already
> > documented there, you probably have a demerit for not following the
> > documented process.
> > 
> 
> Sure I will push DT binding change as a first patch in a new patch series.
> 
> > > 
> > > > 
> > > > > +
> > > > > +	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");
> > > > > +		ret = -EINVAL;
> > > > > +		goto skip_dll;
> > > > > +	}
> > > > > +
> > > > > +	for (i = 0; i < 5; i++) {
> > > > 
> > > > Magic value 5, replace with ARRAY_SIZE
> > > 
> > > I will fix in next patchset.
> > > 
> > > > 
> > > > > +		dll_values[2 * i] = dll_table[i];
> > > > > +		dll_values[2 * i + 1] = dll_table[i + 5];
> > > > > +	}
> > > > > +
> > > > > +	for (i = 0; i < 10; i++)
> > > > > +		dll_table[i] = dll_values[i];
> > > > 
> > > > So three memory copies to rearrange the order of values? That sounds
> > > > like a horrible solution.
> > > 
> > > I will fix in next patchset.
> > > 
> > > > 
> > > > > +
> > > > > +	memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
> > > > > +	msm_host->artanis_dll = true;
> > > > > +
> > > > > +skip_dll:
> > > > > +	if (!dll_table) {
> > > > > +		msm_host->artanis_dll = false;
> > > > > +		dev_err(dev, "Failed to get dll hsr settings from dt\n");
> > > > > +	}
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > >    static int sdhci_msm_probe(struct platform_device *pdev)
> > > > >    {
> > > > >    	struct sdhci_host *host;
> > > > > @@ -2446,6 +2777,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 V2 2/2] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Sachin Gupta]

On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>
>>
>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>
>>>>
>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +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 and also incorporate support for HS200 and
>>>>>> HS400 DLL settings using the device tree.
>>>>>
>>>>> "foo also bar" usually points out that there should be two separate
>>>>> commits.
>>>>
>>>> Thanks for review. I will split it into two patches.
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>>>> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>>>>>> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
>>>>>> Signed-off-by: Jun Li <liju@codeaurora.org>
>>>>>
>>>>> This is very strange and incorrect.
>>>>
>>>> Thanks for review. I will fix the format.
>>>
>>> Well. If you write that you will fix the format, may I ask, how or what
>>> do you plan to fix?
>>>
>>
>> I will add Co-developed-by and Signed-off-by for co-authors and add
>> signed-off-by for author at the last.
>>
>>>>
>>>>>
>>>>>> ---
>>>>>>     drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
>>>>>>     1 file changed, 353 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>>>> index 2a5e588779fc..4ecb362f7f2a 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
>>>>>
>>>>> What about the platforms where TCXO has different frequency?
>>>>>
>>>>
>>>> All emmc targets have 192 Mhz as TCXO freq.
>>>
>>> So, it's not a TCXO freq, but some other base freq?
>>>
>>
>> It’s a TCXO frequency, this is as per hardware team recommendation.
> 
> First of all, it's not 192 MHz. Second, is it an actual XO freq or some
> other interim frequency? I'm asking since some platforms use higher
> frequencies for the XO.
> 

Yes its 19.2 Mhz sorry for the confusion.

>>
>>>>
>>>>>>     #define INVALID_TUNING_PHASE	-1
>>>>>>     #define SDHCI_MSM_MIN_CLOCK	400000
>>>>>> @@ -256,6 +258,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.
>>>>>
>>>>> Why?
>>>>
>>>> I will update the comment message in next patchset.
>>>
>>> Well, you can respond to a question first. And once something is settled
>>> you can get that to the commit message. It might save some round-trip
>>> time.
>>>
>>
>> My intention for the comment is that as per Hardware Documents, as part of
>> DLL sequence DLL registers should be configured first. My above comment is
>> confusing, will remove it.
>>
>>>>
>>>>>
>>>>>> + */
>>>>>> +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 +279,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 +308,17 @@ struct sdhci_msm_host {
>>>>>>     	u32 dll_config;
>>>>>>     	u32 ddr_config;
>>>>>>     	bool vqmmc_enabled;
>>>>>> +	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)
>>>>>> @@ -778,6 +805,210 @@ 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;
>>>>>> +}
>>>>>
>>>>> Why??? Why do you need a function to return a static value?
>>>>>
>>>>
>>>> This is just rearrangement of the function. This function already exist,
>>>> moving here to avoid predeclaration.
>>>
>>> Okay.
>>>
>>>>>> +
>>>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>> +
>>>>>> +	if (host->clock != msm_host->clk_rate)
>>>>>> +		sup_clk = sup_clk / 2;
>>>>>> +
>>>>>> +	return sup_clk;
>>>>>
>>>>> Why?
>>>>
>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>
>>> Please explain the maths. You get the rate from the clock, then you
>>> round it, but it is the rate that has just been returned, so there
>>> should be no need to round it. And after that there a division by two
>>> for some reason. So I've asked for an explanation for that code.
>>>
>>
>> clk_round_rate is used in case of over clocking issue we can round it to the
>> usable frequency.
> 
> If it is a frequency _returned_ by the clock driver, why do you need to
> round it? It sounds like that freq should be usable anyway.
> 

I agree, rounding will be taken care by clock driver. Will remove in my 
next patch.

>> Divide by 2 is used as for HS400 the tuning happens in
>> HS200 mode only so to update the frequency to 192 Mhz.
> 
> Again, is it really 192 MHz? Or 19.2 MHz?
> Also if it is for HS400, then shouldn't /2 be limited to that mode?
> 

Yes, It is 192 MHz.
As part of eMMC Init, driver will try to init with the best mode 
supported by controller and device. In this case it is HS400 mode, But 
as part of HS400 mode, we perform Tuning in HS200 mode only where we 
need to configure half of the clock.

>>
>>>>
>>>>>
>>>>>> +}
>>>>>> +
>>>>>> +/* 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 & ~(0xFF << 10)) | (mclk_freq << 10);
>>>>>
>>>>> GENMASK, FIELD_PREP?
>>>>
>>>> Sure I will use the suggested macros.
>>>>
>>>>>
>>>>>> +			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);
>>>>>> @@ -900,14 +1131,35 @@ 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)
>>>>>> +{
>>>>>> +	unsigned char timing = host->mmc->ios.timing;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
>>>>>> +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
>>>>>> +	else
>>>>>> +		ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
>>>>>> +
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>> +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__);
>>>>>> @@ -915,7 +1167,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;
>>>>>> @@ -1003,7 +1255,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;
>>>>>> @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>>>>>>     					sdhci_priv_msm_offset(host);
>>>>>>     	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
>>>>>> -
>>>>>
>>>>> Unrelated, please drop.
>>>>
>>>> I will fix it in next patchset.
>>>>
>>>>>
>>>>>>     	/*
>>>>>>     	 * Currently the core_ddr_config register defaults to desired
>>>>>>     	 * configuration on reset. Currently reprogramming the power on
>>>>>> @@ -1026,7 +1277,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 +
>>>>>> @@ -1083,11 +1338,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__);
>>>>>> @@ -1095,7 +1349,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;
>>>>>> @@ -1115,7 +1370,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);
>>>>>> @@ -1154,7 +1409,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;
>>>>>> @@ -1223,7 +1479,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;
>>>>>> @@ -1752,11 +2008,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.
>>>>>>      *
>>>>>> @@ -2400,6 +2651,86 @@ 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, u32 size)
>>>>>> +{
>>>>>> +	struct device_node *np = dev->of_node;
>>>>>> +	u32 *arr = NULL;
>>>>>> +	int ret = 0;
>>>>>> +	size_t sz;
>>>>>> +
>>>>>> +	if (!np) {
>>>>>> +		ret = -ENODEV;
>>>>>> +		goto out;
>>>>>> +	}
>>>>>> +	if (!of_get_property(np, prop_name, len)) {
>>>>>> +		ret = -EINVAL;
>>>>>> +		goto out;
>>>>>> +	}
>>>>>> +	sz = *len = *len / sizeof(*arr);
>>>>>
>>>>> You obviously skipped checkpatch run. Please don't do that.
>>>>
>>>> Before submitting the patchset I have already executed the checkpatch please
>>>> find the output.
>>>> 	 $ ./scripts/checkpatch.pl patch/*
>>>> 	-----------------------------
>>>> 	patch/0000-cover-letter.patch
>>>> 	-----------------------------
>>>> 	total: 0 errors, 0 warnings, 0 lines checked
>>>> 	
>>>> 	patch/0000-cover-letter.patch has no obvious style problems and is ready
>>>> for submission.
>>>> 	---------------------------------------------------------------------
>>>> 	patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
>>>> 	---------------------------------------------------------------------
>>>> 	total: 0 errors, 0 warnings, 18 lines checked
>>>> 	
>>>> 	patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch has
>>>> no obvious style problems and is ready for submission.
>>>> 	---------------------------------------------------------------------
>>>> 	patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
>>>> 	---------------------------------------------------------------------
>>>> 	total: 0 errors, 0 warnings, 494 lines checked
>>>> 	
>>>> 	patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch has
>>>> no obvious style problems and is ready for submission.
>>>
>>> Strangely enogh checkpatch.pl doesn't warn about this line, although it
>>> has the check for it:
>>>
>>>                   if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
>>>                           CHK("MULTIPLE_ASSIGNMENTS",
>>>                               "multiple assignments should be avoided\n" . $herecurr);
>>>                   }
>>>
>>> Running checkpatch.pl --strict will give you more things to fix though.
>>>
>>
>> Sorry, even with the --strict checkpatch does not throw error or warning.
>> But I will modify the line to make it simple assignment.
> 
> And all other checkpatch issues too, please.
> 

Sure resolved.

>>
>>> And anyway, the API not so logical. You pass the size, then you return
>>> the number of elements through the len pointer. Please pass and return
>>> the same thing (e.g. pass the number of elements in the passed array,
>>> return the number of elements retrieved from DT).
>>>
>>
>> Thank you for the comment,  I will modify the API and remove size input
>> variable. we can use only data and length.
>>
>>
>>>>
>>>>>
>>>>>> +	if (sz <= 0 || (size > 0 && (sz > size))) {
>>>>>> +		dev_err(dev, "%s invalid size\n", prop_name);
>>>>>> +		ret = -EINVAL;
>>>>>> +		goto out;
>>>>>> +	}
>>>>>> +
>>>>>> +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
>>>>>> +	if (!arr) {
>>>>>> +		ret = -ENOMEM;
>>>>>> +		goto out;
>>>>>> +	}
>>>>>> +
>>>>>> +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
>>>>>> +	if (ret < 0) {
>>>>>> +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
>>>>>> +		goto out;
>>>>>> +	}
>>>>>> +	*bw_vecs = arr;
>>>>>> +out:
>>>>>> +	if (ret)
>>>>>> +		*len = 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;
>>>>>> +	u32 dll_values[10];
>>>>>> +	int ret = 0, i;
>>>>>> +
>>>>>> +	if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
>>>>>> +		&dll_table, &dll_table_len, 0))
>>>>>> +		goto skip_dll;
>>>>>
>>>>> Missing update for the bindings.
>>>>
>>>> I will update in the next patchset.
>>>
>>> Please update your internal upstreaming site: bindings changes MUST
>>> always come before the corresponding driver changes. If it is already
>>> documented there, you probably have a demerit for not following the
>>> documented process.
>>>
>>
>> Sure I will push DT binding change as a first patch in a new patch series.
>>
>>>>
>>>>>
>>>>>> +
>>>>>> +	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");
>>>>>> +		ret = -EINVAL;
>>>>>> +		goto skip_dll;
>>>>>> +	}
>>>>>> +
>>>>>> +	for (i = 0; i < 5; i++) {
>>>>>
>>>>> Magic value 5, replace with ARRAY_SIZE
>>>>
>>>> I will fix in next patchset.
>>>>
>>>>>
>>>>>> +		dll_values[2 * i] = dll_table[i];
>>>>>> +		dll_values[2 * i + 1] = dll_table[i + 5];
>>>>>> +	}
>>>>>> +
>>>>>> +	for (i = 0; i < 10; i++)
>>>>>> +		dll_table[i] = dll_values[i];
>>>>>
>>>>> So three memory copies to rearrange the order of values? That sounds
>>>>> like a horrible solution.
>>>>
>>>> I will fix in next patchset.
>>>>
>>>>>
>>>>>> +
>>>>>> +	memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
>>>>>> +	msm_host->artanis_dll = true;
>>>>>> +
>>>>>> +skip_dll:
>>>>>> +	if (!dll_table) {
>>>>>> +		msm_host->artanis_dll = false;
>>>>>> +		dev_err(dev, "Failed to get dll hsr settings from dt\n");
>>>>>> +	}
>>>>>> +
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>>     static int sdhci_msm_probe(struct platform_device *pdev)
>>>>>>     {
>>>>>>     	struct sdhci_host *host;
>>>>>> @@ -2446,6 +2777,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 V2 2/2] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Dmitry Baryshkov]

On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
> 
> 
> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
> > On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
> > > 
> > > 
> > > On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> > > > On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> > > > > 
> > > > > 
> > > > > On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> > > > > > On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:

> > > > > > > +
> > > > > > > +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_round_rate(core_clk, clk_get_rate(core_clk));
> > > > > > > +
> > > > > > > +	if (host->clock != msm_host->clk_rate)
> > > > > > > +		sup_clk = sup_clk / 2;
> > > > > > > +
> > > > > > > +	return sup_clk;
> > > > > > 
> > > > > > Why?
> > > > > 
> > > > > Sorry, I did not understand your question. Can you please explain in detail.
> > > > 
> > > > Please explain the maths. You get the rate from the clock, then you
> > > > round it, but it is the rate that has just been returned, so there
> > > > should be no need to round it. And after that there a division by two
> > > > for some reason. So I've asked for an explanation for that code.
> > > > 
> > > 
> > > clk_round_rate is used in case of over clocking issue we can round it to the
> > > usable frequency.
> > 
> > If it is a frequency _returned_ by the clock driver, why do you need to
> > round it? It sounds like that freq should be usable anyway.
> > 
> 
> I agree, rounding will be taken care by clock driver. Will remove in my next
> patch.
> 
> > > Divide by 2 is used as for HS400 the tuning happens in
> > > HS200 mode only so to update the frequency to 192 Mhz.
> > 
> > Again, is it really 192 MHz? Or 19.2 MHz?
> > Also if it is for HS400, then shouldn't /2 be limited to that mode?
> > 
> 
> Yes, It is 192 MHz.

Good, thanks for the confirmation.

> As part of eMMC Init, driver will try to init with the best mode supported
> by controller and device. In this case it is HS400 mode, But as part of
> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
> half of the clock.

This isn't an answer to the question. Let me rephrase it for you: if the
/2 is only used for HS400, why should it be attempted in all other
modes? Please limit the /2 just to HS400.

-- 
With best wishes
Dmitry

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

[Ram Prakash Gupta]

On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>> +
>>>>>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>> +
>>>>>>>> +	if (host->clock != msm_host->clk_rate)
>>>>>>>> +		sup_clk = sup_clk / 2;
>>>>>>>> +
>>>>>>>> +	return sup_clk;
>>>>>>> Why?
>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>> round it, but it is the rate that has just been returned, so there
>>>>> should be no need to round it. And after that there a division by two
>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>
>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>> usable frequency.
>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>> round it? It sounds like that freq should be usable anyway.
>>>
>> I agree, rounding will be taken care by clock driver. Will remove in my next
>> patch.
>>
>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>
>> Yes, It is 192 MHz.
> Good, thanks for the confirmation.
>
>> As part of eMMC Init, driver will try to init with the best mode supported
>> by controller and device. In this case it is HS400 mode, But as part of
>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>> half of the clock.
> This isn't an answer to the question. Let me rephrase it for you: if the
> /2 is only used for HS400, why should it be attempted in all other
> modes? Please limit the /2 just to HS400.

Hi Dmitry,

like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
hs200 tuning and hs400 selection. In this case we must divide clk by 2
to get 192MHz and we find this as host->clock wont be equal to 
msm_host->clk_rate. Now if we go for only HS200 mode supported card, there
the supported clock value would be 192Mhz itself and we need to pass
clk freq as 192MHz itself, hence division by 2 wont be needed, that is
achieved there as host->clock would be equal to msm_host->clk_rate. Hence
no other check is needed here.

sorry for it took time to update as I was gathering all this data.
since Sachin have already pushed patchset #3, and if this explanation
helps, let me know if we can continue on patchset #3.

Thanks,
Ram

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

[Dmitry Baryshkov]

On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
> 
> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
> > On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
> >> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
> >>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
> >>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> >>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> >>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> >>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
> >>>>>>>> +
> >>>>>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
> >>>>>>>> +
> >>>>>>>> +	if (host->clock != msm_host->clk_rate)
> >>>>>>>> +		sup_clk = sup_clk / 2;
> >>>>>>>> +
> >>>>>>>> +	return sup_clk;
> >>>>>>> Why?
> >>>>>> Sorry, I did not understand your question. Can you please explain in detail.
> >>>>> Please explain the maths. You get the rate from the clock, then you
> >>>>> round it, but it is the rate that has just been returned, so there
> >>>>> should be no need to round it. And after that there a division by two
> >>>>> for some reason. So I've asked for an explanation for that code.
> >>>>>
> >>>> clk_round_rate is used in case of over clocking issue we can round it to the
> >>>> usable frequency.
> >>> If it is a frequency _returned_ by the clock driver, why do you need to
> >>> round it? It sounds like that freq should be usable anyway.
> >>>
> >> I agree, rounding will be taken care by clock driver. Will remove in my next
> >> patch.
> >>
> >>>> Divide by 2 is used as for HS400 the tuning happens in
> >>>> HS200 mode only so to update the frequency to 192 Mhz.
> >>> Again, is it really 192 MHz? Or 19.2 MHz?
> >>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
> >>>
> >> Yes, It is 192 MHz.
> > Good, thanks for the confirmation.
> >
> >> As part of eMMC Init, driver will try to init with the best mode supported
> >> by controller and device. In this case it is HS400 mode, But as part of
> >> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
> >> half of the clock.
> > This isn't an answer to the question. Let me rephrase it for you: if the
> > /2 is only used for HS400, why should it be attempted in all other
> > modes? Please limit the /2 just to HS400.
> 
> Hi Dmitry,
> 
> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
> hs200 tuning and hs400 selection. In this case we must divide clk by 2
> to get 192MHz and we find this as host->clock wont be equal to 
> msm_host->clk_rate.

What are host->clock and msm_host->clk_rate at this point?
What is the host->flags value? See sdhci_msm_hc_select_mode().

> Now if we go for only HS200 mode supported card, there
> the supported clock value would be 192Mhz itself and we need to pass
> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
> no other check is needed here.

Please think about the cause, not about the symptom. Clocks being
unequal is a result of some other checks being performed by the driver.
Please use those checks too.

> 
> sorry for it took time to update as I was gathering all this data.

6 months? Well, that's a nice time to "gather all this data".

> since Sachin have already pushed patchset #3, and if this explanation
> helps, let me know if we can continue on patchset #3.
> 
> Thanks,
> Ram
> 

-- 
With best wishes
Dmitry

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

[Ram Prakash Gupta]

On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>> +
>>>>>>>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>> +
>>>>>>>>>> +	if (host->clock != msm_host->clk_rate)
>>>>>>>>>> +		sup_clk = sup_clk / 2;
>>>>>>>>>> +
>>>>>>>>>> +	return sup_clk;
>>>>>>>>> Why?
>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>
>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>> usable frequency.
>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>
>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>> patch.
>>>>
>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>
>>>> Yes, It is 192 MHz.
>>> Good, thanks for the confirmation.
>>>
>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>> half of the clock.
>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>> /2 is only used for HS400, why should it be attempted in all other
>>> modes? Please limit the /2 just to HS400.
>> Hi Dmitry,
>>
>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>> to get 192MHz and we find this as host->clock wont be equal to 
>> msm_host->clk_rate.
> What are host->clock and msm_host->clk_rate at this point?
> What is the host->flags value? See sdhci_msm_hc_select_mode().

There are 2 paths which are traced to this function when card initializes
in HS400 mode, please consider below call stack in 2 paths

sdhci_msm_configure_dll
sdhci_msm_dll_config
sdhci_msm_execute_tuning
mmc_execute_tuning
mmc_init_card
_mmc_resume
mmc_runtime_resume

with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
and host->flags as 0x90c6.

and

sdhci_msm_configure_dll
sdhci_msm_dll_config
sdhci_msm_set_uhs_signaling
sdhci_set_ios
mmc_set_clock
mmc_set_bus_speed
mmc_select_hs400
mmc_init_card
_mmc_resume
mmc_runtime_resume

with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
and host->flags as 0x90c6 which are same as 1st.

Now if card is initialized in HS200 mode only below is the call stack

sdhci_msm_configure_dll
sdhci_msm_dll_config
sdhci_msm_execute_tuning
mmc_execute_tuning
mmc_init_card
_mmc_resume
mmc_runtime_resume

with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
and host->flags as 0x90c6.

now if you see the host->flags value, its same across the modes, and if
I am getting it right from the pointed out function
sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.

and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
before sdhci_msm_dll_config() call.

so this /2, is eventually called only for HS400 mode.

Thanks,
Ram

>
>> Now if we go for only HS200 mode supported card, there
>> the supported clock value would be 192Mhz itself and we need to pass
>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>> no other check is needed here.
> Please think about the cause, not about the symptom. Clocks being
> unequal is a result of some other checks being performed by the driver.
> Please use those checks too.
>
>> sorry for it took time to update as I was gathering all this data.
> 6 months? Well, that's a nice time to "gather all this data".

Took it up from sachin last month but still its a long gap.
Thanks for helping revive.

>
>> since Sachin have already pushed patchset #3, and if this explanation
>> helps, let me know if we can continue on patchset #3.
>>
>> Thanks,
>> Ram
>>

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

[Dmitry Baryshkov]

On 31/07/2025 14:46, Ram Prakash Gupta wrote:
> 
> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>>> +
>>>>>>>>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>>> +
>>>>>>>>>>> +	if (host->clock != msm_host->clk_rate)
>>>>>>>>>>> +		sup_clk = sup_clk / 2;
>>>>>>>>>>> +
>>>>>>>>>>> +	return sup_clk;
>>>>>>>>>> Why?
>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>>
>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>>> usable frequency.
>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>>
>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>>> patch.
>>>>>
>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>>
>>>>> Yes, It is 192 MHz.
>>>> Good, thanks for the confirmation.
>>>>
>>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>>> half of the clock.
>>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>>> /2 is only used for HS400, why should it be attempted in all other
>>>> modes? Please limit the /2 just to HS400.
>>> Hi Dmitry,
>>>
>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>>> to get 192MHz and we find this as host->clock wont be equal to
>>> msm_host->clk_rate.
>> What are host->clock and msm_host->clk_rate at this point?
>> What is the host->flags value? See sdhci_msm_hc_select_mode().
> 
> There are 2 paths which are traced to this function when card initializes
> in HS400 mode, please consider below call stack in 2 paths
> 
> sdhci_msm_configure_dll
> sdhci_msm_dll_config
> sdhci_msm_execute_tuning
> mmc_execute_tuning
> mmc_init_card
> _mmc_resume
> mmc_runtime_resume
> 
> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000

Please check the rates explicitly in the code rather than just checking 
that they are not equal.

> and host->flags as 0x90c6.
> 
> and
> 
> sdhci_msm_configure_dll
> sdhci_msm_dll_config
> sdhci_msm_set_uhs_signaling
> sdhci_set_ios
> mmc_set_clock
> mmc_set_bus_speed
> mmc_select_hs400
> mmc_init_card
> _mmc_resume
> mmc_runtime_resume
> 
> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
> and host->flags as 0x90c6 which are same as 1st.
> 
> Now if card is initialized in HS200 mode only below is the call stack
> 
> sdhci_msm_configure_dll
> sdhci_msm_dll_config
> sdhci_msm_execute_tuning
> mmc_execute_tuning
> mmc_init_card
> _mmc_resume
> mmc_runtime_resume
> 
> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
> and host->flags as 0x90c6.
> 
> now if you see the host->flags value, its same across the modes, and if
> I am getting it right from the pointed out function
> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
> 
> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
> before sdhci_msm_dll_config() call.
> 
> so this /2, is eventually called only for HS400 mode.
> 
> Thanks,
> Ram
> 
>>
>>> Now if we go for only HS200 mode supported card, there
>>> the supported clock value would be 192Mhz itself and we need to pass
>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>>> no other check is needed here.
>> Please think about the cause, not about the symptom. Clocks being
>> unequal is a result of some other checks being performed by the driver.
>> Please use those checks too.
>>
>>> sorry for it took time to update as I was gathering all this data.
>> 6 months? Well, that's a nice time to "gather all this data".
> 
> Took it up from sachin last month but still its a long gap.
> Thanks for helping revive.
> 
>>
>>> since Sachin have already pushed patchset #3, and if this explanation
>>> helps, let me know if we can continue on patchset #3.
>>>
>>> Thanks,
>>> Ram
>>>


-- 
With best wishes
Dmitry

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

[Ram Prakash Gupta]

On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
> On 31/07/2025 14:46, Ram Prakash Gupta wrote:
>>
>> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
>>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>>>> +
>>>>>>>>>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>>>> +
>>>>>>>>>>>> +    if (host->clock != msm_host->clk_rate)
>>>>>>>>>>>> +        sup_clk = sup_clk / 2;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    return sup_clk;
>>>>>>>>>>> Why?
>>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>>>
>>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>>>> usable frequency.
>>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>>>
>>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>>>> patch.
>>>>>>
>>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>>>
>>>>>> Yes, It is 192 MHz.
>>>>> Good, thanks for the confirmation.
>>>>>
>>>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>>>> half of the clock.
>>>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>>>> /2 is only used for HS400, why should it be attempted in all other
>>>>> modes? Please limit the /2 just to HS400.
>>>> Hi Dmitry,
>>>>
>>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>>>> to get 192MHz and we find this as host->clock wont be equal to
>>>> msm_host->clk_rate.
>>> What are host->clock and msm_host->clk_rate at this point?
>>> What is the host->flags value? See sdhci_msm_hc_select_mode().
>>
>> There are 2 paths which are traced to this function when card initializes
>> in HS400 mode, please consider below call stack in 2 paths
>>
>> sdhci_msm_configure_dll
>> sdhci_msm_dll_config
>> sdhci_msm_execute_tuning
>> mmc_execute_tuning
>> mmc_init_card
>> _mmc_resume
>> mmc_runtime_resume
>>
>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>
> Please check the rates explicitly in the code rather than just checking that they are not equal.

in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
immediately after return. And sdhci_msm_dll_config() is called after that.

Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
flag is set, and clock set is multiplying the clk rate by 2 in below call stack

msm_set_clock_rate_for_bus_mode
sdhci_msm_execute_tuning
mmc_execute_tuning
mmc_init_card

so this clk rate is doubling only with HS400 mode selection and while setting up
dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
is twice the host->clock as mentioned above.


>
>> and host->flags as 0x90c6.
>>
>> and
>>
>> sdhci_msm_configure_dll
>> sdhci_msm_dll_config
>> sdhci_msm_set_uhs_signaling
>> sdhci_set_ios
>> mmc_set_clock
>> mmc_set_bus_speed
>> mmc_select_hs400
>> mmc_init_card
>> _mmc_resume
>> mmc_runtime_resume
>>
>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>> and host->flags as 0x90c6 which are same as 1st.
>>
>> Now if card is initialized in HS200 mode only below is the call stack
>>
>> sdhci_msm_configure_dll
>> sdhci_msm_dll_config
>> sdhci_msm_execute_tuning
>> mmc_execute_tuning
>> mmc_init_card
>> _mmc_resume
>> mmc_runtime_resume
>>
>> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
>> and host->flags as 0x90c6.
>>
>> now if you see the host->flags value, its same across the modes, and if
>> I am getting it right from the pointed out function
>> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
>> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
>> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
>>
>> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
>> before sdhci_msm_dll_config() call.
>>
>> so this /2, is eventually called only for HS400 mode.
>>
>> Thanks,
>> Ram
>>
>>>
>>>> Now if we go for only HS200 mode supported card, there
>>>> the supported clock value would be 192Mhz itself and we need to pass
>>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>>>> no other check is needed here.
>>> Please think about the cause, not about the symptom. Clocks being
>>> unequal is a result of some other checks being performed by the driver.
>>> Please use those checks too.
>>>
>>>> sorry for it took time to update as I was gathering all this data.
>>> 6 months? Well, that's a nice time to "gather all this data".
>>
>> Took it up from sachin last month but still its a long gap.
>> Thanks for helping revive.
>>
>>>
>>>> since Sachin have already pushed patchset #3, and if this explanation
>>>> helps, let me know if we can continue on patchset #3.
>>>>
>>>> Thanks,
>>>> Ram
>>>>
>
>

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

[Dmitry Baryshkov]

On Thu, Aug 07, 2025 at 01:28:28AM +0530, Ram Prakash Gupta wrote:
> 
> On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
> > On 31/07/2025 14:46, Ram Prakash Gupta wrote:
> >>
> >> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
> >>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
> >>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
> >>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
> >>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
> >>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> >>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> >>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +    if (host->clock != msm_host->clk_rate)
> >>>>>>>>>>>> +        sup_clk = sup_clk / 2;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +    return sup_clk;
> >>>>>>>>>>> Why?
> >>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
> >>>>>>>>> Please explain the maths. You get the rate from the clock, then you
> >>>>>>>>> round it, but it is the rate that has just been returned, so there
> >>>>>>>>> should be no need to round it. And after that there a division by two
> >>>>>>>>> for some reason. So I've asked for an explanation for that code.
> >>>>>>>>>
> >>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
> >>>>>>>> usable frequency.
> >>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
> >>>>>>> round it? It sounds like that freq should be usable anyway.
> >>>>>>>
> >>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
> >>>>>> patch.
> >>>>>>
> >>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
> >>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
> >>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
> >>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
> >>>>>>>
> >>>>>> Yes, It is 192 MHz.
> >>>>> Good, thanks for the confirmation.
> >>>>>
> >>>>>> As part of eMMC Init, driver will try to init with the best mode supported
> >>>>>> by controller and device. In this case it is HS400 mode, But as part of
> >>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
> >>>>>> half of the clock.
> >>>>> This isn't an answer to the question. Let me rephrase it for you: if the
> >>>>> /2 is only used for HS400, why should it be attempted in all other
> >>>>> modes? Please limit the /2 just to HS400.
> >>>> Hi Dmitry,
> >>>>
> >>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
> >>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
> >>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
> >>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
> >>>> to get 192MHz and we find this as host->clock wont be equal to
> >>>> msm_host->clk_rate.
> >>> What are host->clock and msm_host->clk_rate at this point?
> >>> What is the host->flags value? See sdhci_msm_hc_select_mode().
> >>
> >> There are 2 paths which are traced to this function when card initializes
> >> in HS400 mode, please consider below call stack in 2 paths
> >>
> >> sdhci_msm_configure_dll
> >> sdhci_msm_dll_config
> >> sdhci_msm_execute_tuning
> >> mmc_execute_tuning
> >> mmc_init_card
> >> _mmc_resume
> >> mmc_runtime_resume
> >>
> >> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
> >
> > Please check the rates explicitly in the code rather than just checking that they are not equal.
> 
> in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
> DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
> sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
> sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
> immediately after return. And sdhci_msm_dll_config() is called after that.
> 
> Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
> sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
> flag is set, and clock set is multiplying the clk rate by 2 in below call stack
> 
> msm_set_clock_rate_for_bus_mode
> sdhci_msm_execute_tuning
> mmc_execute_tuning
> mmc_init_card
> 
> so this clk rate is doubling only with HS400 mode selection and while setting up
> dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
> is twice the host->clock as mentioned above.

I don't see how it's relevant. I'm asking you to check for the rate
values explicitly in the driver code rather than just checking that
rateA != rateB. You might error out if rateA != rateB and they are not
192 MHz / 384 MHz

> 
> 
> >
> >> and host->flags as 0x90c6.
> >>
> >> and
> >>
> >> sdhci_msm_configure_dll
> >> sdhci_msm_dll_config
> >> sdhci_msm_set_uhs_signaling
> >> sdhci_set_ios
> >> mmc_set_clock
> >> mmc_set_bus_speed
> >> mmc_select_hs400
> >> mmc_init_card
> >> _mmc_resume
> >> mmc_runtime_resume
> >>
> >> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
> >> and host->flags as 0x90c6 which are same as 1st.
> >>
> >> Now if card is initialized in HS200 mode only below is the call stack
> >>
> >> sdhci_msm_configure_dll
> >> sdhci_msm_dll_config
> >> sdhci_msm_execute_tuning
> >> mmc_execute_tuning
> >> mmc_init_card
> >> _mmc_resume
> >> mmc_runtime_resume
> >>
> >> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
> >> and host->flags as 0x90c6.
> >>
> >> now if you see the host->flags value, its same across the modes, and if
> >> I am getting it right from the pointed out function
> >> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
> >> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
> >> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
> >>
> >> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
> >> before sdhci_msm_dll_config() call.
> >>
> >> so this /2, is eventually called only for HS400 mode.
> >>
> >> Thanks,
> >> Ram
> >>
> >>>
> >>>> Now if we go for only HS200 mode supported card, there
> >>>> the supported clock value would be 192Mhz itself and we need to pass
> >>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
> >>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
> >>>> no other check is needed here.
> >>> Please think about the cause, not about the symptom. Clocks being
> >>> unequal is a result of some other checks being performed by the driver.
> >>> Please use those checks too.
> >>>
> >>>> sorry for it took time to update as I was gathering all this data.
> >>> 6 months? Well, that's a nice time to "gather all this data".
> >>
> >> Took it up from sachin last month but still its a long gap.
> >> Thanks for helping revive.
> >>
> >>>
> >>>> since Sachin have already pushed patchset #3, and if this explanation
> >>>> helps, let me know if we can continue on patchset #3.
> >>>>
> >>>> Thanks,
> >>>> Ram
> >>>>
> >
> >

-- 
With best wishes
Dmitry

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

[Ram Prakash Gupta]

On 8/9/2025 3:10 PM, Dmitry Baryshkov wrote:
> On Thu, Aug 07, 2025 at 01:28:28AM +0530, Ram Prakash Gupta wrote:
>> On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
>>> On 31/07/2025 14:46, Ram Prakash Gupta wrote:
>>>> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>>>>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +    if (host->clock != msm_host->clk_rate)
>>>>>>>>>>>>>> +        sup_clk = sup_clk / 2;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +    return sup_clk;
>>>>>>>>>>>>> Why?
>>>>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>>>>>
>>>>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>>>>>> usable frequency.
>>>>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>>>>>
>>>>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>>>>>> patch.
>>>>>>>>
>>>>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>>>>>
>>>>>>>> Yes, It is 192 MHz.
>>>>>>> Good, thanks for the confirmation.
>>>>>>>
>>>>>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>>>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>>>>>> half of the clock.
>>>>>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>>>>>> /2 is only used for HS400, why should it be attempted in all other
>>>>>>> modes? Please limit the /2 just to HS400.
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>>>>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>>>>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>>>>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>>>>>> to get 192MHz and we find this as host->clock wont be equal to
>>>>>> msm_host->clk_rate.
>>>>> What are host->clock and msm_host->clk_rate at this point?
>>>>> What is the host->flags value? See sdhci_msm_hc_select_mode().
>>>> There are 2 paths which are traced to this function when card initializes
>>>> in HS400 mode, please consider below call stack in 2 paths
>>>>
>>>> sdhci_msm_configure_dll
>>>> sdhci_msm_dll_config
>>>> sdhci_msm_execute_tuning
>>>> mmc_execute_tuning
>>>> mmc_init_card
>>>> _mmc_resume
>>>> mmc_runtime_resume
>>>>
>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>>> Please check the rates explicitly in the code rather than just checking that they are not equal.
>> in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
>> DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
>> sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
>> sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
>> immediately after return. And sdhci_msm_dll_config() is called after that.
>>
>> Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
>> sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
>> flag is set, and clock set is multiplying the clk rate by 2 in below call stack
>>
>> msm_set_clock_rate_for_bus_mode
>> sdhci_msm_execute_tuning
>> mmc_execute_tuning
>> mmc_init_card
>>
>> so this clk rate is doubling only with HS400 mode selection and while setting up
>> dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
>> is twice the host->clock as mentioned above.
> I don't see how it's relevant. I'm asking you to check for the rate
> values explicitly in the driver code rather than just checking that
> rateA != rateB. You might error out if rateA != rateB and they are not
> 192 MHz / 384 MHz

ok I see, but I got one another alternate to this, I can try use similar checks used in
function msm_get_clock_mult_for_bus_mode(), ios.timing == MMC_TIMING_MMC_HS400 
host->flags & SDHCI_HS400_TUNING, but for this I ll have to move check
host->flags &= ~SDHCI_HS400_TUNING in function sdhci_msm_execute_tuning () at the bottom
of this function, this will make code more readable and consistent to checks at other
places but I am testing it right now and will update.

If this doesn't work then I ll explicitly use the rate value in check.

Thanks,
Ram

>
>>
>>>> and host->flags as 0x90c6.
>>>>
>>>> and
>>>>
>>>> sdhci_msm_configure_dll
>>>> sdhci_msm_dll_config
>>>> sdhci_msm_set_uhs_signaling
>>>> sdhci_set_ios
>>>> mmc_set_clock
>>>> mmc_set_bus_speed
>>>> mmc_select_hs400
>>>> mmc_init_card
>>>> _mmc_resume
>>>> mmc_runtime_resume
>>>>
>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>>>> and host->flags as 0x90c6 which are same as 1st.
>>>>
>>>> Now if card is initialized in HS200 mode only below is the call stack
>>>>
>>>> sdhci_msm_configure_dll
>>>> sdhci_msm_dll_config
>>>> sdhci_msm_execute_tuning
>>>> mmc_execute_tuning
>>>> mmc_init_card
>>>> _mmc_resume
>>>> mmc_runtime_resume
>>>>
>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
>>>> and host->flags as 0x90c6.
>>>>
>>>> now if you see the host->flags value, its same across the modes, and if
>>>> I am getting it right from the pointed out function
>>>> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
>>>> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
>>>> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
>>>>
>>>> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
>>>> before sdhci_msm_dll_config() call.
>>>>
>>>> so this /2, is eventually called only for HS400 mode.
>>>>
>>>> Thanks,
>>>> Ram
>>>>
>>>>>> Now if we go for only HS200 mode supported card, there
>>>>>> the supported clock value would be 192Mhz itself and we need to pass
>>>>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>>>>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>>>>>> no other check is needed here.
>>>>> Please think about the cause, not about the symptom. Clocks being
>>>>> unequal is a result of some other checks being performed by the driver.
>>>>> Please use those checks too.
>>>>>
>>>>>> sorry for it took time to update as I was gathering all this data.
>>>>> 6 months? Well, that's a nice time to "gather all this data".
>>>> Took it up from sachin last month but still its a long gap.
>>>> Thanks for helping revive.
>>>>
>>>>>> since Sachin have already pushed patchset #3, and if this explanation
>>>>>> helps, let me know if we can continue on patchset #3.
>>>>>>
>>>>>> Thanks,
>>>>>> Ram
>>>>>>
>>>

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

[Ram Prakash Gupta]

On 8/21/2025 7:45 PM, Ram Prakash Gupta wrote:
> On 8/9/2025 3:10 PM, Dmitry Baryshkov wrote:
>> On Thu, Aug 07, 2025 at 01:28:28AM +0530, Ram Prakash Gupta wrote:
>>> On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
>>>> On 31/07/2025 14:46, Ram Prakash Gupta wrote:
>>>>> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
>>>>>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>>>>>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>>>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +    if (host->clock != msm_host->clk_rate)
>>>>>>>>>>>>>>> +        sup_clk = sup_clk / 2;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +    return sup_clk;
>>>>>>>>>>>>>> Why?
>>>>>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>>>>>>
>>>>>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>>>>>>> usable frequency.
>>>>>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>>>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>>>>>>
>>>>>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>>>>>>> patch.
>>>>>>>>>
>>>>>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>>>>>>
>>>>>>>>> Yes, It is 192 MHz.
>>>>>>>> Good, thanks for the confirmation.
>>>>>>>>
>>>>>>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>>>>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>>>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>>>>>>> half of the clock.
>>>>>>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>>>>>>> /2 is only used for HS400, why should it be attempted in all other
>>>>>>>> modes? Please limit the /2 just to HS400.
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>>>>>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>>>>>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>>>>>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>>>>>>> to get 192MHz and we find this as host->clock wont be equal to
>>>>>>> msm_host->clk_rate.
>>>>>> What are host->clock and msm_host->clk_rate at this point?
>>>>>> What is the host->flags value? See sdhci_msm_hc_select_mode().
>>>>> There are 2 paths which are traced to this function when card initializes
>>>>> in HS400 mode, please consider below call stack in 2 paths
>>>>>
>>>>> sdhci_msm_configure_dll
>>>>> sdhci_msm_dll_config
>>>>> sdhci_msm_execute_tuning
>>>>> mmc_execute_tuning
>>>>> mmc_init_card
>>>>> _mmc_resume
>>>>> mmc_runtime_resume
>>>>>
>>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>>>> Please check the rates explicitly in the code rather than just checking that they are not equal.
>>> in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
>>> DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
>>> sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
>>> sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
>>> immediately after return. And sdhci_msm_dll_config() is called after that.
>>>
>>> Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
>>> sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
>>> flag is set, and clock set is multiplying the clk rate by 2 in below call stack
>>>
>>> msm_set_clock_rate_for_bus_mode
>>> sdhci_msm_execute_tuning
>>> mmc_execute_tuning
>>> mmc_init_card
>>>
>>> so this clk rate is doubling only with HS400 mode selection and while setting up
>>> dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
>>> is twice the host->clock as mentioned above.
>> I don't see how it's relevant. I'm asking you to check for the rate
>> values explicitly in the driver code rather than just checking that
>> rateA != rateB. You might error out if rateA != rateB and they are not
>> 192 MHz / 384 MHz
> ok I see, but I got one another alternate to this, I can try use similar checks used in
> function msm_get_clock_mult_for_bus_mode(), ios.timing == MMC_TIMING_MMC_HS400 
> host->flags & SDHCI_HS400_TUNING, but for this I ll have to move check
> host->flags &= ~SDHCI_HS400_TUNING in function sdhci_msm_execute_tuning () at the bottom
> of this function, this will make code more readable and consistent to checks at other
> places but I am testing it right now and will update.
>
> If this doesn't work then I ll explicitly use the rate value in check.
>
> Thanks,
> Ram

Hi Dmitry,

adding checks for ios.timing == MMC_TIMING_MMC_HS400 && host->flags & SDHCI_HS400_TUNING
serves the same purpose for dividing the clk when mode is hs400 and hs400_tuning flag is
set, and clk value checks can be avoided now.

With this HS200, HS400, HS400ES modes of eMMC is tested.

so if this approach looks good to you, then I would like to proceed with next patchset.

Thanks,
Ram

>>>>> and host->flags as 0x90c6.
>>>>>
>>>>> and
>>>>>
>>>>> sdhci_msm_configure_dll
>>>>> sdhci_msm_dll_config
>>>>> sdhci_msm_set_uhs_signaling
>>>>> sdhci_set_ios
>>>>> mmc_set_clock
>>>>> mmc_set_bus_speed
>>>>> mmc_select_hs400
>>>>> mmc_init_card
>>>>> _mmc_resume
>>>>> mmc_runtime_resume
>>>>>
>>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>>>>> and host->flags as 0x90c6 which are same as 1st.
>>>>>
>>>>> Now if card is initialized in HS200 mode only below is the call stack
>>>>>
>>>>> sdhci_msm_configure_dll
>>>>> sdhci_msm_dll_config
>>>>> sdhci_msm_execute_tuning
>>>>> mmc_execute_tuning
>>>>> mmc_init_card
>>>>> _mmc_resume
>>>>> mmc_runtime_resume
>>>>>
>>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
>>>>> and host->flags as 0x90c6.
>>>>>
>>>>> now if you see the host->flags value, its same across the modes, and if
>>>>> I am getting it right from the pointed out function
>>>>> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
>>>>> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
>>>>> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
>>>>>
>>>>> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
>>>>> before sdhci_msm_dll_config() call.
>>>>>
>>>>> so this /2, is eventually called only for HS400 mode.
>>>>>
>>>>> Thanks,
>>>>> Ram
>>>>>
>>>>>>> Now if we go for only HS200 mode supported card, there
>>>>>>> the supported clock value would be 192Mhz itself and we need to pass
>>>>>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>>>>>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>>>>>>> no other check is needed here.
>>>>>> Please think about the cause, not about the symptom. Clocks being
>>>>>> unequal is a result of some other checks being performed by the driver.
>>>>>> Please use those checks too.
>>>>>>
>>>>>>> sorry for it took time to update as I was gathering all this data.
>>>>>> 6 months? Well, that's a nice time to "gather all this data".
>>>>> Took it up from sachin last month but still its a long gap.
>>>>> Thanks for helping revive.
>>>>>
>>>>>>> since Sachin have already pushed patchset #3, and if this explanation
>>>>>>> helps, let me know if we can continue on patchset #3.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ram
>>>>>>>

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

[Dmitry Baryshkov]

On Thu, Aug 28, 2025 at 01:30:21PM +0530, Ram Prakash Gupta wrote:
> 
> On 8/21/2025 7:45 PM, Ram Prakash Gupta wrote:
> > On 8/9/2025 3:10 PM, Dmitry Baryshkov wrote:
> >> On Thu, Aug 07, 2025 at 01:28:28AM +0530, Ram Prakash Gupta wrote:
> >>> On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
> >>>> On 31/07/2025 14:46, Ram Prakash Gupta wrote:
> >>>>> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
> >>>>>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
> >>>>>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
> >>>>>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
> >>>>>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
> >>>>>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
> >>>>>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> >>>>>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> +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_round_rate(core_clk, clk_get_rate(core_clk));
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> +    if (host->clock != msm_host->clk_rate)
> >>>>>>>>>>>>>>> +        sup_clk = sup_clk / 2;
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> +    return sup_clk;
> >>>>>>>>>>>>>> Why?
> >>>>>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
> >>>>>>>>>>>> Please explain the maths. You get the rate from the clock, then you
> >>>>>>>>>>>> round it, but it is the rate that has just been returned, so there
> >>>>>>>>>>>> should be no need to round it. And after that there a division by two
> >>>>>>>>>>>> for some reason. So I've asked for an explanation for that code.
> >>>>>>>>>>>>
> >>>>>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
> >>>>>>>>>>> usable frequency.
> >>>>>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
> >>>>>>>>>> round it? It sounds like that freq should be usable anyway.
> >>>>>>>>>>
> >>>>>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
> >>>>>>>>> patch.
> >>>>>>>>>
> >>>>>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
> >>>>>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
> >>>>>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
> >>>>>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
> >>>>>>>>>>
> >>>>>>>>> Yes, It is 192 MHz.
> >>>>>>>> Good, thanks for the confirmation.
> >>>>>>>>
> >>>>>>>>> As part of eMMC Init, driver will try to init with the best mode supported
> >>>>>>>>> by controller and device. In this case it is HS400 mode, But as part of
> >>>>>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
> >>>>>>>>> half of the clock.
> >>>>>>>> This isn't an answer to the question. Let me rephrase it for you: if the
> >>>>>>>> /2 is only used for HS400, why should it be attempted in all other
> >>>>>>>> modes? Please limit the /2 just to HS400.
> >>>>>>> Hi Dmitry,
> >>>>>>>
> >>>>>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
> >>>>>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
> >>>>>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
> >>>>>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
> >>>>>>> to get 192MHz and we find this as host->clock wont be equal to
> >>>>>>> msm_host->clk_rate.
> >>>>>> What are host->clock and msm_host->clk_rate at this point?
> >>>>>> What is the host->flags value? See sdhci_msm_hc_select_mode().
> >>>>> There are 2 paths which are traced to this function when card initializes
> >>>>> in HS400 mode, please consider below call stack in 2 paths
> >>>>>
> >>>>> sdhci_msm_configure_dll
> >>>>> sdhci_msm_dll_config
> >>>>> sdhci_msm_execute_tuning
> >>>>> mmc_execute_tuning
> >>>>> mmc_init_card
> >>>>> _mmc_resume
> >>>>> mmc_runtime_resume
> >>>>>
> >>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
> >>>> Please check the rates explicitly in the code rather than just checking that they are not equal.
> >>> in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
> >>> DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
> >>> sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
> >>> sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
> >>> immediately after return. And sdhci_msm_dll_config() is called after that.
> >>>
> >>> Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
> >>> sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
> >>> flag is set, and clock set is multiplying the clk rate by 2 in below call stack
> >>>
> >>> msm_set_clock_rate_for_bus_mode
> >>> sdhci_msm_execute_tuning
> >>> mmc_execute_tuning
> >>> mmc_init_card
> >>>
> >>> so this clk rate is doubling only with HS400 mode selection and while setting up
> >>> dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
> >>> is twice the host->clock as mentioned above.
> >> I don't see how it's relevant. I'm asking you to check for the rate
> >> values explicitly in the driver code rather than just checking that
> >> rateA != rateB. You might error out if rateA != rateB and they are not
> >> 192 MHz / 384 MHz
> > ok I see, but I got one another alternate to this, I can try use similar checks used in
> > function msm_get_clock_mult_for_bus_mode(), ios.timing == MMC_TIMING_MMC_HS400 
> > host->flags & SDHCI_HS400_TUNING, but for this I ll have to move check
> > host->flags &= ~SDHCI_HS400_TUNING in function sdhci_msm_execute_tuning () at the bottom
> > of this function, this will make code more readable and consistent to checks at other
> > places but I am testing it right now and will update.
> >
> > If this doesn't work then I ll explicitly use the rate value in check.
> >
> > Thanks,
> > Ram
> 
> Hi Dmitry,
> 
> adding checks for ios.timing == MMC_TIMING_MMC_HS400 && host->flags & SDHCI_HS400_TUNING
> serves the same purpose for dividing the clk when mode is hs400 and hs400_tuning flag is
> set, and clk value checks can be avoided now.
> 
> With this HS200, HS400, HS400ES modes of eMMC is tested.
> 
> so if this approach looks good to you, then I would like to proceed with next patchset.

LGTM.


-- 
With best wishes
Dmitry