Re: [PATCH v2 20/41] arm : create generic uart initialization function

[Julien Grall <julien.grall@citrix.com>]

Hi Parth,

On 17/05/15 21:03, Parth Dixit wrote:
> Rename dt-uart.c to arm-uart.c and create new generic uart init function.
> move dt_uart_init to uart_init.Refactor pl011 driver to dt and common
> initialization parts. This will be useful later when acpi specific
> uart initialization function is introduced.

There is 2 distinct changes in this patch:
	- Introduction of the generic UART
	- Refactoring of PL011

Each changes should be in a separate patch for helping the review.

> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> ---
>  xen/arch/arm/setup.c       |   2 +-
>  xen/drivers/char/Makefile  |   2 +-
>  xen/drivers/char/dt-uart.c | 107 ---------------------------------------------
>  xen/drivers/char/pl011.c   |  47 ++++++++++++--------
>  xen/include/xen/serial.h   |   3 +-
>  5 files changed, 33 insertions(+), 128 deletions(-)
>  delete mode 100644 xen/drivers/char/dt-uart.c
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 5711077..1b2d74c 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -771,7 +771,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      gic_preinit();
>  
> -    dt_uart_init();
> +    uart_init();

As said by Jan, this is arm specific. I would rename into arm_uart_init.

>      console_init_preirq();
>      console_init_ring();
>  
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index 47fc3f9..a8f65c1 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -6,5 +6,5 @@ obj-$(HAS_EXYNOS4210) += exynos4210-uart.o
>  obj-$(HAS_OMAP) += omap-uart.o
>  obj-$(HAS_SCIF) += scif-uart.o
>  obj-$(HAS_EHCI) += ehci-dbgp.o
> -obj-$(CONFIG_ARM) += dt-uart.o
> +obj-$(CONFIG_ARM) += arm-uart.o
>  obj-y += serial.o

> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index 67e6df5..f0c3daf 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -225,9 +225,32 @@ static struct uart_driver __read_mostly pl011_driver = {
>      .stop_tx      = pl011_tx_stop,
>      .vuart_info   = pl011_vuart,
>  };
> +static int __init pl011_uart_init(struct pl011 *uart, u64 addr, u64 size)
> +{
> +    uart->clock_hz  = 0x16e3600;
> +    uart->baud      = BAUD_AUTO;
> +    uart->data_bits = 8;
> +    uart->parity    = PARITY_NONE;
> +    uart->stop_bits = 1;
> +
> +    uart->regs = ioremap_nocache(addr, size);
> +    if ( !uart->regs )
> +    {
> +        printk("pl011: Unable to map the UART memory\n");
> +        return -ENOMEM;
> +    }
> +
> +    uart->vuart.base_addr = addr;
> +    uart->vuart.size = size;
> +    uart->vuart.data_off = DR;
> +    uart->vuart.status_off = FR;
> +    uart->vuart.status = 0;
> +
> +    return 0;
> +}
>  
>  /* TODO: Parse UART config from the command line */
> -static int __init pl011_uart_init(struct dt_device_node *dev,
> +static int __init dt_pl011_uart_init(struct dt_device_node *dev,
>                                    const void *data)
>  {
>      const char *config = data;
> @@ -242,12 +265,6 @@ static int __init pl011_uart_init(struct dt_device_node *dev,
>  
>      uart = &pl011_com;
>  
> -    uart->clock_hz  = 0x16e3600;
> -    uart->baud      = BAUD_AUTO;
> -    uart->data_bits = 8;
> -    uart->parity    = PARITY_NONE;
> -    uart->stop_bits = 1;
> -
>      res = dt_device_get_address(dev, 0, &addr, &size);
>      if ( res )
>      {
> @@ -264,19 +281,13 @@ static int __init pl011_uart_init(struct dt_device_node *dev,
>      }
>      uart->irq = res;

IRQ can be passed as parameters of pl011_uart_init.

>  
> -    uart->regs = ioremap_nocache(addr, size);
> -    if ( !uart->regs )
> +    res = pl011_uart_init(uart, addr, size);
> +    if ( res < 0 )
>      {
> -        printk("pl011: Unable to map the UART memory\n");
> -        return -ENOMEM;
> +        printk("pl011: Unable to initialize\n");
> +        return res;
>      }
>  
> -    uart->vuart.base_addr = addr;
> -    uart->vuart.size = size;
> -    uart->vuart.data_off = DR;
> -    uart->vuart.status_off = FR;
> -    uart->vuart.status = 0;
> -
>      /* Register with generic serial driver. */
>      serial_register_uart(SERHND_DTUART, &pl011_driver, uart);

This could be moved in pl011_uart_init. With the other changes suggested
above, you don't need anymore the variable uart here and the code would
be more compact.

>  
> @@ -293,7 +304,7 @@ static const struct dt_device_match pl011_dt_match[] __initconst =
>  
>  DT_DEVICE_START(pl011, "PL011 UART", DEVICE_SERIAL)
>          .dt_match = pl011_dt_match,
> -        .init = pl011_uart_init,
> +        .init = dt_pl011_uart_init,
>  DT_DEVICE_END
>  
>  /*
> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index 71e6ade..484a6a8 100644
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -98,6 +98,7 @@ struct uart_driver {
>  #define SERHND_HI       (1<<2) /* Mux/demux each transferred char by MSB. */
>  #define SERHND_LO       (1<<3) /* Ditto, except that the MSB is cleared.  */
>  #define SERHND_COOKED   (1<<4) /* Newline/carriage-return translation?    */
> +#define SERHND_UART     (0<<0) /* handler configured from ACPI */

Why did you introduce a new SERHND rather than renaming SERHND_DTUART?

Regards,

-- 
Julien Grall