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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Oct 23, 2026 at 03:19:28PM -0600, James Hilliard wrote:
> H616 has 256-byte transmit and receive FIFOs, but its optional DesignWare
> component parameter register reads as zero. Generic 16550 probing leaves
> the port using the 16-byte FIFO configuration and its trigger-level table.
> DMA attachment also depends on a FIFO size being available during probe,
> so these ports do not reach the existing 8250 DMA path.
> 
> Keep PORT_16550A and supply the FIFO size, transmit load and capabilities
> when matching the H616 platform data. Provide receive-trigger callbacks
> for the documented thresholds of 1, 64, 128 and 254 bytes. The normal
> 16550A half-full trigger selects 128 bytes on this integration. Serialize
> trigger changes with termios and hold a runtime PM reference while
> programming FCR, preserving the FIFO contents.
> 
> Advertise automatic flow control so CRTSCTS enables MCR.AFCE. The
> DesignWare termios path selects hardware CTS handling, so the capability
> must be present for the corresponding hardware control to be enabled.
> 
> Skip optional-register probing for this fixed configuration. H616 has
> debug-divisor and RS485 control registers at the offsets used to detect
> DesignWare hardware RS485 and fractional-divisor support. Probing these
> offsets writes unrelated registers and can select incompatible callbacks.
> Keep the BUSY-safe integer divisor hook and software RS485 handling.
> 
> Use the existing SKIP_SET_RATE quirk to keep the UART clock enabled during
> termios changes. Its clock is a gate on the shared APB2 source, rather than
> an independent baud-rate generator.
> 
> Attach DMA from the fixed configuration before registering the port.
> Configure single-byte RX and TX bursts because the peripheral request can
> correspond to just one character. The existing startup path requests the
> firmware-described channels and falls back to PIO when they are unavailable
> or the port is a kernel console.

...

> +static int dw8250_h616_set_rxtrig(struct uart_port *p, unsigned char bytes)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(p);
> +	int i, ret;

Why is 'i' signed?

> +	/* Use the nearest lower threshold, as the generic 8250 implementation does. */
> +	for (i = 1; i < ARRAY_SIZE(dw8250_h616_rxtrig_bytes); i++)
> +		if (bytes < dw8250_h616_rxtrig_bytes[i])
> +			break;
> +
> +	guard(mutex)(&p->state->port.mutex);
> +
> +	ret = pm_runtime_resume_and_get(p->dev);
> +	if (ret < 0)
> +		return ret;

Don't we have a guard()() for this?

> +	scoped_guard(uart_port_lock_irqsave, p) {
> +		up->fcr &= ~UART_FCR_TRIGGER_MASK;
> +		up->fcr |= (i - 1) << UART_FCR_R_TRIG_SHIFT;
> +		/* Changing the trigger alone preserves the FIFO contents. */
> +		serial_port_out(p, UART_FCR, up->fcr);
> +	}
> +	pm_runtime_put(p->dev);
> +
> +	return 0;
> +}

...

> static int dw8250_probe(struct platform_device *pdev)

> -	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;
> -		up->dma = &data->data.dma;
> +	if (data->pdata == &dw8250_h616_data) {
> +		dw8250_h616_setup(up, data);
> +	} else {
> +		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;
> +			up->dma = &data->data.dma;
> +		}
>  	}

This looks unscalable and too much churn. What you should do is to add
a .setup() callback member to struct dw8250_platform_data and assign that
to dw8250_setup_port(), and change the

	if (!data->skip_autocfg)
		dw8250_setup_port(p);

to

	if (data->pdata->setup)
		data->pdata->setup(...);

which effectively kills that skip_autocfg. This is done in a separate patch.
Then in your patch you won't need this churn.

-- 
With Best Regards,
Andy Shevchenko