Re: [PATCH v1 1/1] mmc: sdhci-msm: Set ice clk rate

[Adrian Hunter]

On 03/06/2026 10:03, Ram Prakash Gupta wrote:
> 
> 
> On 6/1/2026 1:00 PM, Adrian Hunter wrote:
>> On 29/05/2026 11:10, Ram Prakash Gupta wrote:
>>> Set ice clk rate from sdhci msm platform driver, needed for
>>> target which are having legacy ice support, and need sdhci msm
>>> platform driver to set rate.
>>
>> Please expand upon what "legacy" means here?
>>
> 
> for devices where ice node is not created as separate device node those
> are referred here as legacy, separate device node for ice starts with
> below change: https://lore.kernel.org/all/20230407105029.2274111-1-abel.vesa@xxxxxxxxxx/
> 
> also I will update legacy that ice nodes which are created withing mmc dt
> node, so that ambiguity about legacy is clear.
> 
>> For CQ case, qcom_ice_create() prefers "ice_core_clk" before
>> "ice".  How does that relate to this?  Please clarify that in the
>> commit message also.
>>
> 
> "ice" is the naming convention used for emmc ice core clk in dt and
> "ice_core_clk" is the naming convention for ufs ice core clk. In the
> function you referred, since ice driver is common for both storage media,
> it tries to parse both the clock.
> 
> Here we are handling "ice" as this is needed for emmc only. I will add
> the same in commit message.
> 
>>>
>>> Signed-off-by: Ram Prakash Gupta <ram.gupta@xxxxxxxxxxxxxxxx>
>>> ---
>>>  drivers/mmc/host/sdhci-msm.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>> index b4131b12df56..c6a073718aa4 100644
>>> --- a/drivers/mmc/host/sdhci-msm.c
>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>> @@ -286,6 +286,7 @@ struct sdhci_msm_host {
>>>  	/* core, iface, cal and sleep clocks */
>>>  	struct clk_bulk_data bulk_clks[4];
>>>  #ifdef CONFIG_MMC_CRYPTO
>>> +	struct clk *ice_clk;	/* ICE clock */
>>
>> Why keep ice_clk?
>>
> 
> here we need this ice_clk because rate set is required only when ice clk
> is added with emmc node in dt, and in case we try to use the clk entry of
> qcom_ice structure it will set the rate for new ice node as well which is
> separate.
> 
> but also we can avoid this, since this one time operation, and we can reuse
> local variable clk inside sdhci_msm_probe, so it wont be needed. I will remove
> this in next patchset.
> 
>>>  	struct qcom_ice *ice;
>>>  #endif
>>>  	unsigned long clk_rate;
>>> @@ -2708,6 +2709,17 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>>  			return ret;
>>>  	}
>>>  
>>> +#ifdef CONFIG_MMC_CRYPTO
>>> +	/* Setup ICE clock */
>>> +	msm_host->ice_clk = devm_clk_get(&pdev->dev, "ice");
>>> +	if (!IS_ERR(msm_host->ice_clk)) {
>>
>> Does not attempt to deal with -EPROBE_DEFER, although bus_clk above
>> doesn't either.
>>
> 
> here need is just to set the rate, rest of the enablement part would be 
> taken care in ice driver, hence we can avoid this handling here.

If devm_clk_get() returns -EPROBE_DEFER, then the rate will not be set,
so you are relying on devm_clk_get() never to return -EPROBE_DEFER.

> 
>>> +		/* Vote for max. clk rate for max. performance */
>>> +		ret = clk_set_rate(msm_host->ice_clk, INT_MAX);
>>> +		if (ret)
>>> +			dev_err(&pdev->dev, "ice clk set rate failed (%d)\n", ret);
>>> +	}
>>> +#endif
>>
>> Could put this in a helper+stub function in the "Inline Crypto Engine
>> (ICE) support" section, to save having #ifdef CONFIG_MMC_CRYPTO here
>>
> 
> sure, will take care of this in next patchset.
> 
>>> +
>>>  	/* Setup main peripheral bus clock */
>>>  	clk = devm_clk_get(&pdev->dev, "iface");
>>>  	if (IS_ERR(clk)) {
>>
>