diff mbox series

[v4,1/1] tty: serial: fsl_lpuart: optimize the timer based EOP logic

Message ID 20230509153624.1073946-1-shenwei.wang@nxp.com
State New
Headers show
Series [v4,1/1] tty: serial: fsl_lpuart: optimize the timer based EOP logic | expand

Commit Message

Shenwei Wang May 9, 2023, 3:36 p.m. UTC
DMA transfer may end prematurely due to the DMA Rx timeout even during an
active transfer because a constant timeout does not accurately simulate an
EOP event. The timer should only complete a DMA transfer once the idle
period satisfies a specified interval which is baud rate dependant.
The problem has been observed with low baud rates but could occur also
with high baud rates.

This patch uses a timer to simulate the hardware EOP (End Of Package) event.
The idea is to make the DMA Rx timeout baud rate dependent and check the DMA
residue count before copying data to the TTY buffer. If the residue count
remains unchanged since the last interrupt, that indicates no new data was
received. In this case, the DMA should complete as an EOP event. Otherwise,
new data was received during the interval and the EOP condition is not met
so restart the DMA Rx timeout

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
V4:
  - improve the patch comments per Ilpo's suggestion.
  - use nsecs_to_jiffies

V3:
  - change the rx_watermark from 8 to 7 because the dma request is
    generated when the fifo level is greater than this value.

V2:
  - this version is to address the review feedback from Ilpo.
  - reset the last_residue when rx dma starts.
  - simplified the character counting in the RX circular buffer.
  - use max_t() and DIV_ROUND_UP()

 drivers/tty/serial/fsl_lpuart.c | 50 ++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 4 deletions(-)

--
2.34.1

Comments

Shenwei Wang May 10, 2023, 1:51 p.m. UTC | #1
> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Wednesday, May 10, 2023 5:34 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; linux-serial <linux-serial@vger.kernel.org>;
> imx@lists.linux.dev; dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH v4 1/1] tty: serial: fsl_lpuart: optimize the timer based
> EOP logic
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> On Tue, 9 May 2023, Shenwei Wang wrote:
> 
> > DMA transfer may end prematurely due to the DMA Rx timeout even during
> > an active transfer because a constant timeout does not accurately
> > simulate an EOP event. The timer should only complete a DMA transfer
> > once the idle period satisfies a specified interval which is baud rate dependant.
> > The problem has been observed with low baud rates but could occur also
> > with high baud rates.
> >
> > This patch uses a timer to simulate the hardware EOP (End Of Package) event.
> 
> You could open EOP at the previous paragraph where it appears first.
> 

That sounds more suitable. 

> > The idea is to make the DMA Rx timeout baud rate dependent and check
> > the DMA
> 
> Just remove this "the idea is to" and just state what was done by starting with
> "Make" like I initially said.
> 

Agree.

> > residue count before copying data to the TTY buffer. If the residue
> > count remains unchanged since the last interrupt, that indicates no
> > new data was received. In this case, the DMA should complete as an EOP
> > event. Otherwise, new data was received during the interval and the
> > EOP condition is not met so restart the DMA Rx timeout
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> However, please check a few additional comments below.
> 
> > ---
> > V4:
> >   - improve the patch comments per Ilpo's suggestion.
> > -     .rx_watermark = 31,
> > +     .rx_watermark = 7, /* A lower watermark is ideal for low baud
> > + rates. */
> 
> It would be better to have this change in own patch. It might be something that
> causes discussion/problems later on so it would be better to have it as a
> separate patch. But I'm not sure how tightly it's related to
> DMA_RX_IDLE_CHARS which may make taking it out harder.
> 

You are right. The rx_watermark here should no more than the DMA_RX_IDLE_CHARS.

Thanks,
Shenwei

> >  };
> >  static struct lpuart_soc_data imxrt1050_data = {
> >       .devtype = IMXRT1050_LPUART,
> > @@ -1255,6 +1257,8 @@ static void lpuart_copy_rx_to_tty(struct lpuart_port
> *sport)
> >               sport->port.icount.rx += copied;
> >       }
> >
> > +     sport->last_residue = state.residue;
diff mbox series

Patch

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index c91916e13648..f21920024618 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -238,6 +238,7 @@ 

 /* Rx DMA timeout in ms, which is used to calculate Rx ring buffer size */
 #define DMA_RX_TIMEOUT		(10)
+#define DMA_RX_IDLE_CHARS	(8)
 #define UART_AUTOSUSPEND_TIMEOUT	3000

 #define DRIVER_NAME	"fsl-lpuart"
@@ -282,6 +283,7 @@  struct lpuart_port {
 	struct scatterlist	rx_sgl, tx_sgl[2];
 	struct circ_buf		rx_ring;
 	int			rx_dma_rng_buf_len;
+	int                     last_residue;
 	unsigned int		dma_tx_nents;
 	wait_queue_head_t	dma_wait;
 	bool			is_cs7; /* Set to true when character size is 7 */
@@ -331,7 +333,7 @@  static struct lpuart_soc_data imx8qxp_data = {
 	.devtype = IMX8QXP_LPUART,
 	.iotype = UPIO_MEM32,
 	.reg_off = IMX_REG_OFF,
-	.rx_watermark = 31,
+	.rx_watermark = 7, /* A lower watermark is ideal for low baud rates. */
 };
 static struct lpuart_soc_data imxrt1050_data = {
 	.devtype = IMXRT1050_LPUART,
@@ -1255,6 +1257,8 @@  static void lpuart_copy_rx_to_tty(struct lpuart_port *sport)
 		sport->port.icount.rx += copied;
 	}

+	sport->last_residue = state.residue;
+
 exit:
 	dma_sync_sg_for_device(chan->device->dev, &sport->rx_sgl, 1,
 			       DMA_FROM_DEVICE);
@@ -1272,11 +1276,37 @@  static void lpuart_dma_rx_complete(void *arg)
 	lpuart_copy_rx_to_tty(sport);
 }

+/*
+ * Timer function to simulate the hardware EOP (End Of Package) event.
+ * The timer callback is to check for new RX data and copy to TTY buffer.
+ * If no new data are received since last interval, the EOP condition is
+ * met, complete the DMA transfer by copying the data. Otherwise, just
+ * restart timer.
+ */
 static void lpuart_timer_func(struct timer_list *t)
 {
 	struct lpuart_port *sport = from_timer(sport, t, lpuart_timer);
+	struct dma_chan *chan = sport->dma_rx_chan;
+	struct circ_buf *ring = &sport->rx_ring;
+	struct dma_tx_state state;
+	unsigned long flags;
+	int count;

-	lpuart_copy_rx_to_tty(sport);
+	dmaengine_tx_status(chan, sport->dma_rx_cookie, &state);
+	ring->head = sport->rx_sgl.length - state.residue;
+	count = CIRC_CNT(ring->head, ring->tail, sport->rx_sgl.length);
+
+	/* Check if new data received before copying */
+	if ((count != 0) && (sport->last_residue == state.residue))
+		lpuart_copy_rx_to_tty(sport);
+	else
+		mod_timer(&sport->lpuart_timer,
+				jiffies + sport->dma_rx_timeout);
+
+	if (spin_trylock_irqsave(&sport->port.lock, flags)) {
+		sport->last_residue = state.residue;
+		spin_unlock_irqrestore(&sport->port.lock, flags);
+	}
 }

 static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
@@ -1297,9 +1327,20 @@  static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
 	 */
 	sport->rx_dma_rng_buf_len = (DMA_RX_TIMEOUT * baud /  bits / 1000) * 2;
 	sport->rx_dma_rng_buf_len = (1 << fls(sport->rx_dma_rng_buf_len));
+	sport->rx_dma_rng_buf_len = max_t(int,
+			sport->rxfifo_size * 2,
+			sport->rx_dma_rng_buf_len);
+	/*
+	 * Keep this condition check in case rxfifo_size is unavailable
+	 * for some SoCs.
+	 */
 	if (sport->rx_dma_rng_buf_len < 16)
 		sport->rx_dma_rng_buf_len = 16;

+	sport->last_residue = 0;
+	sport->dma_rx_timeout = max(nsecs_to_jiffies(
+		sport->port.frame_time * DMA_RX_IDLE_CHARS), 1UL);
+
 	ring->buf = kzalloc(sport->rx_dma_rng_buf_len, GFP_ATOMIC);
 	if (!ring->buf)
 		return -ENOMEM;
@@ -1687,12 +1728,13 @@  static void lpuart_rx_dma_startup(struct lpuart_port *sport)
 	if (!sport->dma_rx_chan)
 		goto err;

+	/* set default Rx DMA timeout */
+	sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
+
 	ret = lpuart_start_rx_dma(sport);
 	if (ret)
 		goto err;

-	/* set Rx DMA timeout */
-	sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
 	if (!sport->dma_rx_timeout)
 		sport->dma_rx_timeout = 1;