Skip to contentSkip to conversation

spi: cadence-quadspi: runtime PM fixes

PATCH In review Johan Hovold 3 participants 9 messages Apr 21 – Apr 27

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

cover spi: cadence-quadspi: runtime PM fixes

This series fixes some runtime PM related issues in the cadence-quadspi driver.

Included is also a couple of related cleanups.

Johan

Johan Hovold (6):
spi: cadence-quadspi: fix runtime pm disable imbalance on probe
failure
spi: cadence-quadspi: fix clock imbalance on probe failure
spi: cadence-quadspi: fix unclocked access on unbind
spi: cadence-quadspi: fix runtime pm and clock imbalance on unbind
spi: cadence-quadspi: clean up disable runtime pm quirk
spi: cadence-quadspi: drop redundant match data lookup

 drivers/spi/spi-cadence-quadspi.c | 66 +++++++++++++------------------
 1 file changed, 27 insertions(+), 39 deletions(-)
Signature · 1 line
2.52.0
#1 lore raw

PATCH 1/6 spi: cadence-quadspi: fix runtime pm disable imbalance on probe failure

A recent attempt to fix the probe error handling introduced a runtime PM disable depth imbalance by incorrectly disabling runtime PM on early failures (e.g. probe deferral).

Fixes f18c8cfa4f1a ("spi: cadence-qspi: Fix probe error path and remove")
Cc Miquel Raynal (Schneider Electric) <[email protected]>
---
 drivers/spi/spi-cadence-quadspi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
drivers/spi/spi-cadence-quadspi.c +5 -6
index 50ef65fc5ded..5040e4e1cce0 100644
@@ -1871,7 +1871,7 @@ static int cqspi_probe(struct platform_device *pdev)
 	ret = clk_bulk_prepare_enable(CLK_QSPI_NUM, cqspi->clks);
 	if (ret) {
 		dev_err(dev, "Cannot enable QSPI clocks.\n");
-		goto disable_rpm;
+		return ret;
 	}
 
 	/* Obtain QSPI reset control */
@@ -1981,7 +1981,7 @@ static int cqspi_probe(struct platform_device *pdev)
 		ret = cqspi_request_mmap_dma(cqspi);
 		if (ret == -EPROBE_DEFER) {
 			dev_err_probe(&pdev->dev, ret, "Failed to request mmap DMA\n");
-			goto disable_controller;
+			goto disable_rpm;
 		}
 	}
 
@@ -1999,14 +1999,13 @@ static int cqspi_probe(struct platform_device *pdev)
 release_dma_chan:
 	if (cqspi->rx_chan)
 		dma_release_channel(cqspi->rx_chan);
-disable_controller:
+disable_rpm:
+	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM)))
+		pm_runtime_disable(dev);
 	cqspi_controller_enable(cqspi, 0);
 disable_clks:
 	if (pm_runtime_get_sync(&pdev->dev) >= 0)
 		clk_bulk_disable_unprepare(CLK_QSPI_NUM, cqspi->clks);
-disable_rpm:
-	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM)))
-		pm_runtime_disable(dev);
 
 	return ret;
 }
Signature · 1 line
2.52.0
Reviewed-by on v1 1/6
#2 lore raw

Re: [PATCH 1/6] spi: cadence-quadspi: fix runtime pm disable imbalance on probe failure

Hi,

Quoted 8 lines · Johan Hovold

A recent attempt to fix the probe error handling introduced a runtime PM disable depth imbalance by incorrectly disabling runtime PM on early failures (e.g. probe deferral).

Fixes f18c8cfa4f1a ("spi: cadence-qspi: Fix probe error path and remove")
Cc Miquel Raynal (Schneider Electric) <[email protected]>

Reading it again, I probably got confused by the impact of pm_runtime_set_active(), what you propose looks correct.

Thanks,
Miquèl

#3 lore raw

PATCH 2/6 spi: cadence-quadspi: fix clock imbalance on probe failure

In reply to Johan Hovold

Drop the bogus runtime PM get on probe failures that was never needed and that leaks a usage count reference while preventing the clocks from being disabled (as runtime PM has not yet been enabled).

Fixes 1889dd208197 ("spi: cadence-quadspi: Fix clock disable on probe failure path")
Cc Anurag Dutta <[email protected]>
---
 drivers/spi/spi-cadence-quadspi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/spi/spi-cadence-quadspi.c +1 -2
index 5040e4e1cce0..b79f48f2420c 100644
@@ -2004,8 +2004,7 @@ static int cqspi_probe(struct platform_device *pdev)
 		pm_runtime_disable(dev);
 	cqspi_controller_enable(cqspi, 0);
 disable_clks:
-	if (pm_runtime_get_sync(&pdev->dev) >= 0)
-		clk_bulk_disable_unprepare(CLK_QSPI_NUM, cqspi->clks);
+	clk_bulk_disable_unprepare(CLK_QSPI_NUM, cqspi->clks);
 
 	return ret;
 }
Signature · 1 line
2.52.0
#4 lore raw

PATCH 3/6 spi: cadence-quadspi: fix unclocked access on unbind

In reply to Johan Hovold

Make sure that the controller is runtime resumed before disabling it during driver unbind to avoid an unclocked register access.

This issue was flagged by Sashiko when reviewing a controller deregistration fix.

Fixes 0578a6dbfe75 ("spi: spi-cadence-quadspi: add runtime pm support")
Cc Dhruva Gole <[email protected]>
---
 drivers/spi/spi-cadence-quadspi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/spi/spi-cadence-quadspi.c +3 -4
index b79f48f2420c..87dc14c53675 100644
@@ -2028,14 +2028,13 @@ static void cqspi_remove(struct platform_device *pdev)
 	if (cqspi->rx_chan)
 		dma_release_channel(cqspi->rx_chan);
 
-	cqspi_controller_enable(cqspi, 0);
-
-
 	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM)))
 		ret = pm_runtime_get_sync(&pdev->dev);
 
-	if (ret >= 0)
+	if (ret >= 0) {
+		cqspi_controller_enable(cqspi, 0);
 		clk_bulk_disable_unprepare(CLK_QSPI_NUM, cqspi->clks);
+	}
 
 	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
 		pm_runtime_put_sync(&pdev->dev);
Signature · 1 line
2.52.0
#5 lore raw

PATCH 4/6 spi: cadence-quadspi: fix runtime pm and clock imbalance on unbind

In reply to Johan Hovold

Make sure to balance the runtime PM usage count before returning on probe failure (to allow the controller to suspend after a probe deferral) and to only drop the usage count on driver unbind to avoid a clock disable imbalance.

Also restore the autosuspend setting.

Fixes 0578a6dbfe75 ("spi: spi-cadence-quadspi: add runtime pm support")
Cc Dhruva Gole <[email protected]>
---
 drivers/spi/spi-cadence-quadspi.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)
drivers/spi/spi-cadence-quadspi.c +10 -7
index 87dc14c53675..1b0d6186c7ef 100644
@@ -1864,10 +1864,6 @@ static int cqspi_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return -ENXIO;
 
-	ret = pm_runtime_set_active(dev);
-	if (ret)
-		return ret;
-
 	ret = clk_bulk_prepare_enable(CLK_QSPI_NUM, cqspi->clks);
 	if (ret) {
 		dev_err(dev, "Cannot enable QSPI clocks.\n");
@@ -1966,10 +1962,11 @@ static int cqspi_probe(struct platform_device *pdev)
 	cqspi->sclk = 0;
 
 	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
-		pm_runtime_enable(dev);
 		pm_runtime_set_autosuspend_delay(dev, CQSPI_AUTOSUSPEND_TIMEOUT);
 		pm_runtime_use_autosuspend(dev);
 		pm_runtime_get_noresume(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
 	}
 
 	host->num_chipselect = cqspi->num_chipselect;
@@ -2000,8 +1997,12 @@ static int cqspi_probe(struct platform_device *pdev)
 	if (cqspi->rx_chan)
 		dma_release_channel(cqspi->rx_chan);
 disable_rpm:
-	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM)))
+	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
 		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_put_noidle(dev);
+		pm_runtime_dont_use_autosuspend(dev);
+	}
 	cqspi_controller_enable(cqspi, 0);
 disable_clks:
 	clk_bulk_disable_unprepare(CLK_QSPI_NUM, cqspi->clks);
@@ -2037,8 +2038,10 @@ static void cqspi_remove(struct platform_device *pdev)
 	}
 
 	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
-		pm_runtime_put_sync(&pdev->dev);
 		pm_runtime_disable(&pdev->dev);
+		pm_runtime_set_suspended(&pdev->dev);
+		pm_runtime_put_noidle(&pdev->dev);
+		pm_runtime_dont_use_autosuspend(&pdev->dev);
 	}
 }
 
Signature · 1 line
2.52.0
#6 lore raw

PATCH 5/6 spi: cadence-quadspi: clean up disable runtime pm quirk

In reply to Johan Hovold

Commit 30dbc1c8d50f ("spi: cadence-qspi: defer runtime support on socfpga if reset bit is enabled") fixed a warm reset issue on SoCFPGA by disabling runtime PM on that platform.

Clean up the quirk implementation by never dropping the runtime PM usage count on probe instead of sprinkling conditionals throughout the driver which makes the code unnecessarily hard to read and maintain.

Cc Khairul Anuar Romli <[email protected]>
Cc Adrian Ng Ho Yin <[email protected]>
Cc Niravkumar L Rabara <[email protected]>
Cc Matthew Gerlach <[email protected]>
---
 drivers/spi/spi-cadence-quadspi.c | 47 +++++++++++++------------------
 1 file changed, 19 insertions(+), 28 deletions(-)
drivers/spi/spi-cadence-quadspi.c +19 -28
index 1b0d6186c7ef..f9f2313413cb 100644
@@ -1478,7 +1478,6 @@ static int cqspi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	int ret;
 	struct cqspi_st *cqspi = spi_controller_get_devdata(mem->spi->controller);
 	struct device *dev = &cqspi->pdev->dev;
-	const struct cqspi_driver_platdata *ddata = of_device_get_match_data(dev);
 
 	if (refcount_read(&cqspi->inflight_ops) == 0)
 		return -ENODEV;
@@ -1494,18 +1493,15 @@ static int cqspi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		return -EBUSY;
 	}
 
-	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
-		ret = pm_runtime_resume_and_get(dev);
-		if (ret) {
-			dev_err(&mem->spi->dev, "resume failed with %d\n", ret);
-			goto dec_inflight_refcount;
-		}
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		dev_err(&mem->spi->dev, "resume failed with %d\n", ret);
+		goto dec_inflight_refcount;
 	}
 
 	ret = cqspi_mem_process(mem, op);
 
-	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM)))
-		pm_runtime_put_autosuspend(dev);
+	pm_runtime_put_autosuspend(dev);
 
 	if (ret)
 		dev_err(&mem->spi->dev, "operation failed with %d\n", ret);
@@ -1961,13 +1957,11 @@ static int cqspi_probe(struct platform_device *pdev)
 	cqspi->current_cs = -1;
 	cqspi->sclk = 0;
 
-	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
-		pm_runtime_set_autosuspend_delay(dev, CQSPI_AUTOSUSPEND_TIMEOUT);
-		pm_runtime_use_autosuspend(dev);
-		pm_runtime_get_noresume(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-	}
+	pm_runtime_set_autosuspend_delay(dev, CQSPI_AUTOSUSPEND_TIMEOUT);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
 
 	host->num_chipselect = cqspi->num_chipselect;
 
@@ -1997,12 +1991,11 @@ static int cqspi_probe(struct platform_device *pdev)
 	if (cqspi->rx_chan)
 		dma_release_channel(cqspi->rx_chan);
 disable_rpm:
-	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
-		pm_runtime_disable(dev);
-		pm_runtime_set_suspended(dev);
-		pm_runtime_put_noidle(dev);
-		pm_runtime_dont_use_autosuspend(dev);
-	}
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_dont_use_autosuspend(dev);
+
 	cqspi_controller_enable(cqspi, 0);
 disable_clks:
 	clk_bulk_disable_unprepare(CLK_QSPI_NUM, cqspi->clks);
@@ -2037,12 +2030,10 @@ static void cqspi_remove(struct platform_device *pdev)
 		clk_bulk_disable_unprepare(CLK_QSPI_NUM, cqspi->clks);
 	}
 
-	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
-		pm_runtime_disable(&pdev->dev);
-		pm_runtime_set_suspended(&pdev->dev);
-		pm_runtime_put_noidle(&pdev->dev);
-		pm_runtime_dont_use_autosuspend(&pdev->dev);
-	}
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 }
 
 static int cqspi_runtime_suspend(struct device *dev)
Signature · 1 line
2.52.0
#7 lore raw

PATCH 6/6 spi: cadence-quadspi: drop redundant match data lookup

In reply to Johan Hovold

Use the OF match data stored at probe instead of looking it up again on driver unbind.

---
 drivers/spi/spi-cadence-quadspi.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/spi/spi-cadence-quadspi.c +1 -4
index f9f2313413cb..8ffc6c5f9888 100644
@@ -2005,13 +2005,10 @@ static int cqspi_probe(struct platform_device *pdev)
 
 static void cqspi_remove(struct platform_device *pdev)
 {
-	const struct cqspi_driver_platdata *ddata;
 	struct cqspi_st *cqspi = platform_get_drvdata(pdev);
-	struct device *dev = &pdev->dev;
+	const struct cqspi_driver_platdata *ddata = cqspi->ddata;
 	int ret = 0;
 
-	ddata = of_device_get_match_data(dev);
-
 	spi_unregister_controller(cqspi->host);
 
 	refcount_set(&cqspi->refcount, 0);
Signature · 1 line
2.52.0
maintainer
#8 lore raw
In reply to Johan Hovold
Quoted 8 lines · Johan Hovold

spi: cadence-quadspi: runtime PM fixes

This series fixes some runtime PM related issues in the cadence-quadspi driver.

Included is also a couple of related cleanups.

[...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-7.2

Thanks!

[1/6] spi: cadence-quadspi: fix runtime pm disable imbalance on probe failure

      https://git.kernel.org/broonie/spi/c/5ff4d5d1af0c
[2/6] spi: cadence-quadspi: fix clock imbalance on probe failure
      https://git.kernel.org/broonie/spi/c/cba53fe20c18
[3/6] spi: cadence-quadspi: fix unclocked access on unbind
      https://git.kernel.org/broonie/spi/c/233db2cb14db
[4/6] spi: cadence-quadspi: fix runtime pm and clock imbalance on unbind
      https://git.kernel.org/broonie/spi/c/5e8bb0cc72f1
[5/6] spi: cadence-quadspi: clean up disable runtime pm quirk
      https://git.kernel.org/broonie/spi/c/edbaae583ead
[6/6] spi: cadence-quadspi: drop redundant match data lookup
      https://git.kernel.org/broonie/spi/c/37c9dfa385db

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark