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

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

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.

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