Re: [PATCH 1/2 v2] xen: Add support for initializing 16550 UART using ACPI

[Julien Grall <julien.grall@linaro.org>]

Hi Bhupinder,

On 11/09/2017 10:19 AM, Bhupinder Thakur wrote:
> Currently, Xen supports only DT based initialization of 16550 UART.
> This patch adds support for initializing 16550 UART using ACPI SPCR table.
> 
> This patch also makes the uart initialization code common between DT and
> ACPI based initialization.

Can you please have one patch to refactor the code and one to add ACPI 
support? This will be easier to review.

> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> TBD:
> There was one review comment from Julien about how the uart->io_size is being
> calculated. Currently, I am calulating the io_size based on address of the last
> UART register.
> 
> pci_uart_config also calcualates the uart->io_size like this:
> 
> uart->io_size = max(8U << param->reg_shift,
>                                   param->uart_offset);
> 
> I am not sure whether we can use similar logic for calculating uart->io_size.
> 
> Changes since v1:
> - Reused common code between DT and ACPI based initializations
> 
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
>   xen/drivers/char/ns16550.c  | 132 ++++++++++++++++++++++++++++++++++++++++----
>   xen/include/xen/8250-uart.h |   1 +
>   2 files changed, 121 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index e0f8199..cf42fce 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1463,18 +1463,13 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
>   }
>   
>   #ifdef CONFIG_HAS_DEVICE_TREE
> -static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
> -                                       const void *data)
> +static int ns16550_init_dt(struct ns16550 *uart,
> +                           const struct dt_device_node *dev)
>   {
> -    struct ns16550 *uart;
> -    int res;
> +    int res = 0;
>       u32 reg_shift, reg_width;
>       u64 io_size;
>   
> -    uart = &ns16550_com[0];
> -
> -    ns16550_init_common(uart);
> -
>       uart->baud      = BAUD_AUTO;
>       uart->data_bits = 8;
>       uart->parity    = UART_PARITY_NONE;
> @@ -1510,18 +1505,103 @@ 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");
>   
> +    return res;
> +}
> +#else
> +static int ns16550_init_dt(struct ns16550 *uart,
> +                           const struct dt_device_node *dev)
> +{
> +    return -EINVAL;
> +}
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +#include <xen/acpi.h>
> +static int ns16550_init_acpi(struct ns16550 *uart,
> +                             const void *data)
> +{
> +    struct acpi_table_spcr *spcr = NULL;
> +    int status = 0;
> +
> +    status = acpi_get_table(ACPI_SIG_SPCR, 0,
> +                            (struct acpi_table_header **)&spcr);
> +
> +    if ( ACPI_FAILURE(status) )
> +    {
> +        printk("ns16550: Failed to get SPCR table\n");
> +        return -EINVAL;
> +    }
> +
> +    uart->baud      = BAUD_AUTO;
> +    uart->data_bits = 8;
> +    uart->parity    = spcr->parity;
> +    uart->stop_bits = spcr->stop_bits;
> +    uart->io_base = spcr->serial_port.address;
> +    uart->irq = spcr->interrupt;
> +    uart->reg_width = spcr->serial_port.bit_width / 8;
> +    uart->reg_shift = 0;
> +    uart->io_size = UART_MAX_REG << uart->reg_shift;
> +
> +    irq_set_type(spcr->interrupt, spcr->interrupt_type);
> +
> +    return 0;
> +}
> +#else
> +static int ns16550_init_acpi(struct ns16550 *uart,
> +                             const void *data)
> +{
> +    return -EINVAL;
> +}
> +#endif
> +
> +static int ns16550_uart_init(struct ns16550 **puart,
> +                             const void *data, bool acpi)
> +{
> +    struct ns16550 *uart = &ns16550_com[0];
> +
> +    *puart = uart;
> +
> +    ns16550_init_common(uart);
> +
> +    return ( acpi ) ? ns16550_init_acpi(uart, data)
> +                    : ns16550_init_dt(uart, data);
> +}

This function does not look very useful but getting &ns16550_com[0].
I do agree that we need it is nice to have common code, but I think you 
went too far here.

There are no need for 3 separate functions and 2 functions for each 
firmware.

I think duplicating the code of ns16550_uart_init for ACPI and DT is 
fine. You could then create a function that is a merge vuart_init and 
register_init.

This would also limit the number of #ifdef within this code.

> +
> +static void ns16550_vuart_init(struct ns16550 *uart)
> +{
> +#ifdef CONFIG_ARM
>       uart->vuart.base_addr = uart->io_base;
>       uart->vuart.size = uart->io_size;
> -    uart->vuart.data_off = UART_THR <<uart->reg_shift;
> -    uart->vuart.status_off = UART_LSR<<uart->reg_shift;
> -    uart->vuart.status = UART_LSR_THRE|UART_LSR_TEMT;
> +    uart->vuart.data_off = UART_THR << uart->reg_shift;
> +    uart->vuart.status_off = UART_LSR << uart->reg_shift;
> +    uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT;
> +#endif
> +}
>   
> +static void ns16550_register_uart(struct ns16550 *uart)
> +{
>       /* Register with generic serial driver. */
>       serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
> +}
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
> +                                       const void *data)
> +{
> +    struct ns16550 *uart;
> +    int ret = 0;
> +
> +    ret = ns16550_uart_init(&uart, dev, false);
> +    if ( ret )
> +        return ret;
> +
> +    ns16550_vuart_init(uart);
> +
> +    ns16550_register_uart(uart);
>   
>       dt_device_set_used_by(dev, DOMID_XEN);
>   
> -    return 0;
> +    return ret;
>   }
>   
>   static const struct dt_device_match ns16550_dt_match[] __initconst =
> @@ -1538,6 +1618,34 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>   DT_DEVICE_END
>   
>   #endif /* HAS_DEVICE_TREE */
> +
> +#ifdef CONFIG_ACPI

The code below is going to break x86 build. You need to do #if 
defined(CONFIG_ACPI) && defined(CONFIG_ARM).

> +static int __init ns16550_acpi_uart_init(const void *data)
> +{
> +    struct ns16550 *uart;
> +    int ret = 0;
> +
> +    ret = ns16550_uart_init(&uart, data, true);
> +    if ( ret )
> +        return ret;
> +
> +    ns16550_vuart_init(uart);
> +
> +    ns16550_register_uart(uart);
> +
> +    return ret;
> +}
> +
> +ACPI_DEVICE_START(ns16550c, "16550 COMPAT UART", DEVICE_SERIAL)
> +        .class_type = ACPI_DBG2_16550_COMPATIBLE,
> +        .init = ns16550_acpi_uart_init,
> +ACPI_DEVICE_END
> +ACPI_DEVICE_START(ns16550s, "16550 SUBSET UART", DEVICE_SERIAL)
> +        .class_type = ACPI_DBG2_16550_SUBSET,
> +        .init = ns16550_acpi_uart_init,
> +ACPI_DEVICE_END
> +
> +#endif
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> index 5c3bac3..1b3e137 100644
> --- a/xen/include/xen/8250-uart.h
> +++ b/xen/include/xen/8250-uart.h
> @@ -35,6 +35,7 @@
>   #define UART_USR          0x1f    /* Status register (DW) */
>   #define UART_DLL          0x00    /* divisor latch (ls) (DLAB=1) */
>   #define UART_DLM          0x01    /* divisor latch (ms) (DLAB=1) */
> +#define UART_MAX_REG      (UART_USR + 1)
>   
>   /* Interrupt Enable Register */
>   #define UART_IER_ERDAI    0x01    /* rx data recv'd       */
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel