diff mbox series

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

Message ID 20230717-topic-branch_aon_cleanup-v1-4-27784d27a4f4@linaro.org
State New
Headers
Series Unregister critical branch clocks + some RPM |

Commit Message

Konrad Dybcio July 17, 2023, 3:19 p.m. UTC 
  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.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-sm6375.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Stephan Gerhold July 17, 2023, 4:26 p.m. UTC | #1 
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.

Otherwise it's a retention state, but these are covered by the
corners/levels, not the enable/disable state.

I think most of these power domains are effectively always-on. The
important part is voting for minimal corners/levels so they can go to
minimal retention state with vlow/vmin.

Thanks,
Stephan
Bjorn Andersson July 18, 2023, 4:02 a.m. UTC | #2 
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...

> Otherwise it's a retention state, but these are covered by the
> corners/levels, not the enable/disable state.
> 

I _think_ we still want to suspend each individual device (and the vote
from Linux), and let the system keep us at retention, instead of off...

> I think most of these power domains are effectively always-on. The
> important part is voting for minimal corners/levels so they can go to
> minimal retention state with vlow/vmin.
> 

When you hit s2idle, you should expect to have the majority of the
rpm(h) PDs be voted off.

Regards,
Bjorn

> Thanks,
> Stephan
Stephan Gerhold July 18, 2023, 12:07 p.m. UTC | #3 
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
Konrad Dybcio July 18, 2023, 12:49 p.m. UTC | #4 
On 18.07.2023 14:07, Stephan Gerhold wrote:
> 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. :)
In a very specific and unfortunate situation, there could be no other
CX votes, and trying to access (perhaps at least parts of) GCC would
result in a failure.

Konrad
Johan Hovold July 18, 2023, 1:27 p.m. UTC | #5 
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.

This bit is not correct either (and similar throughout the series).

> Enable runtime PM to keep the power flowing only when necessary.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---

> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
>  	regmap = qcom_cc_map(pdev, &gcc_sm6375_desc);
> -	if (IS_ERR(regmap))
> +	if (IS_ERR(regmap)) {
> +		pm_runtime_put(&pdev->dev);
>  		return PTR_ERR(regmap);
> +	}
>  
>  	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks, ARRAY_SIZE(gcc_dfs_clocks));
>  	if (ret)

Looks like you forgot to update this error path.

> @@ -3817,7 +3828,10 @@ static int gcc_sm6375_probe(struct platform_device *pdev)
>  	clk_lucid_pll_configure(&gpll8, regmap, &gpll8_config);
>  	clk_zonda_pll_configure(&gpll9, regmap, &gpll9_config);
>  
> -	return qcom_cc_really_probe(pdev, &gcc_sm6375_desc, regmap);
> +	ret = qcom_cc_really_probe(pdev, &gcc_sm6375_desc, regmap);
> +	pm_runtime_put(&pdev->dev);
> +
> +	return ret;

Johan
diff mbox series

Patch

diff --git a/drivers/clk/qcom/gcc-sm6375.c b/drivers/clk/qcom/gcc-sm6375.c
index 14dafea45ac9..4b2de545d3f8 100644
--- a/drivers/clk/qcom/gcc-sm6375.c
+++ b/drivers/clk/qcom/gcc-sm6375.c
@@ -7,6 +7,7 @@ 
 #include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 
 #include <dt-bindings/clock/qcom,sm6375-gcc.h>
@@ -3784,9 +3785,19 @@  static int gcc_sm6375_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	int ret;
 
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		return ret;
+
 	regmap = qcom_cc_map(pdev, &gcc_sm6375_desc);
-	if (IS_ERR(regmap))
+	if (IS_ERR(regmap)) {
+		pm_runtime_put(&pdev->dev);
 		return PTR_ERR(regmap);
+	}
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks, ARRAY_SIZE(gcc_dfs_clocks));
 	if (ret)
@@ -3817,7 +3828,10 @@  static int gcc_sm6375_probe(struct platform_device *pdev)
 	clk_lucid_pll_configure(&gpll8, regmap, &gpll8_config);
 	clk_zonda_pll_configure(&gpll9, regmap, &gpll9_config);
 
-	return qcom_cc_really_probe(pdev, &gcc_sm6375_desc, regmap);
+	ret = qcom_cc_really_probe(pdev, &gcc_sm6375_desc, regmap);
+	pm_runtime_put(&pdev->dev);
+
+	return ret;
 }
 
 static struct platform_driver gcc_sm6375_driver = {