Re: [PATCH v6 3/9] qdev: add clock input&output support to devices.

[Damien Hedde]

On 11/25/19 2:30 PM, Philippe Mathieu-Daudé wrote:
> Nitpick: remove trailing dot in patch subject
> 
> On 9/4/19 2:55 PM, Damien Hedde wrote:
>> Add functions to easily add input or output clocks to a device.
>> A clock objects is added as a child of the device.
> 
> <newline>?
> 
>> The api is very similar the gpio's one.
> 
> Maybe "This API is very similar to the QDEV GPIO API."
> 
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <address@hidden>
>>
>> ---
>> I've removed the reviewed-by/tested-by of Philippe because I did a small
>> modification.
>>
>> qdev_connect_clock() which allowed to connect an input to an output is
>> now split in 2:
>> + qdev_get_clock_in() which gets a given input from a device
>> + qdev_connect_clock_out() which connect a given output to a clock
>> (previously fetched by qdev_get_clock_in())
>> This part is located in (qdev-clock.[c|h]).
>> It better matches gpios api and also add the possibility to connect a
>> device's input clock to a random output clock (used in patch 9).
>>
>> Also add missing qdev-clock in the test-qdev-global-props so that tests
>> pass.
>> ---
>>   hw/core/Makefile.objs   |   2 +-
>>   hw/core/qdev-clock.c    | 155 ++++++++++++++++++++++++++++++++++++++++
>>   hw/core/qdev.c          |  32 +++++++++
>>   include/hw/qdev-clock.h |  67 +++++++++++++++++
>>   include/hw/qdev-core.h  |  14 ++++
>>   tests/Makefile.include  |   1 +
> 
> Please setup the scripts/git.orderfile to ease reviews.
> 
>>   6 files changed, 270 insertions(+), 1 deletion(-)
>>   create mode 100644 hw/core/qdev-clock.c
>>   create mode 100644 include/hw/qdev-clock.h
>>
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index 8fcebf2e67..4523d3b5c7 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -1,5 +1,5 @@
>>   # core qdev-related obj files, also used by *-user:
>> -common-obj-y += qdev.o qdev-properties.o
>> +common-obj-y += qdev.o qdev-properties.o qdev-clock.o
>>   common-obj-y += bus.o reset.o
>>   common-obj-y += resettable.o
>>   common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
>> new file mode 100644
>> index 0000000000..bebdd8fa15
>> --- /dev/null
>> +++ b/hw/core/qdev-clock.c
>> @@ -0,0 +1,155 @@
>> +/*
>> + * Device's clock
>> + *
>> + * Copyright GreenSocs 2016-2018
> 
> 2019
> 
>> + *
>> + * Authors:
>> + *  Frederic Konrad <address@hidden>
>> + *  Damien Hedde <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/qdev-clock.h"
>> +#include "hw/qdev-core.h"
>> +#include "qapi/error.h"
>> +
>> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const
>> char *name,
>> +        bool forward)
> 
> Indentation off.
> 
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    /*
>> +     * The clock path will be computed by the device's realize
>> function call.
>> +     * This is required to ensure the clock's canonical path is right
>> and log
>> +     * messages are meaningfull.
>> +     */
>> +    assert(name);
>> +    assert(!dev->realized);
>> +
>> +    /* The ncl structure will be freed in device's finalize function
>> call */
>> +    ncl = g_malloc0(sizeof(*ncl));
> 
> Similar but easier to review:
> 
>        ncl = g_new0(NamedClockList, 1)
> 
>> +    ncl->name = g_strdup(name);
>> +    ncl->forward = forward;
>> +
>> +    QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
>> +    return ncl;
>> +}
>> +
>> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name)
>> +{
>> +    NamedClockList *ncl;
>> +    Object *clk;
>> +
>> +    ncl = qdev_init_clocklist(dev, name, false);
>> +
>> +    clk = object_new(TYPE_CLOCK_OUT);
>> +
>> +    /* will fail if name already exists */
>> +    object_property_add_child(OBJECT(dev), name, clk, &error_abort);
>> +    object_unref(clk); /* remove the initial ref made by object_new */
>> +
>> +    ncl->out = CLOCK_OUT(clk);
>> +    return ncl->out;
>> +}
>> +
>> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
>> +                        ClockCallback *callback, void *opaque)
> 
> Indentation off.
> 
>> +{
>> +    NamedClockList *ncl;
>> +    Object *clk;
>> +
>> +    ncl = qdev_init_clocklist(dev, name, false);
>> +
>> +    clk = object_new(TYPE_CLOCK_IN);
>> +    /*
>> +     * the ref initialized by object_new will be cleared during dev
>> finalize.
>> +     * It allows us to safely remove the callback.
>> +     */
>> +
>> +    /* will fail if name already exists */
>> +    object_property_add_child(OBJECT(dev), name, clk, &error_abort);
>> +
>> +    ncl->in = CLOCK_IN(clk);
>> +    if (callback) {
>> +        clock_set_callback(ncl->in, callback, opaque);
>> +    }
>> +    return ncl->in;
>> +}
>> +
>> +static NamedClockList *qdev_get_clocklist(DeviceState *dev, const
>> char *name)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    QLIST_FOREACH(ncl, &dev->clocks, node) {
>> +        if (strcmp(name, ncl->name) == 0) {
>> +            return ncl;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +void qdev_pass_clock(DeviceState *dev, const char *name,
>> +                     DeviceState *container, const char *cont_name)
>> +{
>> +    NamedClockList *original_ncl, *ncl;
>> +    Object **clk;
> 
> Is it really a Object** or a Object*?

An Object** because it tells where the Object* is stored for the link
property below.

> 
>> +
>> +    assert(container && cont_name);
>> +
>> +    original_ncl = qdev_get_clocklist(container, cont_name);
>> +    assert(original_ncl); /* clock must exist in origin */
>> +
>> +    ncl = qdev_init_clocklist(dev, name, true);
>> +
>> +    if (ncl->out) {
>> +        clk = (Object **)&ncl->out;
>> +    } else {
>> +        clk = (Object **)&ncl->in;
>> +    }
>> +
>> +    /* will fail if name already exists */
>> +    object_property_add_link(OBJECT(dev), name,
>> object_get_typename(*clk),
>> +                             clk, NULL, OBJ_PROP_LINK_STRONG,
>> &error_abort);
>> +}
>> +

--
Damien