Named GPIOs

PATCH Superseded Peter Crosthwaite 2 participants 6 messages May 8, 2014 – May 12, 2014 Arm/Zynq architecture

lore

Share & tools

lore

Peter Crosthwaite

author

2014-05-08 07:00 UTC #0 lore raw

v3 cover Named GPIOs

This series implements Named GPIOs on the qdev level. Patch 1 is the feature presentation.

Second patch I give a useful example of the features use (an SSI cleanup).

Changed sinve v2:
Re-ordered function args (PMM review)
Changed since v1:
use QLIST (AF review)
Dropped former P1 (too far out of scope)

Peter Crosthwaite (2):
qdev: Implement named GPIOs
ssi: Name the CS GPIO.

 hw/arm/stellaris.c                  |  7 ++--
 hw/arm/xilinx_zynq.c                |  2 +-
 hw/core/qdev.c                      | 70 +++++++++++++++++++++++++++++++------
 hw/microblaze/petalogix_ml605_mmu.c |  2 +-
 hw/ssi/ssi.c                        |  4 +--
 include/hw/qdev-core.h              | 24 ++++++++++---
 include/hw/ssi.h                    |  2 ++
 qdev-monitor.c                      | 14 +++++---
 qtest.c                             | 15 +++++---
 9 files changed, 109 insertions(+), 31 deletions(-)

Signature · 1 line

1.9.2.1.g06c4abd

Peter Crosthwaite

author

2014-05-08 07:01 UTC #1 lore raw

PATCH v3 1/2 qdev: Implement named GPIOs

Implement named GPIOs on the Device layer. Listifies the existing GPIOs stuff using string keys. Legacy un-named GPIOs are preserved by using a NULL name string - they are just a single matchable element in the name list.

Signed-off-by Peter Crosthwaite <[email protected]>

---
Changed since v2:
Reordered fn args to be name-before-number (PMM review) changed since v1:
Use QLIST instead of RYO list implementation (AF review) Reorder struct fields for consistency

 hw/core/qdev.c         | 70 ++++++++++++++++++++++++++++++++++++++++++--------
 include/hw/qdev-core.h | 24 ++++++++++++++---
 qdev-monitor.c         | 14 ++++++----
 qtest.c                | 15 +++++++----
 4 files changed, 99 insertions(+), 24 deletions(-)

hw/core/qdev.c +60 ~~-10~~

index 936eae6..8b54db2 100644
@@ -312,30 +312,79 @@ BusState *qdev_get_parent_bus(DeviceState *dev)
     return dev->parent_bus;
 }
 
+static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
+                                               const char *name)
+{
+    NamedGPIOList *ngl;
+
+    QLIST_FOREACH(ngl, &dev->gpios, node) {
+        if (ngl->name == name ||
+                (name && ngl->name && !strcmp(name, ngl->name))) {
+            return ngl;
+        }
+    }
+
+    ngl = g_malloc0(sizeof(*ngl));
+    ngl->name = g_strdup(name);
+    QLIST_INSERT_HEAD(&dev->gpios, ngl, node);
+    return ngl;
+}
+
+void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
+                             const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
+                                     dev, n);
+    gpio_list->num_in += n;
+}
+
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
 {
-    dev->gpio_in = qemu_extend_irqs(dev->gpio_in, dev->num_gpio_in, handler,
-                                        dev, n);
-    dev->num_gpio_in += n;
+    qdev_init_gpio_in_named(dev, handler, NULL, n);
+}
+
+void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
+                              const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(gpio_list->num_out == 0);
+    gpio_list->num_out = n;
+    gpio_list->out = pins;
 }
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
 {
-    assert(dev->num_gpio_out == 0);
-    dev->num_gpio_out = n;
-    dev->gpio_out = pins;
+    qdev_init_gpio_out_named(dev, pins, NULL, n);
+}
+
+qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(n >= 0 && n < gpio_list->num_in);
+    return gpio_list->in[n];
 }
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n)
 {
-    assert(n >= 0 && n < dev->num_gpio_in);
-    return dev->gpio_in[n];
+    return qdev_get_gpio_in_named(dev, NULL, n);
+}
+
+void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
+                                 qemu_irq pin)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(n >= 0 && n < gpio_list->num_out);
+    gpio_list->out[n] = pin;
 }
 
 void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
 {
-    assert(n >= 0 && n < dev->num_gpio_out);
-    dev->gpio_out[n] = pin;
+    qdev_connect_gpio_out_named(dev, NULL, n, pin);
 }
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)

@@ -844,6 +893,7 @@ static void device_initfn(Object *obj)
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
                              (Object **)&dev->parent_bus, NULL, 0,
                              &error_abort);
+    QLIST_INIT(&dev->gpios);
 }
 
 static void device_post_init(Object *obj)

include/hw/qdev-core.h +20 -4

index dbe473c..04fca4e 100644
@@ -131,6 +131,17 @@ typedef struct DeviceClass {
     const char *bus_type;
 } DeviceClass;
 
+typedef struct NamedGPIOList NamedGPIOList;
+
+struct NamedGPIOList {
+    const char *name;
+    qemu_irq *in;
+    int num_in;
+    qemu_irq *out;
+    int num_out;
+    QLIST_ENTRY(NamedGPIOList) node;
+};
+
 /**
  * DeviceState:
  * @realized: Indicates whether the device has been fully constructed.

@@ -148,10 +159,7 @@ struct DeviceState {
     QemuOpts *opts;
     int hotplugged;
     BusState *parent_bus;
-    int num_gpio_out;
-    qemu_irq *gpio_out;
-    int num_gpio_in;
-    qemu_irq *gpio_in;
+    QLIST_HEAD(, NamedGPIOList) gpios;
     QLIST_HEAD(, BusState) child_bus;
     int num_child_bus;
     int instance_id_alias;

@@ -252,7 +260,11 @@ void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
+qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n);
+
 void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
+void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
+                                 qemu_irq pin);
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name);

@@ -262,6 +274,10 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
 /* GPIO inputs also double as IRQ sinks.  */
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n);
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n);
+void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
+                             const char *name, int n);
+void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
+                              const char *name, int n);
 
 BusState *qdev_get_parent_bus(DeviceState *dev);

qdev-monitor.c +9 -5

index 02cbe43..bb4c007 100644
@@ -613,14 +613,18 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
 {
     ObjectClass *class;
     BusState *child;
+    NamedGPIOList *ngl;
+
     qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
                 dev->id ? dev->id : "");
     indent += 2;
-    if (dev->num_gpio_in) {
-        qdev_printf("gpio-in %d\n", dev->num_gpio_in);
-    }
-    if (dev->num_gpio_out) {
-        qdev_printf("gpio-out %d\n", dev->num_gpio_out);
+    QLIST_FOREACH(ngl, &dev->gpios, node) {
+        if (ngl->num_in) {
+            qdev_printf("gpio-in \"%s\" %d\n", ngl->name, ngl->num_in);
+        }
+        if (ngl->num_out) {
+            qdev_printf("gpio-out \"%s\" %d\n", ngl->name, ngl->num_out);
+        }
     }
     class = object_get_class(OBJECT(dev));
     do {

qtest.c +10 -5

index 2aba20d..36c317a 100644
@@ -233,7 +233,8 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
     g_assert(command);
     if (strcmp(words[0], "irq_intercept_out") == 0
         || strcmp(words[0], "irq_intercept_in") == 0) {
-	DeviceState *dev;
+        DeviceState *dev;
+        NamedGPIOList *ngl;
 
         g_assert(words[1]);
         dev = DEVICE(object_resolve_path(words[1], NULL));

@@ -253,10 +254,14 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
 	    return;
         }
 
-        if (words[0][14] == 'o') {
-            qemu_irq_intercept_out(&dev->gpio_out, qtest_irq_handler, dev->num_gpio_out);
-        } else {
-            qemu_irq_intercept_in(dev->gpio_in, qtest_irq_handler, dev->num_gpio_in);
+        QLIST_FOREACH(ngl, &dev->gpios, node) {
+            if (words[0][14] == 'o') {
+                qemu_irq_intercept_out(&ngl->out, qtest_irq_handler,
+                                       ngl->num_out);
+            } else {
+                qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
+                                      ngl->num_in);
+            }
         }
         irq_intercept_dev = dev;
         qtest_send_prefix(chr);

Signature · 1 line

1.9.2.1.g06c4abd

Peter Maydell

2014-05-09 16:52 UTC #2 lore raw

Re: [Qemu-devel] [PATCH v3 1/2] qdev: Implement named GPIOs

Quoted 13 lines · Peter Crosthwaite

Implement named GPIOs on the Device layer. Listifies the existing GPIOs stuff using string keys. Legacy un-named GPIOs are preserved by using a NULL name string - they are just a single matchable element in the name list.
Signed-off-by Peter Crosthwaite <[email protected]>
---
Changed since v2:
Reordered fn args to be name-before-number (PMM review) changed since v1:
Use QLIST instead of RYO list implementation (AF review) Reorder struct fields for consistency

Quoted 5 lines

 hw/core/qdev.c         | 70 ++++++++++++++++++++++++++++++++++++++++++--------
 include/hw/qdev-core.h | 24 ++++++++++++++---
 qdev-monitor.c         | 14 ++++++----
 qtest.c                | 15 +++++++----
 4 files changed, 99 insertions(+), 24 deletions(-)

So, first of all: having thought a bit about it I think we should go ahead with the approach this patch uses. Even if we later come up with some fancy QOM-object based way of doing GPIO lines, it should be easy enough to convert or put in back-compatible functions, and we'll be ahead if we've already done the effort of splitting single GPIO arrays into more sensible multiple named arrays.

Some review follows.

Quoted 6 lines

hw/core/qdev.c +0 -0

index 936eae6..8b54db2 100644
@@ -312,30 +312,79 @@ BusState *qdev_get_parent_bus(DeviceState *dev)
     return dev->parent_bus;

hw/core/qdev.c BusState *qdev_get_parent_bus(DeviceState *dev) · line 322

Quoted 10 lines

hw/core/qdev.c +8 -0

 }
 
+static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
+                                               const char *name)
+{
+    NamedGPIOList *ngl;
+
+    QLIST_FOREACH(ngl, &dev->gpios, node) {
+        if (ngl->name == name ||
+                (name && ngl->name && !strcmp(name, ngl->name))) {

This slightly odd looking check is because NULL is a valid name, yes? That could probably use a comment.

hw/core/qdev.c BusState *qdev_get_parent_bus(DeviceState *dev) · line 328

Quoted 6 lines

hw/core/qdev.c +6 -0

+            return ngl;
+        }
+    }
+
+    ngl = g_malloc0(sizeof(*ngl));
+    ngl->name = g_strdup(name);

We're going to leak these when the device is deinited.

The current gpio handling code is not exactly an airtight leakfree design, but let's see if we can improve things while we're here.

I think device_finalize() needs to:
* iterate through the gpio list array
** call qemu_free_irqs(ngl->in)
** don't do anything with ngl->out because the caller of
qdev_init_gpio_out() owns that [usually embedded in their QOM object struct]
** free ngl->name
** free the ngl node itself

Quoted 152 lines

hw/core/qdev.c +46 ~~-10~~

+    QLIST_INSERT_HEAD(&dev->gpios, ngl, node);
+    return ngl;
+}
+
+void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
+                             const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
+                                     dev, n);
+    gpio_list->num_in += n;
+}
+
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
 {
-    dev->gpio_in = qemu_extend_irqs(dev->gpio_in, dev->num_gpio_in, handler,
-                                        dev, n);
-    dev->num_gpio_in += n;
+    qdev_init_gpio_in_named(dev, handler, NULL, n);
+}
+
+void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
+                              const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(gpio_list->num_out == 0);
+    gpio_list->num_out = n;
+    gpio_list->out = pins;
 }
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
 {
-    assert(dev->num_gpio_out == 0);
-    dev->num_gpio_out = n;
-    dev->gpio_out = pins;
+    qdev_init_gpio_out_named(dev, pins, NULL, n);
+}
+
+qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(n >= 0 && n < gpio_list->num_in);
+    return gpio_list->in[n];
 }
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n)
 {
-    assert(n >= 0 && n < dev->num_gpio_in);
-    return dev->gpio_in[n];
+    return qdev_get_gpio_in_named(dev, NULL, n);
+}
+
+void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
+                                 qemu_irq pin)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(n >= 0 && n < gpio_list->num_out);
+    gpio_list->out[n] = pin;
 }
 
 void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
 {
-    assert(n >= 0 && n < dev->num_gpio_out);
-    dev->gpio_out[n] = pin;
+    qdev_connect_gpio_out_named(dev, NULL, n, pin);
 }
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)

@@ -844,6 +893,7 @@ static void device_initfn(Object *obj)
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
                              (Object **)&dev->parent_bus, NULL, 0,
                              &error_abort);
+    QLIST_INIT(&dev->gpios);
 }
 
 static void device_post_init(Object *obj)

include/hw/qdev-core.h +20 -4

index dbe473c..04fca4e 100644
@@ -131,6 +131,17 @@ typedef struct DeviceClass {
     const char *bus_type;
 } DeviceClass;
 
+typedef struct NamedGPIOList NamedGPIOList;
+
+struct NamedGPIOList {
+    const char *name;
+    qemu_irq *in;
+    int num_in;
+    qemu_irq *out;
+    int num_out;
+    QLIST_ENTRY(NamedGPIOList) node;
+};
+
 /**
  * DeviceState:
  * @realized: Indicates whether the device has been fully constructed.

@@ -148,10 +159,7 @@ struct DeviceState {
     QemuOpts *opts;
     int hotplugged;
     BusState *parent_bus;
-    int num_gpio_out;
-    qemu_irq *gpio_out;
-    int num_gpio_in;
-    qemu_irq *gpio_in;
+    QLIST_HEAD(, NamedGPIOList) gpios;
     QLIST_HEAD(, BusState) child_bus;
     int num_child_bus;
     int instance_id_alias;

@@ -252,7 +260,11 @@ void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
+qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n);
+
 void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
+void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
+                                 qemu_irq pin);
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name);

@@ -262,6 +274,10 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
 /* GPIO inputs also double as IRQ sinks.  */
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n);
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n);
+void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
+                             const char *name, int n);
+void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
+                              const char *name, int n);
 
 BusState *qdev_get_parent_bus(DeviceState *dev);

qdev-monitor.c +2 -2

index 02cbe43..bb4c007 100644
@@ -613,14 +613,18 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
 {
     ObjectClass *class;
     BusState *child;
+    NamedGPIOList *ngl;
+
     qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
                 dev->id ? dev->id : "");
     indent += 2;
-    if (dev->num_gpio_in) {
-        qdev_printf("gpio-in %d\n", dev->num_gpio_in);

qdev-monitor.c static void qdev_print(Monitor *mon, DeviceState *dev, int indent) · line 627

Quoted 10 lines

qdev-monitor.c +7 -3

-    }
-    if (dev->num_gpio_out) {
-        qdev_printf("gpio-out %d\n", dev->num_gpio_out);
+    QLIST_FOREACH(ngl, &dev->gpios, node) {
+        if (ngl->num_in) {
+            qdev_printf("gpio-in \"%s\" %d\n", ngl->name, ngl->num_in);
+        }
+        if (ngl->num_out) {
+            qdev_printf("gpio-out \"%s\" %d\n", ngl->name, ngl->num_out);
+        }

This is going to hand printf a NULL pointer for the name if it's printing the default gpio arrays, isn't it?

Quoted 23 lines

qdev-monitor.c +0 -0

     }
     class = object_get_class(OBJECT(dev));
     do {

qtest.c +2 -3

index 2aba20d..36c317a 100644
@@ -233,7 +233,8 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
     g_assert(command);
     if (strcmp(words[0], "irq_intercept_out") == 0
         || strcmp(words[0], "irq_intercept_in") == 0) {
-       DeviceState *dev;
+        DeviceState *dev;
+        NamedGPIOList *ngl;
 
         g_assert(words[1]);
         dev = DEVICE(object_resolve_path(words[1], NULL));

@@ -253,10 +254,14 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
            return;
         }
 
-        if (words[0][14] == 'o') {
-            qemu_irq_intercept_out(&dev->gpio_out, qtest_irq_handler, dev->num_gpio_out);

qtest.c static void qtest_process_command(CharDriverState *chr, gchar **words) · line 264

Quoted 10 lines

qtest.c +8 -2

-        } else {
-            qemu_irq_intercept_in(dev->gpio_in, qtest_irq_handler, dev->num_gpio_in);
+        QLIST_FOREACH(ngl, &dev->gpios, node) {
+            if (words[0][14] == 'o') {
+                qemu_irq_intercept_out(&ngl->out, qtest_irq_handler,
+                                       ngl->num_out);
+            } else {
+                qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
+                                      ngl->num_in);
+            }

This means that if the qtest intercepts GPIOs then it won't be able to tell which GPIO array the IRQ raise/lower messages it gets correspond to (if you look at qtest_irq_handler the message that is emitted is just "IRQ raise %d" or "IRQ lower %d").

I would suggest that you make this patch just retain the current behaviour of intercepting only the default gpio in/out arrays (ie the name==NULL ones). Then you can have a separate followup patch which fixes the qtest protocol messages (both the 'irq_intercept_{in,out}' ones from the test and the IRQ raise/lower replies) to include the gpio array name.

Quoted 6 lines

qtest.c +0 -1

         }
         irq_intercept_dev = dev;
         qtest_send_prefix(chr);
--

1.9.2.1.g06c4abd

thanks
-- PMM

Peter Crosthwaite

author

2014-05-12 01:09 UTC #3 lore raw

Re: [Qemu-devel] [PATCH v3 1/2] qdev: Implement named GPIOs

Quoted 38 lines · Peter Maydell

Quoted 13 lines · Peter Crosthwaite
Implement named GPIOs on the Device layer. Listifies the existing GPIOs stuff using string keys. Legacy un-named GPIOs are preserved by using a NULL name string - they are just a single matchable element in the name list.
Signed-off-by Peter Crosthwaite <[email protected]>
---
Changed since v2:
Reordered fn args to be name-before-number (PMM review) changed since v1:
Use QLIST instead of RYO list implementation (AF review) Reorder struct fields for consistency
Quoted 5 lines
 hw/core/qdev.c         | 70 ++++++++++++++++++++++++++++++++++++++++++--------
 include/hw/qdev-core.h | 24 ++++++++++++++---
 qdev-monitor.c         | 14 ++++++----
 qtest.c                | 15 +++++++----
 4 files changed, 99 insertions(+), 24 deletions(-)
So, first of all: having thought a bit about it I think we should go ahead with the approach this patch uses. Even if we later come up with some fancy QOM-object based way of doing GPIO lines, it should be easy enough to convert or put in back-compatible functions, and we'll be ahead if we've already done the effort of splitting single GPIO arrays into more sensible multiple named arrays.
Some review follows.
Quoted 8 lines
hw/core/qdev.c +0 -0
index 936eae6..8b54db2 100644
@@ -312,30 +312,79 @@ BusState *qdev_get_parent_bus(DeviceState *dev)
     return dev->parent_bus;
 }
 

Quoted 12 lines

hw/core/qdev.c BusState *qdev_get_parent_bus(DeviceState *dev) · line 322
Quoted 8 lines
hw/core/qdev.c +8 -0
+static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
+                                               const char *name)
+{
+    NamedGPIOList *ngl;
+
+    QLIST_FOREACH(ngl, &dev->gpios, node) {
+        if (ngl->name == name ||
+                (name && ngl->name && !strcmp(name, ngl->name))) {
This slightly odd looking check is because NULL is a valid name, yes? That could probably use a comment.

Added a comment. Also change conditional to be more self documenting around NULL matching:

if (!(ngl->name && !name) ||

Quoted 13 lines

hw/core/qdev.c BusState *qdev_get_parent_bus(DeviceState *dev) · line 328
Quoted 6 lines
hw/core/qdev.c +6 -0
+            return ngl;
+        }
+    }
+
+    ngl = g_malloc0(sizeof(*ngl));
+    ngl->name = g_strdup(name);
We're going to leak these when the device is deinited.
The current gpio handling code is not exactly an airtight leakfree design, but let's see if we can improve things while we're here.

Quoted 9 lines

I think device_finalize() needs to:
* iterate through the gpio list array
** call qemu_free_irqs(ngl->in)
** don't do anything with ngl->out because the caller of
qdev_init_gpio_out() owns that [usually embedded in their QOM object struct]
** free ngl->name
** free the ngl node itself

Implemented.

Quoted 157 lines

hw/core/qdev.c +46 ~~-10~~

+    QLIST_INSERT_HEAD(&dev->gpios, ngl, node);
+    return ngl;
+}
+
+void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
+                             const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
+                                     dev, n);
+    gpio_list->num_in += n;
+}
+
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
 {
-    dev->gpio_in = qemu_extend_irqs(dev->gpio_in, dev->num_gpio_in, handler,
-                                        dev, n);
-    dev->num_gpio_in += n;
+    qdev_init_gpio_in_named(dev, handler, NULL, n);
+}
+
+void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
+                              const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(gpio_list->num_out == 0);
+    gpio_list->num_out = n;
+    gpio_list->out = pins;
 }
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
 {
-    assert(dev->num_gpio_out == 0);
-    dev->num_gpio_out = n;
-    dev->gpio_out = pins;
+    qdev_init_gpio_out_named(dev, pins, NULL, n);
+}
+
+qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(n >= 0 && n < gpio_list->num_in);
+    return gpio_list->in[n];
 }
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n)
 {
-    assert(n >= 0 && n < dev->num_gpio_in);
-    return dev->gpio_in[n];
+    return qdev_get_gpio_in_named(dev, NULL, n);
+}
+
+void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
+                                 qemu_irq pin)
+{
+    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
+
+    assert(n >= 0 && n < gpio_list->num_out);
+    gpio_list->out[n] = pin;
 }
 
 void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
 {
-    assert(n >= 0 && n < dev->num_gpio_out);
-    dev->gpio_out[n] = pin;
+    qdev_connect_gpio_out_named(dev, NULL, n, pin);
 }
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)

@@ -844,6 +893,7 @@ static void device_initfn(Object *obj)
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
                              (Object **)&dev->parent_bus, NULL, 0,
                              &error_abort);
+    QLIST_INIT(&dev->gpios);
 }
 
 static void device_post_init(Object *obj)

include/hw/qdev-core.h +20 -4

index dbe473c..04fca4e 100644
@@ -131,6 +131,17 @@ typedef struct DeviceClass {
     const char *bus_type;
 } DeviceClass;
 
+typedef struct NamedGPIOList NamedGPIOList;
+
+struct NamedGPIOList {
+    const char *name;
+    qemu_irq *in;
+    int num_in;
+    qemu_irq *out;
+    int num_out;
+    QLIST_ENTRY(NamedGPIOList) node;
+};
+
 /**
  * DeviceState:
  * @realized: Indicates whether the device has been fully constructed.

@@ -148,10 +159,7 @@ struct DeviceState {
     QemuOpts *opts;
     int hotplugged;
     BusState *parent_bus;
-    int num_gpio_out;
-    qemu_irq *gpio_out;
-    int num_gpio_in;
-    qemu_irq *gpio_in;
+    QLIST_HEAD(, NamedGPIOList) gpios;
     QLIST_HEAD(, BusState) child_bus;
     int num_child_bus;
     int instance_id_alias;

@@ -252,7 +260,11 @@ void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
+qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n);
+
 void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
+void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
+                                 qemu_irq pin);
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name);

@@ -262,6 +274,10 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
 /* GPIO inputs also double as IRQ sinks.  */
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n);
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n);
+void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
+                             const char *name, int n);
+void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
+                              const char *name, int n);
 
 BusState *qdev_get_parent_bus(DeviceState *dev);

qdev-monitor.c +4 -5

index 02cbe43..bb4c007 100644
@@ -613,14 +613,18 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
 {
     ObjectClass *class;
     BusState *child;
+    NamedGPIOList *ngl;
+
     qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
                 dev->id ? dev->id : "");
     indent += 2;
-    if (dev->num_gpio_in) {
-        qdev_printf("gpio-in %d\n", dev->num_gpio_in);
-    }
-    if (dev->num_gpio_out) {
-        qdev_printf("gpio-out %d\n", dev->num_gpio_out);
+    QLIST_FOREACH(ngl, &dev->gpios, node) {
+        if (ngl->num_in) {

Quoted 9 lines

qdev-monitor.c static void qdev_print(Monitor *mon, DeviceState *dev, int indent) · line 627
Quoted 5 lines
qdev-monitor.c +5 -0
+            qdev_printf("gpio-in \"%s\" %d\n", ngl->name, ngl->num_in);
+        }
+        if (ngl->num_out) {
+            qdev_printf("gpio-out \"%s\" %d\n", ngl->name, ngl->num_out);
+        }
This is going to hand printf a NULL pointer for the name if it's printing the default gpio arrays, isn't it?

Fixed with a ? : check.

Quoted 40 lines

Quoted 23 lines

qdev-monitor.c +0 -0

     }
     class = object_get_class(OBJECT(dev));
     do {

qtest.c +2 -3

index 2aba20d..36c317a 100644
@@ -233,7 +233,8 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
     g_assert(command);
     if (strcmp(words[0], "irq_intercept_out") == 0
         || strcmp(words[0], "irq_intercept_in") == 0) {
-       DeviceState *dev;
+        DeviceState *dev;
+        NamedGPIOList *ngl;
 
         g_assert(words[1]);
         dev = DEVICE(object_resolve_path(words[1], NULL));

@@ -253,10 +254,14 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
            return;
         }
 
-        if (words[0][14] == 'o') {
-            qemu_irq_intercept_out(&dev->gpio_out, qtest_irq_handler, dev->num_gpio_out);

qtest.c static void qtest_process_command(CharDriverState *chr, gchar **words) · line 264

Quoted 10 lines

qtest.c +8 -2

-        } else {
-            qemu_irq_intercept_in(dev->gpio_in, qtest_irq_handler, dev->num_gpio_in);
+        QLIST_FOREACH(ngl, &dev->gpios, node) {
+            if (words[0][14] == 'o') {
+                qemu_irq_intercept_out(&ngl->out, qtest_irq_handler,
+                                       ngl->num_out);
+            } else {
+                qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
+                                      ngl->num_in);
+            }

Quoted 8 lines

I would suggest that you make this patch just retain the current behaviour of intercepting only the default gpio in/out arrays (ie the name==NULL ones). Then you can have a separate followup patch which fixes the qtest protocol messages (both the 'irq_intercept_{in,out}' ones from the test and the IRQ raise/lower replies) to include the gpio array name.

Done.

Respin on list. Thanks for the review.

Regards,
Peter

Quoted 10 lines

Quoted 6 lines

qtest.c +0 -1

         }
         irq_intercept_dev = dev;
         qtest_send_prefix(chr);
--

1.9.2.1.g06c4abd

thanks
-- PMM

Peter Crosthwaite

author

2014-05-08 07:01 UTC #4 lore raw

PATCH v3 2/2 ssi: Name the CS GPIO.

In reply to Peter Crosthwaite

To get it out of the default GPIO list. This allows child devices to use the un-named GPIO namespace without having to be SSI aware. That is, there is no more need for machines to know about the obscure policy where GPIO 0 is the SSI chip-select and GPIO 1..N are the concrete class GPIOs (defined locally as 0..N-1).

This is most notable is stellaris, which uses a device which has both SSI and concrete level GPIOs.

Signed-off-by Peter Crosthwaite <[email protected]>

---

 hw/arm/stellaris.c                  | 7 ++++---
 hw/arm/xilinx_zynq.c                | 2 +-
 hw/microblaze/petalogix_ml605_mmu.c | 2 +-
 hw/ssi/ssi.c                        | 4 ++--
 include/hw/ssi.h                    | 2 ++
 5 files changed, 10 insertions(+), 7 deletions(-)

hw/arm/stellaris.c +4 -3

index d6cc77b..b37669a 100644
@@ -1287,9 +1287,10 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
 
             sddev = ssi_create_slave(bus, "ssi-sd");
             ssddev = ssi_create_slave(bus, "ssd0323");
-            gpio_out[GPIO_D][0] = qemu_irq_split(qdev_get_gpio_in(sddev, 0),
-                                                 qdev_get_gpio_in(ssddev, 0));
-            gpio_out[GPIO_C][7] = qdev_get_gpio_in(ssddev, 1);
+            gpio_out[GPIO_D][0] = qemu_irq_split(
+                    qdev_get_gpio_in_named(sddev, SSI_GPIO_CS, 0),
+                    qdev_get_gpio_in_named(ssddev, SSI_GPIO_CS, 0));
+            gpio_out[GPIO_C][7] = qdev_get_gpio_in(ssddev, 0);
 
             /* Make sure the select pin is high.  */
             qemu_irq_raise(gpio_out[GPIO_D][0]);

hw/arm/xilinx_zynq.c +1 -1

index 9ee21e7..1ce7b79 100644
@@ -94,7 +94,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
         for (j = 0; j < num_ss; ++j) {
             flash_dev = ssi_create_slave(spi, "n25q128");
 
-            cs_line = qdev_get_gpio_in(flash_dev, 0);
+            cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
             sysbus_connect_irq(busdev, i * num_ss + j + 1, cs_line);
         }
     }

hw/microblaze/petalogix_ml605_mmu.c +1 -1

index 40a9f5c..3d7aa6e 100644
@@ -196,7 +196,7 @@ petalogix_ml605_init(QEMUMachineInitArgs *args)
             qemu_irq cs_line;
 
             dev = ssi_create_slave(spi, "n25q128");
-            cs_line = qdev_get_gpio_in(dev, 0);
+            cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
             sysbus_connect_irq(busdev, i+1, cs_line);
         }
     }

hw/ssi/ssi.c +2 -2

index 017f022..47c188a 100644
@@ -60,7 +60,7 @@ static int ssi_slave_init(DeviceState *dev)
 
     if (ssc->transfer_raw == ssi_transfer_raw_default &&
             ssc->cs_polarity != SSI_CS_NONE) {
-        qdev_init_gpio_in(dev, ssi_cs_default, 1);
+        qdev_init_gpio_in_named(dev, ssi_cs_default, SSI_GPIO_CS, 1);
     }
 
     return ssc->init(s);

@@ -156,7 +156,7 @@ static int ssi_auto_connect_slave(Object *child, void *opaque)
         return 0;
     }
 
-    cs_line = qdev_get_gpio_in(DEVICE(dev), 0);
+    cs_line = qdev_get_gpio_in_named(DEVICE(dev), SSI_GPIO_CS, 0);
     qdev_set_parent_bus(DEVICE(dev), BUS(arg->bus));
     **arg->cs_linep = cs_line;
     (*arg->cs_linep)++;

include/hw/ssi.h +2 -0

index 6c13fb2..df0f838 100644
@@ -23,6 +23,8 @@ typedef struct SSISlave SSISlave;
 #define SSI_SLAVE_GET_CLASS(obj) \
      OBJECT_GET_CLASS(SSISlaveClass, (obj), TYPE_SSI_SLAVE)
 
+#define SSI_GPIO_CS "ssi-gpio-cs"
+
 typedef enum {
     SSI_CS_NONE = 0,
     SSI_CS_LOW,

Signature · 1 line

1.9.2.1.g06c4abd

Peter Maydell

Reviewed-by on v3 2/2

2014-05-09 16:56 UTC #5 lore raw

Re: [Qemu-devel] [PATCH v3 2/2] ssi: Name the CS GPIO.

Quoted 10 lines · Peter Crosthwaite

To get it out of the default GPIO list. This allows child devices to use the un-named GPIO namespace without having to be SSI aware. That is, there is no more need for machines to know about the obscure policy where GPIO 0 is the SSI chip-select and GPIO 1..N are the concrete class GPIOs (defined locally as 0..N-1).
This is most notable is stellaris, which uses a device which has both SSI and concrete level GPIOs.
Signed-off-by Peter Crosthwaite <[email protected]>

Reviewed-by Peter Maydell <[email protected]>

-- PMM