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

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

[Ram Prakash Gupta]

Add support to configure the ICE clock to the maximum supported frequency
to address the review comments from the below ICE upsteaming discussion:
https://lore.kernel.org/all/7a0db353-02f7-4188-b2d7-9098548f1920@oss.qualcomm.com/

And also to address performance degradation of ~50%-70%.
https://lore.kernel.org/all/aTpbZHRUj6QF3KSE@hu-arakshit-hyd.qualcomm.com/

Below is the link for the ICE upstreaming where ice clock is being
configured to max frequency:
https://lore.kernel.org/all/20251121-enable-ufs-ice-clock-scaling-v2-3-66cb72998041@oss.qualcomm.com/


Ram Prakash Gupta (1):
  mmc: sdhci-msm: Set ice clk rate

 drivers/mmc/host/sdhci-msm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.34.1

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

[Ram Prakash Gupta]

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.

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 */
 	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)) {
+		/* 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
+
 	/* Setup main peripheral bus clock */
 	clk = devm_clk_get(&pdev->dev, "iface");
 	if (IS_ERR(clk)) {
-- 
2.34.1

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

[Abhinaba Rakshit]

On Fri, May 29, 2026 at 01:40:45PM +0530, 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.
> 
> 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 */
>  	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)) {
> +		/* 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);

If we keep dev_err(), the failure path should probably go to bus_clk_disable.
However, since boosting the core clock to max is kept optional,
I think dev_warn() is more appropriate here and avoids making this a fatal error.

Abhinaba Rakshit 

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

[Ram Prakash Gupta]

On 5/31/2026 10:54 PM, Abhinaba Rakshit wrote:
> On Fri, May 29, 2026 at 01:40:45PM +0530, 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.
>>
>> 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 */
>>  	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)) {
>> +		/* 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);
> 
> If we keep dev_err(), the failure path should probably go to bus_clk_disable.
> However, since boosting the core clock to max is kept optional,
> I think dev_warn() is more appropriate here and avoids making this a fatal error.
> 
> Abhinaba Rakshit 

Here its only setting rate of clk, so no clean up is needed, hence logging is 
sufficient here, but I agree dev_warn should suffice instead of dev_err. I will
update this to dev_warn.

Thanks,
Ram

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

[Adrian Hunter]

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 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.

> 
> 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?

>  	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.

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

> +
>  	/* Setup main peripheral bus clock */
>  	clk = devm_clk_get(&pdev->dev, "iface");
>  	if (IS_ERR(clk)) {

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

[Ram Prakash Gupta]

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

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@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.

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

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@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.

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

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

[Ram Prakash Gupta]

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

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

[Konrad Dybcio]

On 6/3/26 9:03 AM, 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.
> 
>>> +		/* Vote for max. clk rate for max. performance */
>>> +		ret = clk_set_rate(msm_host->ice_clk, INT_MAX);

This will cause crashes or at least brownouts because it carries no
votes for the parent power domains (CX etc.). DT should be updated
instead, this is a dead path

Konrad