MIME-Version: 1.0
References: <20230310153151.293608-1-zenghuchen@google.com> <20230311022334.he6ev6tswfc2xcwo@mobilestation>
In-Reply-To: <20230311022334.he6ev6tswfc2xcwo@mobilestation>
From:   Jack Chen <zenghuchen@google.com>
Date:   Wed, 15 Mar 2023 14:06:36 -0400
Message-ID: <CALvyBcWewTs5pRwXktKcAuCOogXgQ8jkiwp5ToG7T+Dr2qaJjA@mail.gmail.com>
Subject: Re: [PATCH] spi: dw: remove delay between write and read
To:     Serge Semin <fancer.lancer@gmail.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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Serge,

> 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.

Thanks so much for your feedback. I understand the purpose of the specifica=
lly
calculated delay now. However, whether freeing cpu to other threads actuall=
y
depends on the size of delay. If the delay is smaller than 10 us, normally =
it
will cause busy-looping in cpu instead of freeing it.
And the delay does not work in all cases. For example:
if I am running the spi at 20M with a fifo size to be 8, and transfering a =
huge
chunk of data (4096 bytes) in one transfer, based on the delay calculation,=
 it
would add a 3200 ns delay between each sub-transfer, which is transformed t=
o
4us delay and in most cases on most platforms, udelay is not precise enough=
 and
I measured >=3D 5 us delay in most cases on my platform. So at least 1.8 us=
 extra
delay is added. Considering the time to fill tx_fifo, let's round it to 2us=
.
The actual time needed to transfer 8 bytes at 20M speed is just 3.2 us but =
we
added an extra delay of 2 us on average. When we consider the whole chunk o=
f
data (4096 bytes) in the whole transfer, we added more than 1 ms delay. Thi=
s
extra delay is long enough to fail a big chunk of data transfer application=
s (
e.g. image, audio.).

To overcome the extra delay, maybe we can consider the following two
proposals:
1) add a node in dts and allow users to enable the delay in polling mode.
2) Let's compare the needed delay time (bytes to transfer in tx fifo) to 10=
 us,
        and only call spi_delay_exec when the delay is bigger than 10 us. S=
ince
        When the delay is smaller than 10 us, short delay calls
(ndelay/udelay)
        are just busy-loops, even calling delay won't freeing cpu to
other tasks.
What is your opinion?
Thanks
Jack Chen

On Fri, Mar 10, 2023 at 9:23=E2=80=AFPM Serge Semin <fancer.lancer@gmail.co=
m> wrote:
>
> 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 b=
y
> > 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 performe=
d
> > - * procedure has been successful.
> > + * as possible to the Tx FIFO, then read from the Rx FIFO and check wh=
ether 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 completin=
g 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 t=
he
> > @@ -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 =3D SPI_DELAY_UNIT_SCK;
> > -     nbits =3D dws->n_bytes * BITS_PER_BYTE;
> > -
> >       do {
> >               dw_writer(dws);
> > -
> > -             delay.value =3D nbits * (dws->rx_len - dws->tx_len);
> > -             spi_delay_exec(&delay, transfer);
> > -
> >               dw_reader(dws);
> > -
> > +             spi_transfer_delay_exec(transfer);
> >               ret =3D dw_spi_check_status(dws, true);
> >               if (ret)
> >                       return ret;
> > --
> > 2.40.0.rc1.284.g88254d51c5-goog
> >