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

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

On 21 May 2015 at 16:58, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

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.

ok, will move into seperate patches.

> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> ---
> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel