> From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> > ---
> > 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.
>
> > + 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.
>
>
> 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.
> - 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.
>
> 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.
>
> > +static int ast8250_handle_irq(struct uart_port *port) {
> > + return serial8250_handle_irq(port, serial_port_in(port, UART_IIR));
> > +}
> > +
> > +static int ast8250_startup(struct uart_port *port) { #ifdef
> > +CONFIG_SERIAL_8250_DMA
>
> This ifdef'fery is entirely unnecessary.
Will remove as suggested.
>
> > + int rc;
> > + struct uart_8250_port *up = up_to_u8250p(port);
>
> Reverse xmas tree order.
Will revise as suggested.
>
> > +
> > + rc = serial8250_do_startup(port);
> > + if (rc)
> > + return rc;
> > +
> > + /*
> > + * The default RX DMA is launched upon rising DR bit.
> > + *
> > + * However, this can result in byte lost if UART FIFO has
> > + * been overruned before the DMA engine gets prepared and
> > + * read the data out. This is especially common when UART
> > + * DMA is used for file transfer. Thus we initiate RX DMA
> > + * as early as possible.
> > + */
> > + if (up->dma)
> > + return ast8250_rx_dma(up);
>
> Once you start using the general 8250 dma code and add the DMA Rx always
> capabilities flag, this can go into serial8250_do_startup(). Since
> serial8250_rx_dma() always exists independent of CONFIG_SERIAL_8250_DMA,
> no include guards are necessary.
>
> ...After which you probably don't need this whole function anymore.
Agree.
>
> > +
> > + return 0;
> > +#else
> > + return serial8250_do_startup(port);
> > +#endif
> > +}
> > +
> > +static void ast8250_shutdown(struct uart_port *port) {
> > + return serial8250_do_shutdown(port); }
> > +
> > +static int ast8250_probe(struct platform_device *pdev) {
> > + int rc;
>
> Put this as last.
Will revise as suggested.
>
> > + struct uart_8250_port uart = {};
> > + struct uart_port *port = &uart.port;
> > + struct device *dev = &pdev->dev;
> > + struct ast8250_data *data;
> > + struct resource *res;
> > +
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->irq = platform_get_irq(pdev, 0);
> > + if (data->irq < 0)
> > + return data->irq;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (res == NULL) {
> > + dev_err(dev, "failed to get register base\n");
> > + return -ENODEV;
> > + }
> > +
> > + data->regs = devm_ioremap(dev, res->start, resource_size(res));
> > + if (IS_ERR(data->regs)) {
> > + dev_err(dev, "failed to map registers\n");
> > + return PTR_ERR(data->regs);
> > + }
> > +
> > + data->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(data->clk)) {
> > + dev_err(dev, "failed to get clocks\n");
> > + return -ENODEV;
> > + }
> > +
> > + rc = clk_prepare_enable(data->clk);
> > + if (rc) {
> > + dev_err(dev, "failed to enable clock\n");
> > + return rc;
> > + }
> > +
> > + data->rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> > + if (!IS_ERR(data->rst))
> > + reset_control_deassert(data->rst);
> > +
> > + spin_lock_init(&port->lock);
> > + port->dev = dev;
> > + port->type = PORT_16550A;
> > + port->irq = data->irq;
> > + port->line = of_alias_get_id(dev->of_node, "serial");
> > + port->handle_irq = ast8250_handle_irq;
> > + port->mapbase = res->start;
> > + port->mapsize = resource_size(res);
> > + port->membase = data->regs;
> > + port->uartclk = clk_get_rate(data->clk);
> > + port->regshift = 2;
> > + port->iotype = UPIO_MEM32;
> > + port->flags = UPF_FIXED_TYPE | UPF_FIXED_PORT | UPF_SHARE_IRQ;
> > + port->startup = ast8250_startup;
> > + port->shutdown = ast8250_shutdown;
> > + port->private_data = data;
>
> > +#ifdef CONFIG_SERIAL_8250_DMA
> > + data->dma.rxconf.src_maxburst = UART_XMIT_SIZE;
> > + data->dma.txconf.dst_maxburst = UART_XMIT_SIZE;
> > + uart.dma = &data->dma;
> > +#endif
>
> Add a setup function for this and make an empty function with the same name
> when CONFIG_SERIAL_8250_DMA is not there.
Will revise as suggested.
>
> > +
> > + data->line = serial8250_register_8250_port(&uart);
> > + if (data->line < 0) {
> > + dev_err(dev, "failed to register 8250 port\n");
> > + return data->line;
> > + }
> > +
> > + platform_set_drvdata(pdev, data);
> > +
> > + return 0;
> > +}
> > +
> > +static int ast8250_remove(struct platform_device *pdev) {
> > + struct ast8250_data *data = platform_get_drvdata(pdev);
> > +
> > + serial8250_unregister_port(data->line);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id ast8250_of_match[] = {
> > + { .compatible = "aspeed,ast2600-uart" },
> > + { },
> > +};
> > +
> > +static struct platform_driver ast8250_platform_driver = {
> > + .driver = {
> > + .name = DEVICE_NAME,
> > + .of_match_table = ast8250_of_match,
> > + },
> > + .probe = ast8250_probe,
> > + .remove = ast8250_remove,
> > +};
> > +
> > +module_platform_driver(ast8250_platform_driver);
> > +
Thanks,
Chiawei
