Re: [PATCH V2 21/33] xen/arm: Use device tree API in pl011 UART driver

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

On 05/08/2013 04:17 PM, Ian Campbell wrote:

> On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote:
>> @@ -227,32 +239,56 @@ static struct uart_driver __read_mostly pl011_driver = {
>>      .tx_ready     = pl011_tx_ready,
>>      .putc         = pl011_putc,
>>      .getc         = pl011_getc,
>> -    .irq          = pl011_irq
>> +    .irq          = pl011_irq,
>> +    .dt_irq_get   = pl011_dt_irq,
>>  };
>>  
>> -/* TODO: Parse UART config from device-tree or command-line */
>> -
>> -void __init pl011_init(int index, unsigned long register_base_address)
>> +/* TODO: Parse UART config from the command line */
>> +static int __init pl011_uart_init(struct dt_device_node *dev,
>> +                                  const void *data)
>>  {
>> +    const struct serial_arm_defaults *defaults = data;
>>      struct pl011 *uart;
>> +    int res;
>>  
>> -    if ( (index < 0) || (index > 1) )
>> -        return;
>> +    if ( (defaults->index < 0) || (defaults->index > 1) )
>> +        return -EINVAL;
>>  
>> -    uart = &pl011_com[index];
>> +    uart = &pl011_com[defaults->index];
>>  
>>      uart->clock_hz  = 0x16e3600;
>>      uart->baud      = 38400;
>>      uart->data_bits = 8;
>>      uart->parity    = PARITY_NONE;
>>      uart->stop_bits = 1;
>> -    uart->irq       = 37; /* TODO Need to find this from devicetree */
>> -    uart->regs      = (uint32_t *) register_base_address;
>> +    uart->regs      = (uint32_t *) defaults->register_base_address;
> 
> Should this not come from struct dt_device_node?
> 
> Perhaps the driver needs to be instantiating its own FIXMAP mapping
> instead of doing it in common code? In fact can we not get rid of the
> fixmap for the runtime (not early) console and use the ioremap stuff
> which Stefano just enabled with his vmap patches, or the early_ioremap
> stuff if necessary.

Right. I will use ioremap in the next patch series.

> That does then call into question the whole serial_arm_defaults thing.
> Perhaps index should just be the order in which they are registered (and
> at the moment it is effectively hardcoded to 0 anyway)


I'm thinking about using serial_arm_defaults for baud rate,... For the
moment the different values are hardcoded.

{arm,dt}_init_uart will parse the dtuart parameters on the command line
and then fill serial_arm_defaults.

-- 
Julien