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

[Zhang Zheng <zhangzheng@xxxxxxxxxxx>]

On Sun, Oct 11, 2026 at 12:11:27AM +0200, Baptiste Le Duc wrote:
> > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > index 593d208483..953d166314 100644
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -352,6 +353,14 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
> >      /* No interrupts. */
> >      ns_write_reg(uart, UART_IER, 0);
> Do we know of a UART where UUE reads back as 1 whatever is written? I
> could not find any such hardware, so I am not sure the check is needed
> but it might be wrong. If not, then we could drop this write but if so,
> then I propose to move it in a dedicated function:
> 
>         ```
>         static bool ns_is_xscale(const struct ns16550 *uart)
>         {
>             /*
>             * We're going to explicitly set the UUE bit to 0 before
>             * trying to write and read a 1 just to make sure it's not
>             * already a 1 and maybe locked there before we even start.
>             */
>             ns_write_reg(uart, UART_IER, 0);
> 
>             if ( !(ns_read_reg(uart, UART_IER) & UART_IER_UUE) ) {
>                 /* Now we are in a 0 state */
>                 ns_write_reg(uart, UART_IER, UART_IER_UUE);
>                 if ( ns_read_reg(uart, UART_IER) & UART_IER_UUE )
>                     return true;
>             }
>             return false;
>         }
>         ```
I agree with Oleksii's suggestion that the XScale quirk should be
derived from the Device Tree compatible for DT-described UARTs. Therefore,
I think will drop this register probe then.
> > +    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;
> > +    }
> > +
> >      /* 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));
> >
> Every IER write has to keep UUE on an XScale UART, otherwise the UART is
> switched off. Open-coding the ternary at each call site makes it easy
> for a future caller to forget. Would it be better to add a helper, e.g.:
> 
>       static void ns_write_ier(const struct ns16550 *uart, unsigned int val)
>       {
>           ns_write_reg(uart, UART_IER,
>                        uart->xscale ? val | UART_IER_UUE : val);
>       }
> 
> and use it for all futures IER writes?
> 
> Thanks for contributing!
> 
> -- 
> Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
Agreed. I will add an `ns_write_ier()` helper and use it for all IER
write paths, including `ns16550_setup_preirq()`,
`ns16550_setup_postirq()`, and `ns16550_interrupt()`.

Thanks for pointing this out.

Zhang