Re: [PATCH v3 1/2] xen/ns16550: identify XScale UARTs from device tree

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 10/20/26 5:45 AM, Zhang Zheng wrote:
> Identify XScale UARTs using intel,xscale-uart, since IER bit 6 has other
> meanings on some UART variants. Keep UUE set when writing IER so the
> UART stays enabled when switching to polling, and enable RTOIE along
> with receive interrupts.
> 
> Signed-off-by: Zhang Zheng <zhangzheng@iscas.ac.cn>
> ---
>   xen/drivers/char/ns16550.c  | 29 +++++++++++++++++++++--------
>   xen/include/xen/8250-uart.h |  2 ++
>   2 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index be388c8e65..1f54155d67 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 */
> @@ -168,6 +169,14 @@ static void ns_write_reg(const struct ns16550 *uart, unsigned int reg, u8 c)
>       }
>   }
>   
> +static void ns_write_ier(const struct ns16550 *uart, u8 ier)

Nit: please use uint8_t in new code.

> +{
> +    if ( uart->xscale )
> +        ier |= UART_IER_UUE;
> +
> +    ns_write_reg(uart, UART_IER, ier);
> +}
> +

[...]

>   
>   #ifdef CONFIG_ARM
> @@ -749,16 +759,16 @@ static int __init check_existence(struct ns16550 *uart)
>        * there's no point trying anything else.
>        */
>       scratch = ns_read_reg(uart, UART_IER);
> -    ns_write_reg(uart, UART_IER, 0);
> +    ns_write_ier(uart, 0);
>   
>       /*
>        * Mask out IER[7:4] bits for test as some UARTs (e.g. TL
>        * 16C754B) allow only to modify them if an EFR bit is set.
>        */
>       scratch2 = ns_read_reg(uart, UART_IER) & 0x0f;
> -    ns_write_reg(uart,UART_IER, 0x0F);
> +    ns_write_ier(uart, 0x0f);
>       scratch3 = ns_read_reg(uart, UART_IER) & 0x0f;
> -    ns_write_reg(uart, UART_IER, scratch);
> +    ns_write_ier(uart, scratch);
>       if ( (scratch2 != 0) || (scratch3 != 0x0F) )
>           return 0;

check_existence() is only reached from ns16550_parse_port_config()
(i.e. never for a DT-described UART), and for MMIO UARTs it returns
early anyway, so uart->xscale is always false here. I don't mind the
conversion for consistency, but then please mention it in the
description, together with the 0x0F -> 0x0f / missing blank style
changes.


>   
> @@ -2034,6 +2044,8 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
>   
>       uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
>   
> +    uart->xscale = dt_device_is_compatible(dev, "intel,xscale-uart");
> +
>   #ifdef CONFIG_ARM
>       uart->vuart.base_addr = uart->io_base;
>       uart->vuart.size = uart->io_size;
> @@ -2055,6 +2067,7 @@ static const struct dt_device_match ns16550_dt_match[] __initconst =
>       DT_MATCH_COMPATIBLE("ns16550"),
>       DT_MATCH_COMPATIBLE("ns16550a"),
>       DT_MATCH_COMPATIBLE("snps,dw-apb-uart"),
> +    DT_MATCH_COMPATIBLE("intel,xscale-uart"),
>       DT_MATCH_COMPATIBLE("brcm,bcm2835-aux-uart"),
>       { /* sentinel */ },
>   };
> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> index d13352940c..5709a6c91b 100644
> --- a/xen/include/xen/8250-uart.h
> +++ b/xen/include/xen/8250-uart.h
> @@ -42,6 +42,8 @@
>   #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_RTOIE    0x10    /* Receive timeout interrupt enable (XScale) */

This line is 81 characters long. Perhaps something like
"/* rx timeout (XScale)  */" to also match the style of the
surrounding comments?

> +#define UART_IER_UUE      0x40    /* UART unit enable (XScale) */

A side question, not for this patch: Linux uses a 32-byte
FIFO for PORT_XSCALE 
(https://elixir.bootlin.com/linux/v7.3/source/drivers/tty/serial/8250/8250_port.c#L147), 
while here fifo_size will end up being detected as 16. Is that intentional?

The code change itself looks fine to me.

~ Oleksii