You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "davids5 (via GitHub)" <gi...@apache.org> on 2023/10/24 16:13:38 UTC
[PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
davids5 opened a new pull request, #11020:
URL: https://github.com/apache/nuttx/pull/11020
## Summary
The DMA can bring in more rx data, than the number of
DMA completions call backs. The call backs happen on
idle, 1/2 and full events. But in between these events
the DMA can write more data to the buffers memory that
need to be brought in to the cache. (invalidate)
We do the invalidate on the reads from the fifo memory
if the the DMA as commited since the last read.
## Impact
There is dropped data at high baud rates.
## Testing
imxrt1170 px4 nxp_fmurt1170-v1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1370532073
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -2345,14 +2346,26 @@ static bool imxrt_rxflowcontrol(struct uart_dev_s *dev,
#ifdef SERIAL_HAVE_RXDMA
static int imxrt_dma_receive(struct uart_dev_s *dev, unsigned int *status)
{
- struct imxrt_uart_s *priv = (struct imxrt_uart_s *)dev;
- uint32_t nextrx = imxrt_dma_nextrx(priv);
- int c = 0;
+ struct imxrt_uart_s *priv = (struct imxrt_uart_s *)dev;
+ static uint32_t last_nextrx = -1;
+ uint32_t nextrx = imxrt_dma_nextrx(priv);
+ int c = 0;
/* Check if more data is available */
if (nextrx != priv->rxdmanext)
{
+ /* Now we must ensure the cache is updated if the DMA has
+ * updated again.
+ */
+
+ if (last_nextrx != nextrx)
+ {
+ up_invalidate_dcache((uintptr_t)priv->rxfifo,
+ (uintptr_t)priv->rxfifo + RXDMA_BUFFER_SIZE);
+ last_nextrx = nextrx;
+ }
Review Comment:
Yes. We always have: The Rx is used in circular mode. Looping the data in the the "fifo". If you invalidate in the callback only that is not enough. The above code detect that the DMA moved and re-invalidates. It saves about 14/15 invalidates.
I think we need this fix across all the arches that use this "rxfifo" and have a dcache
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372197146
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
@danielappiagyei-bc - I updated https://github.com/apache/nuttx/pull/11020#discussion_r1372179887 also the optimization may make the branch look link it is taking the `then of if ((regval16 & EDMA_TCD_CITER_ELINK) != 0)'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372197146
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
@danielappiagyei-bc - I updated https://github.com/apache/nuttx/pull/11020#discussion_r1372179887 also the optimization may make the branch look like it is taking the `then of if ((regval16 & EDMA_TCD_CITER_ELINK) != 0)'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1370501788
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -2345,14 +2346,26 @@ static bool imxrt_rxflowcontrol(struct uart_dev_s *dev,
#ifdef SERIAL_HAVE_RXDMA
static int imxrt_dma_receive(struct uart_dev_s *dev, unsigned int *status)
{
- struct imxrt_uart_s *priv = (struct imxrt_uart_s *)dev;
- uint32_t nextrx = imxrt_dma_nextrx(priv);
- int c = 0;
+ struct imxrt_uart_s *priv = (struct imxrt_uart_s *)dev;
+ static uint32_t last_nextrx = -1;
+ uint32_t nextrx = imxrt_dma_nextrx(priv);
+ int c = 0;
/* Check if more data is available */
if (nextrx != priv->rxdmanext)
{
+ /* Now we must ensure the cache is updated if the DMA has
+ * updated again.
+ */
+
+ if (last_nextrx != nextrx)
+ {
+ up_invalidate_dcache((uintptr_t)priv->rxfifo,
+ (uintptr_t)priv->rxfifo + RXDMA_BUFFER_SIZE);
+ last_nextrx = nextrx;
+ }
Review Comment:
So we simply can do a read while DMA is running? Is that correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "danielappiagyei-bc (via GitHub)" <gi...@apache.org>.
danielappiagyei-bc commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1370809227
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
I've tested by adding a DEBUGASSERT and can confirm that it is possible to get 0 for `dmaresidual`. It took awhile though, at _least_ 500 seconds, probably closer to a 1000 (I wasn't present after 500s but my debugger caught the reset exception). Please add the conditional that I did in my branch to all affected files: https://github.com/apache/nuttx/pull/10911/files
Edit: assertion occured a second time at 900 seconds
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372150226
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
To be clear the the test I am running is not based on default baud rate. it is running at 3Mpbs. It uses hardware HS.
Do you have gaps in the data coming in greater then 1 character time in a single message?
Please try and set the fifo back to 32 and the idle count 1, that will cause the DMA call back to be run more often and move the data from the fifo to the rx buffer.
The loop is a DLAST to the TCD loop.
Once the port is open. [this should always be true
](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_edma.c#L1249C2-L1249C42) Are you opening it once or opening and closing it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "danielappiagyei-bc (via GitHub)" <gi...@apache.org>.
danielappiagyei-bc commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1373486112
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
Appreciate the thorough work here david. I don't have an answer for why I'm experiencing the issue while you're not. I apologize for not having a branch with a minimum, reproducible example to give you, as I'm using my company's codebase as a test bed. Are you opposed to me just making a PR with my suggested fix? It is only an extra `if` statement with a simple conditional so performance really shouldn't be affected. It also does not make the existing code suddenly incorrect, so I don't see any negatives to it
```
static unsigned int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
unsigned int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
if (dmaresidual == 0)
{
/* We've finished transferring RXDMA_BUFFER_SIZE bytes. The next byte
* will be placed at the beginning of the buffer
*/
return 0;
}
else
{
return RXDMA_BUFFER_SIZE - dmaresidual;
}
}
```
This is essentially equivalent to what you did for `kinetis_serial.c`:
```
return (RXDMA_BUFFER_SIZE - (int)dmaresidual) % RXDMA_BUFFER_SIZE;
```
Note that `kinetis_edma.c` [also has a EDMA_TCD_DONE flag](https://github.com/apache/nuttx/blob/master/arch/arm/src/kinetis/kinetis_edma.c#L1051)
The register description of the eDMA `DONE` flag I posted earlier does not say that it won't automatically be set for our configuration. I can post on the NXP support forum and ask them specifically about the IMXRT1064 and place the reply here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "danielappiagyei-bc (via GitHub)" <gi...@apache.org>.
danielappiagyei-bc commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1370809227
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
I've tested by adding a DEBUGASSERT and can confirm that it is possible to get 0 for `dmaresidual`. It took awhile though, at _least_ 500 seconds, probably closer to a 1000 (I wasn't present after 500s but my debugger caught the reset exception). Please add the conditional that I did in my branch to all affected files: https://github.com/apache/nuttx/pull/10911/files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1371212483
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
@danielappiagyei-bc
The rxfifo is a ring buffer. Written to by the DMA and read from by code. N.B `imxrt_dma_nextrx` is only used on the RX side to check if data has been added by the DMA.
Based on this setup
```
/* Configure for circular DMA reception into the RX FIFO */
config.saddr = priv->uartbase + IMXRT_LPUART_DATA_OFFSET;
config.daddr = (uint32_t) priv->rxfifo;
config.soff = 0;
config.doff = 1;
config.iter = RXDMA_BUFFER_SIZE;
config.flags = EDMA_CONFIG_LINKTYPE_LINKNONE |
EDMA_CONFIG_LOOPDEST |
EDMA_CONFIG_INTHALF |
EDMA_CONFIG_INTMAJOR;
config.ssize = EDMA_8BIT;
config.dsize = EDMA_8BIT;
config.nbytes = 1;
#ifdef CONFIG_KINETIS_EDMA_ELINK
config.linkch = 0;
#endif
```
The DMA reads a byte from IMXRT_LPUART_DATA_OFFSET and writes a byte it to priv->rxfifo adding 1 to to the destination address register and decrementing the count register. (That starts at RXDMA_BUFFER_SIZE. ) EDMA_CONFIG_LOOPDEST causes the destination address register to be reloaded with priv->rxfifo at count = 0 and the count is reloaded.
Therefore it can not write outside the buffer.
I could image a case were the count is sampled before the reload but I have not seen it.
Even if it did, the net effect is `imxrt_dma_nextrx` returns 32. But the only way the return from `imxrt_dma_nextrx` is used is to compare it with the `priv->rxdmanext` to determine if data is in the ring.
`priv->rxdmanext` is constrained from 0 - RXDMA_BUFFER_SIZE-1; by
```
priv->rxdmanext++;
if (priv->rxdmanext == RXDMA_BUFFER_SIZE)
{
priv->rxdmanext = 0;
}
```
read access is via
```
c = priv->rxfifo[priv->rxdmanext]
```
Not the DMA' index returned from imxrt_dma_nextrx().
Glitch of 32 would just say there is data, the next read, will see the truth. So the assert !=0 should stay out.
But the nagging question is how do you see 0..
What are your config settings for DMA and LPUART?
Here is what I have
```
CONFIG_IMXRT_EDMA=y
CONFIG_IMXRT_EDMA_EDBG=y
CONFIG_IMXRT_EDMA_ELINK=y
CONFIG_IMXRT_EDMA_NTCD=64\
# ttyS2
CONFIG_LPUART4_RXDMA=y
CONFIG_LPUART4_TXDMA=y
ONFIG_IMXRT_LPUART4=y
CONFIG_LPUART4_BAUD=57600
CONFIG_LPUART4_IFLOWCONTROL=y
CONFIG_LPUART4_OFLOWCONTROL=y
CONFIG_LPUART4_RXBUFSIZE=600
CONFIG_LPUART4_RXDMA=y
CONFIG_LPUART4_TXBUFSIZE=3000
CONFIG_LPUART4_TXDMA=y
```
I am running on this https://github.com/PX4/PX4-Autopilot/tree/pr-imxrt1170-imxrt1062 branch with `make nxp_fmurt1170-v1` using `serial_test -s -c -p /dev/ttyS2 -b 3000000` on the DUT and `./linux-serial-test -s -p /dev/serial/by-id/usb-FTDI_TTL-234X-3V3_FT2QRM97-if00-port0 -c -b 3000000` on my linux box.
imxrt_dma_nextrx returns unsigned int. So the DEBUGASSERT ignores the dmaresidual > 0 term.
Replacing it with
<img width="932" alt="image" src="https://github.com/apache/nuttx/assets/1945821/46caece0-97f1-4383-a681-1dffb1cf1d7e">
Will trap the condition. I have a break on the assert firing. But it has never. 20 minute in.....
<img width="701" alt="image" src="https://github.com/apache/nuttx/assets/1945821/732f5497-708e-4c53-b56e-0912e4b098fa">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "danielappiagyei-bc (via GitHub)" <gi...@apache.org>.
danielappiagyei-bc commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372396374
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
Thanks David, I will run this later (probably tomorrow) when I have a chance. I've actually never used nutshell before so will have to get familiar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#issuecomment-1917073839
@jturnsek How would you like to change it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "danielappiagyei-bc (via GitHub)" <gi...@apache.org>.
danielappiagyei-bc commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1370757990
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
I like this DEBUGASSERT but I would like to change it to include `DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE && dmaresidual >= 0`). I could have sworn `imxrt_dmach_getcount()` returned 0 before; I will test again today and let you know.
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
I like this DEBUGASSERT but I would like to change it to include `DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE && dmaresidual > 0`). I could have sworn `imxrt_dmach_getcount()` returned 0 before; I will test again today and let you know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1371163923
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
I will look into t this....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "danielappiagyei-bc (via GitHub)" <gi...@apache.org>.
danielappiagyei-bc commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372064314
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
> The rxfifo is a ring buffer. Written to by the DMA and read from by code. N.B imxrt_dma_nextrx is only used on the RX side to check if data has been added by the DMA.
>
> The DMA reads a byte from IMXRT_LPUART_DATA_OFFSET and writes a byte it to priv->rxfifo adding 1 to to the destination address register and decrementing the count register. (That starts at RXDMA_BUFFER_SIZE. ) EDMA_CONFIG_LOOPDEST causes the destination address register to be reloaded with priv->rxfifo at count = 0 and the count is reloaded.
Agree with everything here and my DMA RX TCD setup is the exact same as what the code snippet you posted. I have not altered it and am on nuttx 12.
> Therefore it can not write outside the buffer.
I think I said writes were possible outside the buffer in the original issue I wrote but I was wrong, you are correct here :)
> But the nagging question is how do you see 0..
See the logic in the [imxrt_dmach_getcount()](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_edma.c#L1235) function that is called by [imxrt_dma_nextrx()](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_serial.c#L1375).
In `imxrt_edma.c` L1249, `remaining` is the value that will be returned and it is initialized to 0. The number of remaining bytes is only calculated if the CHANNEL DONE flag (`EDMA_TCD_CSR_DONE`) _isn't_ set. Otherwise, `remaining` is returned unmodified with the initial value of 0. I've added an `else {DEBUGPANIC();}` and placed a breakpoint there that does get hit reliably. I know we configured DMA to operate in a circular fashion but the user manual for the IMXRT1064 doesn't say that the `EDMA_TCD_CSR_DONE` flag _won't_ be set when using the circular buffer configuration.
From the reg description:
> The value of this field is reset to 0 by the hardware (_when the channel is activated_) or by software.
Emphasis mine. The flag does eventually get cleared but I'm not sure whether it's the DMA hardware clearing it or software (the [imxrt_dmach_interrupt()](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_edma.c#L544) function in `imxrt_edma.c`. I'm assuming that even though the channel is used in a circular buffer fashion, it is only "activated" once, so `imxrt_dmach_interrupt()` must be responsible for clearing it.
> What are your config settings for DMA and LPUART?
I should have included this first, my apologies. I've got a much higher baud rate than you:
```
#LPUART3
CONFIG_IMXRT_LPUART3=y
CONFIG_LPUART3_BAUD=460800
CONFIG_LPUART3_SERIAL_CONSOLE=n
CONFIG_LPUART3_SERIALDRIVER=y
CONFIG_LPUART3_TXDMA=n
CONFIG_LPUART3_TXBUFSIZE=128
CONFIG_LPUART3_RXDMA=y
# serial RX buffer SMI reads from
CONFIG_LPUART3_RXBUFSIZE=512
```
I also:
- hard-coded the value of `CONFIG_IMXRT_SERIAL_RXDMA_BUFFER_SIZE` in `imxrt_serial.c` to 256 instead of 32 (hardcoded because it's not a kconfig (yet))
- Made the UART require 32 consecutive idle characters before setting the IDLE flag in the LPUART STATUS register (a feature I'll be bringing to nuttx soon). To do so, I added this line to the bottom of [imxrt_lpuart_configure](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_lowputc.c#L363):
```
modreg32(LPUART_CTRL_IDLECFG_32, LPUART_CTRL_IDLECFG_MASK, base + IMXRT_LPUART_CTRL_OFFSET);
```
Everything else is whatever defaults kconfig uses.
> imxrt_dma_nextrx returns unsigned int. So the DEBUGASSERT ignores the dmaresidual > 0 term.
An unsigned int can still be 0 so im not sure why the compiler would optimize that out. Regardless, `!= 0` displays intent better and works just as fine
> Will trap the condition. I have a break on the assert firing. But it has never. 20 minute in.....
If it is indeed a race condition and it takes me 15+ minutes at my much higher baud rate of 400k I figure it would take even longer for your rate
When I comment out my fix:
```
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
int nextrx = RXDMA_BUFFER_SIZE - dmaresidual;
/* if index is past end of buffer, reset to 0*/
// if (nextrx == RXDMA_BUFFER_SIZE)
// {
// nextrx = 0;
// }
return nextrx;
}
```
I get CRC errors once again, randing from every 5 seconds to every 100+ seconds
> I could image a case were the count is sampled before the reload but I have not seen it.
> Even if it did, the net effect is imxrt_dma_nextrx returns 32. But the only way the return from imxrt_dma_nextrx is used is to compare it with the priv->rxdmanext to determine if data is in the ring.
> Glitch of 32 would just say there is data, the next read, will see the truth. So the assert !=0 should stay out.
In [this](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_serial.c#L2356) comparison, what happens when `rxdmanext` is `0` and `nextrx` is (incorrectly) `RXDMA_BUFFER_SIZE` instead of `0`. We're comparing the next head of the circular buffer to the tail. Normally with a circular buffer, when the next head == the tail, we wouldn't read the data. In this case, we do read it when we shouldn't: `c = priv->rxfifo[priv->rxdmanext];`
This is the incorrect character being read, which would cause the CRC errors im seeing, right?
I'm sending at least 230 bytes every 10ms, will update you with solid numbers when i have a chance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko merged PR #11020:
URL: https://github.com/apache/nuttx/pull/11020
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372665663
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
@danielappiagyei-bc
I dusted off a 1062 board MCU: i.MX RT1062 r1.1, rev. 1
I added
```
diff --git a/arch/arm/src/imxrt/imxrt_serial.c b/arch/arm/src/imxrt/imxrt_serial.c
index 4d344f4815..14165aca10 100644
--- a/arch/arm/src/imxrt/imxrt_serial.c
+++ b/arch/arm/src/imxrt/imxrt_serial.c
@@ -1056,7 +1056,7 @@ static char g_lpuart8rxbuffer[CONFIG_LPUART8_RXBUFSIZE];
static char g_lpuart8txbuffer[LPUART8_TXBUFSIZE_ADJUSTED] \
LPUART8_TXBUFSIZE_ALGN;
#endif
-
+
#ifdef CONFIG_IMXRT_LPUART9
static char g_lpuart9rxbuffer[CONFIG_LPUART9_RXBUFSIZE];
static char g_lpuart9txbuffer[LPUART9_TXBUFSIZE_ADJUSTED]
@@ -1923,6 +1923,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ ASSERT(dmaresidual != 0);
DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
return RXDMA_BUFFER_SIZE - dmaresidual;
```
and set a break on it failing.
```
nsh> serial_test -c -s -p /dev/ttyS2 -b 3000000
serial test app
/dev/ttyS2: count for this session: rx=30852, tx=60416, rx err=0
/dev/ttyS2: count for this session: rx=975236, tx=1838080, rx err=0
/dev/ttyS2: count for this session: rx=1919620, tx=3615744, rx err=0
/dev/ttyS2: count for this session: rx=2864004, tx=5393408, rx err=0
/dev/ttyS2: count for this session: rx=3807844, tx=7170048, rx err=0
.
... it was running 0 errors but I attached a logic analyze
..
.
/dev/ttyS2: count for this session: rx=345661140, tx=650660864, rx err=362
/dev/ttyS2: count for this session: rx=346605524, tx=652438528, rx err=362
/dev/ttyS2: count for this session: rx=347549908, tx=654216192, rx err=362
/dev/ttyS2: count for this session: rx=348494292, tx=655993856, rx err=362
/dev/ttyS2: count for this session: rx=349438676, tx=657771520, rx err=362
/dev/ttyS2: count for this session: rx=350383060, tx=659549184, rx err=362
/dev/ttyS2: count for this session: rx=351327444, tx=661326848, rx err=362
/dev/ttyS2: count for this session: rx=352271828, tx=663104512, rx err=362
/dev/ttyS2: count for this session: rx=353216212, tx=664882176, rx err=362
/dev/ttyS2: count for this session: rx=354160596, tx=666659840, rx err=362
/dev/ttyS2: count for this session: rx=355104980, tx=668437504, rx err=362
/dev/ttyS2: count for this session: rx=356049364, tx=670215168, rx err=362
/dev/ttyS2: count for this session: rx=356993748, tx=671992832, rx err=362
/dev/ttyS2: count for this session: rx=357937588, tx=673769472, rx err=362
/dev/ttyS2: count for this session: rx=358881972, tx=675547136, rx err=362
/dev/ttyS2: count for this session: rx=359826356, tx=677324800, rx err=362
/dev/ttyS2: count for this session: rx=360770756, tx=679102464, rx err=362
/dev/ttyS2: count for this session: rx=361715140, tx=680880128, rx err=362
/dev/ttyS2: count for this session: rx=362659524, tx=682657792, rx err=362
```
Running for 2340 Second - no breakpoint
Yellow TX->PC
Green CTS<-PC
Orange TX->PC
Blue RTS->PC
There are ~544 characters per RTS-
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372179887
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
> agree that this scenario can and does happen
I agree - HW wise you can try this?
Use putty on the remotes side with an FTDI to the port.
Place a break in imxrt_serial.c [line: 1925] the call to ` int dmaresidual = imxrt_dmach_getcount(priv->rxdma);`
reboot nuttx
On the nuttx side `nsh> cat /dev/ttySn` where n is your port.
hit a 'U' on the putty side.
the break point will not be hit until you hit a char on the putty side
step into .
I get
<img width="1546" alt="image" src="https://github.com/apache/nuttx/assets/1945821/6568060d-c3a3-42ed-aceb-85bdef9b5310">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1374408499
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
@danielappiagyei-bc - I finally was able to force the condition. PR is https://github.com/apache/nuttx/pull/11070
There is a race, which I would have expected to see more on the faster CPU...but I forced it....While sending data to the UART. I added a loop in imxrt_dma_nextrx to re-call `imxrt_dmach_getcount()` N times and check for 0. I then could break on it and see the DMA as done, until I stepped and it reloaded. Thank you for reporting it and you patience!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#issuecomment-1779250837
Let's merge this change and continue improvements in the next PRs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "jturnsek (via GitHub)" <gi...@apache.org>.
jturnsek commented on PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#issuecomment-1916196596
@davids5 I am working on imxrt117x where I also have found some problems with edma. I am using edma with Flexio driver to get ADC samples in. My driver is using the same logic as imxrt_serial rxdma logic and SOMETIMES I am getting corrupted data. Especially when the system is stressed. What I have found out so far is that DONE flag is sometimes not set when it should be., although interrupt is triggered. Because I am relying on DONE flag passed to my callback function, I get wrong context in. If the system is not stressed, DONE flag is set every done-interrupt, half-interrupt has DONE flag set to zero. Looking further with the debugger, I've noticed that when DONE flag was not set, destination address was a few words ahead. This lead me to conclusion that my interrupt routine servicing is delayed a little bit and eDMA ran a little bit further, activating new major loop. From the documentation:
_Channel Done
This field indicates whether the eDMA has completed the major loop. The eDMA engine sets the value of
this field to 1 when the CITER count reaches zero. The value of this field is reset to 0 by the hardware
(when the channel is activated) or by software._
one can see that DONE flag can be reset by hardware. Because imxrt_edma is checking DONE status quite a few times and is also influencing on imxrt_dmach_getcount which is then used in imxrt_serial, maybe this could also be a reason to get missing characters. What do you think? I think imxrt_edma driver should be revised here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372179887
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
I agree - HW wise you can try this?
Use putty on the remotes side with an FTDI to the port.
Place a break in imxrt_serial.c [line: 1925] the call to ` int dmaresidual = imxrt_dmach_getcount(priv->rxdma);`
reboot nuttx
On the nuttx side `nsh> cat /dev/ttySn` where n is your port.
hit a 'U' on the putty side.
the break point will not be hit until you hit a char on the putty side
step into .
I get
<img width="1546" alt="image" src="https://github.com/apache/nuttx/assets/1945821/6568060d-c3a3-42ed-aceb-85bdef9b5310">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372179887
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
Updated!
> agree that this scenario can and does happen
I agree - HW wise you can try this? - it is a simple statc test that should answer the "Do we see different" things on our HW.
Use putty on the remotes side with an FTDI to the port.
Place a break in imxrt_serial.c [line: 1925] the call to ` int dmaresidual = imxrt_dmach_getcount(priv->rxdma);`
reboot nuttx
On the nuttx side `nsh> cat /dev/ttySn` where n is your port.
hit a 'U' on the putty side.
the break point will not be hit until you hit a char on the putty side
step into .
I get
<img width="1546" alt="image" src="https://github.com/apache/nuttx/assets/1945821/6568060d-c3a3-42ed-aceb-85bdef9b5310">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372182563
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
maybe the 1170 and the 1060 are acting different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#issuecomment-1778126328
Under "Impact" where it says "There is dropped data at high baud rates" do you mean that this change _fixes_ the dropped data at high baud rates, or do you mean that data is continues to be dropped at high baud rates despite the fix? (Not saying that the latter should block merging; in fact, it's probably better to merge a change that improves things, even if not perfect yet.) Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "danielappiagyei-bc (via GitHub)" <gi...@apache.org>.
danielappiagyei-bc commented on PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#issuecomment-1777918843
excellent findings david, im sure this took a lot of hardcore debugging! thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "danielappiagyei-bc (via GitHub)" <gi...@apache.org>.
danielappiagyei-bc commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1370757990
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
I like this DEBUGASSERT but I would like to change it to `DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE && dmaresidual > 0`). I could have sworn `imxrt_dmach_getcount()` returned 0 before; I will test again today and let you know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "danielappiagyei-bc (via GitHub)" <gi...@apache.org>.
danielappiagyei-bc commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372153749
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
I'd also like to add that on other chips that you have a fix similar to mine on kinetis chips, so you may have ran into this same issue in the past:
- from 2 years ago: https://github.com/apache/nuttx/blame/master/arch/arm/src/kinetis/kinetis_lpserial.c#L1665
- from 5 years ago: https://github.com/apache/nuttx/blame/master/arch/arm/src/kinetis/kinetis_serial.c#L1834
so to be clear, we shouldn't add the `!= 0` to the debugassert, we should instead add your fix:
```
return (RXDMA_BUFFER_SIZE - (int)dmaresidual) % RXDMA_BUFFER_SIZE;
```
, or my equivalent one I posted above.
I can do so in a separate PR but I want to make sure we're on the same page and both agree that this scenario can and does happen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372197146
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
@danielappiagyei-bc - I updated https://github.com/apache/nuttx/pull/11020#discussion_r1372179887 also the optimization may make the branch look line it is taking the `then of if ((regval16 & EDMA_TCD_CITER_ELINK) != 0)'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#issuecomment-1916759223
@jturnsek @davids5 To handle high load, maybe this driver needs a DMA page-flipping implementation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "danielappiagyei-bc (via GitHub)" <gi...@apache.org>.
danielappiagyei-bc commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1374901557
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
Thanks @davids5,
I appreciate you taking the time to verify that the issue existed and wasn't just a bug on my end :pray:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1374203882
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s *priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
@danielappiagyei-bc
I think the % operations is constant time and branch less. That would be my preferred solution it if is needed.
I am not convinced it is the correct thing to do yet, I would like to understand the root cause because it may be hiding a greater problem.
A few questions:
1. Are you opening and closing the port between messages?
2. Did you test with 1 char idle timeout and the fifo buffer size of 32?
3. Do you run with a separate interrupt stack?
4. What is you current stack pentation?
If you configure NuttX with the debug options for debugging hardfault and post the output when the assertion fires the task list and stacks usage will be show,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#issuecomment-1916687167
@jturnsek https://github.com/apache/nuttx/commit/6ad7b82cd67e4a2c392419d3e21398ae21495471 changed this behavior.
See the addition of imxrt_dmach_idle arch/arm/src/imxrt/imxrt_edma.c 9bb1666cbd0ef06990d9a5c015a040ce652b8549 and Ensuring the DMA Stop (called off a DMA completion) did not invoke the DMA completion callback again 9bb1666cbd0ef06990d9a5c015a040ce652b8549
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "jturnsek (via GitHub)" <gi...@apache.org>.
jturnsek commented on PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#issuecomment-1916863382
> @jturnsek [6ad7b82](https://github.com/apache/nuttx/commit/6ad7b82cd67e4a2c392419d3e21398ae21495471) changed this behavior. See the addition of imxrt_dmach_idle arch/arm/src/imxrt/imxrt_edma.c [9bb1666](https://github.com/apache/nuttx/commit/9bb1666cbd0ef06990d9a5c015a040ce652b8549) and Ensuring the DMA Stop (called off a DMA completion) did not invoke the DMA completion callback again [9bb1666](https://github.com/apache/nuttx/commit/9bb1666cbd0ef06990d9a5c015a040ce652b8549)
@davids5 Fix was imxrt_serial related, yes. But my point was that we have an imxrt_edma driver calling callback function and passing DONE status to it, which my not be passed every time. If eDMA is set as it is in imxrt_serial driver (and my driver), API is not working or one could not rely on done argument.
@hartmannathan Of course I agree with you. We should look how to simplify use of adriver in case of high load situations, becuase this is the main reason for DMA to exist.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] imxrt:serial Ensure the cache is updated if the DMA has updated again [nuttx]
Posted by "jturnsek (via GitHub)" <gi...@apache.org>.
jturnsek commented on PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#issuecomment-1917196839
> @jturnsek How would you like to change it?
I think it would be better to use different method for detection of DONE, in case of LOOPBACK destination enabled. Maybe somehow with CITER checking or similar? I do not have a viable solution yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org