Message-ID: <bb60a145-1e8f-4004-b266-9f26a11440b9@quicinc.com>
Date: Thu, 26 Dec 2024 11:22:40 +0530
From: Sachin Gupta <quic_sachgupt@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Adrian Hunter <adrian.hunter@...el.com>,
        Ulf Hansson
	<ulf.hansson@...aro.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-mmc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <quic_cang@...cinc.com>, <quic_nguyenb@...cinc.com>,
        <quic_bhaskarv@...cinc.com>, <quic_mapa@...cinc.com>,
        <quic_narepall@...cinc.com>, <quic_nitirawa@...cinc.com>,
        <quic_rampraka@...cinc.com>, <quic_sartgarg@...cinc.com>
Subject: Re: [PATCH V2 2/2] mmc: sdhci-msm: Rectify DLL programming sequence
 for SDCC



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@...cinc.com>
>> Signed-off-by: Bao D. Nguyen <nguyenb@...eaurora.org>
>> Signed-off-by: Sarthak Garg <sartgarg@...eaurora.org>
>> Signed-off-by: Jun Li <liju@...eaurora.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
>>
> 

Powered by blists - more mailing lists