From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: ChiaWei Wang <chiawei_wang@aspeedtech.com>
Cc: "vkoul@kernel.org" <vkoul@kernel.org>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"krzysztof.kozlowski+dt@linaro.org"
<krzysztof.kozlowski+dt@linaro.org>,
"joel@jms.id.au" <joel@jms.id.au>,
"andrew@aj.id.au" <andrew@aj.id.au>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jirislaby@kernel.org>,
"pmenzel@molgen.mpg.de" <pmenzel@molgen.mpg.de>,
"hdanton@sina.com" <hdanton@sina.com>,
"dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
LKML <linux-kernel@vger.kernel.org>,
linux-serial <linux-serial@vger.kernel.org>,
"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>
Subject: RE: [PATCH v3 4/5] serial: 8250: Add AST2600 UART driver
Date: Mon, 20 Mar 2023 12:48:58 +0200 (EET) [thread overview]
Message-ID: <a5b0d7d8-d78a-a12f-783-419713742d5@linux.intel.com> (raw)
In-Reply-To: <KL1PR0601MB37819E400753132F11F0202D91809@KL1PR0601MB3781.apcprd06.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 6213 bytes --]
On Mon, 20 Mar 2023, ChiaWei Wang wrote:
> > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Sent: Monday, March 20, 2023 5:43 PM
> >
> > On Mon, 20 Mar 2023, Chia-Wei Wang wrote:
> >
> > > Add new UART driver with DMA support for Aspeed AST2600 SoCs.
> > > The drivers mainly prepare the dma instance based on the 8250_dma
> > > implementation to leverage the AST2600 UART DMA (UDMA) engine.
> > >
> > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> > > ---
> > > drivers/tty/serial/8250/8250_aspeed.c | 224
> > ++++++++++++++++++++++++++
> > > drivers/tty/serial/8250/Kconfig | 8 +
> > > drivers/tty/serial/8250/Makefile | 1 +
> > > 3 files changed, 233 insertions(+)
> > > create mode 100644 drivers/tty/serial/8250/8250_aspeed.c
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_aspeed.c
> > > b/drivers/tty/serial/8250/8250_aspeed.c
> > > new file mode 100644
> > > index 000000000000..04d0bf6fba28
> > > --- /dev/null
> > > +++ b/drivers/tty/serial/8250/8250_aspeed.c
> > > @@ -0,0 +1,224 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) ASPEED Technology Inc.
> > > + */
> > > +#include <linux/device.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/serial_8250.h>
> > > +#include <linux/serial_reg.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/reset.h>
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/circ_buf.h>
> > > +#include <linux/tty_flip.h>
> > > +#include <linux/pm_runtime.h>
> > > +
> > > +#include "8250.h"
> > > +
> > > +#define DEVICE_NAME "aspeed-uart"
> > > +
> > > +struct ast8250_data {
> > > + int line;
> > > + int irq;
> > > + u8 __iomem *regs;
> > > + struct reset_control *rst;
> > > + struct clk *clk;
> > > +#ifdef CONFIG_SERIAL_8250_DMA
> > > + struct uart_8250_dma dma;
> > > +#endif
> > > +};
> > > +
> > > +#ifdef CONFIG_SERIAL_8250_DMA
> > > +static int ast8250_rx_dma(struct uart_8250_port *p);
> > > +
> > > +static void ast8250_rx_dma_complete(void *param) {
> > > + struct uart_8250_port *p = param;
> > > + struct uart_8250_dma *dma = p->dma;
> > > + struct tty_port *tty_port = &p->port.state->port;
> > > + struct dma_tx_state state;
> > > + int count;
> > > +
> > > + dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> > > +
> > > + count = dma->rx_size - state.residue;
> > > +
> > > + tty_insert_flip_string(tty_port, dma->rx_buf, count);
> > > + p->port.icount.rx += count;
> > > +
> > > + tty_flip_buffer_push(tty_port);
> > > +
> > > + ast8250_rx_dma(p);
> > > +}
> > > +
> > > +static int ast8250_rx_dma(struct uart_8250_port *p) {
> > > + struct uart_8250_dma *dma = p->dma;
> > > + struct dma_async_tx_descriptor *tx;
> > > +
> > > + tx = dmaengine_prep_slave_single(dma->rxchan, dma->rx_addr,
> > > + dma->rx_size, DMA_DEV_TO_MEM,
> > > + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> > > + if (!tx)
> > > + return -EBUSY;
> >
> > How does the DMA Rx "loop" restart when this is taken?
>
> The loop re-starts from ast8250_startup.
Why would startup get called again?
> > > + tx->callback = ast8250_rx_dma_complete;
> > > + tx->callback_param = p;
> > > +
> > > + dma->rx_cookie = dmaengine_submit(tx);
> > > +
> > > + dma_async_issue_pending(dma->rxchan);
> > > +
> > > + return 0;
> > > +}
> > > +#endif
> >
> > These 2 functions look very similar to what 8250_dma offers for you. The only
> > difference I could see is that always start DMA Rx thing which could be
> > handled by adding some capability flag into uart_8250_dma for those UARTs
> > that can launch DMA Rx while Rx queue is empty.
> >
> > So, just use the standard 8250_dma functions and make the small capabilities
> > flag tweak there.
> >
> > By using the stock functions you also avoid 8250_dma Rx and your DMA Rx
> > racing like they currently would (8250_port assigns the functions from
> > 8250_dma when you don't specify the rx handler and the default 8250 irq
> > handler will call into those standard 8250 DMA functions).
>
> Yes for the difference described.
>
> Our customers usually use UDMA for file-transmissions over UART.
> And I found the preceding bytes will get lost easily due to the late
> start of DMA engine.
>
> In fact, I was seeking the default implementation to always start RX DMA
> instead of enabling it upon DR bit rising. But no luck and thus add
> ast8250_rx_dma. (The default 8250 ISR also called into up->dma->rx_dma)
>
> If adding a new capability flag is the better way to go, I will try to
> implement in that way for further review.
Yes it would be much better.
Add unsigned int capabilities into uart_8250_dma and put the necessary
checks + code into general code. Don't add any #ifdef
CONFIG_SERIAL_8250_DMA into 8250_port.c nor 8250_dma.c. Instead, if you
feel a need for one, use the #ifdef ... #else ... #endif in 8250.h to
provide an empty static inline function for the #else case.
> > I'm curious about this HW and how it behaves under these two scenarios:
> > - When Rx is empty, does UART/DMA just sit there waiting forever?
>
> Yes.
Okay.
> > - When a stream of incoming Rx characters suddenly ends, how does
> > UART/DMA
> > react? ...On 8250 UARTs I'm familiar with this triggers UART_IIR_TIMEOUT
> > which you don't seem to handle.
>
> UDMA also has a timeout control.
> If the data suddenly ends and timeout occurs, UDMA will trigger an interrupt.
> UDMA ISR then check if there is data available using DMA read/write
> pointers and invokes callback if any.
Okay. And the UART side won't trigger any interrupts?
> > When you provide answer to those two questions, I can try to help you further
> > on how to integrate into the standard 8250 DMA code.
>
> Thanks!
> It would be great using the default one to avoid mostly duplicated code.
You need to take a look into handle_rx_dma() what to do there. Probably
just call to ->rx_dma() unconditionally to prevent UART interrupts from
messing up with DMA Rx. This restart for DMA Rx is just for backup if the
DMA Rx "loop" stopped due to an error.
--
i.
next prev parent reply other threads:[~2023-03-20 10:53 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-20 8:11 [PATCH v3 0/5] arm: aspeed: Add UART DMA support Chia-Wei Wang
2023-03-20 8:11 ` [PATCH v3 1/5] dt-bindings: serial: 8250: Add aspeed,ast2600-uart Chia-Wei Wang
2023-03-20 8:11 ` [PATCH v3 2/5] dt-bindings: dmaengine: Add AST2600 UDMA bindings Chia-Wei Wang
2023-03-21 21:32 ` Rob Herring
2023-03-20 8:11 ` [PATCH v3 3/5] dmaengine: aspeed: Add AST2600 UART DMA driver Chia-Wei Wang
2023-03-20 11:14 ` Ilpo Järvinen
2023-03-22 8:34 ` ChiaWei Wang
2023-03-20 8:11 ` [PATCH v3 4/5] serial: 8250: Add AST2600 UART driver Chia-Wei Wang
2023-03-20 9:42 ` Ilpo Järvinen
2023-03-20 10:19 ` ChiaWei Wang
2023-03-20 10:48 ` Ilpo Järvinen [this message]
2023-03-22 8:34 ` ChiaWei Wang
2023-03-20 8:11 ` [PATCH v3 5/5] ARM: dts: aspeed-g6: Add UDMA node Chia-Wei Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a5b0d7d8-d78a-a12f-783-419713742d5@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=andrew@aj.id.au \
--cc=chiawei_wang@aspeedtech.com \
--cc=devicetree@vger.kernel.org \
--cc=dmaengine@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=hdanton@sina.com \
--cc=jirislaby@kernel.org \
--cc=joel@jms.id.au \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-aspeed@lists.ozlabs.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=openbmc@lists.ozlabs.org \
--cc=pmenzel@molgen.mpg.de \
--cc=robh+dt@kernel.org \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).