Re: [RFC PATCH 00/16] Centralize the ownership of power rail votes for Qualcomm clocks

[Dmitry Baryshkov]

On Wed, Oct 14, 2026 at 02:19:00PM +0200, Konrad Dybcio wrote:
> On Qualcomm SoCs, both the PLLs and other types of clocks alike require
> specific voltage rail levels to operate in-spec, but through the
> separation and decentralization of clock and voltage votes, these
> requirements are often not met and are never explicitly stated.
> This results in clocks being undervolted, or at the very least a state
> we're unable to reason about.
> 
> The devices consuming these clocks very rarely have their own
> requirements in that regard. (1)
> 
> This series moves the frequency-power_domain-perf_state mappings inside
> the clock drivers, ensuring a single, trusted source of truth is
> established and that the silicon is no longer driven at out-of-spec
> conditions.
> 
> To illustrate the nuances involved, here's a couple examples of hidden
> dependencies, as found on SM8650:
> * video_cc_mvs0_clk requires LOW_SVS to run at 900 MHz, but the
>   PLL it sources the signal from requires SVS (higher corner) to
>   produce that frequency. Similarly, to produce 1140 MHz, the clock
>   requires SVS, but the PLL then requires SVS_L1.
>   Analogous story for mvs1_clk.

Is PLL a part of the the same domain or a different domain?

> * gpu_cc_gmu_clk requires MX to be at SVS to output 625 MHz, the PLL
>   requires SVS_L1 to produce that frequency.
> * A number of camera clocks follow the same pattern.
> * Additionally, some clocks like cam_cc_cci_{0,1,2}_clk require
>   MMCX to run at LOW_SVS, but the PLL has the additional requirement
>   of keeping MXC (a wholly different rail) at SVS_L1.
> 
> There's a broad agreement that a consumer should only have to care
> about the clock it consumes, and that configuring its parents is the
> responsibility of the CCF. Requiring the consumers to care about the
> above cases (as is the expectation today) is a clear violation of
> that contract.

I think that the conclusion was that the consumers should care about the
full set of supplies they need, which includes all transient and leaf
supplies.

> A clock controller may have on the order of hundreds of clocks, each
> one with its own frequency-power map, and potentially all of them
> could be similarly affected.

You have described several cases, where PLLs stand out, while the rest
of the clocks are typically more or less in line. Would it make sense
instead to solve the same issue in a slightly different way:

clock-controller@abcdef {
    compatible = "qcom,sm8888-clockcc";
    // required to access registers of the controller
    power-domains = <&rpmhpd RPMHPD_MMCCX>;
    required-opps = <&rpmhpd_opp_svs>;

    pll0 {
        power-domains = <&rpmhpd RPMHPD_MMMX>;
        operating-points = <&trivian_opp_table>;

        trivian_opp_table: opp-table {
	    compatible = "operating-points-v2";

	    opp-100000 {
	        opp-hz = /bits/ 64 <100000>;
		required-opps = <&rpmhpd_opp_super_low_svs>;
	    };

	    opp-500000 {
	        opp-hz = /bits/ 64 <500000>;
		required-opps = <&rpmhpd_opp_turbo>;
	    };
        };
    };

    pll1 {
        /* same PLL type, same table */
        power-domains = <&rpmhpd RPMHPD_MMMX>;
        operating-points = <&trivian_opp_table>;
    };

    pll2 {
        power-domains = <&rpmhpd RPMHPD_MMMX>;
        operating-points = <&completian_opp_table>;

	/* other PLL type */
        completian_opp_table: opp-table {
	    compatible = "operating-points-v2";

	    opp-123456 {
	        opp-hz = /bits/ 64 <123456>;
		required-opps = <&rpmhpd_opp_super_low_svs>;
	    };

	    opp-333333 {
	        opp-hz = /bits/ 64 <333333>;
		required-opps = <&rpmhpd_opp_nominal>;
	    };

	    opp-888888 {
	        opp-hz = /bits/ 64 <888888>;
		required-opps = <&rpmhpd_opp_lower_turbo>;
	    };
        };
    };
};

The clock controller driver would then create a per-PLL device, bind it
with the OPP table, etc. The consumers would still vote on the resources
necessary to run the leaf clock, while the clock controller would care
about the PLLs.

You can guess, I don't like the idea of having voting tables in the
clock driver...

> Recently, some drivers (Iris, CAMSS) have attempted to circumvent these
> problems by holding a direct reference to the power rails and managing
> the votes manually, through placing direct votes on each rail via an
> OPP table. That however has a number of drawbacks:
> * Because more than just one clock needs to be scaled at runtime, the
>   OPP table either has to contain NUM_CLOCKS * NUM_RATES to handle
>   that (which blows up pretty quickly), assume the worst case (and
>   waste power), or ignore that problem (and undervolt the clock
>   controller), instead of just being able to call set_rate() on each
>   clock as needed

If the clocks are independent, it's a different question. I don't think
we have a way to describe that we need OPP table a for clock CLK_A and
table b for CLK_B.

If we need to scale several clocks in a lock, the problem is manageable.
We need 2x entries (at least for the typical cases), because we need to
have one entry for each MMCX level and for each MX level.

> * The data ends up being dispersed across various consumers, even
>   though it logically belongs to the clock controller, making it
>   absolutely unauditable

Yes... See the proposal above.

> * It's horrendously error-prone because of the number of clocks
>   involved, the depth of the clock tree and the fact that at the end
>   of the day, one still has to consult the clock controller
>   documentation

One needs to consult it anyway, more or less.

> 
> The approach proposed here also happens to more naturally fit into the
> process of how an SoC is brought up - clock controllers are always
> the very first pieces that need to be complete before other IP blocks
> can be brought up. And because of (1), enclosing the frequency-corner
> data within the clock drivers takes a weight off of everyone else's
> shoulders.
> 
> On a related topic, the GDSCs are modeled as children of *all* power
> domains consumed by a clock controller at once. GDSCs are on/off switches
> with some fancy logic attached to them and may need some (or none) rails
> enabled to power on or off while meeting the hardware timing
> requirements. Voting on them was never quite right in the first place
> and is once again only a side-effect of the clocks' power requirements
> not being handled centrally.
> 
> This series removes the vote propagation and the overly broad parent-
> child links, resulting in dropping unnecessary/incorrect votes that are
> now encapsulated by the clk-dvfs infrastructure. Big emphasis on (1)
> once again.
> 
> The implementation is as follows:
> 
> Each clock describes the rail levels it needs at each frequency. The
> new clk-dvfs helper combines these requirements and submits the
> resulting votes through GenPD and RPMHPD. Votes remain held while a
> clock is prepared, and rate or parent changes reserve the new
> requirements before any clocks are reprogrammed. This also requires CCF
> to report programming failures and OPP to retry failed transitions
> instead of relying on stale cached state.
> 
>                 Consumer: select rate, prepare clock
>                                    |
>                                    v
>         +------------------------ CCF ----------------------+
>         | Walk the clock tree and reserve resources          |
>         | before programming rates or parents                |
>         +--------------------------------------------------+
>                                    |
>                                    v
>  +------------------------------------------------------------------+
>  |                                                                    |
>  |    GPLL9 ------> SDCC2 RCG -------> SDCC2 apps clock               |
>  |      |               |                                             |
>  |      |               |                         Other GCC clocks    |
>  |      v               v                              |              |
>  |  +------------+  +------------+                     v              |
>  |  | PLL's own  |  | RCG's own  |          Per-clock requirements    |
>  |  | rate table |  | rate table |                         |          |
>  |  +-----+------+  +-----+------+                         |          |
>  |        |               |                                |          |
>  |        | CX            | CX + MX                        |          |

RCG doesn't (typically) care about MX. It is not split between multiple
domains, is it?

>  |        v               v                                v          |
>  |    +--------------------------------------------------------+      |
>  |    |                  Clock-owned DVFS                        |    |
>  |    |                                                          |    |
>  |    | Prepared votes + temporary transition votes              |    |

This can quickly become a lock dependency (and lockdep) hell. Consider
two consumers setting the same clock concurrently (heh, nice design, I
know). For each of the consumers we must make sure that there is no
AB-BA voting update, which means that the whole sequence of
pre-set-rate vote, clk_set_rate() and post-set-rate vote should happen
under a single lock. However under the lock we vote for genpds, which
might trigger other transitions or, in the worst case, wake up the
domain.

>  |    |                          |                               |    |
>  |    |               Maximum requirement per rail               |    |
>  |    +-----------+--------------+--------------+--------------+      |
>  |                |              |              |                     |
>  |            CX voter      CX_AO voter     MX voter                  |
>  +----------------+-------------+-------------+---------------------+
>                    |              |              |
>                    v              v              v
>        +----------------------- GenPD ----------------------+
>        |      Aggregate GCC votes with other consumers       |
>        +---------------------------------------------------+
>                                   |
>                                   v
>                             +-----------+
>                             |   RPMHPD  |
>                             +-----------+
> 
> Straightening out all of the above lets us tackle another problem
> with the upstream description of the power grid - the dependency
> between various rails. Most notably:
> * CX does NOT need to follow the performance state of MMCX

I think this depends on the RPMh translating votes or not. Is it really
fine to run MMCX at turbo if CX is at low_svs?

> * MX/MXA/MXC and CX are co-dependent only on some platforms (in coarse
>   terms, SM8350 and before)
> 
> However, the main part of series needs to land first to ensure that
> dropping those relationships doesn't remove some wrongful votes the
> system relied on to stay functional. Resolving that will save power and
> finally ensure total stability as far as the providers are concerned.
> 
> SM8650 is used as an example in this series, but the conversion will
> have to be done for all Qualcomm platforms. That will follow soon
> after the patches are merged. DFS clocks are left out of this RFC to
> reduce noise (the remaining diff is not particularly large but also
> requires boilerplate changes to bus drivers).
> 
> dt-bindings changes are not included since it's an RFC.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> ---
> Konrad Dybcio (16):
>       clk: Propagate rate programming failures
>       clk: Acquire resources for clock transitions
>       OPP: Retry failed default single-clock transitions
>       pmdomain: core: Support named indexed virtual attachments
>       pmdomain: core: Support subdomains without pstate propagation
>       pmdomain: core: Support indexed power-domain attachment lists
>       clk: qcom: Add clock-owned GenPD DVFS aggregation
>       clk: qcom: dvfs: Add debugfs
>       clk: qcom: clk-alpha-pll: Add DVFS (un)prepare
>       clk: qcom: clk-alpha-pll: Vote for the PLL calibration frequency
>       clk: qcom: gdsc: Make GDSC parenting optional
>       clk: qcom: gdsc: Link required power domains
>       [DNM] arm64: dts: qcom: sm8650: Describe clock controller power rails
>       [DNM] clk: qcom: Enable clk-dvfs on SM8650
>       [DNM] arm64: dts: qcom: sm8650: Drop required-opps
>       [DNM] pmdomain: qcom: rpmhpd: Stop forwarding MMCX pstate to CX
> 
>  arch/arm64/boot/dts/qcom/sm8650.dtsi        |  62 +--
>  drivers/base/power/common.c                 |  24 +-
>  drivers/clk/Makefile                        |   1 +
>  drivers/clk/clk.c                           | 793 +++++++++++++++++++++++-----
>  drivers/clk/clk_resource_test.c             | 376 +++++++++++++
>  drivers/clk/clk_test.c                      | 194 ++++++-
>  drivers/clk/qcom/Makefile                   |   1 +
>  drivers/clk/qcom/camcc-sm8650.c             | 282 ++++++++++
>  drivers/clk/qcom/clk-alpha-pll.c            | 317 ++++++++++-
>  drivers/clk/qcom/clk-dvfs.c                 | 635 ++++++++++++++++++++++
>  drivers/clk/qcom/clk-dvfs.h                 |  41 ++
>  drivers/clk/qcom/clk-rcg2.c                 |  93 +++-
>  drivers/clk/qcom/clk-regmap.h               |   6 +
>  drivers/clk/qcom/common.c                   |  28 +-
>  drivers/clk/qcom/common.h                   |   3 +
>  drivers/clk/qcom/dispcc-sm8550.c            | 328 ++++++++++--
>  drivers/clk/qcom/gcc-sm8650.c               | 261 +++++++++
>  drivers/clk/qcom/gdsc.c                     | 154 +++++-
>  drivers/clk/qcom/gdsc.h                     |   7 +
>  drivers/clk/qcom/gpucc-sm8650.c             | 101 +++-
>  drivers/clk/qcom/videocc-sm8550.c           | 135 ++++-
>  drivers/opp/core.c                          |  72 ++-
>  drivers/opp/opp.h                           |   2 +
>  drivers/pmdomain/core.c                     |  60 ++-
>  drivers/pmdomain/qcom/rpmhpd.c              |  14 +-
>  include/dt-bindings/clock/qcom,sm8650-gcc.h |   2 +
>  include/linux/clk-provider.h                |  39 ++
>  include/linux/pm_domain.h                   |  33 +-
>  28 files changed, 3734 insertions(+), 330 deletions(-)
> ---
> base-commit: 4eb2586bd40db27d77cc5b724ca9386ed0c71d53
> change-id: 20261001-topic-qc_clk_dvfs-ef907f059611
> 
> Best regards,
> --  
> Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> 

-- 
With best wishes
Dmitry