From 697e5d3925350950a618379c750f7c9223566265 Mon Sep 17 00:00:00 2001 From: jflyper Date: Sun, 25 Jun 2017 17:21:29 +0900 Subject: [PATCH 1/3] Fix UART TX DMA corruption - Use ATOMIC while examining and updating DMA status - Mask premature TC interrupt by examining transfer count register (CNTDR/NTDR) for F1 and F4. --- src/main/drivers/nvic.h | 1 + src/main/drivers/serial_uart.c | 114 +++++++++++++++++------ src/main/drivers/serial_uart_impl.h | 2 +- src/main/drivers/serial_uart_stm32f10x.c | 7 +- src/main/drivers/serial_uart_stm32f30x.c | 5 +- src/main/drivers/serial_uart_stm32f4xx.c | 7 +- src/main/target/NERO/target.h | 3 + src/main/target/OMNIBUSF4/target.h | 1 - 8 files changed, 94 insertions(+), 46 deletions(-) diff --git a/src/main/drivers/nvic.h b/src/main/drivers/nvic.h index d582f5406d..2a3887404a 100644 --- a/src/main/drivers/nvic.h +++ b/src/main/drivers/nvic.h @@ -11,6 +11,7 @@ #define NVIC_PRIO_MPU_INT_EXTI NVIC_BUILD_PRIORITY(0x0f, 0x0f) #define NVIC_PRIO_MAG_INT_EXTI NVIC_BUILD_PRIORITY(0x0f, 0x0f) #define NVIC_PRIO_WS2811_DMA NVIC_BUILD_PRIORITY(1, 2) // TODO - is there some reason to use high priority? (or to use DMA IRQ at all?) +#define NVIC_PRIO_SERIALUART_TXDMA NVIC_BUILD_PRIORITY(1, 1) // Highest of all SERIALUARTx_TXDMA #define NVIC_PRIO_SERIALUART1_TXDMA NVIC_BUILD_PRIORITY(1, 1) #define NVIC_PRIO_SERIALUART1_RXDMA NVIC_BUILD_PRIORITY(1, 1) #define NVIC_PRIO_SERIALUART1 NVIC_BUILD_PRIORITY(1, 1) diff --git a/src/main/drivers/serial_uart.c b/src/main/drivers/serial_uart.c index d439a9e784..0002b27d0c 100644 --- a/src/main/drivers/serial_uart.c +++ b/src/main/drivers/serial_uart.c @@ -28,10 +28,14 @@ #include "platform.h" #include "build/build_config.h" +#include "build/atomic.h" #include "common/utils.h" + +#include "drivers/dma.h" #include "drivers/gpio.h" #include "drivers/inverter.h" +#include "drivers/nvic.h" #include "drivers/rcc.h" #include "drivers/serial.h" @@ -52,34 +56,87 @@ void uartSetMode(serialPort_t *instance, portMode_t mode) uartReconfigure(uartPort); } -void uartStartTxDMA(uartPort_t *s) +void uartTryStartTxDMA(uartPort_t *s) { + // uartTryStartTxDMA must be protected, since it is called from + // uartWrite and handleUsartTxDma (an ISR). + + ATOMIC_BLOCK(NVIC_PRIO_SERIALUART_TXDMA) { #ifdef STM32F4 - DMA_Cmd(s->txDMAStream, DISABLE); - DMA_MemoryTargetConfig(s->txDMAStream, (uint32_t)&s->port.txBuffer[s->port.txBufferTail], DMA_Memory_0); - //s->txDMAStream->M0AR = (uint32_t)&s->port.txBuffer[s->port.txBufferTail]; - if (s->port.txBufferHead > s->port.txBufferTail) { - s->txDMAStream->NDTR = s->port.txBufferHead - s->port.txBufferTail; - s->port.txBufferTail = s->port.txBufferHead; - } - else { - s->txDMAStream->NDTR = s->port.txBufferSize - s->port.txBufferTail; - s->port.txBufferTail = 0; - } - s->txDMAEmpty = false; - DMA_Cmd(s->txDMAStream, ENABLE); + if (s->txDMAStream->CR & 1) { + // DMA is already in progress + return; + } + + // For F4 (and F1), there are cases that NDTR (CNDTR for F1) is non-zero upon TC interrupt. + // We couldn't find out the root cause, so mask the case here. + + if (s->txDMAStream->NDTR) { + // Possible premature TC case. + goto reenable; + } + + // DMA_Cmd(s->txDMAStream, DISABLE); // XXX It's already disabled. + + if (s->port.txBufferHead == s->port.txBufferTail) { + // No more data to transmit. + s->txDMAEmpty = true; + return; + } + + // Start a new transaction. + + DMA_MemoryTargetConfig(s->txDMAStream, (uint32_t)&s->port.txBuffer[s->port.txBufferTail], DMA_Memory_0); + //s->txDMAStream->M0AR = (uint32_t)&s->port.txBuffer[s->port.txBufferTail]; + if (s->port.txBufferHead > s->port.txBufferTail) { + s->txDMAStream->NDTR = s->port.txBufferHead - s->port.txBufferTail; + s->port.txBufferTail = s->port.txBufferHead; + } + else { + s->txDMAStream->NDTR = s->port.txBufferSize - s->port.txBufferTail; + s->port.txBufferTail = 0; + } + s->txDMAEmpty = false; + + reenable: + DMA_Cmd(s->txDMAStream, ENABLE); #else - s->txDMAChannel->CMAR = (uint32_t)&s->port.txBuffer[s->port.txBufferTail]; - if (s->port.txBufferHead > s->port.txBufferTail) { - s->txDMAChannel->CNDTR = s->port.txBufferHead - s->port.txBufferTail; - s->port.txBufferTail = s->port.txBufferHead; - } else { - s->txDMAChannel->CNDTR = s->port.txBufferSize - s->port.txBufferTail; - s->port.txBufferTail = 0; - } - s->txDMAEmpty = false; - DMA_Cmd(s->txDMAChannel, ENABLE); + if (s->txDMAChannel->CCR & 1) { + // DMA is already in progress + return; + } + + // For F1 (and F4), there are cases that CNDTR (NDTR for F4) is non-zero upon TC interrupt. + // We couldn't find out the root cause, so mask the case here. + // F3 is not confirmed to be vulnerable, but not excluded as a safety. + + if (s->txDMAChannel->CNDTR) { + // Possible premature TC case. + goto reenable; + } + + if (s->port.txBufferHead == s->port.txBufferTail) { + // No more data to transmit. + s->txDMAEmpty = true; + return; + } + + // Start a new transaction. + + s->txDMAChannel->CMAR = (uint32_t)&s->port.txBuffer[s->port.txBufferTail]; + if (s->port.txBufferHead > s->port.txBufferTail) { + s->txDMAChannel->CNDTR = s->port.txBufferHead - s->port.txBufferTail; + s->port.txBufferTail = s->port.txBufferHead; + } else { + s->txDMAChannel->CNDTR = s->port.txBufferSize - s->port.txBufferTail; + s->port.txBufferTail = 0; + } + s->txDMAEmpty = false; + + reenable: + DMA_Cmd(s->txDMAChannel, ENABLE); #endif + } } uint32_t uartTotalRxBytesWaiting(const serialPort_t *instance) @@ -198,13 +255,12 @@ void uartWrite(serialPort_t *instance, uint8_t ch) } #ifdef STM32F4 - if (s->txDMAStream) { - if (!(s->txDMAStream->CR & 1)) + if (s->txDMAStream) #else - if (s->txDMAChannel) { - if (!(s->txDMAChannel->CCR & 1)) + if (s->txDMAChannel) #endif - uartStartTxDMA(s); + { + uartTryStartTxDMA(s); } else { USART_ITConfig(s->USARTx, USART_IT_TXE, ENABLE); } diff --git a/src/main/drivers/serial_uart_impl.h b/src/main/drivers/serial_uart_impl.h index 48cef851f2..24ae7d7589 100644 --- a/src/main/drivers/serial_uart_impl.h +++ b/src/main/drivers/serial_uart_impl.h @@ -161,7 +161,7 @@ extern uartDevice_t *uartDevmap[]; extern const struct serialPortVTable uartVTable[]; -void uartStartTxDMA(uartPort_t *s); +void uartTryStartTxDMA(uartPort_t *s); uartPort_t *serialUART(UARTDevice device, uint32_t baudRate, portMode_t mode, portOptions_t options); diff --git a/src/main/drivers/serial_uart_stm32f10x.c b/src/main/drivers/serial_uart_stm32f10x.c index f6ecd7e560..517c4cb3c8 100644 --- a/src/main/drivers/serial_uart_stm32f10x.c +++ b/src/main/drivers/serial_uart_stm32f10x.c @@ -109,12 +109,9 @@ void uart_tx_dma_IRQHandler(dmaChannelDescriptor_t* descriptor) { uartPort_t *s = (uartPort_t*)(descriptor->userParam); DMA_CLEAR_FLAG(descriptor, DMA_IT_TCIF); - DMA_Cmd(descriptor->ref, DISABLE); + DMA_Cmd(descriptor->channel, DISABLE); // XXX F1 needs this!!! - if (s->port.txBufferHead != s->port.txBufferTail) - uartStartTxDMA(s); - else - s->txDMAEmpty = true; + uartTryStartTxDMA(s); } // XXX Should serialUART be consolidated? diff --git a/src/main/drivers/serial_uart_stm32f30x.c b/src/main/drivers/serial_uart_stm32f30x.c index 881d634286..b94f81f508 100644 --- a/src/main/drivers/serial_uart_stm32f30x.c +++ b/src/main/drivers/serial_uart_stm32f30x.c @@ -170,10 +170,7 @@ static void handleUsartTxDma(dmaChannelDescriptor_t* descriptor) DMA_CLEAR_FLAG(descriptor, DMA_IT_TCIF); DMA_Cmd(descriptor->ref, DISABLE); - if (s->port.txBufferHead != s->port.txBufferTail) - uartStartTxDMA(s); - else - s->txDMAEmpty = true; + uartTryStartTxDMA(s); } void serialUARTInitIO(IO_t txIO, IO_t rxIO, portMode_t mode, portOptions_t options, uint8_t af, uint8_t index) diff --git a/src/main/drivers/serial_uart_stm32f4xx.c b/src/main/drivers/serial_uart_stm32f4xx.c index 89904d8c44..c47e2ee3ff 100644 --- a/src/main/drivers/serial_uart_stm32f4xx.c +++ b/src/main/drivers/serial_uart_stm32f4xx.c @@ -166,12 +166,7 @@ const uartHardware_t uartHardware[UARTDEV_COUNT] = { static void handleUsartTxDma(uartPort_t *s) { - DMA_Cmd(s->txDMAStream, DISABLE); - - if (s->port.txBufferHead != s->port.txBufferTail) - uartStartTxDMA(s); - else - s->txDMAEmpty = true; + uartTryStartTxDMA(s); } void dmaIRQHandler(dmaChannelDescriptor_t* descriptor) diff --git a/src/main/target/NERO/target.h b/src/main/target/NERO/target.h index 225f3c03c4..c528c177b3 100644 --- a/src/main/target/NERO/target.h +++ b/src/main/target/NERO/target.h @@ -80,14 +80,17 @@ #define USE_UART1 #define UART1_RX_PIN PA10 #define UART1_TX_PIN PA9 +//#define USE_UART1_TX_DMA #define USE_UART3 #define UART3_RX_PIN PB11 #define UART3_TX_PIN PB10 +//#define USE_UART3_TX_DMA #define USE_UART6 #define UART6_RX_PIN PC7 #define UART6_TX_PIN PC6 +//#define USE_UART6_TX_DMA #define USE_SOFTSERIAL1 #define USE_SOFTSERIAL2 diff --git a/src/main/target/OMNIBUSF4/target.h b/src/main/target/OMNIBUSF4/target.h index 4be0de9714..c1def4f3c7 100644 --- a/src/main/target/OMNIBUSF4/target.h +++ b/src/main/target/OMNIBUSF4/target.h @@ -126,7 +126,6 @@ #define USE_UART1 #define UART1_RX_PIN PA10 #define UART1_TX_PIN PA9 -#define UART1_AHB1_PERIPHERALS RCC_AHB1Periph_DMA2 #define USE_UART3 #define UART3_RX_PIN PB11 From 1dc5486e0253ff907616988150135eb0c8238830 Mon Sep 17 00:00:00 2001 From: jflyper Date: Sun, 25 Jun 2017 18:32:01 +0900 Subject: [PATCH 2/3] F1 uses ref, not channel --- src/main/drivers/serial_uart_stm32f10x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/drivers/serial_uart_stm32f10x.c b/src/main/drivers/serial_uart_stm32f10x.c index 517c4cb3c8..26e354a29f 100644 --- a/src/main/drivers/serial_uart_stm32f10x.c +++ b/src/main/drivers/serial_uart_stm32f10x.c @@ -109,7 +109,7 @@ void uart_tx_dma_IRQHandler(dmaChannelDescriptor_t* descriptor) { uartPort_t *s = (uartPort_t*)(descriptor->userParam); DMA_CLEAR_FLAG(descriptor, DMA_IT_TCIF); - DMA_Cmd(descriptor->channel, DISABLE); // XXX F1 needs this!!! + DMA_Cmd(descriptor->ref, DISABLE); // XXX F1 needs this!!! uartTryStartTxDMA(s); } From 563c340ab5aa6cecbd9ff324457a3c39c869648a Mon Sep 17 00:00:00 2001 From: jflyper Date: Sun, 25 Jun 2017 19:40:44 +0900 Subject: [PATCH 3/3] Revert to uartStartTxDMA for F7 Also revert target.h for NERO (used for testing) --- src/main/drivers/serial_uart_impl.h | 4 ++++ src/main/target/NERO/target.h | 3 --- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/drivers/serial_uart_impl.h b/src/main/drivers/serial_uart_impl.h index 24ae7d7589..e9cb8dda2a 100644 --- a/src/main/drivers/serial_uart_impl.h +++ b/src/main/drivers/serial_uart_impl.h @@ -161,7 +161,11 @@ extern uartDevice_t *uartDevmap[]; extern const struct serialPortVTable uartVTable[]; +#ifdef USE_HAL_DRIVER +void uartStartTxDMA(uartPort_t *s); +#else void uartTryStartTxDMA(uartPort_t *s); +#endif uartPort_t *serialUART(UARTDevice device, uint32_t baudRate, portMode_t mode, portOptions_t options); diff --git a/src/main/target/NERO/target.h b/src/main/target/NERO/target.h index c528c177b3..225f3c03c4 100644 --- a/src/main/target/NERO/target.h +++ b/src/main/target/NERO/target.h @@ -80,17 +80,14 @@ #define USE_UART1 #define UART1_RX_PIN PA10 #define UART1_TX_PIN PA9 -//#define USE_UART1_TX_DMA #define USE_UART3 #define UART3_RX_PIN PB11 #define UART3_TX_PIN PB10 -//#define USE_UART3_TX_DMA #define USE_UART6 #define UART6_RX_PIN PC7 #define UART6_TX_PIN PC6 -//#define USE_UART6_TX_DMA #define USE_SOFTSERIAL1 #define USE_SOFTSERIAL2