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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 9029 bytes --]

On Wed, 28 Oct 2026, James Hilliard wrote:

> On Wed, Oct 28, 2026 at 2:39 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> 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.

Okay, but it would be nice to mention that then in the changelog as the 
justification.

> > > 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@gmail.com>
> > > ---
> > >  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.
> >
> 

-- 
 i.