The recent refactoring of where runtime PM is enabled done in commit f1eb4e792bb1 ("spi: spi-cadence-quadspi: Enable pm runtime earlier to avoid imbalance") made the fact that when we do a pm_runtime_disable() in the error paths of probe() we can trigger a runtime disable which in turn results in duplicate clock disables. This is particularly likely to happen when there is missing or broken DT description for the flashes attached to the controller.
Early on in the probe function we do a pm_runtime_get_noresume() since the probe function leaves the device in a powered up state but in the error path we can't assume that PM is enabled so we also manually disable everything, including clocks. This means that when runtime PM is active both it and the probe function release the same reference to the main clock for the IP, triggering warnings from the clock subsystem:
[ 8.693719] clk:75:7 already disabled [ 8.693791] WARNING: CPU: 1 PID: 185 at /usr/src/kernel/drivers/clk/clk.c:1188 clk_core_disable+0xa0/0xb ... [ 8.694261] clk_core_disable+0xa0/0xb4 (P) [ 8.694272] clk_disable+0x38/0x60 [ 8.694283] cqspi_probe+0x7c8/0xc5c [spi_cadence_quadspi] [ 8.694309] platform_probe+0x5c/0xa4
Dealing with this issue properly is complicated by the fact that we don't know if runtime PM is active so can't tell if it will disable the clocks or not. We can, however, sidestep the issue for the flash descriptions by moving their parsing to when we parse the controller properties which also save us doing a bunch of setup which can never be used so let's do that.
Reported-by: Francesco Dolcini francesco@dolcini.it Closes: https://lore.kernel.org/r/20251201072844.GA6785@francesco-nb Signed-off-by: Mark Brown broonie@kernel.org Cc: stable@vger.kernel.org --- Changes in v2: - Switch to moving the DT parsing earlier so we avoid triggering the clock referencing problems. - Link to v1: https://patch.msgid.link/20251202-spi-cadence-qspi-runtime-pm-imbalance-v1-1... --- drivers/spi/spi-cadence-quadspi.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index af6d050da1c8..bdbeef05cd72 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -1845,6 +1845,12 @@ static int cqspi_probe(struct platform_device *pdev) return -ENODEV; }
+ ret = cqspi_setup_flash(cqspi); + if (ret) { + dev_err(dev, "failed to setup flash parameters %d\n", ret); + return ret; + } + /* Obtain QSPI clock. */ cqspi->clk = devm_clk_get(dev, NULL); if (IS_ERR(cqspi->clk)) { @@ -1988,12 +1994,6 @@ static int cqspi_probe(struct platform_device *pdev) pm_runtime_get_noresume(dev); }
- ret = cqspi_setup_flash(cqspi); - if (ret) { - dev_err(dev, "failed to setup flash parameters %d\n", ret); - goto probe_setup_failed; - } - host->num_chipselect = cqspi->num_chipselect;
if (ddata && (ddata->quirks & CQSPI_SUPPORT_DEVICE_RESET))
--- base-commit: cebdea5fc60642a39a76c237257a7e6662336006 change-id: 20251202-spi-cadence-qspi-runtime-pm-imbalance-657740cf7eae
Best regards, -- Mark Brown broonie@kernel.org
On 19:13-20251204, Mark Brown wrote:
The recent refactoring of where runtime PM is enabled done in commit f1eb4e792bb1 ("spi: spi-cadence-quadspi: Enable pm runtime earlier to avoid imbalance") made the fact that when we do a pm_runtime_disable() in the error paths of probe() we can trigger a runtime disable which in turn results in duplicate clock disables. This is particularly likely to happen when there is missing or broken DT description for the flashes attached to the controller.
Early on in the probe function we do a pm_runtime_get_noresume() since the probe function leaves the device in a powered up state but in the error path we can't assume that PM is enabled so we also manually disable everything, including clocks. This means that when runtime PM is active both it and the probe function release the same reference to the main clock for the IP, triggering warnings from the clock subsystem:
[ 8.693719] clk:75:7 already disabled [ 8.693791] WARNING: CPU: 1 PID: 185 at /usr/src/kernel/drivers/clk/clk.c:1188 clk_core_disable+0xa0/0xb ... [ 8.694261] clk_core_disable+0xa0/0xb4 (P) [ 8.694272] clk_disable+0x38/0x60 [ 8.694283] cqspi_probe+0x7c8/0xc5c [spi_cadence_quadspi] [ 8.694309] platform_probe+0x5c/0xa4
Dealing with this issue properly is complicated by the fact that we don't know if runtime PM is active so can't tell if it will disable the clocks or not. We can, however, sidestep the issue for the flash descriptions by moving their parsing to when we parse the controller properties which also save us doing a bunch of setup which can never be used so let's do that.
Reported-by: Francesco Dolcini francesco@dolcini.it Closes: https://lore.kernel.org/r/20251201072844.GA6785@francesco-nb Signed-off-by: Mark Brown broonie@kernel.org Cc: stable@vger.kernel.org
https://gist.github.com/nmenon/5ca89b617113e9dbb31d4630586af945#file-gistfil... next-20251204 + this patch -> The issue still exists at least on my platform.
On Thu, Dec 04, 2025 at 02:00:27PM -0600, Nishanth Menon wrote:
On 19:13-20251204, Mark Brown wrote:
The recent refactoring of where runtime PM is enabled done in commit f1eb4e792bb1 ("spi: spi-cadence-quadspi: Enable pm runtime earlier to avoid imbalance") made the fact that when we do a pm_runtime_disable()
https://gist.github.com/nmenon/5ca89b617113e9dbb31d4630586af945#file-gistfil... next-20251204 + this patch -> The issue still exists at least on my platform.
Right, so from the log this is the one I think I mentioned earlier that the error path isn't being triggered by cqspi_setup_flash() but by something else which doesn't log anything:
[ 1.489445] 2840000.serial: ttyS4 at MMIO 0x2840000 (irq = 210, base_baud = 3000000) is a 8250 [ 1.498635] ------------[ cut here ]------------ [ 1.503239] clk:104:0 already disabled [ 1.507019] WARNING: drivers/clk/clk.c:1188 at clk_core_disable+0x80/0xa0, CPU#0: kworker/u8:2/55
so I'd not expect this to help in that case, it's specifically avoiding the issue Francesco reported where it's the DT parse. Can you put some debug statements in or something to confirm what triggers the error handling? Sorry, I'd not remembered that we'd got other triggers when I sent this.
Also, I wonder if the below completely untested change will help with that issue? I'm a bit nervous that this might mess up some power domain handling.
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index bdbeef05cd72..ff7beacbc085 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -1884,11 +1884,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_prepare_enable(cqspi->clk); if (ret) { dev_err(dev, "Cannot enable QSPI clock.\n"); @@ -1987,13 +1982,6 @@ 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_enable(dev); - pm_runtime_set_autosuspend_delay(dev, CQSPI_AUTOSUSPEND_TIMEOUT); - pm_runtime_use_autosuspend(dev); - pm_runtime_get_noresume(dev); - } - host->num_chipselect = cqspi->num_chipselect;
if (ddata && (ddata->quirks & CQSPI_SUPPORT_DEVICE_RESET)) @@ -2005,10 +1993,21 @@ static int cqspi_probe(struct platform_device *pdev) goto probe_setup_failed; }
+ if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) { + ret = pm_runtime_set_active(dev); + if (ret) + goto err_probe_setup_failed; + + pm_runtime_enable(dev); + pm_runtime_set_autosuspend_delay(dev, CQSPI_AUTOSUSPEND_TIMEOUT); + pm_runtime_use_autosuspend(dev); + pm_runtime_get_noresume(dev); + } + ret = spi_register_controller(host); if (ret) { dev_err(&pdev->dev, "failed to register SPI ctlr %d\n", ret); - goto probe_setup_failed; + goto probe_pm_failed; }
if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) { @@ -2017,9 +2016,10 @@ static int cqspi_probe(struct platform_device *pdev) }
return 0; -probe_setup_failed: +probe_pm_failed: if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) pm_runtime_disable(dev); +probe_setup_failed: cqspi_controller_enable(cqspi, 0); probe_reset_failed: if (cqspi->is_jh7110)
linux-stable-mirror@lists.linaro.org
-
Mark Brown -
Nishanth Menon