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

[Julien Grall <julien.grall@linaro.org>]

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@linaro.org>
> 
> 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@linaro.org>
>> + * 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.

>> +{
>> +    struct dt_device_node *dev;
>> +    int ret;
>> +    u64 addr, size;
>> +    struct serial_arm_defaults defaults;
>> +    const char *devalias = opt_dtuart;
>> +
>> +    if ( !console_has("dtuart") || !strcmp(opt_dtuart, "") )
>> +    {
>> +        early_printk("No console\n");
>> +        return;
>> +    }
>> +
>> +    early_printk("Looking for UART console %s\n", devalias);
>> +    dev = dt_find_node_by_alias(devalias);
>> +
>> +    if ( !dev )
>> +    {
>> +        early_printk("Unable to find device \"%s\"\n", devalias);
>> +        return;
>> +    }
>> +
>> +    /* TODO: Handle UART with 0 or multiple base address */
>> +    ret = dt_device_get_address(dev, 0, &addr, &size);
>> +    if ( ret )
>> +    {
>> +        early_printk("Unable to retrieve the base address of the serial\n");
>> +        return;
>> +    }
>> +
>> +    clear_fixmap(FIXMAP_CONSOLE);
>> +    set_fixmap(FIXMAP_CONSOLE, addr >> PAGE_SHIFT, DEV_SHARED);
>> +
>> +    addr = FIXMAP_ADDR(FIXMAP_CONSOLE) + (addr & (PAGE_SIZE - 1));
>> +
>> +    defaults.index = 0;
>> +    defaults.register_base_address = addr;
>> +
>> +    ret = device_init(dev, DEVICE_SERIAL, &defaults);
>> +
>> +    if ( ret )
>> +        early_printk("Unable to initialize serial: %d\n", ret);
>> +}
>> diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
>> index 0ae7e4d..c4c4a84 100644
>> --- a/xen/drivers/char/serial.c
>> +++ b/xen/drivers/char/serial.c
>> @@ -271,6 +271,12 @@ int __init serial_parse_handle(char *conf)
>>          goto common;
>>      }
>>  
>> +    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.

-- 
Julien