On Mon, 10 Jun 2024, Ferry Toth wrote: > Op 07-06-2024 om 22:32 schreef Ferry Toth: > > Op 22-04-2024 om 07:51 schreef Jiri Slaby: > > > On 19. 04. 24, 17:12, Neil Armstrong wrote: > > > > On 05/04/2024 08:08, Jiri Slaby (SUSE) wrote: > > > > > This series switches tty serial layer to use kfifo instead of > > > > > circ_buf. > > > > > > > > > > The reasoning can be found in the switching patch in this series: > > > > > """ > > > > > Switch from struct circ_buf to proper kfifo. kfifo provides much > > > > > better > > > > > API, esp. when wrap-around of the buffer needs to be taken into > > > > > account. > > > > > Look at pl011_dma_tx_refill() or cpm_uart_tx_pump() changes for > > > > > example. > > > > > > > > > > Kfifo API can also fill in scatter-gather DMA structures, so it easier > > > > > for that use case too. Look at lpuart_dma_tx() for example. Note that > > > > > not all drivers can be converted to that (like atmel_serial), they > > > > > handle DMA specially. > > > > > > > > > > Note that usb-serial uses kfifo for TX for ages. > > > > > """ > > > Sadly, everyone had a chance to test the series: > > > https://lore.kernel.org/all/20240319095315.27624-1-jirislaby@xxxxxxxxxx/ > > > for more than two weeks before I sent this version for inclusion. And then > > > it took another 5 days till this series appeared in -next. But noone with > > > this HW apparently cared enough back then. I'd wish they (you) didn't. > > > Maybe next time, people will listen more carefully: > > > === > > > This is Request for Testing as I cannot test all the changes > > > (obviously). So please test your HW's serial properly. > > > === > > > > > > > and should've been dropped immediately when the first regressions were > > > > reported. > > > > > > Provided the RFT was mostly ignored (anyone who tested that here, or I > > > only wasted my time?), how exactly would dropping help me finding > > > potential issues in the series? In the end, noone is running -next in > > > production, so glitches are sort of expected, right? And I believe I > > > smashed them quickly enough (despite I was sidetracked to handle the n_gsm > > > issue). But I might be wrong, as usual. > > > > I arrived at this party a bit late, sorry about that. No good excuses. > > > > > So no, dropping is not helping moving forward, actions taken by e.g. Marek > > > Szyprowski <m.szyprowski@xxxxxxxxxxx> do, IMNSHO. > > > > Good news is I tested on Merrifield (Intel Edison) which is slow (500MHz) > > and has a HSU that can transmit up to 3.5Mb/s. It really normally needs DMA > > and just a single interrupt at the end of transmit and receive for which I > > my own patches locally. The bounce buffer I was using on transmit broke due > > to this patch, so I dropped that. Still, with the extra interrupts caused by > > the circ buffer wrapping around it seems to work well. Too late to add my > > Tested-by. > > > > One question though: in 8250_dma.c serial8250_tx_dma() you mention "/* kfifo > > can do more than one sg, we don't (quite yet) */". > > > > I see the opportunity to use 2 sg entries to get all the data out in one dma > > transfer, but there doesn't seem to be much documentation or examples on how > > to do that. It seems just increasing nents to 2 would do the trick? > > Nevertheless I got this to work. Very nice. Thanks for this series. > I am seeing only 2 interrupts (x2 as each interrupt happens twice), one for > dma complete. The 2nd, not sure but likely, uart tx done. > In any case the whole buffer is transferred without interchar gaps. > > > So, what was the reason to "don't (quite yet)"? > > Before considering to send out a patch for this, are there any caveats that > I'm overlooking? Not exactly related to that quoted comment, but you should Cc the person who added RNZ1 DMA a year or two back (in 8250_dw.c) because it required writing Tx length into some custom register. I don't know the meaning of that HW specific register so it would be good to get confirmation the HW is okay if it gets more than 1 sg entry (at worst, a HW-specific limit on nents might need to be imposed). -- i.
