> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Sent: Wednesday, May 3, 2023 4:03 AM
> To: Shenwei Wang <shenwei.wang@xxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jiri Slaby
> <jirislaby@xxxxxxxxxx>; linux-serial <linux-serial@xxxxxxxxxxxxxxx>;
> imx@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> Subject: [EXT] Re: [PATCH 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, 2 May 2023, Shenwei Wang wrote:
>
> > At low baud rates, the DMA transfer may end prematurely due to the
> > timer, even during an active transfer. This does not accurately
> > simulate an EOP event as intended. We expect the timer to only
> > complete a DMA transfer once the idle period satisfies a specified interval.
> >
> > The patch checks 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. Instead, the timer restarts.
>
> This description is lacking something. It does not explain why the stuff in second
> paragraph is necessary at all as setting a longer timer based on the (lower) baud
> rate would avoid the need to do the timer restart.
>
Agree. Would add the following to the last sentence: "if no new characters are received, the timer just restarts".
> > Signed-off-by: Shenwei Wang <shenwei.wang@xxxxxxx>
> > ---
> > drivers/tty/serial/fsl_lpuart.c | 52
> > ++++++++++++++++++++++++++++++---
> > 1 file changed, 48 insertions(+), 4 deletions(-)
> > }
> >
> > +/*
> > + * Timer function to simulate the hardware EOP(End Of Package) event.
>
> Missing space
>
Will fix it in next version.
> > + * The timer callback is to check for new RX data and copy to TTY buffer.
> > + * If no new data since last interrupt, restart timer. Otherwise,
> > + copy data
> > + * and continue normal logic.
> > + */
> > 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 = 0;
> >
> > - lpuart_copy_rx_to_tty(sport);
> > + dmaengine_tx_status(chan, sport->dma_rx_cookie, &state);
>
> > + ring->head = sport->rx_sgl.length - state.residue;
> > +
> > + if (ring->head < ring->tail)
> > + count = sport->rx_sgl.length - ring->tail;
> > + else if (ring->tail < ring->head)
> > + count = ring->head - ring->tail;
>
> linux/circ_buf.h has functions which likely handle what you want to do here.
> They will get you true count across wrap too which this above does not do.
>
> Given this is essentially duplicates count calculation some refactor would seem
> more useful here rather than recalculating the count again in
> lpuart_copy_rx_to_tty().
>
> Also lpuart_handle_sysrq() duplicates the same calculations.
>
Just had a check with the circ_buf.h, and this piece of codes can be simplified
with the CIRC_CNT() function.
The other part you mentioned should be optimized as well. I will submit a separate
patch to do that after finishing this one.
> > +
> > + /* Check if new data received before copying */
> > + if ((count != 0) && (sport->last_residue == state.residue))
>
> I'm unsure about this condition being right.
>
> What will happen when rx_sgl.length (or -1 of that, I'm not sure which way "full
> size" is here) worth of data has been DMA'ed. Does this condition end up
> delaying copy such that it's done only on every other call here?
>
The timer function will only complete a DMA transfer when there is new data in the buffer
and the data has been idle for the specified interval.
The "full buffer" situation should be handled by the DMA completion callback itself. A "full" buffer
means the DMA transaction has received sufficient data and invoked the completion callback.
> Also, should you reset last_residue in lpuart_start_rx_dma() ? I think that would
> solve the "full size" problem.
>
> > + 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 +1330,19 @@ 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));
> > + if (sport->rx_dma_rng_buf_len < sport->rxfifo_size * 2)
> > + sport->rx_dma_rng_buf_len = sport->rxfifo_size * 2;
>
> max_t()
>
> > +
> > + /*
> > + * 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->dma_rx_timeout =
> > + msecs_to_jiffies((1000 * 10 * DMA_RX_IDLE_CHARS) / baud
> > + + 1);
>
> There's ->frame_time these days in uart_port which you should base frame
> timing related calculations. I wouldn't mind if that existing ->frame_time math
> that is visible in your patch's context is also converted (in a separate patch).
>
> I'm assuming that magic 10 is assumed number of bits and 1000 MSEC_PER_SEC.
> That +1 seems odd, did you mean DIV_ROUND_UP() ?
Yes, it is 10 bits and 1000 ms. +1 here is similar to the result of round up.
And also the ->frame_time could be used for simplicity.
>
> > +
> > ring->buf = kzalloc(sport->rx_dma_rng_buf_len, GFP_ATOMIC);
> > if (!ring->buf)
> > return -ENOMEM;
> > @@ -1687,12 +1730,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;
> >
> >
>
> --
> i.
