Hi
adding Phil
Op 12-06-2024 om 15:13 schreef Ilpo Järvinen:
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?
Currently I have this working on mrfld:
diff --git a/drivers/tty/serial/8250/8250_dma.c
b/drivers/tty/serial/8250/8250_dma.c
index 8a353e3cc3dd..d215c494ee24 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -89,7 +89,9 @@ int serial8250_tx_dma(struct uart_8250_port *p)
struct tty_port *tport = &p->port.state->port;
struct dma_async_tx_descriptor *desc;
struct uart_port *up = &p->port;
- struct scatterlist sg;
+ struct scatterlist *sg;
+ struct scatterlist sgl[2];
+ int i;
int ret;
if (dma->tx_running) {
@@ -110,18 +112,17 @@ int serial8250_tx_dma(struct uart_8250_port *p)
serial8250_do_prepare_tx_dma(p);
- sg_init_table(&sg, 1);
- /* kfifo can do more than one sg, we don't (quite yet) */
- ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
+ sg_init_table(sgl, ARRAY_SIZE(sgl));
+
+ ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, sgl,
ARRAY_SIZE(sgl),
UART_XMIT_SIZE, dma->tx_addr);
- /* we already checked empty fifo above, so there should be something */
- if (WARN_ON_ONCE(ret != 1))
- return 0;
+ dma->tx_size = 0;
- dma->tx_size = sg_dma_len(&sg);
+ for_each_sg(sgl, sg, ret, i)
+ dma->tx_size += sg_dma_len(sg);
- desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1,
+ desc = dmaengine_prep_slave_sg(dma->txchan, sgl, ret,
DMA_MEM_TO_DEV,
DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
if (!desc) {
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
RZN1
I think you are referring to aa63d786cea2 ("serial: 8250: dw: Add
support for DMA flow controlling devices") by
Phil Edworthy<phil.edworthy@xxxxxxxxxxx>?
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
I see dw8250_prepare_tx_dma() has RZN1_UART_xDMACR_BLK_SZ(dma->tx_size)
is okay if it gets more than 1 sg entry (at worst, a HW-specific limit
on nents might need to be imposed).
And is there a way to get the maximum nents supported? I thought
kfifo_dma_out_prepare_mapped() would return a safe number.
