Re: [PATCH v6] crypto: qce - Add runtime PM and interconnect bandwidth scaling support

[Udit Tiwari <quic_utiwari@quicinc.com>]

Hi Konrad,

Thanks for pointing this out.

I agree with your points regarding the usage of the ACQUIRE guard in 
probe to simplify the error paths, as well as the redundancy of 
icc_enable in the resume path. I will address both in the next version.

While preparing the fix, I performed a self-review and noticed a 
potential issue. Since I am providing my own custom functions for 
runtime suspend/resume (to handle the ICC path), the standard clock 
helpers are no longer called automatically by the PM framework.

I believe I need to manually call pm_clk_resume(dev) and 
pm_clk_suspend(dev) inside my custom functions to ensure the clocks are 
actually gated and ungated.

Does this look correct to you? If you agree, I will include this fix in v7.

Best Regards,
Udit Tiwari

On 2/17/2026 4:12 PM, Konrad Dybcio wrote:
> On 2/10/26 7:14 AM, quic_utiwari@quicinc.com wrote:
>> From: Udit Tiwari <quic_utiwari@quicinc.com>
>>
>> The Qualcomm Crypto Engine (QCE) driver currently lacks support for
>> runtime power management (PM) and interconnect bandwidth control.
>> As a result, the hardware remains fully powered and clocks stay
>> enabled even when the device is idle. Additionally, static
>> interconnect bandwidth votes are held indefinitely, preventing the
>> system from reclaiming unused bandwidth.
>>
>> Address this by enabling runtime PM and dynamic interconnect
>> bandwidth scaling to allow the system to suspend the device when idle
>> and scale interconnect usage based on actual demand. Improve overall
>> system efficiency by reducing power usage and optimizing interconnect
>> resource allocation.
>>
> 
> [...]
> 
>> +	ret = pm_runtime_resume_and_get(dev);
>>   	if (ret)
>>   		return ret;
> 
> I expected this to use the new helper too, removing the need for gotos
> altogether (unless this path needs some other handling which doesn't
> immediately jump out to me)
> 
> [...]
> 
>> +static int __maybe_unused qce_runtime_resume(struct device *dev)
>> +{
>> +	struct qce_device *qce = dev_get_drvdata(dev);
>> +	int ret = 0;
>> +
>> +	ret = icc_enable(qce->mem_path);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = icc_set_bw(qce->mem_path, QCE_DEFAULT_MEM_BANDWIDTH, QCE_DEFAULT_MEM_BANDWIDTH);
>> +	if (ret)
>> +		goto err_icc;
> 
> Just one of these is good - icc_enable() simply calls icc_set_bw() with
> the last known rate. Since we're not setting the rate any earlier,
> just keeping the set_bw() alone seems like the way to go
> 
> Konrad