| Message ID | 1366832691-27130-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, 2013-04-24 at 20:44 +0100, Julien Grall wrote: > In the GIC manual, the number of interrupt lines is computed with the following > formula: 32(N + 1) where N is the value retrieved from GICD_TYPER. My copy of the manual says "The ITLinesNumber field only indicates the maximum number of SPIs that the GIC might support", which excludes SGIs and PPIs. On the other hand it also says that 0b0011 == 128 interrupts, with ID 0..127, and elsewhere it includes SPI and PPI in the term interrupts. So it's not really clear, but I think your interpretation is likely correct. The impact of getting this count wrong is that currently we don't initialise the final 32 interrupts worth of the GICD_FOOn registers, is that right? I was concerned that we special case the first 32 SG/PPI interrupts in various other places, but we don't appear to use gic.lines anywhere other than that. BTW, I also happened across section 3.1.2 "Identifying the supported interrupts" which recommends probing GICD_ISENABLERn to determine which of the 32*(N+1) interrupts are actually available us, might be one for the todo list (would lead to a useful debugging message "request_irq: IRQ%d is not available"). > Signed-off-by: Julien Grall <julien.grall@linaro.org> Acked-by: Ian Campbell <ian.campbell@citrix.com> > --- > xen/arch/arm/gic.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 760c86b..389c217 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -216,7 +216,7 @@ static int gic_route_irq(unsigned int irq, bool_t level, > > ASSERT(!(cpu_mask & ~0xff)); /* Targets bitmap only supports 8 CPUs */ > ASSERT(priority <= 0xff); /* Only 8 bits of priority */ > - ASSERT(irq < gic.lines + 32); /* Can't route interrupts that don't exist */ > + ASSERT(irq < gic.lines); /* Can't route interrupts that don't exist */ > > spin_lock_irqsave(&desc->lock, flags); > spin_lock(&gic.lock); > @@ -264,7 +264,7 @@ static void __init gic_dist_init(void) > GICD[GICD_CTLR] = 0; > > type = GICD[GICD_TYPER]; > - gic.lines = 32 * (type & GICD_TYPE_LINES); > + gic.lines = 32 * ((type & GICD_TYPE_LINES) + 1); > gic.cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5); > printk("GIC: %d lines, %d cpu%s%s (IID %8.8x).\n", > gic.lines, gic.cpus, (gic.cpus == 1) ? "" : "s",
On 04/25/2013 09:10 AM, Ian Campbell wrote: > On Wed, 2013-04-24 at 20:44 +0100, Julien Grall wrote: >> In the GIC manual, the number of interrupt lines is computed with the following >> formula: 32(N + 1) where N is the value retrieved from GICD_TYPER. > > My copy of the manual says "The ITLinesNumber field only indicates the > maximum number of SPIs that the GIC might support", which excludes SGIs > and PPIs. On the other hand it also says that 0b0011 == 128 interrupts, > with ID 0..127, and elsewhere it includes SPI and PPI in the term > interrupts. > > So it's not really clear, but I think your interpretation is likely > correct. Perhaps we need to add a comment to describe the "lines" field to avoid confusion later. > The impact of getting this count wrong is that currently we don't > initialise the final 32 interrupts worth of the GICD_FOOn registers, is > that right? Right. > I was concerned that we special case the first 32 SG/PPI interrupts in > various other places, but we don't appear to use gic.lines anywhere > other than that. > > BTW, I also happened across section 3.1.2 "Identifying the supported > interrupts" which recommends probing GICD_ISENABLERn to determine which > of the 32*(N+1) interrupts are actually available us, might be one for > the todo list (would lead to a useful debugging message "request_irq: > IRQ%d is not available"). > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > >> --- >> xen/arch/arm/gic.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 760c86b..389c217 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -216,7 +216,7 @@ static int gic_route_irq(unsigned int irq, bool_t level, >> >> ASSERT(!(cpu_mask & ~0xff)); /* Targets bitmap only supports 8 CPUs */ >> ASSERT(priority <= 0xff); /* Only 8 bits of priority */ >> - ASSERT(irq < gic.lines + 32); /* Can't route interrupts that don't exist */ >> + ASSERT(irq < gic.lines); /* Can't route interrupts that don't exist */ >> >> spin_lock_irqsave(&desc->lock, flags); >> spin_lock(&gic.lock); >> @@ -264,7 +264,7 @@ static void __init gic_dist_init(void) >> GICD[GICD_CTLR] = 0; >> >> type = GICD[GICD_TYPER]; >> - gic.lines = 32 * (type & GICD_TYPE_LINES); >> + gic.lines = 32 * ((type & GICD_TYPE_LINES) + 1); >> gic.cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5); >> printk("GIC: %d lines, %d cpu%s%s (IID %8.8x).\n", >> gic.lines, gic.cpus, (gic.cpus == 1) ? "" : "s", > >
On Thu, 2013-04-25 at 10:52 +0100, Julien Grall wrote: > On 04/25/2013 09:10 AM, Ian Campbell wrote: > > > On Wed, 2013-04-24 at 20:44 +0100, Julien Grall wrote: > >> In the GIC manual, the number of interrupt lines is computed with the following > >> formula: 32(N + 1) where N is the value retrieved from GICD_TYPER. > > > > My copy of the manual says "The ITLinesNumber field only indicates the > > maximum number of SPIs that the GIC might support", which excludes SGIs > > and PPIs. On the other hand it also says that 0b0011 == 128 interrupts, > > with ID 0..127, and elsewhere it includes SPI and PPI in the term > > interrupts. > > > > So it's not really clear, but I think your interpretation is likely > > correct. > > Perhaps we need to add a comment to describe the "lines" field to avoid > confusion later. Yes, please. > > The impact of getting this count wrong is that currently we don't > > initialise the final 32 interrupts worth of the GICD_FOOn registers, is > > that right? > > > Right. If you are respinning please could you mention this in the changelog, it's the headline feature of the patch really ;-) Ian.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 760c86b..389c217 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -216,7 +216,7 @@ static int gic_route_irq(unsigned int irq, bool_t level, ASSERT(!(cpu_mask & ~0xff)); /* Targets bitmap only supports 8 CPUs */ ASSERT(priority <= 0xff); /* Only 8 bits of priority */ - ASSERT(irq < gic.lines + 32); /* Can't route interrupts that don't exist */ + ASSERT(irq < gic.lines); /* Can't route interrupts that don't exist */ spin_lock_irqsave(&desc->lock, flags); spin_lock(&gic.lock); @@ -264,7 +264,7 @@ static void __init gic_dist_init(void) GICD[GICD_CTLR] = 0; type = GICD[GICD_TYPER]; - gic.lines = 32 * (type & GICD_TYPE_LINES); + gic.lines = 32 * ((type & GICD_TYPE_LINES) + 1); gic.cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5); printk("GIC: %d lines, %d cpu%s%s (IID %8.8x).\n", gic.lines, gic.cpus, (gic.cpus == 1) ? "" : "s",
In the GIC manual, the number of interrupt lines is computed with the following formula: 32(N + 1) where N is the value retrieved from GICD_TYPER. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/gic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)