Hi Cole,
On 8/18/26 11:33 AM, 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 <[email protected]>
---
Alexey, this is the FIFO wiring you asked about. It applies on top of
"spi: Handle spi-{tx,rx}-bus-width 0 as SPI_NO_TX/SPI_NO_RX", since it
needs those bits to exist.
You are right that the first patch is a no-op on its own. With this one
the Flipper One display bus stops running the receive FIFO at all: the
MISO pin is the end-of-frame GPIO, so every byte the controller clocked
in was discarded by the loop anyway.
The part that is not just a mode select is the 8-bit loop. It sets
toread = todo whether or not the caller passed a din, and drains the
receive FIFO to pace itself. In TMOD_TO that FIFO never fills, so it
would spin forever. Leaving toread at zero and letting the existing
rkspi_wait_till_not_busy() close out the chunk keeps the timing the same
for the transmit case, which already relied on that call.
Verification, and its limits. I have no Rockchip board, so this is
OK so this is a bit confusing because in the previous patch you said
it's a boot-log warning on Flipper One, which is a Rockchip device, and
here you say you don't own any Rockchip device. After reading the issue
on GitHub (which I guessed because I had seen repos for Flipper One on
GitHub already; please add a full link next time so we don't have to
guess), it seems you indeed do not own the device and just derived this
from a boot log provided by someone with access to the device.
compile-tested and reasoned from the driver, not run:
$ make jaguar-rk3588_defconfig
$ make CROSS_COMPILE=aarch64-linux-gnu- drivers/spi/rk_spi.o
CC drivers/spi/rk_spi.o (exit 0)
checkpatch --strict is 0/0/0. A full board build stops in binman for want
of BL31 and tee.bin, identically with and without this patch, so that one
is my missing blobs rather than the change.
What I cannot check here is the hardware behaviour: that TMOD_TO really
does leave the receive FIFO idle on a real part, and that a write-only
display still clocks out correctly. If you have a board in front of you,
that is the bit worth a look.
drivers/spi/rk_spi.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/rk_spi.c b/drivers/spi/rk_spi.c
index 2c3d70ba7159..6c2ed3a90bf9 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;
@@ -330,7 +344,7 @@ static int rockchip_spi_claim_bus(struct udevice *dev)
ctrlr0 |= FRF_SPI << FRF_SHIFT;
/* Tx and Rx mode */
- ctrlr0 |= TMOD_TR << TMOD_SHIFT;
+ ctrlr0 |= rkspi_base_tmod(priv) << TMOD_SHIFT;
Please reword the comment to something like
Configure RX/TX mode
(or remove the comment entirely) otherwise it's confusing.
writel(ctrlr0, ®s->ctrlr0);
@@ -472,7 +486,12 @@ static int rockchip_spi_xfer(struct udevice *dev, unsigned int bitlen,
writel(todo - 1, ®s->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;
The SPI core should not even ask the driver to read anything if it knows
the device cannot read. See __spi_validate in the Linux kernel. It
should error out instead of silently returning 0 and not read anything.
This needs to be implemented before ops->xfer() is called. Same applies
to the transmit path if it isn't supported though I wonder what kind of
setup we would have with a controller with no TX routed. This is also
required because otherwise the driver may still read when requested,
c.f. the rockchip_spi_16bit_reader optimization at the beginning of
rockchip_spi_xfer. If the core validates as expected prior to calling
ops->xfer(), then this wouldn't be called when someone attempts to do a
read on an NO_RX SPI device.
Additional thought: because dm_spi_xfer() specifies "writes "bitlen"
bits out the SPI MOSI port and simultaneously clocks "bitlen" bits in
the SPI MISO port. That's just the way SPI works" we cannot do the same
we've done with towrite, that is:
toread = in ? todo : 0;
would be incorrect and breaking assumptions from the code. So what you
suggested would be more appropriate indeed.
/* 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(®s->ctrlr0,
TMOD_MASK << TMOD_SHIFT,
- TMOD_TR << TMOD_SHIFT);
+ rkspi_base_tmod(priv) << TMOD_SHIFT);
We could optimize out two clrsetbits_le32 by checking whether
rkspi_base_tmod(priv) is already TMOD_TO, in which case we don't need to
do this after rockchip_spi_16bit_reader and at the end of
rockchip_spi_xfer since we know it's already correct. I don't think it's
necessarily worth implementing, but wanted to mention it.
Cheers,
Quentin
