[Xen-devel,1/2] xen: Add support for initializing 16550 UART using ACPI

Message ID 1509617589-22760-1-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • [Xen-devel,1/2] xen: Add support for initializing 16550 UART using ACPI
Related show

Commit Message

Bhupinder Thakur Nov. 2, 2017, 10:13 a.m.
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(+)

Comments

Julien Grall Nov. 2, 2017, 12:15 p.m. | #1
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,
Bhupinder Thakur Nov. 8, 2017, 6:16 a.m. | #2
Hi Julien,

On 2 November 2017 at 17:45, Julien Grall <julien.grall@linaro.org> wrote:
> 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?

In pci_uart_config:

uart->io_size = max(8U << param->reg_shift,
                                 param->uart_offset);

I was not sure which param to consider to get the uart_offset. Since
the max register that ns16550 uses is UART_USR, I
calculated the io_size based on that.
>
>> +
>> +    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

Regards,
Bhupinder

Patch

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
+#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;
+    uart->reg_shift = 0;
+    uart->io_size = UART_MAX_REG<<uart->reg_shift;
+
+    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;
+    uart->vuart.status_off = UART_LSR<<uart->reg_shift;
+    uart->vuart.status = UART_LSR_THRE|UART_LSR_TEMT;
+
+    /* 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       */