[RFC] spi: dw: Test the last revision of the DMA module

[RFC] spi: dw: Test the last revision of the DMA module

[Serge Semin]

Hello Andy.
  
Have you tested the recent revision of the DW APB SSI driver with DMA
enabled? I am particularly concerned about the next fix in the driver:

> --- a/drivers/spi/spi-dw-dma.c
> +++ b/drivers/spi/spi-dw-dma.c
> @@ -346,8 +375,8 @@ static int dw_spi_dma_setup(struct dw_spi *dws, struct spi_transfer *xfer)
>  {
>       u16 imr = 0, dma_ctrl = 0;
>  
> -     dw_writel(dws, DW_SPI_DMARDLR, RX_BURST_LEVEL - 1);
> -     dw_writel(dws, DW_SPI_DMATDLR, TX_BURST_LEVEL);
> +     dw_writel(dws, DW_SPI_DMARDLR, dws->rxburst - 1);
> +     dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
>  
>       if (xfer->tx_buf) {
>               dma_ctrl |= SPI_DMA_TDMAE;

Generally speaking it must work (even DW APB SSI/DMA databook suggests to have
such DMATDLR setting), but in our case of a relatively slow DMA engine (it's
clocked with just twice higher frequency with respect to the max SPI bus
speed) sometimes SPI Rx FIFO gets overflown when SPI bus is configured to work
with maximum speed (there are multiple reasons why this happens, but generally
speaking all of them matter only due to the relatively slow DMA engine). The
problem is fixed by reducing a value written into the DMATDLR register.

I am wondering whether you've tested the last revision of the driver and it
worked for your version of the DW APB SSI + DW DMAC IPs. AFAIU DMA engine on
your devices is faster than on ours and has LLPs supported. So if you haven't
noticed any problem in the recent driver, then I'll send a fixup for our version
of the DW APB SSI block only (I'll have to introduce a new compatible string).
Otherwise I could get back a setting of dws->txburst into the DW_SPI_DMATDLR
register, which isn't that optimal as the current DMATDLR setting
of (fifo_len - txburst), but at least will make things working for all DMAs.

-Sergey

Re: [RFC] spi: dw: Test the last revision of the DMA module

[Andy Shevchenko]

On Fri, Jun 26, 2020 at 1:08 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:

> Have you tested the recent revision of the DW APB SSI driver with DMA
> enabled? I am particularly concerned about the next fix in the driver:

Yes.

> > +     dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);

Yes, this line is problematic.
However we have experiencing another issue with Tx underrun, that's
why below has not been shared.

> Generally speaking it must work (even DW APB SSI/DMA databook suggests to have
> such DMATDLR setting), but in our case of a relatively slow DMA engine (it's
> clocked with just twice higher frequency with respect to the max SPI bus
> speed) sometimes SPI Rx FIFO gets overflown when SPI bus is configured to work
> with maximum speed (there are multiple reasons why this happens, but generally
> speaking all of them matter only due to the relatively slow DMA engine). The
> problem is fixed by reducing a value written into the DMATDLR register.
>
> I am wondering whether you've tested the last revision of the driver and it
> worked for your version of the DW APB SSI + DW DMAC IPs. AFAIU DMA engine on
> your devices is faster than on ours and has LLPs supported. So if you haven't
> noticed any problem in the recent driver, then I'll send a fixup for our version
> of the DW APB SSI block only (I'll have to introduce a new compatible string).
> Otherwise I could get back a setting of dws->txburst into the DW_SPI_DMATDLR
> register, which isn't that optimal as the current DMATDLR setting
> of (fifo_len - txburst), but at least will make things working for all DMAs.

That's what I have locally.

commit 43d9abb2711f5096e969adcf1a2fb6456fa6e275 (HEAD -> topic/ehl-dma)
Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date:   Tue Jun 2 15:53:03 2020 +0300

    DEBUG SPI dw (burst fix?)

    Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index 5986c520b196..79342528b1f4 100644
--- a/drivers/spi/spi-dw-dma.c
+++ b/drivers/spi/spi-dw-dma.c
@@ -373,7 +373,7 @@ static int dw_spi_dma_setup(struct dw_spi *dws,
struct spi_transfer *xfer)
        u16 imr = 0, dma_ctrl = 0;

        dw_writel(dws, DW_SPI_DMARDLR, dws->rxburst - 1);
-       dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
+       dw_writel(dws, DW_SPI_DMATDLR, dws->txburst);



-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] spi: dw: Test the last revision of the DMA module

[Serge Semin]

On Fri, Jun 26, 2020 at 01:55:53AM +0300, Andy Shevchenko wrote:
> On Fri, Jun 26, 2020 at 1:08 AM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> 
> > Have you tested the recent revision of the DW APB SSI driver with DMA
> > enabled? I am particularly concerned about the next fix in the driver:
> 
> Yes.
> 
> > > +     dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
> 
> Yes, this line is problematic.
> However we have experiencing another issue with Tx underrun, that's
> why below has not been shared.

Hm, are you sure you meant ?Tx? underrun? DW APB SSI doesn't provide a status
bit of such an error. I don't even know how it might be possible, except a point
when SPI Tx FIFO just gets empty. If you meant SPI <Rx> FIFO underrun, then
it might be due to an invalid Rx DMA channel configuration: something with
BLOCK-length + DST_TR_WITDH + BURST-length setting. Though I am not sure it
could be connected with any of my recent patches for DW APB SSI or DW DMAC.

> 
> > Generally speaking it must work (even DW APB SSI/DMA databook suggests to have
> > such DMATDLR setting), but in our case of a relatively slow DMA engine (it's
> > clocked with just twice higher frequency with respect to the max SPI bus
> > speed) sometimes SPI Rx FIFO gets overflown when SPI bus is configured to work
> > with maximum speed (there are multiple reasons why this happens, but generally
> > speaking all of them matter only due to the relatively slow DMA engine). The
> > problem is fixed by reducing a value written into the DMATDLR register.
> >
> > I am wondering whether you've tested the last revision of the driver and it
> > worked for your version of the DW APB SSI + DW DMAC IPs. AFAIU DMA engine on
> > your devices is faster than on ours and has LLPs supported. So if you haven't
> > noticed any problem in the recent driver, then I'll send a fixup for our version
> > of the DW APB SSI block only (I'll have to introduce a new compatible string).
> > Otherwise I could get back a setting of dws->txburst into the DW_SPI_DMATDLR
> > register, which isn't that optimal as the current DMATDLR setting
> > of (fifo_len - txburst), but at least will make things working for all DMAs.
> 

> That's what I have locally.
> 
> commit 43d9abb2711f5096e969adcf1a2fb6456fa6e275 (HEAD -> topic/ehl-dma)
> Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date:   Tue Jun 2 15:53:03 2020 +0300
> 
>     DEBUG SPI dw (burst fix?)
> 
>     Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> index 5986c520b196..79342528b1f4 100644
> --- a/drivers/spi/spi-dw-dma.c
> +++ b/drivers/spi/spi-dw-dma.c
> @@ -373,7 +373,7 @@ static int dw_spi_dma_setup(struct dw_spi *dws,
> struct spi_transfer *xfer)
>         u16 imr = 0, dma_ctrl = 0;
> 
>         dw_writel(dws, DW_SPI_DMARDLR, dws->rxburst - 1);
> -       dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
> +       dw_writel(dws, DW_SPI_DMATDLR, dws->txburst);

Yep, that will solve the problem. Though in my case of setting something
like: 
> -       dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
> +       dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len / 2 - dws->txburst);

also worked. By doing so we artificially specify a Tx FIFO depth limitation,
which implicitly slows the "SPI Tx <-> Tx DMA channel" down, so occasionally,
if SPI Tx is fast enough while Tx DMA channel isn't, the SPI Tx FIFO might
even get emptied, but at least it will give enough time for the
"SPI Rx <-> Rx DMA channel" pair to fetch the incoming SPI traffic on time and
place the data into the memory (I suppose a text like this should be in a
comment above the line with the DMATDLR register setting). In my case I've
noticed this problem only when I executed several background user-space processes
intensively working with memory (like memory testbenches or just
"dd if=/dev/mem ..." like one-liner). My theory is that the background processes
implicitly slowed the Rx DMA channel down in a way so occasionally the internal
DMA FIFO's got full, due to which the Rx DMA channel couldn't handle the SPI Rx
handshaking interface requests on time to fetch data from the SPI Rx FIFO, so
the SPI Rx FIFO gets overflown. That chain of unfortunate problems is most likely to
happen in case if SPI-bus is fast enough. Obviously if I decrease the SPI bus
frequency, then no overflow will happen.

In case of our hardware setting DMATDLR with a Tx-burst length isn't enough to
completely prevent the SPI Rx FIFO overflow error. We also have to send SG list
entries one-by-one in order to solve the problem with DMA Tx LLP reloaded faster
than the DMA Rx LLP (remember the noLLP problem we've discussed in the DW DMA
mailing list?).

Anyway sorry for the inconvenience my patch caused. I'll send a fixup patch
soon, which will get back the DW_SPI_DMATDLR setting with just "dws->txburst"
value. In addition to that I've discovered a few optimizations for the DW DMAC
driver including a one to speed up the DEV_TO_MEM DMA-slave channel (which will
be useful for the DW APB SSI + DW DMA combination). I'll send a new
DW DMAC-related patchset soon after the current one is finally accepted by Vinod.)

-Sergey

> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [RFC] spi: dw: Test the last revision of the DMA module

[Andy Shevchenko]

On Fri, Jun 26, 2020 at 04:29:21PM +0300, Serge Semin wrote:
> On Fri, Jun 26, 2020 at 01:55:53AM +0300, Andy Shevchenko wrote:
> > On Fri, Jun 26, 2020 at 1:08 AM Serge Semin
> > <Sergey.Semin@baikalelectronics.ru> wrote:
> > 
> > > Have you tested the recent revision of the DW APB SSI driver with DMA
> > > enabled? I am particularly concerned about the next fix in the driver:
> > 
> > Yes.
> > 
> > > > +     dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
> > 
> > Yes, this line is problematic.
> > However we have experiencing another issue with Tx underrun, that's
> > why below has not been shared.
> 
> Hm, are you sure you meant ?Tx? underrun? DW APB SSI doesn't provide a status
> bit of such an error. I don't even know how it might be possible, except a point
> when SPI Tx FIFO just gets empty. If you meant SPI <Rx> FIFO underrun, then
> it might be due to an invalid Rx DMA channel configuration: something with
> BLOCK-length + DST_TR_WITDH + BURST-length setting. Though I am not sure it
> could be connected with any of my recent patches for DW APB SSI or DW DMAC.

Tx overrun or Rx over-/underrun, wording above is not correct from my side. But
unfortunately I don't remember which one I have got.

> > > Generally speaking it must work (even DW APB SSI/DMA databook suggests to have
> > > such DMATDLR setting), but in our case of a relatively slow DMA engine (it's
> > > clocked with just twice higher frequency with respect to the max SPI bus
> > > speed) sometimes SPI Rx FIFO gets overflown when SPI bus is configured to work
> > > with maximum speed (there are multiple reasons why this happens, but generally
> > > speaking all of them matter only due to the relatively slow DMA engine). The
> > > problem is fixed by reducing a value written into the DMATDLR register.
> > >
> > > I am wondering whether you've tested the last revision of the driver and it
> > > worked for your version of the DW APB SSI + DW DMAC IPs. AFAIU DMA engine on
> > > your devices is faster than on ours and has LLPs supported. So if you haven't
> > > noticed any problem in the recent driver, then I'll send a fixup for our version
> > > of the DW APB SSI block only (I'll have to introduce a new compatible string).
> > > Otherwise I could get back a setting of dws->txburst into the DW_SPI_DMATDLR
> > > register, which isn't that optimal as the current DMATDLR setting
> > > of (fifo_len - txburst), but at least will make things working for all DMAs.
> > 
> 
> > That's what I have locally.
> > 
> > commit 43d9abb2711f5096e969adcf1a2fb6456fa6e275 (HEAD -> topic/ehl-dma)
> > Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Date:   Tue Jun 2 15:53:03 2020 +0300
> > 
> >     DEBUG SPI dw (burst fix?)
> > 
> >     Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> > index 5986c520b196..79342528b1f4 100644
> > --- a/drivers/spi/spi-dw-dma.c
> > +++ b/drivers/spi/spi-dw-dma.c
> > @@ -373,7 +373,7 @@ static int dw_spi_dma_setup(struct dw_spi *dws,
> > struct spi_transfer *xfer)
> >         u16 imr = 0, dma_ctrl = 0;
> > 
> >         dw_writel(dws, DW_SPI_DMARDLR, dws->rxburst - 1);
> > -       dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
> > +       dw_writel(dws, DW_SPI_DMATDLR, dws->txburst);
> 
> Yep, that will solve the problem. Though in my case of setting something
> like: 
> > -       dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
> > +       dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len / 2 - dws->txburst);
> 
> also worked. By doing so we artificially specify a Tx FIFO depth limitation,
> which implicitly slows the "SPI Tx <-> Tx DMA channel" down, so occasionally,
> if SPI Tx is fast enough while Tx DMA channel isn't, the SPI Tx FIFO might
> even get emptied, but at least it will give enough time for the
> "SPI Rx <-> Rx DMA channel" pair to fetch the incoming SPI traffic on time and
> place the data into the memory (I suppose a text like this should be in a
> comment above the line with the DMATDLR register setting). In my case I've
> noticed this problem only when I executed several background user-space processes
> intensively working with memory (like memory testbenches or just
> "dd if=/dev/mem ..." like one-liner). My theory is that the background processes
> implicitly slowed the Rx DMA channel down in a way so occasionally the internal
> DMA FIFO's got full, due to which the Rx DMA channel couldn't handle the SPI Rx
> handshaking interface requests on time to fetch data from the SPI Rx FIFO, so
> the SPI Rx FIFO gets overflown. That chain of unfortunate problems is most likely to
> happen in case if SPI-bus is fast enough. Obviously if I decrease the SPI bus
> frequency, then no overflow will happen.
> 
> In case of our hardware setting DMATDLR with a Tx-burst length isn't enough to
> completely prevent the SPI Rx FIFO overflow error. We also have to send SG list
> entries one-by-one in order to solve the problem with DMA Tx LLP reloaded faster
> than the DMA Rx LLP (remember the noLLP problem we've discussed in the DW DMA
> mailing list?).
> 
> Anyway sorry for the inconvenience my patch caused. I'll send a fixup patch
> soon, which will get back the DW_SPI_DMATDLR setting with just "dws->txburst"

Any news about a fix?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] spi: dw: Test the last revision of the DMA module

[Serge Semin]

On Mon, Jul 20, 2020 at 01:51:55PM +0300, Andy Shevchenko wrote:
> On Fri, Jun 26, 2020 at 04:29:21PM +0300, Serge Semin wrote:
> > On Fri, Jun 26, 2020 at 01:55:53AM +0300, Andy Shevchenko wrote:
> > > On Fri, Jun 26, 2020 at 1:08 AM Serge Semin
> > > <Sergey.Semin@baikalelectronics.ru> wrote:
> > > 
> > > > Have you tested the recent revision of the DW APB SSI driver with DMA
> > > > enabled? I am particularly concerned about the next fix in the driver:
> > > 
> > > Yes.
> > > 
> > > > > +     dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
> > > 
> > > Yes, this line is problematic.
> > > However we have experiencing another issue with Tx underrun, that's
> > > why below has not been shared.
> > 
> > Hm, are you sure you meant ?Tx? underrun? DW APB SSI doesn't provide a status
> > bit of such an error. I don't even know how it might be possible, except a point
> > when SPI Tx FIFO just gets empty. If you meant SPI <Rx> FIFO underrun, then
> > it might be due to an invalid Rx DMA channel configuration: something with
> > BLOCK-length + DST_TR_WITDH + BURST-length setting. Though I am not sure it
> > could be connected with any of my recent patches for DW APB SSI or DW DMAC.
> 
> Tx overrun or Rx over-/underrun, wording above is not correct from my side. But
> unfortunately I don't remember which one I have got.
> 
> > > > Generally speaking it must work (even DW APB SSI/DMA databook suggests to have
> > > > such DMATDLR setting), but in our case of a relatively slow DMA engine (it's
> > > > clocked with just twice higher frequency with respect to the max SPI bus
> > > > speed) sometimes SPI Rx FIFO gets overflown when SPI bus is configured to work
> > > > with maximum speed (there are multiple reasons why this happens, but generally
> > > > speaking all of them matter only due to the relatively slow DMA engine). The
> > > > problem is fixed by reducing a value written into the DMATDLR register.
> > > >
> > > > I am wondering whether you've tested the last revision of the driver and it
> > > > worked for your version of the DW APB SSI + DW DMAC IPs. AFAIU DMA engine on
> > > > your devices is faster than on ours and has LLPs supported. So if you haven't
> > > > noticed any problem in the recent driver, then I'll send a fixup for our version
> > > > of the DW APB SSI block only (I'll have to introduce a new compatible string).
> > > > Otherwise I could get back a setting of dws->txburst into the DW_SPI_DMATDLR
> > > > register, which isn't that optimal as the current DMATDLR setting
> > > > of (fifo_len - txburst), but at least will make things working for all DMAs.
> > > 
> > 
> > > That's what I have locally.
> > > 
> > > commit 43d9abb2711f5096e969adcf1a2fb6456fa6e275 (HEAD -> topic/ehl-dma)
> > > Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Date:   Tue Jun 2 15:53:03 2020 +0300
> > > 
> > >     DEBUG SPI dw (burst fix?)
> > > 
> > >     Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > 
> > > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> > > index 5986c520b196..79342528b1f4 100644
> > > --- a/drivers/spi/spi-dw-dma.c
> > > +++ b/drivers/spi/spi-dw-dma.c
> > > @@ -373,7 +373,7 @@ static int dw_spi_dma_setup(struct dw_spi *dws,
> > > struct spi_transfer *xfer)
> > >         u16 imr = 0, dma_ctrl = 0;
> > > 
> > >         dw_writel(dws, DW_SPI_DMARDLR, dws->rxburst - 1);
> > > -       dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
> > > +       dw_writel(dws, DW_SPI_DMATDLR, dws->txburst);
> > 
> > Yep, that will solve the problem. Though in my case of setting something
> > like: 
> > > -       dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
> > > +       dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len / 2 - dws->txburst);
> > 
> > also worked. By doing so we artificially specify a Tx FIFO depth limitation,
> > which implicitly slows the "SPI Tx <-> Tx DMA channel" down, so occasionally,
> > if SPI Tx is fast enough while Tx DMA channel isn't, the SPI Tx FIFO might
> > even get emptied, but at least it will give enough time for the
> > "SPI Rx <-> Rx DMA channel" pair to fetch the incoming SPI traffic on time and
> > place the data into the memory (I suppose a text like this should be in a
> > comment above the line with the DMATDLR register setting). In my case I've
> > noticed this problem only when I executed several background user-space processes
> > intensively working with memory (like memory testbenches or just
> > "dd if=/dev/mem ..." like one-liner). My theory is that the background processes
> > implicitly slowed the Rx DMA channel down in a way so occasionally the internal
> > DMA FIFO's got full, due to which the Rx DMA channel couldn't handle the SPI Rx
> > handshaking interface requests on time to fetch data from the SPI Rx FIFO, so
> > the SPI Rx FIFO gets overflown. That chain of unfortunate problems is most likely to
> > happen in case if SPI-bus is fast enough. Obviously if I decrease the SPI bus
> > frequency, then no overflow will happen.
> > 
> > In case of our hardware setting DMATDLR with a Tx-burst length isn't enough to
> > completely prevent the SPI Rx FIFO overflow error. We also have to send SG list
> > entries one-by-one in order to solve the problem with DMA Tx LLP reloaded faster
> > than the DMA Rx LLP (remember the noLLP problem we've discussed in the DW DMA
> > mailing list?).
> > 
> > Anyway sorry for the inconvenience my patch caused. I'll send a fixup patch
> > soon, which will get back the DW_SPI_DMATDLR setting with just "dws->txburst"
> 

> Any news about a fix?

Fix is in my repo. I was going to submit it together with the rest of the
changes concerning the DMA noLLP problem after Vinod merges my DW DMAC patchset
in. But as you can see for some reason he doesn't rush with that at all. If you
need it I can submit the patch separately tomorrow.

-Segey

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [RFC] spi: dw: Test the last revision of the DMA module

[Andy Shevchenko]

On Tue, Jul 21, 2020 at 12:02 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Mon, Jul 20, 2020 at 01:51:55PM +0300, Andy Shevchenko wrote:
> > On Fri, Jun 26, 2020 at 04:29:21PM +0300, Serge Semin wrote:

...

> > > Anyway sorry for the inconvenience my patch caused. I'll send a fixup patch
> > > soon, which will get back the DW_SPI_DMATDLR setting with just "dws->txburst"
> >
>
> > Any news about a fix?
>
> Fix is in my repo. I was going to submit it together with the rest of the
> changes concerning the DMA noLLP problem after Vinod merges my DW DMAC patchset
> in. But as you can see for some reason he doesn't rush with that at all. If you
> need it I can submit the patch separately tomorrow.

Yes, please!

-- 
With Best Regards,
Andy Shevchenko