* [Qemu-devel] [PATCH v1 1/1] arm_gic: Include the GIC ArchRev in the ICPIDR2 register
@ 2016-01-08 18:57 Alistair Francis
2016-01-11 13:58 ` Peter Maydell
0 siblings, 1 reply; 5+ messages in thread
From: Alistair Francis @ 2016-01-08 18:57 UTC (permalink / raw
To: qemu-devel, peter.maydell; +Cc: qemu-arm, soren.brinkmann, alistair.francis
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.francis@xilinx.com>
Tested-by: Sören Brinkmann <soren.brinkmann@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];
}
--
2.5.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] arm_gic: Include the GIC ArchRev in the ICPIDR2 register
2016-01-08 18:57 [Qemu-devel] [PATCH v1 1/1] arm_gic: Include the GIC ArchRev in the ICPIDR2 register Alistair Francis
@ 2016-01-11 13:58 ` Peter Maydell
2016-01-18 21:41 ` Alistair Francis
0 siblings, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2016-01-11 13:58 UTC (permalink / raw
To: Alistair Francis; +Cc: qemu-arm, QEMU Developers, Soren Brinkmann
On 8 January 2016 at 18:57, Alistair Francis
<alistair.francis@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.francis@xilinx.com>
> Tested-by: Sören Brinkmann <soren.brinkmann@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.
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] arm_gic: Include the GIC ArchRev in the ICPIDR2 register
2016-01-11 13:58 ` Peter Maydell
@ 2016-01-18 21:41 ` Alistair Francis
2016-01-18 22:13 ` Peter Maydell
0 siblings, 1 reply; 5+ messages in thread
From: Alistair Francis @ 2016-01-18 21:41 UTC (permalink / raw
To: Peter Maydell
Cc: qemu-arm, QEMU Developers, Soren Brinkmann, Alistair Francis
On Mon, Jan 11, 2016 at 5:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 January 2016 at 18:57, Alistair Francis
> <alistair.francis@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.francis@xilinx.com>
>> Tested-by: Sören Brinkmann <soren.brinkmann@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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] arm_gic: Include the GIC ArchRev in the ICPIDR2 register
2016-01-18 21:41 ` Alistair Francis
@ 2016-01-18 22:13 ` Peter Maydell
2016-01-19 1:34 ` Alistair Francis
0 siblings, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2016-01-18 22:13 UTC (permalink / raw
To: Alistair Francis; +Cc: qemu-arm, QEMU Developers, Soren Brinkmann
On 18 January 2016 at 21:41, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Mon, Jan 11, 2016 at 5:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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?
Yes, something like that. The fd0..fdc being reserved for 11MPCore
is documented to mean RAZ/WI, so you can just make those array elements
zeroes.
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] arm_gic: Include the GIC ArchRev in the ICPIDR2 register
2016-01-18 22:13 ` Peter Maydell
@ 2016-01-19 1:34 ` Alistair Francis
0 siblings, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2016-01-19 1:34 UTC (permalink / raw
To: Peter Maydell
Cc: qemu-arm, QEMU Developers, Soren Brinkmann, Alistair Francis
On Mon, Jan 18, 2016 at 2:13 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 January 2016 at 21:41, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Mon, Jan 11, 2016 at 5:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 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?
>
> Yes, something like that. The fd0..fdc being reserved for 11MPCore
> is documented to mean RAZ/WI, so you can just make those array elements
> zeroes.
Ok, sounds good to me. I'm about to send a patch out. As basically the
whole patch and title changed it is back to V1.
Thanks,
Alistair
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-01-19 1:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-08 18:57 [Qemu-devel] [PATCH v1 1/1] arm_gic: Include the GIC ArchRev in the ICPIDR2 register Alistair Francis
2016-01-11 13:58 ` Peter Maydell
2016-01-18 21:41 ` Alistair Francis
2016-01-18 22:13 ` Peter Maydell
2016-01-19 1:34 ` Alistair Francis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.