[PATCH v2] spi: cadence-quadspi: Parse DT for flashes with the rest of the DT parsing

[PATCH v2] spi: cadence-quadspi: Parse DT for flashes with the rest of the DT parsing

[Mark Brown]

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-aee8c7fa21f2@kernel.org
---
 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>

Re: [PATCH v2] spi: cadence-quadspi: Parse DT for flashes with the rest of the DT parsing

[Nishanth Menon]

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-gistfile1-txt-L1004
next-20251204 + this patch -> The issue still exists at least on my
platform.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource

Re: [PATCH v2] spi: cadence-quadspi: Parse DT for flashes with the rest of the DT parsing

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3471 bytes --]

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-gistfile1-txt-L1004
> 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)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] spi: cadence-quadspi: Parse DT for flashes with the rest of the DT parsing

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

On Fri, Dec 05, 2025 at 12:03:14AM +0000, Mark Brown wrote:
> On Thu, Dec 04, 2025 at 02:00:27PM -0600, Nishanth Menon wrote:

> +		ret = pm_runtime_set_active(dev);
> +		if (ret)
> +			goto err_probe_setup_failed;

Oops:

s/err_//

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] spi: cadence-quadspi: Parse DT for flashes with the rest of the DT parsing

[Mark Brown]

On Thu, 04 Dec 2025 19:13:35 +0000, 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.
> 
> [...]

Applied to

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

Thanks!

[1/1] spi: cadence-quadspi: Parse DT for flashes with the rest of the DT parsing
      commit: 9f0736a4e136a6eb61e0cf530ddc18ab6d816ba3

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

Re: [PATCH v2] spi: cadence-quadspi: Parse DT for flashes with the rest of the DT parsing

[Dutta, Anurag]

On 15-12-2025 19:29, Mark Brown wrote:
> On Thu, 04 Dec 2025 19:13:35 +0000, 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.
>>
>> [...]
> Applied to
>
>     https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
>
> Thanks!
>
> [1/1] spi: cadence-quadspi: Parse DT for flashes with the rest of the DT parsing
>        commit: 9f0736a4e136a6eb61e0cf530ddc18ab6d816ba3
Hi Mark
I was under the impression that we are agreeing on this solution :
https://lore.kernel.org/all/20251212072312.2711806-1-a-dutta@ti.com/

Regards
Anurag
> 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
>

Re: [PATCH v2] spi: cadence-quadspi: Parse DT for flashes with the rest of the DT parsing

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 356 bytes --]

On Tue, Dec 16, 2025 at 09:38:01AM +0530, Dutta, Anurag wrote:

> I was under the impression that we are agreeing on this solution :
> https://lore.kernel.org/all/20251212072312.2711806-1-a-dutta@ti.com/

I think we should do both, there's no need to do so much of the init
work if the DT setup is just broken so the device can never possibly
instantiate.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]