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

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

On 5/15/2026 10:48 PM, Krzysztof Kozlowski wrote:
> On 15/05/2026 16:22, Linlin Zhang wrote:
>>
>> 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.
>>
> 
> 
> Nothing here resolves the comments. Also, it's top posted. Honestly, I
> won't be talking through you with LLM, so consider patch NAKed.
> 
> Best regards,
> Krzysztof

Sorry for the late due to personal matters. 

At that time, I reviewed your comments one by one and provided answers, then
let AI polish the answer.

I'll add new replies after your comments point by point.

Thanks for the review.