Re: [PATCH v2 1/2] xen/ns16550: detect and preserve UART_IER_UUE

[Zhang Zheng <zhangzheng@xxxxxxxxxxx>]

On Mon, Oct 12, 2026 at 03:24:20PM +0200, Oleksii Kurochko wrote:
> 
> 
> On 10/12/26 2:50 PM, Jan Beulich wrote:
> > On 12.10.2026 11:20, Oleksii Kurochko wrote:
> > > On 10/10/26 5:14 AM, Zhang Zheng wrote:
> > > > XScale-compatible UARTs require UART_IER_UUE to remain set.  Probe the
> > > > bit during setup and preserve it when enabling receive interrupts, so
> > > > the xscale/PXA UART remains operational after ns16550 takes over.
> > > > 
> > > > Signed-off-by: Zhang Zheng <zhangzheng@xxxxxxxxxxx>
> > > > ---
> > > >    xen/drivers/char/ns16550.c  | 12 +++++++++++-
> > > >    xen/include/xen/8250-uart.h |  1 +
> > > >    2 files changed, 12 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > > > index cdda28879e..c174465059 100644
> > > > --- a/xen/drivers/char/ns16550.c
> > > > +++ b/xen/drivers/char/ns16550.c
> > > > @@ -64,6 +64,7 @@ static struct ns16550 {
> > > >        bool intr_works;
> > > >        bool force_polling;
> > > >        bool dw_usr_bsy;
> > > > +    bool xscale;
> > > >    #ifdef NS16550_PCI
> > > >        /* PCI card parameters. */
> > > >        bool pb_bdf_enable;     /* if =1, pb-bdf effective, port behind 
> > > > bridge */
> > > > @@ -352,6 +353,14 @@ static void ns16550_setup_preirq(struct ns16550 
> > > > *uart)
> > > >        /* No interrupts. */
> > > >        ns_write_reg(uart, UART_IER, 0);
> > > > +    uart->xscale = false;
> > > > +    if ( !(ns_read_reg(uart, UART_IER) & UART_IER_UUE) )
> > > > +    {
> > > > +        ns_write_reg(uart, UART_IER, UART_IER_UUE);
> > > > +        if ( ns_read_reg(uart, UART_IER) & UART_IER_UUE )
> > > > +            uart->xscale = true;
> > > > +    }
> > > > +
> > > 
> > > This probe only checks whether IER bit 6 is writable, which isn't
> > > specific to XScale. On UARTs with an EFR (e.g. ST16C650, TI TL16C752,
> > > Exar XR17V35x) the upper IER bits become writable once EFR[4] (ECB) is
> > > set, and bit 6 is then the RTS interrupt enable. If ECB is already set
> > > when the probe runs, it would report XScale, and the driver would then
> > > keep the RTS interrupt enabled.
> > > 
> > > Could the flag be derived from the compatible in ns16550_uart_dt_init()
> > > instead, the same way dw_usr_bsy is?
> > > 
> > >       uart->xscale = dt_device_is_compatible(dev, "intel,xscale-uart");
> > > 
> > > That compatible would also need adding to ns16550_dt_match[]. The
> > > SpacemiT K1 nodes, for example, have no "ns16550a" fallback.
> > 
> > Are you suggesting that Linux'es XScale detection is flawed then, too?
> 
> I looked at it quickly and not flawed as such, but Linux uses that test only
> as a last-resort heuristic, which isn't how it would be used here.
> 
> In Linux the bit 6 test sits at the end of autoconfig_16550a(). That
> only runs for ports whose type isn't known up front (legacy/PnP/PCI
> autoprobing), and only with CONFIG_SERIAL_8250_16550A_VARIANTS or
> UPF_FULL_PROBE. It also comes after the EFR probes, so ST16C65x /
> TL16C75x style parts are classified and return before reaching it.
> Even so, the comment there says "this is probably one of the Xscale
> platform's internal UARTs", and a part whose EFR already has ECB set
> would fall through the EFR check and be reported as XScale. So yes,
> the same corner case exists there.
> 
> Where the type is known, Linux doesn't probe at all: 8250_of maps
> "intel,xscale-uart" to PORT_XSCALE and sets UPF_FIXED_TYPE, so
> autoconfig() (where it is happening the probing in a similar to suggested
> above way) never runs for DT ports. 8250_exar sets UPF_FIXED_TYPE as well.
> 
I agree that it is better to drop the UUE probe and identify the XScale UART
directly from its compatible.

Zhang
> > 
> > > > @@ -446,7 +455,8 @@ static void ns16550_setup_postirq(struct ns16550 
> > > > *uart)
> > > >                         UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | 
> > > > UART_MCR_RTS);
> > > >            /* Enable receive interrupts. */
> > > > -        ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
> > > > +        ns_write_reg(uart, UART_IER,
> > > > +                     UART_IER_ERDAI | (uart->xscale ? UART_IER_UUE : 
> > > > 0));
> > > 
> > > It seems like not all cases are covered. For example,
> > > ns16550_interrupt() writes 0 to IER when it switches to polling, which
> > > would turn off the UART on XScale. Am I missing something? If not then
> > > something like:
> > > 
> > > static void ns_write_ier(const struct ns16550 *uart, uint8_t ier)
> > > {
> > >       if ( uart->xscale )
> > >           ier |= UART_IER_UUE;
> > > 
> > >       ns_write_reg(uart, UART_IER, ier);
> > > }
> > > 
> > > would be needed, used in ns16550_setup_preirq(), ns16550_setup_postirq()
> > 
> > Yes, the more that as per Linux'es driver source, bit 4 may also need 
> > keeping
> > permanently set (not only for XScale, but apparently also for Tegra).
> 
> Good point.
> 
> ~ Oleksii