[PATCH 5.10.y v2 0/4] spi: spi-zynqmp-gqspi: fix runtime PM, then stop on shutdown

[PATCH 5.10.y v2 0/4] spi: spi-zynqmp-gqspi: fix runtime PM, then stop on shutdown

[Itai Handler]

This is v2 of the 5.10.y backport of e922bad8b2d5 ("spi:
spi-zynqmp-gqspi: stop the controller on shutdown"). v1 was dropped
[1]: on 5.10.y its pm_runtime_get_if_in_use() check always returns 0,
so ->shutdown never stops the controller. Rather than give 5.10.y
runtime PM and MMIO logic of its own, v2 first brings the driver's
runtime PM in line with upstream (1-3/4), then applies e922bad8b2d5
as-is (4/4, unchanged from v1).

Why it always returns 0 on 5.10.y
---------------------------------
5.10.y has none of 1-3/4. Its probe() calls
pm_runtime_put_autosuspend() without a matching get, so the usage
count rests at -1. The controller runtime suspends 3s after probe, and
a flash access never resumes it: without auto_runtime_pm, spi-mem
takes no runtime PM reference around an op.

Until 5.10.37 the clocks stayed on regardless, as
zynqmp_qspi_setup_op() enabled them a second time. 24159580abcc, the
5.10.37 backport of c6bdae08012b ("spi: spi-zynqmp-gqspi: fix
clk_enable/disable imbalance issue"), removed that because "pm_runtime
works now"; upstream it was applied directly on top of 1/4, but 5.10.y
took it without 1/4. Since then only probe() and the runtime resume
callback enable the clocks, so flash accesses made more than 3s after
probe run on a controller whose clocks Linux has disabled.

At shutdown, device_shutdown()'s pm_runtime_get_noresume() lifts the
count from -1 to 0, and pm_runtime_get_if_in_use() returns 0 whether
the controller is active or not.

Why 5.10.y lacks the fixes
--------------------------
5.10.198 carried 1/4 and 2/4, line for line as they are here: 2/4 as
19f3d5d13b75, and 1/4 as its Stable-dep-of, 2cdec9c13f81. But
2cdec9c13f81 bears the upstream ID and message of a21fbc42807b, an
earlier commit with the same subject: a one-line change to the probe
error path, dropped in favour of 1/4 when the two branches were merged
for v5.13. Marek reported that 2cdec9c13f81 looked very different from
a21fbc42807b and that it broke a platform for him [2], and 5.10.199
reverted both (1ceaf0d3a883, 649237ccf440). The difference he saw
comes from the label: 2cdec9c13f81's code is that of 58eaa7b2d07d.
What broke was not reported.

Marek, do you remember which platform it was and how it failed? If it
is still around, could you give this series a try?

I could not make this series, or 5.10.198, fail in emulation. Besides
the testing below, I ran both on a full ZynqMP system in QEMU: the PMU
firmware, TF-A and U-Boot of PetaLinux 2021.1 and 2024.2, the QSPI node
with and without its power domain, and a local QEMU change that stalls
the GQSPI while the firmware gates QSPI_REF. Flash reads and writes
across runtime suspend, and unbind and bind, worked with both, and
neither touched the controller while its clock was gated, 4/4's
->shutdown included. v5.10.272 without this series, by contrast, hangs
on its first flash access made more than 3s after probe.

The series
----------
1/4 58eaa7b2d07d makes probe take the reference it drops, and sets
    auto_runtime_pm so that spi-mem resumes the controller around
    every op
2/4 1527b076ae2c makes sure the controller is not runtime suspended
    when the probe error path and remove() disable its clocks by hand
3/4 2219576883e7 undoes autosuspend at driver exit
4/4 e922bad8b2d5 the v1 backport, unchanged

1-3/4 are in 5.15.y and 6.1.y, with no reverts there: 58eaa7b2d07d
since v5.13, 1527b076ae2c since 5.15.135 and 6.1.57, and 2219576883e7
since 5.15.174 and 6.1.120. With them, the runtime PM, clock and
controller (un)registration calls in probe(), remove() and the runtime
PM callbacks are the same as in v7.3, in the same order, bar a
pm_runtime_mark_last_busy() that v7.3's pm_runtime_put_autosuspend()
now makes itself.

Testing
-------
Tested in QEMU only, not on hardware: Xilinx QEMU (xilinx_v2024.2) and
its ZynqMP GQSPI model with an n25q512a11, running v5.10.272 with v1
alone against v5.10.272 with this series.

                                        v1 alone     v2
  usage count after probe                  -1         0
  usage count after unbind and bind        -1         0
  runtime status after a flash read     suspended   active
  GQSPI_EN after "halt -f"                 0x1       0x0

The last row is what e922bad8b2d5 is for: with v1 alone ->shutdown
returns early and leaves the controller enabled. The halt follows a
flash read, with the autosuspend delay raised so that the controller is
still active when ->shutdown runs; GQSPI_EN is then read through the
QEMU monitor. The usage count is read from sysfs, which needs
CONFIG_PM_ADVANCED_DEBUG. v7.3 passes the two checks that do not need
the usage count.

Caveats
-------
- The QEMU GQSPI has fixed clocks and no power domain here, so runtime
  suspend gates nothing: this shows that runtime PM is balanced and
  that ->shutdown now acts. The full-system runs above add the PMU
  firmware and a modelled clock gate, but a model is not silicon: if
  what broke in 5.10.198 depends on the hardware or on that platform's
  firmware, none of this would catch it, hence the question to Marek.

- spi_controller_suspend() in ->shutdown is a no-op on 5.10.y, as the
  controller is not queued. Upstream it also takes the bus lock, and so
  waits for an in-flight spi-mem op; that came with bef4a48f4ef7 ("spi:
  Fix null dereference on suspend", v6.7), which is in 6.1.y but not in
  5.15.y or 5.10.y. 5.10.y's own zynqmp_qspi_suspend() has the same
  gap, so I have left that core change out of this series.

Changes in v2:
- add 1-3/4, so that 4/4 works on 5.10.y as-is
- 4/4 is unchanged

v1: https://lore.kernel.org/stable/20260922140318.668429-1-itai.handler@gmail.com/

[1] https://lore.kernel.org/stable/2026-09-24-daily-reply-0010-re-spi-zynqmp-gqspi-shutdown-5-10-drop@kernel.org/
[2] https://lore.kernel.org/stable/9afe9285-6f46-46d9-bd21-2ea5c4dc43c0@denx.de/

Dinghao Liu (1):
  spi: spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe

Itai Handler (1):
  spi: spi-zynqmp-gqspi: stop the controller on shutdown

Jinjie Ruan (1):
  spi: zynqmp-gqspi: Undo runtime PM changes at driver exit time​

Johan Hovold (1):
  spi: zynqmp-gqspi: fix clock imbalance on probe failure

 drivers/spi/spi-zynqmp-gqspi.c | 60 +++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 5 deletions(-)


base-commit: aca95a35f1bcc0626c4fa4d1252bf2d4c990a1a2
-- 
2.34.1

[PATCH 5.10.y v2 1/4] spi: spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe

[Itai Handler]

From: Dinghao Liu <dinghao.liu@zju.edu.cn>

commit 58eaa7b2d07d3c25e1068b0bf42ca7e7464f4bca upstream.

There is a PM usage counter decrement after zynqmp_qspi_init_hw()
without any refcount increment, which leads to refcount leak.Add
a refcount increment to balance the refcount. Also set
auto_runtime_pm to resume suspended spi controller.

Fixes: 9e3a000362aec ("spi: zynqmp: Add pm runtime support")
Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
Tested-by: Quanyang Wang <quanyang.wang@windriver.com>
Link: https://lore.kernel.org/r/20210415074644.24646-1-dinghao.liu@zju.edu.cn
Signed-off-by: Mark Brown <broonie@kernel.org>
[ itai: 5.10.198 carried this change as commit 2cdec9c13f81, taken as
  a Stable-dep-of 1527b076ae2c ("spi: zynqmp-gqspi: fix clock imbalance
  on probe failure") but under the ID and message of a21fbc42807b, an
  earlier upstream commit of the same subject whose change the merge
  into v5.13 discarded, and 5.10.199 reverted it in commit 1ceaf0d3a883.
  Without it, probe leaves the usage count at -1 and no flash access
  ever runtime resumes the controller, although commit 24159580abcc
  ("spi: spi-zynqmp-gqspi: fix clk_enable/disable imbalance issue"), in
  5.10 since 5.10.37, removed the clock enables from the setup path
  because runtime PM provides them.  Context only: probe registers the
  controller with spi_register_controller() in this tree, as of commit
  aeb00d7e89ca ("spi: zynqmp-gqspi: fix controller deregistration") ]
Assisted-by: LLM
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
 drivers/spi/spi-zynqmp-gqspi.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 5c470daf9367..7223eee6db88 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -1139,11 +1139,16 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 	pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
+
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to pm_runtime_get_sync: %d\n", ret);
+		goto clk_dis_all;
+	}
+
 	/* QSPI controller initializations */
 	zynqmp_qspi_init_hw(xqspi);
 
-	pm_runtime_mark_last_busy(&pdev->dev);
-	pm_runtime_put_autosuspend(&pdev->dev);
 	xqspi->irq = platform_get_irq(pdev, 0);
 	if (xqspi->irq <= 0) {
 		ret = -ENXIO;
@@ -1170,6 +1175,7 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
 			    SPI_TX_DUAL | SPI_TX_QUAD;
 	ctlr->dev.of_node = np;
+	ctlr->auto_runtime_pm = true;
 
 	ret = spi_register_controller(ctlr);
 	if (ret) {
@@ -1177,9 +1183,13 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 		goto clk_dis_all;
 	}
 
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
+
 	return 0;
 
 clk_dis_all:
+	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	clk_disable_unprepare(xqspi->refclk);
-- 
2.34.1

[PATCH 5.10.y v2 2/4] spi: zynqmp-gqspi: fix clock imbalance on probe failure

[Itai Handler]

From: Johan Hovold <johan+linaro@kernel.org>

commit 1527b076ae2cb6a9c590a02725ed39399fcad1cf upstream.

Make sure that the device is not runtime suspended before explicitly
disabling the clocks on probe failure and on driver unbind to avoid a
clock enable-count imbalance.

Fixes: 9e3a000362ae ("spi: zynqmp: Add pm runtime support")
Cc: stable@vger.kernel.org	# 4.19
Cc: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
Link: https://lore.kernel.org/r/Message-Id: <20230622082435.7873-1-johan+linaro@kernel.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
[ itai: 5.10.198 carried this as commit 19f3d5d13b75, and 5.10.199
  reverted it in commit 649237ccf440 along with 2cdec9c13f81, the
  mislabelled backport of the previous patch, which this one depends on.
  Context - zynqmp_qspi_remove() in this tree already starts with
  spi_unregister_controller(), added by commit aeb00d7e89ca ("spi:
  zynqmp-gqspi: fix controller deregistration"); take the runtime PM
  reference before it, which is the order upstream ends up with ]
Assisted-by: LLM
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
 drivers/spi/spi-zynqmp-gqspi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 7223eee6db88..58b8c5c37f9a 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -1189,9 +1189,9 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 	return 0;
 
 clk_dis_all:
-	pm_runtime_put_sync(&pdev->dev);
-	pm_runtime_set_suspended(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
 	clk_disable_unprepare(xqspi->refclk);
 clk_dis_pclk:
 	clk_disable_unprepare(xqspi->pclk);
@@ -1213,13 +1213,17 @@ static int zynqmp_qspi_remove(struct platform_device *pdev)
 {
 	struct zynqmp_qspi *xqspi = platform_get_drvdata(pdev);
 
+	pm_runtime_get_sync(&pdev->dev);
+
 	spi_unregister_controller(xqspi->ctlr);
 
 	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, 0x0);
+
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
 	clk_disable_unprepare(xqspi->refclk);
 	clk_disable_unprepare(xqspi->pclk);
-	pm_runtime_set_suspended(&pdev->dev);
-	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
-- 
2.34.1

[PATCH 5.10.y v2 3/4] spi: zynqmp-gqspi: Undo runtime PM changes at driver exit time​

[Itai Handler]

From: Jinjie Ruan <ruanjinjie@huawei.com>

commit 2219576883e709737f3100aa9ded84976be49bd7 upstream.

It's important to undo pm_runtime_use_autosuspend() with
pm_runtime_dont_use_autosuspend() at driver exit time.

So, call pm_runtime_dont_use_autosuspend() at driver exit time
to fix it.

Fixes: 9e3a000362ae ("spi: zynqmp: Add pm runtime support")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Link: https://patch.msgid.link/20240920091135.2741574-1-ruanjinjie@huawei.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Assisted-by: LLM
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
 drivers/spi/spi-zynqmp-gqspi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 58b8c5c37f9a..167eb9072c3e 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -1190,6 +1190,7 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 
 clk_dis_all:
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 	clk_disable_unprepare(xqspi->refclk);
@@ -1220,6 +1221,7 @@ static int zynqmp_qspi_remove(struct platform_device *pdev)
 	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, 0x0);
 
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 	clk_disable_unprepare(xqspi->refclk);
-- 
2.34.1

[PATCH 5.10.y v2 4/4] spi: spi-zynqmp-gqspi: stop the controller on shutdown

[Itai Handler]

commit e922bad8b2d5028c51a096d083fea41cd0987154 upstream.

The driver has no ->shutdown, and platform_drv_shutdown() has no
fallback of its own.  Unlike pci_device_shutdown(), which clears bus
mastering when kexec_in_progress, nothing on the platform bus disarms a
device that can still write to memory.  The normal kexec path never
calls ->suspend either, so the quiesce in zynqmp_qspi_suspend() is not
reached.

A controller that is still executing a DMA read may therefore keep
writing to memory across a kexec.  QSPIDMA_DST_ADDR still points at
memory owned by the kernel that called kexec, DST_SIZE is non-zero and
the flash is still clocked, so data can keep landing in RAM while the
new kernel is being relocated, and after it has started executing.

That destination is a physical address which means nothing to the new
kernel, so the writes can corrupt whatever now occupies it: kernel text
or data, page tables, or the initrd.  Nothing reports an error and the
resulting behaviour is undefined.

This can be observed by reading GQSPI_EN (offset 0x114) and
QSPIDMA_DST_ADDR/SIZE/STS/CTRL (offsets 0x800 to 0x80c) early in the new
kernel, before the driver probes: without this patch GQSPI_EN reads 1
and QSPIDMA_DST_ADDR still points into the previous kernel's memory.

Add a ->shutdown that stops the controller the way zynqmp_qspi_suspend()
already does.  spi_controller_suspend() stops the queue, waits for a
message that is already executing and makes any later transfer fail with
-ESHUTDOWN, so nothing can be cut short by the register write that
follows.  It may sleep, which is fine here: device_shutdown() runs in
process context.  Unlike ->suspend this cannot abort on error, because a
controller left mastering the bus is worse than a truncated transfer, so
a failure to drain is only logged.

GQSPI_EN_OFST is then cleared, as zynqmp_qspi_remove() and
zynqmp_qspi_suspend() already do.  Skip that write only when
pm_runtime_get_if_in_use() returns 0, i.e. runtime suspended: the clocks
are gated, so the registers are unreachable and the controller cannot be
mastering the bus.  A negative return is not the same thing - it is what
the CONFIG_PM=n stub always returns, and there probe() has enabled pclk
and refclk for good, so the controller is running and must be stopped.

Fixes: dfe11a11d523 ("spi: Add support for Zynq Ultrascale+ MPSoC GQSPI controller")
Cc: stable@vger.kernel.org
Signed-off-by: Itai Handler <itai.handler@gmail.com>
Link: https://patch.msgid.link/20260910174832.873352-1-itai.handler@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
[ itai: context only - zynqmp_qspi_of_match[] still sits between
  zynqmp_qspi_remove() and MODULE_DEVICE_TABLE() in this tree, hoisted to
  the top of the file upstream by commit 29f4d95b97bc ("spi:
  spi-zynqmp-gqspi: Add tap delay support for GQSPI controller on Versal
  platform"), so zynqmp_qspi_shutdown() is placed above it ]
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
 drivers/spi/spi-zynqmp-gqspi.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 167eb9072c3e..840ba03b204a 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -1230,6 +1230,39 @@ static int zynqmp_qspi_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void zynqmp_qspi_shutdown(struct platform_device *pdev)
+{
+	struct zynqmp_qspi *xqspi = platform_get_drvdata(pdev);
+	int ret;
+
+	/*
+	 * Stop the queue and reject any later transfer first, so the write
+	 * below cannot cut into a message that is still being executed.
+	 * Unlike ->suspend this cannot abort on error: a controller left
+	 * mastering the bus is worse than a truncated transfer.
+	 */
+	ret = spi_controller_suspend(xqspi->ctlr);
+	if (ret)
+		dev_warn(&pdev->dev, "could not stop the queue: %d\n", ret);
+
+	/*
+	 * Only a runtime suspended controller can be left alone: its clocks
+	 * are gated, so it cannot be mastering the bus, and its registers
+	 * must not be accessed either.  Any other answer means it may be
+	 * running and has to be stopped.  In particular, on a kernel built
+	 * without runtime PM this returns -EINVAL, and there the clocks
+	 * enabled in probe() are never gated at all.
+	 */
+	ret = pm_runtime_get_if_in_use(&pdev->dev);
+	if (!ret)
+		return;
+
+	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, 0x0);
+
+	if (ret > 0)
+		pm_runtime_put_noidle(&pdev->dev);
+}
+
 static const struct of_device_id zynqmp_qspi_of_match[] = {
 	{ .compatible = "xlnx,zynqmp-qspi-1.0", },
 	{ /* End of table */ }
@@ -1240,6 +1273,7 @@ MODULE_DEVICE_TABLE(of, zynqmp_qspi_of_match);
 static struct platform_driver zynqmp_qspi_driver = {
 	.probe = zynqmp_qspi_probe,
 	.remove = zynqmp_qspi_remove,
+	.shutdown = zynqmp_qspi_shutdown,
 	.driver = {
 		.name = "zynqmp-qspi",
 		.of_match_table = zynqmp_qspi_of_match,
-- 
2.34.1

Re: [PATCH 5.10.y v2 0/4] spi: spi-zynqmp-gqspi: fix runtime PM, then stop on shutdown

[Sasha Levin]

> Marek, do you remember which platform it was and how it failed? If it
> is still around, could you give this series a try?

1/4 and 2/4 are the code that 5.10.199 reverted in 1ceaf0d3a883 ("Revert "spi:
spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe"") and
649237ccf440 ("Revert "spi: zynqmp-gqspi: fix clock imbalance on probe
failure""), and nobody found out what broke.

Marek, Michal: can either of you test or ack v2? I'll queue it once one of you
has.

-- 
Thanks,
Sasha