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

[Damien Hedde]

On 2/25/20 2:27 AM, Alistair Francis wrote:
> /On Mon, Feb 24, 2020 at 9:12 AM Damien Hedde <address@hidden> wrote
>>
>> Add functions to easily handle clocks with devices.
>> Clock inputs and outputs should be used to handle clock propagation
>> between devices.
>> The API is very similar the GPIO API.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <address@hidden>
>> ---
>>
>> I did not changed the constness of @name pointer field in
>> NamedClockList structure.
>> There is no obstacle to do it but the fact that we need to free the
>> allocated data it points to. It is possible to make it const and
>> hack/remove the const to call g_free but I don't know if its
>> allowed in qemu.
>>
>> v7:
>> + update ClockIn/Out types
>> + qdev_connect_clock_out function removed / qdev_connect_clock_in added
>>   instead
>> + qdev_pass_clock renamed to qdev_alias_clock
>> + various small fixes (typos, comment, asserts) (Peter)
>> + move device's instance_finalize code related to clock in qdev-clock.c
>> ---
>>  include/hw/qdev-clock.h | 105 +++++++++++++++++++++++++
>>  include/hw/qdev-core.h  |  12 +++
>>  hw/core/qdev-clock.c    | 169 ++++++++++++++++++++++++++++++++++++++++
>>  hw/core/qdev.c          |  12 +++
>>  hw/core/Makefile.objs   |   2 +-
>>  tests/Makefile.include  |   1 +
>>  6 files changed, 300 insertions(+), 1 deletion(-)
>>  create mode 100644 include/hw/qdev-clock.h
>>  create mode 100644 hw/core/qdev-clock.c
>>
>> diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
>> new file mode 100644
>> index 0000000000..899a95ca6a
>> --- /dev/null
>> +++ b/include/hw/qdev-clock.h
>> @@ -0,0 +1,105 @@
>> +/*
>> + * Device's clock input and output
>> + *
>> + * Copyright GreenSocs 2016-2020
>> + *
>> + * Authors:
>> + *  Frederic Konrad
>> + *  Damien Hedde
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef QDEV_CLOCK_H
>> +#define QDEV_CLOCK_H
>> +
>> +#include "hw/clock.h"
>> +
>> +/**
>> + * qdev_init_clock_in:
>> + * @dev: the device to add an input clock to
>> + * @name: the name of the clock (can't be NULL).
>> + * @callback: optional callback to be called on update or NULL.
>> + * @opaque: argument for the callback
>> + * @returns: a pointer to the newly added clock
>> + *
>> + * Add an input clock to device @dev as a clock named @name.
>> + * This adds a child<> property.
>> + * The callback will be called with @opaque as opaque parameter.
>> + */
>> +Clock *qdev_init_clock_in(DeviceState *dev, const char *name,
>> +                          ClockCallback *callback, void *opaque);
>> +
>> +/**
>> + * qdev_init_clock_out:
>> + * @dev: the device to add an output clock to
>> + * @name: the name of the clock (can't be NULL).
>> + * @callback: optional callback to be called on update or NULL.
> 
> qdev_init_clock_out() doesn't have a callback.
> 
>> + * @returns: a pointer to the newly added clock
> 
>> + *
>> + * Add an output clock to device @dev as a clock named @name.
>> + * This adds a child<> property.
>> + */
>> +Clock *qdev_init_clock_out(DeviceState *dev, const char *name);
>> +
>> +/**
>> + * qdev_get_clock_in:
>> + * @dev: the device which has the clock
>> + * @name: the name of the clock (can't be NULL).
>> + * @returns: a pointer to the clock
>> + *
>> + * Get the input clock @name from @dev or NULL if does not exist.
>> + */
>> +Clock *qdev_get_clock_in(DeviceState *dev, const char *name);
>> +
>> +/**
>> + * qdev_get_clock_out:
>> + * @dev: the device which has the clock
>> + * @name: the name of the clock (can't be NULL).
>> + * @returns: a pointer to the clock
>> + *
>> + * Get the output clock @name from @dev or NULL if does not exist.
>> + */
>> +Clock *qdev_get_clock_out(DeviceState *dev, const char *name);
>> +
>> +/**
>> + * qdev_connect_clock_in:
>> + * @dev: a device
>> + * @name: the name of an input clock in @dev
>> + * @source: the source clock (an output clock of another device for example)
>> + *
>> + * Set the source clock of input clock @name of device @dev to @source.
>> + * @source period update will be propagated to @name clock.
>> + */
>> +static inline void qdev_connect_clock_in(DeviceState *dev, const char *name,
>> +                                         Clock *source)
>> +{
>> +    clock_set_source(qdev_get_clock_in(dev, name), source);
>> +}
>> +
>> +/**
>> + * qdev_alias_clock:
>> + * @dev: the device which has the clock
>> + * @name: the name of the clock in @dev (can't be NULL)
>> + * @alias_dev: the device to add the clock
>> + * @alias_name: the name of the clock in @container
>> + * @returns: a pointer to the clock
>> + *
>> + * Add a clock @alias_name in @alias_dev which is an alias of the clock 
>> @name
>> + * in @dev. The direction _in_ or _out_ will the same as the original.
>> + * An alias clock must not be modified or used by @alias_dev and should
>> + * typically be only only for device composition purpose.
>> + */
>> +Clock *qdev_alias_clock(DeviceState *dev, const char *name,
>> +                        DeviceState *alias_dev, const char *alias_name);
>> +
>> +/**
>> + * qdev_finalize_clocklist:
>> + * @dev: the device being finalize
> 
> It probably should be:
> 
> @dev: the device being finalized
> 
>> + *
>> + * Clear the clocklist from @dev. Only used internally in qdev.
>> + */
>> +void qdev_finalize_clocklist(DeviceState *dev);
>> +
>> +#endif /* QDEV_CLOCK_H */
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 1405b8a990..d87d989e72 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -149,6 +149,17 @@ struct NamedGPIOList {
>>      QLIST_ENTRY(NamedGPIOList) node;
>>  };
>>
>> +typedef struct Clock Clock;
>> +typedef struct NamedClockList NamedClockList;
>> +
>> +struct NamedClockList {
>> +    char *name;
>> +    Clock *clock;
>> +    bool output;
>> +    bool alias;
>> +    QLIST_ENTRY(NamedClockList) node;
>> +};
>> +
>>  /**
>>   * DeviceState:
>>   * @realized: Indicates whether the device has been fully constructed.
>> @@ -171,6 +182,7 @@ struct DeviceState {
>>      bool allow_unplug_during_migration;
>>      BusState *parent_bus;
>>      QLIST_HEAD(, NamedGPIOList) gpios;
>> +    QLIST_HEAD(, NamedClockList) clocks;
>>      QLIST_HEAD(, BusState) child_bus;
>>      int num_child_bus;
>>      int instance_id_alias;
>> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
>> new file mode 100644
>> index 0000000000..9af0159517
>> --- /dev/null
>> +++ b/hw/core/qdev-clock.c
>> @@ -0,0 +1,169 @@
>> +/*
>> + * Device's clock input and output
>> + *
>> + * Copyright GreenSocs 2016-2020
>> + *
>> + * Authors:
>> + *  Frederic Konrad
>> + *  Damien Hedde
>> + *
>> + * 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"
>> +
>> +/*
>> + * qdev_init_clocklist:
>> + * Add a new clock in a device
>> + */
>> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char 
>> *name,
>> +                                           bool output, Clock *clk)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    /*
>> +     * Clock must be added before realize() so that we can compute the
>> +     * clock's canonical path durint device_realize().
> 
> s/durint/during/g
> 
>> +     */
>> +    assert(!dev->realized);
>> +
>> +    /*
>> +     * The ncl structure is freed by qdev_finalize_clocklist() which will
>> +     * be called during @dev's device_finalize().
>> +     */
>> +    ncl = g_new0(NamedClockList, 1);
>> +    ncl->name = g_strdup(name);
>> +    ncl->output = output;
>> +    ncl->alias = (clk != NULL);
>> +
>> +    /*
>> +     * Trying to create a clock whose name clashes with some other
>> +     * clock or property is a bug in the caller and we will abort().
>> +     */
>> +    if (clk == NULL) {
>> +        clk = CLOCK(object_new(TYPE_CLOCK));
>> +        object_property_add_child(OBJECT(dev), name, OBJECT(clk), 
>> &error_abort);
>> +        if (output) {
>> +            /*
>> +             * Remove object_new()'s initial reference.
>> +             * Note that for inputs, the reference created by object_new()
>> +             * will be deleted in qdev_finalize_clocklist().
>> +             */
>> +            object_unref(OBJECT(clk));
>> +        }
>> +    } else {
>> +        object_property_add_link(OBJECT(dev), name,
>> +                                 object_get_typename(OBJECT(clk)),
>> +                                 (Object **) &ncl->clock,
>> +                                 NULL, OBJ_PROP_LINK_STRONG, &error_abort);
>> +    }
>> +
>> +    ncl->clock = clk;
>> +
>> +    QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
>> +    return ncl;
>> +}
>> +
>> +void qdev_finalize_clocklist(DeviceState *dev)
>> +{
>> +    /* called by @dev's device_finalize() */
>> +    NamedClockList *ncl, *ncl_next;
>> +>> +    return NULL;
>> +}
>> +
>> +Clock *qdev_get_clock_in(DeviceState *dev, const char *name)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    assert(name);
>> +
>> +    ncl = qdev_get_clocklist(dev, name);
>> +    assert(!ncl->output);
>> +
>> +    return ncl->clock;
>> +}
>> +
>> +Clock *qdev_get_clock_out(DeviceState *dev, const char *name)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    assert(name);
>> +
>> +    ncl = qdev_get_clocklist(dev, name);
>> +    assert(ncl->output);
>> +
>> +    return ncl->clock;
>> +}
>> +
>> +Clock *qdev_alias_clock(DeviceState *dev, const char *name,
>> +                        DeviceState *alias_dev, const char *alias_name)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    assert(name && alias_name);
>> +
>> +    ncl = qdev_get_clocklist(dev, name);
>> +
>> +    qdev_init_clocklist(alias_dev, alias_name, ncl->output, ncl->clock);
>> +
>> +    return ncl->clock;
>> +}
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 3937d1eb1a..f390697348 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -37,6 +37,7 @@
>>  #include "hw/qdev-properties.h"
>>  #include "hw/boards.h"
>>  #include "hw/sysbus.h"
>> +#include "hw/qdev-clock.h"
>>  #include "migration/vmstate.h"
>>  #include "trace.h"
>>
>> @@ -855,6 +856,7 @@ static void device_set_realized(Object *obj, bool value, 
>> Error **errp)
>>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>      HotplugHandler *hotplug_ctrl;
>>      BusState *bus;
>> +    NamedClockList *ncl;
>>      Error *local_err = NULL;
>>      bool unattached_parent = false;
>>      static int unattached_count;
>> @@ -902,6 +904,13 @@ static void device_set_realized(Object *obj, bool 
>> value, Error **errp)
>>           */
>>          g_free(dev->canonical_path);
>>          dev->canonical_path = object_get_canonical_path(OBJECT(dev));
>> +        QLIST_FOREACH(ncl, &dev->clocks, node) {
>> +            if (ncl->alias) {
>> +                continue;
>> +            } else {
>> +                clock_setup_canonical_path(ncl->clock);
>> +            }
>> +        }
>>
>>          if (qdev_get_vmsd(dev)) {
>>              if (vmstate_register_with_alias_id(VMSTATE_IF(dev),
>> @@ -1025,6 +1034,7 @@ static void device_initfn(Object *obj)
>>      dev->allow_unplug_during_migration = false;
>>
>>      QLIST_INIT(&dev->gpios);
>> +    QLIST_INIT(&dev->clocks);
>>  }
>>
>>  static void device_post_init(Object *obj)
>> @@ -1054,6 +1064,8 @@ static void device_finalize(Object *obj)
>>           */
>>      }
>>
>> +    qdev_finalize_clocklist(dev);
>> +
>>      /* Only send event if the device had been completely realized */
>>      if (dev->pending_deleted_event) {
>>          g_assert(dev->canonical_path);
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index e3d796fdd4..2fdcb7dd00 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -7,7 +7,7 @@ common-obj-y += hotplug.o
>>  common-obj-y += vmstate-if.o
>>  # irq.o needed for qdev GPIO handling:
>>  common-obj-y += irq.o
>> -common-obj-y += clock.o
>> +common-obj-y += clock.o qdev-clock.o
>>
>>  common-obj-$(CONFIG_SOFTMMU) += reset.o
>>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index edcbd475aa..5a4511a86f 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -436,6 +436,7 @@ tests/test-qdev-global-props$(EXESUF): 
>> tests/test-qdev-global-props.o \
>>         hw/core/fw-path-provider.o \
>>         hw/core/reset.o \
>>         hw/core/vmstate-if.o \
>> +       hw/core/clock.o hw/core/qdev-clock.o \
>>         $(test-qapi-obj-y)
>>  tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
>>         migration/vmstate.o migration/vmstate-types.o migration/qemu-file.o \
>> --
>> 2.24.1
>>
>>
>> +    QLIST_FOREACH_SAFE(ncl, &dev->clocks, node, ncl_next) {
>> +        QLIST_REMOVE(ncl, node);
>> +        if (!ncl->output && !ncl->alias) {
>> +            /*
>> +             * We kept a reference on the input clock to ensure it lives up 
>> to
>> +             * this point so we can safely remove the callback.
>> +             * It avoids having a callback to a deleted object if ncl->clock
>> +             * is still referenced somewhere else (eg: by a clock output).
>> +             */
>> +            clock_clear_callback(ncl->clock);
>> +            object_unref(OBJECT(ncl->clock));
>> +        }
>> +        g_free(ncl->name);
>> +        g_free(ncl);
>> +    }
>> +}
>> +
>> +Clock *qdev_init_clock_out(DeviceState *dev, const char *name)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    assert(name);
>> +
>> +    ncl = qdev_init_clocklist(dev, name, true, NULL);
>> +
>> +    return ncl->clock;
>> +}
>> +
>> +Clock *qdev_init_clock_in(DeviceState *dev, const char *name,
>> +                            ClockCallback *callback, void *opaque)
>> +{
>> +    NamedClockList *ncl;
>> +
>> +    assert(name);
>> +
>> +    ncl = qdev_init_clocklist(dev, name, false, NULL);
>> +
>> +    if (callback) {
>> +        clock_set_callback(ncl->clock, callback, opaque);
>> +    }
>> +    return ncl->clock;
>> +}
>> +
>> +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;
>> +        }
>> +    }
>> +
>> +    assert(false);
> 
> Remove this.
You're right, It will fail right after anyway if we get there.

> 
> Otherwise it looks good.
> 
> Alistair
> 

Thanks,
Damien