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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/11/26 12:11 AM, 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;
>          }
>          ```
> > +    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);
>        }
We could avoid ternary operator here:

static void ns_write_ier(const struct ns16550 *uart, unsigned int val)
{
    ns_write_reg(uart, UART_IER,
                 val | (UART_IER_UUE & -(unsigned int)uart->xscale));
}

but it is less readable...

~ Oleksii

> and use it for all futures IER writes?
>
> Thanks for contributing!