Re: [PATCH v4 4/5] serial: 8250_dw: configure the Allwinner H616 UART

[James Hilliard]

On Wed, Oct 28, 2026 at 2:39 AM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, 27 Oct 2026, 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 describe H616's fixed 256-byte FIFO depth in platform
> > data. Use common Allwinner setup helpers to configure the transmit load
> > and receive trigger table from that depth.
> >
> > 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.
> >
> > Build the threshold table during setup and use the shared 8250 conversion
> > helper to select the nearest lower threshold for sysfs writes.
>
> You do that, I can see from the patch, but I came to read this changelog
> to understand "why"?? You provide zero reasoning for this design decision.
>
> Since you fifosize is constant as well, so what benefit building it on the
> fly servers?

The intent is to keep the FIFO depth as the single platform-data input
and derive the thresholds from the common Allwinner mapping. Other sunxi
integrations using that mapping could then supply a different FIFO depth
without duplicating the threshold table.

Building the table once also lets get_rxtrig() and the shared
bytes-to-FCR helper use the same derived values.

>
> > 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.
> >
> > Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>
> > ---
> >  drivers/tty/serial/8250/8250_dw.c | 94 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 94 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > index 1351523f0c47..2f156c01c5a1 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/io.h>
> >  #include <linux/lockdep.h>
> >  #include <linux/module.h>
> > +#include <linux/mutex.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/property.h>
> > @@ -63,6 +64,7 @@
> >  struct dw8250_platform_data {
> >       u8 usr_reg;
> >       u32 cpr_value;
> > +     unsigned int fifo_size;
> >       unsigned int quirks;
> >       void (*setup)(struct uart_port *p);
> >  };
> > @@ -81,6 +83,7 @@ struct dw8250_data {
> >       unsigned int            in_idle:1;
> >
> >       u8                      no_int_count;
> > +     u8                      rxtrig_bytes[UART_FCR_R_TRIG_MAX_STATE];
> >  };
> >
> >  static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data *data)
> > @@ -657,6 +660,89 @@ static void dw8250_octeon_setup(struct uart_port *p)
> >       dw8250_setup_dma(p);
> >  }
> >
> > +/* The trigger is the masked, unshifted FCR receive trigger field. */
> > +static unsigned int dw8250_sunxi_rxtrig_bytes(struct uart_port *p, unsigned int trigger)
> > +{
> > +     switch (trigger) {
> > +     case UART_FCR_TRIGGER_1:
> > +             return 1;
> > +     case UART_FCR_TRIGGER_4:
> > +             return p->fifosize / 4;
> > +     case UART_FCR_TRIGGER_8:
> > +             return p->fifosize / 2;
> > +     default:
> > +             return p->fifosize - 2;
> > +     }
> > +}
> > +
> > +static int dw8250_sunxi_get_rxtrig(struct uart_port *p)
> > +{
> > +     struct dw8250_data *data = to_dw8250_data(p->private_data);
> > +     struct uart_8250_port *up = up_to_u8250p(p);
> > +
> > +     guard(uart_port_lock_irqsave)(p);
> > +
> > +     return data->rxtrig_bytes[UART_FCR_R_TRIG_BITS(up->fcr)];
> > +}
> > +
> > +static int dw8250_sunxi_set_rxtrig(struct uart_port *p, unsigned char bytes)
> > +{
> > +     struct dw8250_data *data = to_dw8250_data(p->private_data);
> > +     struct uart_8250_port *up = up_to_u8250p(p);
> > +     int fcr, ret;
> > +
> > +     fcr = serial8250_bytes_to_fcr(data->rxtrig_bytes, bytes);
> > +     if (fcr < 0)
> > +             return fcr;
> > +
> > +     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 |= fcr;
> > +     /* Changing the trigger alone preserves the FIFO contents. */
> > +     serial_port_out(p, UART_FCR, up->fcr);
> > +
> > +     return 0;
> > +}
> > +
> > +static void dw8250_sunxi_setup_rxtrig(struct uart_port *p)
> > +{
> > +     struct dw8250_data *data = to_dw8250_data(p->private_data);
> > +
> > +     /* Allwinner RX thresholds differ from the standard 16550A table. */
> > +     for (unsigned int i = 0; i < ARRAY_SIZE(data->rxtrig_bytes); i++)
> > +             data->rxtrig_bytes[i] = dw8250_sunxi_rxtrig_bytes(p, i << UART_FCR_R_TRIG_SHIFT);
>
> FIELD_PREP(UART_FCR_TRIGGER_MASK, i)
>
> For clarity, it might be useful to split it into two lines using an
> intermediate variable.
>
> > +     p->get_rxtrig = dw8250_sunxi_get_rxtrig;
> > +     p->set_rxtrig = dw8250_sunxi_set_rxtrig;
> > +}
> > +
> > +static void dw8250_sunxi_setup(struct uart_port *p)
> > +{
> > +     struct dw8250_data *data = to_dw8250_data(p->private_data);
> > +     struct uart_8250_port *up = up_to_u8250p(p);
> > +
> > +     p->type = PORT_16550A;
> > +     p->flags |= UPF_FIXED_TYPE;
> > +     p->fifosize = data->pdata->fifo_size;
> > +     up->tx_loadsz = p->fifosize;
> > +     up->capabilities = UART_CAP_FIFO | UART_CAP_NOTEMT | UART_CAP_AFE;
> > +
> > +     dw8250_sunxi_setup_rxtrig(p);
> > +
> > +     /* Use software RS485 with the Allwinner register layout. */
> > +     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;
> > +}
> > +
> >  static int dw8250_probe(struct platform_device *pdev)
> >  {
> >       struct uart_8250_port uart = {}, *up = &uart;
> > @@ -878,6 +964,13 @@ static const struct dw8250_platform_data dw8250_dw_apb = {
> >       .usr_reg = DW_UART_USR,
> >  };
> >
> > +static const struct dw8250_platform_data dw8250_h616_data = {
> > +     .usr_reg = DW_UART_USR,
> > +     .fifo_size = 256,
> > +     .quirks = DW_UART_QUIRK_SKIP_SET_RATE,
> > +     .setup = dw8250_sunxi_setup,
> > +};
> > +
> >  static const struct dw8250_platform_data dw8250_octeon_3860_data = {
> >       .usr_reg = OCTEON_UART_USR,
> >       .quirks = DW_UART_QUIRK_OCTEON,
> > @@ -929,6 +1022,7 @@ static const struct dw8250_platform_data dw8250_tda54 = {
> >
> >  static const struct of_device_id dw8250_of_match[] = {
> >       { .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
> > +     { .compatible = "allwinner,sun50i-h616-uart", .data = &dw8250_h616_data },
> >       { .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
> >       { .compatible = "marvell,armada-38x-uart", .data = &dw8250_armada_38x_data },
> >       { .compatible = "renesas,rzn1-uart", .data = &dw8250_renesas_rzn1_data },
> >
> >
>
> --
>  i.
>