Re: [Qemu-devel] [PATCH V1 03/10] qemu-clk: allow to bind two clocks together

[KONRAD Frederic]

Le 17/10/2016 à 20:19, Peter Maydell a écrit :
> On 5 October 2016 at 23:10,  <address@hidden> wrote:
> > From: KONRAD Frederic <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >  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