[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 Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote: > >> @@ -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? > > No, there is some DT like the exynos one which is using 0 (i.e > DT_IRQ_TYPE_NONE) for the IRQ type. The underlying physical interrupt must be one or the other though, surely? So either there is some implicit (or perhaps documented?) assumption that NONE==something or the DT is buggy. > > I guess we have to skip setting level/edge property in this case. > > > 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)? > > I was wondering something like that instead: > > if ( (type & DT_IRQ_TYPE_LEVEL_MASK) ) > ... > else if ( (type & DT_IRQ_TYPE_EDGE_BOTH) ) > ... > > So we skip the DT_IRQ_TYPE_NONE. Well, it seems the existing code treats NONE as == level, I don't know if that is deliberate or not. > >> @@ -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? > > No, because with patch #11 the desc->arch.type is only set once by IRQ. I don't follow. What is all this stuff above for if that is the case? Was I misremembering the other instance of desc->arch.type = type? > > >> + > >> + 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? > > Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI. Ah yes. > > We should have an explicit #define for an invalid IRQ number. > > I don't think it's useful because the device tree can't provide an IRQ > smaller than 16. It would also potentially serve to make the code more self-documenting. "return INVALID_IRQ" and "if (irq == INVALID_IRQ)" are a bit more obvious than "return 0" and "if (irq == 0)" (I suppose "if (!irq)" is ok and more normal though) > > >> + 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? > > desc->arch.type should be sync on every CPU. It would be crazy to have a > different IRQ type on every CPU. Well, the code as it stands appears to make a partial attempt at handling just that. If that weren't the case irq_set_type wouldn't be able to fail for cpu > 0. > >> 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? > > We have 5 possibles types: > - default : we don't have to set the edge bit > - level high/low > - edge rising/falling OK. > Even if GICv2 is only handling 2 types, it can change for the next > version of GIC. Well, there aren't all that many more ways you can frob a physical line. I suppose MSIs will account for one more though. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |