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
IRQ can be passed as parameters of pl011_uart_init.> 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;
>
> - 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