> -----Original Message----- > From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> > Sent: Thursday, May 4, 2023 8:02 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: RE: [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 Wed, 3 May 2023, Shenwei Wang wrote: > > From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> > > > 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". > > That, however, is unfortunately not the case I was interested in here. The code > does restart the timer if new characters _were received_ (residue changed), no? > So my request for clarification to the changelong still stands, why is rearming > the timer necessary instead of simply setting a longer timeout right from the > start? > Once new characters are received within the timer interval, the "residue" will be updated by the DMA driver, but the DMA transfer will only complete once it receives the specified number of characters. The purpose of this patch is to prevent the long delay of the DMA transfer when it cannot receive a sufficient number of characters. > (In the first paragraph, you stated the problem is about timer triggering > prematurely with low baud rates.) > > This is not to say that the new approach is wrong but the changelog fails to > explain all facets of what is wrong with the old approach adequately. > The timer is used here to simulate the EOP behavior. This means that when a new character is received within the interval, the timer needs restart. If no new character is received within the specified interval, the timer callback will complete the DMA transfer as if an EOP event occurred. The old approach did not exhibit this expected behavior. It would complete the DMA transfer at the fixed interval regardless of whether new characters were received or not. This incorrect behavior can be easily observed at low baud rates, but it also for high baud rates. > > 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. > > Okay. You might also find CIRC_CNT_TO_END() useful in those inner functions > to calculate the length before the wrap. > > > > > + > > > > + /* 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. > > What about this part? If the transfer always does n chars, the left over residue > can match spuriously for the new transfer and trigger the copy because last and > current residue happen to match (kinda by chance but could be simply due to > repetitive transfer pattern)? > Yes, there is a chance. Will reset it when start_rx_dma. Thanks, Shenwei > > > > + 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. > > Yes, please use ->frame_time. I added it exactly to allow this kind of calculations > to be easily based on actual frame timing (the other questions were just to > gauge if I understood right what is behind your math). > > > -- > i.
