Re: [PATCH v6 00/12] Unregister critical branch clocks + some RPM

[Konrad Dybcio]

On 1/25/24 11:16, Taniya Das wrote:
> Hi Konrad,
>
> Thanks for your patch.
>
> On 1/13/2024 8:20 PM, Konrad Dybcio wrote:
> > On Qualcomm SoCs, certain branch clocks either need to be always-on, or
> > should be if you're interested in touching some part of the hardware.
> >
> > Using CLK_IS_CRITICAL for this purpose sounds like a genius idea,
> > however that messes with the runtime pm handling - if a clock is
> > marked as such, the clock controller device will never enter the
> > "suspended" state, leaving the associated resources online, which in
> > turn breaks SoC-wide suspend.
>
> I am really curious to know a little more about the SoC-Wide Suspend not happening on these targets. Could you add more details here ?
>
> The Resource Power Manager (RPM) is the main aggregator on these targets where the active & sleep votes on XO, shared rails (CX/MX) decide the SoC wide suspend. The High Level OS on our internal platforms never had any suspend issues due to clocks(GCC/GPUCC) or shared rails being kept enabled from the consumers.

With the common clock framework, CLK_IS_CRITICAL blocks pm operations, as
clk_disable fails at some point. Since RPM(h)PDs are modeled as pmdomains,
this in turn results in them never getting disabled, leading to outstanding
votes. Then, RPM(h) sees these votes and (among other things which are not
properly described on most SoCs leading to dangling votes) decides that
CXPD/XOSD/AOSD can't be entered because there's a request on a resource.

> > This series aims to solve that on a couple SoCs that I could test the
> > changes on and it sprinkles some runtime pm enablement atop these drivers.
>
> As CX is a shared resource/rail on these specific targets we definitely do not achieve any power saving with the runtime pm attached to these clock controllers, but I see a little more SW overhead. Though you could please add your observations/comments.

Hm, simply adding a power-domains entry to denote the required
performance state values when voting for downstream GDSCs would
be enough and runtime PM only makes sense if there's an additional
rail, say MMCX or GFX. But see the comment below.

> Removing the CLK_IS_CRITICAL is a good cleanup and moving them to probe is a good way to handle the always-on clocks.

Unless it turns out to be really messy with keeping backwards DTS
compatibilty, the goal is to use the pm_clk APIs to only keep the
interface/xo/sleep clocks for subsystems active when that subsystem
is active (i.e. when SUBSYS_CC is *runtime-active*).

This will result in a treewide patchset.

Konrad