On Tue, Feb 11, 2020 at 01:52:35PM +0000, Alexandru Elisei wrote: > Hi, > > On 2/11/20 1:37 PM, Andrew Jones wrote: > > Let's bail out of the wait loop if we see the expected state > > to save over six seconds of run time. Make sure we wait a bit > > before reading the registers and double check again after, > > though, to somewhat mitigate the chance of seeing the expected > > state by accident. > > > > We also take this opportunity to push more IRQ state code to > > the library. > > > > Signed-off-by: Andrew Jones <[email protected]> > > --- > > arm/timer.c | 30 ++++++++++++------------------ > > lib/arm/asm/gic.h | 11 ++++++----- > > lib/arm/gic.c | 33 +++++++++++++++++++++++++++++++++ > > 3 files changed, 51 insertions(+), 23 deletions(-) > > > > diff --git a/arm/timer.c b/arm/timer.c > > index f5cf775ce50f..3c4e27f20e2e 100644 > > --- a/arm/timer.c > > +++ b/arm/timer.c > > @@ -183,28 +183,22 @@ static bool timer_pending(struct timer_info *info) > > (info->read_ctl() & ARCH_TIMER_CTL_ISTATUS); > > } > > > > -static enum gic_state gic_timer_state(struct timer_info *info) > > +static bool gic_timer_check_state(struct timer_info *info, > > + enum gic_irq_state expected_state) > > { > > - enum gic_state state = GIC_STATE_INACTIVE; > > int i; > > - bool pending, active; > > > > /* Wait for up to 1s for the GIC to sample the interrupt. */ > > for (i = 0; i < 10; i++) { > > - pending = readl(gic_ispendr) & (1 << PPI(info->irq)); > > - active = readl(gic_isactiver) & (1 << PPI(info->irq)); > > - if (!active && !pending) > > - state = GIC_STATE_INACTIVE; > > - if (pending) > > - state = GIC_STATE_PENDING; > > - if (active) > > - state = GIC_STATE_ACTIVE; > > - if (active && pending) > > - state = GIC_STATE_ACTIVE_PENDING; > > mdelay(100); > > + if (gic_irq_state(info->irq) == expected_state) { > > + mdelay(100); > > + if (gic_irq_state(info->irq) == expected_state) > > + return true; > > + } > > } > > > > - return state; > > + return false; > > } > > > > static bool test_cval_10msec(struct timer_info *info) > > @@ -253,11 +247,11 @@ static void test_timer(struct timer_info *info) > > /* Enable the timer, but schedule it for much later */ > > info->write_cval(later); > > info->write_ctl(ARCH_TIMER_CTL_ENABLE); > > - report(!timer_pending(info) && gic_timer_state(info) == > > GIC_STATE_INACTIVE, > > + report(!timer_pending(info) && gic_timer_check_state(info, > > GIC_IRQ_STATE_INACTIVE), > > "not pending before"); > > > > info->write_cval(now - 1); > > - report(timer_pending(info) && gic_timer_state(info) == > > GIC_STATE_PENDING, > > + report(timer_pending(info) && gic_timer_check_state(info, > > GIC_IRQ_STATE_PENDING), > > "interrupt signal pending"); > > > > /* Disable the timer again and prepare to take interrupts */ > > @@ -265,12 +259,12 @@ static void test_timer(struct timer_info *info) > > info->irq_received = false; > > set_timer_irq_enabled(info, true); > > report(!info->irq_received, "no interrupt when timer is disabled"); > > - report(!timer_pending(info) && gic_timer_state(info) == > > GIC_STATE_INACTIVE, > > + report(!timer_pending(info) && gic_timer_check_state(info, > > GIC_IRQ_STATE_INACTIVE), > > "interrupt signal no longer pending"); > > > > info->write_cval(now - 1); > > info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK); > > - report(timer_pending(info) && gic_timer_state(info) == > > GIC_STATE_INACTIVE, > > + report(timer_pending(info) && gic_timer_check_state(info, > > GIC_IRQ_STATE_INACTIVE), > > "interrupt signal not pending"); > > > > report(test_cval_10msec(info), "latency within 10 ms"); > > diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h > > index a72e0cde4e9c..922cbe95750c 100644 > > --- a/lib/arm/asm/gic.h > > +++ b/lib/arm/asm/gic.h > > @@ -47,11 +47,11 @@ > > #ifndef __ASSEMBLY__ > > #include <asm/cpumask.h> > > > > -enum gic_state { > > - GIC_STATE_INACTIVE, > > - GIC_STATE_PENDING, > > - GIC_STATE_ACTIVE, > > - GIC_STATE_ACTIVE_PENDING, > > +enum gic_irq_state { > > + GIC_IRQ_STATE_INACTIVE, > > + GIC_IRQ_STATE_PENDING, > > + GIC_IRQ_STATE_ACTIVE, > > + GIC_IRQ_STATE_ACTIVE_PENDING, > > }; > > > > /* > > @@ -80,6 +80,7 @@ extern u32 gic_iar_irqnr(u32 iar); > > extern void gic_write_eoir(u32 irqstat); > > extern void gic_ipi_send_single(int irq, int cpu); > > extern void gic_ipi_send_mask(int irq, const cpumask_t *dest); > > +extern enum gic_irq_state gic_irq_state(int irq); > > > > #endif /* !__ASSEMBLY__ */ > > #endif /* _ASMARM_GIC_H_ */ > > diff --git a/lib/arm/gic.c b/lib/arm/gic.c > > index 94301169215c..0563b31132c8 100644 > > --- a/lib/arm/gic.c > > +++ b/lib/arm/gic.c > > @@ -146,3 +146,36 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest) > > assert(gic_common_ops && gic_common_ops->ipi_send_mask); > > gic_common_ops->ipi_send_mask(irq, dest); > > } > > + > > +enum gic_irq_state gic_irq_state(int irq) > > +{ > > + enum gic_irq_state state; > > + bool pending = false, active = false; > > + void *base; > > + > > + assert(gic_common_ops); > > + > > + switch (gic_version()) { > > + case 2: > > + base = gicv2_dist_base(); > > + pending = readl(base + GICD_ISPENDR) & (1 << PPI(irq)); > > + active = readl(base + GICD_ISACTIVER) & (1 << PPI(irq)); > > + break; > > + case 3: > > + base = gicv3_sgi_base(); > > + pending = readl(base + GICR_ISPENDR0) & (1 << PPI(irq)); > > + active = readl(base + GICR_ISACTIVER0) & (1 << PPI(irq)); > > + break; > > + } > > + > > + if (!active && !pending) > > + state = GIC_IRQ_STATE_INACTIVE; > > + if (pending) > > + state = GIC_IRQ_STATE_PENDING; > > + if (active) > > + state = GIC_IRQ_STATE_ACTIVE; > > + if (active && pending) > > + state = GIC_IRQ_STATE_ACTIVE_PENDING; > > + > > + return state; > > +} > > Looks good. The gic_ispendr and gic_isactiver variables are not used anymore > and > could be removed, but it's not a big deal. Either way: > > Reviewed-by: Alexandru Elisei <[email protected]>
Indeed. I'll remove them and add your r-b while applying. Thanks, drew > > Thanks, > Alex > _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
