> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Sent: Wednesday, May 10, 2023 5:34 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 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@xxxxxxx>
>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
>
> 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;
