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? (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. > 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)? > > > + 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.
