[PATCH 0/2 v2] xen: ACPI/SPCR based initialization of 8250 UART

[PATCH 0/2 v2] xen: ACPI/SPCR based initialization of 8250 UART

[Bhupinder Thakur]

Currently, Xen supports only DT based initialization of 16550 UART.
This patch set adds support for initializing 16550 UART using ACPI
SPCR table.

It also fixes one issue in HP Moonshot (HPE Proliant Aarch64)
platform, which uses 16550 UART as a console. There is an erratum
required to be implemented to make the UART console work.

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>

Bhupinder Thakur (2):
  xen: Add support for initializing 16550 UART using ACPI
  xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform

 xen/drivers/char/ns16550.c  | 170 ++++++++++++++++++++++++++++++++++++++++----
 xen/include/xen/8250-uart.h |   1 +
 2 files changed, 159 insertions(+), 12 deletions(-)

-- 
2.7.4


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

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

[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 <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)
 {
-    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>
+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 **)&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;
+
+    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)
+                    : 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;
+#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;
+
+    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;
+
+    ret = ns16550_uart_init(&uart, data, true);
+    if ( ret )
+        return ret;
+
+    ns16550_vuart_init(uart);
+
+    ns16550_register_uart(uart);
+
+    return ret;
+}
+
+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       */
-- 
2.7.4


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

[PATCH 2/2 v2] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform

[Bhupinder Thakur]

    The console was not working on HP Moonshot (HPE Proliant Aarch64) because
    the UART registers were accessed as 8-bit aligned addresses. However,
    registers are 32-bit aligned for HP Moonshot.

    Since ACPI/SPCR table does not specify the register shift to be applied to the
    register offset, this patch implements an erratum to correctly set the register
    shift for HP Moonshot.

    Similar erratum was implemented in linux:

    commit 79a648328d2a604524a30523ca763fbeca0f70e3
    Author: Loc Ho <lho@apm.com>
    Date:   Mon Jul 3 14:33:09 2017 -0700

        ACPI: SPCR: Workaround for APM X-Gene 8250 UART 32-alignment errata

        APM X-Gene verion 1 and 2 have an 8250 UART with its register
        aligned to 32-bit. In addition, the latest released BIOS
        encodes the access field as 8-bit access instead 32-bit access.
        This causes no console with ACPI boot as the console
        will not match X-Gene UART port due to the lack of mmio32
        option.

        Signed-off-by: Loc Ho <lho@apm.com>
        Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
        Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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 | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index cf42fce..bb01c46 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1517,6 +1517,33 @@ static int ns16550_init_dt(struct ns16550 *uart,
 
 #ifdef CONFIG_ACPI
 #include <xen/acpi.h>
+/*
+ * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
+ * register aligned to 32-bit. In addition, the BIOS also encoded the
+ * access width to be 8 bits. This function detects this errata condition.
+ */
+static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
+{
+    bool xgene_8250 = false;
+
+    if ( tb->interface_type != ACPI_DBG2_16550_COMPATIBLE )
+        return false;
+
+    if ( memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE) &&
+         memcmp(tb->header.oem_id, "HPE   ", ACPI_OEM_ID_SIZE) )
+        return false;
+
+    if ( !memcmp(tb->header.oem_table_id, "XGENESPC",
+         ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0 )
+        xgene_8250 = true;
+
+    if ( !memcmp(tb->header.oem_table_id, "ProLiant",
+         ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1 )
+        xgene_8250 = true;
+
+    return xgene_8250;
+}
+
 static int ns16550_init_acpi(struct ns16550 *uart,
                              const void *data)
 {
@@ -1539,9 +1566,20 @@ static int ns16550_init_acpi(struct ns16550 *uart,
     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;
 
+    if ( xgene_8250_erratum_present(spcr) )
+    {
+        /*
+         * for xgene v1 and v2 the registers are 32-bit and so a
+         * register shift of 2 has to be applied to get the
+         * correct register offset.
+         */
+        uart->reg_shift = 2;
+    }
+    else
+        uart->reg_shift = 0;
+
+    uart->io_size = UART_MAX_REG << uart->reg_shift;
     irq_set_type(spcr->interrupt, spcr->interrupt_type);
 
     return 0;
-- 
2.7.4


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

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

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

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

[Jan Beulich]

>>> On 09.11.17 at 12:31, <roger.pau@citrix.com> 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 <xen/acpi.h>
> 
> 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: [PATCH 1/2 v2] xen: Add support for initializing 16550 UART using ACPI

[Roger Pau Monné]

On Thu, Nov 09, 2017 at 06:18:21AM -0700, Jan Beulich wrote:
> >>> On 09.11.17 at 12:31, <roger.pau@citrix.com> 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 <xen/acpi.h>
> > 
> > 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: [PATCH 1/2 v2] xen: Add support for initializing 16550 UART using ACPI

[Jan Beulich]

>>> On 09.11.17 at 16:07, <roger.pau@citrix.com> wrote:
> On Thu, Nov 09, 2017 at 06:18:21AM -0700, Jan Beulich wrote:
>> >>> On 09.11.17 at 12:31, <roger.pau@citrix.com> 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 <xen/acpi.h>
>> > 
>> > 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: [PATCH 1/2 v2] xen: Add support for initializing 16550 UART using ACPI

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

This function does not look very useful but getting &ns16550_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 <<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;
> +#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;
> +
> +    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

The code below is going to break x86 build. You need to do #if 
defined(CONFIG_ACPI) && defined(CONFIG_ARM).

> +static int __init ns16550_acpi_uart_init(const void *data)
> +{
> +    struct ns16550 *uart;
> +    int ret = 0;
> +
> +    ret = ns16550_uart_init(&uart, data, true);
> +    if ( ret )
> +        return ret;
> +
> +    ns16550_vuart_init(uart);
> +
> +    ns16550_register_uart(uart);
> +
> +    return ret;
> +}
> +
> +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

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

[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 <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)
>>   {
>> -    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>
>> +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 **)&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;
>> +
>> +    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)
>> +                    : ns16550_init_dt(uart, data);
>> +}
>
> This function does not look very useful but getting &ns16550_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 <<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;
>> +#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;
>> +
>> +    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
>
> The code below is going to break x86 build. You need to do #if 
> defined(CONFIG_ACPI) && defined(CONFIG_ARM).
>
I had put the vuart code under CONFIG_ARM. Is there anything else which 
is ARM specific?

>> +static int __init ns16550_acpi_uart_init(const void *data)
>> +{
>> +    struct ns16550 *uart;
>> +    int ret = 0;
>> +
>> +    ret = ns16550_uart_init(&uart, data, true);
>> +    if ( ret )
>> +        return ret;
>> +
>> +    ns16550_vuart_init(uart);
>> +
>> +    ns16550_register_uart(uart);
>> +
>> +    return ret;
>> +}
>> +
>> +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,
>

Regards,
Bhupinder

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

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

[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 <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?
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 = &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.
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 **)&spcr);
>
> if ( ... )
>
ok.
>> +    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.
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 = &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.
I will add a pre-patch which just restructures the existing code.
>
>> +#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 )
> ...
ok.
>> +
>> +    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.
ok.
>> +    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.
ok. I will 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)
> It seems to me that this is rather UART_NUM_REGS, UART_MAX_REG would
> be UART_USR AFAICT.
ok. I will redefine it to UART_NUM_REGS.
> Thanks, Roger.

Regards,
Bhupinder

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

Re: [PATCH 2/2 v2] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform

[Konrad Rzeszutek Wilk]

On Thu, Nov 09, 2017 at 03:49:24PM +0530, Bhupinder Thakur wrote:
>     The console was not working on HP Moonshot (HPE Proliant Aarch64) because
>     the UART registers were accessed as 8-bit aligned addresses. However,
>     registers are 32-bit aligned for HP Moonshot.
> 
>     Since ACPI/SPCR table does not specify the register shift to be applied to the
>     register offset, this patch implements an erratum to correctly set the register
>     shift for HP Moonshot.
> 
>     Similar erratum was implemented in linux:
> 
>     commit 79a648328d2a604524a30523ca763fbeca0f70e3
>     Author: Loc Ho <lho@apm.com>
>     Date:   Mon Jul 3 14:33:09 2017 -0700
> 
>         ACPI: SPCR: Workaround for APM X-Gene 8250 UART 32-alignment errata
> 
>         APM X-Gene verion 1 and 2 have an 8250 UART with its register
>         aligned to 32-bit. In addition, the latest released BIOS
>         encodes the access field as 8-bit access instead 32-bit access.
>         This causes no console with ACPI boot as the console
>         will not match X-Gene UART port due to the lack of mmio32
>         option.
> 
>         Signed-off-by: Loc Ho <lho@apm.com>
>         Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>         Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Any particular reason you offset this whole commit description by four spaces?

> 
> 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 | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)

This is v2 posting, but I don't see what changed.

Usually you do something like this:

v1: New posting
v2: Nothing changed from v1.

or

v1: New posting
v2: Added more folks on CC
    Added consts in XYZ..

> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index cf42fce..bb01c46 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1517,6 +1517,33 @@ static int ns16550_init_dt(struct ns16550 *uart,
>  
>  #ifdef CONFIG_ACPI
>  #include <xen/acpi.h>
> +/*
> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
> + * register aligned to 32-bit. In addition, the BIOS also encoded the
> + * access width to be 8 bits. This function detects this errata condition.
> + */
> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
> +{
> +    bool xgene_8250 = false;
> +
> +    if ( tb->interface_type != ACPI_DBG2_16550_COMPATIBLE )
> +        return false;
> +
> +    if ( memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE) &&
> +         memcmp(tb->header.oem_id, "HPE   ", ACPI_OEM_ID_SIZE) )
> +        return false;
> +
> +    if ( !memcmp(tb->header.oem_table_id, "XGENESPC",
> +         ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0 )
> +        xgene_8250 = true;

Why not just 'return true' ?

> +
> +    if ( !memcmp(tb->header.oem_table_id, "ProLiant",
> +         ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1 )
> +        xgene_8250 = true;

And return true here too?
> +
> +    return xgene_8250;

And then this is just 'return false' and you don't have xgen_8250 on the stack?

> +}
> +
>  static int ns16550_init_acpi(struct ns16550 *uart,
>                               const void *data)
>  {
> @@ -1539,9 +1566,20 @@ static int ns16550_init_acpi(struct ns16550 *uart,
>      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;
>  
> +    if ( xgene_8250_erratum_present(spcr) )
> +    {
> +        /*
> +         * for xgene v1 and v2 the registers are 32-bit and so a

s/for/For/
> +         * register shift of 2 has to be applied to get the
> +         * correct register offset.
> +         */
> +        uart->reg_shift = 2;
> +    }
> +    else
> +        uart->reg_shift = 0;
> +
> +    uart->io_size = UART_MAX_REG << uart->reg_shift;
>      irq_set_type(spcr->interrupt, spcr->interrupt_type);
>  
>      return 0;
> -- 
> 2.7.4
> 

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

Re: [PATCH 2/2 v2] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform

[George Dunlap]

On Nov 15, 2017, at 9:20 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> On Thu, Nov 09, 2017 at 03:49:24PM +0530, Bhupinder Thakur wrote:
>>    The console was not working on HP Moonshot (HPE Proliant Aarch64) because
>>    the UART registers were accessed as 8-bit aligned addresses. However,
>>    registers are 32-bit aligned for HP Moonshot.
>> 
>>    Since ACPI/SPCR table does not specify the register shift to be applied to the
>>    register offset, this patch implements an erratum to correctly set the register
>>    shift for HP Moonshot.
>> 
>>    Similar erratum was implemented in linux:
>> 
>>    commit 79a648328d2a604524a30523ca763fbeca0f70e3
>>    Author: Loc Ho <lho@apm.com>
>>    Date:   Mon Jul 3 14:33:09 2017 -0700
>> 
>>        ACPI: SPCR: Workaround for APM X-Gene 8250 UART 32-alignment errata
>> 
>>        APM X-Gene verion 1 and 2 have an 8250 UART with its register
>>        aligned to 32-bit. In addition, the latest released BIOS
>>        encodes the access field as 8-bit access instead 32-bit access.
>>        This causes no console with ACPI boot as the console
>>        will not match X-Gene UART port due to the lack of mmio32
>>        option.
>> 
>>        Signed-off-by: Loc Ho <lho@apm.com>
>>        Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>        Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Any particular reason you offset this whole commit description by four spaces?

I get this effect when I use “git show” to look at a changeset for some reason.  Bhupinder, did you perhaps export a changeset as a patch using “git show” and then re-import it?

In any case, this needs to be fixed.

 -George

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

Re: [PATCH 2/2 v2] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform

[Bhupinder Thakur]

Hi,


On Thursday 16 November 2017 03:26 PM, George Dunlap wrote:
> On Nov 15, 2017, at 9:20 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> On Thu, Nov 09, 2017 at 03:49:24PM +0530, Bhupinder Thakur wrote:
>>>     The console was not working on HP Moonshot (HPE Proliant Aarch64) because
>>>     the UART registers were accessed as 8-bit aligned addresses. However,
>>>     registers are 32-bit aligned for HP Moonshot.
>>>
>>>     Since ACPI/SPCR table does not specify the register shift to be applied to the
>>>     register offset, this patch implements an erratum to correctly set the register
>>>     shift for HP Moonshot.
>>>
>>>     Similar erratum was implemented in linux:
>>>
>>>     commit 79a648328d2a604524a30523ca763fbeca0f70e3
>>>     Author: Loc Ho <lho@apm.com>
>>>     Date:   Mon Jul 3 14:33:09 2017 -0700
>>>
>>>         ACPI: SPCR: Workaround for APM X-Gene 8250 UART 32-alignment errata
>>>
>>>         APM X-Gene verion 1 and 2 have an 8250 UART with its register
>>>         aligned to 32-bit. In addition, the latest released BIOS
>>>         encodes the access field as 8-bit access instead 32-bit access.
>>>         This causes no console with ACPI boot as the console
>>>         will not match X-Gene UART port due to the lack of mmio32
>>>         option.
>>>
>>>         Signed-off-by: Loc Ho <lho@apm.com>
>>>         Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>         Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Any particular reason you offset this whole commit description by four spaces?
> I get this effect when I use “git show” to look at a changeset for some reason.  Bhupinder, did you perhaps export a changeset as a patch using “git show” and then re-import it?
>
> In any case, this needs to be fixed.
Yes I copied the commit message from git show. I will align the text.
>   -George
>
Regards,
Bhupinder

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