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

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

[Cole Munz]

The Flipper One display bus has no MISO wire; the pin is the
end-of-frame GPIO. Patch 1 maps spi-{tx,rx}-bus-width = <0> to
SPI_NO_TX/SPI_NO_RX and validates transfers against the bits in
dm_spi_xfer(). Patch 2 has the Rockchip driver pick its transfer mode
from them so the unused FIFO stays out of the transfer. Background and
history are in the v2 cover letter:

https://lore.kernel.org/all/cover.1787238071.git.Munzzyy1@proton.me/

Changes in v3:
- dropped the happy path assertion from the new sandbox test, it is
  already covered by dm_test_spi_xfer (Quentin)
- collected Quentin's Acked-by on patch 1
- reworded the toread comment in rk_spi.c to say what the RX drain was
  actually doing for the transmit path (Quentin)

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     | 29 +++++++++++++++++++++++++----
 drivers/spi/spi-uclass.c | 16 ++++++++++++++++
 include/spi.h            |  2 ++
 test/dm/spi.c            | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 4 deletions(-)


base-commit: 527115ef6783cec49e5610c523c124b399011361
-- 
2.55.0

[PATCH v3 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.

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>
Acked-by: Quentin Schulz <quentin.schulz@cherry.de>
---
Changes in v3: dropped the happy path assertion from the test per
Quentin, collected the Acked-by. Rerun after the trim:

  $ ./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)

 drivers/spi/spi-uclass.c | 16 ++++++++++++++++
 include/spi.h            |  2 ++
 test/dm/spi.c            | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 57 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..a0da12ab6bc0 100644
--- a/test/dm/spi.c
+++ b/test/dm/spi.c
@@ -216,3 +216,42 @@ 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;
+	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 v3 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 v3: reworded the toread comment to name what the RX drain
was doing for the transmit path (pacing the loop against the wire),
per Quentin. Comment only; compile check rerun, exit 0.

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

diff --git a/drivers/spi/rk_spi.c b/drivers/spi/rk_spi.c
index 2c3d70ba7159..82009e61202f 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,14 @@ static int rockchip_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		writel(todo - 1, &regs->ctrlr1);
 		rkspi_enable_chip(regs, true);
 
-		toread = todo;
+		/*
+		 * 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.
+		 */
+		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 +534,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