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

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, &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;

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(&regs->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