analogdevicesinc / msdk

Software Development Kit for Analog Device's MAX-series microcontrollers
Apache License 2.0
60 stars 76 forks source link

SPI RevA1 DMA Issue #879

Closed BrentK-ADI closed 5 months ago

BrentK-ADI commented 6 months ago

There is an issue with the implementation of DMA transfers in SPI RevA1 where if a transfer length is less than the FIFO size, the transfer never gets performed over DMA and the callback never gets issued.

This looks to be caused by the call to MXC_SPI_RevA1_TransHandler here: https://github.com/Analog-Devices-MSDK/msdk/blob/261425f8012dcb2b2e396360713f04a7c5fee828/Libraries/PeriphDrivers/Source/SPI/spi_reva1.c#L985

One fix is to remove that call, but there may be an underlying reason why it is there. Another possible fix is to look at the return value from MXC_SPI_RevA1_TransHandler to see if DMA is still needed.

For now, work arounds in the application side look like the following:

    if( nSize > MXC_SPI_FIFO_DEPTH )
    {
        result = MXC_SPI_MasterTransactionDMA(&spiReq);
    }
    else
    {
        result = MXC_SPI_MasterTransactionAsync(&spiReq);
    }
Jake-Carter commented 5 months ago

Thanks for reporting this @BrentK-ADI

I believe there is a known issue on some parts/revs with the SPI hardware. If you don't pre-load the FIFO with at least a byte the hardware won't start shifting out the TX data. I think the TransHandler there is an attempt to work around that and pre-load the FIFO. A comment would be nice...

I can see how this happens. For transactions smaller than the FIFO size, the DMA request gets set with a length of 0 here after the pre-load:

https://github.com/Analog-Devices-MSDK/msdk/blob/b7a733f3a389b4878cdd825a6ddf9c35b87b7859/Libraries/PeriphDrivers/Source/SPI/spi_reva1.c#L1191

To fix this I think we can check for a 0-length DMA transaction inside that conditional. If detected, we just manually run the callback.

Jake-Carter commented 5 months ago

Hi @BrentK-ADI, just opened a PR with a fix. If you have the chance to test it out and confirm it I'd appreciate it

BrentK-ADI commented 5 months ago

@Jake-Carter Sorry a little late to testing this. Tried it out on the project it was discovered on and everything is now working as expected. Thanks!

Jake-Carter commented 5 months ago

No problem, thanks for confirming!