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

[James Hilliard]

On Sat, Oct 24, 2026 at 8:10 AM Chen-Yu Tsai <wens@xxxxxxxxxx> wrote:
>
> On Sat, Oct 24, 2026 at 10:51 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> >
> > On Sat, Oct 24, 2026 at 10:41:55PM +0900, Chen-Yu Tsai wrote:
> > > On Sat, Oct 24, 2026 at 10:19 PM Andy Shevchenko
> > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > On Sat, Oct 24, 2026 at 10:10:33PM +0900, Chen-Yu Tsai wrote:
> > > > > On Sat, Oct 24, 2026 at 12:12 AM James Hilliard
> > > > > <james.hilliard1@xxxxxxxxx> wrote:
> > > > > > On Fri, Oct 23, 2026 at 2:44 AM Andy Shevchenko
> > > > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > > > > On Fri, Oct 23, 2026 at 01:33:50AM -0600, James Hilliard wrote:
> >
> > ...
> >
> > > > > > > >       /* Internal 8250 only */
> > > > > > > >       UART_PORT_AIROHA                = 124,
> > > > > > > >       UART_PORT_AIROHA_HS             = 125,
> > > > > > > > +     UART_PORT_DW_APB_256    = 126,
> > > > > > >
> > > > > > > Why? Besides the poorly chosen name, what prevents us from modifying the
> > > > > > > corresponding parameters at probe time by leaving the one of the existing
> > > > > > > types?
> > > > > >
> > > > > > The new type was primarily a way to reuse the core's FIFO configuration
> > > > > > table, including the RX trigger mapping. H616 uses thresholds of 1, 64,
> > > > > > 128 and 254 bytes, whereas PORT_16550A maps the same FCR bits to 1, 4, 8
> > > > > > and 14 bytes. Overriding only fifosize and tx_loadsz would leave
> > > > > > rx_trig_bytes reporting and selecting the wrong thresholds.
> > > > > >
> > > > > > The core already has get_rxtrig/set_rxtrig callbacks, though. I'll rework
> > > > > > this to retain PORT_16550A, supply the H616 FIFO parameters during setup,
> > > > > > and use those callbacks for the correct trigger mapping.
> > > > >
> > > > > Please consider just taking the FIFO size from the DT. On some chips
> > > > > not every UART has the same size FIFO, so we already need to deal with
> > > > > it.
> > > > >
> > > > > Also, FIFO size is a standard UART DT property, though somehow not included
> > > > > for dw-uart.
> > > >
> > > > FWIW, it supports all standard properties for UART.
> > > > Check what uart_read_port_properties() does.
> > >
> > > Ah, that's nice. I hacked up something [1] in the past to get it to
> > > work, but that was probably before you added the helper. I never got
> > > around to splitting up the commits and sending it out.
> >
> > Hmm... The helper is in the kernel for the past 2.5 years.
> > Maybe you rebased it and it lost original dates?
>
> I had been carrying the changes locally without committing them. I only
> committed them recently to back them up remotely. So yeah, the dates are
> not accurate.
>
> > But if it's not working, we need to address that, I believe it should work out
> > of the box, because the same function is used across dozen of serial drivers.
>
> The new helper probably works fine. I can rework my code to use the values
> given by the helpers and give it a try sometime next week.

I've incorporated that approach into v3[0]. The H616 DTS supplies
fifo-size = <256>, and the driver uses the value populated by
uart_read_port_properties() for the transmit load and receive-trigger
mapping.

[0] https://lore.kernel.org/all/20261024-submit-h616-uart-fifo-v1-v3-4-286fc1b8c3c1@xxxxxxxxx/T/

>
> > It might be an issue in 8250_dw.c, but I can't tell for sure.
> >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/wens/linux.git/commit/?id=e7e0741fa04f349ad9f94a4059e32467a30ef5d9
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >