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

[Ram Prakash Gupta <quic_rampraka@quicinc.com>]

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

I will rename this to "dll_table".

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

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

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

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

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

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

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

Thanks,
Ram

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