[PATCH 0/3] spi: rockchip: ISR fix and minor cleanups

[PATCH 0/3] spi: rockchip: ISR fix and minor cleanups

[John Madieu]

Hi all,
 
This series fixes one real bug in the Rockchip SPI driver and tidies up
two unrelated cosmetic issues spotted while looking at the same area.
The patches are independent and could be applied in any order; they are
ordered here by decreasing severity.

John Madieu (3):
  spi: rockchip: Read ISR, not IMR, to detect cs-inactive IRQ
  spi: rockchip: Drop unused and broken CR0 macros
  spi: rockchip: Drop dead zero-check on fifo_len

 drivers/spi/spi-rockchip.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/3] spi: rockchip: Read ISR, not IMR, to detect cs-inactive IRQ

[John Madieu]

rockchip_spi_isr() decides whether the current interrupt was the
cs-inactive event by reading IMR:

	if (rs->cs_inactive &&
	    readl_relaxed(rs->regs + ROCKCHIP_SPI_IMR) & INT_CS_INACTIVE)
		ctlr->target_abort(ctlr);

IMR is the interrupt mask register: it tells which sources are enabled,
not which one fired. In the PIO path, rockchip_spi_prepare_irq() enables
both INT_RF_FULL and INT_CS_INACTIVE in IMR when rs->cs_inactive is true:

	if (rs->cs_inactive)
		writel_relaxed(INT_RF_FULL | INT_CS_INACTIVE,
			       rs->regs + ROCKCHIP_SPI_IMR);

so the IMR check is always true once cs_inactive is enabled, and every
PIO interrupt - including normal RF_FULL completions - is dispatched to
ctlr->target_abort(), aborting the transfer. The bug is reachable on
ROCKCHIP_SPI_VER2_TYPE2 in target mode with a DMA-capable controller
when the transfer is short enough to fall back to PIO
(rockchip_spi_can_dma() returns false below fifo_len).

Read ISR (which is RISR masked by IMR) so the check actually reflects
which interrupt fired, and parenthesise the expression for clarity while
at it.

Fixes: 869f2c94db92 ("spi: rockchip: Stop spi slave dma receiver when cs inactive")
Signed-off-by: John Madieu <john.madieu@gmail.com>
---
 drivers/spi/spi-rockchip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 14cd1b9d9793..de39f5da62cb 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -357,7 +357,8 @@ static irqreturn_t rockchip_spi_isr(int irq, void *dev_id)
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
 	/* When int_cs_inactive comes, spi target abort */
-	if (rs->cs_inactive && readl_relaxed(rs->regs + ROCKCHIP_SPI_IMR) & INT_CS_INACTIVE) {
+	if (rs->cs_inactive &&
+	    (readl_relaxed(rs->regs + ROCKCHIP_SPI_ISR) & INT_CS_INACTIVE)) {
 		ctlr->target_abort(ctlr);
 		writel_relaxed(0, rs->regs + ROCKCHIP_SPI_IMR);
 		writel_relaxed(0xffffffff, rs->regs + ROCKCHIP_SPI_ICR);
-- 
2.25.1

[PATCH 2/3] spi: rockchip: Drop unused and broken CR0 macros

[John Madieu]

Two CTRLR0 macros are defined but never referenced, and both are wrong:

 - CR0_XFM_MASK shifts by SPI_XFM_OFFSET, which does not exist anywhere
   in the tree. The intended symbol is CR0_XFM_OFFSET.

 - CR0_MTM_OFFSET is defined as 0x21, i.e. bit 33 of a 32-bit register.
   The value is meaningless and the macro is unused.

Drop both. They can be re-introduced correctly when an actual user
appears.

Signed-off-by: John Madieu <john.madieu@gmail.com>
---
 drivers/spi/spi-rockchip.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index de39f5da62cb..231fbcf0e7aa 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -98,7 +98,6 @@
 #define CR0_FRF_MICROWIRE			0x2
 
 #define CR0_XFM_OFFSET				18
-#define CR0_XFM_MASK				(0x03 << SPI_XFM_OFFSET)
 #define CR0_XFM_TR					0x0
 #define CR0_XFM_TO					0x1
 #define CR0_XFM_RO					0x2
@@ -109,8 +108,6 @@
 
 #define CR0_SOI_OFFSET				23
 
-#define CR0_MTM_OFFSET				0x21
-
 /* Bit fields in SER, 2bit */
 #define SER_MASK					0x3
 
-- 
2.25.1

[PATCH 3/3] spi: rockchip: Drop dead zero-check on fifo_len

[John Madieu]

rs->fifo_len is assigned from get_fifo_len(), which returns 64 for the
two known SPI controller versions and 32 for everything else - never 0.
The subsequent

	if (!rs->fifo_len)
		return dev_err_probe(...);

is therefore unreachable.

Drop the check. If unknown controller versions ever need to fail probe
explicitly, that should be expressed in get_fifo_len() itself, not
through an impossible post-condition.

Signed-off-by: John Madieu <john.madieu@gmail.com>
---
 drivers/spi/spi-rockchip.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 231fbcf0e7aa..1bd48376498a 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -824,8 +824,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	}
 
 	rs->fifo_len = get_fifo_len(rs);
-	if (!rs->fifo_len)
-		return dev_err_probe(&pdev->dev, -EINVAL, "Failed to get fifo length\n");
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, ROCKCHIP_AUTOSUSPEND_TIMEOUT);
 	pm_runtime_use_autosuspend(&pdev->dev);
-- 
2.25.1

Re: [PATCH 3/3] spi: rockchip: Drop dead zero-check on fifo_len

[Mark Brown]

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

On Sat, Apr 25, 2026 at 09:29:36AM +0000, John Madieu wrote:
> rs->fifo_len is assigned from get_fifo_len(), which returns 64 for the
> two known SPI controller versions and 32 for everything else - never 0.
> The subsequent

> 	if (!rs->fifo_len)
> 		return dev_err_probe(...);

> is therefore unreachable.

> Drop the check. If unknown controller versions ever need to fail probe
> explicitly, that should be expressed in get_fifo_len() itself, not
> through an impossible post-condition.

That looks like it's intended as a "you added a new hardware type and
forgot to fill in this field" type check intended to never fire in
production?

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

Re: [PATCH 3/3] spi: rockchip: Drop dead zero-check on fifo_len

[Mark Brown]

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

On Sun, Apr 26, 2026 at 10:57:31PM +0200, john madieu wrote:
> Le dim. 26 avr. 2026 à 22:36, Mark Brown <broonie@kernel.org> a écrit :

> > That looks like it's intended as a "you added a new hardware type and
> > forgot to fill in this field" type check intended to never fire in
> > production?

> Yes, that's a fair reading. I missed that angle.

> Should I drop this patch and resend the series (v2) with just 1/3 and 2/3 ?

No need to resend.

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