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

[Ram Prakash Gupta]

On 6/3/2026 1:18 PM, Adrian Hunter wrote:
> 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.
> 

here -EPROBE_DEFER can be returned when clk driver is not ready, in that
case other clks, core & iface, would also fail to get enabled and then driver
would run into issues.

In case we get -EPROBE_DEFER for "ice" clk only then as you said rate
will not be set for ice clk to max but warning log regarding ice clk
rate not being set would be printed, and the ice clock would be set to
19.2MHz and in my opinion its not fatal.

so -EPROBE_DEFER handling seems overkill here for ice clk.

>>
>>>> +		/* 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)) {
>>>
>>
>