Re: Linux SPI Slave fails to respond under high load on BeagleBone Black

[Vignesh Raghavendra <vigneshr@xxxxxx>]

On 11/13/20 1:18 AM, Glenn Schmottlach wrote:
> On Tue, Nov 10, 2020 at 2:06 AM Vignesh Raghavendra <vigneshr@xxxxxx> wrote:
> 
>> Could you trace how much of data is left in TX FIFO using MCSPI_CHSTAT_0
>> and MCSPI_XFERLEVEL registers?
>>
> 
> When the SPI slave is no longer responding to the SPI master the
> following was observed on the SPI slave:
> 
> devmem2 0x48030130 w    (CHSTAT_0)
> Read at address  0x48030130 (0xb6f5c130): 0x0000004F
> 
> So  0x4F means:
>       RXFFF=1 (FIFO RX buffer is full)
>       TXFFE=1 (FIFO transmit buffer is empty)
>       EOT=1 (Set to 1 at end of SPI transfer)
>       TXS=1 (TX register status: register is empty)
>       RXS=1 (RX register status: register is full)
> 
> So it appears there is data ready to be read from the SPI master and
> there is no data scheduled to be sent.
> 
> devmem2 0x4803017C w  (XFERLEVEL)
> Read at address  0x4803017C (0xb6fd717c): 0x00000000
> 
> So WCNT = 0 and it appears no words are scheduled to be transferred on
> the channel.
> 

Does the transfer resume if you manually updated WCNT field to a 
value > 32 using devmem2 when slave appears to be "stuck"? 

> Again, the SPI slave appears to be "stuck"
> 
[...]
> 
> At this point it's waiting for a completion that appears to never come.
> 
>> One possible reason may be that driver sets up  MCSPI_XFERLEVEL WCNT and
>> enables channel before setting up TX DMA. So if there a master that is
>> continuously requesting data, then there is a possibility that WCNT goes
>> down to 0 (due to shifting of 0s) before DMA had chance to put actual
>> the data into the FIFO.
>>
>> Does it help if you move the programming of WCNT field to
>> omap2_mcspi_tx_dma()?
>>
> 
> That sounds plausible and I *attempted* to move this code but I
> must've implemented it incorrectly because the driver stopped
> functioning. It's likely my implementation and/or interpretation of
> your suggestion is wrong. If you could provide a more explicit
> description of the changes or a patch I would certainly be willing to
> attempt it again.
> 
>> Main intention of adding SPI slave support was to support limited
>> usecase where master and slave exchange very small known messages of
>> predetermined length such as to support at sparse interval (such as dead
> 
> I was hoping to pass high-speed/bandwidth telemetry data from the SPI
> slave to the SPI master. Apparently my use case may not match the one
> described. With that said, I think it may still be achievable with
> tweaking by someone with more experience with the McSPI driver. I
> introduced a "usleep()" inside spi-pipe() just after the SPI transfer
> on the SPI master application. With a long enough delay between
> transfers (~2-10 msec) the SPI master/slave succeeds indefinitely and
> the SPI slave never gets "stuck". Unfortunately, this kills my
> telemetry throughput. So, there is likely some kind of race condition
> in the SPI slave controller code.
> 
> Any additional suggestions or patches to try would be very
> appreciated. I think the potential to work reliably under load is
> there but it still needs a few changes. Hopefully these are small
> changes, localized to a single file, and don't require a big refactor.
> 


Could you see if below diff helps? This delays enabling of channel
until TX DMA is queued so that WCNT does not decrement


diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index d4c9510af393..bf8c6526bcd7 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -426,6 +426,8 @@ static void omap2_mcspi_tx_dma(struct spi_device *spi,
        }
        dma_async_issue_pending(mcspi_dma->dma_tx);
        omap2_mcspi_set_dma_req(spi, 0, 1);
+       if (spi_controller_is_slave(master))
+               omap2_mcspi_set_enable(spi, 1);
 }
 
 static unsigned
@@ -1194,7 +1196,9 @@ static int omap2_mcspi_transfer_one(struct spi_master *master,
                    master->can_dma(master, spi, t))
                        omap2_mcspi_set_fifo(spi, t, 1);
 
-               omap2_mcspi_set_enable(spi, 1);
+               /* For slave TX, enable after DMA is queued */
+               if (!spi_controller_is_slave(master) || !t->tx_buf)
+                       omap2_mcspi_set_enable(spi, 1);
 
                /* RX_ONLY mode needs dummy data in TX reg */
                if (t->tx_buf == NULL)