On Mon, Jan 11, 2016 at 5:58 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 8 January 2016 at 18:57, Alistair Francis
> <alistair.fran...@xilinx.com> wrote:
>> The ARM GIC documentation (page 4-119) describes that bits
>> 7 to 4 of the ICPIDR2 register should include the GIC architecture
>> version. This patche ORs the version into the existing return value.
>>
>> Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com>
>> Tested-by: Sören Brinkmann <soren.brinkm...@xilinx.com>
>> ---
>>
>> hw/intc/arm_gic.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 13e297d..f47d606 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -688,6 +688,10 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr
>> offset, MemTxAttrs attrs)
>> } else /* offset >= 0xfe0 */ {
>> if (offset & 3) {
>> res = 0;
>> + } else if (offset == 0xfe8 && s->revision != REV_11MPCORE &&
>> + s->revision != REV_NVIC) {
>> + /* ICPIDR2 includes the GICv1 or GICv2 version information */
>> + res = gic_id[(offset - 0xfe0) >> 2] | (s->revision << 4);
>> } else {
>> res = gic_id[(offset - 0xfe0) >> 2];
>> }
>
> The current implementation of the ID registers seems to be
> simply "like the 11MPCore interrupt controller". I think we
> should get them right more generally if we're going to fix them:
>
> fd0 .. fdc fe0 .. fec ff0 ... ffc
> 11MPCore reserved 90 13 04 00 0d f0 05 b1
> ARM GICv1 (eg A9) 04 00 00 00 90 b3 1b 00 0d f0 05 b1
> ARM GICv2 (eg A15) 04 00 00 00 90 b4 2b 00 0d f0 05 b1
>
> Your patch doesn't account for ICPIDR1 also having a field that
> changes between GICv1 and GICv2 (for ARM implementations), and
> we're missing the fd0..fdc bytes.
>
> I think this is probably simplest modelled with several
> gic_id arrays and using the appropriate one for 11MPCore/GICv1/GICv2,
> rather than trying to OR extra values into the 11MPCore ID values.
Ok, just to make sure I am reading this right. You think I should just
create three arrays and then if() the revision to determine which one
to use?
Thanks,
Alistair
>
> For the NVIC these registers don't exist at all, and the
> construction of the memory regions in armv7m_nvic.c ensures
> we can't reach this bit of the function, so we don't need
> to specially handle it. (Also there's a rewrite of the NVIC
> to disentangle it from the GIC which hopefully will land
> some time before 2.6.)
>
> thanks
> -- PMM
>
