From: Julien Grall <julien.grall@linaro.org>
To: Bhupinder Thakur <bhupinder.thakur@linaro.org>,
xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>,
Wei Liu <wei.liu2@citrix.com>,
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
George Dunlap <George.Dunlap@eu.citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
Julien Grall <julien.grall@arm.com>,
Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH 1/2 v2] xen: Add support for initializing 16550 UART using ACPI
Date: Mon, 13 Nov 2017 18:51:22 +0000 [thread overview]
Message-ID: <c2711ec8-7e9f-a804-f563-bb929207bbe6@linaro.org> (raw)
In-Reply-To: <1510222764-11746-2-git-send-email-bhupinder.thakur@linaro.org>
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
next prev parent reply other threads:[~2017-11-13 18:51 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-09 10:19 [PATCH 0/2 v2] xen: ACPI/SPCR based initialization of 8250 UART Bhupinder Thakur
2017-11-09 10:19 ` [PATCH 1/2 v2] xen: Add support for initializing 16550 UART using ACPI Bhupinder Thakur
2017-11-09 11:31 ` Roger Pau Monné
2017-11-09 13:18 ` Jan Beulich
2017-11-09 15:07 ` Roger Pau Monné
2017-11-09 15:26 ` Jan Beulich
2017-11-15 11:01 ` Bhupinder Thakur
2017-11-13 18:51 ` Julien Grall [this message]
2017-11-15 8:41 ` Bhupinder Thakur
2017-11-09 10:19 ` [PATCH 2/2 v2] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform Bhupinder Thakur
2017-11-15 21:20 ` Konrad Rzeszutek Wilk
2017-11-16 9:56 ` George Dunlap
2017-11-21 9:13 ` Bhupinder Thakur
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c2711ec8-7e9f-a804-f563-bb929207bbe6@linaro.org \
--to=julien.grall@linaro.org \
--cc=George.Dunlap@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=bhupinder.thakur@linaro.org \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=julien.grall@arm.com \
--cc=konrad.wilk@oracle.com \
--cc=sstabellini@kernel.org \
--cc=tim@xen.org \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.