diff mbox

[RFC,02/11] qemu-clk: allow to attach a clock to a device

Message ID 1465835259-21449-3-git-send-email-fred.konrad@greensocs.com
State New
Headers show

Commit Message

fred.konrad@greensocs.com June 13, 2016, 4:27 p.m. UTC 
From: KONRAD Frederic <fred.konrad@greensocs.com>

This allows to attach a clock to a DeviceState.
Contrary to gpios, the clock pins are not contained in the DeviceState but
with the child property so they can appears in the qom-tree.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 include/qemu/qemu-clock.h | 24 +++++++++++++++++++++++-
 qemu-clock.c              | 22 ++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

Comments

Alistair Francis June 29, 2016, 12:15 a.m. UTC | #1 
On Mon, Jun 13, 2016 at 9:27 AM,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This allows to attach a clock to a DeviceState.
> Contrary to gpios, the clock pins are not contained in the DeviceState but
> with the child property so they can appears in the qom-tree.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  include/qemu/qemu-clock.h | 24 +++++++++++++++++++++++-
>  qemu-clock.c              | 22 ++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h
> index e7acd68..a2ba105 100644
> --- a/include/qemu/qemu-clock.h
> +++ b/include/qemu/qemu-clock.h
> @@ -33,8 +33,30 @@
>  typedef struct qemu_clk {
>      /*< private >*/
>      Object parent_obj;
> +    char *name;            /* name of this clock in the device. */
>  } *qemu_clk;
>
> -#endif /* QEMU_CLOCK_H */
> +/**
> + * qemu_clk_attach_to_device:
> + * @d: the device on which the clock need to be attached.
> + * @clk: the clock which need to be attached.
> + * @name: the name of the clock can't be NULL.
> + *
> + * Attach @clk named @name to the device @d.
> + *
> + */
> +void qemu_clk_attach_to_device(DeviceState *d, qemu_clk clk,

dev instead of just d

> +                               const char *name);
>
> +/**
> + * qemu_clk_get_pin:
> + * @d: the device which contain the clock.
> + * @name: the name of the clock.
> + *
> + * Get the clock named @name located in the device @d, or NULL if not found.
> + *
> + * Returns the clock named @name contained in @d.
> + */
> +qemu_clk qemu_clk_get_pin(DeviceState *d, const char *name);
>
> +#endif /* QEMU_CLOCK_H */
> diff --git a/qemu-clock.c b/qemu-clock.c
> index 4a47fb4..81f2852 100644
> --- a/qemu-clock.c
> +++ b/qemu-clock.c
> @@ -23,6 +23,7 @@
>
>  #include "qemu/qemu-clock.h"
>  #include "hw/hw.h"
> +#include "qapi/error.h"
>
>  /* #define DEBUG_QEMU_CLOCK */
>
> @@ -33,6 +34,27 @@ do { printf("qemu-clock: " fmt , ## __VA_ARGS__); } while (0)
>  #define DPRINTF(fmt, ...) do { } while (0)
>  #endif
>
> +void qemu_clk_attach_to_device(DeviceState *d, qemu_clk clk, const char *name)
> +{
> +    assert(name);
> +    assert(!clk->name);
> +    object_property_add_child(OBJECT(d), name, OBJECT(clk), &error_abort);
> +    clk->name = g_strdup(name);
> +}
> +
> +qemu_clk qemu_clk_get_pin(DeviceState *d, const char *name)
> +{
> +    gchar *path = NULL;
> +    Object *clk;
> +    bool ambiguous;
> +
> +    path = g_strdup_printf("%s/%s", object_get_canonical_path(OBJECT(d)),
> +                           name);
> +    clk = object_resolve_path(path, &ambiguous);

Should ambiguous be passed back to the caller?

> +    g_free(path);
> +    return QEMU_CLOCK(clk);

Shouldn't you check to see if you got something valid before casting?

Thanks,

Alistair

> +}
> +
>  static const TypeInfo qemu_clk_info = {
>      .name          = TYPE_CLOCK,
>      .parent        = TYPE_OBJECT,
> --
> 2.5.5
>
>
fred.konrad@greensocs.com Aug. 2, 2016, 7:47 a.m. UTC | #2 
Le 29/06/2016 à 02:15, Alistair Francis a écrit :
> On Mon, Jun 13, 2016 at 9:27 AM,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This allows to attach a clock to a DeviceState.
>> Contrary to gpios, the clock pins are not contained in the DeviceState but
>> with the child property so they can appears in the qom-tree.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>   include/qemu/qemu-clock.h | 24 +++++++++++++++++++++++-
>>   qemu-clock.c              | 22 ++++++++++++++++++++++
>>   2 files changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h
>> index e7acd68..a2ba105 100644
>> --- a/include/qemu/qemu-clock.h
>> +++ b/include/qemu/qemu-clock.h
>> @@ -33,8 +33,30 @@
>>   typedef struct qemu_clk {
>>       /*< private >*/
>>       Object parent_obj;
>> +    char *name;            /* name of this clock in the device. */
>>   } *qemu_clk;
>>
>> -#endif /* QEMU_CLOCK_H */
>> +/**
>> + * qemu_clk_attach_to_device:
>> + * @d: the device on which the clock need to be attached.
>> + * @clk: the clock which need to be attached.
>> + * @name: the name of the clock can't be NULL.
>> + *
>> + * Attach @clk named @name to the device @d.
>> + *
>> + */
>> +void qemu_clk_attach_to_device(DeviceState *d, qemu_clk clk,
> dev instead of just d
>
>> +                               const char *name);
>>
>> +/**
>> + * qemu_clk_get_pin:
>> + * @d: the device which contain the clock.
>> + * @name: the name of the clock.
>> + *
>> + * Get the clock named @name located in the device @d, or NULL if not found.
>> + *
>> + * Returns the clock named @name contained in @d.
>> + */
>> +qemu_clk qemu_clk_get_pin(DeviceState *d, const char *name);
>>
>> +#endif /* QEMU_CLOCK_H */
>> diff --git a/qemu-clock.c b/qemu-clock.c
>> index 4a47fb4..81f2852 100644
>> --- a/qemu-clock.c
>> +++ b/qemu-clock.c
>> @@ -23,6 +23,7 @@
>>
>>   #include "qemu/qemu-clock.h"
>>   #include "hw/hw.h"
>> +#include "qapi/error.h"
>>
>>   /* #define DEBUG_QEMU_CLOCK */
>>
>> @@ -33,6 +34,27 @@ do { printf("qemu-clock: " fmt , ## __VA_ARGS__); } while (0)
>>   #define DPRINTF(fmt, ...) do { } while (0)
>>   #endif
>>
>> +void qemu_clk_attach_to_device(DeviceState *d, qemu_clk clk, const char *name)
>> +{
>> +    assert(name);
>> +    assert(!clk->name);
>> +    object_property_add_child(OBJECT(d), name, OBJECT(clk), &error_abort);
>> +    clk->name = g_strdup(name);
>> +}
>> +
>> +qemu_clk qemu_clk_get_pin(DeviceState *d, const char *name)
>> +{
>> +    gchar *path = NULL;
>> +    Object *clk;
>> +    bool ambiguous;
>> +
>> +    path = g_strdup_printf("%s/%s", object_get_canonical_path(OBJECT(d)),
>> +                           name);
>> +    clk = object_resolve_path(path, &ambiguous);
> Should ambiguous be passed back to the caller?

Up to you, I don't see the use case in the machine where we want to get 
the clock?

>
>> +    g_free(path);
>> +    return QEMU_CLOCK(clk);
> Shouldn't you check to see if you got something valid before casting?

Yes true, I was relying on the fact that QEMU_CLOCK is in the end:
object_dynamic_cast_assert(..) which according to the doc:

  * If an invalid object is passed to this function, a run time assert 
will be
  * generated.

but it seems not to be the case in reality if CONFIG_QOM_CAST_DEBUG is 
disabled:

Object *object_dynamic_cast_assert(Object *obj, const char *typename,
                                    const char *file, int line, const 
char *func)
{
     trace_object_dynamic_cast_assert(obj ? obj->class->type->name : 
"(null)",
                                      typename, file, line, func);

#ifdef CONFIG_QOM_CAST_DEBUG
     int i;
     Object *inst;

     for (i = 0; obj && i < OBJECT_CLASS_CAST_CACHE; i++) {
         if (obj->class->object_cast_cache[i] == typename) {
             goto out;
         }
     }

     inst = object_dynamic_cast(obj, typename);

     if (!inst && obj) {
         fprintf(stderr, "%s:%d:%s: Object %p is not an instance of type 
%s\n",
                 file, line, func, obj, typename);
         abort();
     }

     assert(obj == inst);

     if (obj && obj == inst) {
         for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
             obj->class->object_cast_cache[i - 1] =
                     obj->class->object_cast_cache[i];
         }
         obj->class->object_cast_cache[i - 1] = typename;
     }

out:
#endif
     return obj;
}

Is that normal?

Thanks,
Fred

>
> Thanks,
>
> Alistair
>
>> +}
>> +
>>   static const TypeInfo qemu_clk_info = {
>>       .name          = TYPE_CLOCK,
>>       .parent        = TYPE_OBJECT,
>> --
>> 2.5.5
>>
>>
Alistair Francis Aug. 4, 2016, 12:26 a.m. UTC | #3 
On Tue, Aug 2, 2016 at 12:47 AM, KONRAD Frederic
<fred.konrad@greensocs.com> wrote:
>
>
> Le 29/06/2016 à 02:15, Alistair Francis a écrit :
>>
>> On Mon, Jun 13, 2016 at 9:27 AM,  <fred.konrad@greensocs.com> wrote:
>>>
>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>
>>> This allows to attach a clock to a DeviceState.
>>> Contrary to gpios, the clock pins are not contained in the DeviceState
>>> but
>>> with the child property so they can appears in the qom-tree.
>>>
>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>> ---
>>>   include/qemu/qemu-clock.h | 24 +++++++++++++++++++++++-
>>>   qemu-clock.c              | 22 ++++++++++++++++++++++
>>>   2 files changed, 45 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h
>>> index e7acd68..a2ba105 100644
>>> --- a/include/qemu/qemu-clock.h
>>> +++ b/include/qemu/qemu-clock.h
>>> @@ -33,8 +33,30 @@
>>>   typedef struct qemu_clk {
>>>       /*< private >*/
>>>       Object parent_obj;
>>> +    char *name;            /* name of this clock in the device. */
>>>   } *qemu_clk;
>>>
>>> -#endif /* QEMU_CLOCK_H */
>>> +/**
>>> + * qemu_clk_attach_to_device:
>>> + * @d: the device on which the clock need to be attached.
>>> + * @clk: the clock which need to be attached.
>>> + * @name: the name of the clock can't be NULL.
>>> + *
>>> + * Attach @clk named @name to the device @d.
>>> + *
>>> + */
>>> +void qemu_clk_attach_to_device(DeviceState *d, qemu_clk clk,
>>
>> dev instead of just d
>>
>>> +                               const char *name);
>>>
>>> +/**
>>> + * qemu_clk_get_pin:
>>> + * @d: the device which contain the clock.
>>> + * @name: the name of the clock.
>>> + *
>>> + * Get the clock named @name located in the device @d, or NULL if not
>>> found.
>>> + *
>>> + * Returns the clock named @name contained in @d.
>>> + */
>>> +qemu_clk qemu_clk_get_pin(DeviceState *d, const char *name);
>>>
>>> +#endif /* QEMU_CLOCK_H */
>>> diff --git a/qemu-clock.c b/qemu-clock.c
>>> index 4a47fb4..81f2852 100644
>>> --- a/qemu-clock.c
>>> +++ b/qemu-clock.c
>>> @@ -23,6 +23,7 @@
>>>
>>>   #include "qemu/qemu-clock.h"
>>>   #include "hw/hw.h"
>>> +#include "qapi/error.h"
>>>
>>>   /* #define DEBUG_QEMU_CLOCK */
>>>
>>> @@ -33,6 +34,27 @@ do { printf("qemu-clock: " fmt , ## __VA_ARGS__); }
>>> while (0)
>>>   #define DPRINTF(fmt, ...) do { } while (0)
>>>   #endif
>>>
>>> +void qemu_clk_attach_to_device(DeviceState *d, qemu_clk clk, const char
>>> *name)
>>> +{
>>> +    assert(name);
>>> +    assert(!clk->name);
>>> +    object_property_add_child(OBJECT(d), name, OBJECT(clk),
>>> &error_abort);
>>> +    clk->name = g_strdup(name);
>>> +}
>>> +
>>> +qemu_clk qemu_clk_get_pin(DeviceState *d, const char *name)
>>> +{
>>> +    gchar *path = NULL;
>>> +    Object *clk;
>>> +    bool ambiguous;
>>> +
>>> +    path = g_strdup_printf("%s/%s",
>>> object_get_canonical_path(OBJECT(d)),
>>> +                           name);
>>> +    clk = object_resolve_path(path, &ambiguous);
>>
>> Should ambiguous be passed back to the caller?
>
>
> Up to you, I don't see the use case in the machine where we want to get the
> clock?

Can't you just set it as NULL then.

>
>>
>>> +    g_free(path);
>>> +    return QEMU_CLOCK(clk);
>>
>> Shouldn't you check to see if you got something valid before casting?
>
>
> Yes true, I was relying on the fact that QEMU_CLOCK is in the end:
> object_dynamic_cast_assert(..) which according to the doc:
>
>  * If an invalid object is passed to this function, a run time assert will
> be
>  * generated.
>
> but it seems not to be the case in reality if CONFIG_QOM_CAST_DEBUG is
> disabled:
>
> Object *object_dynamic_cast_assert(Object *obj, const char *typename,
>                                    const char *file, int line, const char
> *func)
> {
>     trace_object_dynamic_cast_assert(obj ? obj->class->type->name :
> "(null)",
>                                      typename, file, line, func);
>
> #ifdef CONFIG_QOM_CAST_DEBUG
>     int i;
>     Object *inst;
>
>     for (i = 0; obj && i < OBJECT_CLASS_CAST_CACHE; i++) {
>         if (obj->class->object_cast_cache[i] == typename) {
>             goto out;
>         }
>     }
>
>     inst = object_dynamic_cast(obj, typename);
>
>     if (!inst && obj) {
>         fprintf(stderr, "%s:%d:%s: Object %p is not an instance of type
> %s\n",
>                 file, line, func, obj, typename);
>         abort();
>     }
>
>     assert(obj == inst);
>
>     if (obj && obj == inst) {
>         for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
>             obj->class->object_cast_cache[i - 1] =
>                     obj->class->object_cast_cache[i];
>         }
>         obj->class->object_cast_cache[i - 1] = typename;
>     }
>
> out:
> #endif
>     return obj;
> }
>
> Is that normal?

I'm not sure to be honest. I never expected the cast to do any
checking. Someone else probably knows the history here.

Thanks,

Alistair

>
> Thanks,
> Fred
>
>
>>
>> Thanks,
>>
>> Alistair
>>
>>> +}
>>> +
>>>   static const TypeInfo qemu_clk_info = {
>>>       .name          = TYPE_CLOCK,
>>>       .parent        = TYPE_OBJECT,
>>> --
>>> 2.5.5
>>>
>>>
>
>
diff mbox

Patch

diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h
index e7acd68..a2ba105 100644
--- a/include/qemu/qemu-clock.h
+++ b/include/qemu/qemu-clock.h
@@ -33,8 +33,30 @@ 
 typedef struct qemu_clk {
     /*< private >*/
     Object parent_obj;
+    char *name;            /* name of this clock in the device. */
 } *qemu_clk;
 
-#endif /* QEMU_CLOCK_H */
+/**
+ * qemu_clk_attach_to_device:
+ * @d: the device on which the clock need to be attached.
+ * @clk: the clock which need to be attached.
+ * @name: the name of the clock can't be NULL.
+ *
+ * Attach @clk named @name to the device @d.
+ *
+ */
+void qemu_clk_attach_to_device(DeviceState *d, qemu_clk clk,
+                               const char *name);
 
+/**
+ * qemu_clk_get_pin:
+ * @d: the device which contain the clock.
+ * @name: the name of the clock.
+ *
+ * Get the clock named @name located in the device @d, or NULL if not found.
+ *
+ * Returns the clock named @name contained in @d.
+ */
+qemu_clk qemu_clk_get_pin(DeviceState *d, const char *name);
 
+#endif /* QEMU_CLOCK_H */
diff --git a/qemu-clock.c b/qemu-clock.c
index 4a47fb4..81f2852 100644
--- a/qemu-clock.c
+++ b/qemu-clock.c
@@ -23,6 +23,7 @@ 
 
 #include "qemu/qemu-clock.h"
 #include "hw/hw.h"
+#include "qapi/error.h"
 
 /* #define DEBUG_QEMU_CLOCK */
 
@@ -33,6 +34,27 @@  do { printf("qemu-clock: " fmt , ## __VA_ARGS__); } while (0)
 #define DPRINTF(fmt, ...) do { } while (0)
 #endif
 
+void qemu_clk_attach_to_device(DeviceState *d, qemu_clk clk, const char *name)
+{
+    assert(name);
+    assert(!clk->name);
+    object_property_add_child(OBJECT(d), name, OBJECT(clk), &error_abort);
+    clk->name = g_strdup(name);
+}
+
+qemu_clk qemu_clk_get_pin(DeviceState *d, const char *name)
+{
+    gchar *path = NULL;
+    Object *clk;
+    bool ambiguous;
+
+    path = g_strdup_printf("%s/%s", object_get_canonical_path(OBJECT(d)),
+                           name);
+    clk = object_resolve_path(path, &ambiguous);
+    g_free(path);
+    return QEMU_CLOCK(clk);
+}
+
 static const TypeInfo qemu_clk_info = {
     .name          = TYPE_CLOCK,
     .parent        = TYPE_OBJECT,