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

[Krzysztof Kozlowski <krzk@kernel.org>]

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