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

[Linlin Zhang]

On 8/18/2026 3:30 PM, Linlin Zhang wrote:
> 
> 
> 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.
> 

Alternatively, if you would prefer to review a revised patch series that
incorporates the changes we have already agreed upon for patches 0/3 and 1/3,
please let me know and I would be happy to prepare it.

Thanks