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

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

Hi Bhupinder,

Please write a cover letter even if it is small when your send a series 
with multiple patches.

On 02/11/17 10:13, 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.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> 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  | 57 +++++++++++++++++++++++++++++++++++++++++++++
>   xen/include/xen/8250-uart.h |  1 +
>   2 files changed, 58 insertions(+)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index e0f8199..b3f6d85 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1538,6 +1538,63 @@ 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)

> +#include <xen/acpi.h>
> +
> +static int __init ns16550_acpi_uart_init(const void *data)
> +{
> +    struct ns16550 *uart;
> +    acpi_status status;
> +    struct acpi_table_spcr *spcr = NULL;
> +
> +    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 = &ns16550_com[0];
> +
> +    ns16550_init_common(uart);
> +
> +    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;

width / 8;

> +    uart->reg_shift = 0;
> +    uart->io_size = UART_MAX_REG<<uart->reg_shift;

space before and after <<.

Also, io_size seems to be computed differently in pci_uart_config. I am 
not sure why the difference here?

> +
> +    irq_set_type(spcr->interrupt, spcr->interrupt_type);
> +
> +    uart->vuart.base_addr = uart->io_base;
> +    uart->vuart.size = uart->io_size;
> +    uart->vuart.data_off = UART_THR <<uart->reg_shift;

Ditto for the space.

> +    uart->vuart.status_off = UART_LSR<<uart->reg_shift;

Ditto.

> +    uart->vuart.status = UART_LSR_THRE|UART_LSR_TEMT;

Ditto.

Also, the code looks very similar to the DT version. Is there any way to 
share it?

> +
> +    /* Register with generic serial driver. */
> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
> +
> +    return 0;
> +}
> +
> +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