From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen/arm: Missing +1 when then number of interrupt lines for the GIC is computed Date: Thu, 25 Apr 2013 10:52:48 +0100 Message-ID: <5178FCF0.3070005@linaro.org> References: <1366832691-27130-1-git-send-email-julien.grall@linaro.org> <1366877449.20256.410.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1366877449.20256.410.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xensource.com" , "patches@linaro.org" , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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 > > Acked-by: Ian Campbell > >> --- >> 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", > >