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

[Frederic Konrad]

On 02/06/2017 04:58 PM, Cédric Le Goater wrote:
> Hello,
> 
> On 01/26/2017 10:47 AM, 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>
>>
>> V1 -> V2:
>>   * Rename qemu_clk_on_rate_update_cb to QEMUClkRateUpdateCallback and
>>     move the pointer to the structure instead of having a pointer-to-function
>>     type.
>> ---
>>  include/qemu/qemu-clock.h | 67 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-clock.c              | 56 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 123 insertions(+)
>>
>> diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h
>> index 3e692d3..6d30299 100644
>> --- a/include/qemu/qemu-clock.h
>> +++ b/include/qemu/qemu-clock.h
>> @@ -30,12 +30,25 @@
>>  #define TYPE_CLOCK "qemu-clk"
>>  #define QEMU_CLOCK(obj) OBJECT_CHECK(struct qemu_clk, (obj), TYPE_CLOCK)
>>  
>> +typedef struct ClkList ClkList;
>> +typedef uint64_t QEMUClkRateUpdateCallback(void *opaque, uint64_t rate);
>> +
>>  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. */
> 
> 
> So if this is the reference clock rate, which can be divided by 
> settings in control registers, I would call the attribute 
> 'ref_rate' or 'rate_reference' 
> 
>> +    uint64_t out_rate;     /* rate of this clock pin. */
> 
> and this attribute could just be 'rate' and may be if we make 
> a property out of it, we could get rid of FixedClock in patch
> 7/10.
>    
> 
>> +    void *opaque;
>> +    QEMUClkRateUpdateCallback *cb;
> 
> If I understand correctly, the need is to be able to refresh
> the rate of a clock object depending on some settings in a 
> controller. I think we should be using a derived class and 
> an operation for it. it would look better from a QOM point 
> of view. 
> 
> Here is a possibility,
> 
> We could make 'rate' a property and use a set property handler 
> to call the derived class specific operation. This is very 
> similar to the callback but we would remove the need of  
> qemu_clk_update_rate() and use the std mechanisms already in
> place  :
> 
>       object_property_set_int() 
> 
> qemu_clk_refresh() would still be needed to propagate the 
> changes in the settings of a controller to the target clocks. 
> 
> The 'opaque' attribute, which holds the pointer to a controller 
> object, would have to be passed to the derived clock object
> with 
>       object_property_add_const_link() 
> 
> and then grabbed with 
> 
>       object_property_get_link()
> 
> in the realize function of the derived clock object, or whenever 
> it's needed, in the operation for instance.
> 
> This clearly means more code than the actual solution, but 
> that is QOM way I think.  

Yes, the big issue here is that there are several clock inputs and clock
outputs. We need to be able to bind / unbind them if there is a selector
or some such.

So a device will have more than one clock object internally in it which
are "chained" to form a clock tree.

It seems overkill to me to implement one derived object per clock
"block" in the device to get the callback.

> 
>> +    QLIST_HEAD(, ClkList) bound;
>>  } *qemu_clk;
>>  
>> +struct ClkList {
>> +    qemu_clk clk;
>> +    QLIST_ENTRY(ClkList) node;
>> +};
>> +
> 
> Do we really need this intermediate object ? Can't we just
> put the list_entry attribute under qemu_clk ? I haven't
> tried, may be I am wrong but that would simplify the code.

Yes I think it is needed. Actually I didn't find anything which
does this differently.

> 
>>  /**
>>   * qemu_clk_device_add_clock:
>>   * @dev: the device on which the clock needs to be added.
>> @@ -59,4 +72,58 @@ void qemu_clk_device_add_clock(DeviceState *dev, qemu_clk 
>> clk,
>>   */
>>  qemu_clk qemu_clk_device_get_clock(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);
> 
> maybe remove the  _clock suffix. It feels redundant.

Ok.

Thanks,
Fred

> 
> Thanks,
> 
> C. 
> 
>> +
>> +/**
>> + * qemu_clk_unbind:
>> + * @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 in Hz.
>> + *
>> + * 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 on
>> + * the clock source to refresh the clock tree.
>> + *
>> + */
>> +void qemu_clk_refresh(qemu_clk clk);
>>
>> +
>> +/**
>> + * qemu_clk_set_callback:
>> + * @clk: the clock associated to the callback.
>> + * @cb: the function which is called when a refresh happen on the clock 
>> @clk.
>> + * @opaque: the opaque data passed to the callback.
>> + *
>> + * Set the callback @cb which will be called when the clock @clk is updated.
>> + *
>> + */
>> +void qemu_clk_set_callback(qemu_clk clk,
>> +                           QEMUClkRateUpdateCallback *cb,
>> +                           void *opaque);
>> +
>>  #endif /* QEMU_CLOCK_H */
>> diff --git a/qemu-clock.c b/qemu-clock.c
>> index 803deb3..8c12368 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,
>> +                           QEMUClkRateUpdateCallback *cb,
>> +                           void *opaque)
>> +{
>> +    clk->cb = cb;
>> +    clk->opaque = opaque;
>> +}
>> +
>>  void qemu_clk_device_add_clock(DeviceState *dev, qemu_clk clk,
>>                                 const char *name)
>>  {
>>
> 
>