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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Nov 09, 2017 at 03:49:23PM +0530, 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.
> 
> 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)

Why are you dropping the __init attribute?

>  {
> -    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>

Please place the include at the top of the file, together with the
other ones.

> +static int ns16550_init_acpi(struct ns16550 *uart,
> +                             const void *data)
> +{
> +    struct acpi_table_spcr *spcr = NULL;
> +    int status = 0;

I don't think you need to initialize any of those two variables. Or
do:

int status = acpi_get_table(ACPI_SIG_SPCR, 0,
                            (struct acpi_table_header **)&spcr);

if ( ... )


> +    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;

You seem to align some of the '=' above but not all, please do either
one, but consistently.

> +
> +    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)
              ^ unneeded parentheses.

> +                    : ns16550_init_dt(uart, data);
> +}
> +
> +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;

You should try to avoid mixing functional changes with style ones.
Please split this into a pre-patch.

> +#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;

No need to initialize ret, or alternatively you can do:

int ret = ns16550_uart_init(&uart, dev, false);

if ( ret )
...

> +
> +    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
> +static int __init ns16550_acpi_uart_init(const void *data)
> +{
> +    struct ns16550 *uart;
> +    int ret = 0;

Same comment as above.

> +    ret = ns16550_uart_init(&uart, data, true);
> +    if ( ret )
> +        return ret;
> +
> +    ns16550_vuart_init(uart);
> +
> +    ns16550_register_uart(uart);
> +
> +    return ret;

This is always return 0 at this point.

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

It seems to me that this is rather UART_NUM_REGS, UART_MAX_REG would
be UART_USR AFAICT.

Thanks, Roger.

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