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

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

On Sat, Oct 24, 2026 at 02:22:25PM -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.
> 
> Keep PORT_16550A and preserve the FIFO size read from firmware by
> uart_read_port_properties(). Use it for the transmit load and derive the
> receive thresholds from the documented 1-byte, quarter-full, half-full
> and FIFO-depth-minus-two mapping. On H616 these are 1, 64, 128 and 254
> bytes; the normal 16550A half-full FCR setting selects 128 bytes.
> 
> Serialize trigger changes with termios and hold a runtime PM reference
> while programming FCR, preserving the FIFO contents. Propagate runtime
> resume errors before changing either the FCR shadow or the hardware.
> 
> 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.
> 
> Select the fixed configuration through the platform setup callback.
> H616 has debug-divisor and RS485 control registers at the offsets used to
> detect DesignWare hardware RS485 and fractional-divisor support. Avoid
> those probes and retain 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.

...

> +static int dw8250_sunxi_set_rxtrig(struct uart_port *p, unsigned char bytes)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(p);
> +	unsigned int i;
> +	int ret;
> +
> +	/* Use the nearest lower threshold, as the generic 8250 implementation does. */
> +	for (i = 1; i < UART_FCR_R_TRIG_MAX_STATE; i++)
> +		if (bytes < dw8250_sunxi_rxtrig_bytes(p, i))
> +			break;

Can we provide an array of the possible values and deduplicate this loop?

drivers/tty/serial/8250/8250.h:

static inline int serial8250_bytes_to_fcr(unsigned char *rxtrig_bytes, unsigned char bytes)
{
	for (unsigned int i = 1; i < UART_FCR_R_TRIG_MAX_STATE; i++) {
		if (bytes < rxtrig_bytes[i])
			/* Use the nearest lower value */
			return (--i) << UART_FCR_R_TRIG_SHIFT;
        }

        return UART_FCR_R_TRIG_11;
}

drivers/tty/serial/8250/8250_port.c:

static int bytes_to_fcr_rxtrig(struct uart_8250_port *up, unsigned char bytes)
{
        const struct serial8250_config *conf_type = &uart_config[up->port.type];

        if (!conf_type->rxtrig_bytes[UART_FCR_R_TRIG_BITS(UART_FCR_R_TRIG_00)])
                return -EOPNOTSUPP;

	return serial8250_bytes_to_fcr(conf_type->rxtrig_bytes, bytes);
}

(this split should be done in a separate preparatory change).

And here

	u32 fcr;
	...
	fcr = serial8250_bytes_to_fcr(...->rxtrig_bytes, bytes);

> +	guard(mutex)(&p->state->port.mutex);
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(p->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	guard(uart_port_lock_irqsave)(p);
> +
> +	up->fcr &= ~UART_FCR_TRIGGER_MASK;

> +	up->fcr |= (i - 1) << UART_FCR_R_TRIG_SHIFT;

The right side is embedded in the above mentioned helper.

> +	/* Changing the trigger alone preserves the FIFO contents. */
> +	serial_port_out(p, UART_FCR, up->fcr);
> +
> +	return 0;
> +}

...

> +static void dw8250_h616_setup(struct uart_port *p)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(p);
> +
> +	p->type = PORT_16550A;
> +	p->flags |= UPF_FIXED_TYPE;

> +	up->tx_loadsz = p->fifosize;

So, here you can precalculate the array of possible FIFO thresholds.

> +	up->capabilities = UART_CAP_FIFO | UART_CAP_NOTEMT | UART_CAP_AFE;
> +	p->get_rxtrig = dw8250_sunxi_get_rxtrig;
> +	p->set_rxtrig = dw8250_sunxi_set_rxtrig;
> +	p->rs485_config = serial8250_em485_config;
> +	p->rs485_supported = serial8250_em485_supported;
> +	up->rs485_start_tx = serial8250_em485_start_tx;
> +	up->rs485_stop_tx = serial8250_em485_stop_tx;

Please, add more new lines, so we see the groups of different callbacks and add
comments on top of some to explain, like

	/* Custom FIFO trigger thresholds since it differs to standard 16550A */

> +}

-- 
With Best Regards,
Andy Shevchenko