[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 1/2 v2] xen: Add support for initializing 16550 UART using ACPI
- To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
- From: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
- Date: Wed, 15 Nov 2017 16:31:07 +0530
- Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, Julien Grall <julien.grall@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- Delivery-date: Wed, 15 Nov 2017 11:01:43 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
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@xxxxxxxxxx>
---
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@xxxxxxxxxx>
CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
CC: Jan Beulich <jbeulich@xxxxxxxx>
CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Tim Deegan <tim@xxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Julien Grall <julien.grall@xxxxxxx>
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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
