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

[Konrad Rzeszutek Wilk <konrad@darnok.org>]

On Fri, Nov 24, 2017 at 05:09:11PM +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.

Can you provide the link to the spec you are using. I am wondering
if I am looking at some older one.

> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> Changes since v2:
> - renamed UART_MAX_REG to UART_NUM_REGS
> - aligned some assignment statements
> - some coding style changes
> 
> 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  | 67 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/8250-uart.h |  1 +
>  2 files changed, 68 insertions(+)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index c5dfc1e..af4712f 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -29,6 +29,10 @@
>  #ifdef CONFIG_X86
>  #include <asm/fixmap.h>
>  #endif
> +#ifdef CONFIG_ACPI
> +#include <xen/acpi.h>
> +#endif
> +
>  
>  /*
>   * Configure serial port with a string:
> @@ -1565,6 +1569,69 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>  DT_DEVICE_END
>  
>  #endif /* HAS_DEVICE_TREE */
> +
> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM)

I would remove the CONFIG_ARM as the spec says it is possible to use
this on ARM _and_ x86 hardware.

But I guess you can't as ACPI_DEVICE_START is defined to be only
on ARM?

> +
> +static int ns16550_init_acpi(struct ns16550 **puart)
> +{
> +    struct acpi_table_spcr *spcr;
> +    int status;

hmm, not acpi_status ?
> +    struct ns16550 *uart = &ns16550_com[0];
> +
> +    ns16550_init_common(uart);

I would move this below the error check below..
> +
> +    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;

Are those two assumed from the ACPI spec?

Wait a minute. The
https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx

has Baud Rate at Offset 58?

> +    uart->parity    = spcr->parity;
> +    uart->stop_bits = spcr->stop_bits;
> +    uart->io_base   = spcr->serial_port.address;
> +    uart->irq       = spcr->interrupt;

You need to check if the 'Interrupt Type' field is set before
you look at this?

> +    uart->reg_width = spcr->serial_port.bit_width / 8;
> +    uart->reg_shift = 0;
> +    uart->io_size   = UART_NUM_REGS << uart->reg_shift;
> +
> +    irq_set_type(spcr->interrupt, spcr->interrupt_type);

Um, the spec has a whole bunch of other bits set in 'interrupt_type'?

> +
> +    *puart = uart;
> +
> +    return 0;
> +}
> +
> +static int __init ns16550_acpi_uart_init(const void *data)
> +{
> +    int ret;
> +    struct ns16550 *uart;
> +
> +    ret = ns16550_init_acpi(&uart);
> +    if ( ret )
> +        return ret;
> +
> +    ns16550_vuart_init(uart);
> +
> +    ns16550_register_uart(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..849a5c0 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_NUM_REGS     (UART_USR + 1)
>  
>  /* Interrupt Enable Register */
>  #define UART_IER_ERDAI    0x01    /* rx data recv'd       */
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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