diff mbox series

[1/5] spi: spi-zynqmp-gqspi: fix clk_enable/disable imbalance issue

Message ID 20210416004652.2975446-2-quanyang.wang@windriver.com (mailing list archive)
State New, archived
Headers show
Series spi: spi-zynqmp-gqspi: fix spi issues | expand

Commit Message

Quanyang Wang April 16, 2021, 12:46 a.m. UTC 
From: Quanyang Wang <quanyang.wang@windriver.com>

The clks "pclk" and "ref_clk" are enabled twice during the probe. The
first time is in the function zynqmp_qspi_probe and the second time is
in zynqmp_qspi_setup_op which is called by devm_spi_register_controller.
Then calling zynqmp_qspi_remove (rmmod this module) to disable these clks
will trigger a warning as below:

[  309.124604] Unpreparing enabled qspi_ref
[  309.128641] WARNING: CPU: 1 PID: 537 at drivers/clk/clk.c:824 clk_core_unprepare+0x108/0x110

Since pm_runtime works now, clks can be enabled/disabled by calling
zynqmp_runtime_suspend/resume. So we don't need to enable these clks
explicitly in zynqmp_qspi_setup_op. Remove them to fix this issue.

And remove clk enabling/disabling in zynqmp_qspi_resume because there is
no spi transfer operation so enabling ref_clk is redundant meanwhile pclk
is not disabled for it is shared with other peripherals.

Furthermore replace clk_enable/disable with clk_prepare_enable and
clk_disable_unprepare in runtime_suspend/resume functions.

Fixes: 1c26372e5aa9 ("spi: spi-zynqmp-gqspi: Update driver to use spi-mem framework")
Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
---
 drivers/spi/spi-zynqmp-gqspi.c | 47 ++++++----------------------------
 1 file changed, 8 insertions(+), 39 deletions(-)

Comments

Mark Brown April 16, 2021, 12:55 p.m. UTC | #1 
On Fri, Apr 16, 2021 at 08:46:48AM +0800, quanyang.wang@windriver.com wrote:

> Since pm_runtime works now, clks can be enabled/disabled by calling
> zynqmp_runtime_suspend/resume. So we don't need to enable these clks
> explicitly in zynqmp_qspi_setup_op. Remove them to fix this issue.

> Fixes: 1c26372e5aa9 ("spi: spi-zynqmp-gqspi: Update driver to use spi-mem framework")

Are you *sure* this fixes is accurate?  The patch (and several of the
others that flag the same commit) doesn't apply against for-5.12, though
at this point there's not really enough time to send another pull request
so it doesn't super matter though someone will probably need to help out
with stable backports.
Quanyang Wang April 16, 2021, 2:04 p.m. UTC | #2 
Hi Mark,

On 4/16/21 8:55 PM, Mark Brown wrote:
> On Fri, Apr 16, 2021 at 08:46:48AM +0800, quanyang.wang@windriver.com wrote:
>
>> Since pm_runtime works now, clks can be enabled/disabled by calling
>> zynqmp_runtime_suspend/resume. So we don't need to enable these clks
>> explicitly in zynqmp_qspi_setup_op. Remove them to fix this issue.
>> Fixes: 1c26372e5aa9 ("spi: spi-zynqmp-gqspi: Update driver to use spi-mem framework")
> Are you *sure* this fixes is accurate?  The patch (and several of the
> others that flag the same commit) doesn't apply against for-5.12, though
> at this point there's not really enough time to send another pull request
> so it doesn't super matter though someone will probably need to help out
> with stable backports.

I am sorry. These patches should NOT be with "Fixes" tag since they base 
on the patches

which are not with "Fixes". May I send a V2 patch series which remove 
these "Fixes" tags?

Thanks,

Quanyang
Mark Brown April 16, 2021, 3:12 p.m. UTC | #3 
On Fri, Apr 16, 2021 at 10:04:30PM +0800, quanyang.wang wrote:

> I am sorry. These patches should NOT be with "Fixes" tag since they base on
> the patches

> which are not with "Fixes". May I send a V2 patch series which remove these
> "Fixes" tags?

Well, if they're fixing bugs that were present in the named commit then
the the tag makes sense, it's just a question of if they are actually
for that commit or if they are fixing things for other commits like the
runtime PM enablement.
Quanyang Wang April 16, 2021, 3:37 p.m. UTC | #4 
Hi Mark,

On 4/16/21 11:12 PM, Mark Brown wrote:
> On Fri, Apr 16, 2021 at 10:04:30PM +0800, quanyang.wang wrote:
>
>> I am sorry. These patches should NOT be with "Fixes" tag since they base on
>> the patches
>> which are not with "Fixes". May I send a V2 patch series which remove these
>> "Fixes" tags?
> Well, if they're fixing bugs that were present in the named commit then
> the the tag makes sense, it's just a question of if they are actually
> for that commit or if they are fixing things for other commits like the
> runtime PM enablement.

Yes,  they are fixing bugs which are introduced by the named commit.

But if only picking they to stable without the patch "spi: 
spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe",

this spi driver will not work. I don't know how to handle this 
condition, so I send a V2 patch series to delete the tags.

Thanks,

Quanyang
diff mbox series

Patch

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 32e53f379e9b..f9056f0a480c 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -487,24 +487,10 @@  static int zynqmp_qspi_setup_op(struct spi_device *qspi)
 {
 	struct spi_controller *ctlr = qspi->master;
 	struct zynqmp_qspi *xqspi = spi_controller_get_devdata(ctlr);
-	struct device *dev = &ctlr->dev;
-	int ret;
 
 	if (ctlr->busy)
 		return -EBUSY;
 
-	ret = clk_enable(xqspi->refclk);
-	if (ret) {
-		dev_err(dev, "Cannot enable device clock.\n");
-		return ret;
-	}
-
-	ret = clk_enable(xqspi->pclk);
-	if (ret) {
-		dev_err(dev, "Cannot enable APB clock.\n");
-		clk_disable(xqspi->refclk);
-		return ret;
-	}
 	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, GQSPI_EN_MASK);
 
 	return 0;
@@ -863,26 +849,9 @@  static int __maybe_unused zynqmp_qspi_suspend(struct device *dev)
 static int __maybe_unused zynqmp_qspi_resume(struct device *dev)
 {
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct zynqmp_qspi *xqspi = spi_controller_get_devdata(ctlr);
-	int ret = 0;
-
-	ret = clk_enable(xqspi->pclk);
-	if (ret) {
-		dev_err(dev, "Cannot enable APB clock.\n");
-		return ret;
-	}
-
-	ret = clk_enable(xqspi->refclk);
-	if (ret) {
-		dev_err(dev, "Cannot enable device clock.\n");
-		clk_disable(xqspi->pclk);
-		return ret;
-	}
 
 	spi_controller_resume(ctlr);
 
-	clk_disable(xqspi->refclk);
-	clk_disable(xqspi->pclk);
 	return 0;
 }
 
@@ -898,8 +867,8 @@  static int __maybe_unused zynqmp_runtime_suspend(struct device *dev)
 {
 	struct zynqmp_qspi *xqspi = (struct zynqmp_qspi *)dev_get_drvdata(dev);
 
-	clk_disable(xqspi->refclk);
-	clk_disable(xqspi->pclk);
+	clk_disable_unprepare(xqspi->refclk);
+	clk_disable_unprepare(xqspi->pclk);
 
 	return 0;
 }
@@ -917,16 +886,16 @@  static int __maybe_unused zynqmp_runtime_resume(struct device *dev)
 	struct zynqmp_qspi *xqspi = (struct zynqmp_qspi *)dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_enable(xqspi->pclk);
+	ret = clk_prepare_enable(xqspi->pclk);
 	if (ret) {
 		dev_err(dev, "Cannot enable APB clock.\n");
 		return ret;
 	}
 
-	ret = clk_enable(xqspi->refclk);
+	ret = clk_prepare_enable(xqspi->refclk);
 	if (ret) {
 		dev_err(dev, "Cannot enable device clock.\n");
-		clk_disable(xqspi->pclk);
+		clk_disable_unprepare(xqspi->pclk);
 		return ret;
 	}
 
@@ -1136,13 +1105,11 @@  static int zynqmp_qspi_probe(struct platform_device *pdev)
 		goto remove_master;
 	}
 
-	init_completion(&xqspi->data_completion);
-
 	xqspi->refclk = devm_clk_get(&pdev->dev, "ref_clk");
 	if (IS_ERR(xqspi->refclk)) {
 		dev_err(dev, "ref_clk clock not found.\n");
 		ret = PTR_ERR(xqspi->refclk);
-		goto clk_dis_pclk;
+		goto remove_master;
 	}
 
 	ret = clk_prepare_enable(xqspi->pclk);
@@ -1157,6 +1124,8 @@  static int zynqmp_qspi_probe(struct platform_device *pdev)
 		goto clk_dis_pclk;
 	}
 
+	init_completion(&xqspi->data_completion);
+
 	mutex_init(&xqspi->op_lock);
 
 	pm_runtime_use_autosuspend(&pdev->dev);