[Qemu-devel] [PATCH v1 00/16] GPIO/IRQ QOMification: Phase 2 - Getting rid of SYSBUS IRQs

[Qemu-devel] [PATCH v1 00/16] GPIO/IRQ QOMification: Phase 2 - Getting rid of SYSBUS IRQs

[Peter Crosthwaite]

Hi All,

So phase one was the QOMification of qemu_irq. This is the next step.
We start to setup GPIOs as proper QOM objects. Inputs are child objects
of their device. Outputs are settable Links and connection is made
via proper setting of a QOM link.

We then cleanup Sysbus to simply re-use device level GPIOs and get rid
of it's special IRQ handling code.

Depends of my pending QOM array property stuff (the "[*]" series):

https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg04116.html


Peter Crosthwaite (16):
  qdev: gpio: Don't allow name share between I and O
  qdev: gpio: Register GPIO inputs as child objects
  qdev: gpio: Register GPIO outputs as QOM links
  qmp: qstring: Handle NULL strings
  qom: Allow clearing of a Link property
  qom: Demote already-has-a-parent to a regular error
  qdev: gpio: Re-impement qdev_connect_gpio QOM style
  qdev: gpio: Add API for intercepting an IRQ
  qtest/irq: Rework IRQ interception
  irq: Remove qemu_irq_intercept_out
  qdev: gpio: delete NamedGPIOList::out
  qdev: gpio: Remove qdev_init_gpio_out x1 restriction
  qdev: gpio: Define qdev_pass_gpios()
  ssi: xilinx_spi: Initialise CS GPIOs as NULL
  ppc: convert g_new(qemu_irq usages to g_new0
  sysbus: Use TYPE_DEVICE GPIO functionality

 hw/core/irq.c          |  8 +----
 hw/core/qdev.c         | 89 ++++++++++++++++++++++++++++++++++++++++++++++----
 hw/core/sysbus.c       | 20 ++----------
 hw/intc/openpic.c      |  2 +-
 hw/ppc/e500.c          |  2 +-
 hw/ppc/mac_newworld.c  |  2 +-
 hw/ssi/xilinx_spi.c    |  2 +-
 include/hw/irq.h       |  1 -
 include/hw/qdev-core.h |  6 +++-
 include/hw/sysbus.h    |  6 ++--
 qobject/qstring.c      | 12 ++++---
 qom/object.c           | 10 ++++--
 qtest.c                | 11 ++++---
 13 files changed, 120 insertions(+), 51 deletions(-)

-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 01/16] qdev: gpio: Don't allow name share between I and O

[Peter Crosthwaite]

Only allow a GPIO name to be one or the other. Inputs and outputs are
functionally different and should be in different namespaces. Prepares
support for the QOMification of IRQs as Links or Child objects.

The alternative is to munge names .e.g. with "-in" or "-out" suffixes
when giving QOM names. But that reduces clarity and if there are cases
out there where users want I and O with same name they can manually add
their own suffixes.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/qdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index da1ba48..77441aa 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -339,6 +339,7 @@ void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
 {
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
 
+    assert(gpio_list->num_out == 0 || !name);
     gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
                                      dev, n);
     gpio_list->num_in += n;
@@ -354,6 +355,7 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
 {
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
 
+    assert(gpio_list->num_in == 0 || !name);
     assert(gpio_list->num_out == 0);
     gpio_list->num_out = n;
     gpio_list->out = pins;
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 02/16] qdev: gpio: Register GPIO inputs as child objects

[Peter Crosthwaite]

To the device that contains them. This will allow for referencing
a GPIO input from it's canonical path (exciting for dynamic machine
generation!)

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/qdev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 77441aa..35c266a 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -337,11 +337,20 @@ static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
 void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
                              const char *name, int n)
 {
+    int i;
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
 
     assert(gpio_list->num_out == 0 || !name);
     gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
                                      dev, n);
+
+    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-in");
+    for (i = gpio_list->num_in; i < gpio_list->num_in + n; i++) {
+        object_property_add_child(OBJECT(dev), propname,
+                                  OBJECT(gpio_list->in[i]), &error_abort);
+    }
+    g_free(propname);
+
     gpio_list->num_in += n;
 }
 
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 03/16] qdev: gpio: Register GPIO outputs as QOM links

[Peter Crosthwaite]

Within the object that contains the GPIO output. This allows for
connecting GPIO outputs via setting of a Link property.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/qdev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 35c266a..5f55709 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -362,12 +362,23 @@ void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
 void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
                               const char *name, int n)
 {
+    int i;
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
 
     assert(gpio_list->num_in == 0 || !name);
     assert(gpio_list->num_out == 0);
     gpio_list->num_out = n;
     gpio_list->out = pins;
+
+    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
+    for (i = 0; i < n; ++i) {
+        object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
+                                 (Object **)&pins[i],
+                                 object_property_allow_set_link,
+                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                                 &error_abort);
+    }
+    g_free(propname);
 }
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
-- 
2.0.1.1.gfbfc394

Re: [Qemu-devel] [PATCH v1 03/16] qdev: gpio: Register GPIO outputs as QOM links

[Peter Crosthwaite]

On Mon, Aug 4, 2014 at 11:53 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Within the object that contains the GPIO output. This allows for
> connecting GPIO outputs via setting of a Link property.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/core/qdev.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 35c266a..5f55709 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -362,12 +362,23 @@ void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
>  void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>                                const char *name, int n)
>  {
> +    int i;
>      NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>
>      assert(gpio_list->num_in == 0 || !name);
>      assert(gpio_list->num_out == 0);
>      gpio_list->num_out = n;
>      gpio_list->out = pins;
> +
> +    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");

Moved this up the function in V2. sry.

Regards,
Peter

> +    for (i = 0; i < n; ++i) {
> +        object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
> +                                 (Object **)&pins[i],
> +                                 object_property_allow_set_link,
> +                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
> +                                 &error_abort);
> +    }
> +    g_free(propname);
>  }
>
>  void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
> --
> 2.0.1.1.gfbfc394
>

[Qemu-devel] [PATCH v1 04/16] qmp: qstring: Handle NULL strings

[Peter Crosthwaite]

Create a valid qobject even if the input string is null.
qstring->string will be NULL and length will be 0.

This prepares support for clearing of QOM Link properties
where NULL canonical path string will be passes through
this API.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 qobject/qstring.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/qobject/qstring.c b/qobject/qstring.c
index 607b7a1..a15ffed 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -48,14 +48,16 @@ QString *qstring_from_substr(const char *str, int start, int end)
 {
     QString *qstring;
 
-    qstring = g_malloc(sizeof(*qstring));
+    qstring = g_malloc0(sizeof(*qstring));
 
     qstring->length = end - start + 1;
     qstring->capacity = qstring->length;
 
-    qstring->string = g_malloc(qstring->capacity + 1);
-    memcpy(qstring->string, str + start, qstring->length);
-    qstring->string[qstring->length] = 0;
+    if (str) {
+        qstring->string = g_malloc(qstring->capacity + 1);
+        memcpy(qstring->string, str + start, qstring->length);
+        qstring->string[qstring->length] = 0;
+    }
 
     QOBJECT_INIT(qstring, &qstring_type);
 
@@ -69,7 +71,7 @@ QString *qstring_from_substr(const char *str, int start, int end)
  */
 QString *qstring_from_str(const char *str)
 {
-    return qstring_from_substr(str, 0, strlen(str) - 1);
+    return qstring_from_substr(str, 0, (str ? strlen(str) : 0) - 1);
 }
 
 static void capacity_increase(QString *qstring, size_t len)
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 05/16] qom: Allow clearing of a Link property

[Peter Crosthwaite]

By passing in NULL to object_property_set_link.

The lead user of this is the QDEV GPIO framework which will implement
GPIO disconnects via an "unlink".

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 qom/object.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index c869e8e..8821e23 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -881,7 +881,7 @@ char *object_property_get_str(Object *obj, const char *name,
 void object_property_set_link(Object *obj, Object *value,
                               const char *name, Error **errp)
 {
-    gchar *path = object_get_canonical_path(value);
+    gchar *path = value ? object_get_canonical_path(value) : NULL;
     object_property_set_str(obj, path, name, errp);
     g_free(path);
 }
@@ -1172,7 +1172,7 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
 
     visit_type_str(v, &path, name, &local_err);
 
-    if (!local_err && strcmp(path, "") != 0) {
+    if (!local_err && path && strcmp(path, "") != 0) {
         new_target = object_resolve_link(obj, name, path, &local_err);
     }
 
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 06/16] qom: Demote already-has-a-parent to a regular error

[Peter Crosthwaite]

Rather than an abort(). This allows callers to decide whether parenting
an already-parented object is a fatal error condition.

Useful for providing a default value for an object's parent in the case
where you want to set one iff it doesn't already have one.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 qom/object.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index 8821e23..9da0064 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1074,6 +1074,11 @@ void object_property_add_child(Object *obj, const char *name,
     gchar *type;
     ObjectProperty *op;
 
+    if (child->parent != NULL) {
+        error_setg(errp, "child object is already parented");
+        return;
+    }
+
     type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
 
     op = object_property_add(obj, name, type, object_get_child_property, NULL,
@@ -1085,7 +1090,6 @@ void object_property_add_child(Object *obj, const char *name,
 
     op->resolve = object_resolve_child_property;
     object_ref(child);
-    g_assert(child->parent == NULL);
     child->parent = obj;
 
 out:
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 07/16] qdev: gpio: Re-impement qdev_connect_gpio QOM style

[Peter Crosthwaite]

Re-implement as a link setter. This should allow the QOM framework to
keep track of ref counts properly etc.

We need to add a default parent for the connecting input incase it's
coming from a non-qdev source. We simply parent the IRQ to the machine
in this case.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/qdev.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5f55709..e6b2231 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -402,10 +402,14 @@ qemu_irq qdev_get_gpio_in(DeviceState *dev, int 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;
+    char *propname = g_strdup_printf("%s[%d]",
+                                     name ? name : "unnamed-gpio-out", n);
+    if (pin) {
+        object_property_add_child(qdev_get_machine(), "non-qdev-gpio[*]",
+                                  OBJECT(pin), NULL);
+    }
+    object_property_set_link(OBJECT(dev), OBJECT(pin), propname, &error_abort);
+    g_free(propname);
 }
 
 void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 08/16] qdev: gpio: Add API for intercepting an IRQ

[Peter Crosthwaite]

To replace the old qemu_irq intercept API (which had users reaching
into qdev private state for IRQs).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/qdev.c         | 25 +++++++++++++++++++++++++
 include/hw/qdev-core.h |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e6b2231..4cbf773 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -412,6 +412,31 @@ void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
     g_free(propname);
 }
 
+/* disconnect a GPIO ouput, returning the disconnected input (if any) */
+
+static qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
+                                               const char *name, int n)
+{
+    char *propname = g_strdup_printf("%s[%d]",
+                                     name ? name : "unnamed-gpio-out", n);
+
+    qemu_irq ret = (qemu_irq)object_property_get_link(OBJECT(dev), propname,
+                                                      NULL);
+    if (ret) {
+        object_property_set_link(OBJECT(dev), NULL, propname, NULL);
+    }
+    g_free(propname);
+    return ret;
+}
+
+void qdev_intercept_gpio_out(DeviceState *dev, qemu_irq_handler handler,
+                             const char *name, int n)
+{
+    qemu_irq disconnected = qdev_disconnect_gpio_out_named(dev, name, n);
+    qemu_irq icpt = qemu_allocate_irq(handler, disconnected, n);
+    qdev_connect_gpio_out_named(dev, name, n, icpt);
+}
+
 void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
 {
     qdev_connect_gpio_out_named(dev, NULL, n, pin);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 0799ff2..e17e19b 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -274,6 +274,8 @@ 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);
+void qdev_intercept_gpio_out(DeviceState *dev, qemu_irq_handler handler,
+                             const char *name, int n);
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
 
-- 
2.0.1.1.gfbfc394

Re: [Qemu-devel] [PATCH v1 08/16] qdev: gpio: Add API for intercepting an IRQ

[Alexander Graf]

On 04.08.14 03:55, Peter Crosthwaite wrote:
> To replace the old qemu_irq intercept API (which had users reaching
> into qdev private state for IRQs).
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>   hw/core/qdev.c         | 25 +++++++++++++++++++++++++
>   include/hw/qdev-core.h |  2 ++
>   2 files changed, 27 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e6b2231..4cbf773 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -412,6 +412,31 @@ void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
>       g_free(propname);
>   }
>   
> +/* disconnect a GPIO ouput, returning the disconnected input (if any) */
> +
> +static qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
> +                                               const char *name, int n)
> +{
> +    char *propname = g_strdup_printf("%s[%d]",
> +                                     name ? name : "unnamed-gpio-out", n);
> +
> +    qemu_irq ret = (qemu_irq)object_property_get_link(OBJECT(dev), propname,
> +                                                      NULL);
> +    if (ret) {
> +        object_property_set_link(OBJECT(dev), NULL, propname, NULL);
> +    }
> +    g_free(propname);
> +    return ret;
> +}
> +
> +void qdev_intercept_gpio_out(DeviceState *dev, qemu_irq_handler handler,
> +                             const char *name, int n)
> +{
> +    qemu_irq disconnected = qdev_disconnect_gpio_out_named(dev, name, n);
> +    qemu_irq icpt = qemu_allocate_irq(handler, disconnected, n);

This means that we can't pass in any other opaque to the intercepting 
handler which sounds suboptimal to me. Can't we pass in a qemu_irq as 
parameter and a pointer to a field where we can store the disconnected 
qemu_irq?

Then the caller can allocate its own qemu_irq with all the opaque data 
it wants and just have the intercept function overwrite a single field 
in the passed down opaque.

Or you just return the disconnected qemu_irq. Then the caller can do 
whatever it likes with it, such as set icpt->opaque to it.


Alex

Re: [Qemu-devel] [PATCH v1 08/16] qdev: gpio: Add API for intercepting an IRQ

[Peter Crosthwaite]

On Tue, Aug 12, 2014 at 7:16 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 04.08.14 03:55, Peter Crosthwaite wrote:
>>
>> To replace the old qemu_irq intercept API (which had users reaching
>> into qdev private state for IRQs).
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>   hw/core/qdev.c         | 25 +++++++++++++++++++++++++
>>   include/hw/qdev-core.h |  2 ++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index e6b2231..4cbf773 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -412,6 +412,31 @@ void qdev_connect_gpio_out_named(DeviceState *dev,
>> const char *name, int n,
>>       g_free(propname);
>>   }
>>   +/* disconnect a GPIO ouput, returning the disconnected input (if any)
>> */
>> +
>> +static qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
>> +                                               const char *name, int n)
>> +{
>> +    char *propname = g_strdup_printf("%s[%d]",
>> +                                     name ? name : "unnamed-gpio-out",
>> n);
>> +
>> +    qemu_irq ret = (qemu_irq)object_property_get_link(OBJECT(dev),
>> propname,
>> +                                                      NULL);
>> +    if (ret) {
>> +        object_property_set_link(OBJECT(dev), NULL, propname, NULL);
>> +    }
>> +    g_free(propname);
>> +    return ret;
>> +}
>> +
>> +void qdev_intercept_gpio_out(DeviceState *dev, qemu_irq_handler handler,
>> +                             const char *name, int n)
>> +{
>> +    qemu_irq disconnected = qdev_disconnect_gpio_out_named(dev, name, n);
>> +    qemu_irq icpt = qemu_allocate_irq(handler, disconnected, n);
>
>
> This means that we can't pass in any other opaque to the intercepting
> handler which sounds suboptimal to me. Can't we pass in a qemu_irq as
> parameter and a pointer to a field where we can store the disconnected
> qemu_irq?
>
> Then the caller can allocate its own qemu_irq with all the opaque data it
> wants and just have the intercept function overwrite a single field in the
> passed down opaque.
>
> Or you just return the disconnected qemu_irq. Then the caller can do
> whatever it likes with it, such as set icpt->opaque to it.
>

Good idea. Ill play with these options and see what works best.

Regards,
Peter

>
> Alex
>
>

[Qemu-devel] [PATCH v1 09/16] qtest/irq: Rework IRQ interception

[Peter Crosthwaite]

Change the qtest intercept handler to accept just the individual IRQ
being intercepted as opaque. n is still expected to be correctly set
as for the original intercepted irq. qemu_intercept_irq_in is updated
accordingly.

Then covert the qemu_irq_intercept_out call to use qdev intercept
version. This stop qtest from having to mess with the raw IRQ pointers
(still has to mess with names and counts but as step in the right
direction).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/irq.c |  2 +-
 qtest.c       | 11 +++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/core/irq.c b/hw/core/irq.c
index cffced0..7d6318a 100644
--- a/hw/core/irq.c
+++ b/hw/core/irq.c
@@ -140,7 +140,7 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n)
     for (i = 0; i < n; i++) {
         *old_irqs[i] = *gpio_in[i];
         gpio_in[i]->handler = handler;
-        gpio_in[i]->opaque = old_irqs;
+        gpio_in[i]->opaque = old_irqs[i];
     }
 }
 
diff --git a/qtest.c b/qtest.c
index 04a6dc1..4dd5ff4 100644
--- a/qtest.c
+++ b/qtest.c
@@ -197,8 +197,8 @@ static void GCC_FMT_ATTR(2, 3) qtest_send(CharDriverState *chr,
 
 static void qtest_irq_handler(void *opaque, int n, int level)
 {
-    qemu_irq *old_irqs = opaque;
-    qemu_set_irq(old_irqs[n], level);
+    qemu_irq old_irq = opaque;
+    qemu_set_irq(old_irq, level);
 
     if (irq_levels[n] != level) {
         CharDriverState *chr = qtest_chr;
@@ -260,8 +260,11 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
                 continue;
             }
             if (words[0][14] == 'o') {
-                qemu_irq_intercept_out(&ngl->out, qtest_irq_handler,
-                                       ngl->num_out);
+                int i;
+                for (i = 0; i < ngl->num_out; ++i) {
+                    qdev_intercept_gpio_out(dev, qtest_irq_handler,
+                                            ngl->name, i);
+                }
             } else {
                 qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
                                       ngl->num_in);
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 10/16] irq: Remove qemu_irq_intercept_out

[Peter Crosthwaite]

No more users left and obsoleted by qdev_intercept_gpio_out.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/irq.c    | 6 ------
 include/hw/irq.h | 1 -
 2 files changed, 7 deletions(-)

diff --git a/hw/core/irq.c b/hw/core/irq.c
index 7d6318a..fef9cc2 100644
--- a/hw/core/irq.c
+++ b/hw/core/irq.c
@@ -144,12 +144,6 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n)
     }
 }
 
-void qemu_irq_intercept_out(qemu_irq **gpio_out, qemu_irq_handler handler, int n)
-{
-    qemu_irq *old_irqs = *gpio_out;
-    *gpio_out = qemu_allocate_irqs(handler, old_irqs, n);
-}
-
 static const TypeInfo irq_type_info = {
    .name = TYPE_IRQ,
    .parent = TYPE_OBJECT,
diff --git a/include/hw/irq.h b/include/hw/irq.h
index 6f874f5..4c4c2ea 100644
--- a/include/hw/irq.h
+++ b/include/hw/irq.h
@@ -61,6 +61,5 @@ qemu_irq *qemu_irq_proxy(qemu_irq **target, int n);
 /* For internal use in qtest.  Similar to qemu_irq_split, but operating
    on an existing vector of qemu_irq.  */
 void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n);
-void qemu_irq_intercept_out(qemu_irq **gpio_out, qemu_irq_handler handler, int n);
 
 #endif
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 11/16] qdev: gpio: delete NamedGPIOList::out

[Peter Crosthwaite]

All users of GPIO outputs are fully QOMified, using QOM properties to
access the GPIO data. Delete.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/qdev.c         | 1 -
 include/hw/qdev-core.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4cbf773..77b0b09 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -368,7 +368,6 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
     assert(gpio_list->num_in == 0 || !name);
     assert(gpio_list->num_out == 0);
     gpio_list->num_out = n;
-    gpio_list->out = pins;
 
     char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
     for (i = 0; i < n; ++i) {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e17e19b..d3326b1 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -137,7 +137,6 @@ struct NamedGPIOList {
     char *name;
     qemu_irq *in;
     int num_in;
-    qemu_irq *out;
     int num_out;
     QLIST_ENTRY(NamedGPIOList) node;
 };
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 12/16] qdev: gpio: Remove qdev_init_gpio_out x1 restriction

[Peter Crosthwaite]

Previously this was restricted to a single call per-dev/per-name. With
the conversion of the GPIO output state to QOM the implementation can
now handle repeated calls. Remove the restriction.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/qdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 77b0b09..bf2c227 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -366,8 +366,7 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
 
     assert(gpio_list->num_in == 0 || !name);
-    assert(gpio_list->num_out == 0);
-    gpio_list->num_out = n;
+    gpio_list->num_out += n;
 
     char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
     for (i = 0; i < n; ++i) {
-- 
2.0.1.1.gfbfc394

Re: [Qemu-devel] [PATCH v1 12/16] qdev: gpio: Remove qdev_init_gpio_out x1 restriction

[Alexander Graf]

On 04.08.14 03:58, Peter Crosthwaite wrote:
> Previously this was restricted to a single call per-dev/per-name. With
> the conversion of the GPIO output state to QOM the implementation can
> now handle repeated calls. Remove the restriction.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>   hw/core/qdev.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 77b0b09..bf2c227 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -366,8 +366,7 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,

Maybe rename it to _add rather than _init then?


Alex

>       NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>   
>       assert(gpio_list->num_in == 0 || !name);
> -    assert(gpio_list->num_out == 0);
> -    gpio_list->num_out = n;
> +    gpio_list->num_out += n;
>   
>       char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
>       for (i = 0; i < n; ++i) {

Re: [Qemu-devel] [PATCH v1 12/16] qdev: gpio: Remove qdev_init_gpio_out x1 restriction

[Peter Crosthwaite]

On Tue, Aug 12, 2014 at 7:19 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 04.08.14 03:58, Peter Crosthwaite wrote:
>>
>> Previously this was restricted to a single call per-dev/per-name. With
>> the conversion of the GPIO output state to QOM the implementation can
>> now handle repeated calls. Remove the restriction.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>   hw/core/qdev.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 77b0b09..bf2c227 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -366,8 +366,7 @@ void qdev_init_gpio_out_named(DeviceState *dev,
>> qemu_irq *pins,
>
>
> Maybe rename it to _add rather than _init then?
>

Hmmm that's a tree-wide (unless we allow the _named variant to have
inconsistent naming with the well used qdev_init_gpio_out). This
change does make the behaviour consistent with current
qdev_init_gpio_in  behav where you are already allowed multiple calls:

qdev_init_gpio_in(dev, foo, ...);
qdev_init_gpio_in(dev, bar, ...);

Perhaps fix them both together with the tree wide as follow up?

Regards,
Peter

>
> Alex
>
>
>>       NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>>         assert(gpio_list->num_in == 0 || !name);
>> -    assert(gpio_list->num_out == 0);
>> -    gpio_list->num_out = n;
>> +    gpio_list->num_out += n;
>>         char *propname = g_strdup_printf("%s[*]", name ? name :
>> "unnamed-gpio-out");
>>       for (i = 0; i < n; ++i) {
>
>
>

Re: [Qemu-devel] [PATCH v1 12/16] qdev: gpio: Remove qdev_init_gpio_out x1 restriction

[Alexander Graf]

On 12.08.14 12:35, Peter Crosthwaite wrote:
> On Tue, Aug 12, 2014 at 7:19 PM, Alexander Graf <agraf@suse.de> wrote:
>> On 04.08.14 03:58, Peter Crosthwaite wrote:
>>> Previously this was restricted to a single call per-dev/per-name. With
>>> the conversion of the GPIO output state to QOM the implementation can
>>> now handle repeated calls. Remove the restriction.
>>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> ---
>>>
>>>    hw/core/qdev.c | 3 +--
>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 77b0b09..bf2c227 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -366,8 +366,7 @@ void qdev_init_gpio_out_named(DeviceState *dev,
>>> qemu_irq *pins,
>>
>> Maybe rename it to _add rather than _init then?
>>
> Hmmm that's a tree-wide (unless we allow the _named variant to have
> inconsistent naming with the well used qdev_init_gpio_out). This
> change does make the behaviour consistent with current
> qdev_init_gpio_in  behav where you are already allowed multiple calls:
>
> qdev_init_gpio_in(dev, foo, ...);
> qdev_init_gpio_in(dev, bar, ...);
>
> Perhaps fix them both together with the tree wide as follow up?

Sure, a follow-up works for me :).


Alex

[Qemu-devel] [PATCH v1 13/16] qdev: gpio: Define qdev_pass_gpios()

[Peter Crosthwaite]

Allows a container to take ownership of GPIOs in a contained
device and automatically connect them as GPIOs to the container.

This prepares for deprecation of the SYSBUS IRQ functionality, which
has this feature. We push it up to the device level instead of sysbus
level. There's nothing sysbus specific about passing GPIOs to
containers so its a legitimate device-level generic feature.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/qdev.c         | 28 ++++++++++++++++++++++++++++
 include/hw/qdev-core.h |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index bf2c227..708363f 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -440,6 +440,34 @@ void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
     qdev_connect_gpio_out_named(dev, NULL, n, pin);
 }
 
+void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
+                     const char *name)
+{
+    int i;
+    NamedGPIOList *ngl = qdev_get_named_gpio_list(dev, name);
+
+    for (i = 0; i < ngl->num_in; i++) {
+        char *propname = g_strdup_printf("%s[%d]",
+                                         ngl->name ? ngl->name :
+                                                   "unnamed-gpio-in",
+                                         i);
+        object_property_add_alias(OBJECT(container), propname,
+                                  OBJECT(dev), propname,
+                                  &error_abort);
+    }
+    for (i = 0; i < ngl->num_out; i++) {
+        char *propname = g_strdup_printf("%s[%d]",
+                                         ngl->name ? ngl->name :
+                                                     "unnamed-gpio-in",
+                                         i);
+        object_property_add_alias(OBJECT(container), propname,
+                                  OBJECT(dev), propname,
+                                  &error_abort);
+    }
+    QLIST_REMOVE(ngl, node);
+    QLIST_INSERT_HEAD(&container->gpios, ngl, node);
+}
+
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
 {
     BusState *bus;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index d3326b1..08dafda 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -289,6 +289,9 @@ void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
 void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
                               const char *name, int n);
 
+void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
+                     const char *name);
+
 BusState *qdev_get_parent_bus(DeviceState *dev);
 
 /*** BUS API. ***/
-- 
2.0.1.1.gfbfc394

Re: [Qemu-devel] [PATCH v1 13/16] qdev: gpio: Define qdev_pass_gpios()

[Alexander Graf]

On 04.08.14 03:58, Peter Crosthwaite wrote:
> Allows a container to take ownership of GPIOs in a contained
> device and automatically connect them as GPIOs to the container.
>
> This prepares for deprecation of the SYSBUS IRQ functionality, which
> has this feature. We push it up to the device level instead of sysbus
> level. There's nothing sysbus specific about passing GPIOs to
> containers so its a legitimate device-level generic feature.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>   hw/core/qdev.c         | 28 ++++++++++++++++++++++++++++
>   include/hw/qdev-core.h |  3 +++
>   2 files changed, 31 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index bf2c227..708363f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -440,6 +440,34 @@ void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
>       qdev_connect_gpio_out_named(dev, NULL, n, pin);
>   }
>   
> +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
> +                     const char *name)
> +{
> +    int i;
> +    NamedGPIOList *ngl = qdev_get_named_gpio_list(dev, name);
> +
> +    for (i = 0; i < ngl->num_in; i++) {
> +        char *propname = g_strdup_printf("%s[%d]",
> +                                         ngl->name ? ngl->name :
> +                                                   "unnamed-gpio-in",

Really just a minor nit, but I think the code flow would look a lot 
nicer if you did the name check in a separate variable.

   const char *name = ngl->name ? ngl->name : "unnamed-gpio-in";
   for (i = 0; ...) {
     char *propname = g_strdup_printf("%s[%d]", name, i);
     ....
   }

Also I don't fully grasp what the naming scheme is supposed to be here. 
Who sets the name and why is there only a single global name for all GPIOs?


Alex

> +                                         i);
> +        object_property_add_alias(OBJECT(container), propname,
> +                                  OBJECT(dev), propname,
> +                                  &error_abort);
> +    }
> +    for (i = 0; i < ngl->num_out; i++) {
> +        char *propname = g_strdup_printf("%s[%d]",
> +                                         ngl->name ? ngl->name :
> +                                                     "unnamed-gpio-in",
> +                                         i);
> +        object_property_add_alias(OBJECT(container), propname,
> +                                  OBJECT(dev), propname,
> +                                  &error_abort);
> +    }
> +    QLIST_REMOVE(ngl, node);
> +    QLIST_INSERT_HEAD(&container->gpios, ngl, node);
> +}
> +
>   BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
>   {
>       BusState *bus;
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index d3326b1..08dafda 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -289,6 +289,9 @@ void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
>   void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>                                 const char *name, int n);
>   
> +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
> +                     const char *name);
> +
>   BusState *qdev_get_parent_bus(DeviceState *dev);
>   
>   /*** BUS API. ***/

Re: [Qemu-devel] [PATCH v1 13/16] qdev: gpio: Define qdev_pass_gpios()

[Peter Crosthwaite]

On Tue, Aug 12, 2014 at 7:24 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 04.08.14 03:58, Peter Crosthwaite wrote:
>>
>> Allows a container to take ownership of GPIOs in a contained
>> device and automatically connect them as GPIOs to the container.
>>
>> This prepares for deprecation of the SYSBUS IRQ functionality, which
>> has this feature. We push it up to the device level instead of sysbus
>> level. There's nothing sysbus specific about passing GPIOs to
>> containers so its a legitimate device-level generic feature.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>   hw/core/qdev.c         | 28 ++++++++++++++++++++++++++++
>>   include/hw/qdev-core.h |  3 +++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index bf2c227..708363f 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -440,6 +440,34 @@ void qdev_connect_gpio_out(DeviceState * dev, int n,
>> qemu_irq pin)
>>       qdev_connect_gpio_out_named(dev, NULL, n, pin);
>>   }
>>   +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
>> +                     const char *name)
>> +{
>> +    int i;
>> +    NamedGPIOList *ngl = qdev_get_named_gpio_list(dev, name);
>> +
>> +    for (i = 0; i < ngl->num_in; i++) {
>> +        char *propname = g_strdup_printf("%s[%d]",
>> +                                         ngl->name ? ngl->name :
>> +                                                   "unnamed-gpio-in",
>
>
> Really just a minor nit, but I think the code flow would look a lot nicer if
> you did the name check in a separate variable.
>
>   const char *name = ngl->name ? ngl->name : "unnamed-gpio-in";

I think I may even go an extra step and get it macroified. How about:

#define QDEV_GPIO_IN_NAME(a) ((a) ? (a) : "unnamed-gpio-io")

and then you can continue to use it inline without extra variables or
nasty "?:"?

>   for (i = 0; ...) {
>     char *propname = g_strdup_printf("%s[%d]", name, i);
>     ....
>   }
>
> Also I don't fully grasp what the naming scheme is supposed to be here. Who
> sets the name and why is there only a single global name for all GPIOs?
>

Ideally, the instantiating device sets the names. There's not a global
name for all GPIOs, just an over-used default. The intention is that
qdev_init_gpio_in is phased out in favor of qdev_init_gpio_in_named.
If NULL name is given or qdev_init_gpio_in is used, it defaults to
this single global name here. All sysbus IRQs also share a  single
name (seperate from the qdev default) but that sharing is implemented
on the sysbus level and transparent to qdev. The need for a default
name is to appease QOM, which needs a valid string for canonical path.

Regards,
Peter

>
> Alex
>
>
>> +                                         i);
>> +        object_property_add_alias(OBJECT(container), propname,
>> +                                  OBJECT(dev), propname,
>> +                                  &error_abort);
>> +    }
>> +    for (i = 0; i < ngl->num_out; i++) {
>> +        char *propname = g_strdup_printf("%s[%d]",
>> +                                         ngl->name ? ngl->name :
>> +                                                     "unnamed-gpio-in",
>> +                                         i);
>> +        object_property_add_alias(OBJECT(container), propname,
>> +                                  OBJECT(dev), propname,
>> +                                  &error_abort);
>> +    }
>> +    QLIST_REMOVE(ngl, node);
>> +    QLIST_INSERT_HEAD(&container->gpios, ngl, node);
>> +}
>> +
>>   BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
>>   {
>>       BusState *bus;
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index d3326b1..08dafda 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -289,6 +289,9 @@ void qdev_init_gpio_in_named(DeviceState *dev,
>> qemu_irq_handler handler,
>>   void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>>                                 const char *name, int n);
>>   +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
>> +                     const char *name);
>> +
>>   BusState *qdev_get_parent_bus(DeviceState *dev);
>>     /*** BUS API. ***/
>
>
>

Re: [Qemu-devel] [PATCH v1 13/16] qdev: gpio: Define qdev_pass_gpios()

[Alexander Graf]

On 12.08.14 12:48, Peter Crosthwaite wrote:
> On Tue, Aug 12, 2014 at 7:24 PM, Alexander Graf <agraf@suse.de> wrote:
>> On 04.08.14 03:58, Peter Crosthwaite wrote:
>>> Allows a container to take ownership of GPIOs in a contained
>>> device and automatically connect them as GPIOs to the container.
>>>
>>> This prepares for deprecation of the SYSBUS IRQ functionality, which
>>> has this feature. We push it up to the device level instead of sysbus
>>> level. There's nothing sysbus specific about passing GPIOs to
>>> containers so its a legitimate device-level generic feature.
>>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> ---
>>>
>>>    hw/core/qdev.c         | 28 ++++++++++++++++++++++++++++
>>>    include/hw/qdev-core.h |  3 +++
>>>    2 files changed, 31 insertions(+)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index bf2c227..708363f 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -440,6 +440,34 @@ void qdev_connect_gpio_out(DeviceState * dev, int n,
>>> qemu_irq pin)
>>>        qdev_connect_gpio_out_named(dev, NULL, n, pin);
>>>    }
>>>    +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
>>> +                     const char *name)
>>> +{
>>> +    int i;
>>> +    NamedGPIOList *ngl = qdev_get_named_gpio_list(dev, name);
>>> +
>>> +    for (i = 0; i < ngl->num_in; i++) {
>>> +        char *propname = g_strdup_printf("%s[%d]",
>>> +                                         ngl->name ? ngl->name :
>>> +                                                   "unnamed-gpio-in",
>>
>> Really just a minor nit, but I think the code flow would look a lot nicer if
>> you did the name check in a separate variable.
>>
>>    const char *name = ngl->name ? ngl->name : "unnamed-gpio-in";
> I think I may even go an extra step and get it macroified. How about:
>
> #define QDEV_GPIO_IN_NAME(a) ((a) ? (a) : "unnamed-gpio-io")
>
> and then you can continue to use it inline without extra variables or
> nasty "?:"?

The variable will get optimized out, so for the sake of readability I 
would still vote to have it around. Whether you declare it via a macro 
or with an a ? a : b macro I don't really care much about :).

In fact, it might make more sense to literally have the typical

   (a ? a : b)

flow macroified in a generic place somewhere.

>
>>    for (i = 0; ...) {
>>      char *propname = g_strdup_printf("%s[%d]", name, i);
>>      ....
>>    }
>>
>> Also I don't fully grasp what the naming scheme is supposed to be here. Who
>> sets the name and why is there only a single global name for all GPIOs?
>>
> Ideally, the instantiating device sets the names. There's not a global
> name for all GPIOs, just an over-used default. The intention is that
> qdev_init_gpio_in is phased out in favor of qdev_init_gpio_in_named.
> If NULL name is given or qdev_init_gpio_in is used, it defaults to
> this single global name here. All sysbus IRQs also share a  single
> name (seperate from the qdev default) but that sharing is implemented
> on the sysbus level and transparent to qdev. The need for a default
> name is to appease QOM, which needs a valid string for canonical path.

Ah, I missed the fact that qdev_get_named_gpio_list() returns a list of 
elements with exactly the name you searched for in the first place.


Alex

Re: [Qemu-devel] [PATCH v1 13/16] qdev: gpio: Define qdev_pass_gpios()

[Peter Crosthwaite]

On Tue, Aug 12, 2014 at 8:55 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 12.08.14 12:48, Peter Crosthwaite wrote:
>>
>> On Tue, Aug 12, 2014 at 7:24 PM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 04.08.14 03:58, Peter Crosthwaite wrote:
>>>>
>>>> Allows a container to take ownership of GPIOs in a contained
>>>> device and automatically connect them as GPIOs to the container.
>>>>
>>>> This prepares for deprecation of the SYSBUS IRQ functionality, which
>>>> has this feature. We push it up to the device level instead of sysbus
>>>> level. There's nothing sysbus specific about passing GPIOs to
>>>> containers so its a legitimate device-level generic feature.
>>>>
>>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>> ---
>>>>
>>>>    hw/core/qdev.c         | 28 ++++++++++++++++++++++++++++
>>>>    include/hw/qdev-core.h |  3 +++
>>>>    2 files changed, 31 insertions(+)
>>>>
>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>> index bf2c227..708363f 100644
>>>> --- a/hw/core/qdev.c
>>>> +++ b/hw/core/qdev.c
>>>> @@ -440,6 +440,34 @@ void qdev_connect_gpio_out(DeviceState * dev, int
>>>> n,
>>>> qemu_irq pin)
>>>>        qdev_connect_gpio_out_named(dev, NULL, n, pin);
>>>>    }
>>>>    +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
>>>> +                     const char *name)
>>>> +{
>>>> +    int i;
>>>> +    NamedGPIOList *ngl = qdev_get_named_gpio_list(dev, name);
>>>> +
>>>> +    for (i = 0; i < ngl->num_in; i++) {
>>>> +        char *propname = g_strdup_printf("%s[%d]",
>>>> +                                         ngl->name ? ngl->name :
>>>> +                                                   "unnamed-gpio-in",
>>>
>>>
>>> Really just a minor nit, but I think the code flow would look a lot nicer
>>> if
>>> you did the name check in a separate variable.
>>>
>>>    const char *name = ngl->name ? ngl->name : "unnamed-gpio-in";
>>

Just done it this way. Although have used "nm" instead of "name"
instead as "name" is already used.

>> I think I may even go an extra step and get it macroified. How about:
>>
>> #define QDEV_GPIO_IN_NAME(a) ((a) ? (a) : "unnamed-gpio-io")
>>
>> and then you can continue to use it inline without extra variables or
>> nasty "?:"?
>
>
> The variable will get optimized out, so for the sake of readability I would
> still vote to have it around. Whether you declare it via a macro or with an
> a ? a : b macro I don't really care much about :).
>
> In fact, it might make more sense to literally have the typical
>
>   (a ? a : b)
>
> flow macroified in a generic place somewhere.
>

Hmm not sure what this wins though? inline "? :" is concise and self
documenting IMO. You just need to not stuff-up the "()"s (which is
admittedly quite easy to do.

Anyways, it's a patch for another day.

Regards,
Peter

Re: [Qemu-devel] [PATCH v1 13/16] qdev: gpio: Define qdev_pass_gpios()

[Alexander Graf]

On 15.08.14 07:11, Peter Crosthwaite wrote:
> On Tue, Aug 12, 2014 at 8:55 PM, Alexander Graf <agraf@suse.de> wrote:
>> On 12.08.14 12:48, Peter Crosthwaite wrote:
>>> On Tue, Aug 12, 2014 at 7:24 PM, Alexander Graf <agraf@suse.de> wrote:
>>>> On 04.08.14 03:58, Peter Crosthwaite wrote:
>>>>> Allows a container to take ownership of GPIOs in a contained
>>>>> device and automatically connect them as GPIOs to the container.
>>>>>
>>>>> This prepares for deprecation of the SYSBUS IRQ functionality, which
>>>>> has this feature. We push it up to the device level instead of sysbus
>>>>> level. There's nothing sysbus specific about passing GPIOs to
>>>>> containers so its a legitimate device-level generic feature.
>>>>>
>>>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>>> ---
>>>>>
>>>>>     hw/core/qdev.c         | 28 ++++++++++++++++++++++++++++
>>>>>     include/hw/qdev-core.h |  3 +++
>>>>>     2 files changed, 31 insertions(+)
>>>>>
>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>>> index bf2c227..708363f 100644
>>>>> --- a/hw/core/qdev.c
>>>>> +++ b/hw/core/qdev.c
>>>>> @@ -440,6 +440,34 @@ void qdev_connect_gpio_out(DeviceState * dev, int
>>>>> n,
>>>>> qemu_irq pin)
>>>>>         qdev_connect_gpio_out_named(dev, NULL, n, pin);
>>>>>     }
>>>>>     +void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
>>>>> +                     const char *name)
>>>>> +{
>>>>> +    int i;
>>>>> +    NamedGPIOList *ngl = qdev_get_named_gpio_list(dev, name);
>>>>> +
>>>>> +    for (i = 0; i < ngl->num_in; i++) {
>>>>> +        char *propname = g_strdup_printf("%s[%d]",
>>>>> +                                         ngl->name ? ngl->name :
>>>>> +                                                   "unnamed-gpio-in",
>>>>
>>>> Really just a minor nit, but I think the code flow would look a lot nicer
>>>> if
>>>> you did the name check in a separate variable.
>>>>
>>>>     const char *name = ngl->name ? ngl->name : "unnamed-gpio-in";
> Just done it this way. Although have used "nm" instead of "name"
> instead as "name" is already used.
>
>>> I think I may even go an extra step and get it macroified. How about:
>>>
>>> #define QDEV_GPIO_IN_NAME(a) ((a) ? (a) : "unnamed-gpio-io")
>>>
>>> and then you can continue to use it inline without extra variables or
>>> nasty "?:"?
>>
>> The variable will get optimized out, so for the sake of readability I would
>> still vote to have it around. Whether you declare it via a macro or with an
>> a ? a : b macro I don't really care much about :).
>>
>> In fact, it might make more sense to literally have the typical
>>
>>    (a ? a : b)
>>
>> flow macroified in a generic place somewhere.
>>
> Hmm not sure what this wins though? inline "? :" is concise and self
> documenting IMO. You just need to not stuff-up the "()"s (which is
> admittedly quite easy to do.
>
> Anyways, it's a patch for another day.

Yeah, probably not a big win really :).


Alex

[Qemu-devel] [PATCH v1 14/16] ssi: xilinx_spi: Initialise CS GPIOs as NULL

[Peter Crosthwaite]

To properly indicate they are unconnected.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/ssi/xilinx_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c
index 207f47a..620573c 100644
--- a/hw/ssi/xilinx_spi.c
+++ b/hw/ssi/xilinx_spi.c
@@ -329,7 +329,7 @@ static int xilinx_spi_init(SysBusDevice *sbd)
     s->spi = ssi_create_bus(dev, "spi");
 
     sysbus_init_irq(sbd, &s->irq);
-    s->cs_lines = g_new(qemu_irq, s->num_cs);
+    s->cs_lines = g_new0(qemu_irq, s->num_cs);
     ssi_auto_connect_slaves(dev, s->cs_lines, s->spi);
     for (i = 0; i < s->num_cs; ++i) {
         sysbus_init_irq(sbd, &s->cs_lines[i]);
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH v1 15/16] ppc: convert g_new(qemu_irq usages to g_new0

[Peter Crosthwaite]

To indicate the IRQs are initially disconnected.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/intc/openpic.c     | 2 +-
 hw/ppc/e500.c         | 2 +-
 hw/ppc/mac_newworld.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 028529e..7d1f3b9 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -1627,7 +1627,7 @@ static void openpic_realize(DeviceState *dev, Error **errp)
     }
 
     for (i = 0; i < opp->nb_cpus; i++) {
-        opp->dst[i].irqs = g_new(qemu_irq, OPENPIC_OUTPUT_NB);
+        opp->dst[i].irqs = g_new0(qemu_irq, OPENPIC_OUTPUT_NB);
         for (j = 0; j < OPENPIC_OUTPUT_NB; j++) {
             sysbus_init_irq(d, &opp->dst[i].irqs[j]);
         }
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 1a5b30d..16c85ef 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -583,7 +583,7 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
     SysBusDevice *s;
     int i;
 
-    mpic = g_new(qemu_irq, 256);
+    mpic = g_new0(qemu_irq, 256);
 
     if (kvm_enabled()) {
         QemuOpts *machine_opts = qemu_get_machine_opts();
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index f5bccd2..1ec4bb4 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -346,7 +346,7 @@ static void ppc_core99_init(MachineState *machine)
         }
     }
 
-    pic = g_new(qemu_irq, 64);
+    pic = g_new0(qemu_irq, 64);
 
     dev = qdev_create(NULL, TYPE_OPENPIC);
     qdev_prop_set_uint32(dev, "model", OPENPIC_MODEL_RAVEN);
-- 
2.0.1.1.gfbfc394

Re: [Qemu-devel] [PATCH v1 15/16] ppc: convert g_new(qemu_irq usages to g_new0

[Alexander Graf]

On 04.08.14 03:59, Peter Crosthwaite wrote:
> To indicate the IRQs are initially disconnected.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

I suppose you want to convert this one too?

   hw/intc/i8259.c:475:    irq_set = g_malloc(ISA_NUM_IRQS * 
sizeof(qemu_irq));


Alex

Re: [Qemu-devel] [PATCH v1 15/16] ppc: convert g_new(qemu_irq usages to g_new0

[Peter Crosthwaite]

On Tue, Aug 12, 2014 at 7:26 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 04.08.14 03:59, Peter Crosthwaite wrote:
>>
>> To indicate the IRQs are initially disconnected.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
>
> I suppose you want to convert this one too?
>

Yes. Thanks.

Regards,
Peter

>   hw/intc/i8259.c:475:    irq_set = g_malloc(ISA_NUM_IRQS *
> sizeof(qemu_irq));
>
>
> Alex
>
>

Re: [Qemu-devel] [PATCH v1 15/16] ppc: convert g_new(qemu_irq usages to g_new0

[Peter Crosthwaite]

On Tue, Aug 12, 2014 at 7:26 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 04.08.14 03:59, Peter Crosthwaite wrote:
>>
>> To indicate the IRQs are initially disconnected.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
>
> I suppose you want to convert this one too?
>
>   hw/intc/i8259.c:475:    irq_set = g_malloc(ISA_NUM_IRQS *
> sizeof(qemu_irq));
>

So I have figured out a catch-all for this problem. I've set it up
such that when registering the link qdev will memset the pointer to 0.
This patch (along with your new case) is still legit cleanup though
and can go via trivial seperately.

Dropped completely in V2.

Regards,
Peter

>
> Alex
>
>

[Qemu-devel] [PATCH v1 16/16] sysbus: Use TYPE_DEVICE GPIO functionality

[Peter Crosthwaite]

Re-implement the Sysbus GPIOs to use the existing TYPE_DEVICE
GPIO named framework. A constant string name is chosen to avoid
conflicts with existing unnamed GPIOs.

This unifies GPIOs are IRQs for sysbus devices and allows removal
of all Sysbus state for GPIOs.

Any existing and future-added functionality for GPIOs is now
also available for sysbus IRQs.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Changed since v1:
Named each IRQ individually.

 hw/core/sysbus.c    | 20 +++-----------------
 include/hw/sysbus.h |  6 +++---
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index f4e760d..192a729 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -41,11 +41,7 @@ static const TypeInfo system_bus_info = {
 
 void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
 {
-    assert(n >= 0 && n < dev->num_irq);
-    dev->irqs[n] = NULL;
-    if (dev->irqp[n]) {
-        *dev->irqp[n] = irq;
-    }
+    qdev_connect_gpio_out_named(DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ, n, irq);
 }
 
 static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
@@ -89,22 +85,13 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
 /* Request an IRQ source.  The actual IRQ object may be populated later.  */
 void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
 {
-    int n;
-
-    assert(dev->num_irq < QDEV_MAX_IRQ);
-    n = dev->num_irq++;
-    dev->irqp[n] = p;
+    qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1);
 }
 
 /* Pass IRQs from a target device.  */
 void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target)
 {
-    int i;
-    assert(dev->num_irq == 0);
-    dev->num_irq = target->num_irq;
-    for (i = 0; i < dev->num_irq; i++) {
-        dev->irqp[i] = target->irqp[i];
-    }
+    qdev_pass_gpios(DEVICE(target), DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ);
 }
 
 void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
@@ -210,7 +197,6 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
     hwaddr size;
     int i;
 
-    monitor_printf(mon, "%*sirq %d\n", indent, "", s->num_irq);
     for (i = 0; i < s->num_mmio; i++) {
         size = memory_region_size(s->mmio[i].memory);
         monitor_printf(mon, "%*smmio " TARGET_FMT_plx "/" TARGET_FMT_plx "\n",
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index f5aaa05..a0baefc 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -8,7 +8,6 @@
 
 #define QDEV_MAX_MMIO 32
 #define QDEV_MAX_PIO 32
-#define QDEV_MAX_IRQ 512
 
 #define TYPE_SYSTEM_BUS "System"
 #define SYSTEM_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
@@ -33,6 +32,9 @@ typedef struct SysBusDevice SysBusDevice;
  * SysBusDeviceClass is not overriding #DeviceClass.realize, so derived
  * classes overriding it are not required to invoke its implementation.
  */
+
+#define SYSBUS_DEVICE_GPIO_IRQ "sysbus-irq"
+
 typedef struct SysBusDeviceClass {
     /*< private >*/
     DeviceClass parent_class;
@@ -47,8 +49,6 @@ struct SysBusDevice {
     /*< public >*/
 
     int num_irq;
-    qemu_irq irqs[QDEV_MAX_IRQ];
-    qemu_irq *irqp[QDEV_MAX_IRQ];
     int num_mmio;
     struct {
         hwaddr addr;
-- 
2.0.1.1.gfbfc394

Re: [Qemu-devel] [PATCH v1 00/16] GPIO/IRQ QOMification: Phase 2 - Getting rid of SYSBUS IRQs

[Peter Crosthwaite]

Ping!

On Mon, Aug 4, 2014 at 11:51 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Hi All,
>
> So phase one was the QOMification of qemu_irq. This is the next step.
> We start to setup GPIOs as proper QOM objects. Inputs are child objects
> of their device. Outputs are settable Links and connection is made
> via proper setting of a QOM link.
>
> We then cleanup Sysbus to simply re-use device level GPIOs and get rid
> of it's special IRQ handling code.
>
> Depends of my pending QOM array property stuff (the "[*]" series):
>
> https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg04116.html
>
>
> Peter Crosthwaite (16):
>   qdev: gpio: Don't allow name share between I and O
>   qdev: gpio: Register GPIO inputs as child objects
>   qdev: gpio: Register GPIO outputs as QOM links
>   qmp: qstring: Handle NULL strings
>   qom: Allow clearing of a Link property
>   qom: Demote already-has-a-parent to a regular error
>   qdev: gpio: Re-impement qdev_connect_gpio QOM style
>   qdev: gpio: Add API for intercepting an IRQ
>   qtest/irq: Rework IRQ interception
>   irq: Remove qemu_irq_intercept_out
>   qdev: gpio: delete NamedGPIOList::out
>   qdev: gpio: Remove qdev_init_gpio_out x1 restriction
>   qdev: gpio: Define qdev_pass_gpios()
>   ssi: xilinx_spi: Initialise CS GPIOs as NULL
>   ppc: convert g_new(qemu_irq usages to g_new0
>   sysbus: Use TYPE_DEVICE GPIO functionality
>
>  hw/core/irq.c          |  8 +----
>  hw/core/qdev.c         | 89 ++++++++++++++++++++++++++++++++++++++++++++++----
>  hw/core/sysbus.c       | 20 ++----------
>  hw/intc/openpic.c      |  2 +-
>  hw/ppc/e500.c          |  2 +-
>  hw/ppc/mac_newworld.c  |  2 +-
>  hw/ssi/xilinx_spi.c    |  2 +-
>  include/hw/irq.h       |  1 -
>  include/hw/qdev-core.h |  6 +++-
>  include/hw/sysbus.h    |  6 ++--
>  qobject/qstring.c      | 12 ++++---
>  qom/object.c           | 10 ++++--
>  qtest.c                | 11 ++++---
>  13 files changed, 120 insertions(+), 51 deletions(-)
>
> --
> 2.0.1.1.gfbfc394
>

Re: [Qemu-devel] [PATCH v1 00/16] GPIO/IRQ QOMification: Phase 2 - Getting rid of SYSBUS IRQs

[Alexander Graf]

On 04.08.14 03:51, Peter Crosthwaite wrote:
> Hi All,
>
> So phase one was the QOMification of qemu_irq. This is the next step.
> We start to setup GPIOs as proper QOM objects. Inputs are child objects
> of their device. Outputs are settable Links and connection is made
> via proper setting of a QOM link.
>
> We then cleanup Sysbus to simply re-use device level GPIOs and get rid
> of it's special IRQ handling code.
>
> Depends of my pending QOM array property stuff (the "[*]" series):
>
> https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg04116.html

Overall looks like an improvement to me :).


Alex