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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> 
> 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_...

> +        /**

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 #.

> +        /* 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?

Ian.


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