| Message ID | 118acc7c92ab1eac93ad2dcb25944569f4c6ae59.1453328768.git.alistair.francis@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20 January 2016 at 22:42, Alistair Francis <alistair.francis@xilinx.com> wrote: > Update the GIC ID registers (registers above 0xfe0) based on the GIC > revision instead of using the sames values for all GIC implementations. > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com> > --- > V2: > - Update array indexing to match new values > - Check the maximum value of offset as well > > hw/intc/arm_gic.c | 35 ++++++++++++++++++++++++++++++----- > 1 file changed, 30 insertions(+), 5 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 13e297d..8eb3a2d 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -31,8 +31,16 @@ do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0) > #define DPRINTF(fmt, ...) do {} while(0) > #endif > > -static const uint8_t gic_id[] = { > - 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 > +static const uint8_t gic_id_11mpcore[] = { > + 0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 > +}; > + > +static const uint8_t gic_id_gicv1[] = { > + 0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1 > +}; > + > +static const uint8_t gic_id_gicv2[] = { > + 0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1 > }; > > static inline int gic_get_current_cpu(GICState *s) > @@ -683,13 +691,30 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs) > } > > res = s->sgi_pending[irq][cpu]; > - } else if (offset < 0xfe0) { > + } else if (offset < 0xfd0) { > goto bad_reg; > - } else /* offset >= 0xfe0 */ { > + } else /* offset >= 0xfd0 */ { > if (offset & 3) { > res = 0; > + } else if (offset > 0xfdf) { > + goto bad_reg; Where did this condition come from? It will mean all the ID regs fe0 and above end up treated as bad registers rather than taking the code below to return the ID value. > } else { > - res = gic_id[(offset - 0xfe0) >> 2]; > + switch (s->revision) { > + case REV_11MPCORE: > + res = gic_id_11mpcore[(offset - 0xfd0) >> 2]; > + break; > + case 1: > + res = gic_id_gicv1[(offset - 0xfd0) >> 2]; > + break; > + case 2: > + res = gic_id_gicv2[(offset - 0xfd0) >> 2]; > + break; > + case REV_NVIC: > + /* Shouldn't be able to get here */ > + abort(); > + default: > + res = 0; > + } > } > } > return res; > -- > 2.5.0 > thanks -- PMM
On Wed, Jan 20, 2016 at 2:48 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 20 January 2016 at 22:42, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> Update the GIC ID registers (registers above 0xfe0) based on the GIC >> revision instead of using the sames values for all GIC implementations. >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com> >> --- >> V2: >> - Update array indexing to match new values >> - Check the maximum value of offset as well >> >> hw/intc/arm_gic.c | 35 ++++++++++++++++++++++++++++++----- >> 1 file changed, 30 insertions(+), 5 deletions(-) >> >> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c >> index 13e297d..8eb3a2d 100644 >> --- a/hw/intc/arm_gic.c >> +++ b/hw/intc/arm_gic.c >> @@ -31,8 +31,16 @@ do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0) >> #define DPRINTF(fmt, ...) do {} while(0) >> #endif >> >> -static const uint8_t gic_id[] = { >> - 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >> +static const uint8_t gic_id_11mpcore[] = { >> + 0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >> +}; >> + >> +static const uint8_t gic_id_gicv1[] = { >> + 0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >> +}; >> + >> +static const uint8_t gic_id_gicv2[] = { >> + 0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >> }; >> >> static inline int gic_get_current_cpu(GICState *s) >> @@ -683,13 +691,30 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs) >> } >> >> res = s->sgi_pending[irq][cpu]; >> - } else if (offset < 0xfe0) { >> + } else if (offset < 0xfd0) { >> goto bad_reg; >> - } else /* offset >= 0xfe0 */ { >> + } else /* offset >= 0xfd0 */ { >> if (offset & 3) { >> res = 0; >> + } else if (offset > 0xfdf) { >> + goto bad_reg; > > Where did this condition come from? It will mean all > the ID regs fe0 and above end up treated as bad registers > rather than taking the code below to return the ID value. Woops, I copied the wrong value. I mean that to be 0xffc Do you still want a maximum check (otherwise it will walk off the array)? Thanks, Alistair > >> } else { >> - res = gic_id[(offset - 0xfe0) >> 2]; >> + switch (s->revision) { >> + case REV_11MPCORE: >> + res = gic_id_11mpcore[(offset - 0xfd0) >> 2]; >> + break; >> + case 1: >> + res = gic_id_gicv1[(offset - 0xfd0) >> 2]; >> + break; >> + case 2: >> + res = gic_id_gicv2[(offset - 0xfd0) >> 2]; >> + break; >> + case REV_NVIC: >> + /* Shouldn't be able to get here */ >> + abort(); >> + default: >> + res = 0; >> + } >> } >> } >> return res; >> -- >> 2.5.0 >> > > thanks > -- PMM >
On 20 January 2016 at 23:08, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Wed, Jan 20, 2016 at 2:48 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 20 January 2016 at 22:42, Alistair Francis >> <alistair.francis@xilinx.com> wrote: >>> Update the GIC ID registers (registers above 0xfe0) based on the GIC >>> revision instead of using the sames values for all GIC implementations. >>> >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>> Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com> >>> --- >>> V2: >>> - Update array indexing to match new values >>> - Check the maximum value of offset as well >>> >>> hw/intc/arm_gic.c | 35 ++++++++++++++++++++++++++++++----- >>> 1 file changed, 30 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c >>> index 13e297d..8eb3a2d 100644 >>> --- a/hw/intc/arm_gic.c >>> +++ b/hw/intc/arm_gic.c >>> @@ -31,8 +31,16 @@ do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0) >>> #define DPRINTF(fmt, ...) do {} while(0) >>> #endif >>> >>> -static const uint8_t gic_id[] = { >>> - 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >>> +static const uint8_t gic_id_11mpcore[] = { >>> + 0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >>> +}; >>> + >>> +static const uint8_t gic_id_gicv1[] = { >>> + 0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >>> +}; >>> + >>> +static const uint8_t gic_id_gicv2[] = { >>> + 0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >>> }; >>> >>> static inline int gic_get_current_cpu(GICState *s) >>> @@ -683,13 +691,30 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs) >>> } >>> >>> res = s->sgi_pending[irq][cpu]; >>> - } else if (offset < 0xfe0) { >>> + } else if (offset < 0xfd0) { >>> goto bad_reg; >>> - } else /* offset >= 0xfe0 */ { >>> + } else /* offset >= 0xfd0 */ { >>> if (offset & 3) { >>> res = 0; >>> + } else if (offset > 0xfdf) { >>> + goto bad_reg; >> >> Where did this condition come from? It will mean all >> the ID regs fe0 and above end up treated as bad registers >> rather than taking the code below to return the ID value. > > Woops, I copied the wrong value. I mean that to be 0xffc > > Do you still want a maximum check (otherwise it will walk off the array)? FFD..FFF should read as zeroes (like the other non-multiple-of-4 offset addresses in this region), and you can't get more than 0xfff because the size of the memory region is 0x1000. That's not entirely obvious though, so if you want a max check you can put one in. I think that having it match the style of the rest of the if..else if ladder would be better though: ... else if (offset < 0x1000) { array code here; else { g_assert_not_reached(); } or something. thanks -- PMM
On Wed, Jan 20, 2016 at 3:37 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 20 January 2016 at 23:08, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> On Wed, Jan 20, 2016 at 2:48 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 20 January 2016 at 22:42, Alistair Francis >>> <alistair.francis@xilinx.com> wrote: >>>> Update the GIC ID registers (registers above 0xfe0) based on the GIC >>>> revision instead of using the sames values for all GIC implementations. >>>> >>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>>> Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com> >>>> --- >>>> V2: >>>> - Update array indexing to match new values >>>> - Check the maximum value of offset as well >>>> >>>> hw/intc/arm_gic.c | 35 ++++++++++++++++++++++++++++++----- >>>> 1 file changed, 30 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c >>>> index 13e297d..8eb3a2d 100644 >>>> --- a/hw/intc/arm_gic.c >>>> +++ b/hw/intc/arm_gic.c >>>> @@ -31,8 +31,16 @@ do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0) >>>> #define DPRINTF(fmt, ...) do {} while(0) >>>> #endif >>>> >>>> -static const uint8_t gic_id[] = { >>>> - 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >>>> +static const uint8_t gic_id_11mpcore[] = { >>>> + 0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >>>> +}; >>>> + >>>> +static const uint8_t gic_id_gicv1[] = { >>>> + 0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >>>> +}; >>>> + >>>> +static const uint8_t gic_id_gicv2[] = { >>>> + 0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1 >>>> }; >>>> >>>> static inline int gic_get_current_cpu(GICState *s) >>>> @@ -683,13 +691,30 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs) >>>> } >>>> >>>> res = s->sgi_pending[irq][cpu]; >>>> - } else if (offset < 0xfe0) { >>>> + } else if (offset < 0xfd0) { >>>> goto bad_reg; >>>> - } else /* offset >= 0xfe0 */ { >>>> + } else /* offset >= 0xfd0 */ { >>>> if (offset & 3) { >>>> res = 0; >>>> + } else if (offset > 0xfdf) { >>>> + goto bad_reg; >>> >>> Where did this condition come from? It will mean all >>> the ID regs fe0 and above end up treated as bad registers >>> rather than taking the code below to return the ID value. >> >> Woops, I copied the wrong value. I mean that to be 0xffc >> >> Do you still want a maximum check (otherwise it will walk off the array)? > > FFD..FFF should read as zeroes (like the other non-multiple-of-4 > offset addresses in this region), and you can't get more than 0xfff > because the size of the memory region is 0x1000. That's not > entirely obvious though, so if you want a max check you can > put one in. I think that having it match the style of the rest > of the if..else if ladder would be better though: > > ... > else if (offset < 0x1000) { > array code here; > else { > g_assert_not_reached(); > } > Ok, I like that. Thanks, Alistair > or something. > > thanks > -- PMM >
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index 13e297d..8eb3a2d 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -31,8 +31,16 @@ do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0) #define DPRINTF(fmt, ...) do {} while(0) #endif -static const uint8_t gic_id[] = { - 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 +static const uint8_t gic_id_11mpcore[] = { + 0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 +}; + +static const uint8_t gic_id_gicv1[] = { + 0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1 +}; + +static const uint8_t gic_id_gicv2[] = { + 0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1 }; static inline int gic_get_current_cpu(GICState *s) @@ -683,13 +691,30 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs) } res = s->sgi_pending[irq][cpu]; - } else if (offset < 0xfe0) { + } else if (offset < 0xfd0) { goto bad_reg; - } else /* offset >= 0xfe0 */ { + } else /* offset >= 0xfd0 */ { if (offset & 3) { res = 0; + } else if (offset > 0xfdf) { + goto bad_reg; } else { - res = gic_id[(offset - 0xfe0) >> 2]; + switch (s->revision) { + case REV_11MPCORE: + res = gic_id_11mpcore[(offset - 0xfd0) >> 2]; + break; + case 1: + res = gic_id_gicv1[(offset - 0xfd0) >> 2]; + break; + case 2: + res = gic_id_gicv2[(offset - 0xfd0) >> 2]; + break; + case REV_NVIC: + /* Shouldn't be able to get here */ + abort(); + default: + res = 0; + } } } return res;