Re: [PATCH V2 22/33] xen/arm: Use the device tree to map the address range and IRQ to dom0

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

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

> On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote:
>> - gic_route_irq_to_guest takes a dt_irq instead of an IRQ number
>> - remove hardcoded address/IRQ
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> Changes in v2:
>>     - Use the new function dt_irq_is_level_trigger
>>     - Disable DEBUG_DT by default
>>     - Rename parse_device_tree to map_device_from_device_tree
> 
> Should be map_devices_...

Will be fix on the next patch series.

>> +        /**
> 
> Javadoc!
> 
>> +         * Don't map IRQ that have no physical meaning
>> +         * ie: IRQ whose controller is not the GIC
>> +         */
>> +        if ( rirq.controller != dt_interrupt_controller )
>> +        {
>> +            DPRINT("irq %u skipped it controller (%s)\n",
> 
> you might mean "skipped its controller" ? But I think a clearer message
> would be "irq %u not connected to primary controller" or something.
> 
>> +                   i, dt_node_full_name(rirq.controller));
>> +            continue;
>> +        }
>> +
>> +        res = dt_irq_translate(&rirq, &irq);
>> +        if ( res )
>> +        {
>> +            printk(XENLOG_ERR "Unable to translate irq %u for %s\n",
>> +                   i, dt_node_full_name(dev));
>> +            return res;
>> +        }
>> +
>> +        DPRINT("irq %u = %u type = %#x\n", i, irq.irq, irq.type);
> 
> The # format specifier is required by $STANDARD to not be all that
> sensible and/or consistent when when the value is 0, i.e.
> 	printf("%#x\n", 0) => "0"
> 	printf("%#x\n", 1) => "0x1"
> Worse if you use widths then:
>         printf("%#02x\n", 0); => "00"
>         printf("%#02x\n", 1); => "0x1"
>         printf("%#04x\n", 0); => "0000"
>     printf("%#04x\n", 1); => "0x01"
>         
> For this reason we tend to avoid # and just use "0x%...", assuming
> irq==0 is a possibility, and likewise below addr==0 it's probably better
> to avoid #.

Thanks for this hint. I will replace all my %#x by 0x%x.

>> +        /* Don't check return because the IRQ can be use by multiple device */
> 
> "used by"
> 
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index f7b9889..ddad0c8 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -692,13 +692,14 @@ void gic_inject(void)
>>          gic_inject_irq_start();
>>  }
>>  
>> -int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
>> +int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>>                             const char * devname)
>>  {
>>      struct irqaction *action;
>> -    struct irq_desc *desc = irq_to_desc(irq);
>> +    struct irq_desc *desc = irq_to_desc(irq->irq);
>>      unsigned long flags;
>>      int retval;
>> +    bool_t level;
>>  
>>      action = xmalloc(struct irqaction);
>>      if (!action)
>> @@ -706,6 +707,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
>>  
>>      action->dev_id = d;
>>      action->name = devname;
>> +    action->free_on_release = 1;
>>  
>>      spin_lock_irqsave(&desc->lock, flags);
>>      spin_lock(&gic.lock);
>> @@ -713,9 +715,11 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
>>      desc->handler = &gic_guest_irq_type;
>>      desc->status |= IRQ_GUEST;
>>  
>> -    gic_set_irq_properties(irq, 1, 1u << smp_processor_id(), 0xa0);
>> +    level = dt_irq_is_level_trigger(irq);
>> +
>> +    gic_set_irq_properties(irq->irq, level, 1u << smp_processor_id(), 0xa0);
> 
> By the end of this series are all callers going through the above dance?
> git_set_irq_properties could take a dt_irq?


No. I can add patch to use a dt_irq.

-- 
Julien