| Message ID | 20230717-topic-branch_aon_cleanup-v1-4-27784d27a4f4@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Unregister critical branch clocks + some RPM | expand |
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
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
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
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
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 --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 = {
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(-)