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 <damien.he...@greensocs.com>
>>
>> ---
>> 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 <fred.kon...@greensocs.com>
>> + * Damien Hedde <damien.he...@greensocs.com>
>> + *
>> + * 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
