[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc



On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
> For now, ARM uses different IRQ functions to setup an interrupt handler. This
> is a bit annoying for common driver because we have to add idefery when
> an IRQ is setup (see ns16550_init_postirq for an example).
> 
> To avoid to completely fork the IRQ management code, we can introduce a field
> to store the IRQ type (e.g level/edge ...).
> 
> This patch also adds platform_get_irq which will retrieve the IRQ from the
> device tree and setup correctly the IRQ type.
> 
> In order to use this solution, we have to move init_IRQ earlier for the boot
> CPU. It's fine because the code only depends on percpu.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> 
> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/gic.c        |   21 +++++++-----
>  xen/arch/arm/irq.c        |   80 
> ++++++++++++++++++++++++++++++++++++++++-----
>  xen/arch/arm/setup.c      |    3 +-
>  xen/include/asm-arm/gic.h |    5 ++-
>  xen/include/asm-arm/irq.h |    3 ++
>  5 files changed, 91 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 9127ecf..ec2994e 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -223,15 +223,20 @@ static hw_irq_controller gic_guest_irq_type = {
>  
>  /*
>   * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
> + * - desc.lock must be held
>   * already called gic_cpu_init

I think you've injected that line in the middle of a sentence ;-)

>   */
> -static void gic_set_irq_properties(unsigned int irq, bool_t level,
> +static void gic_set_irq_properties(struct irq_desc *desc,
>                                     const cpumask_t *cpu_mask,
>                                     unsigned int priority)
>  {
>      volatile unsigned char *bytereg;
>      uint32_t cfg, edgebit;
>      unsigned int mask;
> +    unsigned int irq = desc->irq;
> +    unsigned int type = desc->arch.type;
> +
> +    ASSERT(spin_is_locked(&desc->lock));
>  
>      spin_lock(&gic.lock);
>  
> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, 
> bool_t level,
>      /* Set edge / level */
>      cfg = GICD[GICD_ICFGR + irq / 16];
>      edgebit = 2u << (2 * (irq % 16));
> -    if ( level )
> +    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )

Is getting DT_IRQ_TYPE_NONE here not an error?

Oh, I see this is the innards of dt_irq_is_level_triggered. Could that
be refactored e.g. into dt_irq_type_is_level_triggered(const something
type)?

>          cfg &= ~edgebit;
>      else
>          cfg |= edgebit;

> @@ -82,6 +83,12 @@ static int __cpuinit init_local_irq_data(void)
>          init_one_irq_desc(desc);
>          desc->irq = irq;
>          desc->action  = NULL;
> +
> +        /* PPIs are include in local_irqs, we have to copy the IRQ type from
> +         * CPU0 otherwise we may miss the type if the IRQ type has been
> +         * set early.
> +         */
> +        desc->arch.type = per_cpu(local_irq_desc, 0)[irq].arch.type;

I thought we had a boot_cpu(foo) accessor, but I guess not (or at least
I can't find it right now).

That might be nicer to add than adding a hardcoded 0 (I suppose it isn't
the only one though).

> @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const 
> cpumask_t *mask)
>      BUG();
>  }
>  
> +static inline int irq_set_type(struct irq_desc *desc, unsigned int type)
> +{
> +    unsigned int flags;
> +    int ret = -EBUSY;
> +
> +    if ( type == DT_IRQ_TYPE_NONE )
> +        return 0;
> +
> +    spin_lock_irqsave(&desc->lock, flags);
> +
> +    if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type )
> +        goto err;
> +
> +    desc->arch.type = type;

There was an open coded assignment in the guest path which unfortunately
I already trimmed. Shouldn't that have all these checks too?

> +
> +    ret = 0;
> +
> +err:
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +    return ret;
> +}
> +
> +unsigned int platform_get_irq(const struct dt_device_node *device,
> +                              int index)
> +{
> +    struct dt_irq dt_irq;
> +    struct irq_desc *desc;
> +    unsigned int type, irq;
> +    int res;
> +
> +    res = dt_device_get_irq(device, index, &dt_irq);
> +    if ( res )
> +        return 0;

Not an error? Do we take precautions against IRQ0 being actually used
somewhere?

We should have an explicit #define for an invalid IRQ number.

> +    irq = dt_irq.irq;
> +    type = dt_irq.type;
> +
> +    /* Setup the IRQ type */
> +
> +    if ( irq < NR_LOCAL_IRQS )
> +    {
> +        unsigned int cpu;
> +        /* For PPIs, we need to set IRQ type on every online CPUs */
> +        for_each_cpu( cpu, &cpu_online_map )
> +        {
> +            desc = &per_cpu(local_irq_desc, cpu)[irq];
> +            res = irq_set_type(desc, type);
> +            if ( res )
> +                return 0;

Error?

Also no need to undo any partial work?

I haven't seen the caller yet, but for PPIs do we not get called for
each CPU as it binds to the PPI anyway?

> +        }
> +    }
> +    else
> +    {
> +        res = irq_set_type(irq_to_desc(irq), type);
> +        if ( res )
> +            return 0;
> +    }
> +
> +    return irq;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C

> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index b52c26f..107c13a 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -16,6 +16,7 @@ struct arch_pirq
>  
>  struct arch_irq_desc {
>      int eoi_cpu;
> +    unsigned int type;

I was wondering through the above if this ought to be a "bool_t level"
or not. Thoughts?

>  };
>  
>  #define NR_LOCAL_IRQS        32
> @@ -46,6 +47,8 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction 
> *new);
>  
>  int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>                            const char *devname);
> +unsigned int platform_get_irq(const struct dt_device_node *device,
> +                              int index);
>  
>  #endif /* _ASM_HW_IRQ_H */
>  /*



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.