STM32H7 serial TX DMA fix

[kk-thrane]

Summary

Fixes TX DMA for STM32H7

Testing

Works for me on an STM32H723

[davids5]

How is this checking for the DMA idle?

[kk-thrane]

The semaphore is used to indicate when DMA is idle or not. The call to nxsem_trywait() in up_dma_txavailable() perform this check.

Did you notice the reverts? The revert commits reintroduces the call to nxsem_trywait() - not my patch.

[davids5]

yes.

I am not clear on the flow, here is my concern

Half done check will run the queue has data check and call uart_xmitchars_dma(&priv->dev); This will call the the up_dma_txavailable and it is not check that the DMA is idle any more.
This can call the send again and Stops the DMA and a starts

[kk-thrane]

The half transfer interrupt is not enabled (for TX), only the completion interrupt is enabled.

I had forgot to remove that. I have pushed version where the half transfer interrupt handling is removed (from TX).

[davids5]

There are 2 paths, through up_dma_txavailable one under interrupt and one from the foreground. The issue that was solved by the critical section was

If a TX DMA completion interrups a forground write.
The TX DMA completion can start a dma_send and it will
then followed by the forground write's dma_send
stoping the,then in progress DMA.

By atomicaly marking the tx dma busy, the forground
write will not perform the dma_send, and will only
enqueue the data. At the next TX dma completion any
data pending in the tx queue will be sent

This case was a random failure and needs to be tested, It was hard to recreate.

[kk-thrane]

The current software in master is not working, because the test in up_dma_txavailable() does not correctly determine whether TX DMA is active or not. This could be fixed, but isn't it better to use a semaphore than a critical section?

I personally also find the code with the semaphore simpler and more intuitive: if TX DMA is running, it is always the ISR which initiates the next transfer.

The if-statement in up_dma_txavailable() could be fixed like this:
if (priv->dev.dmatx.length == 0 && priv->dev.dmatx.nlength == 0 &&
stm32_dmaresidual(priv->txdma) == 0)
But I think the call to stm32_dmaresidual() is not required?

I have a setup, which easily and immediately detects the errors with TX DMA the driver has and have had.

[davids5]

If the only change required is fixing the typo by adding the "== 0" why not do that? I think the CS is a few uS and a nit compared to the semaphore code time and complexity.

[kk-thrane]

To my surprise, nuttx implements nxsem_trywait() using a critical section... so yes, using a critical section instead of the semaphore provides better performance.
Hasn't nuttx got a better way to handle this? E.g. disabling only the DMA IRQ would be better.

I believe the call to stm32_dmaresidual() should be removed. I think is redundant and not a reliable method for testing whether DMA is running?

I don't like dma_up_txcallback() calling up_dma_txavailable(). It works, but it is unnecessary. The ISR doesn't need to enter a critical section. I prefer the dma_up_txcallback() implementation in my patch - just leave out the call to nxsem_post(). What do you think?

I will make another version with the above changes and push to this branch later this week when I have had time to verify it. I'm a little preoccupied by something at the moment.

[davids5]

@kk-thrane

I am glad you went through the nxsem_trywait and see the contrast now.

If you consider the foregrounds looping trying to transmit. I have concerns about the completion call back interrupting that looping. So the CS protects that. Now there is the case of dev.dmatx.length and dev.dmatx.nlength and how the ISR sets them when doing the second portion of the transfer. I believe there is a case where the DMA stm32_dmaresidual has not updated and dev.dmatx.length and dev.dmatx.nlength may be 0. So that is why the check is for the 3 elements.

Not accessing the dev.xmit was a conscious decision to not violate the encapsulation of the interface. Hence the call to up_dma_txavailable() which calls out to the layer managing dev.xmit