Date: Sun, 16 Jun 2024 22:55:13 +0200
From: Ferry Toth <fntoth@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Ferry Toth <fntoth@...il.com>
Cc: Jiri Slaby <jirislaby@...nel.org>, neil.armstrong@...aro.org,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 linux-serial <linux-serial@...r.kernel.org>,
 LKML <linux-kernel@...r.kernel.org>, Al Cooper <alcooperx@...il.com>,
 Alexander Shiyan <shc_work@...l.ru>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Alim Akhtar <alim.akhtar@...sung.com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 "Aneesh Kumar K.V" <aneesh.kumar@...nel.org>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Baolin Wang <baolin.wang@...ux.alibaba.com>, Baruch Siach
 <baruch@...s.co.il>, Bjorn Andersson <andersson@...nel.org>,
 Claudiu Beznea <claudiu.beznea@...on.dev>,
 "David S. Miller" <davem@...emloft.net>, Fabio Estevam <festevam@...il.com>,
 Hammer Hsieh <hammerh0314@...il.com>,
 Christian König <christian.koenig@....com>,
 Christophe Leroy <christophe.leroy@...roup.eu>,
 Chunyan Zhang <zhang.lyra@...il.com>, Jerome Brunet <jbrunet@...libre.com>,
 Jonathan Hunter <jonathanh@...dia.com>, Kevin Hilman <khilman@...libre.com>,
 Konrad Dybcio <konrad.dybcio@...aro.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Kumaravel Thiagarajan <kumaravel.thiagarajan@...rochip.com>,
 Laxman Dewangan <ldewangan@...dia.com>,
 linux-arm-kernel@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
 "Maciej W. Rozycki" <macro@...am.me.uk>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 Matthias Brugger <matthias.bgg@...il.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Michael Ellerman <mpe@...erman.id.au>, Michal Simek <michal.simek@....com>,
 "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
 Nicolas Ferre <nicolas.ferre@...rochip.com>,
 Nicholas Piggin <npiggin@...il.com>, Orson Zhai <orsonzhai@...il.com>,
 Pali Rohár <pali@...nel.org>,
 Patrice Chotard <patrice.chotard@...s.st.com>,
 Peter Korsgaard <jacmet@...site.dk>,
 Richard Genoud <richard.genoud@...il.com>,
 Russell King <linux@...linux.org.uk>, Sascha Hauer <s.hauer@...gutronix.de>,
 Shawn Guo <shawnguo@...nel.org>, Stefani Seibold <stefani@...bold.net>,
 Sumit Semwal <sumit.semwal@...aro.org>,
 Taichi Sugaya <sugaya.taichi@...ionext.com>,
 Takao Orito <orito.takao@...ionext.com>,
 Tharun Kumar P <tharunkumar.pasumarthi@...rochip.com>,
 Thierry Reding <thierry.reding@...il.com>, Timur Tabi <timur@...nel.org>,
 Vineet Gupta <vgupta@...nel.org>, Marek Szyprowski
 <m.szyprowski@...sung.com>, Phil Edworthy <phil.edworthy@...esas.com>
Subject: Re: [PATCH 00/15] tty: serial: switch from circ_buf to kfifo

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@kernel.org/
>>>> 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@...sung.com> 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@...esas.com>?

> 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.

Powered by blists - more mailing lists