[Qemu-devel] [PATCH v3 0/2] Named GPIOs

[Qemu-devel] [PATCH v3 0/2] Named GPIOs

[Peter Crosthwaite]

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(-)

-- 
1.9.2.1.g06c4abd

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

[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 <peter.crosthwaite@xilinx.com>
---
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(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 936eae6..8b54db2 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -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)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index dbe473c..04fca4e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -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);
 
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 02cbe43..bb4c007 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -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 {
diff --git a/qtest.c b/qtest.c
index 2aba20d..36c317a 100644
--- a/qtest.c
+++ b/qtest.c
@@ -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);
-- 
1.9.2.1.g06c4abd

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

[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 <peter.crosthwaite@xilinx.com>
---

 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(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index d6cc77b..b37669a 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -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]);
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 9ee21e7..1ce7b79 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -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);
         }
     }
diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index 40a9f5c..3d7aa6e 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -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);
         }
     }
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index 017f022..47c188a 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -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)++;
diff --git a/include/hw/ssi.h b/include/hw/ssi.h
index 6c13fb2..df0f838 100644
--- a/include/hw/ssi.h
+++ b/include/hw/ssi.h
@@ -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,
-- 
1.9.2.1.g06c4abd

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

[Peter Maydell]

On 8 May 2014 07:59, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 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 <peter.crosthwaite@xilinx.com>
> ---
> 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(-)

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.

> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 936eae6..8b54db2 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -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))) {

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

> +            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

> +    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)
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index dbe473c..04fca4e 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -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);
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 02cbe43..bb4c007 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -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);
> +        }

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

>      }
>      class = object_get_class(OBJECT(dev));
>      do {
> diff --git a/qtest.c b/qtest.c
> index 2aba20d..36c317a 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -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);
> +            }

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.

>          }
>          irq_intercept_dev = dev;
>          qtest_send_prefix(chr);
> --
> 1.9.2.1.g06c4abd
>

thanks
-- PMM

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

[Peter Maydell]

On 8 May 2014 07:59, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 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 <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

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

[Peter Crosthwaite]

On Sat, May 10, 2014 at 2:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 May 2014 07:59, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> 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 <peter.crosthwaite@xilinx.com>
>> ---
>> 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(-)
>
> 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.
>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 936eae6..8b54db2 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -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))) {
>
> 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) ||

>> +            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
>

Implemented.

>> +    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)
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index dbe473c..04fca4e 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -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);
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 02cbe43..bb4c007 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -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);
>> +        }
>
> 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.

>>      }
>>      class = object_get_class(OBJECT(dev));
>>      do {
>> diff --git a/qtest.c b/qtest.c
>> index 2aba20d..36c317a 100644
>> --- a/qtest.c
>> +++ b/qtest.c
>> @@ -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);
>> +            }
>
> 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.
>

Done.

Respin on list. Thanks for the review.

Regards,
Peter

>>          }
>>          irq_intercept_dev = dev;
>>          qtest_send_prefix(chr);
>> --
>> 1.9.2.1.g06c4abd
>>
>
> thanks
> -- PMM
>