* [PATCH 0/3 v3] xen: ACPI/SPCR based initialization of 8250 UART @ 2017-11-24 11:39 Bhupinder Thakur 2017-11-24 11:39 ` [PATCH 1/3 v3] xen: Refactor 16550 UART code Bhupinder Thakur ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Bhupinder Thakur @ 2017-11-24 11:39 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich 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 (3): xen: Refactor 16550 UART code xen: Add support for initializing 16550 UART using ACPI xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform xen/drivers/char/ns16550.c | 156 ++++++++++++++++++++++++++++++++++++++++---- xen/include/xen/8250-uart.h | 1 + 2 files changed, 144 insertions(+), 13 deletions(-) -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3 v3] xen: Refactor 16550 UART code 2017-11-24 11:39 [PATCH 0/3 v3] xen: ACPI/SPCR based initialization of 8250 UART Bhupinder Thakur @ 2017-11-24 11:39 ` Bhupinder Thakur 2017-11-24 13:37 ` Konrad Rzeszutek Wilk 2017-11-24 16:04 ` Julien Grall 2017-11-24 11:39 ` [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI Bhupinder Thakur 2017-11-24 11:39 ` [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform Bhupinder Thakur 2 siblings, 2 replies; 15+ messages in thread From: Bhupinder Thakur @ 2017-11-24 11:39 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich This patch refactors the 8250 UART code so that code can be reused by later patches, which add support for ACPI based UART initialization. Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> --- Changes since v2: - Refactored the code to prepare for later patches. 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 | 53 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 13 deletions(-) diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index e0f8199..c5dfc1e 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -1462,16 +1462,32 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults) ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2); } +#ifdef CONFIG_ARM +static void ns16550_vuart_init(struct ns16550 *uart) +{ + 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; +} +#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) + +static int ns16550_init_dt(struct ns16550 **puart, + const struct dt_device_node *dev) { - struct ns16550 *uart; int res; u32 reg_shift, reg_width; u64 io_size; - - uart = &ns16550_com[0]; + struct ns16550 *uart = &ns16550_com[0]; ns16550_init_common(uart); @@ -1510,18 +1526,29 @@ 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"); - 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; + *puart = uart; - /* Register with generic serial driver. */ - serial_register_uart(uart - ns16550_com, &ns16550_driver, uart); + return 0; +} + +static int __init ns16550_uart_dt_init(struct dt_device_node *dev, + const void *data) +{ + struct ns16550 *uart; + int ret; + + ret = ns16550_init_dt(&uart, data); + + 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 = -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3 v3] xen: Refactor 16550 UART code 2017-11-24 11:39 ` [PATCH 1/3 v3] xen: Refactor 16550 UART code Bhupinder Thakur @ 2017-11-24 13:37 ` Konrad Rzeszutek Wilk 2017-11-24 16:04 ` Julien Grall 1 sibling, 0 replies; 15+ messages in thread From: Konrad Rzeszutek Wilk @ 2017-11-24 13:37 UTC (permalink / raw) To: Bhupinder Thakur Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, xen-devel On Fri, Nov 24, 2017 at 05:09:10PM +0530, Bhupinder Thakur wrote: > This patch refactors the 8250 UART code so that code can be reused > by later patches, which add support for ACPI based UART > initialization. > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > Changes since v2: > - Refactored the code to prepare for later patches. > > 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 | 53 ++++++++++++++++++++++++++++++++++------------ > 1 file changed, 40 insertions(+), 13 deletions(-) > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > index e0f8199..c5dfc1e 100644 > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -1462,16 +1462,32 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults) > ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2); > } > > +#ifdef CONFIG_ARM > +static void ns16550_vuart_init(struct ns16550 *uart) > +{ > + 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; > +} > +#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) > + > +static int ns16550_init_dt(struct ns16550 **puart, > + const struct dt_device_node *dev) > { > - struct ns16550 *uart; > int res; > u32 reg_shift, reg_width; > u64 io_size; > - > - uart = &ns16550_com[0]; > + struct ns16550 *uart = &ns16550_com[0]; I am sure I understand this change. In the #2 patch you are doing this the old way: +static int ns16550_init_acpi(struct ns16550 **puart) +{ + struct acpi_table_spcr *spcr; + int status; + struct ns16550 *uart = &ns16550_com[0]; Any particular resaon for this change? If you can restore this to the original way in this function you can put: Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > ns16550_init_common(uart); > > @@ -1510,18 +1526,29 @@ 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"); > > - 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; > + *puart = uart; > > - /* Register with generic serial driver. */ > - serial_register_uart(uart - ns16550_com, &ns16550_driver, uart); > + return 0; > +} > + > +static int __init ns16550_uart_dt_init(struct dt_device_node *dev, > + const void *data) > +{ > + struct ns16550 *uart; > + int ret; > + > + ret = ns16550_init_dt(&uart, data); > + > + 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 = > -- > 2.7.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3 v3] xen: Refactor 16550 UART code 2017-11-24 11:39 ` [PATCH 1/3 v3] xen: Refactor 16550 UART code Bhupinder Thakur 2017-11-24 13:37 ` Konrad Rzeszutek Wilk @ 2017-11-24 16:04 ` Julien Grall 1 sibling, 0 replies; 15+ messages in thread From: Julien Grall @ 2017-11-24 16:04 UTC (permalink / raw) To: Bhupinder Thakur, xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich Hi Bhupinder, On 24/11/17 11:39, Bhupinder Thakur wrote: > This patch refactors the 8250 UART code so that code can be reused > by later patches, which add support for ACPI based UART > initialization. > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > Changes since v2: > - Refactored the code to prepare for later patches. > > 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 | 53 ++++++++++++++++++++++++++++++++++------------ > 1 file changed, 40 insertions(+), 13 deletions(-) > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > index e0f8199..c5dfc1e 100644 > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -1462,16 +1462,32 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults) > ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2); > } > > +#ifdef CONFIG_ARM > +static void ns16550_vuart_init(struct ns16550 *uart) __init. > +{ > + 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; > +} > +#endif > + > +static void ns16550_register_uart(struct ns16550 *uart) __init > +{ > + /* Register with generic serial driver. */ > + serial_register_uart(uart - ns16550_com, &ns16550_driver, uart); > +} I don't think this function should exist. Arm is using only one port, but this is not true for x86. It would be better to fold this into each implementation (e.g ACPI and DT). > + > #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 **puart, __init > + const struct dt_device_node *dev) > { > - struct ns16550 *uart; > int res; > u32 reg_shift, reg_width; > u64 io_size; > - > - uart = &ns16550_com[0]; > + struct ns16550 *uart = &ns16550_com[0]; > > ns16550_init_common(uart); > > @@ -1510,18 +1526,29 @@ 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"); > > - 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; > + *puart = uart; > > - /* Register with generic serial driver. */ > - serial_register_uart(uart - ns16550_com, &ns16550_driver, uart); > + return 0; > +} > + > +static int __init ns16550_uart_dt_init(struct dt_device_node *dev, > + const void *data) > +{ > + struct ns16550 *uart; > + int ret; > + > + ret = ns16550_init_dt(&uart, data); Why do you need to create ns16550_init_dt? It is only used here, > + > + 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 = > Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI 2017-11-24 11:39 [PATCH 0/3 v3] xen: ACPI/SPCR based initialization of 8250 UART Bhupinder Thakur 2017-11-24 11:39 ` [PATCH 1/3 v3] xen: Refactor 16550 UART code Bhupinder Thakur @ 2017-11-24 11:39 ` Bhupinder Thakur 2017-11-24 13:50 ` Konrad Rzeszutek Wilk ` (2 more replies) 2017-11-24 11:39 ` [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform Bhupinder Thakur 2 siblings, 3 replies; 15+ messages in thread From: Bhupinder Thakur @ 2017-11-24 11:39 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich 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> --- Changes since v2: - renamed UART_MAX_REG to UART_NUM_REGS - aligned some assignment statements - some coding style changes CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Tim Deegan <tim@xen.org> CC: Wei Liu <wei.liu2@citrix.com> CC: Julien Grall <julien.grall@arm.com> xen/drivers/char/ns16550.c | 67 +++++++++++++++++++++++++++++++++++++++++++++ xen/include/xen/8250-uart.h | 1 + 2 files changed, 68 insertions(+) diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index c5dfc1e..af4712f 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -29,6 +29,10 @@ #ifdef CONFIG_X86 #include <asm/fixmap.h> #endif +#ifdef CONFIG_ACPI +#include <xen/acpi.h> +#endif + /* * Configure serial port with a string: @@ -1565,6 +1569,69 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) DT_DEVICE_END #endif /* HAS_DEVICE_TREE */ + +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM) + +static int ns16550_init_acpi(struct ns16550 **puart) +{ + struct acpi_table_spcr *spcr; + int status; + struct ns16550 *uart = &ns16550_com[0]; + + ns16550_init_common(uart); + + 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_NUM_REGS << uart->reg_shift; + + irq_set_type(spcr->interrupt, spcr->interrupt_type); + + *puart = uart; + + return 0; +} + +static int __init ns16550_acpi_uart_init(const void *data) +{ + int ret; + struct ns16550 *uart; + + ret = ns16550_init_acpi(&uart); + if ( ret ) + return ret; + + ns16550_vuart_init(uart); + + ns16550_register_uart(uart); + + return 0; +} + +ACPI_DEVICE_START(ns16550c, "16550 COMPAT UART", DEVICE_SERIAL) + .class_type = ACPI_DBG2_16550_COMPATIBLE, + .init = ns16550_acpi_uart_init, +ACPI_DEVICE_END +ACPI_DEVICE_START(ns16550s, "16550 SUBSET UART", DEVICE_SERIAL) + .class_type = ACPI_DBG2_16550_SUBSET, + .init = ns16550_acpi_uart_init, +ACPI_DEVICE_END + +#endif /* * Local variables: * mode: C diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h index 5c3bac3..849a5c0 100644 --- a/xen/include/xen/8250-uart.h +++ b/xen/include/xen/8250-uart.h @@ -35,6 +35,7 @@ #define UART_USR 0x1f /* Status register (DW) */ #define UART_DLL 0x00 /* divisor latch (ls) (DLAB=1) */ #define UART_DLM 0x01 /* divisor latch (ms) (DLAB=1) */ +#define UART_NUM_REGS (UART_USR + 1) /* Interrupt Enable Register */ #define UART_IER_ERDAI 0x01 /* rx data recv'd */ -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI 2017-11-24 11:39 ` [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI Bhupinder Thakur @ 2017-11-24 13:50 ` Konrad Rzeszutek Wilk 2017-11-24 15:11 ` Andre Przywara 2017-11-24 16:05 ` Julien Grall 2017-11-27 10:01 ` Jan Beulich 2 siblings, 1 reply; 15+ messages in thread From: Konrad Rzeszutek Wilk @ 2017-11-24 13:50 UTC (permalink / raw) To: Bhupinder Thakur Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, xen-devel On Fri, Nov 24, 2017 at 05:09:11PM +0530, Bhupinder Thakur wrote: > Currently, Xen supports only DT based initialization of 16550 UART. > This patch adds support for initializing 16550 UART using ACPI SPCR table. Can you provide the link to the spec you are using. I am wondering if I am looking at some older one. > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > Changes since v2: > - renamed UART_MAX_REG to UART_NUM_REGS > - aligned some assignment statements > - some coding style changes > > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Ian Jackson <ian.jackson@eu.citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Tim Deegan <tim@xen.org> > CC: Wei Liu <wei.liu2@citrix.com> > CC: Julien Grall <julien.grall@arm.com> > > xen/drivers/char/ns16550.c | 67 +++++++++++++++++++++++++++++++++++++++++++++ > xen/include/xen/8250-uart.h | 1 + > 2 files changed, 68 insertions(+) > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > index c5dfc1e..af4712f 100644 > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -29,6 +29,10 @@ > #ifdef CONFIG_X86 > #include <asm/fixmap.h> > #endif > +#ifdef CONFIG_ACPI > +#include <xen/acpi.h> > +#endif > + > > /* > * Configure serial port with a string: > @@ -1565,6 +1569,69 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) > DT_DEVICE_END > > #endif /* HAS_DEVICE_TREE */ > + > +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM) I would remove the CONFIG_ARM as the spec says it is possible to use this on ARM _and_ x86 hardware. But I guess you can't as ACPI_DEVICE_START is defined to be only on ARM? > + > +static int ns16550_init_acpi(struct ns16550 **puart) > +{ > + struct acpi_table_spcr *spcr; > + int status; hmm, not acpi_status ? > + struct ns16550 *uart = &ns16550_com[0]; > + > + ns16550_init_common(uart); I would move this below the error check below.. > + > + status = acpi_get_table(ACPI_SIG_SPCR, 0, > + (struct acpi_table_header **)&spcr); > + > + if ( ACPI_FAILURE(status) ) > + { > + printk("ns16550: Failed to get SPCR table\n"); > + return -EINVAL; > + } > + > + uart->baud = BAUD_AUTO; > + uart->data_bits = 8; Are those two assumed from the ACPI spec? Wait a minute. The https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx has Baud Rate at Offset 58? > + uart->parity = spcr->parity; > + uart->stop_bits = spcr->stop_bits; > + uart->io_base = spcr->serial_port.address; > + uart->irq = spcr->interrupt; You need to check if the 'Interrupt Type' field is set before you look at this? > + uart->reg_width = spcr->serial_port.bit_width / 8; > + uart->reg_shift = 0; > + uart->io_size = UART_NUM_REGS << uart->reg_shift; > + > + irq_set_type(spcr->interrupt, spcr->interrupt_type); Um, the spec has a whole bunch of other bits set in 'interrupt_type'? > + > + *puart = uart; > + > + return 0; > +} > + > +static int __init ns16550_acpi_uart_init(const void *data) > +{ > + int ret; > + struct ns16550 *uart; > + > + ret = ns16550_init_acpi(&uart); > + if ( ret ) > + return ret; > + > + ns16550_vuart_init(uart); > + > + ns16550_register_uart(uart); > + > + return 0; > +} > + > +ACPI_DEVICE_START(ns16550c, "16550 COMPAT UART", DEVICE_SERIAL) > + .class_type = ACPI_DBG2_16550_COMPATIBLE, > + .init = ns16550_acpi_uart_init, > +ACPI_DEVICE_END > +ACPI_DEVICE_START(ns16550s, "16550 SUBSET UART", DEVICE_SERIAL) > + .class_type = ACPI_DBG2_16550_SUBSET, > + .init = ns16550_acpi_uart_init, > +ACPI_DEVICE_END > + > +#endif > /* > * Local variables: > * mode: C > diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h > index 5c3bac3..849a5c0 100644 > --- a/xen/include/xen/8250-uart.h > +++ b/xen/include/xen/8250-uart.h > @@ -35,6 +35,7 @@ > #define UART_USR 0x1f /* Status register (DW) */ > #define UART_DLL 0x00 /* divisor latch (ls) (DLAB=1) */ > #define UART_DLM 0x01 /* divisor latch (ms) (DLAB=1) */ > +#define UART_NUM_REGS (UART_USR + 1) > > /* Interrupt Enable Register */ > #define UART_IER_ERDAI 0x01 /* rx data recv'd */ > -- > 2.7.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI 2017-11-24 13:50 ` Konrad Rzeszutek Wilk @ 2017-11-24 15:11 ` Andre Przywara 2017-11-24 15:52 ` Julien Grall 0 siblings, 1 reply; 15+ messages in thread From: Andre Przywara @ 2017-11-24 15:11 UTC (permalink / raw) To: Konrad Rzeszutek Wilk, Bhupinder Thakur Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, xen-devel Hi, (answering to both Konrad and Bhupinder ...) On 24/11/17 13:50, Konrad Rzeszutek Wilk wrote: > On Fri, Nov 24, 2017 at 05:09:11PM +0530, Bhupinder Thakur wrote: >> Currently, Xen supports only DT based initialization of 16550 UART. >> This patch adds support for initializing 16550 UART using ACPI SPCR table. > > Can you provide the link to the spec you are using. I am wondering > if I am looking at some older one. > >> >> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> >> --- >> Changes since v2: >> - renamed UART_MAX_REG to UART_NUM_REGS >> - aligned some assignment statements >> - some coding style changes >> >> CC: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: George Dunlap <George.Dunlap@eu.citrix.com> >> CC: Ian Jackson <ian.jackson@eu.citrix.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Tim Deegan <tim@xen.org> >> CC: Wei Liu <wei.liu2@citrix.com> >> CC: Julien Grall <julien.grall@arm.com> >> >> xen/drivers/char/ns16550.c | 67 +++++++++++++++++++++++++++++++++++++++++++++ >> xen/include/xen/8250-uart.h | 1 + >> 2 files changed, 68 insertions(+) >> >> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c >> index c5dfc1e..af4712f 100644 >> --- a/xen/drivers/char/ns16550.c >> +++ b/xen/drivers/char/ns16550.c >> @@ -29,6 +29,10 @@ >> #ifdef CONFIG_X86 >> #include <asm/fixmap.h> >> #endif >> +#ifdef CONFIG_ACPI >> +#include <xen/acpi.h> >> +#endif >> + >> >> /* >> * Configure serial port with a string: >> @@ -1565,6 +1569,69 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) >> DT_DEVICE_END >> >> #endif /* HAS_DEVICE_TREE */ >> + >> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM) > > I would remove the CONFIG_ARM as the spec says it is possible to use > this on ARM _and_ x86 hardware. I was thinking the same. Originally the SPCR was x86 only. > But I guess you can't as ACPI_DEVICE_START is defined to be only > on ARM? We could move the #ifdef there. >> + >> +static int ns16550_init_acpi(struct ns16550 **puart) >> +{ >> + struct acpi_table_spcr *spcr; >> + int status; > > hmm, not acpi_status ? >> + struct ns16550 *uart = &ns16550_com[0]; >> + >> + ns16550_init_common(uart); > > I would move this below the error check below.. >> + >> + status = acpi_get_table(ACPI_SIG_SPCR, 0, >> + (struct acpi_table_header **)&spcr); >> + >> + if ( ACPI_FAILURE(status) ) >> + { >> + printk("ns16550: Failed to get SPCR table\n"); >> + return -EINVAL; >> + } >> + >> + uart->baud = BAUD_AUTO; >> + uart->data_bits = 8; > > Are those two assumed from the ACPI spec? I can't find a field for the number of data bits, so I assume it's indeed fixed to 8. > Wait a minute. The > https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx > has Baud Rate at Offset 58? Yes, I see the same. We should use that table. Just wondering what the Moonshot gives you in this table? Is it "7"? >> + uart->parity = spcr->parity; The Spec is a bit weird there, since it only specifies 0 as "No parity", every other value is reserved. Shall we check and bail out if !0 with an error message? >> + uart->stop_bits = spcr->stop_bits; Again if you want to be strict you would need to check against the specified values, which means only "1" is valid. >> + uart->io_base = spcr->serial_port.address; I think this needs to be checked against the address type, because we assume 0 (MMIO) here for ARM, and I guess 1 (PIO) for x86? If I understand the code correctly, we have some wild heuristics to guess the address type at the moment? #ifdef CONFIG_HAS_IOPORTS /* I/O ports are distinguished by their size (16 bits). */ if ( uart->io_base >= 0x10000 ) #endif I wonder if we should store this explicitly, since ACPI (and DT as well) give us this information. >> + uart->irq = spcr->interrupt; > > You need to check if the 'Interrupt Type' field is set before > you look at this? > >> + uart->reg_width = spcr->serial_port.bit_width / 8; I am a bit confused about the SPCR field here. Shouldn't this be encoded in the "Access Size" field instead? "Register Bit Width: The size in bits of the given register. When addressing a data structure, this field must be zero." But I guess the Moonshot gives you 4 in here, but something else in "Access Size"? Cheers, Andre. >> + uart->reg_shift = 0; >> + uart->io_size = UART_NUM_REGS << uart->reg_shift; >> + >> + irq_set_type(spcr->interrupt, spcr->interrupt_type); > > Um, the spec has a whole bunch of other bits set in 'interrupt_type'? > >> + >> + *puart = uart; >> + >> + return 0; >> +} >> + >> +static int __init ns16550_acpi_uart_init(const void *data) >> +{ >> + int ret; >> + struct ns16550 *uart; >> + >> + ret = ns16550_init_acpi(&uart); >> + if ( ret ) >> + return ret; >> + >> + ns16550_vuart_init(uart); >> + >> + ns16550_register_uart(uart); >> + >> + return 0; >> +} >> + >> +ACPI_DEVICE_START(ns16550c, "16550 COMPAT UART", DEVICE_SERIAL) >> + .class_type = ACPI_DBG2_16550_COMPATIBLE, >> + .init = ns16550_acpi_uart_init, >> +ACPI_DEVICE_END >> +ACPI_DEVICE_START(ns16550s, "16550 SUBSET UART", DEVICE_SERIAL) >> + .class_type = ACPI_DBG2_16550_SUBSET, >> + .init = ns16550_acpi_uart_init, >> +ACPI_DEVICE_END >> + >> +#endif >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h >> index 5c3bac3..849a5c0 100644 >> --- a/xen/include/xen/8250-uart.h >> +++ b/xen/include/xen/8250-uart.h >> @@ -35,6 +35,7 @@ >> #define UART_USR 0x1f /* Status register (DW) */ >> #define UART_DLL 0x00 /* divisor latch (ls) (DLAB=1) */ >> #define UART_DLM 0x01 /* divisor latch (ms) (DLAB=1) */ >> +#define UART_NUM_REGS (UART_USR + 1) >> >> /* Interrupt Enable Register */ >> #define UART_IER_ERDAI 0x01 /* rx data recv'd */ >> -- >> 2.7.4 >> >> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI 2017-11-24 15:11 ` Andre Przywara @ 2017-11-24 15:52 ` Julien Grall 0 siblings, 0 replies; 15+ messages in thread From: Julien Grall @ 2017-11-24 15:52 UTC (permalink / raw) To: Andre Przywara, Konrad Rzeszutek Wilk, Bhupinder Thakur Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, xen-devel Hi, On 24/11/17 15:11, Andre Przywara wrote: > On 24/11/17 13:50, Konrad Rzeszutek Wilk wrote: >> On Fri, Nov 24, 2017 at 05:09:11PM +0530, Bhupinder Thakur wrote: >>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c >>> index c5dfc1e..af4712f 100644 >>> --- a/xen/drivers/char/ns16550.c >>> +++ b/xen/drivers/char/ns16550.c >>> @@ -29,6 +29,10 @@ >>> #ifdef CONFIG_X86 >>> #include <asm/fixmap.h> >>> #endif >>> +#ifdef CONFIG_ACPI >>> +#include <xen/acpi.h> >>> +#endif >>> + >>> >>> /* >>> * Configure serial port with a string: >>> @@ -1565,6 +1569,69 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) >>> DT_DEVICE_END >>> >>> #endif /* HAS_DEVICE_TREE */ >>> + >>> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM) >> >> I would remove the CONFIG_ARM as the spec says it is possible to use >> this on ARM _and_ x86 hardware. > > I was thinking the same. Originally the SPCR was x86 only. Yes but Xen does not use SPCR on x86 today. So this will likely lead to a build failure as the function would not be used. Unless someone decided to plug it for x86, the right solution is to ifdef CONFIG_ARM that code. > >> But I guess you can't as ACPI_DEVICE_START is defined to be only >> on ARM? > > We could move the #ifdef there. > >>> + >>> +static int ns16550_init_acpi(struct ns16550 **puart) >>> +{ >>> + struct acpi_table_spcr *spcr; >>> + int status; >> >> hmm, not acpi_status ? >>> + struct ns16550 *uart = &ns16550_com[0]; >>> + >>> + ns16550_init_common(uart); >> >> I would move this below the error check below.. >>> + >>> + status = acpi_get_table(ACPI_SIG_SPCR, 0, >>> + (struct acpi_table_header **)&spcr); >>> + >>> + if ( ACPI_FAILURE(status) ) >>> + { >>> + printk("ns16550: Failed to get SPCR table\n"); >>> + return -EINVAL; >>> + } >>> + >>> + uart->baud = BAUD_AUTO; >>> + uart->data_bits = 8; >> >> Are those two assumed from the ACPI spec? > > I can't find a field for the number of data bits, so I assume it's > indeed fixed to 8. > >> Wait a minute. The >> https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx >> has Baud Rate at Offset 58? > > Yes, I see the same. We should use that table. > Just wondering what the Moonshot gives you in this table? Is it "7"? What is the point to use the baud rate from the table? It should have been configured by the firmware and there are no point for the driver to reconfigure it. It will likely make it worst as AFAICT we don't have the clock frequency in hand. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI 2017-11-24 11:39 ` [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI Bhupinder Thakur 2017-11-24 13:50 ` Konrad Rzeszutek Wilk @ 2017-11-24 16:05 ` Julien Grall 2017-11-27 10:01 ` Jan Beulich 2 siblings, 0 replies; 15+ messages in thread From: Julien Grall @ 2017-11-24 16:05 UTC (permalink / raw) To: Bhupinder Thakur, xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich Hi Bhupinder, On 24/11/17 11:39, 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> > --- > Changes since v2: > - renamed UART_MAX_REG to UART_NUM_REGS > - aligned some assignment statements > - some coding style changes > > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Ian Jackson <ian.jackson@eu.citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Tim Deegan <tim@xen.org> > CC: Wei Liu <wei.liu2@citrix.com> > CC: Julien Grall <julien.grall@arm.com> > > xen/drivers/char/ns16550.c | 67 +++++++++++++++++++++++++++++++++++++++++++++ > xen/include/xen/8250-uart.h | 1 + > 2 files changed, 68 insertions(+) > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > index c5dfc1e..af4712f 100644 > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -29,6 +29,10 @@ > #ifdef CONFIG_X86 > #include <asm/fixmap.h> > #endif > +#ifdef CONFIG_ACPI > +#include <xen/acpi.h> > +#endif > + > > /* > * Configure serial port with a string: > @@ -1565,6 +1569,69 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) > DT_DEVICE_END > > #endif /* HAS_DEVICE_TREE */ > + > +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM) > + > +static int ns16550_init_acpi(struct ns16550 **puart) This should be __init. But why do you need to create 2 separate functions? I don't think this bring any enhancement to the code. > +{ > + struct acpi_table_spcr *spcr; > + int status; > + struct ns16550 *uart = &ns16550_com[0]; > + > + ns16550_init_common(uart); > + > + 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_NUM_REGS << uart->reg_shift; > + > + irq_set_type(spcr->interrupt, spcr->interrupt_type); > + > + *puart = uart; > + > + return 0; > +} > + > +static int __init ns16550_acpi_uart_init(const void *data) > +{ > + int ret; > + struct ns16550 *uart; > + > + ret = ns16550_init_acpi(&uart); > + if ( ret ) > + return ret; > + > + ns16550_vuart_init(uart); > + > + ns16550_register_uart(uart); > + > + return 0; > +} > + > +ACPI_DEVICE_START(ns16550c, "16550 COMPAT UART", DEVICE_SERIAL) > + .class_type = ACPI_DBG2_16550_COMPATIBLE, > + .init = ns16550_acpi_uart_init, > +ACPI_DEVICE_END > +ACPI_DEVICE_START(ns16550s, "16550 SUBSET UART", DEVICE_SERIAL) > + .class_type = ACPI_DBG2_16550_SUBSET, > + .init = ns16550_acpi_uart_init, > +ACPI_DEVICE_END > + > +#endif > /* > * Local variables: > * mode: C > diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h > index 5c3bac3..849a5c0 100644 > --- a/xen/include/xen/8250-uart.h > +++ b/xen/include/xen/8250-uart.h > @@ -35,6 +35,7 @@ > #define UART_USR 0x1f /* Status register (DW) */ > #define UART_DLL 0x00 /* divisor latch (ls) (DLAB=1) */ > #define UART_DLM 0x01 /* divisor latch (ms) (DLAB=1) */ > +#define UART_NUM_REGS (UART_USR + 1) > > /* Interrupt Enable Register */ > #define UART_IER_ERDAI 0x01 /* rx data recv'd */ > Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI 2017-11-24 11:39 ` [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI Bhupinder Thakur 2017-11-24 13:50 ` Konrad Rzeszutek Wilk 2017-11-24 16:05 ` Julien Grall @ 2017-11-27 10:01 ` Jan Beulich 2 siblings, 0 replies; 15+ messages in thread From: Jan Beulich @ 2017-11-27 10:01 UTC (permalink / raw) To: Bhupinder Thakur Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel >>> On 24.11.17 at 12:39, <bhupinder.thakur@linaro.org> wrote: > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -29,6 +29,10 @@ > #ifdef CONFIG_X86 > #include <asm/fixmap.h> > #endif > +#ifdef CONFIG_ACPI > +#include <xen/acpi.h> > +#endif > + > > /* No double blank lines please. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform 2017-11-24 11:39 [PATCH 0/3 v3] xen: ACPI/SPCR based initialization of 8250 UART Bhupinder Thakur 2017-11-24 11:39 ` [PATCH 1/3 v3] xen: Refactor 16550 UART code Bhupinder Thakur 2017-11-24 11:39 ` [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI Bhupinder Thakur @ 2017-11-24 11:39 ` Bhupinder Thakur 2017-11-24 13:51 ` Konrad Rzeszutek Wilk 2017-11-27 10:06 ` Jan Beulich 2 siblings, 2 replies; 15+ messages in thread From: Bhupinder Thakur @ 2017-11-24 11:39 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich 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 in the following commit: 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> --- Changes since v2: - removed the use of local variable xgene_8250 in xgene_8250_erratum_present 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 | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index af4712f..8c4720a 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -1571,6 +1571,30 @@ DT_DEVICE_END #endif /* HAS_DEVICE_TREE */ #if defined(CONFIG_ACPI) && defined(CONFIG_ARM) +/* + * 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) +{ + 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 ) + return true; + + if ( !memcmp(tb->header.oem_table_id, "ProLiant", + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1 ) + return true; + + return false; +} static int ns16550_init_acpi(struct ns16550 **puart) { @@ -1596,7 +1620,19 @@ static int ns16550_init_acpi(struct ns16550 **puart) uart->io_base = spcr->serial_port.address; uart->irq = spcr->interrupt; uart->reg_width = spcr->serial_port.bit_width / 8; - uart->reg_shift = 0; + + 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_NUM_REGS << uart->reg_shift; irq_set_type(spcr->interrupt, spcr->interrupt_type); -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform 2017-11-24 11:39 ` [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform Bhupinder Thakur @ 2017-11-24 13:51 ` Konrad Rzeszutek Wilk 2017-11-27 10:06 ` Jan Beulich 1 sibling, 0 replies; 15+ messages in thread From: Konrad Rzeszutek Wilk @ 2017-11-24 13:51 UTC (permalink / raw) To: Bhupinder Thakur Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, xen-devel On Fri, Nov 24, 2017 at 05:09:12PM +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 in the following commit: > > 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> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > Changes since v2: > - removed the use of local variable xgene_8250 in xgene_8250_erratum_present > > 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 | 38 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 37 insertions(+), 1 deletion(-) > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > index af4712f..8c4720a 100644 > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -1571,6 +1571,30 @@ DT_DEVICE_END > #endif /* HAS_DEVICE_TREE */ > > #if defined(CONFIG_ACPI) && defined(CONFIG_ARM) > +/* > + * 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) > +{ > + 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 ) > + return true; > + > + if ( !memcmp(tb->header.oem_table_id, "ProLiant", > + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1 ) > + return true; > + > + return false; > +} > > static int ns16550_init_acpi(struct ns16550 **puart) > { > @@ -1596,7 +1620,19 @@ static int ns16550_init_acpi(struct ns16550 **puart) > uart->io_base = spcr->serial_port.address; > uart->irq = spcr->interrupt; > uart->reg_width = spcr->serial_port.bit_width / 8; > - uart->reg_shift = 0; > + > + 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_NUM_REGS << uart->reg_shift; > > irq_set_type(spcr->interrupt, spcr->interrupt_type); > -- > 2.7.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform 2017-11-24 11:39 ` [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform Bhupinder Thakur 2017-11-24 13:51 ` Konrad Rzeszutek Wilk @ 2017-11-27 10:06 ` Jan Beulich 2017-11-27 10:30 ` Julien Grall 1 sibling, 1 reply; 15+ messages in thread From: Jan Beulich @ 2017-11-27 10:06 UTC (permalink / raw) To: Bhupinder Thakur Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel >>> On 24.11.17 at 12:39, <bhupinder.thakur@linaro.org> wrote: > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -1571,6 +1571,30 @@ DT_DEVICE_END > #endif /* HAS_DEVICE_TREE */ > > #if defined(CONFIG_ACPI) && defined(CONFIG_ARM) > +/* > + * 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) Is this really to be considered an erratum? From the description it doesn't sound like this couldn't have been a deliberate decision. IOW - does their behavior contradict any spec? (ACPI not providing information in field and access width looks suspicious too - GAS fields exist for both.) Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform 2017-11-27 10:06 ` Jan Beulich @ 2017-11-27 10:30 ` Julien Grall 2017-11-27 10:37 ` Graeme Gregory 0 siblings, 1 reply; 15+ messages in thread From: Julien Grall @ 2017-11-27 10:30 UTC (permalink / raw) To: Jan Beulich, Bhupinder Thakur Cc: Stefano Stabellini, Wei Liu, G Gregory, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel + Graeme On 27/11/17 10:06, Jan Beulich wrote: >>>> On 24.11.17 at 12:39, <bhupinder.thakur@linaro.org> wrote: >> --- a/xen/drivers/char/ns16550.c >> +++ b/xen/drivers/char/ns16550.c >> @@ -1571,6 +1571,30 @@ DT_DEVICE_END >> #endif /* HAS_DEVICE_TREE */ >> >> #if defined(CONFIG_ACPI) && defined(CONFIG_ARM) >> +/* >> + * 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) > > Is this really to be considered an erratum? From the description it > doesn't sound like this couldn't have been a deliberate decision. > IOW - does their behavior contradict any spec? (ACPI not providing > information in field and access width looks suspicious too - GAS fields > exist for both.) I believe the problem here is the firmware table does not describe correctly the hardware. I have CCed Graeme which might be able to confirm. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform 2017-11-27 10:30 ` Julien Grall @ 2017-11-27 10:37 ` Graeme Gregory 0 siblings, 0 replies; 15+ messages in thread From: Graeme Gregory @ 2017-11-27 10:37 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich, Bhupinder Thakur, xen-devel On 27 November 2017 at 10:30, Julien Grall <julien.grall@linaro.org> wrote: > + Graeme > > On 27/11/17 10:06, Jan Beulich wrote: >>>>> >>>>> On 24.11.17 at 12:39, <bhupinder.thakur@linaro.org> wrote: >>> >>> --- a/xen/drivers/char/ns16550.c >>> +++ b/xen/drivers/char/ns16550.c >>> @@ -1571,6 +1571,30 @@ DT_DEVICE_END >>> #endif /* HAS_DEVICE_TREE */ >>> #if defined(CONFIG_ACPI) && defined(CONFIG_ARM) >>> +/* >>> + * 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) >> >> >> Is this really to be considered an erratum? From the description it >> doesn't sound like this couldn't have been a deliberate decision. >> IOW - does their behavior contradict any spec? (ACPI not providing >> information in field and access width looks suspicious too - GAS fields >> exist for both.) > > > I believe the problem here is the firmware table does not describe correctly > the hardware. I have CCed Graeme which might be able to confirm. > Yup, their firmware is wrong, we did tell them many times! Graeme _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-11-27 10:37 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-24 11:39 [PATCH 0/3 v3] xen: ACPI/SPCR based initialization of 8250 UART Bhupinder Thakur 2017-11-24 11:39 ` [PATCH 1/3 v3] xen: Refactor 16550 UART code Bhupinder Thakur 2017-11-24 13:37 ` Konrad Rzeszutek Wilk 2017-11-24 16:04 ` Julien Grall 2017-11-24 11:39 ` [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI Bhupinder Thakur 2017-11-24 13:50 ` Konrad Rzeszutek Wilk 2017-11-24 15:11 ` Andre Przywara 2017-11-24 15:52 ` Julien Grall 2017-11-24 16:05 ` Julien Grall 2017-11-27 10:01 ` Jan Beulich 2017-11-24 11:39 ` [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform Bhupinder Thakur 2017-11-24 13:51 ` Konrad Rzeszutek Wilk 2017-11-27 10:06 ` Jan Beulich 2017-11-27 10:30 ` Julien Grall 2017-11-27 10:37 ` Graeme Gregory
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.