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

[Zhang Zheng <zhangzheng@xxxxxxxxxxx>]

On Mon, Oct 12, 2026 at 11:20:22AM +0200, 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.
> 
This UEE probe section was adapted from Linux's 8250.c `autoconfig_16550a()`.
Nevertheless, I think using the Device Tree compatible is a reasonable solution,
given your point that this bit may also be writable on other UARTs and could
therefore result in false positives.
> >       /* Handle the DesignWare 8250 'busy-detect' quirk. */
> >       handle_dw_usr_busy_quirk(uart);
> > @@ -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()
> and ns16550_interrupt().
> 
Yes, i missed that path. I will add `ns_write_ier()` and use
it in `ns16550_setup_preirq()`, `ns16550_setup_postirq()`, and
`ns16550_interrupt()` so that UUE is preserved on every direct IER
write.
> ns16550_start_tx()/ns16550_stop_tx() do a read-modify-write of IER, so
> they read UUE back from the register and write it out again unchanged.
> They therefore work correctly without the wrapper. They could be
> switched to ns_write_ier() as well for consistency, but that isn't
> strictly necessary and likely could be left in way it looks like now.
> 
> ~ Oleksii
> 
Agreed. Since these paths already preserve UUE, I will leave them unchanged.

Thanks

Zhang
> >       }
> >       if ( uart->irq >= 0 )
> > diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> > index d13352940c..358d264698 100644
> > --- a/xen/include/xen/8250-uart.h
> > +++ b/xen/include/xen/8250-uart.h
> > @@ -42,6 +42,7 @@
> >   #define UART_IER_ETHREI   0x02    /* tx reg. empty        */
> >   #define UART_IER_ELSI     0x04    /* rx line status       */
> >   #define UART_IER_EMSI     0x08    /* MODEM status         */
> > +#define UART_IER_UUE      0x40    /* UART unit enable (XScale/PXA) */
> >   /* Interrupt Identification Register */
> >   #define UART_IIR_NOINT    0x01    /* no interrupt pending */