Re: [PATCH 04/15] clk: qcom: gcc-sm6375: Add runtime PM

From: Stephan Gerhold <stephan@gerhold.net>
To: Bjorn Andersson <andersson@kernel.org>
Cc: Konrad Dybcio <konrad.dybcio@linaro.org>,
	Andy Gross <agross@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Marijn Suijten <marijn.suijten@somainline.org>,
	linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 04/15] clk: qcom: gcc-sm6375: Add runtime PM
Date: Tue, 18 Jul 2023 14:07:12 +0200	[thread overview]
Message-ID: <ZLaAcELnsldc98Uk@gerhold.net> (raw)
In-Reply-To: <pv7pcn5xc7xdjigefrwzvbjvp5lmgfziywfpvxn2nl7hjnvyre@kukniawnm5rm>

On Mon, Jul 17, 2023 at 09:02:29PM -0700, Bjorn Andersson wrote:
> On Mon, Jul 17, 2023 at 06:26:35PM +0200, Stephan Gerhold wrote:
> > On Mon, Jul 17, 2023 at 05:19:11PM +0200, Konrad Dybcio wrote:
> > > The GCC block on SM6375 is powered by the VDD_CX rail. We need to ensure
> > > that it's enabled to prevent unwanted power collapse.
> > > 
> > > Enable runtime PM to keep the power flowing only when necessary.
> > > 
> > 
> > Are you sure this is necessary? If VDD_CX was really possible to fully
> > "power collapse" then I would expect that you lose all register
> > settings. This is not something we want or can even handle for GCC.
> > You would need to restore all frequency settings, branch bits etc etc.
> > 
> 
> This differ between platforms, some allow us to completely power down CX
> while keeping registers state using MX, others require that CX stays in
> retention at least.
> 
> So, CX isn't the only rail powering GCC. For the most part though, we
> have a relationship between frequencies votes for by clients and the
> corner of CX, and hence I think the current description is ok...
> 

This patch is just about sending enable/disable votes for the power
domains though, based on runtime PM which triggers when all the clocks
are disabled.

It's unrelated to voting for CX corners required by certain clock
frequencies (we handle those in the OPP tables of the consumers).
And it's also unrelated to ensuring rentention of register contents
since we actually release all votes when the clocks are idle.

So while adding runtime PM to all the clock drivers sounds nice, I'm
a bit confused what problem we're actually solving with this patch. :)

Thanks,
Stephan