| Message ID | 861da5c44f261ee3f160607c9d128ce0e9f89e9e.1452277501.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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 >
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]; }