Skip to contentSkip to conversation

clk: qcom: fix runtime PM bugs

PATCH Applied Johan Hovold 4 participants 15 messages Jul 18, 2023 – Jul 19, 2023

Subsystems (4)
Stable branch Arm Qualcomm mailing list Common clk Qualcomm clock drivers
lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

cover clk: qcom: fix runtime PM bugs

This series fixes a number of runtime PM related bugs found through inspection.

Johan

Johan Hovold (8):
clk: qcom: camcc-sc7180: fix async resume during probe
clk: qcom: dispcc-sm8450: fix runtime PM imbalance on probe errors
clk: qcom: dispcc-sm8550: fix runtime PM imbalance on probe errors
clk: qcom: gcc-sc8280xp: fix runtime PM imbalance on probe errors
clk: qcom: lpasscc-sc7280: fix missing resume during probe
clk: qcom: q6sstop-qcs404: fix missing resume during probe
clk: qcom: mss-sc7180: fix missing resume during probe
clk: qcom: turingcc-qcs404: fix missing resume during probe

 drivers/clk/qcom/camcc-sc7180.c    |  2 +-
 drivers/clk/qcom/dispcc-sm8450.c   | 13 +++++++++++--
 drivers/clk/qcom/dispcc-sm8550.c   | 13 +++++++++++--
 drivers/clk/qcom/gcc-sc8280xp.c    | 14 +++++++++++---
 drivers/clk/qcom/lpasscc-sc7280.c  | 16 ++++++++++++----
 drivers/clk/qcom/mss-sc7180.c      | 13 ++++++++++++-
 drivers/clk/qcom/q6sstop-qcs404.c  | 15 +++++++++++++--
 drivers/clk/qcom/turingcc-qcs404.c | 13 ++++++++++++-
 8 files changed, 83 insertions(+), 16 deletions(-)
Signature · 1 line
2.41.0
author
#1 lore raw

PATCH 1/8 clk: qcom: camcc-sc7180: fix async resume during probe

To make sure that the controller is runtime resumed and its power domain is enabled before accessing its registers during probe, the synchronous runtime PM interface must be used.

Fixes 8d4025943e13 ("clk: qcom: camcc-sc7180: Use runtime PM ops instead of clk ones")
Cc [email protected] # 5.11
Cc Stephen Boyd <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/clk/qcom/camcc-sc7180.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/clk/qcom/camcc-sc7180.c +1 -1
index 8a4ba7a19ed1..6f56bdbf0204 100644
@@ -1664,7 +1664,7 @@ static int cam_cc_sc7180_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = pm_runtime_get(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
 	if (ret)
 		return ret;
Signature · 1 line
2.41.0
author
#2 lore raw

PATCH 2/8 clk: qcom: dispcc-sm8450: fix runtime PM imbalance on probe errors

In reply to Johan Hovold

Make sure to decrement the runtime PM usage count before returning in case regmap initialisation fails.

Fixes 16fb89f92ec4 ("clk: qcom: Add support for Display Clock Controller on SM8450")
Cc [email protected] # 6.1
Cc Dmitry Baryshkov <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/clk/qcom/dispcc-sm8450.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/clk/qcom/dispcc-sm8450.c +11 -2
index adbfd30bfc96..84db4ff5485a 100644
@@ -1776,8 +1776,10 @@ static int disp_cc_sm8450_probe(struct platform_device *pdev)
 		return ret;
 
 	regmap = qcom_cc_map(pdev, &disp_cc_sm8450_desc);
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		goto err_put_rpm;
+	}
 
 	clk_lucid_evo_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config);
 	clk_lucid_evo_pll_configure(&disp_cc_pll1, regmap, &disp_cc_pll1_config);

@@ -1792,9 +1794,16 @@ static int disp_cc_sm8450_probe(struct platform_device *pdev)
 	regmap_update_bits(regmap, 0xe05c, BIT(0), BIT(0));
 
 	ret = qcom_cc_really_probe(pdev, &disp_cc_sm8450_desc, regmap);
+	if (ret)
+		goto err_put_rpm;
 
 	pm_runtime_put(&pdev->dev);
 
+	return 0;
+
+err_put_rpm:
+	pm_runtime_put_sync(&pdev->dev);
+
 	return ret;
 }
Signature · 1 line
2.41.0
author
#3 lore raw

PATCH 3/8 clk: qcom: dispcc-sm8550: fix runtime PM imbalance on probe errors

In reply to Johan Hovold

Make sure to decrement the runtime PM usage count before returning in case regmap initialisation fails.

Fixes 90114ca11476 ("clk: qcom: add SM8550 DISPCC driver")
Cc [email protected] # 6.3
Cc Neil Armstrong <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/clk/qcom/dispcc-sm8550.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/clk/qcom/dispcc-sm8550.c +11 -2
index 1e5a11081860..b2fae9001ff2 100644
@@ -1761,8 +1761,10 @@ static int disp_cc_sm8550_probe(struct platform_device *pdev)
 		return ret;
 
 	regmap = qcom_cc_map(pdev, &disp_cc_sm8550_desc);
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		goto err_put_rpm;
+	}
 
 	clk_lucid_evo_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config);
 	clk_lucid_evo_pll_configure(&disp_cc_pll1, regmap, &disp_cc_pll1_config);

@@ -1777,9 +1779,16 @@ static int disp_cc_sm8550_probe(struct platform_device *pdev)
 	regmap_update_bits(regmap, 0xe054, BIT(0), BIT(0));
 
 	ret = qcom_cc_really_probe(pdev, &disp_cc_sm8550_desc, regmap);
+	if (ret)
+		goto err_put_rpm;
 
 	pm_runtime_put(&pdev->dev);
 
+	return 0;
+
+err_put_rpm:
+	pm_runtime_put_sync(&pdev->dev);
+
 	return ret;
 }
Signature · 1 line
2.41.0
author
#4 lore raw

PATCH 4/8 clk: qcom: gcc-sc8280xp: fix runtime PM imbalance on probe errors

In reply to Johan Hovold

Make sure to decrement the runtime PM usage count before returning in case RCG dynamic frequency switch initialisation fails.

Fixes 2a541abd9837 ("clk: qcom: gcc-sc8280xp: Add runtime PM")
Cc Konrad Dybcio <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/clk/qcom/gcc-sc8280xp.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/clk/qcom/gcc-sc8280xp.c +11 -3
index 1fb6ffac730c..ac6f8c0c1ccb 100644
@@ -7539,8 +7539,8 @@ static int gcc_sc8280xp_probe(struct platform_device *pdev)
 
 	regmap = qcom_cc_map(pdev, &gcc_sc8280xp_desc);
 	if (IS_ERR(regmap)) {
-		pm_runtime_put(&pdev->dev);
-		return PTR_ERR(regmap);
+		ret = PTR_ERR(regmap);
+		goto err_put_rpm;
 	}
 
 	/*

@@ -7561,11 +7561,19 @@ static int gcc_sc8280xp_probe(struct platform_device *pdev)
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks, ARRAY_SIZE(gcc_dfs_clocks));
 	if (ret)
-		return ret;
+		goto err_put_rpm;
 
 	ret = qcom_cc_really_probe(pdev, &gcc_sc8280xp_desc, regmap);
+	if (ret)
+		goto err_put_rpm;
+
 	pm_runtime_put(&pdev->dev);
 
+	return 0;
+
+err_put_rpm:
+	pm_runtime_put_sync(&pdev->dev);
+
 	return ret;
 }
Signature · 1 line
2.41.0
#5 lore raw

Re: [PATCH 4/8] clk: qcom: gcc-sc8280xp: fix runtime PM imbalance on probe errors

Quoted 4 lines · Johan Hovold

Make sure to decrement the runtime PM usage count before returning in case RCG dynamic frequency switch initialisation fails.

Fixes 2a541abd9837 ("clk: qcom: gcc-sc8280xp: Add runtime PM")

I noticed you merged these for 6.6 instead of 6.5 as I had expected.

Note that I left out the stable tag here as the offending patch was merged as a fix for 6.5 and I specifically assumed this fixup would go in as a fix for 6.5 as well.

Quoted 2 lines
Cc Konrad Dybcio <[email protected]>
Signed-off-by Johan Hovold <[email protected]>

Johan

author
#6 lore raw

PATCH 5/8 clk: qcom: lpasscc-sc7280: fix missing resume during probe

In reply to Johan Hovold

Drivers that enable runtime PM must make sure that the controller is runtime resumed before accessing its registers to prevent the power domain from being disabled.

Fixes 4ab43d171181 ("clk: qcom: Add lpass clock controller driver for SC7280")
Cc [email protected] # 5.16
Cc Taniya Das <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/clk/qcom/lpasscc-sc7280.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/clk/qcom/lpasscc-sc7280.c +12 -4
index 0df2b29e95e3..e6b815aec46a 100644
@@ -118,9 +118,13 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 	ret = pm_clk_add(&pdev->dev, "iface");
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to acquire iface clock\n");
-		goto destroy_pm_clk;
+		goto err_destroy_pm_clk;
 	}
 
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		goto err_destroy_pm_clk;
+
 	if (!of_property_read_bool(pdev->dev.of_node, "qcom,adsp-pil-mode")) {
 		lpass_regmap_config.name = "qdsp6ss";
 		lpass_regmap_config.max_register = 0x3f;

@@ -128,7 +132,7 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 
 		ret = qcom_cc_probe_by_index(pdev, 0, desc);
 		if (ret)
-			goto destroy_pm_clk;
+			goto err_put_rpm;
 	}
 
 	lpass_regmap_config.name = "top_cc";

@@ -137,11 +141,15 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 
 	ret = qcom_cc_probe_by_index(pdev, 1, desc);
 	if (ret)
-		goto destroy_pm_clk;
+		goto err_put_rpm;
+
+	pm_runtime_put(&pdev->dev);
 
 	return 0;
 
-destroy_pm_clk:
+err_put_rpm:
+	pm_runtime_put_sync(&pdev->dev);
+err_destroy_pm_clk:
 	pm_clk_destroy(&pdev->dev);
 
 	return ret;
Signature · 1 line
2.41.0
maintainer
#7 lore raw

Re: [PATCH 5/8] clk: qcom: lpasscc-sc7280: fix missing resume during probe

Quoted 4 lines · Johan Hovold

Drivers that enable runtime PM must make sure that the controller is runtime resumed before accessing its registers to prevent the power domain from being disabled.

NB: the clock framework will runtime resume the controller surrounding operations, even so during probe. But this is not done for resets and gdscs - and in some clock drivers we poke registers directly from probe...

The one time this really matters is where we associate the ahb clock with the runtime state, e.g. in qcs404 turingcc. On most other platforms we just mark these clocks always-on in gcc...

Regards,
Bjorn

Quoted 57 lines
Fixes 4ab43d171181 ("clk: qcom: Add lpass clock controller driver for SC7280")
Cc [email protected] # 5.16
Cc Taniya Das <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/clk/qcom/lpasscc-sc7280.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/clk/qcom/lpasscc-sc7280.c +12 -4
index 0df2b29e95e3..e6b815aec46a 100644
@@ -118,9 +118,13 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 	ret = pm_clk_add(&pdev->dev, "iface");
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to acquire iface clock\n");
-		goto destroy_pm_clk;
+		goto err_destroy_pm_clk;
 	}
 
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		goto err_destroy_pm_clk;
+
 	if (!of_property_read_bool(pdev->dev.of_node, "qcom,adsp-pil-mode")) {
 		lpass_regmap_config.name = "qdsp6ss";
 		lpass_regmap_config.max_register = 0x3f;

@@ -128,7 +132,7 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 
 		ret = qcom_cc_probe_by_index(pdev, 0, desc);
 		if (ret)
-			goto destroy_pm_clk;
+			goto err_put_rpm;
 	}
 
 	lpass_regmap_config.name = "top_cc";

@@ -137,11 +141,15 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 
 	ret = qcom_cc_probe_by_index(pdev, 1, desc);
 	if (ret)
-		goto destroy_pm_clk;
+		goto err_put_rpm;
+
+	pm_runtime_put(&pdev->dev);
 
 	return 0;
 
-destroy_pm_clk:
+err_put_rpm:
+	pm_runtime_put_sync(&pdev->dev);
+err_destroy_pm_clk:
 	pm_clk_destroy(&pdev->dev);
 
 	return ret;
Signature · 1 line
2.41.0
#8 lore raw

Re: [PATCH 5/8] clk: qcom: lpasscc-sc7280: fix missing resume during probe

Quoted 6 lines · Bjorn Andersson
Quoted 4 lines · Johan Hovold

Drivers that enable runtime PM must make sure that the controller is runtime resumed before accessing its registers to prevent the power domain from being disabled.

Quoted 8 lines

NB: the clock framework will runtime resume the controller surrounding operations, even so during probe. But this is not done for resets and gdscs - and in some clock drivers we poke registers directly from probe...

The one time this really matters is where we associate the ahb clock with the runtime state, e.g. in qcs404 turingcc. On most other platforms we just mark these clocks always-on in gcc...

Right, I started looking at this with respect to the PM domain, but my initial commit message only mentioned the need to make sure the controller is resumed, which would have covered such interface clocks as well.

And while ending up with a concurrent request to disable the PM domain is not that likely, there is currently nothing preventing it so it still needs to be fixed.

Johan

author
#9 lore raw

PATCH 6/8 clk: qcom: q6sstop-qcs404: fix missing resume during probe

In reply to Johan Hovold

Drivers that enable runtime PM must make sure that the controller is runtime resumed before accessing its registers to prevent the power domain from being disabled.

Fixes 6cdef2738db0 ("clk: qcom: Add Q6SSTOP clock controller for QCS404")
Cc [email protected] # 5.5
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/clk/qcom/q6sstop-qcs404.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/clk/qcom/q6sstop-qcs404.c +13 -2
index 780074e05841..26e2d63614ac 100644
@@ -174,21 +174,32 @@ static int q6sstopcc_qcs404_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		return ret;
+
 	q6sstop_regmap_config.name = "q6sstop_tcsr";
 	desc = &tcsr_qcs404_desc;
 
 	ret = qcom_cc_probe_by_index(pdev, 1, desc);
 	if (ret)
-		return ret;
+		goto err_put_rpm;
 
 	q6sstop_regmap_config.name = "q6sstop_cc";
 	desc = &q6sstop_qcs404_desc;
 
 	ret = qcom_cc_probe_by_index(pdev, 0, desc);
 	if (ret)
-		return ret;
+		goto err_put_rpm;
+
+	pm_runtime_put(&pdev->dev);
 
 	return 0;
+
+err_put_rpm:
+	pm_runtime_put_sync(&pdev->dev);
+
+	return ret;
 }
 
 static const struct dev_pm_ops q6sstopcc_pm_ops = {
Signature · 1 line
2.41.0
author
#10 lore raw

PATCH 7/8 clk: qcom: mss-sc7180: fix missing resume during probe

In reply to Johan Hovold

Drivers that enable runtime PM must make sure that the controller is runtime resumed before accessing its registers to prevent the power domain from being disabled.

Fixes 8def929c4097 ("clk: qcom: Add modem clock controller driver for SC7180")
Cc [email protected] # 5.7
Cc Taniya Das <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/clk/qcom/mss-sc7180.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/clk/qcom/mss-sc7180.c +12 -1
index 5a1407440662..d106bc65470e 100644
@@ -87,11 +87,22 @@ static int mss_sc7180_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		return ret;
+
 	ret = qcom_cc_probe(pdev, &mss_sc7180_desc);
 	if (ret < 0)
-		return ret;
+		goto err_put_rpm;
+
+	pm_runtime_put(&pdev->dev);
 
 	return 0;
+
+err_put_rpm:
+	pm_runtime_put_sync(&pdev->dev);
+
+	return ret;
 }
 
 static const struct dev_pm_ops mss_sc7180_pm_ops = {
Signature · 1 line
2.41.0
author
#11 lore raw

PATCH 8/8 clk: qcom: turingcc-qcs404: fix missing resume during probe

In reply to Johan Hovold

Drivers that enable runtime PM must make sure that the controller is runtime resumed before accessing its registers to prevent the power domain from being disabled.

Fixes 892df0191b29 ("clk: qcom: Add QCS404 TuringCC")
Cc [email protected] # 5.2
Cc Bjorn Andersson <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/clk/qcom/turingcc-qcs404.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/clk/qcom/turingcc-qcs404.c +12 -1
index 43184459228f..2cd288d6c3e4 100644
@@ -125,11 +125,22 @@ static int turingcc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		return ret;
+
 	ret = qcom_cc_probe(pdev, &turingcc_desc);
 	if (ret < 0)
-		return ret;
+		goto err_put_rpm;
+
+	pm_runtime_put(&pdev->dev);
 
 	return 0;
+
+err_put_rpm:
+	pm_runtime_put_sync(&pdev->dev);
+
+	return ret;
 }
 
 static const struct dev_pm_ops turingcc_pm_ops = {
Signature · 1 line
2.41.0
#12 lore raw
In reply to Johan Hovold
Quoted 4 lines · Johan Hovold

This series fixes a number of runtime PM related bugs found through inspection.

Johan

I was thinking whether we should maybe incorporate RPM enablement into qcom common functions.. The only issue I see is that some clocks register pm clocks atop that, but I guess we could introduce some sort of a pm_clks names array to solve that?

Konrad

#13 lore raw
Quoted 8 lines · Konrad Dybcio
Quoted 2 lines · Johan Hovold

This series fixes a number of runtime PM related bugs found through inspection.

I was thinking whether we should maybe incorporate RPM enablement into qcom common functions.. The only issue I see is that some clocks register pm clocks atop that, but I guess we could introduce some sort of a pm_clks names array to solve that?

Possibly, but you currently also have the hardcoded clock enables and some other variations.

Johan

maintainer Applied
#14 lore raw
In reply to Johan Hovold
Quoted 16 lines · Johan Hovold

This series fixes a number of runtime PM related bugs found through inspection.

Johan

Johan Hovold (8):
clk: qcom: camcc-sc7180: fix async resume during probe
clk: qcom: dispcc-sm8450: fix runtime PM imbalance on probe errors
clk: qcom: dispcc-sm8550: fix runtime PM imbalance on probe errors
clk: qcom: gcc-sc8280xp: fix runtime PM imbalance on probe errors
clk: qcom: lpasscc-sc7280: fix missing resume during probe
clk: qcom: q6sstop-qcs404: fix missing resume during probe
clk: qcom: mss-sc7180: fix missing resume during probe
clk: qcom: turingcc-qcs404: fix missing resume during probe

Quoted 1 line

[...]

Applied, thanks!

[1/8] clk: qcom: camcc-sc7180: fix async resume during probe

      commit: c948ff727e25297f3a703eb5349dd66aabf004e4
[2/8] clk: qcom: dispcc-sm8450: fix runtime PM imbalance on probe errors
      commit: b0f3d01bda6c3f6f811e70f76d2040ae81f64565
[3/8] clk: qcom: dispcc-sm8550: fix runtime PM imbalance on probe errors
      commit: acaf1b3296a504d4a61b685f78baae771421608d
[4/8] clk: qcom: gcc-sc8280xp: fix runtime PM imbalance on probe errors
      commit: 10192ab375c39c58d39cba028d9685cefe1ca3c2
[5/8] clk: qcom: lpasscc-sc7280: fix missing resume during probe
      commit: 66af5339d4f8e20c6d89a490570bd94d40f1a7f6
[6/8] clk: qcom: q6sstop-qcs404: fix missing resume during probe
      commit: 97112c83f4671a4a722f99a53be4e91fac4091bc
[7/8] clk: qcom: mss-sc7180: fix missing resume during probe
      commit: e2349da0fa7ca822cda72f427345b95795358fe7
[8/8] clk: qcom: turingcc-qcs404: fix missing resume during probe
      commit: a9f71a033587c9074059132d34c74eabbe95ef26

Best regards,

Signature · 1 line
Bjorn Andersson <[email protected]>