Skip to contentSkip to conversation

cpufreq: tegra20: Fix imbalanced clock enable count

PATCH In review Dmitry Osipenko 3 participants 7 messages May 22, 2018 – May 24, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 cpufreq: tegra20: Fix imbalanced clock enable count

Tegra20-cpufreq driver missed enabling the CPU clocks. This results in a clock-enable refcount disbalance on PLL_P <-> PLL_X reparent, causing PLL_X to get disabled while it shouldn't. Fix this by enabling the clocks on the driver probe.

---

CPUFreq maintainers,

Please take into account that this patch is made on top of my recent series of patches [0] "Clean up Tegra20 cpufreq driver" that was fully reviewed, but seems not applied yet. Let me know if you prefer to re-spin the [0], including this patch into the series.

[0] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=45321

 drivers/cpufreq/tegra20-cpufreq.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/cpufreq/tegra20-cpufreq.c +15 -1
index 05f57dcd5215..ca5229265b60 100644
@@ -176,6 +176,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 		goto put_pll_x;
 	}
 
+	err = clk_prepare_enable(cpufreq->pll_x_clk);
+	if (err)
+		goto put_pll_p;
+
+	err = clk_prepare_enable(cpufreq->pll_p_clk);
+	if (err)
+		goto disable_pll_x;
+
 	cpufreq->dev = &pdev->dev;
 	cpufreq->driver.get = cpufreq_generic_get;
 	cpufreq->driver.attr = cpufreq_generic_attr;
@@ -192,12 +200,16 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 
 	err = cpufreq_register_driver(&cpufreq->driver);
 	if (err)
-		goto put_pll_p;
+		goto disable_pll_p;
 
 	platform_set_drvdata(pdev, cpufreq);
 
 	return 0;
 
+disable_pll_p:
+	clk_disable_unprepare(cpufreq->pll_p_clk);
+disable_pll_x:
+	clk_disable_unprepare(cpufreq->pll_x_clk);
 put_pll_p:
 	clk_put(cpufreq->pll_p_clk);
 put_pll_x:
@@ -214,6 +226,8 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev)
 
 	cpufreq_unregister_driver(&cpufreq->driver);
 
+	clk_disable_unprepare(cpufreq->pll_p_clk);
+	clk_disable_unprepare(cpufreq->pll_x_clk);
 	clk_put(cpufreq->pll_p_clk);
 	clk_put(cpufreq->pll_x_clk);
 	clk_put(cpufreq->cpu_clk);
Signature · 1 line
2.17.0
maintainer Acked-by on v1 1/1
#1 lore raw
Quoted 8 lines · Dmitry Osipenko

Tegra20-cpufreq driver missed enabling the CPU clocks. This results in a clock-enable refcount disbalance on PLL_P <-> PLL_X reparent, causing PLL_X to get disabled while it shouldn't. Fix this by enabling the clocks on the driver probe.

---

Quoted 8 lines

CPUFreq maintainers,

Please take into account that this patch is made on top of my recent series of patches [0] "Clean up Tegra20 cpufreq driver" that was fully reviewed, but seems not applied yet. Let me know if you prefer to re-spin the [0], including this patch into the series.

[0] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=45321

This is already picked by Rafael and is sitting in pm/bleeding-edge branch. Should get merged into linux-next in a day or two.

Quoted 41 lines
 drivers/cpufreq/tegra20-cpufreq.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/cpufreq/tegra20-cpufreq.c +13 -1
index 05f57dcd5215..ca5229265b60 100644
@@ -176,6 +176,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 		goto put_pll_x;
 	}
 
+	err = clk_prepare_enable(cpufreq->pll_x_clk);
+	if (err)
+		goto put_pll_p;
+
+	err = clk_prepare_enable(cpufreq->pll_p_clk);
+	if (err)
+		goto disable_pll_x;
+
 	cpufreq->dev = &pdev->dev;
 	cpufreq->driver.get = cpufreq_generic_get;
 	cpufreq->driver.attr = cpufreq_generic_attr;
@@ -192,12 +200,16 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 
 	err = cpufreq_register_driver(&cpufreq->driver);
 	if (err)
-		goto put_pll_p;
+		goto disable_pll_p;
 
 	platform_set_drvdata(pdev, cpufreq);
 
 	return 0;
 
+disable_pll_p:
+	clk_disable_unprepare(cpufreq->pll_p_clk);
+disable_pll_x:
+	clk_disable_unprepare(cpufreq->pll_x_clk);
 put_pll_p:
 	clk_put(cpufreq->pll_p_clk);
 put_pll_x:

drivers/cpufreq/tegra20-cpufreq.c static int tegra20_cpufreq_remove(struct platform_device *pdev) · line 233

Quoted 8 lines
drivers/cpufreq/tegra20-cpufreq.c +2 -0
@@ -214,6 +226,8 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev)
 
 	cpufreq_unregister_driver(&cpufreq->driver);
 
+	clk_disable_unprepare(cpufreq->pll_p_clk);
+	clk_disable_unprepare(cpufreq->pll_x_clk);
 	clk_put(cpufreq->pll_p_clk);
 	clk_put(cpufreq->pll_x_clk);
 	clk_put(cpufreq->cpu_clk);
Signature · 1 line
viresh
#2 lore raw
Quoted 16 lines · Viresh Kumar
Quoted 15 lines · Dmitry Osipenko

Tegra20-cpufreq driver missed enabling the CPU clocks. This results in a clock-enable refcount disbalance on PLL_P <-> PLL_X reparent, causing PLL_X to get disabled while it shouldn't. Fix this by enabling the clocks on the driver probe.

---

CPUFreq maintainers,

Please take into account that this patch is made on top of my recent series of patches [0] "Clean up Tegra20 cpufreq driver" that was fully reviewed, but seems not applied yet. Let me know if you prefer to re-spin the [0], including this patch into the series.

Quoted 4 lines
Quoted 1 line

[0] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=45321

This is already picked by Rafael and is sitting in pm/bleeding-edge branch. Should get merged into linux-next in a day or two.

Neat, thank you for letting me know.

Quoted 52 lines
Quoted 41 lines
 drivers/cpufreq/tegra20-cpufreq.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/cpufreq/tegra20-cpufreq.c +13 -1
index 05f57dcd5215..ca5229265b60 100644
@@ -176,6 +176,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 		goto put_pll_x;
 	}
 
+	err = clk_prepare_enable(cpufreq->pll_x_clk);
+	if (err)
+		goto put_pll_p;
+
+	err = clk_prepare_enable(cpufreq->pll_p_clk);
+	if (err)
+		goto disable_pll_x;
+
 	cpufreq->dev = &pdev->dev;
 	cpufreq->driver.get = cpufreq_generic_get;
 	cpufreq->driver.attr = cpufreq_generic_attr;
@@ -192,12 +200,16 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 
 	err = cpufreq_register_driver(&cpufreq->driver);
 	if (err)
-		goto put_pll_p;
+		goto disable_pll_p;
 
 	platform_set_drvdata(pdev, cpufreq);
 
 	return 0;
 
+disable_pll_p:
+	clk_disable_unprepare(cpufreq->pll_p_clk);
+disable_pll_x:
+	clk_disable_unprepare(cpufreq->pll_x_clk);
 put_pll_p:
 	clk_put(cpufreq->pll_p_clk);
 put_pll_x:

drivers/cpufreq/tegra20-cpufreq.c static int tegra20_cpufreq_remove(struct platform_device *pdev) · line 233

Quoted 8 lines
drivers/cpufreq/tegra20-cpufreq.c +2 -0
@@ -214,6 +226,8 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev)
 
 	cpufreq_unregister_driver(&cpufreq->driver);
 
+	clk_disable_unprepare(cpufreq->pll_p_clk);
+	clk_disable_unprepare(cpufreq->pll_x_clk);
 	clk_put(cpufreq->pll_p_clk);
 	clk_put(cpufreq->pll_x_clk);
 	clk_put(cpufreq->cpu_clk);
#3 lore raw
Quoted 17 lines · CEST Dmitry Osipenko
Quoted 16 lines · Viresh Kumar
Quoted 15 lines · Dmitry Osipenko

Tegra20-cpufreq driver missed enabling the CPU clocks. This results in a clock-enable refcount disbalance on PLL_P <-> PLL_X reparent, causing PLL_X to get disabled while it shouldn't. Fix this by enabling the clocks on the driver probe.

---

CPUFreq maintainers,

Please take into account that this patch is made on top of my recent series of patches [0] "Clean up Tegra20 cpufreq driver" that was fully reviewed, but seems not applied yet. Let me know if you prefer to re-spin the [0], including this patch into the series.

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

[0] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=45321

This is already picked by Rafael and is sitting in pm/bleeding-edge branch. Should get merged into linux-next in a day or two.

Neat, thank you for letting me know.

It actually is there in my linux-next branch, but linux-next proper is not taking new material this week AFAICS.

You'll see this in linux-next on Monday, most probably.

#4 lore raw
Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · CEST Dmitry Osipenko
Quoted 18 lines · Viresh Kumar
Quoted 16 lines · Dmitry Osipenko

Tegra20-cpufreq driver missed enabling the CPU clocks. This results in a clock-enable refcount disbalance on PLL_P <-> PLL_X reparent, causing PLL_X to get disabled while it shouldn't. Fix this by enabling the clocks on the driver probe.

---

CPUFreq maintainers,

Please take into account that this patch is made on top of my recent series of patches [0] "Clean up Tegra20 cpufreq driver" that was fully reviewed, but seems not applied yet. Let me know if you prefer to re-spin the [0], including this patch into the series.

[0] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=45321

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

This is already picked by Rafael and is sitting in pm/bleeding-edge branch. Should get merged into linux-next in a day or two.

Neat, thank you for letting me know.

It actually is there in my linux-next branch, but linux-next proper is not taking new material this week AFAICS.

I've found it in yours git repo, thank you.

Quoted 2 lines

You'll see this in linux-next on Monday, most probably.

Yes, I noticed that linux-next stopped updating for awhile.

#5 lore raw
In reply to Viresh Kumar
Quoted 65 lines · Viresh Kumar
Quoted 8 lines · Dmitry Osipenko

Tegra20-cpufreq driver missed enabling the CPU clocks. This results in a clock-enable refcount disbalance on PLL_P <-> PLL_X reparent, causing PLL_X to get disabled while it shouldn't. Fix this by enabling the clocks on the driver probe.

---

Quoted 8 lines

CPUFreq maintainers,

Please take into account that this patch is made on top of my recent series of patches [0] "Clean up Tegra20 cpufreq driver" that was fully reviewed, but seems not applied yet. Let me know if you prefer to re-spin the [0], including this patch into the series.

[0] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=45321

This is already picked by Rafael and is sitting in pm/bleeding-edge branch. Should get merged into linux-next in a day or two.

Quoted 44 lines
 drivers/cpufreq/tegra20-cpufreq.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/cpufreq/tegra20-cpufreq.c +13 -1
index 05f57dcd5215..ca5229265b60 100644
@@ -176,6 +176,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 		goto put_pll_x;
 	}
 
+	err = clk_prepare_enable(cpufreq->pll_x_clk);
+	if (err)
+		goto put_pll_p;
+
+	err = clk_prepare_enable(cpufreq->pll_p_clk);
+	if (err)
+		goto disable_pll_x;
+
 	cpufreq->dev = &pdev->dev;
 	cpufreq->driver.get = cpufreq_generic_get;
 	cpufreq->driver.attr = cpufreq_generic_attr;
@@ -192,12 +200,16 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 
 	err = cpufreq_register_driver(&cpufreq->driver);
 	if (err)
-		goto put_pll_p;
+		goto disable_pll_p;
 
 	platform_set_drvdata(pdev, cpufreq);
 
 	return 0;
 
+disable_pll_p:
+	clk_disable_unprepare(cpufreq->pll_p_clk);
+disable_pll_x:
+	clk_disable_unprepare(cpufreq->pll_x_clk);
 put_pll_p:
 	clk_put(cpufreq->pll_p_clk);
 put_pll_x:
@@ -214,6 +226,8 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev)
 
 	cpufreq_unregister_driver(&cpufreq->driver);
 
Quoted 8 lines

drivers/cpufreq/tegra20-cpufreq.c static int tegra20_cpufreq_remove(struct platform_device *pdev) · line 233

Quoted 5 lines
drivers/cpufreq/tegra20-cpufreq.c +2 -0
+	clk_disable_unprepare(cpufreq->pll_p_clk);
+	clk_disable_unprepare(cpufreq->pll_x_clk);
 	clk_put(cpufreq->pll_p_clk);
 	clk_put(cpufreq->pll_x_clk);
 	clk_put(cpufreq->cpu_clk);

Please hold on this patch, seems I interpreted the cpufreq driver logic incorrectly and it is probably fine as it is. I'll re-check later today.

#6 lore raw
Quoted 75 lines · Dmitry Osipenko
Quoted 65 lines · Viresh Kumar
Quoted 8 lines · Dmitry Osipenko

Tegra20-cpufreq driver missed enabling the CPU clocks. This results in a clock-enable refcount disbalance on PLL_P <-> PLL_X reparent, causing PLL_X to get disabled while it shouldn't. Fix this by enabling the clocks on the driver probe.

---

Quoted 8 lines

CPUFreq maintainers,

Please take into account that this patch is made on top of my recent series of patches [0] "Clean up Tegra20 cpufreq driver" that was fully reviewed, but seems not applied yet. Let me know if you prefer to re-spin the [0], including this patch into the series.

[0] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=45321

This is already picked by Rafael and is sitting in pm/bleeding-edge branch. Should get merged into linux-next in a day or two.

Quoted 44 lines
 drivers/cpufreq/tegra20-cpufreq.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/cpufreq/tegra20-cpufreq.c +13 -1
index 05f57dcd5215..ca5229265b60 100644
@@ -176,6 +176,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 		goto put_pll_x;
 	}
 
+	err = clk_prepare_enable(cpufreq->pll_x_clk);
+	if (err)
+		goto put_pll_p;
+
+	err = clk_prepare_enable(cpufreq->pll_p_clk);
+	if (err)
+		goto disable_pll_x;
+
 	cpufreq->dev = &pdev->dev;
 	cpufreq->driver.get = cpufreq_generic_get;
 	cpufreq->driver.attr = cpufreq_generic_attr;
@@ -192,12 +200,16 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
 
 	err = cpufreq_register_driver(&cpufreq->driver);
 	if (err)
-		goto put_pll_p;
+		goto disable_pll_p;
 
 	platform_set_drvdata(pdev, cpufreq);
 
 	return 0;
 
+disable_pll_p:
+	clk_disable_unprepare(cpufreq->pll_p_clk);
+disable_pll_x:
+	clk_disable_unprepare(cpufreq->pll_x_clk);
 put_pll_p:
 	clk_put(cpufreq->pll_p_clk);
 put_pll_x:
@@ -214,6 +226,8 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev)
 
 	cpufreq_unregister_driver(&cpufreq->driver);
 
Quoted 8 lines

drivers/cpufreq/tegra20-cpufreq.c static int tegra20_cpufreq_remove(struct platform_device *pdev) · line 233

Quoted 5 lines
drivers/cpufreq/tegra20-cpufreq.c +2 -0
+	clk_disable_unprepare(cpufreq->pll_p_clk);
+	clk_disable_unprepare(cpufreq->pll_x_clk);
 	clk_put(cpufreq->pll_p_clk);
 	clk_put(cpufreq->pll_x_clk);
 	clk_put(cpufreq->cpu_clk);
Quoted 3 lines

Please hold on this patch, seems I interpreted the cpufreq driver logic incorrectly and it is probably fine as it is. I'll re-check later today.

I've re-checked the driver and it is fine, PLL_X disabling is expected on a switching parent clock to PLL_P as for the target frequency. This patch isn't needed, please scratch it.

It occurred to me that the driver could be improved further a tad while I was performing the re-check, hence likely there will be couple more patches from me.