On 21 May 2015 at 16:58, Julien Grall <julien.gr...@citrix.com> 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.di...@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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
