Re: [PATCH 2/3] serial: 8250_dw: configure the Allwinner H616 UART

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: 8250_dw: broken H616 RX DMA due to deferred sun6i_dma_issue_pending()
- [High] serial: 8250_dw: missing UART_CAP_AFE in UART_PORT_DW_APB_256 breaks CRTSCTS
--

commit 7b1955806965488d36d1ed6fbef81df345079c9d
Author: James Hilliard <james.hilliard1@gmail.com>

serial: 8250_dw: configure the Allwinner H616 UART

This patch adds a fixed 256-byte FIFO port configuration for the Allwinner
H616 UART, skips optional register probing, sets the SKIP_SET_RATE quirk, and
attaches 8250 DMA with single-byte burst sizes during probe.

> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index eca02e8bd6d13..f03051740be0a 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c

[ ... ]

> @@ -765,14 +780,21 @@ static int dw8250_probe(struct platform_device *pdev)
>  
>  	dw8250_setup_dma_filter(p, data);
>  
> -	if (!data->skip_autocfg)
> -		dw8250_setup_port(p);
> -
> -	/* If we have a valid fifosize, try hooking up DMA */
> -	if (p->fifosize) {
> -		data->data.dma.rxconf.src_maxburst = p->fifosize / 4;
> -		data->data.dma.txconf.dst_maxburst = p->fifosize / 4;
> +	if (data->pdata && data->pdata->type) {
> +		dw8250_setup_port_fixed(up, data->pdata->type);
> +		data->data.dma.rxconf.src_maxburst = data->pdata->dma_maxburst;
> +		data->data.dma.txconf.dst_maxburst = data->pdata->dma_maxburst;
>  		up->dma = &data->data.dma;

[Severity: High]
Will attaching the standard 8250 RX DMA path here work with the Allwinner
sun6i-dma controller, or does it cause RX DMA transfers to be immediately
aborted and leave RX DMA wedged?

When UART_IIR_RDI fires, serial8250_interrupt() loops in hardirq context:

drivers/tty/serial/8250/8250_core.c:serial8250_interrupt() {
	l = i->head;
	do {
		struct uart_8250_port *up = list_entry(l, struct uart_8250_port, list);
		struct uart_port *port = &up->port;

		if (port->handle_irq(port)) {
			handled = 1;
			end = NULL;
		} else if (end == NULL)
			end = l;
		...
	} while (l != end);
}

On the first pass, dw8250_handle_irq() -> serial8250_handle_irq_locked() ->
handle_rx_dma() sees !up->dma->rx_running and breaks out to call
serial8250_rx_dma(), which sets dma->rx_running = 1 and calls
dma_async_issue_pending(dma->rxchan) without masking UART_IER_RDI or draining
the RX FIFO:

drivers/tty/serial/8250/8250_port.c:handle_rx_dma() {
	switch (iir & 0x3f) {
	case UART_IIR_RDI:
		if (!up->dma->rx_running)
			break;
		fallthrough;
	case UART_IIR_RLSI:
	case UART_IIR_RX_TIMEOUT:
		serial8250_rx_dma_flush(up);
		return true;
	}
	return up->dma->rx_dma(up);
}

Unlike DMA controllers that start transfers synchronously in
device_issue_pending, sun6i_dma_issue_pending() defers physical channel
allocation and hardware transfer start to a softirq tasklet:

drivers/dma/sun6i-dma.c:sun6i_dma_issue_pending() {
	if (vchan_issue_pending(&vchan->vc)) {
		spin_lock(&sdev->lock);

		if (!vchan->phy && list_empty(&vchan->node)) {
			list_add_tail(&vchan->node, &sdev->pending);
			tasklet_schedule(&sdev->task);
			...
		}

		spin_unlock(&sdev->lock);
	}
}

Because serial8250_interrupt() is running in hardirq context, the tasklet
cannot run before serial8250_interrupt() immediately executes a second pass
on the port. Since no bytes have been read from the RX FIFO and UART_IER_RDI
is still enabled, UART_IIR still reports UART_IIR_RDI. On this second pass,
handle_rx_dma() sees up->dma->rx_running == 1 and calls
serial8250_rx_dma_flush(up), which pauses and terminates the DMA transfer that
was just queued.

Furthermore, inside serial8250_rx_dma_flush() -> __dma_rx_complete(),
sun6i_dma_tx_status() returns DMA_IN_PROGRESS even after dmaengine_pause(),
causing __dma_rx_complete() to return early without clearing dma->rx_running
or copying any received bytes before dmaengine_terminate_async() discards the
descriptor:

drivers/tty/serial/8250/8250_dma.c:__dma_rx_complete() {
	dma_status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
	if (dma_status == DMA_IN_PROGRESS)
		return;
}

> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index db066a42f61d2..e99d35880a689 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -335,6 +335,14 @@ static const struct serial8250_config uart_config[] = {
>  		.rxtrig_bytes	= {1, 4},
>  		.flags		= UART_CAP_FIFO,
>  	},
> +	[UART_PORT_DW_APB_256] = {
> +		.name		= "DW APB UART 256",
> +		.fifo_size	= 256,
> +		.tx_loadsz	= 256,
> +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
> +		.rxtrig_bytes	= {1, 64, 128, 254},
> +		.flags		= UART_CAP_FIFO | UART_CAP_NOTEMT,
> +	},

[Severity: High]
Should uart_config[UART_PORT_DW_APB_256].flags also include UART_CAP_AFE?

In dw8250_probe(), dw8250_quirks() sets p->set_termios = dw8250_do_set_termios.
When CRTSCTS is enabled, dw8250_do_set_termios() unconditionally sets
UPSTAT_AUTOCTS, which tells serial_core to disable software CTS flow control:

drivers/tty/serial/8250/8250_dwlib.c:dw8250_do_set_termios() {
	p->status &= ~UPSTAT_AUTOCTS;
	if (termios->c_cflag & CRTSCTS)
		p->status |= UPSTAT_AUTOCTS;

	serial8250_do_set_termios(p, termios, old);
}

However, serial8250_do_set_termios() -> serial8250_set_afe() only enables
hardware auto flow control (UART_MCR_AFE) if UART_CAP_AFE is set in
up->capabilities:

drivers/tty/serial/8250/8250_port.c:serial8250_set_afe() {
	if (!(up->capabilities & UART_CAP_AFE))
		return;

	up->mcr &= ~UART_MCR_AFE;
	if (termios->c_cflag & CRTSCTS)
		up->mcr |= UART_MCR_AFE;
}

Because dw8250_setup_port_fixed() bypasses dw8250_setup_port(),
up->capabilities is populated solely from
uart_config[UART_PORT_DW_APB_256].flags. Without UART_CAP_AFE, enabling
CRTSCTS on H616 UARTs (such as Bluetooth HCI UARTs using uart-has-rtscts)
disables software CTS flow control without enabling hardware auto flow control
in UART_MCR.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-submit-h616-uart-fifo-v1-v1-0-e4a3bae2eba8@gmail.com?part=2