| Message ID | 1390581822-32624-5-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: > Actually, when the IRQ is handling by Xen, the setup is done in 2 steps: s/Actually, // I'd also go with a title like "remove need to have specific..." or "remove function to route...". > - Route the IRQ to the current CPU and set priorities > - Set up the handler > > For PPIs, these step are called on every cpus. For SPIs, it's called only ^s cpu they are only called > on the boot CPU. > > Divided the setup in two step complicated the code when a new driver is Dividing into two steps complicates > added by Xen (for instance a SMMU driver). Xen can safely route the IRQ to Xen > when the driver setup the interrupt handler. sets up Although for this final para I'm not sure why a new driver is needed -- either it is already complicated or not. > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/gic.c | 67 +++++++++++++++----------------------------- > xen/arch/arm/setup.c | 3 -- > xen/arch/arm/smpboot.c | 2 -- > xen/arch/arm/time.c | 11 -------- > xen/include/asm-arm/gic.h | 7 ----- > xen/include/asm-arm/time.h | 3 -- > 6 files changed, 23 insertions(+), 70 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index d68bde3..58bcba3 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -254,43 +254,25 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, > spin_unlock(&gic.lock); > } > > -/* Program the GIC to route an interrupt */ > +/* Program the GIC to route an interrupt to the host (eg Xen) > + * - needs to be called with desc.lock held This suggests that the caller must have desc in its hand, but it then passes irq here so we can look it up again. It may as well pass desc I think. > void __init release_irq(unsigned int irq) > { > struct irq_desc *desc; > @@ -601,6 +561,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > int rc; > unsigned long flags; > struct irq_desc *desc; > + bool_t disabled = 0; > > desc = irq_to_desc(irq->irq); > > @@ -620,6 +581,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > return -EADDRINUSE; > } > > + disabled = (desc->action == NULL); > + > + /* First time the IRQ is setup */ > + if ( disabled ) > + { > + bool_t level; > + > + level = dt_irq_is_level_triggered(irq); > + /* It's fine to use smp_processor_id() because: > + * For PPI: irq_desc is banked > + * For SGI: we don't care for now which CPU will receive the > + * interrupt > + * TODO: Handle case where SGI is setup on different CPU than > + * the targeted CPU and the priority. Do you mean s/SGI/SPI/ here? SGIs are inherently per CPU, like PPIs. > + */ > + gic_route_irq(irq->irq, level, cpumask_of(smp_processor_id()), 0xa0); > + } > + > rc = __setup_irq(desc, irq->irq, new); > spin_unlock_irqrestore(&desc->lock, flags); >
Hello Ian, On 02/19/2014 11:45 AM, Ian Campbell wrote: > On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: >> Actually, when the IRQ is handling by Xen, the setup is done in 2 steps: > > s/Actually, // > > I'd also go with a title like "remove need to have specific..." or > "remove function to route...". > >> - Route the IRQ to the current CPU and set priorities >> - Set up the handler >> >> For PPIs, these step are called on every cpus. For SPIs, it's called only > > ^s cpu they are only called > >> on the boot CPU. >> >> Divided the setup in two step complicated the code when a new driver is > > Dividing into two steps complicates > >> added by Xen (for instance a SMMU driver). Xen can safely route the IRQ > > to Xen > >> when the driver setup the interrupt handler. > > sets up Thanks to look at my grammar nits :). > Although for this final para I'm not sure why a new driver is needed -- > either it is already complicated or not. I will remove this para then. >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/arch/arm/gic.c | 67 +++++++++++++++----------------------------- >> xen/arch/arm/setup.c | 3 -- >> xen/arch/arm/smpboot.c | 2 -- >> xen/arch/arm/time.c | 11 -------- >> xen/include/asm-arm/gic.h | 7 ----- >> xen/include/asm-arm/time.h | 3 -- >> 6 files changed, 23 insertions(+), 70 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index d68bde3..58bcba3 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -254,43 +254,25 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, >> spin_unlock(&gic.lock); >> } >> >> -/* Program the GIC to route an interrupt */ >> +/* Program the GIC to route an interrupt to the host (eg Xen) >> + * - needs to be called with desc.lock held > > This suggests that the caller must have desc in its hand, but it then > passes irq here so we can look it up again. It may as well pass desc I > think. Right, I will update release_irq to take an irq_desc in parameters instead of the IRQ. > >> void __init release_irq(unsigned int irq) >> { >> struct irq_desc *desc; >> @@ -601,6 +561,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) >> int rc; >> unsigned long flags; >> struct irq_desc *desc; >> + bool_t disabled = 0; >> >> desc = irq_to_desc(irq->irq); >> >> @@ -620,6 +581,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) >> return -EADDRINUSE; >> } >> >> + disabled = (desc->action == NULL); >> + >> + /* First time the IRQ is setup */ >> + if ( disabled ) >> + { >> + bool_t level; >> + >> + level = dt_irq_is_level_triggered(irq); >> + /* It's fine to use smp_processor_id() because: >> + * For PPI: irq_desc is banked >> + * For SGI: we don't care for now which CPU will receive the >> + * interrupt >> + * TODO: Handle case where SGI is setup on different CPU than >> + * the targeted CPU and the priority. > > Do you mean s/SGI/SPI/ here? SGIs are inherently per CPU, like PPIs. Yes, SPI. Cheers,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index d68bde3..58bcba3 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -254,43 +254,25 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, spin_unlock(&gic.lock); } -/* Program the GIC to route an interrupt */ +/* Program the GIC to route an interrupt to the host (eg Xen) + * - needs to be called with desc.lock held + */ static int gic_route_irq(unsigned int irq, bool_t level, const cpumask_t *cpu_mask, unsigned int priority) { struct irq_desc *desc = irq_to_desc(irq); - unsigned long flags; ASSERT(priority <= 0xff); /* Only 8 bits of priority */ ASSERT(irq < gic.lines); /* Can't route interrupts that don't exist */ - - if ( desc->action != NULL ) - return -EBUSY; - - /* Disable interrupt */ - desc->handler->shutdown(desc); - - spin_lock_irqsave(&desc->lock, flags); + ASSERT(desc->status & IRQ_DISABLED); desc->handler = &gic_host_irq_type; gic_set_irq_properties(irq, level, cpu_mask, priority); - spin_unlock_irqrestore(&desc->lock, flags); return 0; } -/* Program the GIC to route an interrupt with a dt_irq */ -void gic_route_dt_irq(const struct dt_irq *irq, const cpumask_t *cpu_mask, - unsigned int priority) -{ - bool_t level; - - level = dt_irq_is_level_triggered(irq); - - gic_route_irq(irq->irq, level, cpu_mask, priority); -} - static void __init gic_dist_init(void) { uint32_t type; @@ -538,28 +520,6 @@ void gic_disable_cpu(void) spin_unlock(&gic.lock); } -void gic_route_ppis(void) -{ - /* GIC maintenance */ - gic_route_dt_irq(&gic.maintenance, cpumask_of(smp_processor_id()), 0xa0); - /* Route timer interrupt */ - route_timer_interrupt(); -} - -void gic_route_spis(void) -{ - int seridx; - const struct dt_irq *irq; - - for ( seridx = 0; seridx <= SERHND_IDX; seridx++ ) - { - if ( (irq = serial_dt_irq(seridx)) == NULL ) - continue; - - gic_route_dt_irq(irq, cpumask_of(smp_processor_id()), 0xa0); - } -} - void __init release_irq(unsigned int irq) { struct irq_desc *desc; @@ -601,6 +561,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) int rc; unsigned long flags; struct irq_desc *desc; + bool_t disabled = 0; desc = irq_to_desc(irq->irq); @@ -620,6 +581,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) return -EADDRINUSE; } + disabled = (desc->action == NULL); + + /* First time the IRQ is setup */ + if ( disabled ) + { + bool_t level; + + level = dt_irq_is_level_triggered(irq); + /* It's fine to use smp_processor_id() because: + * For PPI: irq_desc is banked + * For SGI: we don't care for now which CPU will receive the + * interrupt + * TODO: Handle case where SGI is setup on different CPU than + * the targeted CPU and the priority. + */ + gic_route_irq(irq->irq, level, cpumask_of(smp_processor_id()), 0xa0); + } + rc = __setup_irq(desc, irq->irq, new); spin_unlock_irqrestore(&desc->lock, flags); diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 5434784..1f6d713 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -711,9 +711,6 @@ void __init start_xen(unsigned long boot_phys_offset, init_IRQ(); - gic_route_ppis(); - gic_route_spis(); - init_maintenance_interrupt(); init_timer_interrupt(); diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index c53c765..807e7d3 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -287,8 +287,6 @@ void __cpuinit start_secondary(unsigned long boot_phys_offset, init_secondary_IRQ(); - gic_route_ppis(); - init_maintenance_interrupt(); init_timer_interrupt(); diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index 81e3e28..cd16318 100644 --- a/xen/arch/arm/time.c +++ b/xen/arch/arm/time.c @@ -218,17 +218,6 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq, 1); } -/* Route timer's IRQ on this CPU */ -void __cpuinit route_timer_interrupt(void) -{ - gic_route_dt_irq(&timer_irq[TIMER_PHYS_NONSECURE_PPI], - cpumask_of(smp_processor_id()), 0xa0); - gic_route_dt_irq(&timer_irq[TIMER_HYP_PPI], - cpumask_of(smp_processor_id()), 0xa0); - gic_route_dt_irq(&timer_irq[TIMER_VIRT_PPI], - cpumask_of(smp_processor_id()), 0xa0); -} - /* Set up the timer interrupt on this CPU */ void __cpuinit init_timer_interrupt(void) { diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 87f4298..3fd1024 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -144,13 +144,6 @@ extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq,int virtual); extern void vgic_clear_pending_irqs(struct vcpu *v); extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq); -/* Program the GIC to route an interrupt with a dt_irq */ -extern void gic_route_dt_irq(const struct dt_irq *irq, - const cpumask_t *cpu_mask, - unsigned int priority); -extern void gic_route_ppis(void); -extern void gic_route_spis(void); - extern void gic_inject(void); extern void gic_clear_pending_irqs(struct vcpu *v); extern int gic_events_need_delivery(void); diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h index 9d302d3..eaa96bc 100644 --- a/xen/include/asm-arm/time.h +++ b/xen/include/asm-arm/time.h @@ -28,9 +28,6 @@ enum timer_ppi /* Get one of the timer IRQ description */ const struct dt_irq* timer_dt_irq(enum timer_ppi ppi); -/* Route timer's IRQ on this CPU */ -extern void __cpuinit route_timer_interrupt(void); - /* Set up the timer interrupt on this CPU */ extern void __cpuinit init_timer_interrupt(void);
Actually, when the IRQ is handling by Xen, the setup is done in 2 steps: - Route the IRQ to the current CPU and set priorities - Set up the handler For PPIs, these step are called on every cpus. For SPIs, it's called only on the boot CPU. Divided the setup in two step complicated the code when a new driver is added by Xen (for instance a SMMU driver). Xen can safely route the IRQ when the driver setup the interrupt handler. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/gic.c | 67 +++++++++++++++----------------------------- xen/arch/arm/setup.c | 3 -- xen/arch/arm/smpboot.c | 2 -- xen/arch/arm/time.c | 11 -------- xen/include/asm-arm/gic.h | 7 ----- xen/include/asm-arm/time.h | 3 -- 6 files changed, 23 insertions(+), 70 deletions(-)