Re: [PATCH v3 17/47] clk: qcom: gcc-sc7180: Tie the CX power domain to controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Enabling runtime PM on the gcc-sc7180 controller fails to allow the CX power domain to power down because CLK_IS_CRITICAL clocks hold permanent PM runtime references.
- [Medium] Manual register configurations performed in probe() will be permanently lost if the CX power domain successfully powers down.
--

--- Patch [17]: [PATCH v3 17/47] clk: qcom: gcc-sc7180: Tie the CX power domain to controller ---
commit bda4bbdc6882e3eeb0fc4bbaf2482c52d18aab7d
Author: Abel Vesa <abel.vesa@oss.qualcomm.com>

clk: qcom: gcc-sc7180: Tie the CX power domain to controller
    
Enable runtime PM for the controller, allowing CX to be powered down
once all GCC consumers have released their clocks and GDSCs.

> diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
> index 4a49ad7a9e5be..f3a245470a4de 100644
> --- a/drivers/clk/qcom/gcc-sc7180.c
> +++ b/drivers/clk/qcom/gcc-sc7180.c
> @@ -2418,6 +2418,7 @@ static const struct qcom_cc_desc gcc_sc7180_desc = {
>  	.num_resets = ARRAY_SIZE(gcc_sc7180_resets),
>  	.gdscs = gcc_sc7180_gdscs,
>  	.num_gdscs = ARRAY_SIZE(gcc_sc7180_gdscs),
> +	.use_rpm = true,

[Severity: Medium]
Will the CX power domain actually be able to power down?

The Common Clock Framework automatically prepares clocks marked with
CLK_IS_CRITICAL during initialization, such as gcc_cpuss_ahb_clk:

drivers/clk/qcom/gcc-sc7180.c:gcc_cpuss_ahb_clk {
    ...
			.flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,
    ...
}

Preparing these clocks calls clk_pm_runtime_get(), which increments the
runtime PM usage counter of the GCC provider device. Since critical clocks
are never unprepared, this reference is never dropped.

Does this prevent the controller's usage count from ever reaching zero?

[Severity: Medium]
If the controller does manage to runtime suspend and the CX power domain
powers off, will the manual register configurations performed in
gcc_sc7180_probe() be lost?

For example:

drivers/clk/qcom/gcc-sc7180.c:gcc_sc7180_probe() {
    ...
	regmap_update_bits(regmap, 0x09ffc, 0x3, 0x3);
	regmap_update_bits(regmap, 0x4d110, 0x3, 0x3);
	regmap_update_bits(regmap, 0x71028, 0x3, 0x3);
    ...
}

Because gcc_sc7180_driver lacks dev_pm_ops with a runtime_resume callback
to restore these settings, and does not use a regcache, will the controller
resume in an uninitialized default state?

>  };
>  
>  static const struct of_device_id gcc_sc7180_match_table[] = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-qcom-tie-cx-to-gcc-v3-0-f1205a92682b@oss.qualcomm.com?part=17