Re: [PATCH v2 2/3] soc: qcom: ice: Enable PM runtime for ICE driver

[Linlin Zhang <linlin.zhang@oss.qualcomm.com>]

Hi Krzysztof,

Thanks for the review.

For the SCMI-based platforms (e.g. sa8255p), the ICE resources such as
clocks are not controlled directly by the ICE driver. Instead, they are
managed by remote firmware and exposed to Linux via power domains. As a
result, the ICE driver cannot use clk_prepare_enable() directly to
control the hardware clock.

The intention of moving the clock handling into runtime PM callbacks is
to align the ICE driver with the power domain framework used on these
platforms. When the ICE device is attached to a power domain, invoking
pm_runtime_resume_and_get() will trigger the provider (remote firmware
via SCMI) to power up the device, which in turn enables the underlying
clock and other resources.

This design follows the guidance where the runtime PM framework is
used as the common mechanism to abstract both:
  - direct clock control on non-SCMI platforms, and
  - firmware-controlled resources via power domains on SCMI platforms.

In both cases, the runtime PM callbacks are responsible for performing
the actual resource enable/disable:
  - for legacy platforms: clk_prepare_enable()/disable_unprepare()
  - for SCMI platforms: power domain on/off handled by firmware

So while it may look like an additional layer on legacy platforms, this
approach provides a unified mechanism without requiring separate driver
entry points or special handling in the upper layers (e.g. UFS driver).

That said, I understand your concern that introducing runtime PM solely
for clock gating can be seen as unnecessary overhead on existing
platforms. I will revisit the implementation to ensure that:
  - the runtime PM integration does not introduce regressions for legacy
    platforms, and
  - the design clearly justifies the common abstraction for both SCMI
    and non-SCMI cases.

In addition, I rewrite the commit message as the following to make the
intention more clear.

  On some platforms the ICE device is placed in a firmware-managed power
  domain. In those cases the ICE core resources (including the clock) are
  not directly controllable by Linux and are instead toggled by the power
  domain provider (e.g. remote firmware via SCMI).

  Wire the ICE device into runtime PM so that a single pm_runtime
  transition is used to bring the ICE device up/down. When the device is
  attached to a PM domain, pm_runtime_resume_and_get()/pm_runtime_put_sync()
  will invoke the PM domain callbacks and let the provider manage the
  resources. On platforms without a PM domain the runtime PM callbacks
  continue to perform the existing clock enable/disable locally.

  No functional change is intended for non-firmware-managed platforms; the
  change provides a common control point that allows ICE to operate when
  resources are owned by a PM domain provider.

Thanks again for the feedback. I would appreciate your further review
and comments.

Best regards,
Linlin

On 5/14/2026 10:06 PM, Krzysztof Kozlowski wrote:
> On 12/05/2026 05:37, Linlin Zhang wrote:
>> The QCOM ICE driver manages the ICE core clock through direct calls to
>> clk_prepare_enable() and clk_disable_unprepare(), which limits integration
> 
> No, it does not limit any integration.
> 
>> with platforms that rely on firmware-managed resources or platform-specific
>> power management mechanisms.
> 
> Nope. It's perfectly correct way of managing clocks. Adding runtime PM
> ONLY to toggle clocks is absolute killer, pointless overhead without
> benefits.
> 
>>
>> Replace direct clock management with runtime PM support by moving clock
>> enable and disable into runtime PM callbacks. Use
>> pm_runtime_resume_and_get() and pm_runtime_put_sync() in qcom_ice_resume()
>> and qcom_ice_suspend() to drive power state transitions, and enable runtime
>> PM in qcom_ice_probe().
>>
>> Reviewed-by: Neeraj Soni <neeraj.soni@oss.qualcomm.com>
>> Reviewed-by: Deepti Jaggi <deepti.jaggi@oss.qualcomm.com>
>> Signed-off-by: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
>> ---
>>  drivers/soc/qcom/ice.c | 58 ++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 53 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
>> index b203bc685cad..6f9d679b530c 100644
>> --- a/drivers/soc/qcom/ice.c
>> +++ b/drivers/soc/qcom/ice.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/of.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>  
>>  #include <linux/firmware/qcom/qcom_scm.h>
>>  
>> @@ -310,8 +311,8 @@ int qcom_ice_resume(struct qcom_ice *ice)
>>  	struct device *dev = ice->dev;
>>  	int err;
>>  
>> -	err = clk_prepare_enable(ice->core_clk);
>> -	if (err) {
>> +	err = pm_runtime_resume_and_get(dev);
>> +	if (err < 0) {
>>  		dev_err(dev, "failed to enable core clock (%d)\n",
>>  			err);
>>  		return err;
>> @@ -323,7 +324,7 @@ EXPORT_SYMBOL_GPL(qcom_ice_resume);
>>  
>>  int qcom_ice_suspend(struct qcom_ice *ice)
>>  {
>> -	clk_disable_unprepare(ice->core_clk);
>> +	pm_runtime_put_sync(ice->dev);
>>  	ice->hwkm_init_complete = false;
>>  
>>  	return 0;
> 
> 
> This is pretty pointless change. At least by quick glance. You changed
> nothing here for PM, except adding indirection layer and more locks.
> Clocks will be gated the same way, no energy savings. But on the other
> hand introducing runtime PM subsystem is huge bunch of code with its own
> locks, completely unnecessary here.
> 
> This itself is poor choice and has NEGATIVE impact on all existing
> platforms without any benefit.
> 
> I am surprised you went through SIX internal reviews, collected two
> internal review tags and no one suggested that using runtime PM ONLY to
> toggle clocks is pretty pointless and undesired.> 
> Best regards,
> Krzysztof