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
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: Addition of STM32H7 MCU's
Hi Robert, Community We have NuttX running on an STM32H723VE, but haven't tested all peripherals. We also did some initial work on an STM32H730, but this has hardly been tested. What is the best way to share the STM32H723VE support with the community? It needs some reviewing. I am concerned we could have broken stuff for other MCUs in the familly, but I can't test this. @Robert: if you are in a hurry, send me an email directly and I'll respond with a patch. Regards Kian From: Robert Turner Sent: 18 January 2024 03:30 To: dev@nuttx.apache.org Subject: Re: Addition of STM32H7 MCU's Nah not internal cache. The SRAM sizes for H723/5 are different from any of those defined in arch/arm/include/stm32h7/chip.h Suspect we need to get these correct as other files use these defs also, such as stm32_allocateheap.c Is Jorge's PR the one merged on Jul 12 (8ceff0d)? Thanks, Robert On Thu, Jan 18, 2024 at 2:56 PM Alan C. Assis wrote: > Hi Robert, > Thank you for the explanation! Is it about internal cache? > > Looking at > https://www.st.com/en/microcontrollers-microprocessors/stm32h7-series.html > I can see that H723/5 shares mostly everything with H333/5. > I only tested NuttX on STM32H743ZI and STM32H753BI (I and Jorge added > support to this few weeks ago). > > Please take a look at Jorge's PRs, probably if you fix the memory in the > linker script and the clock tree for your board NuttX will work fine on it. > > BR, > > Alan > > On Wed, Jan 17, 2024 at 10:25 PM Robert Turner wrote: > > > Apologies, I should have been more specific, I was referring to parts in > > the family which are not currently covered, such as the STM32H723xx which > > we use. The RAM sizes definitions in chip.h for > > CONFIG_STM32H7_STM32H7X3XX/CONFIG_STM32H7_STM32H7X5XX are incorrect for > > the STM32H723xx and STM32H725xx. > > BR, > > Robert > > > > On Thu, Jan 18, 2024 at 1:28 PM Alan C. Assis wrote: > > > > > Robert, > > > STM32H7 family is already supported. > > > > > > Look at arch/arm/src/stm32h7 and equivalent at boards/ > > > > > > BR, > > > > > > Alan > > > > > > On Tuesday, January 16, 2024, Robert Turner wrote: > > > > > > > Did anyone finish supporting the broader STM32H7xx family? If so, is > it > > > > close to being mergeable or sendable as a patch? > > > > > > > > Thanks, > > > > Robert > > > > > > > > On Fri, Sep 8, 2023 at 10:33 PM raiden00pl > > wrote: > > > > > > > > > > You're right, but not entirely) For example, chips of different > > > > subseries > > > > > have different interrupt vector tables. Those. The > stm32h7x3xx_irq.h > > > file > > > > > lists interrupt vectors for the RM0433, but not for the RM0455 or > > > > > RM0468. Although > > > > > some chips from all these series have 7x3 in the name. > > > > > > > > > > I think they are the same (not checked, intuition tells me so). But > > > some > > > > > peripherals are not available on some chips and then the > > > > > corresponding interrupt line is marked RESERVED, or its the same > > > > peripheral > > > > > but with upgraded functionality (QSPI/OCTOSPI) or > > > > > for some reason ST changed its name to confuse devs. There should > be > > no > > > > > conflict between IRQ lines. > > > > > > > > > > > Even if it duplicates 90% of the file it is better than #ifdefing > > the > > > > > > stm32h7x3xx_irq.h file. AKA ifdef rash! > > > > > > > > > > One file approach can be done with only one level of #ifdefs, one > > level > > > > of > > > > > #ifdefs doesn't have a negative impact on code quality (but > > > > > it's probably a matter of individual feelings). > > > > > For IRQ and memory map (and probably DMAMUX), the approach with > > > separate > > > > > files may make sense but for peripheral definitions > > > > > I don't see any benefit in duplicating files. > > > > > > > > > > pt., 8 wrz 2023 o 12:01 > > > napisał(a): > > > > > > > > > > > You're right, but not entirely) For example, chips of different > > > > subseries > > > > > > have different interrupt vector tables. Those. The > > stm32h7x3xx_irq.h > > > > file > > > > > > lists interrupt vectors for the RM0433, but not for the RM0455 or > > > > > RM0468. Although > > > > > > some chips from all these series have 7x3 in the name. > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > *От:* "raiden00pl" > > > > > > *Кому:* "undefined" > > > > > > *Отправлено:* пятница, 8 сентября 2023 г., 12:52 > > > > > > *Тема:* Re: Addition of STM32H7 MCU's > > > > > > > > > > > > From what I'm familiar with STM32H7, all chips use the same > > registers > > > > and > > > > > > bit definitions. > > > > > > Therefore, keeping definitions for different chips in different > > files > > > > > > doesn't make sense in my opinion. > > > > > > The only problem is that some chips support some peripherals > while > > > > others > > > > > > do not. But this can be > > > > > > solved using definitions from Kconfig, where we define the > >
Re: TUN device (PPP) issue?
Hi Zhe I am working on tag nuttx-12.2.1. Your referenced commit did indeed fix the issue. My apologies for not trying on master. I mistakenly though the error was in the TUN device driver, which I noticed had not changed since nuttx-12.2.1. Thanks a lot! Kian From: Zhe Weng 翁�� Sent: 17 January 2024 04:55 To: Kian Karas (KK) Cc: dev@nuttx.apache.org Subject: Re: TUN device (PPP) issue? Hi Kian, Which version of NuttX are you working on? It behaves like a problem I've met before. Do you have this commit in your code? If not, maybe you could have a try: https://github.com/apache/nuttx/commit/e2c9aa65883780747ca00625a1452dddc6f8a138 Best regards Zhe From: Kian Karas (KK) Sent: Tuesday, January 16, 2024 11:53:06 PM To: dev@nuttx.apache.org Subject: TUN device (PPP) issue? Hi community I am experiencing an issue with PPP/TUN and reception of packets. The network stack reports different decoding errors in the received packets e.g.: [ 24.56] [ WARN] ppp: ipv4_in: WARNING: IP packet shorter than length in IP header I can reproduce the issue by sending a number of packets (from my PC over PPP to the TUN device in NuttX), which are all larger than can fit into one IOB *and* which are ignored (e.g. unsupported protocol or IP destination) - i.e. *not* triggering a response / TX packet. I then send a correct ICMP echo request from my PC to NuttX, which causes the above error to be reported. The following PC commands will trigger the error message. My PC has IP 172.29.4.1 and the NuttX ppp interface has 172.29.4.2. Note the first command sends to the *wrong* IP address so that NuttX ignores the ICMP messages. The second commands uses the IP of NuttX and should result in a response. I run the test after a fresh boot and with no other network traffic to/from NuttX. $ ping -I ppp0 -W 0.2 -i 0.2 -c 13 172.29.4.3 -s 156 $ ping -I ppp0 -W 0.2 -c 1 172.29.4.2 -s 0 If I skip the first command, ping works fine. I think the issue is caused by the IOB management in the TUN device driver (drivers/net/tun.c). I am new to NuttX, so I don't quite understand the correct use of IOB, so I am just guessing here. I think that when a packet is received by tun_write() and too large to fit into a single IOB *and* the packet is ignored, the IOB chain "lingers" and is not freed. Subsequent packets received by tun_write() does not end up in the beginning of the first IOB and the IP/TCP/UDP header may then be split across IOB boundary. The network stack assumes the protocol headers are not split across IOB boundaries, so the network stack ends up reading outside the IOB io_data[] array boundaries resulting in undefined behavior. With CONFIG_IOB_DEBUG enabled, notice how the "avail" value decrease for each ignored packet until the final/correct ICMP request (at time 24.54) is copied to the second IOB in the chain. [ 10.06] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 len=184 offset=0 [ 10.06] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 avail=0 len=184 next=0 [ 10.06] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 Copy 182 bytes new len=182 [ 10.07] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 added to the chain [ 10.07] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 avail=0 len=2 next=0 [ 10.08] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 Copy 2 bytes new len=2 [ 10.08] [ INFO] ppp0: tun_net_receive_tun: IPv4 frame [ 10.08] [ INFO] ppp0: ipv4_in: WARNING: Not destined for us; not forwardable... Dropping! [ 10.26] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 len=184 offset=0 [ 10.26] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 avail=168 len=184 next=0x24002a50 [ 10.27] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 Copy 168 bytes new len=168 [ 10.27] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 avail=2 len=16 next=0 [ 10.28] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 Copy 16 bytes new len=16 [ 10.28] [ INFO] ppp0: tun_net_receive_tun: IPv4 frame [ 10.28] [ INFO] ppp0: ipv4_in: WARNING: Not destined for us; not forwardable... Dropping! [ 10.46] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 len=184 offset=0 [ 10.47] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 avail=154 len=184 next=0x24002a50 [ 10.47] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 Copy 154 bytes new len=154 [ 10.48] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 avail=16 len=30 next=0 [ 10.48] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 Copy 30 bytes new len=30 [ 10.48] [ INFO] ppp0: tun_net_receive_tun: IPv4 frame [ 10.49] [ INFO] ppp0: ipv4_in: WARNING: Not destined for us; not forwardable... Dropping! ... [ 12.50] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 len=184 of
TUN device (PPP) issue?
Hi community I am experiencing an issue with PPP/TUN and reception of packets. The network stack reports different decoding errors in the received packets e.g.: [ 24.56] [ WARN] ppp: ipv4_in: WARNING: IP packet shorter than length in IP header I can reproduce the issue by sending a number of packets (from my PC over PPP to the TUN device in NuttX), which are all larger than can fit into one IOB *and* which are ignored (e.g. unsupported protocol or IP destination) - i.e. *not* triggering a response / TX packet. I then send a correct ICMP echo request from my PC to NuttX, which causes the above error to be reported. The following PC commands will trigger the error message. My PC has IP 172.29.4.1 and the NuttX ppp interface has 172.29.4.2. Note the first command sends to the *wrong* IP address so that NuttX ignores the ICMP messages. The second commands uses the IP of NuttX and should result in a response. I run the test after a fresh boot and with no other network traffic to/from NuttX. $ ping -I ppp0 -W 0.2 -i 0.2 -c 13 172.29.4.3 -s 156 $ ping -I ppp0 -W 0.2 -c 1 172.29.4.2 -s 0 If I skip the first command, ping works fine. I think the issue is caused by the IOB management in the TUN device driver (drivers/net/tun.c). I am new to NuttX, so I don't quite understand the correct use of IOB, so I am just guessing here. I think that when a packet is received by tun_write() and too large to fit into a single IOB *and* the packet is ignored, the IOB chain "lingers" and is not freed. Subsequent packets received by tun_write() does not end up in the beginning of the first IOB and the IP/TCP/UDP header may then be split across IOB boundary. The network stack assumes the protocol headers are not split across IOB boundaries, so the network stack ends up reading outside the IOB io_data[] array boundaries resulting in undefined behavior. With CONFIG_IOB_DEBUG enabled, notice how the "avail" value decrease for each ignored packet until the final/correct ICMP request (at time 24.54) is copied to the second IOB in the chain. [ 10.06] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 len=184 offset=0 [ 10.06] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 avail=0 len=184 next=0 [ 10.06] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 Copy 182 bytes new len=182 [ 10.07] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 added to the chain [ 10.07] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 avail=0 len=2 next=0 [ 10.08] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 Copy 2 bytes new len=2 [ 10.08] [ INFO] ppp0: tun_net_receive_tun: IPv4 frame [ 10.08] [ INFO] ppp0: ipv4_in: WARNING: Not destined for us; not forwardable... Dropping! [ 10.26] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 len=184 offset=0 [ 10.26] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 avail=168 len=184 next=0x24002a50 [ 10.27] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 Copy 168 bytes new len=168 [ 10.27] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 avail=2 len=16 next=0 [ 10.28] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 Copy 16 bytes new len=16 [ 10.28] [ INFO] ppp0: tun_net_receive_tun: IPv4 frame [ 10.28] [ INFO] ppp0: ipv4_in: WARNING: Not destined for us; not forwardable... Dropping! [ 10.46] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 len=184 offset=0 [ 10.47] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 avail=154 len=184 next=0x24002a50 [ 10.47] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 Copy 154 bytes new len=154 [ 10.48] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 avail=16 len=30 next=0 [ 10.48] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 Copy 30 bytes new len=30 [ 10.48] [ INFO] ppp0: tun_net_receive_tun: IPv4 frame [ 10.49] [ INFO] ppp0: ipv4_in: WARNING: Not destined for us; not forwardable... Dropping! ... [ 12.50] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 len=184 offset=0 [ 12.51] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 avail=14 len=184 next=0x24002a50 [ 12.51] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 Copy 14 bytes new len=14 [ 12.52] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 avail=156 len=170 next=0 [ 12.52] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 Copy 170 bytes new len=170 [ 12.52] [ INFO] ppp0: tun_net_receive_tun: IPv4 frame [ 12.53] [ INFO] ppp0: ipv4_in: WARNING: Not destined for us; not forwardable... Dropping! [ 24.54] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 len=28 offset=0 [ 24.54] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 avail=0 len=28 next=0x24002a50 [ 24.55] [ INFO] ppp0: iob_copyin_internal: iob=0x24002b20 Copy 0 bytes new len=0 [ 24.55] [ INFO] ppp0: iob_copyin_internal: iob=0x24002a50 avail=170
