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

[Udit Tiwari]

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@xxxxxxxxxxx wrote:
> > From: Udit Tiwari <quic_utiwari@xxxxxxxxxxx>
> >
> > 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