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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> 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);
      }

and use it for all futures IER writes?

Thanks for contributing!

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>