drivers: serial: silabs: bug correction on high baudrate for series 2 #97721
Conversation
zephyrbot
added
platform: Silabs
zephyrbot
requested review from
asmellby,
dcpleung,
jerome-pouiller and
jhedberg
|
|
||
| dma_stop(data->dma_tx.dma_dev, data->dma_tx.dma_channel); | ||
|
|
||
| EUSART_IntEnable(config->eusart, EUSART_IF_TXC); |
There was a problem hiding this comment.
If the transfer already completed when we get here (e.g. because the DMA interrupt is delayed by higher-priority interrupts), TXC will already be set at this point, and the ISR will fire immediately. I assume that's the intent?
If so, shouldn't this be the last thing to happen in the function, in order to robustly tolerate a scenario where the UART ISR has higher priority than the DMA ISR, and will preempt at this point? Otherwise, interrupt priority configuration will determine whether data->dma_tx.enabled = false happens before or after the UART ISR runs.
I assume you have also verified that the UART ISR actually fires if TXC is set prior to enabling the IF.
There was a problem hiding this comment.
For the last part, async uart test are working so it means that we managed to fire the callback otherwise we will not have our TX done event. Moreover, if I strictly quote the rf: "If USART interrupts are enabled, an interrupt will be made if one or more of the interrupt flags in USART_IF and their corresponding bits in USART_IEN are set.", I understand it as : the NVIC line is asserted only when both condition are meet (in any order). I assume it's working but that's a valid question. I test it with a baudrate up to 2000000.
Good point about the preemption of the ISR and yes it should be the last thing to happen in the function.
I first thinking about calling the tx event done directly here but it could lead to situation where we say to the user that the transfer is done but that's not technically right (especially with low baudrate) and i'm not okay with that. That's why i'm going for enabling the interrupt here.
There was a problem hiding this comment.
btw I've done it correctly in usart driver, don't know why I switch in eusart ...
This patch correct a bug where the uart/eusart tx callback might be triggered before the dma complete callback at high baudrate when using async transfer. Signed-off-by: Martin Hoff <[email protected]>
At high baudrate when using async api of the uart, the abort function in not giving the right informations since we're stopping the dma after getting the status of it. It makes the uart_async_api test failed on high baudrate. Signed-off-by: Martin Hoff <[email protected]>
Martinhoff-maker
force-pushed
the
uart-irq-high-baudrate-correction
branch
from
ee2d501 to
a8de046
Compare
|
5 participants
This PR correct a bug where the uart/eusart tx callback might be triggered before the dma complete callback at high baudrate when using async transfer. It leads to an error because you can't actually give a new buffer for the next transfer because of the unavailability of the dma channel.
It also resolved another bug where async_tx_abort function in not giving the right information since we're stopping the dma after getting the status of it. It makes the uart_async_api test failed on high baudrate.
It's a bug found by @shontal1005, can you confirm that it resolves your issue ?