Patchwork [v2,2/2] hw/exynos4210_gic.c: Introduce n_in and n_out propeties.

login
register
mail settings
Submitter Evgeny Voevodin
Date April 12, 2012, 8:13 a.m.
Message ID <1334218381-4208-3-git-send-email-e.voevodin@samsung.com>
Download mbox | patch
Permalink /patch/151998/
State New
Headers show

Comments

Evgeny Voevodin - April 12, 2012, 8:13 a.m.
With these properties irq gate could be tuned to mux up to
QDEV_MAX_IRQ inputs and ouputs. Gate will group inputs
into groups of size n_in/n_out each.

Signed-off-by: Evgeny Voevodin <e.voevodin@samsung.com>
---
 hw/exynos4210.c     |    2 +
 hw/exynos4210_gic.c |   69 +++++++++++++++++++++++++++++++-------------------
 2 files changed, 45 insertions(+), 26 deletions(-)
Peter Maydell - April 16, 2012, 11:01 a.m.
On 12 April 2012 09:13, Evgeny Voevodin <e.voevodin@samsung.com> wrote:
> With these properties irq gate could be tuned to mux up to
> QDEV_MAX_IRQ inputs and ouputs. Gate will group inputs
> into groups of size n_in/n_out each.
> @@ -420,14 +430,20 @@ static int exynos4210_irq_gate_init(SysBusDevice *dev)
>     Exynos4210IRQGateState *s =
>             FROM_SYSBUS(Exynos4210IRQGateState, dev);
>
> +    /* Gate will make each input group of size n_in / n_out */

I think it would be useful to expand this comment to describe how the
device uses its gpio_in and sysbus irq outputs. (This is necessary at
the moment because we don't have the ability to have nicely named
arrays of gpio inputs/outputs, so often the single gpio input array
is arbitrarily divided up and you just have to document in a comment
how it works; see for instance arm_gic.c and arm_mptimer.c. Something
like:

/* This device models a collection of OR gates. There are n_out
 * separate gates, and output sysbus IRQ line N is the output of
 * gate N. The input qdev gpio lines are the inputs to each gate
 * in order:
 *  [0.. n_in/n_out - 1] : inputs to gate 0
 *  [n_in/n_out .. 2*n_in/n_out - 1] : inputs to gate 1
 * and so on.
 */

The other approach to consider would be to make each OR gate a separate
device, and just instantiate N of them. That would probably look
cleaner in the device implementation but be a little more code in
exynos4210.c. I don't have a preference either way -- just a thought.

> +    if ((s->n_in % s->n_out) != 0) {
> +        hw_error("n_in is not multiple of n_out in Irq Gate");

"exynos4210.irq_gate: n_in must be a multiple of n_out".

-- PMM
Evgeny Voevodin - April 16, 2012, 11:14 a.m.
On 16.04.2012 15:01, Peter Maydell wrote:
> On 12 April 2012 09:13, Evgeny Voevodin<e.voevodin@samsung.com>  wrote:
>> With these properties irq gate could be tuned to mux up to
>> QDEV_MAX_IRQ inputs and ouputs. Gate will group inputs
>> into groups of size n_in/n_out each.
>> @@ -420,14 +430,20 @@ static int exynos4210_irq_gate_init(SysBusDevice *dev)
>>      Exynos4210IRQGateState *s =
>>              FROM_SYSBUS(Exynos4210IRQGateState, dev);
>>
>> +    /* Gate will make each input group of size n_in / n_out */
> I think it would be useful to expand this comment to describe how the
> device uses its gpio_in and sysbus irq outputs. (This is necessary at
> the moment because we don't have the ability to have nicely named
> arrays of gpio inputs/outputs, so often the single gpio input array
> is arbitrarily divided up and you just have to document in a comment
> how it works; see for instance arm_gic.c and arm_mptimer.c. Something
> like:
>
> /* This device models a collection of OR gates. There are n_out
>   * separate gates, and output sysbus IRQ line N is the output of
>   * gate N. The input qdev gpio lines are the inputs to each gate
>   * in order:
>   *  [0.. n_in/n_out - 1] : inputs to gate 0
>   *  [n_in/n_out .. 2*n_in/n_out - 1] : inputs to gate 1
>   * and so on.
>   */

Ok.

> The other approach to consider would be to make each OR gate a separate
> device, and just instantiate N of them. That would probably look
> cleaner in the device implementation but be a little more code in
> exynos4210.c. I don't have a preference either way -- just a thought.

Do you mean to have each OR gate with only two inputs? Then if we want 
to mux n inputs into
one output, we'll have to place a number of gates after a gate output... 
As for me it's better
to have one configurable gate with multiple inputs.

>> +    if ((s->n_in % s->n_out) != 0) {
>> +        hw_error("n_in is not multiple of n_out in Irq Gate");
> "exynos4210.irq_gate: n_in must be a multiple of n_out".

Ok.

> -- PMM
>

Thanks!
Peter Maydell - April 16, 2012, 11:17 a.m.
On 16 April 2012 12:14, Evgeny Voevodin <e.voevodin@samsung.com> wrote:
> On 16.04.2012 15:01, Peter Maydell wrote:
>> The other approach to consider would be to make each OR gate a separate
>> device, and just instantiate N of them. That would probably look
>> cleaner in the device implementation but be a little more code in
>> exynos4210.c. I don't have a preference either way -- just a thought.
>
> Do you mean to have each OR gate with only two inputs?

No, I meant that you could have a qdev device which is a single OR
gate with N inputs and one output. (As opposed to now, where
you have a qdev device which is X OR gates all lumped together,
with N * X inputs and X outputs, but each OR gate is actually
not interacting with any of the others.)

-- PMM
Evgeny Voevodin - April 16, 2012, 11:29 a.m.
On 16.04.2012 15:17, Peter Maydell wrote:
> On 16 April 2012 12:14, Evgeny Voevodin<e.voevodin@samsung.com>  wrote:
>> On 16.04.2012 15:01, Peter Maydell wrote:
>>> The other approach to consider would be to make each OR gate a separate
>>> device, and just instantiate N of them. That would probably look
>>> cleaner in the device implementation but be a little more code in
>>> exynos4210.c. I don't have a preference either way -- just a thought.
>> Do you mean to have each OR gate with only two inputs?
> No, I meant that you could have a qdev device which is a single OR
> gate with N inputs and one output. (As opposed to now, where
> you have a qdev device which is X OR gates all lumped together,
> with N * X inputs and X outputs, but each OR gate is actually
> not interacting with any of the others.)
>
> -- PMM
>

Hmmm. Having one output per gate we can configure resulting multiplexer 
easier
then now in the case of different inputs per gate. Seems you're right. 
Again :)

Patch

diff --git a/hw/exynos4210.c b/hw/exynos4210.c
index 5e30387..2cb7890 100644
--- a/hw/exynos4210.c
+++ b/hw/exynos4210.c
@@ -98,6 +98,8 @@  Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
 
     /* IRQ Gate */
     dev = qdev_create(NULL, "exynos4210.irq_gate");
+    qdev_prop_set_uint32(dev, "n_out", EXYNOS4210_NCPUS);
+    qdev_prop_set_uint32(dev, "n_in", EXYNOS4210_IRQ_GATE_NINPUTS);
     qdev_init_nofail(dev);
     /* Get IRQ Gate input in gate_irq */
     for (n = 0; n < EXYNOS4210_IRQ_GATE_NINPUTS; n++) {
diff --git a/hw/exynos4210_gic.c b/hw/exynos4210_gic.c
index ec13140..be95fe4 100644
--- a/hw/exynos4210_gic.c
+++ b/hw/exynos4210_gic.c
@@ -361,18 +361,29 @@  type_init(exynos4210_gic_register_types)
 typedef struct {
     SysBusDevice busdev;
 
-    qemu_irq pic_irq[NCPU]; /* output IRQs to PICs */
-    uint32_t gpio_level[EXYNOS4210_IRQ_GATE_NINPUTS]; /* Input levels */
+    qemu_irq out[QDEV_MAX_IRQ]; /* output IRQs */
+    uint32_t n_out;             /* outputs amount */
+    uint32_t gpio_level[QDEV_MAX_IRQ]; /* Input levels */
+    uint32_t n_in;              /* inputs amount */
+    uint32_t group_size;        /* input group size */
 } Exynos4210IRQGateState;
 
+static Property exynos4210_irq_gate_properties[] = {
+    DEFINE_PROP_UINT32("n_out", Exynos4210IRQGateState, n_out, 1),
+    DEFINE_PROP_UINT32("n_in", Exynos4210IRQGateState, n_in, 1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static const VMStateDescription vmstate_exynos4210_irq_gate = {
     .name = "exynos4210.irq_gate",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .minimum_version_id_old = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32_ARRAY(gpio_level, Exynos4210IRQGateState,
-                EXYNOS4210_IRQ_GATE_NINPUTS),
+        VMSTATE_UINT32(n_out, Exynos4210IRQGateState),
+        VMSTATE_UINT32_ARRAY(gpio_level, Exynos4210IRQGateState, QDEV_MAX_IRQ),
+        VMSTATE_UINT32(n_in, Exynos4210IRQGateState),
+        VMSTATE_UINT32(group_size, Exynos4210IRQGateState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -382,25 +393,24 @@  static void exynos4210_irq_gate_handler(void *opaque, int irq, int level)
 {
     Exynos4210IRQGateState *s =
             (Exynos4210IRQGateState *)opaque;
-    uint32_t odd, even;
-
-    if (irq & 1) {
-        odd = irq;
-        even = irq & ~1;
-    } else {
-        even = irq;
-        odd = irq | 1;
-    }
+    uint32_t n_out, n_group, i;
+
+    assert(irq < s->n_in);
+
+    n_out = irq / s->group_size;
+    n_group = n_out * s->group_size;
 
-    assert(irq < EXYNOS4210_IRQ_GATE_NINPUTS);
     s->gpio_level[irq] = level;
 
-    if (s->gpio_level[odd] >= 1 || s->gpio_level[even] >= 1) {
-        qemu_irq_raise(s->pic_irq[even >> 1]);
-    } else {
-        qemu_irq_lower(s->pic_irq[even >> 1]);
+    for (i = 0; i < s->group_size; i++) {
+        if (s->gpio_level[n_group + i] >= 1) {
+            qemu_irq_raise(s->out[n_out]);
+            return;
+        }
     }
 
+    qemu_irq_lower(s->out[n_out]);
+
     return;
 }
 
@@ -420,14 +430,20 @@  static int exynos4210_irq_gate_init(SysBusDevice *dev)
     Exynos4210IRQGateState *s =
             FROM_SYSBUS(Exynos4210IRQGateState, dev);
 
+    /* Gate will make each input group of size n_in / n_out */
+    if ((s->n_in % s->n_out) != 0) {
+        hw_error("n_in is not multiple of n_out in Irq Gate");
+    }
+
+    s->group_size = s->n_in / s->n_out;
+
     /* Allocate general purpose input signals and connect a handler to each of
      * them */
-    qdev_init_gpio_in(&s->busdev.qdev, exynos4210_irq_gate_handler,
-            EXYNOS4210_IRQ_GATE_NINPUTS);
+    qdev_init_gpio_in(&s->busdev.qdev, exynos4210_irq_gate_handler, s->n_in);
 
-    /* Connect SysBusDev irqs to device specific irqs */
-    for (i = 0; i < NCPU; i++) {
-        sysbus_init_irq(dev, &s->pic_irq[i]);
+    /* Pass gate outs to SysBusDev */
+    for (i = 0; i < s->n_out; i++) {
+        sysbus_init_irq(dev, &s->out[i]);
     }
 
     return 0;
@@ -441,6 +457,7 @@  static void exynos4210_irq_gate_class_init(ObjectClass *klass, void *data)
     k->init = exynos4210_irq_gate_init;
     dc->reset = exynos4210_irq_gate_reset;
     dc->vmsd = &vmstate_exynos4210_irq_gate;
+    dc->props = exynos4210_irq_gate_properties;
 }
 
 static TypeInfo exynos4210_irq_gate_info = {