Re: [PATCH 1/3 v3] xen: Refactor 16550 UART code

[Julien Grall <julien.grall@linaro.org>]

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