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

[Ram Prakash Gupta <ram.gupta@oss.qualcomm.com>]

On 6/4/2026 5:45 PM, Ram Prakash Gupta wrote:
> 
> 
> 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@linaro.org/
>>>
>>> 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@oss.qualcomm.com>
>>>>> ---
>>>>>  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.
>

my bad here, we wont get the print, I will add else case for log.

> 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)) {
>>>>
>>>
>>
>