Hi,
I was going over this part of the gic setup code ( gic.c:gic_dist_init ):
type = GICD[GICD_TYPER];
gic.lines = 32 * (type & GICD_TYPE_LINES);
....
/* Disable all global interrupts */
for ( i = 32; i < gic.lines; i += 32 )
GICD[GICD_ICENABLER + i / 32] = (uint32_t)~0ul;
The ARM GIC manual says about the ITLinesNumber bits of the TYPE register:
Indicates the maximum number of interrupts that the GIC supports. If
ITLinesNumber=N, the
maximum number of interrupts is 32(N+1). The interrupt ID range is
from 0 to (number of IDs – 1). For example:
0b00011
Up to 128 interrupt lines, interrupt IDs 0-127.
Shouldn''t it be gic.lines = 32 * ((type & GICD_TYPE_LINES)+1); ? I
might be wrong but it seems like this way we are missing the last
register in those loops?
Regards,
Sander
On 06/04/2013 08:42 PM, Sander Bogaert wrote:> Hi,Hi,> I was going over this part of the gic setup code ( gic.c:gic_dist_init ): > > type = GICD[GICD_TYPER]; > gic.lines = 32 * (type & GICD_TYPE_LINES); > .... > /* Disable all global interrupts */ > for ( i = 32; i < gic.lines; i += 32 ) > GICD[GICD_ICENABLER + i / 32] = (uint32_t)~0ul;It seems you have an old tree :). A commit (2f2a3e31) was pushed at the end of april to fix this issue. Cheers, -- Julien
I forgot to cc Sander. On 06/04/2013 10:21 PM, Julien Grall wrote:> On 06/04/2013 08:42 PM, Sander Bogaert wrote: > >> Hi, > > Hi, > >> I was going over this part of the gic setup code ( gic.c:gic_dist_init ): >> >> type = GICD[GICD_TYPER]; >> gic.lines = 32 * (type & GICD_TYPE_LINES); >> .... >> /* Disable all global interrupts */ >> for ( i = 32; i < gic.lines; i += 32 ) >> GICD[GICD_ICENABLER + i / 32] = (uint32_t)~0ul; > > It seems you have an old tree :). A commit (2f2a3e31) was pushed at the > end of april to fix this issue. > > Cheers, >
On 4 June 2013 23:23, Julien Grall <julien.grall@citrix.com> wrote:> I forgot to cc Sander. > > On 06/04/2013 10:21 PM, Julien Grall wrote: > >> On 06/04/2013 08:42 PM, Sander Bogaert wrote: >> >>> Hi, >> >> Hi, >> >>> I was going over this part of the gic setup code ( gic.c:gic_dist_init ): >>> >>> type = GICD[GICD_TYPER]; >>> gic.lines = 32 * (type & GICD_TYPE_LINES); >>> .... >>> /* Disable all global interrupts */ >>> for ( i = 32; i < gic.lines; i += 32 ) >>> GICD[GICD_ICENABLER + i / 32] = (uint32_t)~0ul; >> >> It seems you have an old tree :). A commit (2f2a3e31) was pushed at the >> end of april to fix this issue.Ok I see :) Having different trees because of the other issue I mailed about and I was looking in the wrong one. But now that I''m at it anyway, same part of the code: /* Default all global IRQs to level, active low */ for ( i = 32; i < gic.lines; i += 16 ) GICD[GICD_ICFGR + i / 16] = 0x0; /* Route all global IRQs to this CPU */ for ( i = 32; i < gic.lines; i += 4 ) GICD[GICD_ICFGR + i / 4] = cpumask; Shouldn''t that second loop use the GICD_ITARGETSR register? Just aksing :-) Seems strange to set the same field with different values right after eachother also the += 4 loop doesn''t make sense for it since it''s two bit each interrupt line. It would make sense for ITARGETSR.>> >> Cheers, >> > >
On Wed, 2013-06-05 at 10:29 +0200, Sander Bogaert wrote:> On 4 June 2013 23:23, Julien Grall <julien.grall@citrix.com> wrote: > > I forgot to cc Sander. > > > > On 06/04/2013 10:21 PM, Julien Grall wrote: > > > >> On 06/04/2013 08:42 PM, Sander Bogaert wrote: > >> > >>> Hi, > >> > >> Hi, > >> > >>> I was going over this part of the gic setup code ( gic.c:gic_dist_init ): > >>> > >>> type = GICD[GICD_TYPER]; > >>> gic.lines = 32 * (type & GICD_TYPE_LINES); > >>> .... > >>> /* Disable all global interrupts */ > >>> for ( i = 32; i < gic.lines; i += 32 ) > >>> GICD[GICD_ICENABLER + i / 32] = (uint32_t)~0ul; > >> > >> It seems you have an old tree :). A commit (2f2a3e31) was pushed at the > >> end of april to fix this issue. > > Ok I see :) Having different trees because of the other issue I mailed > about and I was looking in the wrong one. But now that I''m at it > anyway, same part of the code: > > /* Default all global IRQs to level, active low */ > for ( i = 32; i < gic.lines; i += 16 ) > GICD[GICD_ICFGR + i / 16] = 0x0; > > /* Route all global IRQs to this CPU */ > for ( i = 32; i < gic.lines; i += 4 ) > GICD[GICD_ICFGR + i / 4] = cpumask; > > Shouldn''t that second loop use the GICD_ITARGETSR register? Just > aksing :-) Seems strange to set the same field with different values > right after eachother also the += 4 loop doesn''t make sense for it > since it''s two bit each interrupt line. It would make sense for > ITARGETSR.Yeah, that looks like a bug...