Re: [PATCH v2 01/16] clk: qcom: gcc-sm8250: Tie the CX power domain to controller

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch's intended power savings are defeated because `gcc_cpuss_ahb_clk` is flagged `CLK_IS_CRITICAL`, permanently holding a runtime PM reference and preventing the CX domain from powering down.
--

--- Patch [1]: [PATCH v2 01/16] clk: qcom: gcc-sm8250: Tie the CX power domain to controller ---
commit e29c0e17d3927082fc53c778fba9e97b8d02b015
Author: Abel Vesa <[email protected]>

clk: qcom: gcc-sm8250: Tie the CX power domain to controller

This commit enables runtime PM for the GCC controller. The intention is to
allow the CX power domain to be powered down once all GCC consumers release
their clocks and GDSCs.

> diff --git a/drivers/clk/qcom/gcc-sm8250.c b/drivers/clk/qcom/gcc-sm8250.c
> --- a/drivers/clk/qcom/gcc-sm8250.c
> +++ b/drivers/clk/qcom/gcc-sm8250.c
> @@ -3618,6 +3618,7 @@ static const struct qcom_cc_desc gcc_sm8250_desc = {
>  	.resets = gcc_sm8250_resets,
>  	.num_resets = ARRAY_SIZE(gcc_sm8250_resets),
>  	.gdscs = gcc_sm8250_gdscs,
> +	.use_rpm = true,

[Severity: Medium]
Does this actually allow the controller to power down in practice?

Looking at gcc_cpuss_ahb_clk in this driver, it is registered with the
CLK_IS_CRITICAL flag:

static struct clk_branch gcc_cpuss_ahb_clk = {
        ...
        .clkr = {
                .hw.init = &(struct clk_init_data){
                        .name = "gcc_cpuss_ahb_clk",
                        .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,
                        ...
                },
        },
};

When the clock core prepares a critical clock during registration, it calls
clk_pm_runtime_get(), which permanently increments the runtime PM usage
counter for the GCC controller device. Because a critical clock is never
unprepared, this runtime PM reference is held forever.

Will the device's PM usage counter ever be able to reach 0 to allow the
CX domain to power down, or does the critical clock structurally block the
intended power savings?

>  	.num_gdscs = ARRAY_SIZE(gcc_sm8250_gdscs),
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1