[PATCH v2 0/2] spi: support devices with no wire in one direction

[PATCH v2 0/2] spi: support devices with no wire in one direction

[Cole Munz]

The Flipper One display bus has no MISO wire. That pin is the
end-of-frame GPIO. The device tree says so with spi-rx-bus-width = <0>,
but the uclass dropped the value and the Rockchip driver clocked the
receive FIFO anyway, discarding every byte. Reported at
https://github.com/flipperdevices/u-boot/issues/33 from the boot log
there; Alexey has since confirmed the fix on the hardware.

Patch 1 maps bus width 0 to SPI_NO_TX/SPI_NO_RX, validates transfers
against the bits in dm_spi_xfer() and adds a sandbox test. Patch 2 has
the Rockchip driver pick its transfer mode from those bits, so the
unused FIFO stays out of the transfer.

Both went out on their own before:

https://lore.kernel.org/all/10e92a74c8a22cc2cd54f5dcccc31594016d4263.1787015208.git.Munzzyy1@proton.me/
https://lore.kernel.org/all/69c35eba7ee1a353107459b8f8032e92506a3fb1.1787045568.git.Munzzyy1@proton.me/

Rolled into one series now, since patch 1 has no user by itself. The
rk_spi mail had also gone out as a reply into the uclass thread, which
is what made b4 read it as a v2 of the prerequisite. Clean thread this
time. Sorry for the noise.

Changes in v2:
- validate NO_RX/NO_TX centrally in dm_spi_xfer() and return -EINVAL,
  matching Linux __spi_validate(), with a sandbox test for both
  rejections (suggested by Quentin)
- reworded the transfer mode comment in rk_spi.c claim_bus (Quentin)
- collected Alexey's Tested-by on patch 2 (Flipper One, warning gone,
  transmit-only display still works)
- added the maintainers get_maintainer.pl lists for the touched files
- one series with a cover letter instead of two stray patches

Cole Munz (2):
  spi: Handle spi-{tx,rx}-bus-width 0 as SPI_NO_TX/SPI_NO_RX
  spi: rockchip: skip the unused FIFO direction on a one-wire device

 drivers/spi/rk_spi.c     | 27 ++++++++++++++++++++++----
 drivers/spi/spi-uclass.c | 16 ++++++++++++++++
 include/spi.h            |  2 ++
 test/dm/spi.c            | 41 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 4 deletions(-)


base-commit: 527115ef6783cec49e5610c523c124b399011361
-- 
2.55.0

[PATCH v2 1/2] spi: Handle spi-{tx, rx}-bus-width 0 as SPI_NO_TX/SPI_NO_RX

[Cole Munz]

The spi-peripheral-props binding shipped in dts/upstream allows a bus
width of 0, meaning no RX or TX is possible on this device. The
switches in spi_slave_of_to_plat() only handle 1/2/4/8, so a width of
0 falls through to the default case and warns "spi-rx-bus-width 0 not
supported" on every boot, even though the devicetree is valid per the
binding. The fact that the wire is missing is then dropped from
plat->mode.

Map 0 to new SPI_NO_TX/SPI_NO_RX mode bits, as Linux has done since
v5.12 ("spi: Add SPI_NO_TX/RX support", mainline d962608ce218).
Bits 16 and 17 are the first free mode bits.

Mapping the bits is not enough on its own, as Quentin pointed out:
nothing would stop a caller from asking for a transfer in a direction
that has no wire, and every controller driver would need its own
guard. Validate centrally in dm_spi_xfer() instead, matching Linux's
__spi_validate(): a din on a SPI_NO_RX device or a dout on a SPI_NO_TX
device fails with -EINVAL before it reaches the driver. A new sandbox
test covers both rejections and the accepted case.

This comes up on devices with no MISO line at all, such as a
write-only SPI display described with spi-rx-bus-width = <0>.

Signed-off-by: Cole Munz <Munzzyy1@proton.me>
---
Changes in v2: added the dm_spi_xfer() validation and the sandbox
test. Rerun on this branch:

  $ ./u-boot -T -c "ut dm dm_test_spi_xfer_no_rx_tx"   failures: 0
  (spi_xfer, spi_flash, spi_find, spi_claim_bus, spi_set_wordlen
  also all failures: 0)

With the dm_spi_xfer() hunk reverted the new test fails both -EINVAL
assertions, so it does test the right thing.

checkpatch flags the #if CONFIG_IS_ENABLED(DM_SPI_FLASH) guard and the
DM_TEST-after-brace placement in the test; both copy the idiom the
existing tests in test/dm/spi.c use, so I kept them consistent.

 drivers/spi/spi-uclass.c | 16 ++++++++++++++++
 include/spi.h            |  2 ++
 test/dm/spi.c            | 41 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index 120565df1497..dd1843ffac19 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -105,6 +105,7 @@ int dm_spi_set_wordlen(struct udevice *dev, unsigned int wordlen)
 int dm_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		const void *dout, void *din, unsigned long flags)
 {
+	struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
 	struct udevice *bus = dev->parent;
 	struct dm_spi_ops *ops = spi_get_ops(bus);
 
@@ -113,6 +114,15 @@ int dm_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	if (!ops->xfer)
 		return -ENOSYS;
 
+	/*
+	 * A device with no wire in one direction cannot transfer in it,
+	 * so reject the request here rather than in every driver.
+	 */
+	if (din && (slave_plat->mode & SPI_NO_RX))
+		return -EINVAL;
+	if (dout && (slave_plat->mode & SPI_NO_TX))
+		return -EINVAL;
+
 	return ops->xfer(dev, bitlen, dout, din, flags);
 }
 
@@ -229,6 +239,9 @@ static int spi_child_post_bind(struct udevice *dev)
 	/* Device DUAL/QUAD mode */
 	value = dev_read_u32_default(dev, "spi-tx-bus-width", 1);
 	switch (value) {
+	case 0:
+		mode |= SPI_NO_TX;
+		break;
 	case 1:
 		break;
 	case 2:
@@ -247,6 +260,9 @@ static int spi_child_post_bind(struct udevice *dev)
 
 	value = dev_read_u32_default(dev, "spi-rx-bus-width", 1);
 	switch (value) {
+	case 0:
+		mode |= SPI_NO_RX;
+		break;
 	case 1:
 		break;
 	case 2:
diff --git a/include/spi.h b/include/spi.h
index 97096a775262..f477763bdc60 100644
--- a/include/spi.h
+++ b/include/spi.h
@@ -34,6 +34,8 @@ struct spinand_info;
 #define SPI_RX_QUAD	BIT(13)			/* receive with 4 wires */
 #define SPI_TX_OCTAL	BIT(14)			/* transmit with 8 wires */
 #define SPI_RX_OCTAL	BIT(15)			/* receive with 8 wires */
+#define SPI_NO_TX	BIT(16)			/* no transmit wire */
+#define SPI_NO_RX	BIT(17)			/* no receive wire */
 
 /* Header byte that marks the start of the message */
 #define SPI_PREAMBLE_END_BYTE	0xec
diff --git a/test/dm/spi.c b/test/dm/spi.c
index a89ba06274fc..cb8f88713a9e 100644
--- a/test/dm/spi.c
+++ b/test/dm/spi.c
@@ -216,3 +216,44 @@ static int dm_test_spi_xfer(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_spi_xfer, UTF_SCAN_PDATA | UTF_SCAN_FDT);
+
+/* Test that a transfer is rejected when the device has no wire for it */
+static int dm_test_spi_xfer_no_rx_tx(struct unit_test_state *uts)
+{
+	struct dm_spi_slave_plat *plat;
+	struct spi_slave *slave;
+	struct udevice *bus;
+	const int busnum = 0, cs = 0;
+	const char dout[5] = {0x9f};
+	unsigned char din[5];
+	uint saved_mode;
+
+	ut_assertok(spi_get_bus_and_cs(busnum, cs, &bus, &slave));
+	ut_assertok(spi_claim_bus(slave));
+	plat = dev_get_parent_plat(slave->dev);
+	saved_mode = plat->mode;
+
+	plat->mode |= SPI_NO_RX;
+	ut_asserteq(-EINVAL, spi_xfer(slave, 40, dout, din,
+				      SPI_XFER_BEGIN | SPI_XFER_END));
+
+	plat->mode = saved_mode | SPI_NO_TX;
+	ut_asserteq(-EINVAL, spi_xfer(slave, 40, dout, din,
+				      SPI_XFER_BEGIN | SPI_XFER_END));
+
+	plat->mode = saved_mode;
+	ut_assertok(spi_xfer(slave, 40, dout, din,
+			     SPI_XFER_BEGIN | SPI_XFER_END));
+	spi_release_bus(slave);
+
+	/*
+	 * Since we are about to destroy all devices, we must tell sandbox
+	 * to forget the emulation device
+	 */
+#if CONFIG_IS_ENABLED(DM_SPI_FLASH)
+	sandbox_sf_unbind_emul(state_get_current(), busnum, cs);
+#endif
+
+	return 0;
+}
+DM_TEST(dm_test_spi_xfer_no_rx_tx, UTF_SCAN_PDATA | UTF_SCAN_FDT);
-- 
2.55.0

[PATCH v2 2/2] spi: rockchip: skip the unused FIFO direction on a one-wire device

[Cole Munz]

The controller has a transfer-mode field that can run transmit-only or
receive-only instead of both, which leaves the unused FIFO out of the
transfer entirely. The driver never used it for that: claim_bus always
programmed TMOD_TR, and the only other mode came from an opportunistic
switch to TMOD_RO for read-only transfers.

A device described with spi-{tx,rx}-bus-width = <0> has no wire in that
direction at all, so now that the width reaches plat->mode as
SPI_NO_TX/SPI_NO_RX, pick the transfer mode from it. A write-only
display stops clocking receive bytes nobody reads.

The transmit-only case needs one more change. The 8-bit loop paces
itself on the receive FIFO and sets toread unconditionally, so with no
receive path it would wait on a FIFO that stays empty forever. Leave
toread at zero there and let the existing wait_till_not_busy() at the
end of the chunk handle completion, which is the same thing that
already covers a transmit component today.

The restore at the end of a read-only transfer went back to a hardcoded
TMOD_TR, which would undo the device's own mode. Restore what the mode
asks for instead.

Signed-off-by: Cole Munz <Munzzyy1@proton.me>
Tested-by: Alexey Charkov <alchark@flipper.net>
---
Changes in v2: reworded the transfer mode comment in claim_bus, picked
up the Tested-by. No functional change since v1; compile check rerun:

  $ make jaguar-rk3588_defconfig
  $ make CROSS_COMPILE=aarch64-linux-gnu- drivers/spi/rk_spi.o
  CC      drivers/spi/rk_spi.o     (exit 0)

 drivers/spi/rk_spi.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/rk_spi.c b/drivers/spi/rk_spi.c
index 2c3d70ba7159..81785b5e949f 100644
--- a/drivers/spi/rk_spi.c
+++ b/drivers/spi/rk_spi.c
@@ -283,6 +283,20 @@ static int rockchip_spi_probe(struct udevice *bus)
 	return 0;
 }
 
+/*
+ * A device that declares spi-{tx,rx}-bus-width = <0> has no wire in that
+ * direction, so the controller can drop the matching FIFO entirely instead
+ * of clocking bytes nobody reads.
+ */
+static u32 rkspi_base_tmod(struct rockchip_spi_priv *priv)
+{
+	if (priv->mode & SPI_NO_RX)
+		return TMOD_TO;
+	if (priv->mode & SPI_NO_TX)
+		return TMOD_RO;
+	return TMOD_TR;
+}
+
 static int rockchip_spi_claim_bus(struct udevice *dev)
 {
 	struct udevice *bus = dev->parent;
@@ -329,8 +343,8 @@ static int rockchip_spi_claim_bus(struct udevice *dev)
 	/* Frame Format */
 	ctrlr0 |= FRF_SPI << FRF_SHIFT;
 
-	/* Tx and Rx mode */
-	ctrlr0 |= TMOD_TR << TMOD_SHIFT;
+	/* Configure RX/TX mode */
+	ctrlr0 |= rkspi_base_tmod(priv) << TMOD_SHIFT;
 
 	writel(ctrlr0, &regs->ctrlr0);
 
@@ -472,7 +486,12 @@ static int rockchip_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		writel(todo - 1, &regs->ctrlr1);
 		rkspi_enable_chip(regs, true);
 
-		toread = todo;
+		/*
+		 * In transmit-only mode the RX FIFO never fills, so waiting
+		 * on it would hang. Completion is handled by the
+		 * wait_till_not_busy() below instead.
+		 */
+		toread = (priv->mode & SPI_NO_RX) ? 0 : todo;
 		/* Only write if we have something to write */
 		towrite = out ? todo : 0;
 		while (toread || towrite) {
@@ -513,7 +532,7 @@ static int rockchip_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	if (!out)
 		clrsetbits_le32(&regs->ctrlr0,
 				TMOD_MASK << TMOD_SHIFT,
-				TMOD_TR << TMOD_SHIFT);
+				rkspi_base_tmod(priv) << TMOD_SHIFT);
 
 	return ret;
 }
-- 
2.55.0

Re: [PATCH v2 1/2] spi: Handle spi-{tx,rx}-bus-width 0 as SPI_NO_TX/SPI_NO_RX

[Quentin Schulz]

Hi Cole,

On 8/20/26 5:26 PM, Cole Munz wrote:
> The spi-peripheral-props binding shipped in dts/upstream allows a bus
> width of 0, meaning no RX or TX is possible on this device. The
> switches in spi_slave_of_to_plat() only handle 1/2/4/8, so a width of
> 0 falls through to the default case and warns "spi-rx-bus-width 0 not
> supported" on every boot, even though the devicetree is valid per the
> binding. The fact that the wire is missing is then dropped from
> plat->mode.
> 
> Map 0 to new SPI_NO_TX/SPI_NO_RX mode bits, as Linux has done since
> v5.12 ("spi: Add SPI_NO_TX/RX support", mainline d962608ce218).
> Bits 16 and 17 are the first free mode bits.
> 
> Mapping the bits is not enough on its own, as Quentin pointed out:
> nothing would stop a caller from asking for a transfer in a direction
> that has no wire, and every controller driver would need its own
> guard. Validate centrally in dm_spi_xfer() instead, matching Linux's
> __spi_validate(): a din on a SPI_NO_RX device or a dout on a SPI_NO_TX
> device fails with -EINVAL before it reaches the driver. A new sandbox
> test covers both rejections and the accepted case.
> 
> This comes up on devices with no MISO line at all, such as a
> write-only SPI display described with spi-rx-bus-width = <0>.
> 
> Signed-off-by: Cole Munz <Munzzyy1@proton.me>
> ---
> Changes in v2: added the dm_spi_xfer() validation and the sandbox
> test. Rerun on this branch:
> 
>    $ ./u-boot -T -c "ut dm dm_test_spi_xfer_no_rx_tx"   failures: 0
>    (spi_xfer, spi_flash, spi_find, spi_claim_bus, spi_set_wordlen
>    also all failures: 0)
> 
> With the dm_spi_xfer() hunk reverted the new test fails both -EINVAL
> assertions, so it does test the right thing.
> 
> checkpatch flags the #if CONFIG_IS_ENABLED(DM_SPI_FLASH) guard and the
> DM_TEST-after-brace placement in the test; both copy the idiom the
> existing tests in test/dm/spi.c use, so I kept them consistent.

Agreed.

> 
>   drivers/spi/spi-uclass.c | 16 ++++++++++++++++
>   include/spi.h            |  2 ++
>   test/dm/spi.c            | 41 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 59 insertions(+)
> 
> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> index 120565df1497..dd1843ffac19 100644
> --- a/drivers/spi/spi-uclass.c
> +++ b/drivers/spi/spi-uclass.c
> @@ -105,6 +105,7 @@ int dm_spi_set_wordlen(struct udevice *dev, unsigned int wordlen)
>   int dm_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   		const void *dout, void *din, unsigned long flags)
>   {
> +	struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
>   	struct udevice *bus = dev->parent;
>   	struct dm_spi_ops *ops = spi_get_ops(bus);
>   
> @@ -113,6 +114,15 @@ int dm_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   	if (!ops->xfer)
>   		return -ENOSYS;
>   
> +	/*
> +	 * A device with no wire in one direction cannot transfer in it,
> +	 * so reject the request here rather than in every driver.
> +	 */
> +	if (din && (slave_plat->mode & SPI_NO_RX))
> +		return -EINVAL;
> +	if (dout && (slave_plat->mode & SPI_NO_TX))
> +		return -EINVAL;
> +
>   	return ops->xfer(dev, bitlen, dout, din, flags);
>   }
>   
> @@ -229,6 +239,9 @@ static int spi_child_post_bind(struct udevice *dev)
>   	/* Device DUAL/QUAD mode */
>   	value = dev_read_u32_default(dev, "spi-tx-bus-width", 1);
>   	switch (value) {
> +	case 0:
> +		mode |= SPI_NO_TX;
> +		break;
>   	case 1:
>   		break;
>   	case 2:
> @@ -247,6 +260,9 @@ static int spi_child_post_bind(struct udevice *dev)
>   
>   	value = dev_read_u32_default(dev, "spi-rx-bus-width", 1);
>   	switch (value) {
> +	case 0:
> +		mode |= SPI_NO_RX;
> +		break;
>   	case 1:
>   		break;
>   	case 2:
> diff --git a/include/spi.h b/include/spi.h
> index 97096a775262..f477763bdc60 100644
> --- a/include/spi.h
> +++ b/include/spi.h
> @@ -34,6 +34,8 @@ struct spinand_info;
>   #define SPI_RX_QUAD	BIT(13)			/* receive with 4 wires */
>   #define SPI_TX_OCTAL	BIT(14)			/* transmit with 8 wires */
>   #define SPI_RX_OCTAL	BIT(15)			/* receive with 8 wires */
> +#define SPI_NO_TX	BIT(16)			/* no transmit wire */
> +#define SPI_NO_RX	BIT(17)			/* no receive wire */
>   
>   /* Header byte that marks the start of the message */
>   #define SPI_PREAMBLE_END_BYTE	0xec
> diff --git a/test/dm/spi.c b/test/dm/spi.c
> index a89ba06274fc..cb8f88713a9e 100644
> --- a/test/dm/spi.c
> +++ b/test/dm/spi.c
> @@ -216,3 +216,44 @@ static int dm_test_spi_xfer(struct unit_test_state *uts)
>   	return 0;
>   }
>   DM_TEST(dm_test_spi_xfer, UTF_SCAN_PDATA | UTF_SCAN_FDT);
> +
> +/* Test that a transfer is rejected when the device has no wire for it */
> +static int dm_test_spi_xfer_no_rx_tx(struct unit_test_state *uts)
> +{
> +	struct dm_spi_slave_plat *plat;
> +	struct spi_slave *slave;
> +	struct udevice *bus;
> +	const int busnum = 0, cs = 0;
> +	const char dout[5] = {0x9f};
> +	unsigned char din[5];
> +	uint saved_mode;
> +
> +	ut_assertok(spi_get_bus_and_cs(busnum, cs, &bus, &slave));
> +	ut_assertok(spi_claim_bus(slave));
> +	plat = dev_get_parent_plat(slave->dev);
> +	saved_mode = plat->mode;
> +
> +	plat->mode |= SPI_NO_RX;
> +	ut_asserteq(-EINVAL, spi_xfer(slave, 40, dout, din,
> +				      SPI_XFER_BEGIN | SPI_XFER_END));
> +
> +	plat->mode = saved_mode | SPI_NO_TX;
> +	ut_asserteq(-EINVAL, spi_xfer(slave, 40, dout, din,
> +				      SPI_XFER_BEGIN | SPI_XFER_END));
> +
> +	plat->mode = saved_mode;
> +	ut_assertok(spi_xfer(slave, 40, dout, din,
> +			     SPI_XFER_BEGIN | SPI_XFER_END));

Please remove the happy path, it's already tested in dm_test_spi_xfer().

I'm wondering if we cannot simply merge the unhappy paths into 
dm_test_spi_xfer() and have only one test. No strong opinion here.

This looks ok to me, with either of the above done, so:

Acked-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v2 2/2] spi: rockchip: skip the unused FIFO direction on a one-wire device

[Quentin Schulz]

Hi Cole,

On 8/20/26 5:26 PM, Cole Munz wrote:
> The controller has a transfer-mode field that can run transmit-only or
> receive-only instead of both, which leaves the unused FIFO out of the
> transfer entirely. The driver never used it for that: claim_bus always
> programmed TMOD_TR, and the only other mode came from an opportunistic
> switch to TMOD_RO for read-only transfers.
> 
> A device described with spi-{tx,rx}-bus-width = <0> has no wire in that
> direction at all, so now that the width reaches plat->mode as
> SPI_NO_TX/SPI_NO_RX, pick the transfer mode from it. A write-only
> display stops clocking receive bytes nobody reads.
> 
> The transmit-only case needs one more change. The 8-bit loop paces
> itself on the receive FIFO and sets toread unconditionally, so with no
> receive path it would wait on a FIFO that stays empty forever. Leave
> toread at zero there and let the existing wait_till_not_busy() at the
> end of the chunk handle completion, which is the same thing that
> already covers a transmit component today.
> 
> The restore at the end of a read-only transfer went back to a hardcoded
> TMOD_TR, which would undo the device's own mode. Restore what the mode
> asks for instead.
> 
> Signed-off-by: Cole Munz <Munzzyy1@proton.me>
> Tested-by: Alexey Charkov <alchark@flipper.net>
> ---
> Changes in v2: reworded the transfer mode comment in claim_bus, picked
> up the Tested-by. No functional change since v1; compile check rerun:
> 
>    $ make jaguar-rk3588_defconfig
>    $ make CROSS_COMPILE=aarch64-linux-gnu- drivers/spi/rk_spi.o
>    CC      drivers/spi/rk_spi.o     (exit 0)
> 
>   drivers/spi/rk_spi.c | 27 +++++++++++++++++++++++----
>   1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/rk_spi.c b/drivers/spi/rk_spi.c
> index 2c3d70ba7159..81785b5e949f 100644
> --- a/drivers/spi/rk_spi.c
> +++ b/drivers/spi/rk_spi.c
> @@ -283,6 +283,20 @@ static int rockchip_spi_probe(struct udevice *bus)
>   	return 0;
>   }
>   
> +/*
> + * A device that declares spi-{tx,rx}-bus-width = <0> has no wire in that
> + * direction, so the controller can drop the matching FIFO entirely instead
> + * of clocking bytes nobody reads.
> + */
> +static u32 rkspi_base_tmod(struct rockchip_spi_priv *priv)
> +{
> +	if (priv->mode & SPI_NO_RX)
> +		return TMOD_TO;
> +	if (priv->mode & SPI_NO_TX)
> +		return TMOD_RO;
> +	return TMOD_TR;
> +}
> +
>   static int rockchip_spi_claim_bus(struct udevice *dev)
>   {
>   	struct udevice *bus = dev->parent;
> @@ -329,8 +343,8 @@ static int rockchip_spi_claim_bus(struct udevice *dev)
>   	/* Frame Format */
>   	ctrlr0 |= FRF_SPI << FRF_SHIFT;
>   
> -	/* Tx and Rx mode */
> -	ctrlr0 |= TMOD_TR << TMOD_SHIFT;
> +	/* Configure RX/TX mode */
> +	ctrlr0 |= rkspi_base_tmod(priv) << TMOD_SHIFT;
>   
>   	writel(ctrlr0, &regs->ctrlr0);
>   
> @@ -472,7 +486,12 @@ static int rockchip_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   		writel(todo - 1, &regs->ctrlr1);
>   		rkspi_enable_chip(regs, true);
>   
> -		toread = todo;
> +		/*
> +		 * In transmit-only mode the RX FIFO never fills, so waiting
> +		 * on it would hang. Completion is handled by the
> +		 * wait_till_not_busy() below instead.
> +		 */

I got confused by the wording here. Can I suggest:

/* When RX wire is not routed, the RX FIFO can never fill, so waiting on 
it would hang. */

I don't understand the context for the second sentence though, we are 
always waiting until not busy, if there's something to transmit, it 
doesn't have anything to do with the RX path does it? What am I missing 
here?

Cheers,
Quentin

Re: [PATCH v2 2/2] spi: rockchip: skip the unused FIFO direction on a one-wire device

[Cole Munz]

Hi Quentin,

On 8/20/26 5:57 PM, Quentin Schulz wrote:
> I got confused by the wording here. Can I suggest:
>
> /* When RX wire is not routed, the RX FIFO can never fill, so waiting on
> it would hang. */
>
> I don't understand the context for the second sentence though, we are
> always waiting until not busy, if there's something to transmit, it
> doesn't have anything to do with the RX path does it? What am I missing
> here?

The RX drain was doing double duty in TMOD_TR. Byte N only shows up in
the RX FIFO after byte N has gone out on the wire. Draining toread
bytes is therefore also what made the loop wait for the transfer
itself. With toread forced to zero the loop exits as soon as the last
byte lands in the TX FIFO, which says nothing about the wire. From
there the wait_till_not_busy() below is the only wait left. That is
what the sentence tried to point at.

You're right that it explained none of that. v3 takes your first
sentence and adds the pacing part:

	When the RX wire is not routed, the RX FIFO never fills,
	so waiting on it would hang. Draining it was also what
	paced this loop against the wire, so the transmit-only
	path relies on the wait_till_not_busy() below for
	completion instead.

The test trim from your other mail is in v3 too. Thanks for both
reviews.

Cheers,
Cole

Re: [PATCH v2 2/2] spi: rockchip: skip the unused FIFO direction on a one-wire device

[Quentin Schulz]

Hi Cole,

On 8/20/26 7:00 PM, Cole Munz wrote:
> Hi Quentin,
> 
> On 8/20/26 5:57 PM, Quentin Schulz wrote:
>> I got confused by the wording here. Can I suggest:
>>
>> /* When RX wire is not routed, the RX FIFO can never fill, so waiting on
>> it would hang. */
>>
>> I don't understand the context for the second sentence though, we are
>> always waiting until not busy, if there's something to transmit, it
>> doesn't have anything to do with the RX path does it? What am I missing
>> here?
> 
> The RX drain was doing double duty in TMOD_TR. Byte N only shows up in
> the RX FIFO after byte N has gone out on the wire. Draining toread

Are you sure? The controller needs to tell the peripheral which data 
it's interested in. Therefore, the peripheral cannot send data to the 
controller until it knows what it wants? c.f. 
https://learn.sparkfun.com/tutorials/serial-peripheral-interface-spi/all 
in the Receiving data section. Concurrent TX and RX may happen but it's 
not a given (and in any case, you'd have a first PICO transfer to 
specify what to read next on the POCI line).

At a logic level, the current while loop may very well write all of 
towrite without a single toread being read (due to the RX FIFO being 
empty in the controller) and then go through all toread, or it could 
write 8b and read 8b at a time (or a mixed number of 8b write/read, 
though I don't think this is necessarily possible).

> bytes is therefore also what made the loop wait for the transfer
> itself. With toread forced to zero the loop exits as soon as the last
> byte lands in the TX FIFO, which says nothing about the wire. From
> there the wait_till_not_busy() below is the only wait left. That is
> what the sentence tried to point at.
> 
> You're right that it explained none of that. v3 takes your first
> sentence and adds the pacing part:
> 
> 	When the RX wire is not routed, the RX FIFO never fills,
> 	so waiting on it would hang. Draining it was also what
> 	paced this loop against the wire, so the transmit-only

The controller has 64x 16b-wide RX and TX FIFOs, so my reading of the 
TRM means that you can have up to 64 16b data pending in RX and/or TX 
FIFOs. So I'm not sure "pacing" is the correct term here. If we're doing 
full-duplex (which depends on the device and how we write the driver 
since we need to write once in the TX FIFO before doing full-duplex and 
both reading and writing at the same time), then I think it's pretty 
much guaranteed we're sending and reading bits on PICO/POCI at the same 
clock edge. If we aren't doing full-duplex, then we may have to wait to 
read from the RX FIFO after we've actually sent stuff on the wire from 
the TX FIFO. In conclusion, the wait_till_not_busy() is actually only 
useful if we're doing a tx-only transfer as writing to the TX FIFO 
doesn't mean it's sent over the wire. If we agree on this, then I think 
it's more appropriate to reword the comment just before 
rkspi_wait_till_not_busy() to specify we wait till the TX FIFO has been 
sent over the wire before starting a new transfer as disabling the 
controller will clear all FIFOs.

Cheers,
Quentin

Re: [PATCH v2 2/2] spi: rockchip: skip the unused FIFO direction on a one-wire device

[Cole Munz]

Hi Quentin,

On 8/21/26 12:12 PM, Quentin Schulz wrote:
> Are you sure? The controller needs to tell the peripheral which data
> it's interested in. Therefore, the peripheral cannot send data to the
> controller until it knows what it wants?

At the protocol level you're right. The peripheral has nothing useful
to say before it sees a command. At the controller level TMOD_TR still
shifts POCI into the RX FIFO on every frame no matter what the
peripheral drives. The existing loop depends on that: toread counts
down todo bytes even for write-only transfers where in is NULL and
every byte is discarded. If the RX FIFO stayed empty there the loop
would hang today.

> The controller has 64x 16b-wide RX and TX FIFOs, so my reading of the
> TRM means that you can have up to 64 16b data pending in RX and/or TX
> FIFOs. So I'm not sure "pacing" is the correct term here.

Fair, pacing was the wrong word. Writes run ahead into the FIFO and
nothing slows them down. The RX drain gates the loop exit instead. The
last RX byte only lands once the last frame has clocked, so in TMOD_TR
the loop cannot exit before the wire is done. In TMOD_TO that gate is
gone and wait_till_not_busy() is all that remains. Same conclusion as
yours.

> If we agree on this, then I think
> it's more appropriate to reword the comment just before
> rkspi_wait_till_not_busy() to specify we wait till the TX FIFO has been
> sent over the wire before starting a new transfer as disabling the
> controller will clear all FIFOs.

Agreed. v4 shrinks the toread comment to your one sentence and rewords
the one above rkspi_wait_till_not_busy() to say the TX FIFO can still
hold unsent data and disabling the controller clears the FIFOs. Thanks
for digging into the TRM.

Cheers,
Cole