|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |