Date:   Sat, 11 Mar 2023 05:23:34 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Jack Chen <zenghuchen@google.com>
Cc:     Mark Brown <broonie@kernel.org>, linux-spi@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Jesus Sanchez-Palencia <jesussanp@google.com>,
        Mark Slevinsky <markslevinsky@google.com>
Subject: Re: [PATCH] spi: dw: remove delay between write and read
Message-ID: <20230311022334.he6ev6tswfc2xcwo@mobilestation>
References: <20230310153151.293608-1-zenghuchen@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230310153151.293608-1-zenghuchen@google.com>
Precedence: bulk

Hi Jack

On Fri, Mar 10, 2023 at 10:31:51AM -0500, Jack Chen wrote:
> Delay between write and read in polling mode is not necessary in dw spi
> driver. It was added assuming that dw spi controller need the delay to
> send data from tx fifo to spi devices. But it is not needed because
> following reasons:
> 1) dw spi datasheet claims transfer begins when first data word is
>    present in the transmit FIFO and a slave is enabled. So at least we
>    do not need the full fifo-size-transfer time delay.
> 2) in practice, due to spi devices implementation, spi full-duplex
>    (write and read real data) is always split into two transfers.

In practice the delay is specifically added to minimize the dummy
loops in the poll-based transfer. It's calculated based on the number
of bytes pushed to the Tx FIFO and the SPI-bus clock rate (that's why
the spi_transfer.effective_speed_hz field is initialized in the
driver). So after all of them are transferred we get to start reading
data from the Rx FIFO. Until then the kernel thread is supposed to
sleep giving up the CPU for another tasks.

> Delay between spi transfers may be needed. But this can be introduced by
> using a more formal helper function "spi_transfer_delay_exec", in which
> the delay time is passed by users through spi_ioc_transfer.

This is wrong. spi_transfer.delay is supposed to be executed after the
whole transfer is completed. You suggest to to do that in between some
random data chunks pushed and pulled from the controller FIFO.
Moreover that delay is already performed by the SPI-core:
https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi.c#L1570

-Serge(y)

> 
> Signed-off-by: Jack Chen <zenghuchen@google.com>
> ---
>  drivers/spi/spi-dw-core.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index c3bfb6c84cab..7c10fb353567 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -379,9 +379,12 @@ static void dw_spi_irq_setup(struct dw_spi *dws)
>  
>  /*
>   * The iterative procedure of the poll-based transfer is simple: write as much
> - * as possible to the Tx FIFO, wait until the pending to receive data is ready
> - * to be read, read it from the Rx FIFO and check whether the performed
> - * procedure has been successful.
> + * as possible to the Tx FIFO, then read from the Rx FIFO and check whether the
> + * performed procedure has been successful.
> + *
> + * Delay is introduced in the end of each transfer before (optionally) changing
> + * the chipselect status, then starting the next transfer or completing the
> + * list of @spi_message.
>   *
>   * Note this method the same way as the IRQ-based transfer won't work well for
>   * the SPI devices connected to the controller with native CS due to the
> @@ -390,21 +393,12 @@ static void dw_spi_irq_setup(struct dw_spi *dws)
>  static int dw_spi_poll_transfer(struct dw_spi *dws,
>  				struct spi_transfer *transfer)
>  {
> -	struct spi_delay delay;
> -	u16 nbits;
>  	int ret;
>  
> -	delay.unit = SPI_DELAY_UNIT_SCK;
> -	nbits = dws->n_bytes * BITS_PER_BYTE;
> -
>  	do {
>  		dw_writer(dws);
> -
> -		delay.value = nbits * (dws->rx_len - dws->tx_len);
> -		spi_delay_exec(&delay, transfer);
> -
>  		dw_reader(dws);
> -
> +		spi_transfer_delay_exec(transfer);
>  		ret = dw_spi_check_status(dws, true);
>  		if (ret)
>  			return ret;
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
>