[V1,03/10] qemu-clk: allow to bind two clocks together

Commit Message

KONRAD Frédéric Oct. 5, 2016, 10:10 p.m. UTC

From: KONRAD Frederic <[e-mail]>

This introduces the clock binding and the update part.
When the qemu_clk_rate_update(qemu_clk, int) function is called:
  * The clock callback is called on the qemu_clk so it can change the rate.
  * The qemu_clk_rate_update function is called on all the driven clock.

Signed-off-by: KONRAD Frederic <[e-mail]>
---
 include/qemu/qemu-clock.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++
 qemu-clock.c              | 56 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)

Comments

Peter Maydell Oct. 17, 2016, 6:19 p.m. UTC | #1

On 5 October 2016 at 23:10,  <[e-mail]> wrote:
> From: KONRAD Frederic <[e-mail]>
>
> This introduces the clock binding and the update part.
> When the qemu_clk_rate_update(qemu_clk, int) function is called:
>   * The clock callback is called on the qemu_clk so it can change the rate.
>   * The qemu_clk_rate_update function is called on all the driven clock.
>
> Signed-off-by: KONRAD Frederic <[e-mail]>
> ---
>  include/qemu/qemu-clock.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-clock.c              | 56 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 122 insertions(+)
>
> diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h
> index 1d56a2e..d575566 100644
> --- a/include/qemu/qemu-clock.h
> +++ b/include/qemu/qemu-clock.h
> @@ -27,15 +27,29 @@
>  #include "qemu/osdep.h"
>  #include "qom/object.h"
>
> +typedef uint64_t (*qemu_clk_on_rate_update_cb)(void *opaque, uint64_t rate);

I think it's more readable to have the typedef define
the function type, not the pointer-to-function type. (See for
instance CPReadFn, CPWriteFn in target-arm/cpu.h.) That way
your function pointers in structs and so on have type
"MyFnType *fn;" and are more obviously pointers.

(Also QEMU style says camelcase for types. QEMUClkRateUpdateCallback?)

> +
>  #define TYPE_CLOCK "qemu-clk"
>  #define QEMU_CLOCK(obj) OBJECT_CHECK(struct qemu_clk, (obj), TYPE_CLOCK)
>
> +typedef struct ClkList ClkList;
> +
>  typedef struct qemu_clk {
>      /*< private >*/
>      Object parent_obj;
>      char *name;            /* name of this clock in the device. */
> +    uint64_t in_rate;      /* rate of the clock which drive this pin. */
> +    uint64_t out_rate;     /* rate of this clock pin. */
> +    void *opaque;
> +    qemu_clk_on_rate_update_cb cb;
> +    QLIST_HEAD(, ClkList) bound;
>  } *qemu_clk;
>
> +struct ClkList {
> +    qemu_clk clk;
> +    QLIST_ENTRY(ClkList) node;
> +};
> +
>  /**
>   * qemu_clk_attach_to_device:
>   * @dev: the device on which the clock need to be attached.
> @@ -59,4 +73,56 @@ void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk,
>   */
>  qemu_clk qemu_clk_get_pin(DeviceState *dev, const char *name);
>
> +/**
> + * qemu_clk_bind_clock:
> + * @out: the clock output.
> + * @in: the clock input.
> + *
> + * Connect the clock together. This is unidirectional so a
> + * qemu_clk_update_rate will go from @out to @in.
> + *
> + */
> +void qemu_clk_bind_clock(qemu_clk out, qemu_clk in);

Hang on, I thought that passing a clock to
qemu_clk_attach_to_device() was going to be the thing
that connected the clock up...

> +
> +/**
> + * qemu_clk_unbound:
> + * @out: the clock output.
> + * @in: the clock input.
> + *
> + * Disconnect the clocks if they were bound together.
> + *
> + */
> +void qemu_clk_unbind(qemu_clk out, qemu_clk in);

Function prototype and comment don't match...

> +
> +/**
> + * qemu_clk_update_rate:
> + * @clk: the clock to update.
> + * @rate: the new rate.
> + *
> + * Update the @clk to the new @rate.
> + *
> + */
> +void qemu_clk_update_rate(qemu_clk clk, uint64_t rate);

What units is this 'rate' in ?

> +
> +/**
> + * qemu_clk_refresh:
> + * @clk: the clock to be refreshed.
> + *
> + * If a model alters the topology of a clock tree, it must call this function
> + * to refresh the clock tree.
> + *
> + */
> +void qemu_clk_refresh(qemu_clk clk);

...for which clock in the tree does it have to call the function?
All of them? Any one of them at random?

> +
> +/**
> + * qemu_clk_set_callback:
> + * @clk: the clock where to set the callback.
> + * @cb: the callback to associate to the callback.
> + * @opaque: the opaque data passed to the calback.
> + *
> + */
> +void qemu_clk_set_callback(qemu_clk clk,
> +                           qemu_clk_on_rate_update_cb cb,
> +                           void *opaque);
> +
>  #endif /* QEMU_CLOCK_H */
> diff --git a/qemu-clock.c b/qemu-clock.c
> index 0ba6caf..541f615 100644
> --- a/qemu-clock.c
> +++ b/qemu-clock.c
> @@ -37,6 +37,62 @@
>      }                                                                        \
>  } while (0);
>
> +void qemu_clk_refresh(qemu_clk clk)
> +{
> +    qemu_clk_update_rate(clk, clk->in_rate);
> +}
> +
> +void qemu_clk_update_rate(qemu_clk clk, uint64_t rate)
> +{
> +    ClkList *child;
> +
> +    clk->in_rate = rate;
> +    clk->out_rate = rate;
> +
> +    if (clk->cb) {
> +        clk->out_rate = clk->cb(clk->opaque, rate);
> +    }
> +
> +    DPRINTF("%s output rate updated to %" PRIu64 "\n",
> +            object_get_canonical_path(OBJECT(clk)),
> +            clk->out_rate);
> +
> +    QLIST_FOREACH(child, &clk->bound, node) {
> +        qemu_clk_update_rate(child->clk, clk->out_rate);
> +    }
> +}
> +
> +void qemu_clk_bind_clock(qemu_clk out, qemu_clk in)
> +{
> +    ClkList *child;
> +
> +    child = g_malloc(sizeof(child));
> +    assert(child);
> +    child->clk = in;
> +    QLIST_INSERT_HEAD(&out->bound, child, node);
> +    qemu_clk_update_rate(in, out->out_rate);
> +}
> +
> +void qemu_clk_unbind(qemu_clk out, qemu_clk in)
> +{
> +    ClkList *child, *next;
> +
> +    QLIST_FOREACH_SAFE(child, &out->bound, node, next) {
> +        if (child->clk == in) {
> +            QLIST_REMOVE(child, node);
> +            g_free(child);
> +        }
> +    }
> +}
> +
> +void qemu_clk_set_callback(qemu_clk clk,
> +                           qemu_clk_on_rate_update_cb cb,
> +                           void *opaque)
> +{
> +    clk->cb = cb;
> +    clk->opaque = opaque;
> +}
> +
>  void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk,
>                                 const char *name)
>  {
> --
> 2.5.5
>


thanks
-- PMM

KONRAD Frédéric Oct. 17, 2016, 8:17 p.m. UTC | #2

Le 17/10/2016 à 20:19, Peter Maydell a écrit :
> On 5 October 2016 at 23:10,  <[e-mail]> wrote:
>> From: KONRAD Frederic <[e-mail]>
>>
>> This introduces the clock binding and the update part.
>> When the qemu_clk_rate_update(qemu_clk, int) function is called:
>>   * The clock callback is called on the qemu_clk so it can change the rate.
>>   * The qemu_clk_rate_update function is called on all the driven clock.
>>
>> Signed-off-by: KONRAD Frederic <[e-mail]>
>> ---
>>  include/qemu/qemu-clock.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-clock.c              | 56 ++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 122 insertions(+)
>>
>> diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h
>> index 1d56a2e..d575566 100644
>> --- a/include/qemu/qemu-clock.h
>> +++ b/include/qemu/qemu-clock.h
>> @@ -27,15 +27,29 @@
>>  #include "qemu/osdep.h"
>>  #include "qom/object.h"
>>
>> +typedef uint64_t (*qemu_clk_on_rate_update_cb)(void *opaque, uint64_t rate);
>
> I think it's more readable to have the typedef define
> the function type, not the pointer-to-function type. (See for
> instance CPReadFn, CPWriteFn in target-arm/cpu.h.) That way
> your function pointers in structs and so on have type
> "MyFnType *fn;" and are more obviously pointers.
>
> (Also QEMU style says camelcase for types. QEMUClkRateUpdateCallback?)

Ok I'll fix that!

>
>> +
>>  #define TYPE_CLOCK "qemu-clk"
>>  #define QEMU_CLOCK(obj) OBJECT_CHECK(struct qemu_clk, (obj), TYPE_CLOCK)
>>
>> +typedef struct ClkList ClkList;
>> +
>>  typedef struct qemu_clk {
>>      /*< private >*/
>>      Object parent_obj;
>>      char *name;            /* name of this clock in the device. */
>> +    uint64_t in_rate;      /* rate of the clock which drive this pin. */
>> +    uint64_t out_rate;     /* rate of this clock pin. */
>> +    void *opaque;
>> +    qemu_clk_on_rate_update_cb cb;
>> +    QLIST_HEAD(, ClkList) bound;
>>  } *qemu_clk;
>>
>> +struct ClkList {
>> +    qemu_clk clk;
>> +    QLIST_ENTRY(ClkList) node;
>> +};
>> +
>>  /**
>>   * qemu_clk_attach_to_device:
>>   * @dev: the device on which the clock need to be attached.
>> @@ -59,4 +73,56 @@ void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk,
>>   */
>>  qemu_clk qemu_clk_get_pin(DeviceState *dev, const char *name);
>>
>> +/**
>> + * qemu_clk_bind_clock:
>> + * @out: the clock output.
>> + * @in: the clock input.
>> + *
>> + * Connect the clock together. This is unidirectional so a
>> + * qemu_clk_update_rate will go from @out to @in.
>> + *
>> + */
>> +void qemu_clk_bind_clock(qemu_clk out, qemu_clk in);
>
> Hang on, I thought that passing a clock to
> qemu_clk_attach_to_device() was going to be the thing
> that connected the clock up...
>

qemu_clk_attach_to_device() adds the clock to the device so it can be
found later to allow eg: qtree to show the clock tree.

qemu_clk_bind_clock do the actual bind between the clock.

>> +
>> +/**
>> + * qemu_clk_unbound:
>> + * @out: the clock output.
>> + * @in: the clock input.
>> + *
>> + * Disconnect the clocks if they were bound together.
>> + *
>> + */
>> +void qemu_clk_unbind(qemu_clk out, qemu_clk in);
>
> Function prototype and comment don't match...
>
>> +
>> +/**
>> + * qemu_clk_update_rate:
>> + * @clk: the clock to update.
>> + * @rate: the new rate.
>> + *
>> + * Update the @clk to the new @rate.
>> + *
>> + */
>> +void qemu_clk_update_rate(qemu_clk clk, uint64_t rate);
>
> What units is this 'rate' in ?

It's Hz, will add that to the comment.
>
>> +
>> +/**
>> + * qemu_clk_refresh:
>> + * @clk: the clock to be refreshed.
>> + *
>> + * If a model alters the topology of a clock tree, it must call this function
>> + * to refresh the clock tree.
>> + *
>> + */
>> +void qemu_clk_refresh(qemu_clk clk);
>
> ...for which clock in the tree does it have to call the function?
> All of them? Any one of them at random?
Ok I will precise that.

Thanks,
Fred

>
>> +
>> +/**
>> + * qemu_clk_set_callback:
>> + * @clk: the clock where to set the callback.
>> + * @cb: the callback to associate to the callback.
>> + * @opaque: the opaque data passed to the calback.
>> + *
>> + */
>> +void qemu_clk_set_callback(qemu_clk clk,
>> +                           qemu_clk_on_rate_update_cb cb,
>> +                           void *opaque);
>> +
>>  #endif /* QEMU_CLOCK_H */
>> diff --git a/qemu-clock.c b/qemu-clock.c
>> index 0ba6caf..541f615 100644
>> --- a/qemu-clock.c
>> +++ b/qemu-clock.c
>> @@ -37,6 +37,62 @@
>>      }                                                                        \
>>  } while (0);
>>
>> +void qemu_clk_refresh(qemu_clk clk)
>> +{
>> +    qemu_clk_update_rate(clk, clk->in_rate);
>> +}
>> +
>> +void qemu_clk_update_rate(qemu_clk clk, uint64_t rate)
>> +{
>> +    ClkList *child;
>> +
>> +    clk->in_rate = rate;
>> +    clk->out_rate = rate;
>> +
>> +    if (clk->cb) {
>> +        clk->out_rate = clk->cb(clk->opaque, rate);
>> +    }
>> +
>> +    DPRINTF("%s output rate updated to %" PRIu64 "\n",
>> +            object_get_canonical_path(OBJECT(clk)),
>> +            clk->out_rate);
>> +
>> +    QLIST_FOREACH(child, &clk->bound, node) {
>> +        qemu_clk_update_rate(child->clk, clk->out_rate);
>> +    }
>> +}
>> +
>> +void qemu_clk_bind_clock(qemu_clk out, qemu_clk in)
>> +{
>> +    ClkList *child;
>> +
>> +    child = g_malloc(sizeof(child));
>> +    assert(child);
>> +    child->clk = in;
>> +    QLIST_INSERT_HEAD(&out->bound, child, node);
>> +    qemu_clk_update_rate(in, out->out_rate);
>> +}
>> +
>> +void qemu_clk_unbind(qemu_clk out, qemu_clk in)
>> +{
>> +    ClkList *child, *next;
>> +
>> +    QLIST_FOREACH_SAFE(child, &out->bound, node, next) {
>> +        if (child->clk == in) {
>> +            QLIST_REMOVE(child, node);
>> +            g_free(child);
>> +        }
>> +    }
>> +}
>> +
>> +void qemu_clk_set_callback(qemu_clk clk,
>> +                           qemu_clk_on_rate_update_cb cb,
>> +                           void *opaque)
>> +{
>> +    clk->cb = cb;
>> +    clk->opaque = opaque;
>> +}
>> +
>>  void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk,
>>                                 const char *name)
>>  {
>> --
>> 2.5.5
>>
>
>
> thanks
> -- PMM
>

Patch

diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h
index 1d56a2e..d575566 100644
--- a/include/qemu/qemu-clock.h
+++ b/include/qemu/qemu-clock.h
@@ -27,15 +27,29 @@ 
 #include "qemu/osdep.h"
 #include "qom/object.h"
 
+typedef uint64_t (*qemu_clk_on_rate_update_cb)(void *opaque, uint64_t rate);
+
 #define TYPE_CLOCK "qemu-clk"
 #define QEMU_CLOCK(obj) OBJECT_CHECK(struct qemu_clk, (obj), TYPE_CLOCK)
 
+typedef struct ClkList ClkList;
+
 typedef struct qemu_clk {
     /*< private >*/
     Object parent_obj;
     char *name;            /* name of this clock in the device. */
+    uint64_t in_rate;      /* rate of the clock which drive this pin. */
+    uint64_t out_rate;     /* rate of this clock pin. */
+    void *opaque;
+    qemu_clk_on_rate_update_cb cb;
+    QLIST_HEAD(, ClkList) bound;
 } *qemu_clk;
 
+struct ClkList {
+    qemu_clk clk;
+    QLIST_ENTRY(ClkList) node;
+};
+
 /**
  * qemu_clk_attach_to_device:
  * @dev: the device on which the clock need to be attached.
@@ -59,4 +73,56 @@  void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk,
  */
 qemu_clk qemu_clk_get_pin(DeviceState *dev, const char *name);
 
+/**
+ * qemu_clk_bind_clock:
+ * @out: the clock output.
+ * @in: the clock input.
+ *
+ * Connect the clock together. This is unidirectional so a
+ * qemu_clk_update_rate will go from @out to @in.
+ *
+ */
+void qemu_clk_bind_clock(qemu_clk out, qemu_clk in);
+
+/**
+ * qemu_clk_unbound:
+ * @out: the clock output.
+ * @in: the clock input.
+ *
+ * Disconnect the clocks if they were bound together.
+ *
+ */
+void qemu_clk_unbind(qemu_clk out, qemu_clk in);
+
+/**
+ * qemu_clk_update_rate:
+ * @clk: the clock to update.
+ * @rate: the new rate.
+ *
+ * Update the @clk to the new @rate.
+ *
+ */
+void qemu_clk_update_rate(qemu_clk clk, uint64_t rate);
+
+/**
+ * qemu_clk_refresh:
+ * @clk: the clock to be refreshed.
+ *
+ * If a model alters the topology of a clock tree, it must call this function
+ * to refresh the clock tree.
+ *
+ */
+void qemu_clk_refresh(qemu_clk clk);
+
+/**
+ * qemu_clk_set_callback:
+ * @clk: the clock where to set the callback.
+ * @cb: the callback to associate to the callback.
+ * @opaque: the opaque data passed to the calback.
+ *
+ */
+void qemu_clk_set_callback(qemu_clk clk,
+                           qemu_clk_on_rate_update_cb cb,
+                           void *opaque);
+
 #endif /* QEMU_CLOCK_H */
diff --git a/qemu-clock.c b/qemu-clock.c
index 0ba6caf..541f615 100644
--- a/qemu-clock.c
+++ b/qemu-clock.c
@@ -37,6 +37,62 @@ 
     }                                                                        \
 } while (0);
 
+void qemu_clk_refresh(qemu_clk clk)
+{
+    qemu_clk_update_rate(clk, clk->in_rate);
+}
+
+void qemu_clk_update_rate(qemu_clk clk, uint64_t rate)
+{
+    ClkList *child;
+
+    clk->in_rate = rate;
+    clk->out_rate = rate;
+
+    if (clk->cb) {
+        clk->out_rate = clk->cb(clk->opaque, rate);
+    }
+
+    DPRINTF("%s output rate updated to %" PRIu64 "\n",
+            object_get_canonical_path(OBJECT(clk)),
+            clk->out_rate);
+
+    QLIST_FOREACH(child, &clk->bound, node) {
+        qemu_clk_update_rate(child->clk, clk->out_rate);
+    }
+}
+
+void qemu_clk_bind_clock(qemu_clk out, qemu_clk in)
+{
+    ClkList *child;
+
+    child = g_malloc(sizeof(child));
+    assert(child);
+    child->clk = in;
+    QLIST_INSERT_HEAD(&out->bound, child, node);
+    qemu_clk_update_rate(in, out->out_rate);
+}
+
+void qemu_clk_unbind(qemu_clk out, qemu_clk in)
+{
+    ClkList *child, *next;
+
+    QLIST_FOREACH_SAFE(child, &out->bound, node, next) {
+        if (child->clk == in) {
+            QLIST_REMOVE(child, node);
+            g_free(child);
+        }
+    }
+}
+
+void qemu_clk_set_callback(qemu_clk clk,
+                           qemu_clk_on_rate_update_cb cb,
+                           void *opaque)
+{
+    clk->cb = cb;
+    clk->opaque = opaque;
+}
+
 void qemu_clk_attach_to_device(DeviceState *dev, qemu_clk clk,
                                const char *name)
 {