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

2017-11-15 Thread Bhupinder Thakur 

Hi,


On Thursday 09 November 2017 05:01 PM, Roger Pau Monné wrote:

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 
---
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 
CC: George Dunlap 
CC: Ian Jackson 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
CC: Julien Grall 

  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?
This is a helper I defined for initializing the uart and called from the 
main __init function.



  {
-struct ns16550 *uart;
-int res;
+int res = 0;
  u32 reg_shift, reg_width;
  u64 io_size;
  
-uart = _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 

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

ok.



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

if ( ... )


ok.

+status = acpi_get_table(ACPI_SIG_SPCR, 0,
+(struct acpi_table_header **));
+
+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.

I will align the assignments.

+
+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 = _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 vuart.status_off = UART_LSRvuart.status = UART_LSR_THRE|UART_LSR_TEMT;
+uart->vuart.data_off = UART_THR << uart->reg_shift;
+

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

2017-11-15 Thread Bhupinder Thakur 

Hi Julien,


On Tuesday 14 November 2017 12:21 AM, Julien Grall wrote:

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.



ok.


Signed-off-by: Bhupinder Thakur 
---
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 
CC: George Dunlap 
CC: Ian Jackson 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
CC: Julien Grall 

  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 = _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 
+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 **));
+
+    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 = _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 _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.


We can retain the ns16550_init_acpi() and ns16550_init_dt() and call 
them directly from the main __init functions.



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 vuart.status_off = UART_LSR

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

2017-11-13 Thread Julien Grall 

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 
---
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 
CC: George Dunlap 
CC: Ian Jackson 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
CC: Julien Grall 

  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 = _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 
+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 **));
+
+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 = _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 _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 vuart.status_off = UART_LSRvuart.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 |

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

2017-11-09 Thread Jan Beulich 
>>> On 09.11.17 at 16:07,  wrote:
> On Thu, Nov 09, 2017 at 06:18:21AM -0700, Jan Beulich wrote:
>> >>> On 09.11.17 at 12:31,  wrote:
>> > On Thu, Nov 09, 2017 at 03:49:23PM +0530, Bhupinder Thakur wrote:
>> >> +static int ns16550_init_dt(struct ns16550 *uart,
>> >> +   const struct dt_device_node *dev)
>> >> +{
>> >> +return -EINVAL;
>> >> +}
>> >> +#endif
>> >> +
>> >> +#ifdef CONFIG_ACPI
>> >> +#include 
>> > 
>> > Please place the include at the top of the file, together with the
>> > other ones.
>> 
>> I disagree here, at least as long as that header isn't itself expanding
>> to nothing (or only stubs) when !CONFIG_ACPI.
> 
> The header already has a CONFIG_ACPI guard inside, but it doesn't
> cover the full content, so my suggestion was to move the include to
> the top of the file, but keeping the CONFIG_ACPI guards around it.

Ah, I see. I'd then still prefer one less #ifdef by keeping it where it
is, but that's a matter of taste I admit.

Jan


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

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

2017-11-09 Thread Roger Pau Monné 
On Thu, Nov 09, 2017 at 06:18:21AM -0700, Jan Beulich wrote:
> >>> On 09.11.17 at 12:31,  wrote:
> > On Thu, Nov 09, 2017 at 03:49:23PM +0530, Bhupinder Thakur wrote:
> >> +static int ns16550_init_dt(struct ns16550 *uart,
> >> +   const struct dt_device_node *dev)
> >> +{
> >> +return -EINVAL;
> >> +}
> >> +#endif
> >> +
> >> +#ifdef CONFIG_ACPI
> >> +#include 
> > 
> > Please place the include at the top of the file, together with the
> > other ones.
> 
> I disagree here, at least as long as that header isn't itself expanding
> to nothing (or only stubs) when !CONFIG_ACPI.

The header already has a CONFIG_ACPI guard inside, but it doesn't
cover the full content, so my suggestion was to move the include to
the top of the file, but keeping the CONFIG_ACPI guards around it.

Roger.

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

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

2017-11-09 Thread Jan Beulich 
>>> On 09.11.17 at 12:31,  wrote:
> On Thu, Nov 09, 2017 at 03:49:23PM +0530, Bhupinder Thakur wrote:
>> +static int ns16550_init_dt(struct ns16550 *uart,
>> +   const struct dt_device_node *dev)
>> +{
>> +return -EINVAL;
>> +}
>> +#endif
>> +
>> +#ifdef CONFIG_ACPI
>> +#include 
> 
> Please place the include at the top of the file, together with the
> other ones.

I disagree here, at least as long as that header isn't itself expanding
to nothing (or only stubs) when !CONFIG_ACPI.

Jan


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

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

2017-11-09 Thread Roger Pau Monné 
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 
> ---
> 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 
> CC: George Dunlap 
> CC: Ian Jackson 
> CC: Jan Beulich 
> CC: Konrad Rzeszutek Wilk 
> CC: Stefano Stabellini 
> CC: Tim Deegan 
> CC: Wei Liu 
> CC: Julien Grall 
> 
>  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 = _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 

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 **));

if ( ... )


> +status = acpi_get_table(ACPI_SIG_SPCR, 0,
> +(struct acpi_table_header **));
> +
> +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 = _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->vuart.status_off = UART_LSR -uart->vuart.status = UART_LSR_THRE|UART_LSR_TEMT;
> +uart->vuart.data_off =

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

2017-11-09 Thread Bhupinder Thakur 
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 
---
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 
CC: George Dunlap 
CC: Ian Jackson 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
CC: Julien Grall 

 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 = _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 
+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 **));
+
+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 = _com[0];
+
+*puart = uart;
+
+ns16550_init_common(uart);
+
+return ( acpi ) ? ns16550_init_acpi(uart, data)
+: 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 vuart.status_off = UART_LSRvuart.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, _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(, 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