Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Harshal Dev]

Hi Manivannan,

On 3/3/2026 10:38 PM, Manivannan Sadhasivam wrote:
> On Tue, Mar 03, 2026 at 02:11:06PM +0530, Harshal Dev wrote:
>> Hi Manivannan,
>>
>> On 2/20/2026 8:14 PM, Manivannan Sadhasivam wrote:
>>> On Fri, Jan 23, 2026 at 12:41:35PM +0530, Harshal Dev wrote:
>>>> Since Qualcomm inline-crypto engine (ICE) is now a dedicated driver
>>>> de-coupled from the QCOM UFS driver, it should explicitly vote for it's
>>>> needed resources during probe, specifically the UFS_PHY_GDSC power-domain
>>>> and the 'core' and 'iface' clocks.
>>>
>>> You don't need to vote for a single power domain since genpd will do that for
>>> you before the driver probes.
>>>
>>
>> Unfortunately, without enabling the power domain during probe, I am seeing occasional
>> clock stuck messages on LeMans RB8. Am I missing something? Could you point me to any
>> docs with more information on the the genpd framework?
>>
> 
> genpd_dev_pm_attach() called before a platform driver probe(), powers ON the
> domain.

You are correct. I just double confirmed this. I am going to remove all pm_runtime_* API
calls from this commit as they aren't needed. Ack.

> 
>> Logs for reference:
>>
>> [    6.195019] gcc_ufs_phy_ice_core_clk status stuck at 'off'
>> [    6.195031] WARNING: CPU: 5 PID: 208 at drivers/clk/qcom/clk-branch.c:87 clk_branch_toggle+0x174/0x18c
>>
>> [...]
>>
>> [    6.248412] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [    6.248415] pc : clk_branch_toggle+0x174/0x18c
>> [    6.248417] lr : clk_branch_toggle+0x174/0x18c
>> [    6.248418] sp : ffff80008217b770
>> [    6.248419] x29: ffff80008217b780 x28: ffff80008217bbb0 x27: ffffadf880a5f07c
>> [    6.248422] x26: ffffadf880a5c1d8 x25: 0000000000000001 x24: 0000000000000001
>> [    6.248424] x23: ffffadf8a0d1e740 x22: 0000000000000001 x21: ffffadf8a1d06160
>> [    6.248426] x20: ffffadf89f86e5a8 x19: 0000000000000000 x18: fffffffffffe9050
>> [    6.248429] x17: 000000000404006d x16: ffffadf89f8166c4 x15: ffffadf8a1ab6c70
>> [    6.347820] x14: 0000000000000000 x13: ffffadf8a1ab6cf8 x12: 000000000000060f
>> [    6.355145] x11: 0000000000000205 x10: ffffadf8a1b11d70 x9 : ffffadf8a1ab6cf8
>> [    6.362470] x8 : 00000000ffffefff x7 : ffffadf8a1b0ecf8 x6 : 0000000000000205
>> [    6.369795] x5 : ffff000ef1ceb408 x4 : 40000000fffff205 x3 : ffff521650ba3000
>> [    6.377120] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000928dd780
>> [    6.384444] Call trace:
>> [    6.386962]  clk_branch_toggle+0x174/0x18c (P)
>> [    6.391530]  clk_branch2_enable+0x1c/0x28
>> [    6.395644]  clk_core_enable+0x6c/0xac
>> [    6.399502]  clk_enable+0x2c/0x4c
>> [    6.402913]  devm_clk_get_optional_enabled+0xac/0x108
>> [    6.408096]  qcom_ice_create.part.0+0x50/0x2fc [qcom_ice]
>> [    6.413646]  qcom_ice_probe+0x58/0xa8 [qcom_ice]
>> [    6.418384]  platform_probe+0x5c/0x98
>> [    6.422153]  really_probe+0xbc/0x29c
>> [    6.425826]  __driver_probe_device+0x78/0x12c
>> [    6.430307]  driver_probe_device+0x3c/0x15c
>> [    6.434605]  __driver_attach+0x90/0x19c
>> [    6.438547]  bus_for_each_dev+0x7c/0xe0
>> [    6.442486]  driver_attach+0x24/0x30
>> [    6.446158]  bus_add_driver+0xe4/0x208
>> [    6.450013]  driver_register+0x5c/0x124
>> [    6.453954]  __platform_driver_register+0x24/0x30
>> [    6.458780]  qcom_ice_driver_init+0x24/0x1000 [qcom_ice]
>> [    6.464229]  do_one_initcall+0x80/0x1c8
>> [    6.468173]  do_init_module+0x58/0x234
>> [    6.472028]  load_module+0x1a84/0x1c84
>> [    6.475881]  init_module_from_file+0x88/0xcc
>> [    6.480262]  __arm64_sys_finit_module+0x144/0x330
>> [    6.485097]  invoke_syscall+0x48/0x10c
>> [    6.488954]  el0_svc_common.constprop.0+0xc0/0xe0
>> [    6.493790]  do_el0_svc+0x1c/0x28
>> [    6.497203]  el0_svc+0x34/0xec
>> [    6.500348]  el0t_64_sync_handler+0xa0/0xe4
>> [    6.504645]  el0t_64_sync+0x198/0x19c
>> [    6.508414] ---[ end trace 0000000000000000 ]---
>> [    6.514544] qcom-ice 1d88000.crypto: probe with driver qcom-ice failed
>>  
>>>> Also updated the suspend and resume callbacks to handle votes on these
>>>> resources.
>>>>
>>>> Signed-off-by: Harshal Dev <harshal.dev@xxxxxxxxxxxxxxxx>
>>>
>>> Where is the Fixes tag?
>>
>> Ack, I will add it in v2 of this patch.
>>
>>>
>>>> ---
>>>>  drivers/soc/qcom/ice.c | 20 ++++++++++++++++++++
>>>>  1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
>>>> index b203bc685cad..4b50d05ca02a 100644
>>>> --- a/drivers/soc/qcom/ice.c
>>>> +++ b/drivers/soc/qcom/ice.c
>>>> @@ -16,6 +16,8 @@
>>>>  #include <linux/of.h>
>>>>  #include <linux/of_platform.h>
>>>>  #include <linux/platform_device.h>
>>>> +#include <linux/pm.h>
>>>> +#include <linux/pm_runtime.h>
>>>>  
>>>>  #include <linux/firmware/qcom/qcom_scm.h>
>>>>  
>>>> @@ -108,6 +110,7 @@ struct qcom_ice {
>>>>  	void __iomem *base;
>>>>  
>>>>  	struct clk *core_clk;
>>>> +	struct clk *iface_clk;
>>>>  	bool use_hwkm;
>>>>  	bool hwkm_init_complete;
>>>>  	u8 hwkm_version;
>>>> @@ -310,12 +313,20 @@ int qcom_ice_resume(struct qcom_ice *ice)
>>>>  	struct device *dev = ice->dev;
>>>>  	int err;
>>>>  
>>>> +	pm_runtime_get_sync(dev);
>>>
>>> This is not needed as the power domain would be enabled at this point.
>>
>> Would this be enabled due to the genpd framework? I am not observing that
>> during probe. Because this call is made by the UFS/EMMC driver, perhaps you
>> mean the situation at this point is different?
>>
> 
> If you pass 'power-domains' property in DT, genpd will power it ON at this
> point.

Ack.

> 
>>>
>>>>  	err = clk_prepare_enable(ice->core_clk);
>>>>  	if (err) {
>>>>  		dev_err(dev, "failed to enable core clock (%d)\n",
>>>>  			err);
>>>>  		return err;
>>>>  	}
>>>> +
>>>> +	err = clk_prepare_enable(ice->iface_clk);
>>>> +	if (err) {
>>>> +		dev_err(dev, "failed to enable iface clock (%d)\n",
>>>> +			err);
>>>> +		return err;
>>>> +	}
>>>
>>> Use clk_bulk API to enable all clocks in one go.
>>
>> Ack, I'll use clk_bulk_prepare_enable().
>>
>>>
>>>>  	qcom_ice_hwkm_init(ice);
>>>>  	return qcom_ice_wait_bist_status(ice);
>>>>  }
>>>> @@ -323,7 +334,9 @@ EXPORT_SYMBOL_GPL(qcom_ice_resume);
>>>>  
>>>>  int qcom_ice_suspend(struct qcom_ice *ice)
>>>>  {
>>>> +	clk_disable_unprepare(ice->iface_clk);
>>>
>>> Same here.
>>
>> Ack, clk_bulk_disable_unprepare() would look good.
>> As Konrad pointed out, if iface clock is not present in DT, thse APIs are
>> fine with NULL pointers here.
>>
>>>
>>>>  	clk_disable_unprepare(ice->core_clk);
>>>> +	pm_runtime_put_sync(ice->dev);
>>>
>>> Not needed.
>>>
>>>>  	ice->hwkm_init_complete = false;
>>>>  
>>>>  	return 0;
>>>> @@ -584,6 +597,10 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>>>>  	if (IS_ERR(engine->core_clk))
>>>>  		return ERR_CAST(engine->core_clk);
>>>>  
>>>> +	engine->iface_clk = devm_clk_get_enabled(dev, "iface_clk");
>>>> +	if (IS_ERR(engine->iface_clk))
>>>> +		return ERR_CAST(engine->iface_clk);
>>>> +
>>>
>>> Same here. Use devm_clk_bulk_get_all_enabled().
>>
>> As per discussion on the DT binding patch, I can do this once we decide to break the
>> DT backward compatibility with a subsequent patch which makes both clocks mandatory.
>> For v2, I am planning to continue to treat the 'iface' clock as optional via
>> devm_clk_get_optional() API.
>>
> 
> Even if you do not mark 'iface' as 'required', this API will work just fine. It
> will get and enable whatever clocks defined in the DT node. It is upto the
> binding to define, what all should be present.

Agreed Manivannan, however, I realize that for legacy DT bindings, where ICE instance is
specified as part of the UFS/EMMC driver node, qcom_ice_create() receives the storage
device, if we call devm_clk_bulk_get_all_enabled() then all clocks specified in the
storage node would be returned and enabled. However, qcom_ice_create() should only enable
clocks relevant for ICE operation, i.e., core and iface clocks. iface being optional
for the time being as discussed.

And so, for suspend() and resume() as well, it seems I will have to continue with preparing
and enabling/disabling both the clocks individually.

> 
>>>
>>>>  	if (!qcom_ice_check_supported(engine))
>>>>  		return ERR_PTR(-EOPNOTSUPP);
>>>>  
>>>> @@ -725,6 +742,9 @@ static int qcom_ice_probe(struct platform_device *pdev)
>>>>  		return PTR_ERR(base);
>>>>  	}
>>>>  
>>>> +	devm_pm_runtime_enable(&pdev->dev);
>>>> +	pm_runtime_get_sync(&pdev->dev);
>>>
>>> If you want to mark & enable the runtime PM status, you should just do:
>>>
>>> 	devm_pm_runtime_set_active_enabled();	
>>>
>>> But this is not really needed in this patch. You can add it in a separate patch
>>> for the sake of correctness.
>>
>> If my understanding is correct, I need to call pm_runtime_get_sync() to enable
>> the power domain after enabling the PM runtime to ensure further calls to enable
>> the iface clock do not encounter failure. Just calling devm_pm_runtime_set_active_enabled()
>> will only enable the PM runtime and set it's status to 'active'. It will not enable
>> the power domain.
>>
> 
> Again, you DO NOT need to handle a single power domain in the driver, genpd will
> do it for you. If that is not helping, then something else is going wrong.
> 

Ack.

Regards,
Harshal

> - Mani
>