Re: [Xen-devel] [PATCH V2 20/33] xen/arm: Add generic UART to get the device in the device tree

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-05-08 at 16:58 +0100, Julien Grall wrote:
> On 05/08/2013 03:01 PM, Ian Campbell wrote:
> 
> > On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote:
> >> This generic UART will find the right UART via xen command line
> >> with dtuart=myserial.
> > 
> > I suppose there way to determine sensible default, since it differs on
> > every platform?
> > 
> >> "myserial" is the alias of the UART in the device tree. Xen will retrieve
> >> the information via the device tree and call the initialization function 
> >> for
> >> this specific UART thanks to the device API.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > 
> > You need to CC code code maintainers when you change core code. Keir
> > added.
> >>
> >> Changes in v2:
> >>     - Use dtuart parameter instead of com1. The first one is more arm
> >>     while the latter is more x86
> >> ---
> >>  xen/arch/arm/setup.c         |    3 +-
> >>  xen/drivers/char/Makefile    |    1 +
> >>  xen/drivers/char/arm-uart.c  |   81 
> >> ++++++++++++++++++++++++++++++++++++++++++
> >>  xen/drivers/char/serial.c    |    6 ++++
> >>  xen/include/asm-arm/config.h |    2 +-
> >>  xen/include/xen/serial.h     |    7 ++++
> >>  6 files changed, 98 insertions(+), 2 deletions(-)
> >>  create mode 100644 xen/drivers/char/arm-uart.c
> >>
> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> >> index e4228f7..7b2df8b 100644
> >> --- a/xen/arch/arm/setup.c
> >> +++ b/xen/arch/arm/setup.c
> >> @@ -435,8 +435,9 @@ void __init start_xen(unsigned long boot_phys_offset,
> >>  #ifdef EARLY_UART_ADDRESS
> >>      /* TODO Need to get device tree or command line for UART address */
> >>      pl011_init(0, FIXMAP_ADDR(FIXMAP_CONSOLE));
> >> -    console_init_preirq();
> >>  #endif
> >> +    arm_uart_init();
> >> +    console_init_preirq();
> >>  
> >>      /* FIXME: Do something smarter */
> >>      dt_switch_to_printk();
> >> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> >> index ab2246d..e68a54a 100644
> >> --- a/xen/drivers/char/Makefile
> >> +++ b/xen/drivers/char/Makefile
> >> @@ -2,4 +2,5 @@ obj-y += console.o
> >>  obj-$(HAS_NS16550) += ns16550.o
> >>  obj-$(HAS_PL011) += pl011.o
> >>  obj-$(HAS_EHCI) += ehci-dbgp.o
> >> +obj-$(CONFIG_ARM) += arm-uart.o
> >>  obj-y += serial.o
> >> diff --git a/xen/drivers/char/arm-uart.c b/xen/drivers/char/arm-uart.c
> >> new file mode 100644
> >> index 0000000..c76875e
> >> --- /dev/null
> >> +++ b/xen/drivers/char/arm-uart.c
> >> @@ -0,0 +1,81 @@
> >> +/*
> >> + * xen/drivers/char/arm-uart.c
> >> + *
> >> + * Generic ARM uart retrieved via the device tree
> >> + *
> >> + * Julien Grall <julien.grall@xxxxxxxxxx>
> >> + * Copyright (c) 2013 Linaro Limited.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +
> >> +#include <asm/device.h>
> >> +#include <asm/early_printk.h>
> >> +#include <asm/types.h>
> >> +#include <xen/console.h>
> >> +#include <xen/device_tree.h>
> >> +#include <xen/mm.h>
> >> +#include <xen/serial.h>
> >> +
> >> +/*
> >> + * Configure UART port with a string:
> >> + * alias
> >> + *
> >> + * @alias: alias used in the device tree for the UART
> >> + * TODO: Implement config  in each UART driver.
> >> + */
> >> +static char __initdata opt_dtuart[30] = "";
> >> +string_param("dtuart", opt_dtuart);
> >> +
> >> +void __init arm_uart_init(void)
> > 
> > This (and the file and struct serial_arm_defaults etc) could perhaps be
> > better called dt_uart_init etc? There's nothing inherently ARM specific
> > here.
> 
> The FIXMAP_CONSOLE is ARM specific it can not works on x86. I'm
> wondering if it's usefull to move FIXMAP_CONSOLE on each UART driver.

I think I came to the same conclusion later in the series too, or more
likely the uart drivers should use ioremap.

> >> +    if ( !strncmp(conf, "dtuart", 5) )
> >> +    {
> >> +        handle = SERHND_COM1;
> > 
> > Do you mean COM1 here? Or did you intend to add SERHND_DT?
> 
> I mean COM1, I use the first serial slot for dtuart. I don't really see
> why we need to extend the number of serial slot.

I just dislike using things with one name for another purpose.

Ultimately this bit is up to Keir though.

Ian.



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