[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 04/07/2014 04:03 PM, Ian Campbell wrote: >> /* >> * - 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 ;-) Oops right. >> */ >> -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? No, there is some DT like the exynos one which is using 0 (i.e DT_IRQ_TYPE_NONE) for the IRQ type. 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. > >> 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). It's also used in mm.c when page tables is setup. I will introduce boot_cpu macro. >> @@ -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. >> + >> + 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. > 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. >> + 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. > 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? No, this function is only called once, when the DT is parsed at startup. Basically, this function will replace dt_device_get_irq call. >> + } >> + } >> + 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? We have 5 possibles types: - default : we don't have to set the edge bit - level high/low - edge rising/falling Even if GICv2 is only handling 2 types, it can change for the next version of GIC. Regards, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |