| Message ID | 20200211133705.1398-1-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v2] arm64: timer: Speed up gic-timer-state check | expand |
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 <drjones@redhat.com> > --- > 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 <alexandru.elisei@arm.com> Thanks, Alex
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 <drjones@redhat.com> > > --- > > 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 <alexandru.elisei@arm.com> Indeed. I'll remove them and add your r-b while applying. Thanks, drew > > Thanks, > Alex >
Hi Drew, On 2020/2/11 21:37, 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 <drjones@redhat.com> [...] > + > +enum gic_irq_state gic_irq_state(int irq) This is a *generic* name while this function only deals with PPI. Maybe we can use something like gic_ppi_state() instead? Or you will have to take all interrupt types into account in a single function, which is not a easy job I think. > +{ > + 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)); And you may also want to ensure that the 'irq' is valid for PPI(). Or personally, I'd just use a real PPI number (PPI(info->irq)) as the input parameter of this function. > + 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; > +} > Otherwise, Reviewed-by: Zenghui Yu <yuzenghui@huawei.com> Tested-by: Zenghui Yu <yuzenghui@huawei.com> Thanks
On 2020/2/11 22:50, Zenghui Yu wrote: > Hi Drew, > > On 2020/2/11 21:37, 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 <drjones@redhat.com> > > [...] > >> + >> +enum gic_irq_state gic_irq_state(int irq) > > This is a *generic* name while this function only deals with PPI. > Maybe we can use something like gic_ppi_state() instead? Or you > will have to take all interrupt types into account in a single > function, which is not a easy job I think. Just to follow up, gic_irq_get_irqchip_state()/gic_peek_irq() [*] is the Linux implementation of this for PPIs and SPIs. [*] linux/drivers/irqchip/irq-gic-v3.c Thanks, Zenghui
On Tue, Feb 11, 2020 at 10:50:58PM +0800, Zenghui Yu wrote: > Hi Drew, > > On 2020/2/11 21:37, 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 <drjones@redhat.com> > > [...] > > > + > > +enum gic_irq_state gic_irq_state(int irq) > > This is a *generic* name while this function only deals with PPI. > Maybe we can use something like gic_ppi_state() instead? Or you > will have to take all interrupt types into account in a single > function, which is not a easy job I think. Very good point. > > > +{ > > + 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)); > > And you may also want to ensure that the 'irq' is valid for PPI(). > Or personally, I'd just use a real PPI number (PPI(info->irq)) as > the input parameter of this function. Indeed, if we want to make this a general function we should require the caller to pass 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; > > +} > > > > Otherwise, > > Reviewed-by: Zenghui Yu <yuzenghui@huawei.com> > Tested-by: Zenghui Yu <yuzenghui@huawei.com> Thanks, but I guess I should squash in changes to make this function more general. My GIC/SPI skills are weak, so I'm not sure this is right, especially because the SPI stuff doesn't yet have a user to validate it. However, if all reviewers think it's correct, then I'll squash it into the arm/queue branch. I've added Andre and Eric to help review too. Thanks, drew diff --git a/arm/timer.c b/arm/timer.c index ae5fdbf54b35..44621b4f2967 100644 --- a/arm/timer.c +++ b/arm/timer.c @@ -189,9 +189,9 @@ static bool gic_timer_check_state(struct timer_info *info, /* Wait for up to 1s for the GIC to sample the interrupt. */ for (i = 0; i < 10; i++) { mdelay(100); - if (gic_irq_state(info->irq) == expected_state) { + if (gic_irq_state(PPI(info->irq)) == expected_state) { mdelay(100); - if (gic_irq_state(info->irq) == expected_state) + if (gic_irq_state(PPI(info->irq)) == expected_state) return true; } } diff --git a/lib/arm/gic.c b/lib/arm/gic.c index 0563b31132c8..3924dd1d1ee6 100644 --- a/lib/arm/gic.c +++ b/lib/arm/gic.c @@ -150,22 +150,37 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest) enum gic_irq_state gic_irq_state(int irq) { enum gic_irq_state state; - bool pending = false, active = false; - void *base; + void *ispendr, *isactiver; + bool pending, active; 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)); + ispendr = gicv2_dist_base() + GICD_ISPENDR; + isactiver = gicv2_dist_base() + GICD_ISACTIVER; break; case 3: - base = gicv3_sgi_base(); - pending = readl(base + GICR_ISPENDR0) & (1 << PPI(irq)); - active = readl(base + GICR_ISACTIVER0) & (1 << PPI(irq)); + if (irq < GIC_NR_PRIVATE_IRQS) { + ispendr = gicv3_sgi_base() + GICR_ISPENDR0; + isactiver = gicv3_sgi_base() + GICR_ISACTIVER0; + } else { + ispendr = gicv3_dist_base() + GICD_ISPENDR; + isactiver = gicv3_dist_base() + GICD_ISACTIVER; + } break; + default: + assert(0); + } + + if (irq < GIC_NR_PRIVATE_IRQS) { + pending = readl(ispendr) & (1 << irq); + active = readl(isactiver) & (1 << irq); + } else { + int offset = (irq - GIC_FIRST_SPI) / 32; + int mask = 1 << ((irq - GIC_FIRST_SPI) % 32); + pending = readl(ispendr + offset) & mask; + active = readl(isactiver + offset) & mask; } if (!active && !pending)
On Tue, Feb 11, 2020 at 11:32:14PM +0800, Zenghui Yu wrote: > On 2020/2/11 22:50, Zenghui Yu wrote: > > Hi Drew, > > > > On 2020/2/11 21:37, 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 <drjones@redhat.com> > > > > [...] > > > > > + > > > +enum gic_irq_state gic_irq_state(int irq) > > > > This is a *generic* name while this function only deals with PPI. > > Maybe we can use something like gic_ppi_state() instead? Or you > > will have to take all interrupt types into account in a single > > function, which is not a easy job I think. > > Just to follow up, gic_irq_get_irqchip_state()/gic_peek_irq() [*] is > the Linux implementation of this for PPIs and SPIs. > > [*] linux/drivers/irqchip/irq-gic-v3.c > Thanks. I just skimmed that now and it looks like the diff I sent is pretty close. But, I do see a bug in my diff (missing '* 4' on the offset calculation). Thanks, drew
Hi Drew, On 2020/2/11 23:41, Andrew Jones wrote: > On Tue, Feb 11, 2020 at 10:50:58PM +0800, Zenghui Yu wrote: >> Hi Drew, >> >> On 2020/2/11 21:37, 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 <drjones@redhat.com> >> >> [...] >> >>> + >>> +enum gic_irq_state gic_irq_state(int irq) >> >> This is a *generic* name while this function only deals with PPI. >> Maybe we can use something like gic_ppi_state() instead? Or you >> will have to take all interrupt types into account in a single >> function, which is not a easy job I think. > > Very good point. > >> >>> +{ >>> + 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)); >> >> And you may also want to ensure that the 'irq' is valid for PPI(). >> Or personally, I'd just use a real PPI number (PPI(info->irq)) as >> the input parameter of this function. > > Indeed, if we want to make this a general function we should require > the caller to pass 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; >>> +} >>> >> >> Otherwise, >> >> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com> >> Tested-by: Zenghui Yu <yuzenghui@huawei.com> > > Thanks, but I guess I should squash in changes to make this function more > general. My GIC/SPI skills are weak, so I'm not sure this is right, > especially because the SPI stuff doesn't yet have a user to validate it. (I guess the PL031 can be another new user.) > However, if all reviewers think it's correct, then I'll squash it into > the arm/queue branch. I've added Andre and Eric to help review too. > > Thanks, > drew > > > diff --git a/arm/timer.c b/arm/timer.c > index ae5fdbf54b35..44621b4f2967 100644 > --- a/arm/timer.c > +++ b/arm/timer.c > @@ -189,9 +189,9 @@ static bool gic_timer_check_state(struct timer_info *info, > /* Wait for up to 1s for the GIC to sample the interrupt. */ > for (i = 0; i < 10; i++) { > mdelay(100); > - if (gic_irq_state(info->irq) == expected_state) { > + if (gic_irq_state(PPI(info->irq)) == expected_state) { > mdelay(100); > - if (gic_irq_state(info->irq) == expected_state) > + if (gic_irq_state(PPI(info->irq)) == expected_state) > return true; > } > } > diff --git a/lib/arm/gic.c b/lib/arm/gic.c > index 0563b31132c8..3924dd1d1ee6 100644 > --- a/lib/arm/gic.c > +++ b/lib/arm/gic.c > @@ -150,22 +150,37 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest) > enum gic_irq_state gic_irq_state(int irq) > { > enum gic_irq_state state; > - bool pending = false, active = false; > - void *base; > + void *ispendr, *isactiver; > + bool pending, active; > > assert(gic_common_ops); We can also assert that only interrupts with ID smaller than 1020 will be handled. > > 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)); > + ispendr = gicv2_dist_base() + GICD_ISPENDR; > + isactiver = gicv2_dist_base() + GICD_ISACTIVER; > break; > case 3: > - base = gicv3_sgi_base(); > - pending = readl(base + GICR_ISPENDR0) & (1 << PPI(irq)); > - active = readl(base + GICR_ISACTIVER0) & (1 << PPI(irq)); > + if (irq < GIC_NR_PRIVATE_IRQS) { > + ispendr = gicv3_sgi_base() + GICR_ISPENDR0; > + isactiver = gicv3_sgi_base() + GICR_ISACTIVER0; > + } else { > + ispendr = gicv3_dist_base() + GICD_ISPENDR; > + isactiver = gicv3_dist_base() + GICD_ISACTIVER; > + } > break; > + default: > + assert(0); > + } > + > + if (irq < GIC_NR_PRIVATE_IRQS) { > + pending = readl(ispendr) & (1 << irq); > + active = readl(isactiver) & (1 << irq); > + } else { > + int offset = (irq - GIC_FIRST_SPI) / 32; > + int mask = 1 << ((irq - GIC_FIRST_SPI) % 32); No need to minus GIC_FIRST_SPI. And therefore these two cases can actually be merged. > + pending = readl(ispendr + offset) & mask; > + active = readl(isactiver + offset) & mask; > } > > if (!active && !pending) Otherwise this looks good enough (to me) for now, and let's wait other reviewers to comment. I've used the following diff to give the pl031 test a go (roughly written, not dig into PL031 so much), it just works fine :) Thanks, Zenghui diff --git a/arm/pl031.c b/arm/pl031.c index 86035fa..2d4160f 100644 --- a/arm/pl031.c +++ b/arm/pl031.c @@ -118,11 +118,12 @@ static int check_rtc_freq(void) return 0; } -static bool gic_irq_pending(void) +static bool gic_pl031_pending(void) { - uint32_t offset = (pl031_irq / 32) * 4; + enum gic_irq_state state = gic_irq_state(pl031_irq); - return readl(gic_ispendr + offset) & (1 << (pl031_irq & 31)); + return state == GIC_IRQ_STATE_PENDING || + state == GIC_IRQ_STATE_ACTIVE_PENDING; } static void gic_irq_unmask(void) [...] /* replace all gic_irq_pending() with gic_pl031_pending() */ [...] diff --git a/lib/arm/gic.c b/lib/arm/gic.c index 3924dd1..34d77e3 100644 --- a/lib/arm/gic.c +++ b/lib/arm/gic.c @@ -152,6 +152,7 @@ enum gic_irq_state gic_irq_state(int irq) enum gic_irq_state state; void *ispendr, *isactiver; bool pending, active; + int offset, mask; assert(gic_common_ops); @@ -173,15 +174,10 @@ enum gic_irq_state gic_irq_state(int irq) assert(0); } - if (irq < GIC_NR_PRIVATE_IRQS) { - pending = readl(ispendr) & (1 << irq); - active = readl(isactiver) & (1 << irq); - } else { - int offset = (irq - GIC_FIRST_SPI) / 32; - int mask = 1 << ((irq - GIC_FIRST_SPI) % 32); - pending = readl(ispendr + offset) & mask; - active = readl(isactiver + offset) & mask; - } + offset = (irq / 32) * 4; + mask = 1 << (irq % 32); + pending = readl(ispendr + offset) & mask; + active = readl(isactiver + offset) & mask; if (!active && !pending) state = GIC_IRQ_STATE_INACTIVE;
On Wed, Feb 12, 2020 at 10:57:09AM +0800, Zenghui Yu wrote: > Hi Drew, > > On 2020/2/11 23:41, Andrew Jones wrote: > > On Tue, Feb 11, 2020 at 10:50:58PM +0800, Zenghui Yu wrote: > > > Hi Drew, > > > > > > On 2020/2/11 21:37, 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 <drjones@redhat.com> > > > > > > [...] > > > > > > > + > > > > +enum gic_irq_state gic_irq_state(int irq) > > > > > > This is a *generic* name while this function only deals with PPI. > > > Maybe we can use something like gic_ppi_state() instead? Or you > > > will have to take all interrupt types into account in a single > > > function, which is not a easy job I think. > > > > Very good point. > > > > > > > > > +{ > > > > + 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)); > > > > > > And you may also want to ensure that the 'irq' is valid for PPI(). > > > Or personally, I'd just use a real PPI number (PPI(info->irq)) as > > > the input parameter of this function. > > > > Indeed, if we want to make this a general function we should require > > the caller to pass 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; > > > > +} > > > > > > > > > > Otherwise, > > > > > > Reviewed-by: Zenghui Yu <yuzenghui@huawei.com> > > > Tested-by: Zenghui Yu <yuzenghui@huawei.com> > > > > Thanks, but I guess I should squash in changes to make this function more > > general. My GIC/SPI skills are weak, so I'm not sure this is right, > > especially because the SPI stuff doesn't yet have a user to validate it. > > (I guess the PL031 can be another new user.) > > > However, if all reviewers think it's correct, then I'll squash it into > > the arm/queue branch. I've added Andre and Eric to help review too. > > > > Thanks, > > drew > > > > > > diff --git a/arm/timer.c b/arm/timer.c > > index ae5fdbf54b35..44621b4f2967 100644 > > --- a/arm/timer.c > > +++ b/arm/timer.c > > @@ -189,9 +189,9 @@ static bool gic_timer_check_state(struct timer_info *info, > > /* Wait for up to 1s for the GIC to sample the interrupt. */ > > for (i = 0; i < 10; i++) { > > mdelay(100); > > - if (gic_irq_state(info->irq) == expected_state) { > > + if (gic_irq_state(PPI(info->irq)) == expected_state) { > > mdelay(100); > > - if (gic_irq_state(info->irq) == expected_state) > > + if (gic_irq_state(PPI(info->irq)) == expected_state) > > return true; > > } > > } > > diff --git a/lib/arm/gic.c b/lib/arm/gic.c > > index 0563b31132c8..3924dd1d1ee6 100644 > > --- a/lib/arm/gic.c > > +++ b/lib/arm/gic.c > > @@ -150,22 +150,37 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest) > > enum gic_irq_state gic_irq_state(int irq) > > { > > enum gic_irq_state state; > > - bool pending = false, active = false; > > - void *base; > > + void *ispendr, *isactiver; > > + bool pending, active; > > assert(gic_common_ops); > > We can also assert that only interrupts with ID smaller than 1020 > will be handled. Good idea > > > 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)); > > + ispendr = gicv2_dist_base() + GICD_ISPENDR; > > + isactiver = gicv2_dist_base() + GICD_ISACTIVER; > > break; > > case 3: > > - base = gicv3_sgi_base(); > > - pending = readl(base + GICR_ISPENDR0) & (1 << PPI(irq)); > > - active = readl(base + GICR_ISACTIVER0) & (1 << PPI(irq)); > > + if (irq < GIC_NR_PRIVATE_IRQS) { > > + ispendr = gicv3_sgi_base() + GICR_ISPENDR0; > > + isactiver = gicv3_sgi_base() + GICR_ISACTIVER0; > > + } else { > > + ispendr = gicv3_dist_base() + GICD_ISPENDR; > > + isactiver = gicv3_dist_base() + GICD_ISACTIVER; > > + } > > break; > > + default: > > + assert(0); > > + } > > + > > + if (irq < GIC_NR_PRIVATE_IRQS) { > > + pending = readl(ispendr) & (1 << irq); > > + active = readl(isactiver) & (1 << irq); > > + } else { > > + int offset = (irq - GIC_FIRST_SPI) / 32; > > + int mask = 1 << ((irq - GIC_FIRST_SPI) % 32); > > No need to minus GIC_FIRST_SPI. And therefore these two cases > can actually be merged. Yup, will do > > > + pending = readl(ispendr + offset) & mask; > > + active = readl(isactiver + offset) & mask; > > } > > if (!active && !pending) > > Otherwise this looks good enough (to me) for now, and let's wait > other reviewers to comment. I've used the following diff to give > the pl031 test a go (roughly written, not dig into PL031 so much), > it just works fine :) > > > Thanks, > Zenghui > > diff --git a/arm/pl031.c b/arm/pl031.c > index 86035fa..2d4160f 100644 > --- a/arm/pl031.c > +++ b/arm/pl031.c > @@ -118,11 +118,12 @@ static int check_rtc_freq(void) > return 0; > } > > -static bool gic_irq_pending(void) > +static bool gic_pl031_pending(void) > { > - uint32_t offset = (pl031_irq / 32) * 4; > + enum gic_irq_state state = gic_irq_state(pl031_irq); > > - return readl(gic_ispendr + offset) & (1 << (pl031_irq & 31)); > + return state == GIC_IRQ_STATE_PENDING || > + state == GIC_IRQ_STATE_ACTIVE_PENDING; Nice way to test, but I'll leave this change out. > } > > static void gic_irq_unmask(void) > > [...] > /* replace all gic_irq_pending() with gic_pl031_pending() */ > [...] > > diff --git a/lib/arm/gic.c b/lib/arm/gic.c > index 3924dd1..34d77e3 100644 > --- a/lib/arm/gic.c > +++ b/lib/arm/gic.c > @@ -152,6 +152,7 @@ enum gic_irq_state gic_irq_state(int irq) > enum gic_irq_state state; > void *ispendr, *isactiver; > bool pending, active; > + int offset, mask; > > assert(gic_common_ops); > > @@ -173,15 +174,10 @@ enum gic_irq_state gic_irq_state(int irq) > assert(0); > } > > - if (irq < GIC_NR_PRIVATE_IRQS) { > - pending = readl(ispendr) & (1 << irq); > - active = readl(isactiver) & (1 << irq); > - } else { > - int offset = (irq - GIC_FIRST_SPI) / 32; > - int mask = 1 << ((irq - GIC_FIRST_SPI) % 32); > - pending = readl(ispendr + offset) & mask; > - active = readl(isactiver + offset) & mask; > - } > + offset = (irq / 32) * 4; > + mask = 1 << (irq % 32); > + pending = readl(ispendr + offset) & mask; > + active = readl(isactiver + offset) & mask; > > if (!active && !pending) > state = GIC_IRQ_STATE_INACTIVE; > I'll send a final patch now for review, but I'm pretty happy with this so I've gone ahead and squashed it into arm/queue already. I kept Alex's r-b as there shouldn't be any functional change with respect to what he reviewed. Thanks, drew
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; +}
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 <drjones@redhat.com> --- arm/timer.c | 30 ++++++++++++------------------ lib/arm/asm/gic.h | 11 ++++++----- lib/arm/gic.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 23 deletions(-)