Re: STM32H7 serial TX DMA issues
Hi,
here is the pull request:
https://github.com/apache/nuttx/pull/11871
My initial comments (and "fix") for uart_xmitchars_dma() are no longer
relevant. Hence, those changes are no longer included.
Regards
Kian
From: Sebastien Lorquet
Sent: 08 March 2024 11:13
To: dev@nuttx.apache.org
Subject: Re: STM32H7 serial TX DMA issues
Hello,
Yes, stm32h7 uart transmission has issues. You can easily test this in
nsh with just an echo command and a very long string, eg > 64 ascii
chars. At first I believed it was buffering problems.
This caused me some headaches 1.5 years ago, but the DMA serial driver
is too complex for me to debug. I have disabled CONFIG_UARTn_TXDMA on
relevant uarts of my board.
Please give the link to your PR when it's ready so I can follow this
closely.
Thank you,
Sebastien
Le 08/03/2024 à 10:29, David Sidrane a écrit :
> Hi Kian,
>
> The Problem with the semaphore is it cause blocking when the port
> is opened non blocking.
>
> Please do PR so we can review it.
>
> David
>
>
> -Original Message-
> From: Kian Karas (KK)
> Sent: Friday, March 8, 2024 4:18 AM
> To: dev@nuttx.apache.org
> Subject: STM32H7 serial TX DMA issues
>
> Hi community
>
> The STM32H7 serial driver TX DMA logic is no longer working properly.
>
> The issues started with commit 660ac63b. Subsequent attempts (f92a9068,
> 6c186b60) have failed to get it working again.
>
> I think the original idea of 660ac63b is right, it just failed to restart
> TX DMA upon TX DMA completion (if needed).
>
> I would suggest reverting the following commits: 6c186b60 58f2a7b1
> 69a8b5b5. Then add the following patch as an amendment:
>
> diff --git a/arch/arm/src/stm32h7/stm32_serial.c
> b/arch/arm/src/stm32h7/stm32_serial.c
> index 120ea0f3b5..fc90c5d521 100644
> --- a/arch/arm/src/stm32h7/stm32_serial.c
> +++ b/arch/arm/src/stm32h7/stm32_serial.c
> @@ -3780,11 +3780,20 @@ static void up_dma_txcallback(DMA_HANDLE handle,
> uint8_t status, void *arg)
> }
> }
>
> - nxsem_post(>txdmasem);
> -
> /* Adjust the pointers */
>
> uart_xmitchars_done(>dev);
> +
> + /* Initiate another transmit if data is ready */
> +
> + if (priv->dev.xmit.tail != priv->dev.xmit.head)
> +{
> + uart_xmitchars_dma(>dev);
> +}
> + else
> +{
> + nxsem_post(>txdmasem);
> +}
> }
> #endif
>
> @@ -3806,6 +3815,14 @@ static void up_dma_txavailable(struct uart_dev_s
> *dev)
> int rv = nxsem_trywait(>txdmasem);
> if (rv == OK)
> {
> + if (dev->xmit.head == dev->xmit.tail)
> +{
> + /* No data to transfer. Release semaphore. */
> +
> + nxsem_post(>txdmasem);
> + return;
> +}
> +
> uart_xmitchars_dma(dev);
> }
> }
>
>
> However, uart_xmitchars_dma() is currently not safe to call from an
> interrupt service routine, so the following patch would also be required:
>
> diff --git a/drivers/serial/serial_dma.c b/drivers/serial/serial_dma.c
> index aa99e801ff..b2603953ad 100644
> --- a/drivers/serial/serial_dma.c
> +++ b/drivers/serial/serial_dma.c
> @@ -97,26 +97,29 @@ void uart_xmitchars_dma(FAR uart_dev_t *dev) {
> FAR struct uart_dmaxfer_s *xfer = >dmatx;
>
> - if (dev->xmit.head == dev->xmit.tail)
> + size_t head = dev->xmit.head;
> + size_t tail = dev->xmit.tail;
> +
> + if (head == tail)
> {
> /* No data to transfer. */
>
> return;
> }
>
> - if (dev->xmit.tail < dev->xmit.head)
> + if (tail < head)
> {
> - xfer->buffer = >xmit.buffer[dev->xmit.tail];
> - xfer->length = dev->xmit.head - dev->xmit.tail;
> + xfer->buffer = >xmit.buffer[tail];
> + xfer->length = head - tail;
> xfer->nbuffer = NULL;
> xfer->nlength = 0;
> }
> else
> {
> - xfer->buffer = >xmit.buffer[dev->xmit.tail];
> - xfer->length = dev->xmit.size - dev->xmit.tail;
> + xfer->buffer = >xmit.buffer[tail];
> + xfer->length = dev->xmit.size - tail;
> xfer->nbuffer = dev->xmit.buffer;
> - xfer->nlength = dev->xmit.head;
> + xfer->nlength = head;
> }
>
> dev->tx_count += xfer->length + xfer->nlength;
>
>
> Any thoughts?
>
> Regards
> Kian
Re: STM32H7 serial TX DMA issues
Hello,
Yes, stm32h7 uart transmission has issues. You can easily test this in
nsh with just an echo command and a very long string, eg > 64 ascii
chars. At first I believed it was buffering problems.
This caused me some headaches 1.5 years ago, but the DMA serial driver
is too complex for me to debug. I have disabled CONFIG_UARTn_TXDMA on
relevant uarts of my board.
Please give the link to your PR when it's ready so I can follow this
closely.
Thank you,
Sebastien
Le 08/03/2024 à 10:29, David Sidrane a écrit :
Hi Kian,
The Problem with the semaphore is it cause blocking when the port
is opened non blocking.
Please do PR so we can review it.
David
-Original Message-
From: Kian Karas (KK)
Sent: Friday, March 8, 2024 4:18 AM
To: dev@nuttx.apache.org
Subject: STM32H7 serial TX DMA issues
Hi community
The STM32H7 serial driver TX DMA logic is no longer working properly.
The issues started with commit 660ac63b. Subsequent attempts (f92a9068,
6c186b60) have failed to get it working again.
I think the original idea of 660ac63b is right, it just failed to restart
TX DMA upon TX DMA completion (if needed).
I would suggest reverting the following commits: 6c186b60 58f2a7b1
69a8b5b5. Then add the following patch as an amendment:
diff --git a/arch/arm/src/stm32h7/stm32_serial.c
b/arch/arm/src/stm32h7/stm32_serial.c
index 120ea0f3b5..fc90c5d521 100644
--- a/arch/arm/src/stm32h7/stm32_serial.c
+++ b/arch/arm/src/stm32h7/stm32_serial.c
@@ -3780,11 +3780,20 @@ static void up_dma_txcallback(DMA_HANDLE handle,
uint8_t status, void *arg)
}
}
- nxsem_post(>txdmasem);
-
/* Adjust the pointers */
uart_xmitchars_done(>dev);
+
+ /* Initiate another transmit if data is ready */
+
+ if (priv->dev.xmit.tail != priv->dev.xmit.head)
+{
+ uart_xmitchars_dma(>dev);
+}
+ else
+{
+ nxsem_post(>txdmasem);
+}
}
#endif
@@ -3806,6 +3815,14 @@ static void up_dma_txavailable(struct uart_dev_s
*dev)
int rv = nxsem_trywait(>txdmasem);
if (rv == OK)
{
+ if (dev->xmit.head == dev->xmit.tail)
+{
+ /* No data to transfer. Release semaphore. */
+
+ nxsem_post(>txdmasem);
+ return;
+}
+
uart_xmitchars_dma(dev);
}
}
However, uart_xmitchars_dma() is currently not safe to call from an
interrupt service routine, so the following patch would also be required:
diff --git a/drivers/serial/serial_dma.c b/drivers/serial/serial_dma.c
index aa99e801ff..b2603953ad 100644
--- a/drivers/serial/serial_dma.c
+++ b/drivers/serial/serial_dma.c
@@ -97,26 +97,29 @@ void uart_xmitchars_dma(FAR uart_dev_t *dev) {
FAR struct uart_dmaxfer_s *xfer = >dmatx;
- if (dev->xmit.head == dev->xmit.tail)
+ size_t head = dev->xmit.head;
+ size_t tail = dev->xmit.tail;
+
+ if (head == tail)
{
/* No data to transfer. */
return;
}
- if (dev->xmit.tail < dev->xmit.head)
+ if (tail < head)
{
- xfer->buffer = >xmit.buffer[dev->xmit.tail];
- xfer->length = dev->xmit.head - dev->xmit.tail;
+ xfer->buffer = >xmit.buffer[tail];
+ xfer->length = head - tail;
xfer->nbuffer = NULL;
xfer->nlength = 0;
}
else
{
- xfer->buffer = >xmit.buffer[dev->xmit.tail];
- xfer->length = dev->xmit.size - dev->xmit.tail;
+ xfer->buffer = >xmit.buffer[tail];
+ xfer->length = dev->xmit.size - tail;
xfer->nbuffer = dev->xmit.buffer;
- xfer->nlength = dev->xmit.head;
+ xfer->nlength = head;
}
dev->tx_count += xfer->length + xfer->nlength;
Any thoughts?
Regards
Kian
RE: STM32H7 serial TX DMA issues
Hi Kian,
The Problem with the semaphore is it cause blocking when the port
is opened non blocking.
Please do PR so we can review it.
David
-Original Message-
From: Kian Karas (KK)
Sent: Friday, March 8, 2024 4:18 AM
To: dev@nuttx.apache.org
Subject: STM32H7 serial TX DMA issues
Hi community
The STM32H7 serial driver TX DMA logic is no longer working properly.
The issues started with commit 660ac63b. Subsequent attempts (f92a9068,
6c186b60) have failed to get it working again.
I think the original idea of 660ac63b is right, it just failed to restart
TX DMA upon TX DMA completion (if needed).
I would suggest reverting the following commits: 6c186b60 58f2a7b1
69a8b5b5. Then add the following patch as an amendment:
diff --git a/arch/arm/src/stm32h7/stm32_serial.c
b/arch/arm/src/stm32h7/stm32_serial.c
index 120ea0f3b5..fc90c5d521 100644
--- a/arch/arm/src/stm32h7/stm32_serial.c
+++ b/arch/arm/src/stm32h7/stm32_serial.c
@@ -3780,11 +3780,20 @@ static void up_dma_txcallback(DMA_HANDLE handle,
uint8_t status, void *arg)
}
}
- nxsem_post(>txdmasem);
-
/* Adjust the pointers */
uart_xmitchars_done(>dev);
+
+ /* Initiate another transmit if data is ready */
+
+ if (priv->dev.xmit.tail != priv->dev.xmit.head)
+{
+ uart_xmitchars_dma(>dev);
+}
+ else
+{
+ nxsem_post(>txdmasem);
+}
}
#endif
@@ -3806,6 +3815,14 @@ static void up_dma_txavailable(struct uart_dev_s
*dev)
int rv = nxsem_trywait(>txdmasem);
if (rv == OK)
{
+ if (dev->xmit.head == dev->xmit.tail)
+{
+ /* No data to transfer. Release semaphore. */
+
+ nxsem_post(>txdmasem);
+ return;
+}
+
uart_xmitchars_dma(dev);
}
}
However, uart_xmitchars_dma() is currently not safe to call from an
interrupt service routine, so the following patch would also be required:
diff --git a/drivers/serial/serial_dma.c b/drivers/serial/serial_dma.c
index aa99e801ff..b2603953ad 100644
--- a/drivers/serial/serial_dma.c
+++ b/drivers/serial/serial_dma.c
@@ -97,26 +97,29 @@ void uart_xmitchars_dma(FAR uart_dev_t *dev) {
FAR struct uart_dmaxfer_s *xfer = >dmatx;
- if (dev->xmit.head == dev->xmit.tail)
+ size_t head = dev->xmit.head;
+ size_t tail = dev->xmit.tail;
+
+ if (head == tail)
{
/* No data to transfer. */
return;
}
- if (dev->xmit.tail < dev->xmit.head)
+ if (tail < head)
{
- xfer->buffer = >xmit.buffer[dev->xmit.tail];
- xfer->length = dev->xmit.head - dev->xmit.tail;
+ xfer->buffer = >xmit.buffer[tail];
+ xfer->length = head - tail;
xfer->nbuffer = NULL;
xfer->nlength = 0;
}
else
{
- xfer->buffer = >xmit.buffer[dev->xmit.tail];
- xfer->length = dev->xmit.size - dev->xmit.tail;
+ xfer->buffer = >xmit.buffer[tail];
+ xfer->length = dev->xmit.size - tail;
xfer->nbuffer = dev->xmit.buffer;
- xfer->nlength = dev->xmit.head;
+ xfer->nlength = head;
}
dev->tx_count += xfer->length + xfer->nlength;
Any thoughts?
Regards
Kian
STM32H7 serial TX DMA issues
Hi community
The STM32H7 serial driver TX DMA logic is no longer working properly.
The issues started with commit 660ac63b. Subsequent attempts (f92a9068,
6c186b60) have failed to get it working again.
I think the original idea of 660ac63b is right, it just failed to restart TX
DMA upon TX DMA completion (if needed).
I would suggest reverting the following commits: 6c186b60 58f2a7b1 69a8b5b5.
Then add the following patch as an amendment:
diff --git a/arch/arm/src/stm32h7/stm32_serial.c
b/arch/arm/src/stm32h7/stm32_serial.c
index 120ea0f3b5..fc90c5d521 100644
--- a/arch/arm/src/stm32h7/stm32_serial.c
+++ b/arch/arm/src/stm32h7/stm32_serial.c
@@ -3780,11 +3780,20 @@ static void up_dma_txcallback(DMA_HANDLE handle,
uint8_t status, void *arg)
}
}
- nxsem_post(>txdmasem);
-
/* Adjust the pointers */
uart_xmitchars_done(>dev);
+
+ /* Initiate another transmit if data is ready */
+
+ if (priv->dev.xmit.tail != priv->dev.xmit.head)
+{
+ uart_xmitchars_dma(>dev);
+}
+ else
+{
+ nxsem_post(>txdmasem);
+}
}
#endif
@@ -3806,6 +3815,14 @@ static void up_dma_txavailable(struct uart_dev_s *dev)
int rv = nxsem_trywait(>txdmasem);
if (rv == OK)
{
+ if (dev->xmit.head == dev->xmit.tail)
+{
+ /* No data to transfer. Release semaphore. */
+
+ nxsem_post(>txdmasem);
+ return;
+}
+
uart_xmitchars_dma(dev);
}
}
However, uart_xmitchars_dma() is currently not safe to call from an interrupt
service routine, so the following patch would also be required:
diff --git a/drivers/serial/serial_dma.c b/drivers/serial/serial_dma.c
index aa99e801ff..b2603953ad 100644
--- a/drivers/serial/serial_dma.c
+++ b/drivers/serial/serial_dma.c
@@ -97,26 +97,29 @@ void uart_xmitchars_dma(FAR uart_dev_t *dev)
{
FAR struct uart_dmaxfer_s *xfer = >dmatx;
- if (dev->xmit.head == dev->xmit.tail)
+ size_t head = dev->xmit.head;
+ size_t tail = dev->xmit.tail;
+
+ if (head == tail)
{
/* No data to transfer. */
return;
}
- if (dev->xmit.tail < dev->xmit.head)
+ if (tail < head)
{
- xfer->buffer = >xmit.buffer[dev->xmit.tail];
- xfer->length = dev->xmit.head - dev->xmit.tail;
+ xfer->buffer = >xmit.buffer[tail];
+ xfer->length = head - tail;
xfer->nbuffer = NULL;
xfer->nlength = 0;
}
else
{
- xfer->buffer = >xmit.buffer[dev->xmit.tail];
- xfer->length = dev->xmit.size - dev->xmit.tail;
+ xfer->buffer = >xmit.buffer[tail];
+ xfer->length = dev->xmit.size - tail;
xfer->nbuffer = dev->xmit.buffer;
- xfer->nlength = dev->xmit.head;
+ xfer->nlength = head;
}
dev->tx_count += xfer->length + xfer->nlength;
Any thoughts?
Regards
Kian
