diff mbox

[v3,9/9] arm: make number of a9mpcore GIC interrupts configurable

Message ID 1325016827-11503-10-git-send-email-mark.langsdorf@calxeda.com
State New
Headers show

Commit Message

Mark Langsdorf Dec. 27, 2011, 8:13 p.m. UTC 
Increase the maximum number of GIC interrupts for a9mp to 192, and
create a configurable property defaulting to 96 so that device
modelers can set the value appropriately for their SoC.

Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
---
Changes from v2
	Skipped
Changes from v1
	Increase the number of a9mp interrupts to 192
	Add a property defaulting to 96
	Add a num_irq member in the gic state structure
	Use the num_irq value as appropriate
	Add num_irq argument to gic_init()
	Add num_irq to various CPU calls to gic_init

 hw/a9mpcore.c     |    9 +++++++--
 hw/arm11mpcore.c  |    2 +-
 hw/arm_gic.c      |   40 +++++++++++++++++++++-------------------
 hw/armv7m_nvic.c  |    2 +-
 hw/realview_gic.c |    2 +-
 5 files changed, 31 insertions(+), 24 deletions(-)

Comments

Peter Maydell Dec. 27, 2011, 9:59 p.m. UTC | #1 
On 27 December 2011 20:13, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
> Increase the maximum number of GIC interrupts for a9mp to 192, and
> create a configurable property defaulting to 96 so that device
> modelers can set the value appropriately for their SoC.

>  /* Configuration for arm_gic.c:
>  * number of external IRQ lines, max number of CPUs, how to ID current CPU
>  */
> -#define GIC_NIRQ 96
> +#define GIC_NIRQ 192

This (still) isn't the maximum number of GIC interrupts for
an A9MP. You want 256.

Moreover:
now we have a per-cpu #define which is setting the compile-time
maximum on a run-time parameter. That's pretty pointless --
we should just have arm_gic.c have its own (purely internal)
#define of the maximum number of interrupts it can permit,
and the cpu-specific private peripheral code should only be
setting the run-time parameter. You can drop the #define completely
from a9mpcore.c &co. (don't forget to update the comment)

The GIC architectural limit on number of interrupts is 1020;
that would (I think) imply a ~64K memory usage by the GIC,
which we can live with I think.

>  #define NCPU 4
>
>  static inline int
> @@ -37,6 +37,7 @@ typedef struct a9mp_priv_state {
>     MemoryRegion ptimer_iomem;
>     MemoryRegion container;
>     DeviceState *mptimer;
> +    uint32_t num_irq;
>  } a9mp_priv_state;
>
>  static uint64_t a9_scu_read(void *opaque, target_phys_addr_t offset,
> @@ -152,8 +153,11 @@ static int a9mp_priv_init(SysBusDevice *dev)
>     if (s->num_cpu > NCPU) {
>         hw_error("a9mp_priv_init: num-cpu may not be more than %d\n", NCPU);
>     }
> +    if (s->num_irq > GIC_NIRQ) {
> +        hw_error("a9mp_priv_init: num-irq may not be more than %d\n", GIC_NIRQ);
> +    }

No need to check this here -- just pass it through
to gic_init() and let gic_init() hw_error if needed.

> --- a/hw/arm11mpcore.c
> +++ b/hw/arm11mpcore.c
> @@ -132,7 +132,7 @@ static int mpcore_priv_init(SysBusDevice *dev)
>  {
>     mpcore_priv_state *s = FROM_SYSBUSGIC(mpcore_priv_state, dev);
>
> -    gic_init(&s->gic, s->num_cpu);
> +    gic_init(&s->gic, s->num_cpu, GIC_NIRQ);

11MPCore, like A9MP, has a configurable number of interrupt lines
(up to 256 including the 32 internal ones, also like A9MP).

> -static void gic_init(gic_state *s)
> +static void gic_init(gic_state *s, int num_irq)
>  #endif
>  {
>     int i;
> @@ -808,7 +809,8 @@ static void gic_init(gic_state *s)
>  #if NCPU > 1
>     s->num_cpu = num_cpu;
>  #endif
> -    qdev_init_gpio_in(&s->busdev.qdev, gic_set_irq, GIC_NIRQ - 32);
> +    s->num_irq = num_irq;

This is where you should be hw_error()ing if num_irq is
too big.

I didn't see any changes to gic_load/gic_save, which means they
will still be saving all GIC_NIRQ entries in each of the per-irq
state arrays; this means that you've broken save/load compatibility.
I think this can be fixed by having save/load only save/load the
entries which are actually used (ie loop up to s->num_irq rather
than GIC_NIRQ).

-- PMM
Mark Langsdorf Dec. 27, 2011, 10:04 p.m. UTC | #2 
On 12/27/2011 03:59 PM, Peter Maydell wrote:
> On 27 December 2011 20:13, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
>> Increase the maximum number of GIC interrupts for a9mp to 192, and
>> create a configurable property defaulting to 96 so that device
>> modelers can set the value appropriately for their SoC.
> 
>>  /* Configuration for arm_gic.c:
>>  * number of external IRQ lines, max number of CPUs, how to ID current CPU
>>  */
>> -#define GIC_NIRQ 96
>> +#define GIC_NIRQ 192
> 
> This (still) isn't the maximum number of GIC interrupts for
> an A9MP. You want 256.

I know, but I figured "more than anyone would be using" would
be sufficient.

> Moreover:
> now we have a per-cpu #define which is setting the compile-time
> maximum on a run-time parameter. That's pretty pointless --
> we should just have arm_gic.c have its own (purely internal)
> #define of the maximum number of interrupts it can permit,
> and the cpu-specific private peripheral code should only be
> setting the run-time parameter. You can drop the #define completely
> from a9mpcore.c &co. (don't forget to update the comment)

Okay.

> The GIC architectural limit on number of interrupts is 1020;
> that would (I think) imply a ~64K memory usage by the GIC,
> which we can live with I think.

I think you just said you wanted me to define the gic internal
maximum as 1020. I just want to be sure I understood you there.

>> --- a/hw/arm11mpcore.c
>> +++ b/hw/arm11mpcore.c
>> @@ -132,7 +132,7 @@ static int mpcore_priv_init(SysBusDevice *dev)
>>  {
>>     mpcore_priv_state *s = FROM_SYSBUSGIC(mpcore_priv_state, dev);
>>
>> -    gic_init(&s->gic, s->num_cpu);
>> +    gic_init(&s->gic, s->num_cpu, GIC_NIRQ);
> 
> 11MPCore, like A9MP, has a configurable number of interrupt lines
> (up to 256 including the 32 internal ones, also like A9MP).

I don't have 11mpcore to test with, but I'll make the change.

>> -static void gic_init(gic_state *s)
>> +static void gic_init(gic_state *s, int num_irq)
>>  #endif
>>  {
>>     int i;
>> @@ -808,7 +809,8 @@ static void gic_init(gic_state *s)
>>  #if NCPU > 1
>>     s->num_cpu = num_cpu;
>>  #endif
>> -    qdev_init_gpio_in(&s->busdev.qdev, gic_set_irq, GIC_NIRQ - 32);
>> +    s->num_irq = num_irq;
> 
> This is where you should be hw_error()ing if num_irq is
> too big.
> 
> I didn't see any changes to gic_load/gic_save, which means they
> will still be saving all GIC_NIRQ entries in each of the per-irq
> state arrays; this means that you've broken save/load compatibility.
> I think this can be fixed by having save/load only save/load the
> entries which are actually used (ie loop up to s->num_irq rather
> than GIC_NIRQ).

I had considered that, but wasn't sure enough about the pros and
cons. I'll make the changes and thanks for the feedback.

--Mark Langsdorf
Calxeda, Inc.
Peter Maydell Dec. 27, 2011, 10:09 p.m. UTC | #3 
On 27 December 2011 22:04, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
> On 12/27/2011 03:59 PM, Peter Maydell wrote:
>> The GIC architectural limit on number of interrupts is 1020;
>> that would (I think) imply a ~64K memory usage by the GIC,
>> which we can live with I think.
>
> I think you just said you wanted me to define the gic internal
> maximum as 1020. I just want to be sure I understood you there.

That's "we should define it as 1020 unless you or somebody else
thinks that's a terrible idea for some reason". Sorry, that was
a bit cryptic.

-- PMM
diff mbox

Patch

diff --git a/hw/a9mpcore.c b/hw/a9mpcore.c
index 3ef0e13..89504c4 100644
--- a/hw/a9mpcore.c
+++ b/hw/a9mpcore.c
@@ -13,7 +13,7 @@ 
 /* Configuration for arm_gic.c:
  * number of external IRQ lines, max number of CPUs, how to ID current CPU
  */
-#define GIC_NIRQ 96
+#define GIC_NIRQ 192
 #define NCPU 4
 
 static inline int
@@ -37,6 +37,7 @@  typedef struct a9mp_priv_state {
     MemoryRegion ptimer_iomem;
     MemoryRegion container;
     DeviceState *mptimer;
+    uint32_t num_irq;
 } a9mp_priv_state;
 
 static uint64_t a9_scu_read(void *opaque, target_phys_addr_t offset,
@@ -152,8 +153,11 @@  static int a9mp_priv_init(SysBusDevice *dev)
     if (s->num_cpu > NCPU) {
         hw_error("a9mp_priv_init: num-cpu may not be more than %d\n", NCPU);
     }
+    if (s->num_irq > GIC_NIRQ) {
+        hw_error("a9mp_priv_init: num-irq may not be more than %d\n", GIC_NIRQ);
+    }
 
-    gic_init(&s->gic, s->num_cpu);
+    gic_init(&s->gic, s->num_cpu, s->num_irq);
 
     s->mptimer = qdev_create(NULL, "arm_mptimer");
     qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
@@ -216,6 +220,7 @@  static SysBusDeviceInfo a9mp_priv_info = {
     .qdev.reset = a9mp_priv_reset,
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT32("num-cpu", a9mp_priv_state, num_cpu, 1),
+        DEFINE_PROP_UINT32("num-irq", a9mp_priv_state, num_irq, 96),
         DEFINE_PROP_END_OF_LIST(),
     }
 };
diff --git a/hw/arm11mpcore.c b/hw/arm11mpcore.c
index bc0457e..039b401 100644
--- a/hw/arm11mpcore.c
+++ b/hw/arm11mpcore.c
@@ -132,7 +132,7 @@  static int mpcore_priv_init(SysBusDevice *dev)
 {
     mpcore_priv_state *s = FROM_SYSBUSGIC(mpcore_priv_state, dev);
 
-    gic_init(&s->gic, s->num_cpu);
+    gic_init(&s->gic, s->num_cpu, GIC_NIRQ);
     s->mptimer = qdev_create(NULL, "arm_mptimer");
     qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
     qdev_init_nofail(s->mptimer);
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index 0339cf5..cc21262 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -111,6 +111,7 @@  typedef struct gic_state
     struct gic_state *backref[NCPU];
     MemoryRegion cpuiomem[NCPU+1]; /* CPU interfaces */
 #endif
+    int num_irq;
 } gic_state;
 
 /* TODO: Many places that call this routine could be optimized.  */
@@ -133,7 +134,7 @@  static void gic_update(gic_state *s)
         }
         best_prio = 0x100;
         best_irq = 1023;
-        for (irq = 0; irq < GIC_NIRQ; irq++) {
+        for (irq = 0; irq < s->num_irq; irq++) {
             if (GIC_TEST_ENABLED(irq, cm) && GIC_TEST_PENDING(irq, cm)) {
                 if (GIC_GET_PRIORITY(irq, cpu) < best_prio) {
                     best_prio = GIC_GET_PRIORITY(irq, cpu);
@@ -222,7 +223,7 @@  static void gic_complete_irq(gic_state * s, int cpu, int irq)
     int update = 0;
     int cm = 1 << cpu;
     DPRINTF("EOI %d\n", irq);
-    if (irq >= GIC_NIRQ) {
+    if (irq >= s->num_irq) {
         /* This handles two cases:
          * 1. If software writes the ID of a spurious interrupt [ie 1023]
          * to the GICC_EOIR, the GIC ignores that write.
@@ -279,7 +280,7 @@  static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
         if (offset == 0)
             return s->enabled;
         if (offset == 4)
-            return ((GIC_NIRQ / 32) - 1) | ((NUM_CPU(s) - 1) << 5);
+            return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5);
         if (offset < 0x08)
             return 0;
         if (offset >= 0x80) {
@@ -295,7 +296,7 @@  static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
         else
             irq = (offset - 0x180) * 8;
         irq += GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         res = 0;
         for (i = 0; i < 8; i++) {
@@ -310,7 +311,7 @@  static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
         else
             irq = (offset - 0x280) * 8;
         irq += GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         res = 0;
         mask = (irq < 32) ?  cm : ALL_CPU_MASK;
@@ -322,7 +323,7 @@  static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
     } else if (offset < 0x400) {
         /* Interrupt Active.  */
         irq = (offset - 0x300) * 8 + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         res = 0;
         mask = (irq < 32) ?  cm : ALL_CPU_MASK;
@@ -334,14 +335,14 @@  static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
     } else if (offset < 0x800) {
         /* Interrupt Priority.  */
         irq = (offset - 0x400) + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         res = GIC_GET_PRIORITY(irq, cpu);
 #ifndef NVIC
     } else if (offset < 0xc00) {
         /* Interrupt CPU Target.  */
         irq = (offset - 0x800) + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         if (irq >= 29 && irq <= 31) {
             res = cm;
@@ -351,7 +352,7 @@  static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
     } else if (offset < 0xf00) {
         /* Interrupt Configuration.  */
         irq = (offset - 0xc00) * 2 + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         res = 0;
         for (i = 0; i < 4; i++) {
@@ -426,7 +427,7 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
     } else if (offset < 0x180) {
         /* Interrupt Set Enable.  */
         irq = (offset - 0x100) * 8 + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         if (irq < 16)
           value = 0xff;
@@ -451,7 +452,7 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
     } else if (offset < 0x200) {
         /* Interrupt Clear Enable.  */
         irq = (offset - 0x180) * 8 + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         if (irq < 16)
           value = 0;
@@ -468,7 +469,7 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
     } else if (offset < 0x280) {
         /* Interrupt Set Pending.  */
         irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         if (irq < 16)
           irq = 0;
@@ -481,7 +482,7 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
     } else if (offset < 0x300) {
         /* Interrupt Clear Pending.  */
         irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         for (i = 0; i < 8; i++) {
             /* ??? This currently clears the pending bit for all CPUs, even
@@ -497,7 +498,7 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
     } else if (offset < 0x800) {
         /* Interrupt Priority.  */
         irq = (offset - 0x400) + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         if (irq < 32) {
             s->priority1[irq][cpu] = value;
@@ -508,7 +509,7 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
     } else if (offset < 0xc00) {
         /* Interrupt CPU Target.  */
         irq = (offset - 0x800) + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         if (irq < 29)
             value = 0;
@@ -518,7 +519,7 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
     } else if (offset < 0xf00) {
         /* Interrupt Configuration.  */
         irq = (offset - 0xc00) * 4 + GIC_BASE_IRQ;
-        if (irq >= GIC_NIRQ)
+        if (irq >= s->num_irq)
             goto bad_reg;
         if (irq < 32)
             value |= 0xaa;
@@ -798,9 +799,9 @@  static int gic_load(QEMUFile *f, void *opaque, int version_id)
 }
 
 #if NCPU > 1
-static void gic_init(gic_state *s, int num_cpu)
+static void gic_init(gic_state *s, int num_cpu, int num_irq)
 #else
-static void gic_init(gic_state *s)
+static void gic_init(gic_state *s, int num_irq)
 #endif
 {
     int i;
@@ -808,7 +809,8 @@  static void gic_init(gic_state *s)
 #if NCPU > 1
     s->num_cpu = num_cpu;
 #endif
-    qdev_init_gpio_in(&s->busdev.qdev, gic_set_irq, GIC_NIRQ - 32);
+    s->num_irq = num_irq;
+    qdev_init_gpio_in(&s->busdev.qdev, gic_set_irq, s->num_irq - 32);
     for (i = 0; i < NUM_CPU(s); i++) {
         sysbus_init_irq(&s->busdev, &s->parent_irq[i]);
     }
diff --git a/hw/armv7m_nvic.c b/hw/armv7m_nvic.c
index bf8c3c5..b97c904 100644
--- a/hw/armv7m_nvic.c
+++ b/hw/armv7m_nvic.c
@@ -384,7 +384,7 @@  static int armv7m_nvic_init(SysBusDevice *dev)
 {
     nvic_state *s= FROM_SYSBUSGIC(nvic_state, dev);
 
-    gic_init(&s->gic);
+    gic_init(&s->gic, GIC_NIRQ);
     memory_region_add_subregion(get_system_memory(), 0xe000e000, &s->gic.iomem);
     s->systick.timer = qemu_new_timer_ns(vm_clock, systick_timer_tick, s);
     vmstate_register(&dev->qdev, -1, &vmstate_nvic, s);
diff --git a/hw/realview_gic.c b/hw/realview_gic.c
index 8c4d509..a967f36 100644
--- a/hw/realview_gic.c
+++ b/hw/realview_gic.c
@@ -37,7 +37,7 @@  static int realview_gic_init(SysBusDevice *dev)
 {
     RealViewGICState *s = FROM_SYSBUSGIC(RealViewGICState, dev);
 
-    gic_init(&s->gic);
+    gic_init(&s->gic, GIC_NIRQ);
     realview_gic_map_setup(s);
     sysbus_init_mmio(dev, &s->container);
     return 0;